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