[Patch] USB UHCI portstatus correction

[Patch] USB UHCI portstatus correction

[Aleš Nesrsta]

[-- Attachment #1: Type: text/plain, Size: 263 bytes --]

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...

Best regards
Ales

[-- Attachment #2: usb_patch_uhci_100620_0 --]
[-- Type: text/x-patch, Size: 1375 bytes --]

diff -urB ./usb/bus/usb/uhci.c ./usb_patched/bus/usb/uhci.c
--- ./usb/bus/usb/uhci.c	2010-06-20 10:20:58.000000000 +0200
+++ ./usb_patched/bus/usb/uhci.c	2010-06-20 10:59:03.000000000 +0200
@@ -612,8 +612,23 @@
   status = grub_uhci_readreg16 (u, reg);
   grub_dprintf ("uhci", "detect=0x%02x\n", status);
 
+  if (!enable) /* We don't need reset port */
+    {
+      /* Disable the port.  */
+      grub_uhci_writereg16 (u, reg, 0 << 2);
+      grub_dprintf ("uhci", "waiting for the port to be disabled\n");
+      endtime = grub_get_time_ms () + 1000;
+      while ((grub_uhci_readreg16 (u, reg) & (1 << 2)))
+        if (grub_get_time_ms () > endtime)
+          return grub_error (GRUB_ERR_IO, "UHCI Timed out");
+
+      status = grub_uhci_readreg16 (u, reg);
+      grub_dprintf ("uhci", ">3detect=0x%02x\n", status);
+      return GRUB_ERR_NONE;
+    }
+    
   /* Reset the port.  */
-  grub_uhci_writereg16 (u, reg, enable << 9);
+  grub_uhci_writereg16 (u, reg, 1 << 9);
 
   /* Wait for the reset to complete.  XXX: How long exactly?  */
   grub_millisleep (50); /* For root hub should be nominaly 50ms */
@@ -623,7 +638,7 @@
   grub_millisleep (10);
 
   /* Enable the port.  */
-  grub_uhci_writereg16 (u, reg, enable << 2);
+  grub_uhci_writereg16 (u, reg, 1 << 2);
   grub_millisleep (10);
 
   grub_dprintf ("uhci", "waiting for the port to be enabled\n");

Re: [Patch] USB UHCI portstatus correction

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 750 bytes --]

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.
> Best regards
> Ales
>   
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>   


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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: [Patch] USB UHCI portstatus correction

[Aleš Nesrsta]

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.

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.

Best regards
Ales

> > Best regards
> > Ales
> >   
> >
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > http://lists.gnu.org/mailman/listinfo/grub-devel
> >   
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [Patch] USB UHCI portstatus correction

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 3064 bytes --]

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
>>> Grub-devel@gnu.org
>>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>>   
>>>       
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>     
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>   


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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: [Patch] USB UHCI portstatus correction

[Aleš Nesrsta]

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

> 
> > 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
> >>> Grub-devel@gnu.org
> >>> http://lists.gnu.org/mailman/listinfo/grub-devel
> >>>   
> >>>       
> >>
> >> _______________________________________________
> >> Grub-devel mailing list
> >> Grub-devel@gnu.org
> >> http://lists.gnu.org/mailman/listinfo/grub-devel
> >>     
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > http://lists.gnu.org/mailman/listinfo/grub-devel
> >
> >   
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [Patch] USB UHCI portstatus correction

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 4230 bytes --]

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
>>>>> Grub-devel@gnu.org
>>>>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>>>>   
>>>>>       
>>>>>           
>>>> _______________________________________________
>>>> Grub-devel mailing list
>>>> Grub-devel@gnu.org
>>>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>>>     
>>>>         
>>> _______________________________________________
>>> Grub-devel mailing list
>>> Grub-devel@gnu.org
>>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>>
>>>   
>>>       
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>     
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>   


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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: [Patch] USB UHCI portstatus correction

[Aleš Nesrsta]

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...


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
> >>>>> Grub-devel@gnu.org
> >>>>> http://lists.gnu.org/mailman/listinfo/grub-devel
> >>>>>   
> >>>>>       
> >>>>>           
> >>>> _______________________________________________
> >>>> Grub-devel mailing list
> >>>> Grub-devel@gnu.org
> >>>> http://lists.gnu.org/mailman/listinfo/grub-devel
> >>>>     
> >>>>         
> >>> _______________________________________________
> >>> Grub-devel mailing list
> >>> Grub-devel@gnu.org
> >>> http://lists.gnu.org/mailman/listinfo/grub-devel
> >>>
> >>>   
> >>>       
> >>
> >> _______________________________________________
> >> Grub-devel mailing list
> >> Grub-devel@gnu.org
> >> http://lists.gnu.org/mailman/listinfo/grub-devel
> >>     
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > http://lists.gnu.org/mailman/listinfo/grub-devel
> >
> >   
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [Patch] USB UHCI portstatus correction

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 8453 bytes --]

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
>>>>>>> Grub-devel@gnu.org
>>>>>>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>>>>>>   
>>>>>>>       
>>>>>>>           
>>>>>>>               
>>>>>> _______________________________________________
>>>>>> Grub-devel mailing list
>>>>>> Grub-devel@gnu.org
>>>>>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>>>>>     
>>>>>>         
>>>>>>             
>>>>> _______________________________________________
>>>>> Grub-devel mailing list
>>>>> Grub-devel@gnu.org
>>>>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>>>>
>>>>>   
>>>>>       
>>>>>           
>>>> _______________________________________________
>>>> Grub-devel mailing list
>>>> Grub-devel@gnu.org
>>>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>>>     
>>>>         
>>> _______________________________________________
>>> Grub-devel mailing list
>>> Grub-devel@gnu.org
>>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>>
>>>   
>>>       
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>     
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>   


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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]