[RFC PATCH] qmi_wwan: set correct altsetting for Gobi 1K devices

[RFC PATCH] qmi_wwan: set correct altsetting for Gobi 1K devices

[Dan Williams]

With Gobi 1K devices, USB interface #3's altsetting is 0 by default, but
altsetting 0 only provides one interrupt endpoint and is not sufficent
for QMI.  Altsetting 1 provides all 3 endpoints required for qmi_wwan
and works with QMI.

IIRC the altsetting used to be set by qcserial back before we made
qcserial stop touching interfaces that qmi_wwan was going to use.  But
now that qcserial only touches the modem interface, we need qmi_wwan to
set the correct altsetting on the QMI interface.

The attached patch works for my Gobi1K (and should work for all other
1Ks too) but seems somewhat ugly.  What approach should we take?
Basically, we need to know that a device is Gobi1K at probe time so we
can set the right altsetting on it.

Gobi 1K layout for intf#3 is:

    Interface Descriptor:  255/255/255
      bInterfaceNumber        3
      bAlternateSetting       0
      Endpoint Descriptor:  Interrupt IN
    Interface Descriptor:  255/255/255
      bInterfaceNumber        3
      bAlternateSetting       1
      Endpoint Descriptor:  Interrupt IN
      Endpoint Descriptor:  Bulk IN
      Endpoint Descriptor:  Bulk OUT

Dan

---
diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
index efb5c7c..50e1b7c 100644
--- a/drivers/net/usb/qmi_wwan.c
+++ b/drivers/net/usb/qmi_wwan.c
@@ -43,6 +43,9 @@
  * commands on a serial interface
  */
 
+/* Quirks for the usbnet 'struct driver_info' data field */
+#define QUIRK_GOBI_1K  (1 << 0)   /* QMI interface requires altsetting 1 */
+
 /* driver specific data */
 struct qmi_wwan_state {
 	struct usb_driver *subdriver;
@@ -326,6 +329,15 @@ static const struct driver_info	qmi_wwan_info = {
 	.manage_power	= qmi_wwan_manage_power,
 };
 
+static const struct driver_info	gobi1k_info = {
+	.description	= "WWAN/QMI device",
+	.flags		= FLAG_WWAN,
+	.bind		= qmi_wwan_bind,
+	.unbind		= qmi_wwan_unbind,
+	.manage_power	= qmi_wwan_manage_power,
+	.data           = QUIRK_GOBI_1K,
+};
+
 #define HUAWEI_VENDOR_ID	0x12D1
 
 /* map QMI/wwan function by a fixed interface number */
@@ -335,7 +347,8 @@ static const struct driver_info	qmi_wwan_info = {
 
 /* Gobi 1000 QMI/wwan interface number is 3 according to qcserial */
 #define QMI_GOBI1K_DEVICE(vend, prod) \
-	QMI_FIXED_INTF(vend, prod, 3)
+	USB_DEVICE_INTERFACE_NUMBER(vend, prod, 3), \
+	.driver_info = (unsigned long)&gobi1k_info
 
 /* Gobi 2000/3000 QMI/wwan interface number is 0 according to qcserial */
 #define QMI_GOBI_DEVICE(vend, prod) \
@@ -541,6 +554,9 @@ MODULE_DEVICE_TABLE(usb, products);
 static int qmi_wwan_probe(struct usb_interface *intf, const struct usb_device_id *prod)
 {
 	struct usb_device_id *id = (struct usb_device_id *)prod;
+	struct driver_info *info;
+	u8 intf_num = intf->cur_altsetting->desc.bInterfaceNumber;
+	int retval;
 
 	/* Workaround to enable dynamic IDs.  This disables usbnet
 	 * blacklisting functionality.  Which, if required, can be
@@ -552,6 +568,20 @@ static int qmi_wwan_probe(struct usb_interface *intf, const struct usb_device_id
 		id->driver_info = (unsigned long)&qmi_wwan_info;
 	}
 
+	info = (struct driver_info *) id->driver_info;
+	if (info->data & QUIRK_GOBI_1K) {
+		/* Gobi 1K's QMI interface is always USB interface #3 */
+		BUG_ON(intf_num != 3);
+
+		retval = usb_set_interface(interface_to_usbdev (intf),
+					   intf_num, 1);
+		if (retval < 0) {
+			dev_err(&intf->dev, "Failed to set altsetting 1: %d\n",
+			        retval);
+			return -ENODEV;
+		}
+	}
+
 	return usbnet_probe(intf, id);
 }
 

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

Re: [RFC PATCH] qmi_wwan: set correct altsetting for Gobi 1K devices

[Bjørn Mork]

Dan Williams <dcbw@redhat.com> wrote:

>With Gobi 1K devices, USB interface #3's altsetting is 0 by default,
>but
>altsetting 0 only provides one interrupt endpoint and is not sufficent
>for QMI.  Altsetting 1 provides all 3 endpoints required for qmi_wwan
>and works with QMI.
>
>IIRC the altsetting used to be set by qcserial back before we made
>qcserial stop touching interfaces that qmi_wwan was going to use.  But
>now that qcserial only touches the modem interface, we need qmi_wwan to
>set the correct altsetting on the QMI interface.

No, I broke this when I tried to be smart and merged the 1 and 2 interface probes. That's the second time I break probing of these devices by making too many unfounded assumptions. 

This used to work because usbnet_get_endpoints selects the first usable altsetting. It doesn't work anymore because qmi_wwan checks the number of endpoints before calling usbnet_get_endpoints.

>The attached patch works for my Gobi1K (and should work for all other
>1Ks too) but seems somewhat ugly.  What approach should we take?
>Basically, we need to know that a device is Gobi1K at probe time so we
>can set the right altsetting on it.
>
>Gobi 1K layout for intf#3 is:
>
>    Interface Descriptor:  255/255/255
>      bInterfaceNumber        3
>      bAlternateSetting       0
>      Endpoint Descriptor:  Interrupt IN
>    Interface Descriptor:  255/255/255
>      bInterfaceNumber        3
>      bAlternateSetting       1
>      Endpoint Descriptor:  Interrupt IN
>      Endpoint Descriptor:  Bulk IN
>      Endpoint Descriptor:  Bulk OUT

Ouch. I did not know this. I should get one of these devices, I guess. ..


>Dan
>
>---
>diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
>index efb5c7c..50e1b7c 100644
>--- a/drivers/net/usb/qmi_wwan.c
>+++ b/drivers/net/usb/qmi_wwan.c
>@@ -43,6 +43,9 @@
>  * commands on a serial interface
>  */
> 
>+/* Quirks for the usbnet 'struct driver_info' data field */
>+#define QUIRK_GOBI_1K  (1 << 0)   /* QMI interface requires altsetting
>1 */
>+
> /* driver specific data */
> struct qmi_wwan_state {
> 	struct usb_driver *subdriver;
>@@ -326,6 +329,15 @@ static const struct driver_info	qmi_wwan_info = {
> 	.manage_power	= qmi_wwan_manage_power,
> };
> 
>+static const struct driver_info	gobi1k_info = {
>+	.description	= "WWAN/QMI device",
>+	.flags		= FLAG_WWAN,
>+	.bind		= qmi_wwan_bind,
>+	.unbind		= qmi_wwan_unbind,
>+	.manage_power	= qmi_wwan_manage_power,
>+	.data           = QUIRK_GOBI_1K,
>+};
>+
> #define HUAWEI_VENDOR_ID	0x12D1
> 
> /* map QMI/wwan function by a fixed interface number */
>@@ -335,7 +347,8 @@ static const struct driver_info	qmi_wwan_info = {
> 
> /* Gobi 1000 QMI/wwan interface number is 3 according to qcserial */
> #define QMI_GOBI1K_DEVICE(vend, prod) \
>-	QMI_FIXED_INTF(vend, prod, 3)
>+	USB_DEVICE_INTERFACE_NUMBER(vend, prod, 3), \
>+	.driver_info = (unsigned long)&gobi1k_info
> 
>/* Gobi 2000/3000 QMI/wwan interface number is 0 according to qcserial
>*/
> #define QMI_GOBI_DEVICE(vend, prod) \
>@@ -541,6 +554,9 @@ MODULE_DEVICE_TABLE(usb, products);
>static int qmi_wwan_probe(struct usb_interface *intf, const struct
>usb_device_id *prod)
> {
> 	struct usb_device_id *id = (struct usb_device_id *)prod;
>+	struct driver_info *info;
>+	u8 intf_num = intf->cur_altsetting->desc.bInterfaceNumber;
>+	int retval;
> 
> 	/* Workaround to enable dynamic IDs.  This disables usbnet
> 	 * blacklisting functionality.  Which, if required, can be
>@@ -552,6 +568,20 @@ static int qmi_wwan_probe(struct usb_interface
>*intf, const struct usb_device_id
> 		id->driver_info = (unsigned long)&qmi_wwan_info;
> 	}
> 
>+	info = (struct driver_info *) id->driver_info;
>+	if (info->data & QUIRK_GOBI_1K) {
>+		/* Gobi 1K's QMI interface is always USB interface #3 */
>+		BUG_ON(intf_num != 3);

Definitely not. We return ENODEV if the probe logic fails for whatever reason. 

>+		retval = usb_set_interface(interface_to_usbdev (intf),
>+					   intf_num, 1);
>+		if (retval < 0) {
>+			dev_err(&intf->dev, "Failed to set altsetting 1: %d\n",
>+			        retval);
>+			return -ENODEV;
>+		}
>+	}

Risking that I am trying to be too smart again. .. But I really think we can fix this in a more generic way by making the probe behave more like it did before. How about just letting it continue without erring out until it has called usbnet_get_endpoints? That should work without needing any quirks. 


Bjørn

Re: [RFC PATCH] qmi_wwan: set correct altsetting for Gobi 1K devices

[Bjørn Mork]

Bjørn Mork <bjorn@mork.no> writes:

>
> Risking that I am trying to be too smart again. .. But I really think
> we can fix this in a more generic way by making the probe behave more
> like it did before. How about just letting it continue without erring
> out until it has called usbnet_get_endpoints? That should work without
> needing any quirks.

I was thinking about something like this patch, dropping both the
unnecessary verification of bNumEndpoints and of the CDC functional
descriptors.  If we find a CDC Union, then fine - we use it to locate
the data interface. If we find a CDC Ether, then fine - we use it to set
the MAC address.

This should end up calling usbnet_get_endpoints (first thing
qmi_wwan_register_subdriver does), and succeed if any data interface
altsetting have the necessary endpoints.

There really are few reasons to do any strict verification step in the
probe in this driver, because there really are no usable markers on the
functions. That's why the driver does explicit interface matching in
the first place.  So it makes sense to allow the probe to succeed if
collecting the 3 required endpoints is at all possible.

It will of course succeed on a number of unsupported interfaces if
dynamical IDs are used.  But with nothing to differentiate a QMI/wwan
function from other functions, there is really nothing we can do to
prevent that. If we are to support dynamic IDs, then there will be false
positives.  The user can manually unbind the driver from unwanted
matches in this case.

Only build tested for now. I will test on the devices I have before
submitting, if this works for you:


diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
index efb5c7c..1cfa80c 100644
--- a/drivers/net/usb/qmi_wwan.c
+++ b/drivers/net/usb/qmi_wwan.c
@@ -139,18 +139,11 @@ static int qmi_wwan_bind(struct usbnet *dev, struct usb_interface *intf)
 
 	BUILD_BUG_ON((sizeof(((struct usbnet *)0)->data) < sizeof(struct qmi_wwan_state)));
 
-	/* control and data is shared? */
-	if (intf->cur_altsetting->desc.bNumEndpoints == 3) {
-		info->control = intf;
-		info->data = intf;
-		goto shared;
-	}
-
-	/* else require a single interrupt status endpoint on control intf */
-	if (intf->cur_altsetting->desc.bNumEndpoints != 1)
-		goto err;
+	/* set up initial state */
+	info->control = intf;
+	info->data = intf;
 
-	/* and a number of CDC descriptors */
+	/* some vendors add CDC descriptors */
 	while (len > 3) {
 		struct usb_descriptor_header *h = (void *)buf;
 
@@ -207,25 +200,17 @@ next_desc:
 		buf += h->bLength;
 	}
 
-	/* did we find all the required ones? */
-	if (!(found & (1 << USB_CDC_HEADER_TYPE)) ||
-	    !(found & (1 << USB_CDC_UNION_TYPE))) {
-		dev_err(&intf->dev, "CDC functional descriptors missing\n");
-		goto err;
-	}
-
-	/* verify CDC Union */
-	if (desc->bInterfaceNumber != cdc_union->bMasterInterface0) {
-		dev_err(&intf->dev, "bogus CDC Union: master=%u\n", cdc_union->bMasterInterface0);
-		goto err;
-	}
-
-	/* need to save these for unbind */
-	info->control = intf;
-	info->data = usb_ifnum_to_if(dev->udev,	cdc_union->bSlaveInterface0);
-	if (!info->data) {
-		dev_err(&intf->dev, "bogus CDC Union: slave=%u\n", cdc_union->bSlaveInterface0);
-		goto err;
+	/* using two interfaces if we found a CDC Union */
+	if (cdc_union) {
+		if (desc->bInterfaceNumber != cdc_union->bMasterInterface0) {
+			dev_err(&intf->dev, "bogus CDC Union: master=%u\n", cdc_union->bMasterInterface0);
+			goto err;
+		}
+		info->data = usb_ifnum_to_if(dev->udev,	cdc_union->bSlaveInterface0);
+		if (!info->data) {
+			dev_err(&intf->dev, "bogus CDC Union: slave=%u\n", cdc_union->bSlaveInterface0);
+			goto err;
+		}
 	}
 
 	/* errors aren't fatal - we can live with the dynamic address */
@@ -234,12 +219,13 @@ next_desc:
 		usbnet_get_ethernet_addr(dev, cdc_ether->iMACAddress);
 	}
 
-	/* claim data interface and set it up */
-	status = usb_driver_claim_interface(driver, info->data, dev);
-	if (status < 0)
-		goto err;
+	/* claim separate data interface? */
+	if (info->control != info->data) {
+		status = usb_driver_claim_interface(driver, info->data, dev);
+		if (status < 0)
+			goto err;
+	}
 
-shared:
 	status = qmi_wwan_register_subdriver(dev);
 	if (status < 0 && info->control != info->data) {
 		usb_set_intfdata(info->data, NULL);