Re: [PATCH net,stable-3.8] net: qmi_wwan: set correct altsetting for Gobi 1K devices

[Dan Williams <dcbw@redhat.com>]

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@redhat.com>
> Signed-off-by: Bjørn Mork

Works on my UML290, Gobi3K, Gobi1K, Gobi2K, and E362.

Tested-by: Dan Williams <dcbw@redhat.com>

>  <bjorn@mork.no>
> ---
> 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);