Re: [PATCH] Re: [grub-devel] loongson-2f mini-pc (fuloong) elf image generation.

[Vladimir 'φ-coder/phcoder' Serbinenko]

Go ahead
On 18.07.2013 18:10, Aleš Nesrsta wrote:
> Hi,
> 
> after some debugging I have found bug(s) in handling of QHs related to
> EHCI low speed split interrupt transfers.
> 
> Attached patch seems to solve problem described below (non-working USB
> keyboard attached to the same port where was USB disk previously).
> 
> Please try it - maybe it solves also reported keyboard problem on
> fuloong/loongson.
> 
> BR,
> Ales
> 
>> I forgot one more thing - if You want to try repeat USBMS/USB keyboard
>> problem described below, You need to access the connected USB disk (e.g.
>> by "ls" command) before removing it.
>>
>>> Hi Vladimir,
>>>
>>> I have some additional information - results of some today's
>>> experiments.
>>> The main important thing related to USB keyboard:
>>>
>>> USB keyboard doesn't work if it is connected as first device to the same
>>> non-root hub port, where was connected USBMS device previously!
>>> If the keyboard is disconnected and connected again to the same port, it
>>> works.
>>> this behavior is systematical, it can be repeated at any time (at least
>>> on my PC).
>>>
>>> What is very interesting, USB keyboard is listed by command "usb".
>>> And it doesn't matter if previously connected USBMS device was HIGH of
>>> FULL device.
>>>
>>> I.e., according these test results, even if the USB keybord is not
>>> working, it is connected to port and addressed properly and its control
>>> pipe is working, but device itself is not working - maybe there is
>>> something wrong in "high level" driver (usb_keyboard) - ?
>>>
>>>
>>> There is also another bad thing - after some number of
>>> connecting/disconnecting of the keyboard (at least about 20 and more)
>>> the GRUB crashes - more properly, it reboots PC, i.e. there is probably
>>> also some memory leak...
>>>
>>> BR,
>>> Ales
>>>
>>>>
>>>> 1.
>>>> Some of my USBMS devices have problems to work properly. It seems to be
>>>> some regression because they worked well on some older revisions... :-(
>>>> I did not make any investigation it this direction yet, as this problem
>>>> is probably not related to latest changes (fix of root ports) - so I
>>>> ignore them for now.
>>>>
>>>> 2.
>>>> Sometimes some devices are not recognized (not working) in the case
>>>> when
>>>> they are connected before USB "starting" time (before the moment when
>>>> USB modules/drivers are loaded) or when hub with this device is
>>>> connected.
>>>> Additionally, it seems to happen only if device is connected via
>>>> hub(s),
>>>> not directly into root port - at least it was behavior during my tests
>>>> (but I did only few tests on root ports, I focused my tests to USB
>>>> keyboard connected via hub(s) etc.).
>>>> It looks like, in some rare cases, usbhub.c maybe miss some non-root
>>>> hub
>>>> port change(s). Unfortunately I had no enough time to try debug these
>>>> situations.
>>>>
>>>> So I went through Your changes in usbhub.c and other USB related files.
>>>> Unfortunately, I did not found reason of the problem mentioned above in
>>>> point 2. yet.
>>>> But I found some another points to discuss:
>>>>
>>>> a)
>>>> I found my old mistake related to variable "pending_reset".
>>>> Meaning of this variable is to avoid concurrent reset on devices which
>>>> are connected to the same controller HW instance.
>>>> This variable is stored in wrong place - it should be located not in
>>>> "struct grub_usb_controller_dev" but in "struct grub_*hci" (i.e.
>>>> grub_uhci, grub_ohci etc.).
>>>> In fact, its current location is not totally bad - it is also working,
>>>> but it can slow down handling of USB devices (mainly in USB "starting"
>>>> phase) in case when there are more controllers of the same type.
>>>>
>>>>
>>>> b)
>>>> There is missing waiting for device stable power in case when device is
>>>> connected to ROOT hub later than in USB "starting" time.
>>>> This could possibly lead to wrong device reset and malfunction.
>>>>
>>>> I.e. the "first half" of "grub_usb_poll_devices" should be little bit
>>>> changed, it is not correct to call "attach_root_port" immediately when
>>>> "detect_dev" detected device connection - it should be done e.g. in
>>>> similar way as in "grub_usb_controller_dev_register" (or maybe
>>>> better in
>>>> some another, "background" way, like You did for non-root hub - to
>>>> prevent unwanted delay in execution of another GRUB parts).
>>>>
>>>>
>>>> c)
>>>> I thought about this old code:
>>>>
>>>> "...
>>>> poll_nonroot_hub (grub_usb_device_t dev)
>>>> {
>>>>    grub_usb_err_t err;
>>>>    unsigned i;
>>>>    grub_uint8_t changed;
>>>>    grub_size_t actual, len;
>>>>
>>>>    if (!dev->hub_transfer)
>>>>      return;
>>>> ..."
>>>>
>>>> I think, as the possible "error recovery", the better than current
>>>> immediate return could be to try to call
>>>> "grub_usb_bulk_read_background"
>>>> to schedule new background transfer for this hub before return - ?
>>>>
>>>>
>>>> d)
>>>> Cosmetic thing:
>>>> It will be fine to rename declaration
>>>> static struct grub_usb_hub *hubs;
>>>> to
>>>> static struct grub_usb_hub *root_hubs;
>>>> to be more self explanative... :-)
>>>>
>>>>
>>>> What do You think about the points a)-d) ?
>>>>
>>>> BR,
>>>> Ales
>>>>
>>>> _______________________________________________
>>>> Grub-devel mailing list
>>>> address@hidden
>>>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>>>
>>>
>>> _______________________________________________
>>> Grub-devel mailing list
>>> address@hidden
>>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>>
>>
>> _______________________________________________
>> Grub-devel mailing list
>> address@hidden
>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>
> 
> 
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

signature.asc
Description: OpenPGP digital signature