Re: [linux-usb-devel] Re: [OOPS] reproducible oops with 2.6.5-rc2-bk3

Re: [linux-usb-devel] Re: [OOPS] reproducible oops with 2.6.5-rc2-bk3

[Alan Stern]

On Thu, 25 Mar 2004, Colin Leroy wrote:

> > > usb 3-1: new full speed USB device using address 3
> > > Oops: kernel access of bad area, sig: 11 [#1]
> > For info: reverting this change fixes it. Didn't find out why, yet.
> >
> >
> http://linux.bkbits.net:8080/linux-2.5/diffs/drivers/usb/class/cdc-acm.c@1.54?nav=index.html|src/|src/drivers|src/drivers/usb|src/drivers/usb/class|hist/drivers/usb/class/cdc-acm.c
> 
> Maybe that's due to this change in drivers/usb/core/message.c:
> @@ -1056,8 +1055,20 @@
>   /* re-init hc/hcd interface/endpoint state */
>   for (i = 0; i < config->desc.bNumInterfaces; i++) {
>    struct usb_interface *intf = config->interface[i];
> +  struct usb_host_interface *alt;
> +
> +  alt = usb_altnum_to_altsetting(intf, 0);
> +
> +  /* No altsetting 0?  We'll assume the first altsetting.
> +   * We could use a GetInterface call, but if a device is
> +   * so non-compliant that it doesn't have altsetting 0
> +   * then I wouldn't trust its reply anyway.
> +   */
> +  if (!alt)
> +   alt = &intf->altsetting[0];
> 
> -  intf->act_altsetting = 0;
> +  intf->cur_altsetting = alt;
> +  intf->act_altsetting = alt - intf->altsetting;
>    usb_enable_interface(dev, intf);
>   }
>   return 0;
> If I understand it well, this changes the previous default behaviour;
> intf->cur_altsetting will be intf->altsetting[0], like before, only if
> there's no intf->altsetting[i].desc.bAlternateSetting == 0.

That's right.  However, the oops you saw shouldn't happen so long as
intf->cur_altsetting points to something valid.  I got the impression that
in the cdc-acm probe routine maybe it was a null pointer.  Can you insert
a statement in the cdc-acm probe function to print out the values of ifcom
and ifdata, to check that they aren't NULL?  While you're at it also try
printing out the value of ifcom->desc.bNumEndpoints; if that is 0 it will
cause an oops.

> I'm not sure the change in cdc-acm (which no longer uses index 0
> altsetting) is correct. Or is this another bug in my phone (motorola C350)
> which should be handled differently than other cdc-acm devices ?

It's hard to say without more information.  The contents of 
/proc/bus/usb/devices or the output of lsusb with the phone plugged in 
would help.

Alan Stern

Re: [linux-usb-devel] Re: [OOPS] reproducible oops with 2.6.5-rc2-bk3

[Colin Leroy]

Hi,

> That's right.  However, the oops you saw shouldn't happen so long as
> intf->cur_altsetting points to something valid.  I got the impression
that
> in the cdc-acm probe routine maybe it was a null pointer.

Yes, i think so too.

> Can you insert
> a statement in the cdc-acm probe function to print out the values of
ifcom
> and ifdata, to check that they aren't NULL?

Already done, that's what helped me find this was related to altsettings
stuff. I didn't check to see if cur_altsetting was null, but a dev_dbg()
at the beginnning of the function was appearing, and not the dev_dbg() I
had put after this block.

> > I'm not sure the change in cdc-acm (which no longer uses index 0
> > altsetting) is correct. Or is this another bug in my phone (motorola
C350)
> > which should be handled differently than other cdc-acm devices ?
>
> It's hard to say without more information.  The contents of
> /proc/bus/usb/devices or the output of lsusb with the phone plugged in
> would help.

Ok, I'll try to do that tonight.
-- 
Colin

Re: [linux-usb-devel] Re: [OOPS] reproducible oops with 2.6.5-rc2-bk3

[Colin Leroy]

Hi again,

> > If I understand it well, this changes the previous default behaviour;
> > intf->cur_altsetting will be intf->altsetting[0], like before, only if
> > there's no intf->altsetting[i].desc.bAlternateSetting == 0.
>
> That's right.  However, the oops you saw shouldn't happen so long as
> intf->cur_altsetting points to something valid.  I got the impression
that
> in the cdc-acm probe routine maybe it was a null pointer.

Additionally, there's still a reference to altsetting[0] in cdc-acm.c (in
acm_ctrl_msg()), I don't know if it's intentional or should have been
converted from
    acm->control->altsetting[0].desc.bInterfaceNumber
to
    acm->control->cur_altsetting.desc.bInterfaceNumber
?
-- 
Colin
  This message represents the official view of the voices
  in my head.

Re: [linux-usb-devel] Re: [OOPS] reproducible oops with 2.6.5-rc2-bk3

[Alan Stern]

On Thu, 25 Mar 2004, Colin Leroy wrote:

> Additionally, there's still a reference to altsetting[0] in cdc-acm.c (in
> acm_ctrl_msg()), I don't know if it's intentional or should have been
> converted from
>     acm->control->altsetting[0].desc.bInterfaceNumber
> to
>     acm->control->cur_altsetting.desc.bInterfaceNumber
> ?

It's not as bad as it looks.  All the altsetting entries for an interface
are guaranteed to have the same value stored in desc.bInterfaceNumber, so
it really doesn't matter whether that value is retrieved from the first
entry in the altsetting array or the current entry -- provided of course
that cur_altsetting is a valid pointer!  It's mostly just a matter of 
taste.  I generally prefer to use cur_altsetting because that's the one 
currently installed in the device.

Alan Stern

Re: [linux-usb-devel] Re: [OOPS] reproducible oops with 2.6.5-rc2-bk3

[Colin Leroy]

On 25 Mar 2004 at 11h03, Alan Stern wrote:

> It's not as bad as it looks. 

Ok, thanks for the clarification. In the mean time here's the lsusb -v output for my phone. I'll try to find out why we have a null pointer.


Bus 003 Device 003: ID 22b8:3802
Device Descriptor:
  bLength                18
  bDescriptorType         1
  bcdUSB              10.01
  bDeviceClass            2 Communications
  bDeviceSubClass         0
  bDeviceProtocol         0
  bMaxPacketSize0         8
  idVendor           0x22b8
  idProduct          0x3802
  bcdDevice            1.00
  iManufacturer           1 Motorola Inc.
  iProduct                2 Motorola Phone (C350)
  iSerial                 0
  bNumConfigurations      1
  Configuration Descriptor:
    bLength                 9
    bDescriptorType         2
    wTotalLength           67
    bNumInterfaces          2
    bConfigurationValue     1
    iConfiguration          4
    bmAttributes         0xc0
      Self Powered
    MaxPower               20mA
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        0
      bAlternateSetting       0
      bNumEndpoints           1
      bInterfaceClass         2 Communications
      bInterfaceSubClass      2 Abstract (modem)
      bInterfaceProtocol      1 AT-commands (v.25ter)
      iInterface              5 Motorola Communication Interface
  unknown descriptor type: 05 24 00 01 01
  unknown descriptor type: 05 24 01 03 01
  unknown descriptor type: 05 24 06 00 01
  unknown descriptor type: 04 24 02 02
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x89  EP 9 IN
        bmAttributes            3
          Transfer Type            Interrupt
          Synch Type               none
        wMaxPacketSize         16
        bInterval              10
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        1
      bAlternateSetting       0
      bNumEndpoints           2
      bInterfaceClass        10 Data
      bInterfaceSubClass      0 Unused
      bInterfaceProtocol      0
      iInterface             16 Motorola Data Interface
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x01  EP 1 OUT
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               none
        wMaxPacketSize         16
        bInterval               0
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x82  EP 2 IN
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               none
        wMaxPacketSize         16
        bInterval               0
  Language IDs: (length=4)
     0409 English(US)

Thanks!
-- 
Colin

[PATCH] Re: [linux-usb-devel] Re: [OOPS] reproducible oops with 2.6.5-rc2-bk3

[Colin Leroy]

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

On 25 Mar 2004 at 11h03, Alan Stern wrote:

Hi, 

Found out !
cdc-acm wants both interfaces to be ready (cur_altsetting initialized) when acm_probe() is called. Hence, we have to make two parts out of the loop in message.c::usb_set_configuration(): one to init things, one to register them. 

The attached patch does that. It fixes the oops, and doesn't break any of my USB peripheral (printer, scanner, mouse, and diskonkey).

I hope it's fine enough to go in :)
-- 
Colin

[-- Attachment #2: cdc-acm.oops.patch --]
[-- Type: application/octet-stream, Size: 726 bytes --]

--- drivers/usb/core/message.c.orig	2004-03-25 18:34:04.000000000 +0100
+++ drivers/usb/core/message.c	2004-03-25 18:42:41.267697224 +0100
@@ -1178,10 +1178,17 @@
 				 dev->bus->busnum, dev->devpath,
 				 configuration,
 				 alt->desc.bInterfaceNumber);
+		}
+		
+		/* all interfaces are initialized, we can now 
+		 * register them
+		 */
+		for (i = 0; i < cp->desc.bNumInterfaces; ++i) {
+			struct usb_interface *intf = cp->interface[i];
 			dev_dbg (&dev->dev,
 				"registering %s (config #%d, interface %d)\n",
 				intf->dev.bus_id, configuration,
-				alt->desc.bInterfaceNumber);
+				intf->cur_altsetting->desc.bInterfaceNumber);
 			device_register (&intf->dev);
 			usb_create_driverfs_intf_files (intf);
 		}

Re: [PATCH] Re: [linux-usb-devel] Re: [OOPS] reproducible oops with 2.6.5-rc2-bk3

[Alan Stern]

On Thu, 25 Mar 2004, Colin Leroy wrote:

> Hi, 
> 
> Found out !
> cdc-acm wants both interfaces to be ready (cur_altsetting initialized) when acm_probe() is called. Hence, we have to make two parts out of the loop in message.c::usb_set_configuration(): one to init things, one to register them. 
> 
> The attached patch does that. It fixes the oops, and doesn't break any of my USB peripheral (printer, scanner, mouse, and diskonkey).
> 
> I hope it's fine enough to go in :)

It's obvious once you see the problem, isn't it?

Duncan Sands found exactly the same problem and the same solution a few 
months ago, but it was never added in.

In this case, your patch could be improved by calling device_initialize()  
during the first loop and device_add() during the second.  However, that
region of code is kind of in flux right at the moment.  When things settle
down, I promise to remember your change and make sure it gets in.

Alan Stern

Re: [PATCH] Re: [linux-usb-devel] Re: [OOPS] reproducible oops with 2.6.5-rc2-bk3

[Colin Leroy]

On 25 Mar 2004 at 13h03, Alan Stern wrote:

Hi, 

> In this case, your patch could be improved by calling device_initialize()  
> during the first loop and device_add() during the second.  However, that
> region of code is kind of in flux right at the moment.  When things settle
> down, I promise to remember your change and make sure it gets in.

ok :) 
Will this get stabilized before 2.6.5 or after ? (just so I remember to patch it myself if needed...)

Thanks,
-- 
Colin

Re: [PATCH] Re: [linux-usb-devel] Re: [OOPS] reproducible oops with 2.6.5-rc2-bk3

[Alan Stern]

On Thu, 25 Mar 2004, Colin Leroy wrote:

> On 25 Mar 2004 at 13h03, Alan Stern wrote:
> 
> Hi, 
> 
> > In this case, your patch could be improved by calling device_initialize()  
> > during the first loop and device_add() during the second.  However, that
> > region of code is kind of in flux right at the moment.  When things settle
> > down, I promise to remember your change and make sure it gets in.
> 
> ok :) 
> Will this get stabilized before 2.6.5 or after ? (just so I remember to patch it myself if needed...)

I don't know.  "After" is my best guess.

Alan Stern

Re: [PATCH] Re: [linux-usb-devel] Re: [OOPS] reproducible oops with 2.6.5-rc2-bk3

[Colin Leroy]

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

On 25 Mar 2004 at 13h03, Alan Stern wrote:

Hi, 

> In this case, your patch could be improved by calling device_initialize()  
> during the first loop and device_add() during the second. 

Here you are :)
-- 
Colin

[-- Attachment #2: cdc-acm.oops.patch --]
[-- Type: application/octet-stream, Size: 963 bytes --]

--- drivers/usb/core/message.c.orig	2004-03-25 18:34:04.000000000 +0100
+++ drivers/usb/core/message.c	2004-03-25 20:53:06.000000000 +0100
@@ -1179,10 +1179,24 @@
 				 configuration,
 				 alt->desc.bInterfaceNumber);
 			dev_dbg (&dev->dev,
+				"initializing %s (config #%d, interface %d)\n",
+				intf->dev.bus_id, configuration,
+				intf->cur_altsetting->desc.bInterfaceNumber);
+			device_initialize (&intf->dev);			
+		}
+		
+		/* all interfaces are initialized, we can now 
+		 * register them
+		 */
+		for (i = 0; i < cp->desc.bNumInterfaces; ++i) {
+			struct usb_interface *intf = cp->interface[i];
+			dev_dbg (&dev->dev,
 				"registering %s (config #%d, interface %d)\n",
 				intf->dev.bus_id, configuration,
-				alt->desc.bInterfaceNumber);
-			device_register (&intf->dev);
+				intf->cur_altsetting->desc.bInterfaceNumber);
+			if ((ret = device_add (&intf->dev)) < 0)
+				goto out;
+				
 			usb_create_driverfs_intf_files (intf);
 		}
 	}