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