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