Re: [Patch] USB UHCI portstatus correction

[Vladimir 'φ-coder/phcoder' Serbinenko]

On 07/06/2010 07:19 PM, Aleš Nesrsta wrote:
> 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?
>>     
> Sorry about misunderstanding.
>
> No problem to explain it - simply, this part code was changed only to
> prevent misunderstanding... :-)
>
>
> If "enable" is FALSE, then no reset of USB port is requested/necessary
> and code goes via this way:
> ...
> if (!enable) /* We don't need reset port */
>     {
>       /* Disable the port.  */
>  ...
>      return GRUB_ERR_NONE;
>     }
> I.e., Port Enable bit is cleared (and all another bits except WRC
> bits... - it should be not relevant in case of port disabling).
>
> If "enable" is TRUE, it is necessary to do port reset, i.e. it is
> executed this code:
> ...
>   /* Reset the port.  */
>   grub_uhci_writereg16 (u, reg, 1 << 9);
> ...
>   return GRUB_ERR_NONE;
> }
> But when this part of code is executed, "enable" is (should be..) always
> TRUE, so why to use "enable" instead of 1...? It can be confusing...
>
>   
Oh, I missed the return statement. My bad. Now I have no problem with
the patch going into usb branch. I'll remerge usb branch into trunk
soon. Once I test it on Yeeloong
> Note 1:
> In both cases (set or reset of Port Enable bit) is made some waiting -
> it is because specification says: "Note that the bit status does not
> change until the port state actually changes and that there may be a
> delay in disabling or enabling a port if there is a transaction
> currently in progress on the USB".
>
>
> Note 2:
> As I wrote, this patch corrects the problem when function
> grub_uhci_portstatus is called with enable=FALSE.
> What is maybe interesting, in "trunk" this function is never called with
> enable=FALSE, it is called only from this part of usbhub.c
> (grub_usb_root_hub):
> ...
>  for (i = 0; i < ports; i++)
>     {
>       grub_usb_speed_t speed = controller->dev->detect_dev (controller,
> i);
>
>       if (speed != GRUB_USB_SPEED_NONE)
>       {
>         grub_usb_device_t dev;
>
>         /* Enable the port.  */
>         err = controller->dev->portstatus (controller, i, 1);
>         if (err)
>           continue;
>
>         /* Enable the port and create a device.  */
>         dev = grub_usb_hub_add_dev (controller, speed);
>         if (! dev)
>           continue;
>
>         /* If the device is a Hub, scan it for more devices.  */
>         if (dev->descdev.class == 0x09)
>           grub_usb_add_hub (dev);
>       }
>     }
> ...
>
> In "usb" branch looks the same function differently:
> It calls new function "attach_root_port" which contains:
> ...
>   /* Disable the port. XXX: Why? */
>   err = controller->dev->portstatus (controller, portno, 0);
>   if (err)
>     return;
>
>   /* Enable the port.  */
>   err = controller->dev->portstatus (controller, portno, 1);
>   if (err)
>     return;
>
>   /* Enable the port and create a device.  */
>   dev = grub_usb_hub_add_dev (controller, speed);
>   if (! dev)
>     return;
> ...
> I.e. this functions first disables the port (I don't know why - as I
> wrote in text below from previous e-mail... - could it be some unwanted
> rest of some experiments?) and this causes timeout problem which is
> corrected in my patch.
>
>
> Feel free to revert this patch or modify it as necessary...
>
> Regards
> Ales
>
>   
>>>>     
>>>>         
>>>>> 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
>>>
>>>   
>>>       
>>
>> _______________________________________________
>> 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