On Wed, 2013-03-13 at 13:25 +0100, Bjørn Mork wrote:
> commit bd877e4 ("net: qmi_wwan: use a single bind function for
> all device types") made Gobi 1K devices fail probing.
>
> Using the number of endpoints in the default altsetting to decide
> whether the function use one or two interfaces is wrong. Other
> altsettings may provide more endpoints.
>
> 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. 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
>
> Prior to commit bd877e4, we would call usbnet_get_endpoints
> before giving up finding enough endpoints. Removing the early
> endpoint number test and the strict functional descriptor
> requirement allow qmi_wwan_bind to continue until
> usbnet_get_endpoints has made the final attempt to collect
> endpoints. This restores the behaviour from before commit
> bd877e4 without losing the added benefit of using a single bind
> function.
>
> The driver has always required a CDC Union functional descriptor
> for two-interface functions. Using the existence of this
> descriptor to detect two-interface functions is the logically
> correct method.
>
> Reported-by: Dan Williams <dcbw@xxxxxxxxxx>
> Signed-off-by: Bjørn Mork
Works on my UML290, Gobi3K, Gobi1K, Gobi2K, and E362.
Tested-by: Dan Williams <dcbw@xxxxxxxxxx>
> <bjorn@xxxxxxx>
> ---
> Dan,
>
> could you test this on a Gobi 1K device? It should fix the problem,
> but I'd like to verify my assumptions for once :)
>
> This needs to go to stable-3.8 as well.
>
>
> Bjørn
>
>
> drivers/net/usb/qmi_wwan.c | 49 +++++++++++++++-----------------------------
> 1 file changed, 16 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
> index efb5c7c..968d5d5 100644
> --- a/drivers/net/usb/qmi_wwan.c
> +++ b/drivers/net/usb/qmi_wwan.c
> @@ -139,16 +139,9 @@ 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 */
> while (len > 3) {
> @@ -207,25 +200,14 @@ 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;
> + /* Use separate control and data interfaces if we found a CDC Union */
> + if (cdc_union) {
> + info->data = usb_ifnum_to_if(dev->udev, cdc_union->bSlaveInterface0);
> + if (desc->bInterfaceNumber != cdc_union->bMasterInterface0 || !info->data) {
> + dev_err(&intf->dev, "bogus CDC Union: master=%u, slave=%u\n",
> + cdc_union->bMasterInterface0, cdc_union->bSlaveInterface0);
> + goto err;
> + }
> }
>
> /* errors aren't fatal - we can live with the dynamic address */
> @@ -235,11 +217,12 @@ next_desc:
> }
>
> /* claim data interface and set it up */
> - status = usb_driver_claim_interface(driver, info->data, dev);
> - if (status < 0)
> - goto err;
> + 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);
--
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
