From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with archive (Exim 4.43) id 1OYcID-0007xY-VR for mharc-grub-devel@gnu.org; Tue, 13 Jul 2010 06:00:50 -0400 Received: from [140.186.70.92] (port=49856 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OYcIA-0007vy-ET for grub-devel@gnu.org; Tue, 13 Jul 2010 06:00:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OYcI8-0007dR-P6 for grub-devel@gnu.org; Tue, 13 Jul 2010 06:00:46 -0400 Received: from mail-bw0-f41.google.com ([209.85.214.41]:55653) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OYcI8-0007d9-FX for grub-devel@gnu.org; Tue, 13 Jul 2010 06:00:44 -0400 Received: by bwz9 with SMTP id 9so3797228bwz.0 for ; Tue, 13 Jul 2010 03:00:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:message-id:date:from :user-agent:mime-version:to:subject:references:in-reply-to :x-enigmail-version:content-type; bh=TynKVZVOECtPNrJw4yttXNfR11gzE4oZkyZWYitXjm0=; b=tAYF2S6JlKHJ5GwDHxdslKG5YavSSnYIIrGQF/6kP0ZuL+gVSYPSzXghKWKiWKzOE/ PgrwqXgHe1VZ5k2xCwJ367HHZO2C+HPzxDHeZ0gXMB8tf6Dih8XF8l3f0mReCgh4+MRp h/c4Zjayzk3HLP939WwEg8yrTI2yA4dcbd3nY= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=message-id:date:from:user-agent:mime-version:to:subject:references :in-reply-to:x-enigmail-version:content-type; b=V/tGxctN4QARvTwiZF4coH0+PCnyGf9IokMhPy8/4SfjeqIsZGXi1EEaBkRkqTjHWK 6X5a3WR141KdfinOibfbW/++zep0gPfiGAI9WnFzrB33DROKFm/Nt19zjIiQlJFhxXkd QIh0SUqCoEPBXPreldh0ep715QYkNU87yqIYE= Received: by 10.204.160.90 with SMTP id m26mr1217875bkx.45.1279015242509; Tue, 13 Jul 2010 03:00:42 -0700 (PDT) Received: from debian.bg45.phnet (vpn-global-158-dhcp.ethz.ch [129.132.211.158]) by mx.google.com with ESMTPS id s17sm22998266bkx.6.2010.07.13.03.00.40 (version=TLSv1/SSLv3 cipher=RC4-MD5); Tue, 13 Jul 2010 03:00:40 -0700 (PDT) Message-ID: <4C3C3946.20906@gmail.com> Date: Tue, 13 Jul 2010 12:00:38 +0200 From: =?UTF-8?B?VmxhZGltaXIgJ8+GLWNvZGVyL3BoY29kZXInIFNlcmJpbmVua28=?= User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.10) Gecko/20100620 Icedove/3.0.5 MIME-Version: 1.0 To: grub-devel@gnu.org References: <1277025782.4952.6.camel@pracovna> <4C250757.9030207@gmail.com> <1277503421.4714.39.camel@pracovna> <4C28D240.3000401@gmail.com> <1278349926.4721.20.camel@pracovna> <4C327163.5000308@gmail.com> <1278436741.4618.58.camel@pracovna> In-Reply-To: <1278436741.4618.58.camel@pracovna> X-Enigmail-Version: 1.0.1 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="------------enig6A6358C90A6EAEFDF0C80D9B" X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 2) Subject: Re: [Patch] USB UHCI portstatus correction X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 13 Jul 2010 10:00:48 -0000 This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig6A6358C90A6EAEFDF0C80D9B Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 07/06/2010 07:19 PM, Ale=C5=A1 Nesrsta wrote: > Vladimir '=CF=86-coder/phcoder' Serbinenko : > =20 >> On 07/05/2010 07:12 PM, Ale=C5=A1 Nesrsta wrote: >> =20 >>> Vladimir '=CF=86-coder/phcoder' Serbinenko wrote: >>> =20 >>> =20 >>>> On 06/26/2010 12:03 AM, Ale=C5=A1 Nesrsta wrote: >>>> =20 >>>> =20 >>>>> Vladimir '=CF=86-coder/phcoder' Serbinenko wrote: >>>>> =20 >>>>> =20 >>>>> =20 >>>>>> On 06/20/2010 11:23 AM, Ale=C5=A1 Nesrsta wrote: >>>>>> =20 >>>>>> =20 >>>>>> =20 >>>>>>> Hi, >>>>>>> >>>>>>> I found some mistake in uhci.c in grub_uhci_portstatus when enabl= e=3D0. >>>>>>> There is proposal of correction. >>>>>>> >>>>>>> Without correction portstatus reported false timeout when enable=3D= 0 >>>>>>> because it is waiting for reset to be done but none is performed.= =2E. >>>>>>> >>>>>>> =20 >>>>>>> =20 >>>>>>> =20 >>>>>>> =20 >>>>>> This patch seems to change much more that you say. In particular >>>>>> enable=3D0 is a request to disable port and you seem to always ena= ble it. >>>>>> This is likely to break other code. >>>>>> =20 >>>>>> =20 >>>>>> =20 >>>>> Hi, >>>>> You are right according to some possible side-effects. But the line= s >>>>> ... >>>>> 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=3D0, 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. >>>>> >>>>> =20 >>>>> =20 >>>>> =20 >>>> 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. >>>> >>>> =20 >>>> =20 >>> OK, I committed it into trunk as rev. 2522. >>> Regards, Ales >>> >>> =20 >>> =20 >> 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? >> =20 > 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..) alway= s > TRUE, so why to use "enable" instead of 1...? It can be confusing... > > =20 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=3DFALSE. > What is maybe interesting, in "trunk" this function is never called wit= h > enable=3DFALSE, it is called only from this part of usbhub.c > (grub_usb_root_hub): > ... > for (i =3D 0; i < ports; i++) > { > grub_usb_speed_t speed =3D controller->dev->detect_dev (controlle= r, > i); > > if (speed !=3D GRUB_USB_SPEED_NONE) > { > grub_usb_device_t dev; > > /* Enable the port. */ > err =3D controller->dev->portstatus (controller, i, 1); > if (err) > continue; > > /* Enable the port and create a device. */ > dev =3D 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 =3D=3D 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 =3D controller->dev->portstatus (controller, portno, 0); > if (err) > return; > > /* Enable the port. */ > err =3D controller->dev->portstatus (controller, portno, 1); > if (err) > return; > > /* Enable the port and create a device. */ > dev =3D 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 > > =20 >>>> =20 >>>> =20 >>>>> 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 enoug= h, >>>>> because, according to USB specification: >>>>> - hub should automatically disable the port if device is disconnect= ed 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 co= de. >>>>> >>>>> =20 >>>>> =20 >>>>> =20 >>>> It's the right strategy: if it doesn't bug and unlikely to, leave it= alone. >>>> =20 >>>> =20 >>>>> Best regards >>>>> Ales >>>>> >>>>> =20 >>>>> =20 >>>>> =20 >>>>>>> Best regards >>>>>>> Ales >>>>>>> =20 >>>>>>> >>>>>>> >>>>>>> _______________________________________________ >>>>>>> Grub-devel mailing list >>>>>>> Grub-devel@gnu.org >>>>>>> http://lists.gnu.org/mailman/listinfo/grub-devel >>>>>>> =20 >>>>>>> =20 >>>>>>> =20 >>>>>>> =20 >>>>>> _______________________________________________ >>>>>> Grub-devel mailing list >>>>>> Grub-devel@gnu.org >>>>>> http://lists.gnu.org/mailman/listinfo/grub-devel >>>>>> =20 >>>>>> =20 >>>>>> =20 >>>>> _______________________________________________ >>>>> Grub-devel mailing list >>>>> Grub-devel@gnu.org >>>>> http://lists.gnu.org/mailman/listinfo/grub-devel >>>>> >>>>> =20 >>>>> =20 >>>>> =20 >>>> _______________________________________________ >>>> Grub-devel mailing list >>>> Grub-devel@gnu.org >>>> http://lists.gnu.org/mailman/listinfo/grub-devel >>>> =20 >>>> =20 >>> _______________________________________________ >>> Grub-devel mailing list >>> Grub-devel@gnu.org >>> http://lists.gnu.org/mailman/listinfo/grub-devel >>> >>> =20 >>> =20 >> >> _______________________________________________ >> Grub-devel mailing list >> Grub-devel@gnu.org >> http://lists.gnu.org/mailman/listinfo/grub-devel >> =20 > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > http://lists.gnu.org/mailman/listinfo/grub-devel > > =20 --=20 Regards Vladimir '=CF=86-coder/phcoder' Serbinenko --------------enig6A6358C90A6EAEFDF0C80D9B Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iF4EAREKAAYFAkw8OUYACgkQNak7dOguQgn7sgEAkcuy7FEca+KT3B5d9RPkq3rc 0TsdR6kphaZIDz1LbEsA/RCeVMeejRMbbjwyJN5JltC7TCLpY3IRTQWkFBj3CtVu =DCub -----END PGP SIGNATURE----- --------------enig6A6358C90A6EAEFDF0C80D9B--