Grub needs to check the programming interface for usb controllers

Grub needs to check the programming interface for usb controllers

[Oliver Henshaw]

Grub needs to check that the usb programming interface is of the
correct type. Otherwise, hilarity ensues when e.g. bus/usb/ohci.c
attempts to initialise an uhci controller. This can easily be
reproduced by typing 'insmod ohci' in the grub rescue disk in qemu,
with usb enabled.

Additionally, the usb interface (which was defined but not used in
bus/usb/ohci.c) was set to the wrong value.

I've split this into a patch series bookended by more cosmetic
patches. The first patch consists of minor fixes: removing some
doubled lines (which look like a pasto, but I could be wrong),
changing an int to a grub_uint32_t (to match the types of all other
similar variables) and I've added some whitespace fixes. The final
patch is purely cosmetic: it defines the pci Class, Subclass and
Programming Interface fields in terms of the Class Code register, so
that the code better reflects the spec.

This last patch may seem minor, but the (previously unused) usb
interface was set to the wrong value in bus/usb/ohci.c up to now -
this means that the code now better reflects the structure of the
Class Code register, and better documents the specification.

The patch series has been tested on on real hardware, and each patch
has been tested with qemu and a grub rescue image. Both ohci and uhci
cases have been tested (qemu has a virtual ohci controller, but the
option to enable it must be patched in).


Thanks,
Oliver

[PATCH 0/4] Re: Grub needs to check the programming interface for usb controllers

[oliver.henshaw]

Here's a second try. I used quilt to manage the patch series but mailed them by hand instead of exporting to a mailbox, and didn't realise that they weren't named as *.patch (otherwise I think the content type would have been fine). I should probably move on to git, but I was a little intimidated by the idea of re-writing history - manipulating a patch series seemed more natural to me. But onwards and upwards.

I've removed all changes to trailing whitespace, as requested. There is still a whitespace change in side a comment, something I noticed with syntax highlighting.


Thanks,
Oliver

[PATCH 1/4] Minor Cleanup

[oliver.henshaw]

[-- Attachment #1: usb-minor-cleanup --]
[-- Type: text/plain, Size: 1819 bytes --]

Changelog:
	* bus/usb/uhci.c: Remove un-needed doubled lines.
	* bus/usb/ohci.c: Likewise. Change interf to grub_uint32_t. Remove whitespace inside comment.
---
 bus/usb/ohci.c |    7 ++-----
 bus/usb/uhci.c |    2 --
 2 files changed, 2 insertions(+), 7 deletions(-)

Index: grub2/bus/usb/ohci.c
===================================================================
--- grub2.orig/bus/usb/ohci.c
+++ grub2/bus/usb/ohci.c
@@ -118,7 +118,7 @@ grub_ohci_pci_iter (int bus, int device,
 {
   grub_uint32_t class;
   grub_uint32_t subclass;
-  int interf;
+  grub_uint32_t interf;
   grub_uint32_t base;
   grub_pci_address_t addr;
   struct grub_ohci *o;
@@ -127,8 +127,6 @@ grub_ohci_pci_iter (int bus, int device,
 
   addr = grub_pci_make_address (bus, device, func, 2);
   class = grub_pci_read (addr);
-  addr = grub_pci_make_address (bus, device, func, 2);
-  class = grub_pci_read (addr);
 
   interf = class & 0xFF;
   subclass = (class >> 16) & 0xFF;
@@ -171,8 +169,7 @@ grub_ohci_pci_iter (int bus, int device,
   frame_interval = grub_ohci_readreg32 (o, GRUB_OHCI_REG_FRAME_INTERVAL);
 
   /* Suspend the OHCI by issuing a reset.  */
-  grub_ohci_writereg32 (o, GRUB_OHCI_REG_CMDSTATUS, 1); /* XXX: Magic.
-							  */
+  grub_ohci_writereg32 (o, GRUB_OHCI_REG_CMDSTATUS, 1); /* XXX: Magic.  */
   grub_millisleep (1);
   grub_dprintf ("ohci", "OHCI reset\n");
 
Index: grub2/bus/usb/uhci.c
===================================================================
--- grub2.orig/bus/usb/uhci.c
+++ grub2/bus/usb/uhci.c
@@ -151,8 +151,6 @@ grub_uhci_pci_iter (int bus, int device,
 
   addr = grub_pci_make_address (bus, device, func, 2);
   class = grub_pci_read (addr);
-  addr = grub_pci_make_address (bus, device, func, 2);
-  class = grub_pci_read (addr);
 
   subclass = (class >> 16) & 0xFF;
   class >>= 24;

[PATCH 2/4] Fix inteface definition for ohci

[oliver.henshaw]

[-- Attachment #1: usb-fix-interface --]
[-- Type: text/plain, Size: 559 bytes --]

Changelog:
	* bus/usb/ohci.c: Set interf with correct field.
---
 bus/usb/ohci.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: grub2/bus/usb/ohci.c
===================================================================
--- grub2.orig/bus/usb/ohci.c
+++ grub2/bus/usb/ohci.c
@@ -128,7 +128,7 @@ grub_ohci_pci_iter (int bus, int device,
   addr = grub_pci_make_address (bus, device, func, 2);
   class = grub_pci_read (addr);
 
-  interf = class & 0xFF;
+  interf = (class >> 8) & 0xFF;
   subclass = (class >> 16) & 0xFF;
   class >>= 24;
 

[PATCH 3/4] Check usb programming interface

[oliver.henshaw]

[-- Attachment #1: usb-check-interface --]
[-- Type: text/plain, Size: 2551 bytes --]

Changelog:
	* bus/usb/ohci.c: Check programming interface is ohci. Add grub_dprintf for symmetry 
			  with bus/usb/uhci.c.
	* bus/usb/uhci.c: Check programming interface is uhci. Add interf variable for 
			  Programming Interface. Print interface with class/subclass.
---
 bus/usb/ohci.c |    5 ++++-
 bus/usb/uhci.c |    8 +++++---
 2 files changed, 9 insertions(+), 4 deletions(-)

Index: grub2/bus/usb/ohci.c
===================================================================
--- grub2.orig/bus/usb/ohci.c
+++ grub2/bus/usb/ohci.c
@@ -133,7 +133,7 @@ grub_ohci_pci_iter (int bus, int device,
   class >>= 24;
 
   /* If this is not an OHCI controller, just return.  */
-  if (class != 0x0c || subclass != 0x03)
+  if (class != 0x0c || subclass != 0x03 || interf != 0x10)
     return 0;
 
   /* Determine IO base address.  */
@@ -159,6 +159,9 @@ grub_ohci_pci_iter (int bus, int device,
   /* Reserve memory for the HCCA.  */
   o->hcca = (struct grub_ohci_hcca *) grub_memalign (256, 256);
 
+  grub_dprintf ("ohci", "class=0x%02x 0x%02x interface 0x%02x base=0x%x\n",
+ 		class, subclass, interf, o->iobase);
+
   /* Check if the OHCI revision is actually 1.0 as supported.  */
   revision = grub_ohci_readreg32 (o, GRUB_OHCI_REG_REVISION);
   grub_dprintf ("ohci", "OHCI revision=0x%02x\n", revision & 0xFF);
Index: grub2/bus/usb/uhci.c
===================================================================
--- grub2.orig/bus/usb/uhci.c
+++ grub2/bus/usb/uhci.c
@@ -143,6 +143,7 @@ grub_uhci_pci_iter (int bus, int device,
 {
   grub_uint32_t class;
   grub_uint32_t subclass;
+  grub_uint32_t interf;
   grub_uint32_t base;
   grub_uint32_t fp;
   grub_pci_address_t addr;
@@ -152,11 +153,12 @@ grub_uhci_pci_iter (int bus, int device,
   addr = grub_pci_make_address (bus, device, func, 2);
   class = grub_pci_read (addr);
 
+  interf = (class >> 8) & 0xFF;
   subclass = (class >> 16) & 0xFF;
   class >>= 24;
 
   /* If this is not an UHCI controller, just return.  */
-  if (class != 0x0c || subclass != 0x03)
+  if (class != 0x0c || subclass != 0x03 || interf != 0x00)
     return 0;
 
   /* Determine IO base address.  */
@@ -177,8 +179,8 @@ grub_uhci_pci_iter (int bus, int device,
   u->framelist = 0;
   u->qh = 0;
   u->td = 0;
-  grub_dprintf ("uhci", "class=0x%02x 0x%02x base=0x%x\n",
-		class, subclass, u->iobase);
+  grub_dprintf ("uhci", "class=0x%02x 0x%02x interface 0x%02x base=0x%x\n",
+		class, subclass, interf, u->iobase);
 
   /* Reserve a page for the frame list.  */
   u->framelist = grub_memalign (4096, 4096);

[PATCH 4/4] Define fields in terms of the Class Code register

[oliver.henshaw]

[-- Attachment #1: usb-change-to-class_code --]
[-- Type: text/plain, Size: 2169 bytes --]

Changelog:
	* bus/usb/ohci.c: Define the Class, Subclass and Programming Interface fields in 
			  terms of the 3 byte Class Code register.
	* bus/usb/uhci.c: Likewise.
---
 bus/usb/ohci.c |    9 +++++----
 bus/usb/uhci.c |    9 +++++----
 2 files changed, 10 insertions(+), 8 deletions(-)

Index: grub2/bus/usb/ohci.c
===================================================================
--- grub2.orig/bus/usb/ohci.c
+++ grub2/bus/usb/ohci.c
@@ -116,6 +116,7 @@ static int NESTED_FUNC_ATTR
 grub_ohci_pci_iter (int bus, int device, int func,
 		    grub_pci_id_t pciid __attribute__((unused)))
 {
+  grub_uint32_t class_code;
   grub_uint32_t class;
   grub_uint32_t subclass;
   grub_uint32_t interf;
@@ -126,11 +127,11 @@ grub_ohci_pci_iter (int bus, int device,
   grub_uint32_t frame_interval;
 
   addr = grub_pci_make_address (bus, device, func, 2);
-  class = grub_pci_read (addr);
+  class_code = grub_pci_read (addr) >> 8;
 
-  interf = (class >> 8) & 0xFF;
-  subclass = (class >> 16) & 0xFF;
-  class >>= 24;
+  interf = class_code & 0xFF;
+  subclass = (class_code >> 8) & 0xFF;
+  class = class_code >> 16;
 
   /* If this is not an OHCI controller, just return.  */
   if (class != 0x0c || subclass != 0x03 || interf != 0x10)
Index: grub2/bus/usb/uhci.c
===================================================================
--- grub2.orig/bus/usb/uhci.c
+++ grub2/bus/usb/uhci.c
@@ -141,6 +141,7 @@ static int NESTED_FUNC_ATTR
 grub_uhci_pci_iter (int bus, int device, int func,
 		    grub_pci_id_t pciid __attribute__((unused)))
 {
+  grub_uint32_t class_code;
   grub_uint32_t class;
   grub_uint32_t subclass;
   grub_uint32_t interf;
@@ -151,11 +152,11 @@ grub_uhci_pci_iter (int bus, int device,
   int i;
 
   addr = grub_pci_make_address (bus, device, func, 2);
-  class = grub_pci_read (addr);
+  class_code = grub_pci_read (addr) >> 8;
 
-  interf = (class >> 8) & 0xFF;
-  subclass = (class >> 16) & 0xFF;
-  class >>= 24;
+  interf = class_code & 0xFF;
+  subclass = (class_code >> 8) & 0xFF;
+  class = class_code >> 16;
 
   /* If this is not an UHCI controller, just return.  */
   if (class != 0x0c || subclass != 0x03 || interf != 0x00)

Re: [PATCH 0/4] Re: Grub needs to check the programming interface for usb controllers

[Pavel Roskin]

On Mon, 2009-06-08 at 18:45 +0100, oliver.henshaw@gmail.com wrote:

> Here's a second try. I used quilt to manage the patch series but
> mailed them by hand instead of exporting to a mailbox, and didn't
> realise that they weren't named as *.patch (otherwise I think the
> content type would have been fine). I should probably move on to git,
> but I was a little intimidated by the idea of re-writing history -
> manipulating a patch series seemed more natural to me. But onwards and
> upwards.

STGit manipulates the patches.  It was inspired by quilt.  It should be
really easy to learn for someone who knows git.

> I've removed all changes to trailing whitespace, as requested. There
> is still a whitespace change in side a comment, something I noticed
> with syntax highlighting.

I've applied your patches.  The third patch in the series introduced a
compile warning due to iobase being a pointer on OHCI.  I fixed that.

Please note that the ChangeLog is wrapped at the column 72.  All
non-empty lines except the author's name start with one tab.  Please
specify thew functions affected by your changes unless the changes are
in the top-level code or all over the place.

-- 
Regards,
Pavel Roskin

[PATCH 3/4] Check usb programming interface

[Oliver Henshaw]

[-- Attachment #1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: usb-check-interface --]
[-- Type: application/octet-stream, Size: 2548 bytes --]

Changelog:
	* bus/usb/ohci.c: Check programming interface is ohci. Add grub_dprintf for symmetry 
			  with bus/usb/uhci.c.
	* bus/usb/uhci.c: Check programming interface is uhci. Add interf variable for 
			  Programming Interface. Print interface with class/subclass.
---
 bus/usb/ohci.c |    5 ++++-
 bus/usb/uhci.c |    8 +++++---
 2 files changed, 9 insertions(+), 4 deletions(-)

Index: grub2/bus/usb/ohci.c
===================================================================
--- grub2.orig/bus/usb/ohci.c
+++ grub2/bus/usb/ohci.c
@@ -133,7 +133,7 @@ grub_ohci_pci_iter (int bus, int device,
   class >>= 24;
 
   /* If this is not an OHCI controller, just return.  */
-  if (class != 0x0c || subclass != 0x03)
+  if (class != 0x0c || subclass != 0x03 || interf != 0x10)
     return 0;
 
   /* Determine IO base address.  */
@@ -159,6 +159,9 @@ grub_ohci_pci_iter (int bus, int device,
   /* Reserve memory for the HCCA.  */
   o->hcca = (struct grub_ohci_hcca *) grub_memalign (256, 256);
 
+  grub_dprintf ("ohci", "class=0x%02x 0x%02x interface 0x%02x base=0x%x\n",
+ 		class, subclass, interf, o->iobase);
+
   /* Check if the OHCI revision is actually 1.0 as supported.  */
   revision = grub_ohci_readreg32 (o, GRUB_OHCI_REG_REVISION);
   grub_dprintf ("ohci", "OHCI revision=0x%02x\n", revision & 0xFF);
Index: grub2/bus/usb/uhci.c
===================================================================
--- grub2.orig/bus/usb/uhci.c
+++ grub2/bus/usb/uhci.c
@@ -143,6 +143,7 @@ grub_uhci_pci_iter (int bus, int device,
 {
   grub_uint32_t class;
   grub_uint32_t subclass;
+  grub_uint32_t interf;
   grub_uint32_t base;
   grub_uint32_t fp;
   grub_pci_address_t addr;
@@ -152,11 +153,12 @@ grub_uhci_pci_iter (int bus, int device,
   addr = grub_pci_make_address (bus, device, func, 2);
   class = grub_pci_read (addr);
 
+  interf = (class >> 8) & 0xFF;
   subclass = (class >> 16) & 0xFF;
   class >>= 24;
 
   /* If this is not an UHCI controller, just return.  */
-  if (class != 0x0c || subclass != 0x03)
+  if (class != 0x0c || subclass != 0x03 || interf != 0x00)
     return 0;
 
   /* Determine IO base address.  */
@@ -177,8 +179,8 @@ grub_uhci_pci_iter (int bus, int device,
   u->framelist = 0;
   u->qh = 0;
   u->td = 0;
-  grub_dprintf ("uhci", "class=0x%02x 0x%02x base=0x%x\n",
-		class, subclass, u->iobase);
+  grub_dprintf ("uhci", "class=0x%02x 0x%02x interface 0x%02x base=0x%x\n",
+		class, subclass, interf, u->iobase);
 
   /* Reserve a page for the frame list.  */
   u->framelist = grub_memalign (4096, 4096);