Patchwork CDC NCM: release interfaces fix in unbind()

login
register
mail settings
Submitter Alexey Orishko
Date May 15, 2011, 8:28 p.m.
Message ID <1305491307-6548-1-git-send-email-alexey.orishko@stericsson.com>
Download mbox | patch
Permalink /patch/95663/
State Rejected
Delegated to: David Miller
Headers show

Comments

Alexey Orishko - May 15, 2011, 8:28 p.m.
Signed-off-by: Alexey Orishko <alexey.orishko@stericsson.com>
---
 drivers/net/usb/cdc_ncm.c |   13 +++++--------
 1 files changed, 5 insertions(+), 8 deletions(-)
David Miller - May 16, 2011, 7:13 p.m.
From: Alexey Orishko <alexey.orishko@gmail.com>
Date: Sun, 15 May 2011 22:28:27 +0200

> Signed-off-by: Alexey Orishko <alexey.orishko@stericsson.com>

I don't know about this.

The usb_set_intfdata() calls at cdc_ncm_bind() time are unconditional,
and are performed regardless of whether ->data_claimed or
->control_claimed are set.

So conditionalizing the setting of them to NULL here doesn't seem
right at all.

I'm not applying this patch.
--
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
Alexey ORISHKO - May 17, 2011, 12:52 a.m.
> The usb_set_intfdata() calls at cdc_ncm_bind() time are unconditional,
> and are performed regardless of whether ->data_claimed or
> ->control_claimed are set.
> 
> So conditionalizing the setting of them to NULL here doesn't seem
> right at all.
> 
> I'm not applying this patch.

Looks like I have to pay more attention to bind/unbind code as it was
copy-paste to some extent. Looking at cdc-acm and cdc_ether drivers,
there is no need to claim control (master) interface neither release it
in unbind().
Some confusion was added by other driver code that handles crippled
devices, which mix ctrl and data interfaces... Shall not be the case here.
I see, that some clean up can be done and "xxx_claimed" fields in context
structure can be removed, so will post a new version of patch shortly.

Regards,
Alexey
--
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
Oliver Neukum - May 17, 2011, 6:26 a.m.
Am Dienstag, 17. Mai 2011, 02:52:34 schrieb Alexey ORISHKO:

> Some confusion was added by other driver code that handles crippled
> devices, which mix ctrl and data interfaces... Shall not be the case here.
> I see, that some clean up can be done and "xxx_claimed" fields in context
> structure can be removed, so will post a new version of patch shortly.

This code is a bit tricky in those drivers because you cannot assume a certain
order of calls to disconnect() w.r.t. the interfaces. If you claim multiple
interfaces, you need to release all other interfaces, as soon as any interface
is disconnected.

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

Patch

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 4ab557d..500b1a6 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				"15-May-2011"
 
 /* CDC NCM subclass 3.2.1 */
 #define USB_CDC_NCM_NDP16_LENGTH_MIN		0x10
@@ -652,28 +652,25 @@  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_set_intfdata(ctx->data, NULL);
 		usb_driver_release_interface(driver, ctx->data);
 		ctx->data_claimed = 0;
 	}
 
 	if (ctx->control_claimed) {
+		usb_set_intfdata(ctx->control, NULL);
 		usb_driver_release_interface(driver, ctx->control);
 		ctx->control_claimed = 0;
 	}
 
+	usb_set_intfdata(ctx->intf, NULL);
 	cdc_ncm_free(ctx);
 }