ACM USB modem on Kernel 2.6.0-test

DISCUSSION David Brownell 2 participants 5 messages Oct 11, 2003 – Oct 19, 2003

Subsystems (2)

USB USB ACM driver

lore

Share & tools

lore

David Brownell

author

2003-10-11 18:46 UTC #0 lore raw

Quoted 3 lines

usb 3-3: configuration #1 chosen from 2 choices drivers/usb/class/cdc-acm.c: need inactive config #2 drivers/usb/class/cdc-acm.c: need inactive config #2

Until we get more intelligence somewhere, do this:

   # cd /sys/bus/usb/devices/3-3
   # echo '2' > bConfigurationValue
   #

That makes the device use vendor-neutral protocols to talk to the host, not MSFT-proprietary ones. (It's important to use the numbers from those messages; they will change if you use different USB ports.)

Hmm ... maybe usbcore would be better off with a less naive algorithm for choosing defaults. Like, preferring configurations without proprietary device protocols. That'd solve every cdc-acm case, and likely others.

- Dave

Jamie Lokier

2003-10-12 12:08 UTC #1 lore raw

Quoted 4 lines · David Brownell

Hmm ... maybe usbcore would be better off with a less naive algorithm for choosing defaults. Like, preferring configurations without proprietary device protocols. That'd solve every cdc-acm case, and likely others.

Presumably 2.4 does that, because my acm modem works with 2.3 and 2.4 kernels.

Do you know anything about the proprietary protocols, btw?

-- Jamie

David Brownell

author

2003-10-12 18:53 UTC #2 lore raw

Quoted 10 lines · Jamie Lokier

Quoted 4 lines · David Brownell
Hmm ... maybe usbcore would be better off with a less naive algorithm for choosing defaults. Like, preferring configurations without proprietary device protocols. That'd solve every cdc-acm case, and likely others.
Presumably 2.4 does that, because my acm modem works with 2.3 and 2.4 kernels.

No, 2.4 is just as dumb -- but it had a way to kluge around that. But that kluge doesn't work any more on 2.6, mostly because usb_set_configuration() now behaves sanely. (Not only does it shut down the old configuration ... but it also sets up the new one correctly.)

Your modem should work just fine with 2.6 too, if you just switch to the other configuration from userspace. However, I'd certainly like to reduce the need for such steps.

Quoted 1 line

Do you know anything about the proprietary protocols, btw?

My understanding is that knowing technical details involves signing NDAs with MSFT. But I've not investigated much; likely other people know more.

- Dave

David Brownell

author

2003-10-17 20:05 UTC #3 lore raw

In reply to David Brownell

Quoted 5 lines · David Brownell

Hmm ... maybe usbcore would be better off with a less naive algorithm for choosing defaults. Like, preferring configurations without proprietary device protocols. That'd solve every cdc-acm case, and likely others.

In fact, here's a patch with that very change. Does it make current 2.6.0-test kernels work "out of the box" again with your USB modems?

- Dave

drivers/usb/core/usb.c +16 -3

@@ -991,6 +997,7 @@
 	int err = -EINVAL;
 	int i;
 	int j;
+	int config;
 
 	/*
 	 * Set the driver for the usb device to point to the "generic" driver.

@@ -1105,15 +1112,27 @@
 
 	/* choose and set the configuration. that registers the interfaces
 	 * with the driver core, and lets usb device drivers bind to them.
+	 * NOTE:  should interact with hub power budgeting.
 	 */
+	config = dev->config[0].desc.bConfigurationValue;
 	if (dev->descriptor.bNumConfigurations != 1) {
+		for (i = 0; i < dev->descriptor.bNumConfigurations; i++) {
+			/* heuristic:  Linux is more likely to have class
+			 * drivers, so avoid vendor-specific interfaces.
+			 */
+			if (dev->config[i].interface[0]->altsetting
+						->desc.bInterfaceClass
+					== USB_CLASS_VENDOR_SPEC)
+				continue;
+			config = dev->config[i].desc.bConfigurationValue;
+			break;
+		}
 		dev_info(&dev->dev,
 			"configuration #%d chosen from %d choices\n",
-			dev->config[0].desc.bConfigurationValue,
+			config,
 			dev->descriptor.bNumConfigurations);
 	}
-	err = usb_set_configuration(dev,
-			dev->config[0].desc.bConfigurationValue);
+	err = usb_set_configuration(dev, config);
 	if (err) {
 		dev_err(&dev->dev, "can't set config #%d, error %d\n",
 			dev->config[0].desc.bConfigurationValue, err);

David Brownell

author

2003-10-19 15:07 UTC #4 lore raw

Quoted 11 lines

Quoted 5 lines
Quoted 3 lines
In fact, here's a patch with that very change. Does it make current 2.6.0-test kernels work "out of the box" again with your USB modems?
Yes, it works with ELSA Microlink USB. Thanks.
Hmm. Too early. I get either a "acm: probe of 3-3:2.1 failed with error -5"
but it works or a Unable to handle kernel NULL pointer dereference at virtual address 00000008
...
EIP: 0060:[usb_driver_claim_interface+67/112] Tainted: P
...

Well, the "it works at all (without the sysfs write)" is what that patch was about -- so it's still a clear win!

But cdc-acm probe() is pretty broken, and I'm told it's had strange behavior in various other cases for a while, including some oopsing. Like this; not a new bug.

Try this cdc-acm patch. One user reported that it made oopsing go away, the bogus probe() errors stopped, and even the /proc/bus/usb/devices listings were finally right (both interfaces now claimed by cdc_acm). Plus it should stop the pointless hotplugging of "cdc_acm" for Ethernet devices (including MSFT's RNDIS).

- Dave

drivers/usb/class/cdc-acm.c +43 ~~-39~~

@@ -1,5 +1,5 @@
 /*
- * acm.c  Version 0.21
+ * acm.c  Version 0.22
  *
  * Copyright (c) 1999 Armin Fuerst	<[email protected]>
  * Copyright (c) 1999 Pavel Machek	<[email protected]>

@@ -24,6 +24,8 @@
  *	v0.19 - fixed CLOCAL handling (thanks to Richard Shih-Ping Chan)
  *	v0.20 - switched to probing on interface (rather than device) class
  *	v0.21 - revert to probing on device for devices with multiple configs
+ *	v0.22 - probe only the control interface. if usbcore doesn't choose the
+ *		config we want, sysadmin changes bConfigurationValue in sysfs.
  */
 
 /*

@@ -139,7 +141,8 @@
 
 struct acm {
 	struct usb_device *dev;				/* the corresponding usb device */
-	struct usb_interface *iface;			/* the interfaces - +0 control +1 data */
+	struct usb_interface *control;			/* control interface */
+	struct usb_interface *data;			/* data interface */
 	struct tty_struct *tty;				/* the corresponding tty */
 	struct urb *ctrlurb, *readurb, *writeurb;	/* urbs */
 	struct acm_line line;				/* line coding (bits, stop, parity) */

@@ -167,12 +170,15 @@
 {
 	int retval = usb_control_msg(acm->dev, usb_sndctrlpipe(acm->dev, 0),
 		request, USB_RT_ACM, value,
-		acm->iface[0].altsetting[0].desc.bInterfaceNumber,
+		acm->control->altsetting[0].desc.bInterfaceNumber,
 		buf, len, HZ * 5);
 	dbg("acm_control_msg: rq: 0x%02x val: %#x len: %#x result: %d", request, value, len, retval);
 	return retval < 0 ? retval : 0;
 }
 
+/* devices aren't required to support these requests.
+ * the cdc acm descriptor tells whether they do...
+ */
 #define acm_set_control(acm, control)	acm_ctrl_msg(acm, ACM_REQ_SET_CONTROL, control, NULL, 0)
 #define acm_set_line(acm, line)		acm_ctrl_msg(acm, ACM_REQ_SET_LINE, 0, line, sizeof(struct acm_line))
 #define acm_send_break(acm, ms)		acm_ctrl_msg(acm, ACM_REQ_SEND_BREAK, ms, NULL, 0)

@@ -211,7 +217,7 @@
 
 		case ACM_IRQ_NETWORK:
 
-			dbg("%s network", data[0] ? "connected to" : "disconnected from");
+			dbg("%s network", dr->wValue ? "connected to" : "disconnected from");
 			break;
 
 		case ACM_IRQ_LINE_STATE:

@@ -546,17 +552,15 @@
 	struct usb_device *dev;
 	struct acm *acm;
 	struct usb_host_config *cfacm;
+	struct usb_interface *data;
 	struct usb_host_interface *ifcom, *ifdata;
 	struct usb_endpoint_descriptor *epctrl, *epread, *epwrite;
-	int readsize, ctrlsize, minor, i, j;
+	int readsize, ctrlsize, minor, j;
 	unsigned char *buf;
 
 	dev = interface_to_usbdev (intf);
-	for (i = 0; i < dev->descriptor.bNumConfigurations; i++) {
-
-		cfacm = dev->config + i;
 
-		dbg("probing config %d", cfacm->desc.bConfigurationValue);
+		cfacm = dev->actconfig;
 
 		for (j = 0; j < cfacm->desc.bNumInterfaces - 1; j++) {

@@ -564,19 +568,23 @@
 			    usb_interface_claimed(cfacm->interface[j + 1]))
 			continue;
 
-			ifcom = cfacm->interface[j]->altsetting + 0;
-			ifdata = cfacm->interface[j + 1]->altsetting + 0;
-
-			if (ifdata->desc.bInterfaceClass != 10 || ifdata->desc.bNumEndpoints < 2) {
-				ifcom = cfacm->interface[j + 1]->altsetting + 0;
+			/* We know we're probe()d with the control interface.
+			 * FIXME ACM doesn't guarantee the data interface is
+			 * adjacent to the control interface, or that if one
+			 * is there it's not for call management ... so use
+			 * the cdc union descriptor whenever there is one.
+			 */
+			ifcom = intf->altsetting + 0;
+			if (intf == cfacm->interface[j]) {
+				ifdata = cfacm->interface[j + 1]->altsetting + 0;
+				data = cfacm->interface[j + 1];
+			} else if (intf == cfacm->interface[j + 1]) {
 				ifdata = cfacm->interface[j]->altsetting + 0;
-				if (ifdata->desc.bInterfaceClass != 10 || ifdata->desc.bNumEndpoints < 2)
-					continue;
-			}
+				data = cfacm->interface[j];
+			} else
+				continue;
 
-			if (ifcom->desc.bInterfaceClass != 2 || ifcom->desc.bInterfaceSubClass != 2 ||
-			    ifcom->desc.bInterfaceProtocol < 1 || ifcom->desc.bInterfaceProtocol > 6 ||
-			    ifcom->desc.bNumEndpoints < 1)
+			if (ifdata->desc.bInterfaceClass != 10 || ifdata->desc.bNumEndpoints < 2)
 				continue;
 
 			epctrl = &ifcom->endpoint[0].desc;

@@ -593,15 +601,6 @@
 				epwrite = &ifdata->endpoint[0].desc;
 			}
 
-			/* FIXME don't scan every config. it's either correct
-			 * when we probe(), or some other task must fix this.
-			 */
-			if (dev->actconfig != cfacm) {
-				err("need inactive config #%d",
-					cfacm->desc.bConfigurationValue);
-				return -ENODEV;
-			}
-
 			for (minor = 0; minor < ACM_TTY_MINORS && acm_table[minor]; minor++);
 			if (acm_table[minor]) {
 				err("no more free acm devices");

@@ -617,7 +616,8 @@
 			ctrlsize = epctrl->wMaxPacketSize;
 			readsize = epread->wMaxPacketSize;
 			acm->writesize = epwrite->wMaxPacketSize;
-			acm->iface = cfacm->interface[j];
+			acm->control = intf;
+			acm->data = data;
 			acm->minor = minor;
 			acm->dev = dev;

@@ -665,7 +665,7 @@
 				buf += readsize, acm->writesize, acm_write_bulk, acm);
 			acm->writeurb->transfer_flags |= URB_NO_FSBR;
 
-			info("ttyACM%d: USB ACM device", minor);
+			dev_info(&intf->dev, "ttyACM%d: USB ACM device", minor);
 
 			acm_set_control(acm, acm->ctrlout);

@@ -673,8 +673,7 @@
 			acm->line.databits = 8;
 			acm_set_line(acm, &acm->line);
 
-			usb_driver_claim_interface(&acm_driver, acm->iface + 0, acm);
-			usb_driver_claim_interface(&acm_driver, acm->iface + 1, acm);
+			usb_driver_claim_interface(&acm_driver, data, acm);
 
 			tty_register_device(acm_tty_driver, minor, &intf->dev);

@@ -682,7 +681,6 @@
 			usb_set_intfdata (intf, acm);
 			return 0;
 		}
-	}
 
 	return -EIO;
 }

@@ -705,8 +703,7 @@
 
 	kfree(acm->ctrlurb->transfer_buffer);
 
-	usb_driver_release_interface(&acm_driver, acm->iface + 0);
-	usb_driver_release_interface(&acm_driver, acm->iface + 1);
+	usb_driver_release_interface(&acm_driver, acm->data);
 
 	if (!acm->used) {
 		tty_unregister_device(acm_tty_driver, acm->minor);

@@ -727,8 +724,15 @@
  */
 
 static struct usb_device_id acm_ids[] = {
-	{ USB_DEVICE_INFO(USB_CLASS_COMM, 0, 0) },
-	{ USB_DEVICE_INFO(USB_CLASS_COMM, 2, 0) },
+	/* control interfaces with various AT-command sets */
+	{ USB_INTERFACE_INFO(USB_CLASS_COMM, 2, 1) },
+	{ USB_INTERFACE_INFO(USB_CLASS_COMM, 2, 2) },
+	{ USB_INTERFACE_INFO(USB_CLASS_COMM, 2, 3) },
+	{ USB_INTERFACE_INFO(USB_CLASS_COMM, 2, 4) },
+	{ USB_INTERFACE_INFO(USB_CLASS_COMM, 2, 5) },
+	{ USB_INTERFACE_INFO(USB_CLASS_COMM, 2, 6) },
+
+	/* NOTE:  COMM/2/0xff is likely MSFT RNDIS ... NOT a modem!! */
 	{ }
 };

@@ -736,7 +740,7 @@
 
 static struct usb_driver acm_driver = {
 	.owner =	THIS_MODULE,
-	.name =		"acm",
+	.name =		"cdc_acm",
 	.probe =	acm_probe,
 	.disconnect =	acm_disconnect,
 	.id_table =	acm_ids,