Re: [Patch] USB UHCI portstatus correction

[Vladimir 'φ-coder/phcoder' Serbinenko]

On 06/26/2010 12:03 AM, Aleš Nesrsta wrote:
> Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>   
>> On 06/20/2010 11:23 AM, Aleš Nesrsta wrote:
>>     
>>> Hi,
>>>
>>> I found some mistake in uhci.c in grub_uhci_portstatus when enable=0.
>>> There is proposal of correction.
>>>
>>> Without correction portstatus reported false timeout when enable=0
>>> because it is waiting for reset to be done but none is performed...
>>>
>>>   
>>>       
>> This patch seems to change much more that you say. In particular
>> enable=0 is a request to disable port and you seem to always enable it.
>> This is likely to break other code.
>>     
> Hi,
> You are right according to some possible side-effects. But the lines
> ...
> if (!enable) /* We don't need reset port */
>     {
>       /* Disable the port.  */
>       grub_uhci_writereg16 (u, reg, 0 << 2);
> ...
> should disable the port as the bit "Port Enable" is reset.
>
> Port reset should be not necessary when disabling port - according to
> USB specification, reset of port should be done only to enable port and
> mainly to bring newly connected device to "Default" state (i.e. to state
> when device accepts communication via address 0).
>
> Of course:
> - I can be wrong
> - it should be tested according to some side-effect
>
> But in original code is real bug - if this function is called with
> enable=0, it reports false timeout as it is waiting for bit which will
> never set in this case.
> This bug should be corrected in some way.
>
>   
I have nothing against that change. I was mainly referring to:

-  grub_uhci_writereg16 (u, reg, enable << 9);
+  grub_uhci_writereg16 (u, reg, 1 << 9);
Which seems to always enable the port.


> There is also question, why does the function attach_root_port (in
> usbhub.c) disable and enable of port before initialize connected
> device ?
> Reset & enable of port (if new device is connected) should be enough,
> because, according to USB specification:
> - hub should automatically disable the port if device is disconnected or
> port is not powered
> - ports should be disabled by hub after power-up of hub
> But maybe there are some special cases or buggy hubs/devices which needs
> such behavior (?) - I don't know, so I didn't touch this part of code.
>
>   
It's the right strategy: if it doesn't bug and unlikely to, leave it alone.
> Best regards
> Ales
>
>   
>>> Best regards
>>> Ales
>>>   
>>>
>>>
>>> _______________________________________________
>>> Grub-devel mailing list
>>> address@hidden
>>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>>   
>>>       
>>
>> _______________________________________________
>> Grub-devel mailing list
>> address@hidden
>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>     
>
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>   


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko

signature.asc
Description: OpenPGP digital signature