[PATCH v2] CDC NCM: release interfaces fix in unbind()

[PATCH v2] CDC NCM: release interfaces fix in unbind()

[Alexey Orishko]

Changes:
- claim slave/data interface during bind() and release in
 unbind() unconditionally
- in case of error during bind(), release claimed data
 interface in the same function
- remove obsolited "*_claimed" entries from driver context

Signed-off-by: Alexey Orishko <alexey.orishko@stericsson.com>
---
 drivers/net/usb/cdc_ncm.c |   66 +++++++++++---------------------------------
 1 files changed, 17 insertions(+), 49 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 4ab557d..7fa00d7 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -54,7 +54,7 @@
 #include <linux/usb/usbnet.h>
 #include <linux/usb/cdc.h>
 
-#define	DRIVER_VERSION				"06-May-2011"
+#define	DRIVER_VERSION				"17-May-2011"
 
 /* CDC NCM subclass 3.2.1 */
 #define USB_CDC_NCM_NDP16_LENGTH_MIN		0x10
@@ -134,8 +134,6 @@ struct cdc_ncm_ctx {
 	u16 tx_ndp_modulus;
 	u16 tx_seq;
 	u16 connected;
-	u8 data_claimed;
-	u8 control_claimed;
 };
 
 static void cdc_ncm_tx_timeout(unsigned long arg);
@@ -460,17 +458,6 @@ static void cdc_ncm_free(struct cdc_ncm_ctx *ctx)
 
 	del_timer_sync(&ctx->tx_timer);
 
-	if (ctx->data_claimed) {
-		usb_set_intfdata(ctx->data, NULL);
-		usb_driver_release_interface(driver_of(ctx->intf), ctx->data);
-	}
-
-	if (ctx->control_claimed) {
-		usb_set_intfdata(ctx->control, NULL);
-		usb_driver_release_interface(driver_of(ctx->intf),
-								ctx->control);
-	}
-
 	if (ctx->tx_rem_skb != NULL) {
 		dev_kfree_skb_any(ctx->tx_rem_skb);
 		ctx->tx_rem_skb = NULL;
@@ -495,7 +482,7 @@ static int cdc_ncm_bind(struct usbnet *dev, struct usb_interface *intf)
 
 	ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
 	if (ctx == NULL)
-		goto error;
+		return -ENODEV;
 
 	memset(ctx, 0, sizeof(*ctx));
 
@@ -572,42 +559,32 @@ advance:
 		goto error;
 
 	/* claim interfaces, if any */
-	if (ctx->data != intf) {
-		temp = usb_driver_claim_interface(driver, ctx->data, dev);
-		if (temp)
-			goto error;
-		ctx->data_claimed = 1;
-	}
-
-	if (ctx->control != intf) {
-		temp = usb_driver_claim_interface(driver, ctx->control, dev);
-		if (temp)
-			goto error;
-		ctx->control_claimed = 1;
-	}
+	temp = usb_driver_claim_interface(driver, ctx->data, dev);
+	if (temp)
+		goto error;
 
 	iface_no = ctx->data->cur_altsetting->desc.bInterfaceNumber;
 
 	/* reset data interface */
 	temp = usb_set_interface(dev->udev, iface_no, 0);
 	if (temp)
-		goto error;
+		goto error2;
 
 	/* initialize data interface */
 	if (cdc_ncm_setup(ctx))
-		goto error;
+		goto error2;
 
 	/* configure data interface */
 	temp = usb_set_interface(dev->udev, iface_no, 1);
 	if (temp)
-		goto error;
+		goto error2;
 
 	cdc_ncm_find_endpoints(ctx, ctx->data);
 	cdc_ncm_find_endpoints(ctx, ctx->control);
 
 	if ((ctx->in_ep == NULL) || (ctx->out_ep == NULL) ||
 	    (ctx->status_ep == NULL))
-		goto error;
+		goto error2;
 
 	dev->net->ethtool_ops = &cdc_ncm_ethtool_ops;
 
@@ -617,7 +594,7 @@ advance:
 
 	temp = usbnet_get_ethernet_addr(dev, ctx->ether_desc->iMACAddress);
 	if (temp)
-		goto error;
+		goto error2;
 
 	dev_info(&dev->udev->dev, "MAC-Address: "
 				"0x%02x:0x%02x:0x%02x:0x%02x:0x%02x:0x%02x\n",
@@ -642,6 +619,10 @@ advance:
 	ctx->tx_speed = ctx->rx_speed = 0;
 	return 0;
 
+error2:
+	usb_set_intfdata(ctx->control, NULL);
+	usb_set_intfdata(ctx->data, NULL);
+	usb_driver_release_interface(driver, ctx->data);
 error:
 	cdc_ncm_free((struct cdc_ncm_ctx *)dev->data[0]);
 	dev->data[0] = 0;
@@ -652,27 +633,14 @@ error:
 static void cdc_ncm_unbind(struct usbnet *dev, struct usb_interface *intf)
 {
 	struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
-	struct usb_driver *driver;
+	struct usb_driver *driver = driver_of(intf);
 
 	if (ctx == NULL)
 		return;		/* no setup */
 
-	driver = driver_of(intf);
-
-	usb_set_intfdata(ctx->data, NULL);
 	usb_set_intfdata(ctx->control, NULL);
-	usb_set_intfdata(ctx->intf, NULL);
-
-	/* release interfaces, if any */
-	if (ctx->data_claimed) {
-		usb_driver_release_interface(driver, ctx->data);
-		ctx->data_claimed = 0;
-	}
-
-	if (ctx->control_claimed) {
-		usb_driver_release_interface(driver, ctx->control);
-		ctx->control_claimed = 0;
-	}
+	usb_set_intfdata(ctx->data, NULL);
+	usb_driver_release_interface(driver, ctx->data);
 
 	cdc_ncm_free(ctx);
 }
-- 
1.7.4.3

Re: [PATCH v2] CDC NCM: release interfaces fix in unbind()

[Alan Stern]

On Tue, 17 May 2011, Alexey Orishko wrote:

> Changes:
> - claim slave/data interface during bind() and release in
>  unbind() unconditionally
> - in case of error during bind(), release claimed data
>  interface in the same function
> - remove obsolited "*_claimed" entries from driver context

...

> @@ -572,42 +559,32 @@ advance:
>  		goto error;
>  
>  	/* claim interfaces, if any */
> -	if (ctx->data != intf) {
> -		temp = usb_driver_claim_interface(driver, ctx->data, dev);
> -		if (temp)
> -			goto error;
> -		ctx->data_claimed = 1;
> -	}
> -
> -	if (ctx->control != intf) {
> -		temp = usb_driver_claim_interface(driver, ctx->control, dev);
> -		if (temp)
> -			goto error;
> -		ctx->control_claimed = 1;
> -	}
> +	temp = usb_driver_claim_interface(driver, ctx->data, dev);
> +	if (temp)
> +		goto error;

Here and later on, the patch seems to have forgotten about the control 
interface.  Is this deliberate or an oversight?

Alan Stern

RE: [PATCH v2] CDC NCM: release interfaces fix in unbind()

[Alexey ORISHKO]

> From: Alan Stern [mailto:stern@rowland.harvard.edu]
> Sent: Wednesday, May 18, 2011 4:55 PM
> 
> Here and later on, the patch seems to have forgotten about the control
> interface.  Is this deliberate or an oversight?
> 
> Alan Stern

Kernel docs says, that usb_driver_claim_interface() is used by usb
device drivers that need to claim more than one interface. I assume,
it's needed for second interface only. Am I wrong?

NCM driver was partly based on ECM code. I did check existing drivers
for CDC ACM and CDC ECM, which also uses 2 interfaces: master (control)
and data (bulk), but I was unable to find any code, which claims master
interface.

If we need explicitly claim/release master interface (which is *intf
parameter in bind() and unbind()), then cdc-acm and usb_ether drivers
should be updated as well.

I wonder, if Greg or Oliver could provide any comments why master interface
is not claimed in modem/ether drivers, since they are working with the
code for quite a while.

Regards,
Alexey

RE: [PATCH v2] CDC NCM: release interfaces fix in unbind()

[Alan Stern]

On Wed, 18 May 2011, Alexey ORISHKO wrote:

> > From: Alan Stern [mailto:stern@rowland.harvard.edu]
> > Sent: Wednesday, May 18, 2011 4:55 PM
> > 
> > Here and later on, the patch seems to have forgotten about the control
> > interface.  Is this deliberate or an oversight?
> > 
> > Alan Stern
> 
> Kernel docs says, that usb_driver_claim_interface() is used by usb
> device drivers that need to claim more than one interface. I assume,
> it's needed for second interface only. Am I wrong?

Well, if a driver wants to claim three interfaces (which is more than 
one), it would call usb_driver_claim_interface() for both the second 
and third interfaces, not only the second.

In general, when the driver gets probed for any one of the interfaces,
it should identify all the interfaces it's interested in and claim
them.  However, it should skip the interface currently being probed --
that usb_driver_claim_interface() call would fail anyway since an
interface can't be claimed while it is being probed.

Similarly, when any of the interfaces is unbound, the driver should 
release them all.

> NCM driver was partly based on ECM code. I did check existing drivers
> for CDC ACM and CDC ECM, which also uses 2 interfaces: master (control)
> and data (bulk), but I was unable to find any code, which claims master
> interface.
> 
> If we need explicitly claim/release master interface (which is *intf
> parameter in bind() and unbind()), then cdc-acm and usb_ether drivers
> should be updated as well.

As far as I can tell, those drivers check that they are being probed
for the control interface, so all they need to claim is the data
interface.

You could do something similar -- have the bind routine return 
-ENODEV if it's not being called for the control interface.  But the 
unbind routine would still have to release both interfaces, since it 
can't rely on being called for the control interface first.

Alan Stern

Re: [PATCH v2] CDC NCM: release interfaces fix in unbind()

[Oliver Neukum]

Am Mittwoch, 18. Mai 2011, 19:11:37 schrieb Alexey ORISHKO:
> I wonder, if Greg or Oliver could provide any comments why master interface
> is not claimed in modem/ether drivers, since they are working with the
> code for quite a while.

Probe is called for the master interface. You claim only _additional_
interfaces. The problem is in disconnect(). You may be called in any order.

	HTH
		Oliver

--
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