[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Patch] USB UHCI portstatus correction
From: |
Vladimir 'φ-coder/phcoder' Serbinenko |
Subject: |
Re: [Patch] USB UHCI portstatus correction |
Date: |
Tue, 06 Jul 2010 01:57:23 +0200 |
User-agent: |
Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.10) Gecko/20100620 Icedove/3.0.5 |
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