From patchwork Wed Mar 13 07:13:40 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Bj=C3=B8rn_Mork?= X-Patchwork-Id: 227164 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 81F9B2C0090 for ; Wed, 13 Mar 2013 18:13:55 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755077Ab3CMHNu (ORCPT ); Wed, 13 Mar 2013 03:13:50 -0400 Received: from canardo.mork.no ([148.122.252.1]:38445 "EHLO canardo.mork.no" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752285Ab3CMHNt convert rfc822-to-8bit (ORCPT ); Wed, 13 Mar 2013 03:13:49 -0400 Received: from nemi.mork.no (nemi.mork.no [IPv6:2001:4620:9:2:216:eaff:feb3:788]) (authenticated bits=0) by canardo.mork.no (8.14.3/8.14.3) with ESMTP id r2D7DeTV007489 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NOT); Wed, 13 Mar 2013 08:13:41 +0100 Received: from bjorn by nemi.mork.no with local (Exim 4.80) (envelope-from ) id 1UFfsa-0001SV-B6; Wed, 13 Mar 2013 08:13:40 +0100 From: =?utf-8?Q?Bj=C3=B8rn_Mork?= To: Dan Williams Cc: linux-usb@vger.kernel.org, netdev@vger.kernel.org Subject: Re: [RFC PATCH] qmi_wwan: set correct altsetting for Gobi 1K devices Organization: m References: <1363117205.9644.17.camel@dcbw.foobar.com> <2400f481-f2eb-440d-b74d-ff9bfccd1179@email.android.com> Date: Wed, 13 Mar 2013 08:13:40 +0100 In-Reply-To: <2400f481-f2eb-440d-b74d-ff9bfccd1179@email.android.com> (=?utf-8?Q?=22Bj=C3=B8rn?= Mork"'s message of "Tue, 12 Mar 2013 22:17:53 +0100") Message-ID: <87hakf93h7.fsf@nemi.mork.no> User-Agent: Gnus/5.11002 (No Gnus v0.20) Emacs/23.4 (gnu/linux) MIME-Version: 1.0 X-Virus-Scanned: clamav-milter 0.97.6 at canardo X-Virus-Status: Clean Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Bjørn Mork 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: --- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html 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);