Re: [Patch] USB UHCI portstatus correction

[Vladimir 'φ-coder/phcoder' Serbinenko]

On 07/05/2010 07:12 PM, Aleš Nesrsta wrote:
> Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>   
>> 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.
>>
>>     
> OK, I committed it into trunk as rev. 2522.
> Regards, Ales
>
>   
I'm about to revert your patch. I never approved the patch as whole. I
think you misunderstood me. When I approve patch I explicitly say "Go
ahead for mainline" or "Go ahead for experimental". In this case I
explicitly don't understand why you change (enable << 9) to (1 << 9).
Could you explain this?
>>     
>>> 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
>>>
>>>   
>>>       
>>
>> _______________________________________________
>> 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