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