2.6.6-rc1: cdc-acm still (differently) broken

2.6.6-rc1: cdc-acm still (differently) broken

[Colin Leroy]

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

Hi,

cdc-acm was broken since after 2.6.4, due to the alt_cursetting changes. I sent a patch, which has been integrated (well, the same one has ;-)) not long ago.
I gave 2.6.6-rc1 a try, and found that cdc-acm is now broken is a new way:
when plugging the phone, acm_probe() fails on interface #0; I traced the problem to this: usb_interface_claimed() returns true - and in fact intf->dev.driver is already cdc-acm (despite the fact that this is the first call to acm_probe() !), for reasons beyond my comprehension.

But, even if the interface is claimed, the intfdata hasn't been set, which allows to do another check: the attached patch fixes this bug. 

HTH,
-- 
Colin

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

--- drivers/usb/class/cdc-acm.c.orig	2004-04-15 20:04:47.051145144 +0200
+++ drivers/usb/class/cdc-acm.c	2004-04-15 20:05:52.419207688 +0200
@@ -585,7 +585,8 @@
 
 		for (j = 0; j < cfacm->desc.bNumInterfaces - 1; j++) {
 		    
-			if (usb_interface_claimed(cfacm->interface[j]) ||
+			if ((usb_interface_claimed(cfacm->interface[j]) 
+				&& usb_get_intfdata(cfacm->interface[j]) != NULL ) ||
 			    usb_interface_claimed(cfacm->interface[j + 1]))
 				continue;
 

Re: [linux-usb-devel] 2.6.6-rc1: cdc-acm still (differently) broken

[Oliver Neukum]

Am Donnerstag, 15. April 2004 20:11 schrieb Colin Leroy:
> Hi,
>
> cdc-acm was broken since after 2.6.4, due to the alt_cursetting changes. I
> sent a patch, which has been integrated (well, the same one has ;-)) not
> long ago. I gave 2.6.6-rc1 a try, and found that cdc-acm is now broken is a
> new way: when plugging the phone, acm_probe() fails on interface #0; I
> traced the problem to this: usb_interface_claimed() returns true - and in
> fact intf->dev.driver is already cdc-acm (despite the fact that this is the
> first call to acm_probe() !), for reasons beyond my comprehension.
>
> But, even if the interface is claimed, the intfdata hasn't been set, which
> allows to do another check: the attached patch fixes this bug.

But somebody else may have claimed the interface. You can't simply
assume that only cdc-acm will take the device.

	Regards
		Oliver

Re: [linux-usb-devel] 2.6.6-rc1: cdc-acm still (differently) broken

[David Brownell]

Colin Leroy wrote:
> Hi,
> 
> I gave 2.6.6-rc1 a try, and found that cdc-acm is now broken is a new way:
> ... usb_interface_claimed() returns true ... intf->dev.driver is already cdc-acm 

The interface being probed is by definition not going to be claimed
by any other driver ... it shouldn't check or claim that interface.

That test has always been buggy -- better to just remove it.  For
that matter, usb_interface_claimed() calls should all vanish ... it's
better to fail if claiming the interface fails (one step, not two).
Care to try an updated patch?

This started to matter because as of RC1, usbcore got rid of the last of
some pre-driver-model code for driver binding.  There might be a similar
bug in the ALSA usb audio driver, according to 'grep'.

- Dave



> 
> HTH,

Re: [linux-usb-devel] 2.6.6-rc1: cdc-acm still (differently) broken

[Colin Leroy]

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

On 15 Apr 2004 at 11h04, David Brownell wrote:

Hi David, 

> That test has always been buggy -- better to just remove it.  For
> that matter, usb_interface_claimed() calls should all vanish ... it's
> better to fail if claiming the interface fails (one step, not two).
> Care to try an updated patch?

Like this one? It works. I'm a bit wondering, however, how comes 
usb_interface_claimed() returns true, and the check in 
usb_driver_claim_interface() passes?


-- 
Colin

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

--- drivers/usb/class/cdc-acm.c.orig	2004-04-15 20:04:47.000000000 +0200
+++ drivers/usb/class/cdc-acm.c	2004-04-15 21:20:00.255123616 +0200
@@ -585,10 +585,6 @@
 
 		for (j = 0; j < cfacm->desc.bNumInterfaces - 1; j++) {
 		    
-			if (usb_interface_claimed(cfacm->interface[j]) ||
-			    usb_interface_claimed(cfacm->interface[j + 1]))
-				continue;
-
 			/* 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
@@ -696,7 +692,13 @@
 			acm->line.databits = 8;
 			acm_set_line(acm, &acm->line);
 
-			usb_driver_claim_interface(&acm_driver, data, acm);
+			if ( (j = usb_driver_claim_interface(&acm_driver, data, acm)) != 0) {
+				err("claim failed");
+				usb_free_urb(acm->ctrlurb);
+				kfree(acm);
+				kfree(buf);
+				return j;
+			} 
 
 			tty_register_device(acm_tty_driver, minor, &intf->dev);
 

Re: [linux-usb-devel] 2.6.6-rc1: cdc-acm still (differently) broken

[David Brownell]

Hi Colin,

>>That test has always been buggy -- better to just remove it.  For
>>that matter, usb_interface_claimed() calls should all vanish ... it's
>>better to fail if claiming the interface fails (one step, not two).
>>Care to try an updated patch?
> 
> 
> Like this one? It works. I'm a bit wondering, however, how comes 
> usb_interface_claimed() returns true, and the check in 
> usb_driver_claim_interface() passes?

Pretty much like that one, but not leaking the other urbs ... :)

There are two interfaces involved, for "control" and "data".
"Control" is being probed; and "data" is what gets claimed.

For more info, you could see how "usbnet" handles CDC Ethernet;
see how it parses the CDC Union descriptor (which is what the
FIXME refers to).  Or read the CDC spec, from www.usb.org as PDF.

- Dave

Re: [linux-usb-devel] 2.6.6-rc1: cdc-acm still (differently) broken

[Colin Leroy]

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

On 15 Apr 2004 at 12h04, David Brownell wrote:

Hi, 

> Pretty much like that one, but not leaking the other urbs ... :)

The joys of copy/pasting :)

> There are two interfaces involved, for "control" and "data".
> "Control" is being probed; and "data" is what gets claimed.

Yup, I realized that after sending the mail.

Here's another patch, which fixes the leak. It also fixes the FIXME, by looking at all interfaces to find the data one. Is it correct ?

(indentation is ugly in this part, I'll send a cosmetic patch if this one is accepted)...
-- 
Colin

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

--- drivers/usb/class/cdc-acm.c.orig	2004-04-15 20:04:47.000000000 +0200
+++ drivers/usb/class/cdc-acm.c	2004-04-16 12:21:40.120865512 +0200
@@ -581,45 +581,47 @@
 
 	dev = interface_to_usbdev (intf);
 
-		cfacm = dev->actconfig;
-
-		for (j = 0; j < cfacm->desc.bNumInterfaces - 1; j++) {
-		    
-			if (usb_interface_claimed(cfacm->interface[j]) ||
-			    usb_interface_claimed(cfacm->interface[j + 1]))
-				continue;
+			cfacm = dev->actconfig;
+	
+			/* We know we're probe()d with the control interface. */
+			ifcom = intf->cur_altsetting;
 
-			/* We know we're probe()d with the control interface.
-			 * FIXME ACM doesn't guarantee the data interface is
+			/* 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.
+			 * is there it's not for call management ... so find
+			 * it
 			 */
-			ifcom = intf->cur_altsetting;
-			if (intf == cfacm->interface[j]) {
-				ifdata = cfacm->interface[j + 1]->cur_altsetting;
-				data = cfacm->interface[j + 1];
-			} else if (intf == cfacm->interface[j + 1]) {
+			for (j = 0; j < cfacm->desc.bNumInterfaces; j++) {
 				ifdata = cfacm->interface[j]->cur_altsetting;
 				data = cfacm->interface[j];
-			} else 
-				continue;
-			
-			if (ifdata->desc.bInterfaceClass != 10 || ifdata->desc.bNumEndpoints < 2)
-				continue;
-
-			epctrl = &ifcom->endpoint[0].desc;
-			epread = &ifdata->endpoint[0].desc;
-			epwrite = &ifdata->endpoint[1].desc;
-
-			if ((epctrl->bEndpointAddress & 0x80) != 0x80 || (epctrl->bmAttributes & 3) != 3 ||
-			   (epread->bmAttributes & 3) != 2 || (epwrite->bmAttributes & 3) != 2 ||
-			   ((epread->bEndpointAddress & 0x80) ^ (epwrite->bEndpointAddress & 0x80)) != 0x80)
-				continue;
-
-			if ((epread->bEndpointAddress & 0x80) != 0x80) {
-				epread = &ifdata->endpoint[1].desc;
-				epwrite = &ifdata->endpoint[0].desc;
+
+				if (ifdata->desc.bInterfaceClass == 10 &&
+				    ifdata->desc.bNumEndpoints == 2) {
+					epctrl = &ifcom->endpoint[0].desc;
+					epread = &ifdata->endpoint[0].desc;
+					epwrite = &ifdata->endpoint[1].desc;
+
+					if ((epctrl->bEndpointAddress & 0x80) != 0x80 ||
+					    (epctrl->bmAttributes & 3) != 3 ||
+					    (epread->bmAttributes & 3) != 2 || 
+					    (epwrite->bmAttributes & 3) != 2 ||
+					    ((epread->bEndpointAddress & 0x80) ^ (epwrite->bEndpointAddress & 0x80)) != 0x80) 
+						goto next_interface;
+
+					dbg("found data interface at %d\n", j);
+					break;
+				} else {
+next_interface:
+					ifdata = NULL;
+					data = NULL;
+				}
+			}
+
+			/* there's been a problem */
+			if (!ifdata) {
+				dbg("interface not found (%p)\n", ifdata);
+				return -ENODEV;
+
 			}
 
 			for (minor = 0; minor < ACM_TTY_MINORS && acm_table[minor]; minor++);
@@ -696,16 +698,21 @@
 			acm->line.databits = 8;
 			acm_set_line(acm, &acm->line);
 
-			usb_driver_claim_interface(&acm_driver, data, acm);
+			if ( (j = usb_driver_claim_interface(&acm_driver, data, acm)) != 0) {
+				err("claim failed");
+				usb_free_urb(acm->ctrlurb);
+				usb_free_urb(acm->readurb);
+				usb_free_urb(acm->writeurb);
+				kfree(acm);
+				kfree(buf);
+				return j;
+			} 
 
 			tty_register_device(acm_tty_driver, minor, &intf->dev);
 
 			acm_table[minor] = acm;
 			usb_set_intfdata (intf, acm);
 			return 0;
-		}
-
-	return -EIO;
 }
 
 static void acm_disconnect(struct usb_interface *intf)

Re: [linux-usb-devel] 2.6.6-rc1: cdc-acm still (differently) broken

[David Brownell]

Hi Colin,

> Here's another patch, which fixes the leak. It also fixes the
> FIXME, by looking at all interfaces to find the data one. Is it correct ?

It's looking better, but what I'd rather see is code scanning the
CDC descriptors (see "usbnet").  The deal is that there's actually
no guarantee that there's only one data interface, although that
seems to hold true for many current products.

But unless you're interested in finishing a much-needed rewrite
of that cdc-acm probe() code, this might be a good place to stop.
(Or at least pause!)

- Dave

[PATCH] Re: [linux-usb-devel] 2.6.6-rc1: cdc-acm still (differently) broken

[Colin Leroy]

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

On 16 Apr 2004 at 11h04, David Brownell wrote:

Hi, 

> It's looking better, but what I'd rather see is code scanning the
> CDC descriptors (see "usbnet").  The deal is that there's actually
> no guarantee that there's only one data interface, although that
> seems to hold true for many current products.

Ok... However I don't think i'll find the time to do that right now.
So I think I'll take your advice that it's a good place to pause :)

> But unless you're interested in finishing a much-needed rewrite
> of that cdc-acm probe() code, this might be a good place to stop.
> (Or at least pause!)

Ok, here is the cosmetic patch following the previous one, then :)

HTH,
-- 
Colin

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

--- drivers/usb/class/cdc-acm.c.new	2004-04-16 23:44:14.208234976 +0200
+++ drivers/usb/class/cdc-acm.c	2004-04-16 23:44:45.442486648 +0200
@@ -581,138 +581,138 @@
 
 	dev = interface_to_usbdev (intf);
 
-			cfacm = dev->actconfig;
-	
-			/* We know we're probe()d with the control interface. */
-			ifcom = intf->cur_altsetting;
-
-			/* 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 find
-			 * it
-			 */
-			for (j = 0; j < cfacm->desc.bNumInterfaces; j++) {
-				ifdata = cfacm->interface[j]->cur_altsetting;
-				data = cfacm->interface[j];
-
-				if (ifdata->desc.bInterfaceClass == 10 &&
-				    ifdata->desc.bNumEndpoints == 2) {
-					epctrl = &ifcom->endpoint[0].desc;
-					epread = &ifdata->endpoint[0].desc;
-					epwrite = &ifdata->endpoint[1].desc;
-
-					if ((epctrl->bEndpointAddress & 0x80) != 0x80 ||
-					    (epctrl->bmAttributes & 3) != 3 ||
-					    (epread->bmAttributes & 3) != 2 || 
-					    (epwrite->bmAttributes & 3) != 2 ||
-					    ((epread->bEndpointAddress & 0x80) ^ (epwrite->bEndpointAddress & 0x80)) != 0x80) 
-						goto next_interface;
-
-					dbg("found data interface at %d\n", j);
-					break;
-				} else {
+	cfacm = dev->actconfig;
+
+	/* We know we're probe()d with the control interface. */
+	ifcom = intf->cur_altsetting;
+
+	/* 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 find
+	 * it
+	 */
+	for (j = 0; j < cfacm->desc.bNumInterfaces; j++) {
+		ifdata = cfacm->interface[j]->cur_altsetting;
+		data = cfacm->interface[j];
+
+		if (ifdata->desc.bInterfaceClass == 10 &&
+		    ifdata->desc.bNumEndpoints == 2) {
+			epctrl = &ifcom->endpoint[0].desc;
+			epread = &ifdata->endpoint[0].desc;
+			epwrite = &ifdata->endpoint[1].desc;
+
+			if ((epctrl->bEndpointAddress & 0x80) != 0x80 ||
+			    (epctrl->bmAttributes & 3) != 3 ||
+			    (epread->bmAttributes & 3) != 2 || 
+			    (epwrite->bmAttributes & 3) != 2 ||
+			    ((epread->bEndpointAddress & 0x80) ^ (epwrite->bEndpointAddress & 0x80)) != 0x80) 
+				goto next_interface;
+
+			dbg("found data interface at %d\n", j);
+			break;
+		} else {
 next_interface:
-					ifdata = NULL;
-					data = NULL;
-				}
-			}
-
-			/* there's been a problem */
-			if (!ifdata) {
-				dbg("interface not found (%p)\n", ifdata);
-				return -ENODEV;
-
-			}
-
-			for (minor = 0; minor < ACM_TTY_MINORS && acm_table[minor]; minor++);
-			if (acm_table[minor]) {
-				err("no more free acm devices");
-				return -ENODEV;
-			}
-
-			if (!(acm = kmalloc(sizeof(struct acm), GFP_KERNEL))) {
-				err("out of memory");
-				return -ENOMEM;
-			}
-			memset(acm, 0, sizeof(struct acm));
-
-			ctrlsize = epctrl->wMaxPacketSize;
-			readsize = epread->wMaxPacketSize;
-			acm->writesize = epwrite->wMaxPacketSize;
-			acm->control = intf;
-			acm->data = data;
-			acm->minor = minor;
-			acm->dev = dev;
-
-			acm->bh.func = acm_rx_tasklet;
-			acm->bh.data = (unsigned long) acm;
-			INIT_WORK(&acm->work, acm_softint, acm);
-
-			if (!(buf = kmalloc(ctrlsize + readsize + acm->writesize, GFP_KERNEL))) {
-				err("out of memory");
-				kfree(acm);
-				return -ENOMEM;
-			}
-
-			acm->ctrlurb = usb_alloc_urb(0, GFP_KERNEL);
-			if (!acm->ctrlurb) {
-				err("out of memory");
-				kfree(acm);
-				kfree(buf);
-				return -ENOMEM;
-			}
-			acm->readurb = usb_alloc_urb(0, GFP_KERNEL);
-			if (!acm->readurb) {
-				err("out of memory");
-				usb_free_urb(acm->ctrlurb);
-				kfree(acm);
-				kfree(buf);
-				return -ENOMEM;
-			}
-			acm->writeurb = usb_alloc_urb(0, GFP_KERNEL);
-			if (!acm->writeurb) {
-				err("out of memory");
-				usb_free_urb(acm->readurb);
-				usb_free_urb(acm->ctrlurb);
-				kfree(acm);
-				kfree(buf);
-				return -ENOMEM;
-			}
-
-			usb_fill_int_urb(acm->ctrlurb, dev, usb_rcvintpipe(dev, epctrl->bEndpointAddress),
-				buf, ctrlsize, acm_ctrl_irq, acm, epctrl->bInterval);
-
-			usb_fill_bulk_urb(acm->readurb, dev, usb_rcvbulkpipe(dev, epread->bEndpointAddress),
-				buf += ctrlsize, readsize, acm_read_bulk, acm);
-			acm->readurb->transfer_flags |= URB_NO_FSBR;
-
-			usb_fill_bulk_urb(acm->writeurb, dev, usb_sndbulkpipe(dev, epwrite->bEndpointAddress),
-				buf += readsize, acm->writesize, acm_write_bulk, acm);
-			acm->writeurb->transfer_flags |= URB_NO_FSBR;
-
-			dev_info(&intf->dev, "ttyACM%d: USB ACM device", minor);
-
-			acm_set_control(acm, acm->ctrlout);
-
-			acm->line.speed = cpu_to_le32(9600);
-			acm->line.databits = 8;
-			acm_set_line(acm, &acm->line);
-
-			if ( (j = usb_driver_claim_interface(&acm_driver, data, acm)) != 0) {
-				err("claim failed");
-				usb_free_urb(acm->ctrlurb);
-				usb_free_urb(acm->readurb);
-				usb_free_urb(acm->writeurb);
-				kfree(acm);
-				kfree(buf);
-				return j;
-			} 
-
-			tty_register_device(acm_tty_driver, minor, &intf->dev);
-
-			acm_table[minor] = acm;
-			usb_set_intfdata (intf, acm);
-			return 0;
+			ifdata = NULL;
+			data = NULL;
+		}
+	}
+
+	/* there's been a problem */
+	if (!ifdata) {
+		dbg("interface not found (%p)\n", ifdata);
+		return -ENODEV;
+
+	}
+
+	for (minor = 0; minor < ACM_TTY_MINORS && acm_table[minor]; minor++);
+	if (acm_table[minor]) {
+		err("no more free acm devices");
+		return -ENODEV;
+	}
+
+	if (!(acm = kmalloc(sizeof(struct acm), GFP_KERNEL))) {
+		err("out of memory");
+		return -ENOMEM;
+	}
+	memset(acm, 0, sizeof(struct acm));
+
+	ctrlsize = epctrl->wMaxPacketSize;
+	readsize = epread->wMaxPacketSize;
+	acm->writesize = epwrite->wMaxPacketSize;
+	acm->control = intf;
+	acm->data = data;
+	acm->minor = minor;
+	acm->dev = dev;
+
+	acm->bh.func = acm_rx_tasklet;
+	acm->bh.data = (unsigned long) acm;
+	INIT_WORK(&acm->work, acm_softint, acm);
+
+	if (!(buf = kmalloc(ctrlsize + readsize + acm->writesize, GFP_KERNEL))) {
+		err("out of memory");
+		kfree(acm);
+		return -ENOMEM;
+	}
+
+	acm->ctrlurb = usb_alloc_urb(0, GFP_KERNEL);
+	if (!acm->ctrlurb) {
+		err("out of memory");
+		kfree(acm);
+		kfree(buf);
+		return -ENOMEM;
+	}
+	acm->readurb = usb_alloc_urb(0, GFP_KERNEL);
+	if (!acm->readurb) {
+		err("out of memory");
+		usb_free_urb(acm->ctrlurb);
+		kfree(acm);
+		kfree(buf);
+		return -ENOMEM;
+	}
+	acm->writeurb = usb_alloc_urb(0, GFP_KERNEL);
+	if (!acm->writeurb) {
+		err("out of memory");
+		usb_free_urb(acm->readurb);
+		usb_free_urb(acm->ctrlurb);
+		kfree(acm);
+		kfree(buf);
+		return -ENOMEM;
+	}
+
+	usb_fill_int_urb(acm->ctrlurb, dev, usb_rcvintpipe(dev, epctrl->bEndpointAddress),
+		buf, ctrlsize, acm_ctrl_irq, acm, epctrl->bInterval);
+
+	usb_fill_bulk_urb(acm->readurb, dev, usb_rcvbulkpipe(dev, epread->bEndpointAddress),
+		buf += ctrlsize, readsize, acm_read_bulk, acm);
+	acm->readurb->transfer_flags |= URB_NO_FSBR;
+
+	usb_fill_bulk_urb(acm->writeurb, dev, usb_sndbulkpipe(dev, epwrite->bEndpointAddress),
+		buf += readsize, acm->writesize, acm_write_bulk, acm);
+	acm->writeurb->transfer_flags |= URB_NO_FSBR;
+
+	dev_info(&intf->dev, "ttyACM%d: USB ACM device", minor);
+
+	acm_set_control(acm, acm->ctrlout);
+
+	acm->line.speed = cpu_to_le32(9600);
+	acm->line.databits = 8;
+	acm_set_line(acm, &acm->line);
+
+	if ( (j = usb_driver_claim_interface(&acm_driver, data, acm)) != 0) {
+		err("claim failed");
+		usb_free_urb(acm->ctrlurb);
+		usb_free_urb(acm->readurb);
+		usb_free_urb(acm->writeurb);
+		kfree(acm);
+		kfree(buf);
+		return j;
+	} 
+
+	tty_register_device(acm_tty_driver, minor, &intf->dev);
+
+	acm_table[minor] = acm;
+	usb_set_intfdata (intf, acm);
+	return 0;
 }
 
 static void acm_disconnect(struct usb_interface *intf)

Re: [PATCH] Re: [linux-usb-devel] 2.6.6-rc1: cdc-acm still (differently) broken

[Colin Leroy]

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

On 16 Apr 2004 at 23h04, Colin Leroy wrote:

Hi, 

> Ok, here is the cosmetic patch following the previous one, then :)

Uhm, previous patch is broken.
Here are the two again.

-- 
Colin

[-- Attachment #2: 1_cdc-acm.patch --]
[-- Type: application/octet-stream, Size: 3474 bytes --]

--- drivers/usb/class/cdc-acm.c.orig	2004-04-15 20:04:47.000000000 +0200
+++ drivers/usb/class/cdc-acm.c	2004-04-16 12:21:40.120865512 +0200
@@ -581,45 +581,47 @@
 
 	dev = interface_to_usbdev (intf);
 
-		cfacm = dev->actconfig;
-
-		for (j = 0; j < cfacm->desc.bNumInterfaces - 1; j++) {
-		    
-			if (usb_interface_claimed(cfacm->interface[j]) ||
-			    usb_interface_claimed(cfacm->interface[j + 1]))
-			continue;
+			cfacm = dev->actconfig;
+	
+			/* We know we're probe()d with the control interface. */
+			ifcom = intf->cur_altsetting;
 
-			/* We know we're probe()d with the control interface.
-			 * FIXME ACM doesn't guarantee the data interface is
+			/* 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.
+			 * is there it's not for call management ... so find
+			 * it
 			 */
-			ifcom = intf->cur_altsetting;
-			if (intf == cfacm->interface[j]) {
-				ifdata = cfacm->interface[j + 1]->cur_altsetting;
-				data = cfacm->interface[j + 1];
-			} else if (intf == cfacm->interface[j + 1]) {
+			for (j = 0; j < cfacm->desc.bNumInterfaces; j++) {
 				ifdata = cfacm->interface[j]->cur_altsetting;
 				data = cfacm->interface[j];
-			} else
-				continue;
-
-			if (ifdata->desc.bInterfaceClass != 10 || ifdata->desc.bNumEndpoints < 2)
-				continue;
-
-			epctrl = &ifcom->endpoint[0].desc;
-			epread = &ifdata->endpoint[0].desc;
-			epwrite = &ifdata->endpoint[1].desc;
-
-			if ((epctrl->bEndpointAddress & 0x80) != 0x80 || (epctrl->bmAttributes & 3) != 3 ||
-			   (epread->bmAttributes & 3) != 2 || (epwrite->bmAttributes & 3) != 2 ||
-			   ((epread->bEndpointAddress & 0x80) ^ (epwrite->bEndpointAddress & 0x80)) != 0x80)
-				continue;
-
-			if ((epread->bEndpointAddress & 0x80) != 0x80) {
-				epread = &ifdata->endpoint[1].desc;
-				epwrite = &ifdata->endpoint[0].desc;
+
+				if (ifdata->desc.bInterfaceClass == 10 &&
+				    ifdata->desc.bNumEndpoints == 2) {
+					epctrl = &ifcom->endpoint[0].desc;
+					epread = &ifdata->endpoint[0].desc;
+					epwrite = &ifdata->endpoint[1].desc;
+
+					if ((epctrl->bEndpointAddress & 0x80) != 0x80 ||
+					    (epctrl->bmAttributes & 3) != 3 ||
+					    (epread->bmAttributes & 3) != 2 || 
+					    (epwrite->bmAttributes & 3) != 2 ||
+					    ((epread->bEndpointAddress & 0x80) ^ (epwrite->bEndpointAddress & 0x80)) != 0x80) 
+						goto next_interface;
+
+					dbg("found data interface at %d\n", j);
+					break;
+				} else {
+next_interface:
+					ifdata = NULL;
+					data = NULL;
+				}
+			}
+
+			/* there's been a problem */
+			if (!ifdata) {
+				dbg("interface not found (%p)\n", ifdata);
+				return -ENODEV;
+
 			}
 
 			for (minor = 0; minor < ACM_TTY_MINORS && acm_table[minor]; minor++);
@@ -696,16 +698,21 @@
 			acm->line.databits = 8;
 			acm_set_line(acm, &acm->line);
 
-			usb_driver_claim_interface(&acm_driver, data, acm);
+			if ( (j = usb_driver_claim_interface(&acm_driver, data, acm)) != 0) {
+				err("claim failed");
+				usb_free_urb(acm->ctrlurb);
+				usb_free_urb(acm->readurb);
+				usb_free_urb(acm->writeurb);
+				kfree(acm);
+				kfree(buf);
+				return j;
+			} 
 
 			tty_register_device(acm_tty_driver, minor, &intf->dev);
 
 			acm_table[minor] = acm;
 			usb_set_intfdata (intf, acm);
 			return 0;
-		}
-
-	return -EIO;
 }
 
 static void acm_disconnect(struct usb_interface *intf)

[-- Attachment #3: 2_cdc-acm-cosmetic.patch --]
[-- Type: application/octet-stream, Size: 7663 bytes --]

--- drivers/usb/class/cdc-acm.c.new	2004-04-16 23:44:14.208234976 +0200
+++ drivers/usb/class/cdc-acm.c	2004-04-16 23:44:45.442486648 +0200
@@ -581,138 +581,138 @@
 
 	dev = interface_to_usbdev (intf);
 
-			cfacm = dev->actconfig;
-	
-			/* We know we're probe()d with the control interface. */
-			ifcom = intf->cur_altsetting;
-
-			/* 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 find
-			 * it
-			 */
-			for (j = 0; j < cfacm->desc.bNumInterfaces; j++) {
-				ifdata = cfacm->interface[j]->cur_altsetting;
-				data = cfacm->interface[j];
-
-				if (ifdata->desc.bInterfaceClass == 10 &&
-				    ifdata->desc.bNumEndpoints == 2) {
-					epctrl = &ifcom->endpoint[0].desc;
-					epread = &ifdata->endpoint[0].desc;
-					epwrite = &ifdata->endpoint[1].desc;
-
-					if ((epctrl->bEndpointAddress & 0x80) != 0x80 ||
-					    (epctrl->bmAttributes & 3) != 3 ||
-					    (epread->bmAttributes & 3) != 2 || 
-					    (epwrite->bmAttributes & 3) != 2 ||
-					    ((epread->bEndpointAddress & 0x80) ^ (epwrite->bEndpointAddress & 0x80)) != 0x80) 
-						goto next_interface;
-
-					dbg("found data interface at %d\n", j);
-					break;
-				} else {
+	cfacm = dev->actconfig;
+
+	/* We know we're probe()d with the control interface. */
+	ifcom = intf->cur_altsetting;
+
+	/* 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 find
+	 * it
+	 */
+	for (j = 0; j < cfacm->desc.bNumInterfaces; j++) {
+		ifdata = cfacm->interface[j]->cur_altsetting;
+		data = cfacm->interface[j];
+
+		if (ifdata->desc.bInterfaceClass == 10 &&
+		    ifdata->desc.bNumEndpoints == 2) {
+			epctrl = &ifcom->endpoint[0].desc;
+			epread = &ifdata->endpoint[0].desc;
+			epwrite = &ifdata->endpoint[1].desc;
+
+			if ((epctrl->bEndpointAddress & 0x80) != 0x80 ||
+			    (epctrl->bmAttributes & 3) != 3 ||
+			    (epread->bmAttributes & 3) != 2 || 
+			    (epwrite->bmAttributes & 3) != 2 ||
+			    ((epread->bEndpointAddress & 0x80) ^ (epwrite->bEndpointAddress & 0x80)) != 0x80) 
+				goto next_interface;
+
+			dbg("found data interface at %d\n", j);
+			break;
+		} else {
 next_interface:
-					ifdata = NULL;
-					data = NULL;
-				}
-			}
-
-			/* there's been a problem */
-			if (!ifdata) {
-				dbg("interface not found (%p)\n", ifdata);
-				return -ENODEV;
-
-			}
-
-			for (minor = 0; minor < ACM_TTY_MINORS && acm_table[minor]; minor++);
-			if (acm_table[minor]) {
-				err("no more free acm devices");
-				return -ENODEV;
-			}
-
-			if (!(acm = kmalloc(sizeof(struct acm), GFP_KERNEL))) {
-				err("out of memory");
-				return -ENOMEM;
-			}
-			memset(acm, 0, sizeof(struct acm));
-
-			ctrlsize = epctrl->wMaxPacketSize;
-			readsize = epread->wMaxPacketSize;
-			acm->writesize = epwrite->wMaxPacketSize;
-			acm->control = intf;
-			acm->data = data;
-			acm->minor = minor;
-			acm->dev = dev;
-
-			acm->bh.func = acm_rx_tasklet;
-			acm->bh.data = (unsigned long) acm;
-			INIT_WORK(&acm->work, acm_softint, acm);
-
-			if (!(buf = kmalloc(ctrlsize + readsize + acm->writesize, GFP_KERNEL))) {
-				err("out of memory");
-				kfree(acm);
-				return -ENOMEM;
-			}
-
-			acm->ctrlurb = usb_alloc_urb(0, GFP_KERNEL);
-			if (!acm->ctrlurb) {
-				err("out of memory");
-				kfree(acm);
-				kfree(buf);
-				return -ENOMEM;
-			}
-			acm->readurb = usb_alloc_urb(0, GFP_KERNEL);
-			if (!acm->readurb) {
-				err("out of memory");
-				usb_free_urb(acm->ctrlurb);
-				kfree(acm);
-				kfree(buf);
-				return -ENOMEM;
-			}
-			acm->writeurb = usb_alloc_urb(0, GFP_KERNEL);
-			if (!acm->writeurb) {
-				err("out of memory");
-				usb_free_urb(acm->readurb);
-				usb_free_urb(acm->ctrlurb);
-				kfree(acm);
-				kfree(buf);
-				return -ENOMEM;
-			}
-
-			usb_fill_int_urb(acm->ctrlurb, dev, usb_rcvintpipe(dev, epctrl->bEndpointAddress),
-				buf, ctrlsize, acm_ctrl_irq, acm, epctrl->bInterval);
-
-			usb_fill_bulk_urb(acm->readurb, dev, usb_rcvbulkpipe(dev, epread->bEndpointAddress),
-				buf += ctrlsize, readsize, acm_read_bulk, acm);
-			acm->readurb->transfer_flags |= URB_NO_FSBR;
-
-			usb_fill_bulk_urb(acm->writeurb, dev, usb_sndbulkpipe(dev, epwrite->bEndpointAddress),
-				buf += readsize, acm->writesize, acm_write_bulk, acm);
-			acm->writeurb->transfer_flags |= URB_NO_FSBR;
-
-			dev_info(&intf->dev, "ttyACM%d: USB ACM device", minor);
-
-			acm_set_control(acm, acm->ctrlout);
-
-			acm->line.speed = cpu_to_le32(9600);
-			acm->line.databits = 8;
-			acm_set_line(acm, &acm->line);
-
-			if ( (j = usb_driver_claim_interface(&acm_driver, data, acm)) != 0) {
-				err("claim failed");
-				usb_free_urb(acm->ctrlurb);
-				usb_free_urb(acm->readurb);
-				usb_free_urb(acm->writeurb);
-				kfree(acm);
-				kfree(buf);
-				return j;
-			} 
-
-			tty_register_device(acm_tty_driver, minor, &intf->dev);
-
-			acm_table[minor] = acm;
-			usb_set_intfdata (intf, acm);
-			return 0;
+			ifdata = NULL;
+			data = NULL;
+		}
+	}
+
+	/* there's been a problem */
+	if (!ifdata) {
+		dbg("interface not found (%p)\n", ifdata);
+		return -ENODEV;
+
+	}
+
+	for (minor = 0; minor < ACM_TTY_MINORS && acm_table[minor]; minor++);
+	if (acm_table[minor]) {
+		err("no more free acm devices");
+		return -ENODEV;
+	}
+
+	if (!(acm = kmalloc(sizeof(struct acm), GFP_KERNEL))) {
+		err("out of memory");
+		return -ENOMEM;
+	}
+	memset(acm, 0, sizeof(struct acm));
+
+	ctrlsize = epctrl->wMaxPacketSize;
+	readsize = epread->wMaxPacketSize;
+	acm->writesize = epwrite->wMaxPacketSize;
+	acm->control = intf;
+	acm->data = data;
+	acm->minor = minor;
+	acm->dev = dev;
+
+	acm->bh.func = acm_rx_tasklet;
+	acm->bh.data = (unsigned long) acm;
+	INIT_WORK(&acm->work, acm_softint, acm);
+
+	if (!(buf = kmalloc(ctrlsize + readsize + acm->writesize, GFP_KERNEL))) {
+		err("out of memory");
+		kfree(acm);
+		return -ENOMEM;
+	}
+
+	acm->ctrlurb = usb_alloc_urb(0, GFP_KERNEL);
+	if (!acm->ctrlurb) {
+		err("out of memory");
+		kfree(acm);
+		kfree(buf);
+		return -ENOMEM;
+	}
+	acm->readurb = usb_alloc_urb(0, GFP_KERNEL);
+	if (!acm->readurb) {
+		err("out of memory");
+		usb_free_urb(acm->ctrlurb);
+		kfree(acm);
+		kfree(buf);
+		return -ENOMEM;
+	}
+	acm->writeurb = usb_alloc_urb(0, GFP_KERNEL);
+	if (!acm->writeurb) {
+		err("out of memory");
+		usb_free_urb(acm->readurb);
+		usb_free_urb(acm->ctrlurb);
+		kfree(acm);
+		kfree(buf);
+		return -ENOMEM;
+	}
+
+	usb_fill_int_urb(acm->ctrlurb, dev, usb_rcvintpipe(dev, epctrl->bEndpointAddress),
+		buf, ctrlsize, acm_ctrl_irq, acm, epctrl->bInterval);
+
+	usb_fill_bulk_urb(acm->readurb, dev, usb_rcvbulkpipe(dev, epread->bEndpointAddress),
+		buf += ctrlsize, readsize, acm_read_bulk, acm);
+	acm->readurb->transfer_flags |= URB_NO_FSBR;
+
+	usb_fill_bulk_urb(acm->writeurb, dev, usb_sndbulkpipe(dev, epwrite->bEndpointAddress),
+		buf += readsize, acm->writesize, acm_write_bulk, acm);
+	acm->writeurb->transfer_flags |= URB_NO_FSBR;
+
+	dev_info(&intf->dev, "ttyACM%d: USB ACM device", minor);
+
+	acm_set_control(acm, acm->ctrlout);
+
+	acm->line.speed = cpu_to_le32(9600);
+	acm->line.databits = 8;
+	acm_set_line(acm, &acm->line);
+
+	if ( (j = usb_driver_claim_interface(&acm_driver, data, acm)) != 0) {
+		err("claim failed");
+		usb_free_urb(acm->ctrlurb);
+		usb_free_urb(acm->readurb);
+		usb_free_urb(acm->writeurb);
+		kfree(acm);
+		kfree(buf);
+		return j;
+	} 
+
+	tty_register_device(acm_tty_driver, minor, &intf->dev);
+
+	acm_table[minor] = acm;
+	usb_set_intfdata (intf, acm);
+	return 0;
 }
 
 static void acm_disconnect(struct usb_interface *intf)

Re: [linux-usb-devel] 2.6.6-rc1: cdc-acm still (differently) broken

[Alan Stern]

On Thu, 15 Apr 2004, Colin Leroy wrote:

> Like this one? It works. I'm a bit wondering, however, how comes 
> usb_interface_claimed() returns true, and the check in 
> usb_driver_claim_interface() passes?

While an interface is being probed, usb_interface_claimed() will always
return true for it.  That's because the probing code sets
interface->dev.driver (which is what usb_interface_claimed() tests) before
calling the probe function.  If the probe fails then interface->dev.driver
will be reset.  This all happens inside bus_match() in drivers/base/bus.c.

For the same reason, the test of (dev->driver) in 
usb_driver_claim_interface() -- that is the test you meant, isn't it? -- 
passes (i.e., yields a true value).

Alan Stern