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

[Colin Leroy <colin@colino.net>]

[-- 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)