Re: [PATCH RFC] cdc_acm: handle shared control/data interface

[Tilman Schmidt <tilman@xxxxxxx>]

On Sun, 28 Dec 2008 15:38:25 +0100, Oliver Neukum wrote:
> You cannot do this this way. Sharing an interface is against the spec.

I see. Thanks for that information. So a device advertising an
interface with:

      bInterfaceNumber        2
      bNumEndpoints           3
      bInterfaceClass         2 Communications
      bInterfaceSubClass      2 Abstract (modem)
      bInterfaceProtocol      1 AT-commands (v.25ter)
      CDC Union:
        bMasterInterface        2
        bSlaveInterface         2 

would not be conforming to the USB standard?

> If you do a generic work around you need to verify the endpoints are
> correct.

You mean something like this?

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index d50a99f..7ce549a 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -896,8 +896,8 @@ static int acm_probe (struct usb_interface *intf,
 	struct usb_interface *control_interface;
 	struct usb_interface *data_interface;
 	struct usb_endpoint_descriptor *epctrl;
-	struct usb_endpoint_descriptor *epread;
-	struct usb_endpoint_descriptor *epwrite;
+	struct usb_endpoint_descriptor *epread = NULL;
+	struct usb_endpoint_descriptor *epwrite = NULL;
 	struct usb_device *usb_dev = interface_to_usbdev(intf);
 	struct acm *acm;
 	int minor;
@@ -1028,29 +1028,38 @@ skip_normal_probe:
 	if (intf != control_interface)
 		return -ENODEV;
 	
-	if (usb_interface_claimed(data_interface)) { /* valid in this context */
+	if (intf != data_interface && usb_interface_claimed(data_interface)) { /* valid in this context */
 		dev_dbg(&intf->dev,"The data interface isn't available\n");
 		return -EBUSY;
 	}
 
 
-	if (data_interface->cur_altsetting->desc.bNumEndpoints < 2)
-		return -EINVAL;
-
 	epctrl = &control_interface->cur_altsetting->endpoint[0].desc;
-	epread = &data_interface->cur_altsetting->endpoint[0].desc;
-	epwrite = &data_interface->cur_altsetting->endpoint[1].desc;
-
-
-	/* workaround for switched endpoints */
-	if (!usb_endpoint_dir_in(epread)) {
-		/* descriptors are swapped */
-		struct usb_endpoint_descriptor *t;
-		dev_dbg(&intf->dev,"The data interface has switched endpoints\n");
-		
-		t = epread;
-		epread = epwrite;
-		epwrite = t;
+
+	/* find data endpoints */
+	i = 0;
+	if (data_interface == control_interface) {
+		dev_dbg(&intf->dev,
+			"Shared interface. That's against the spec.\n");
+		i++;		/* skip control EP */
+	}
+	while (i < data_interface->cur_altsetting->desc.bNumEndpoints) {
+		struct usb_endpoint_descriptor *ep
+			= &data_interface->cur_altsetting->endpoint[i++].desc;
+		if (usb_endpoint_xfer_bulk(ep)) {
+			if (epread == NULL && usb_endpoint_dir_in(ep))
+				epread = ep;
+			else if (epwrite == NULL && usb_endpoint_dir_out(ep))
+				epwrite = ep;
+		}
+	}
+	if (epread == NULL) {
+		err("missing input pipe\n");
+		return -EINVAL;
+	}
+	if (epwrite == NULL) {
+		err("missing output pipe\n");
+		return -EINVAL;
 	}
 	dbg("interfaces are valid");
 	for (minor = 0; minor < ACM_TTY_MINORS && acm_table[minor]; minor++);


Thanks,
Tilman
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html