[PATCH RESEND V3 net-next 0/3] huawei_cdc_ncm driver introduction

[PATCH RESEND V3 net-next 0/3] huawei_cdc_ncm driver introduction

[Enrico Mioso]

These patches are all related to the new huawei_cdc_ncm driver, supporting 
devices that use the NCM protocol as a transport layer for other protocols. 
this is the case of the Huawei E3131 3G modem.

In this version - I actually added the driver file! :) I don't know how I ended 
up forgetting about it, I should have learned git usage better after all this 
time! Sorry for the time I wasted you.

Enrico Mioso (3):
  net: cdc_ncm: Export cdc_ncm_{tx,rx}_fixup functions for re-use
  net: huawei_cdc_ncm: Introduce the huawei_cdc_ncm driver
  net: cdc_ncm: remove non-standard NCM device IDs

 drivers/net/usb/Kconfig          |   11 ++
 drivers/net/usb/Makefile         |    1 +
 drivers/net/usb/cdc_ncm.c        |   17 +--
 drivers/net/usb/huawei_cdc_ncm.c |  210 ++++++++++++++++++++++++++++++++++++++
 include/linux/usb/cdc_ncm.h      |    3 +
 5 files changed, 229 insertions(+), 13 deletions(-)
 create mode 100644 drivers/net/usb/huawei_cdc_ncm.c

-- 
1.7.10.4

[PATCH RESEND V3 net-next 1/3] net: cdc_ncm: Export cdc_ncm_{tx,rx}_fixup functions for re-use

[Enrico Mioso]

Some drivers implementing NCM-like protocols, may re-use those functions, as is
the case in the huawei_cdc_ncm driver.
Export them via EXPORT_SYMBOL_GPL, in accordance with how other functions have
been exported.

Signed-off-by: Enrico Mioso <mrkiko.rs@gmail.com>
---
 drivers/net/usb/cdc_ncm.c   |    6 ++++--
 include/linux/usb/cdc_ncm.h |    3 +++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 43afde8..62686be 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -858,7 +858,7 @@ static void cdc_ncm_txpath_bh(unsigned long param)
 	}
 }
 
-static struct sk_buff *
+struct sk_buff *
 cdc_ncm_tx_fixup(struct usbnet *dev, struct sk_buff *skb, gfp_t flags)
 {
 	struct sk_buff *skb_out;
@@ -885,6 +885,7 @@ error:
 
 	return NULL;
 }
+EXPORT_SYMBOL_GPL(cdc_ncm_tx_fixup);
 
 /* verify NTB header and return offset of first NDP, or negative error */
 int cdc_ncm_rx_verify_nth16(struct cdc_ncm_ctx *ctx, struct sk_buff *skb_in)
@@ -965,7 +966,7 @@ error:
 }
 EXPORT_SYMBOL_GPL(cdc_ncm_rx_verify_ndp16);
 
-static int cdc_ncm_rx_fixup(struct usbnet *dev, struct sk_buff *skb_in)
+int cdc_ncm_rx_fixup(struct usbnet *dev, struct sk_buff *skb_in)
 {
 	struct sk_buff *skb;
 	struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
@@ -1040,6 +1041,7 @@ err_ndp:
 error:
 	return 0;
 }
+EXPORT_SYMBOL_GPL(cdc_ncm_rx_fixup);
 
 static void
 cdc_ncm_speed_change(struct cdc_ncm_ctx *ctx,
diff --git a/include/linux/usb/cdc_ncm.h b/include/linux/usb/cdc_ncm.h
index cc25b70..163244b 100644
--- a/include/linux/usb/cdc_ncm.h
+++ b/include/linux/usb/cdc_ncm.h
@@ -133,3 +133,6 @@ extern void cdc_ncm_unbind(struct usbnet *dev, struct usb_interface *intf);
 extern struct sk_buff *cdc_ncm_fill_tx_frame(struct cdc_ncm_ctx *ctx, struct sk_buff *skb, __le32 sign);
 extern int cdc_ncm_rx_verify_nth16(struct cdc_ncm_ctx *ctx, struct sk_buff *skb_in);
 extern int cdc_ncm_rx_verify_ndp16(struct sk_buff *skb_in, int ndpoffset);
+struct sk_buff *
+cdc_ncm_tx_fixup(struct usbnet *dev, struct sk_buff *skb, gfp_t flags);
+int cdc_ncm_rx_fixup(struct usbnet *dev, struct sk_buff *skb_in);
-- 
1.7.10.4

[PATCH RESEND V3 net-next 2/3] net: huawei_cdc_ncm: Introduce the huawei_cdc_ncm driver

[Enrico Mioso]

This driver supports devices using the NCM protocol as an encapsulation layer
for other protocols, like the E3131 Huawei 3G modem. This drivers approach was 
heavily inspired by the qmi_wwan approach & code model.

Suggested-by: Bjorn Mork <bjorn@mork.no>
Signed-off-by: Enrico Mioso <mrkiko.rs@gmail.com>
---
 drivers/net/usb/Kconfig          |   11 ++
 drivers/net/usb/Makefile         |    1 +
 drivers/net/usb/huawei_cdc_ncm.c |  210 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 222 insertions(+)
 create mode 100644 drivers/net/usb/huawei_cdc_ncm.c

diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
index d84bfd4..6e56751 100644
--- a/drivers/net/usb/Kconfig
+++ b/drivers/net/usb/Kconfig
@@ -242,6 +242,17 @@ config USB_NET_CDC_NCM
 	    * ST-Ericsson M343 HSPA Mobile Broadband Modem (reference design)
 	    * Ericsson F5521gw Mobile Broadband Module
 
+config USB_NET_HUAWEI_CDC_NCM
+	tristate "Huawei-style CDC NCM support"
+	depends on USB_USBNET
+	select USB_WDM
+	select USB_NET_CDC_NCM
+	help
+		This driver aims to support huawei-style NCM devices, that use ncm as a
+		transport for other protocols.
+		To compile this driver as a module, choose M here: the module will be
+		called huawei_cdc_ncm.
+
 config USB_NET_CDC_MBIM
 	tristate "CDC MBIM support"
 	depends on USB_USBNET
diff --git a/drivers/net/usb/Makefile b/drivers/net/usb/Makefile
index e817178..fd0e6a7 100644
--- a/drivers/net/usb/Makefile
+++ b/drivers/net/usb/Makefile
@@ -31,6 +31,7 @@ obj-$(CONFIG_USB_IPHETH)	+= ipheth.o
 obj-$(CONFIG_USB_SIERRA_NET)	+= sierra_net.o
 obj-$(CONFIG_USB_NET_CX82310_ETH)	+= cx82310_eth.o
 obj-$(CONFIG_USB_NET_CDC_NCM)	+= cdc_ncm.o
+obj-$(CONFIG_USB_NET_HUAWEI_CDC_NCM)	+= huawei_cdc_ncm.o
 obj-$(CONFIG_USB_VL600)		+= lg-vl600.o
 obj-$(CONFIG_USB_NET_QMI_WWAN)	+= qmi_wwan.o
 obj-$(CONFIG_USB_NET_CDC_MBIM)	+= cdc_mbim.o
diff --git a/drivers/net/usb/huawei_cdc_ncm.c b/drivers/net/usb/huawei_cdc_ncm.c
new file mode 100644
index 0000000..d3426b9
--- /dev/null
+++ b/drivers/net/usb/huawei_cdc_ncm.c
@@ -0,0 +1,210 @@
+/*
+ * huawei_cdc_ncm.c - handles huawei-style NCM devices.
+ * Copyright (C) 2013	 Enrico Mioso <mrkiko.rs@gmail.com>
+ * This driver handles devices resembling the CDC NCM standard, but
+ * encapsulating another protocol inside it. An example are some Huawei 3G
+ * devices, exposing an embedded AT channel where you can set up the NCM
+ * connection.
+ * This code has been heavily inspired by the cdc_mbim.c driver, which is
+ * Copyright (c) 2012  Smith Micro Software, Inc.
+ * Copyright (c) 2012  Bjørn Mork <bjorn@mork.no>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/ethtool.h>
+#include <linux/if_vlan.h>
+#include <linux/ip.h>
+#include <linux/mii.h>
+#include <linux/usb.h>
+#include <linux/usb/cdc.h>
+#include <linux/usb/usbnet.h>
+#include <linux/usb/cdc-wdm.h>
+#include <linux/usb/cdc_ncm.h>
+
+/* Driver data */
+struct huawei_cdc_ncm_state {
+	struct cdc_ncm_ctx *ctx;
+	atomic_t pmcount;
+	struct usb_driver *subdriver;
+	struct usb_interface *control;
+	struct usb_interface *data;
+};
+
+static int huawei_cdc_ncm_manage_power(struct usbnet *usbnet_dev, int on)
+{
+	struct huawei_cdc_ncm_state *drvstate = (void *)&usbnet_dev->data;
+	int rv = 0;
+
+	if ((on && atomic_add_return(1, &drvstate->pmcount) == 1) || (!on && atomic_dec_and_test(&drvstate->pmcount))) {
+		rv = usb_autopm_get_interface(usbnet_dev->intf);
+		if (rv < 0)
+			goto err;
+		usbnet_dev->intf->needs_remote_wakeup = on;
+		usb_autopm_put_interface(usbnet_dev->intf);
+	}
+err:
+	return rv;
+}
+
+static int huawei_cdc_ncm_wdm_manage_power(struct usb_interface *intf, int status)
+{
+	struct usbnet *usbnet_dev = usb_get_intfdata(intf);
+
+	/* can be called while disconnecting */
+	if (!usbnet_dev)
+		return 0;
+
+	return huawei_cdc_ncm_manage_power(usbnet_dev, status);
+}
+
+
+static int huawei_cdc_ncm_bind(struct usbnet *usbnet_dev, struct usb_interface *intf)
+{
+	struct cdc_ncm_ctx *ctx;
+	struct usb_driver *subdriver = ERR_PTR(-ENODEV);
+	int ret = -ENODEV;
+	struct huawei_cdc_ncm_state *drvstate = (void *)&usbnet_dev->data;
+
+	ret = cdc_ncm_bind_common(usbnet_dev, intf, 1); /* altsetting should be 1 for NCM devices */
+	if (ret)
+		goto err;
+
+	ctx = drvstate->ctx;
+
+	if (usbnet_dev->status)
+		subdriver = usb_cdc_wdm_register(ctx->control,
+						 &usbnet_dev->status->desc,
+						 256, /* CDC-WMC r1.1 requires wMaxCommand to be "at least 256 decimal (0x100)" */
+						 huawei_cdc_ncm_wdm_manage_power);
+	if (IS_ERR(subdriver)) {
+		ret = PTR_ERR(subdriver);
+		cdc_ncm_unbind(usbnet_dev, intf);
+		goto err;
+	}
+
+	/* Prevent usbnet from using the status descriptor */
+	usbnet_dev->status = NULL;
+
+	drvstate->subdriver = subdriver;
+
+err:
+	return ret;
+}
+
+static void huawei_cdc_ncm_unbind(struct usbnet *usbnet_dev, struct usb_interface *intf)
+{
+	struct huawei_cdc_ncm_state *drvstate = (void *)&usbnet_dev->data;
+	struct cdc_ncm_ctx *ctx = drvstate->ctx;
+
+	if (drvstate->subdriver && drvstate->subdriver->disconnect)
+		drvstate->subdriver->disconnect(ctx->control);
+	drvstate->subdriver = NULL;
+
+	cdc_ncm_unbind(usbnet_dev, intf);
+}
+
+static int huawei_cdc_ncm_suspend(struct usb_interface *intf, pm_message_t message)
+{
+	int ret = 0;
+	struct usbnet *usbnet_dev = usb_get_intfdata(intf);
+	struct huawei_cdc_ncm_state *drvstate = (void *)&usbnet_dev->data;
+	struct cdc_ncm_ctx *ctx = drvstate->ctx;
+
+	if (ctx == NULL) {
+		ret = -1;
+		goto error;
+	}
+
+	ret = usbnet_suspend(intf, message);
+	if (ret < 0)
+		goto error;
+
+	if (intf == ctx->control && drvstate->subdriver && drvstate->subdriver->suspend)
+		ret = drvstate->subdriver->suspend(intf, message);
+	if (ret < 0)
+		usbnet_resume(intf);
+
+error:
+	return ret;
+}
+
+static int huawei_cdc_ncm_resume(struct usb_interface *intf)
+{
+	int ret = 0;
+	struct usbnet *usbnet_dev = usb_get_intfdata(intf);
+	struct huawei_cdc_ncm_state *drvstate = (void *)&usbnet_dev->data;
+	struct cdc_ncm_ctx *ctx = drvstate->ctx;
+	bool callsub = (intf == ctx->control && drvstate->subdriver && drvstate->subdriver->resume); /* should we call subdriver's resume? ? */
+
+	if (callsub)
+		ret = drvstate->subdriver->resume(intf);
+	if (ret < 0)
+		goto err;
+	ret = usbnet_resume(intf);
+	if (ret < 0 && callsub && drvstate->subdriver->suspend)
+		drvstate->subdriver->suspend(intf, PMSG_SUSPEND);
+err:
+	return ret;
+}
+
+static int huawei_cdc_ncm_check_connect(struct usbnet *usbnet_dev)
+{
+	struct cdc_ncm_ctx *ctx;
+
+	ctx = (struct cdc_ncm_ctx *)usbnet_dev->data[0];
+
+	if (ctx == NULL)
+		return 1; /* disconnected */
+
+	return !ctx->connected;
+}
+
+static const struct driver_info huawei_cdc_ncm_info = {
+	.description = "Huawei CDC NCM device",
+	.flags = FLAG_NO_SETINT | FLAG_MULTI_PACKET | FLAG_WWAN,
+	.bind = huawei_cdc_ncm_bind,
+	.unbind = huawei_cdc_ncm_unbind,
+	.check_connect = huawei_cdc_ncm_check_connect,
+	.manage_power = huawei_cdc_ncm_manage_power,
+	.rx_fixup = cdc_ncm_rx_fixup,
+	.tx_fixup = cdc_ncm_tx_fixup,
+};
+
+static const struct usb_device_id huawei_cdc_ncm_devs[] = {
+	/* Huawei NCM devices disguised as vendor specific */
+	{ USB_VENDOR_AND_INTERFACE_INFO(0x12d1, 0xff, 0x02, 0x16),
+	  .driver_info = (unsigned long)&huawei_cdc_ncm_info,
+	},
+	{ USB_VENDOR_AND_INTERFACE_INFO(0x12d1, 0xff, 0x02, 0x46),
+	  .driver_info = (unsigned long)&huawei_cdc_ncm_info,
+	},
+	{ USB_VENDOR_AND_INTERFACE_INFO(0x12d1, 0xff, 0x02, 0x76),
+	  .driver_info = (unsigned long)&huawei_cdc_ncm_info,
+	},
+
+	/* Terminating entry */
+	{
+	},
+};
+MODULE_DEVICE_TABLE(usb, huawei_cdc_ncm_devs);
+
+static struct usb_driver huawei_cdc_ncm_driver = {
+	.name = "huawei_cdc_ncm",
+	.id_table = huawei_cdc_ncm_devs,
+	.probe = usbnet_probe,
+	.disconnect = usbnet_disconnect,
+	.suspend = huawei_cdc_ncm_suspend,
+	.resume = huawei_cdc_ncm_resume,
+	.reset_resume = huawei_cdc_ncm_resume,
+	.supports_autosuspend = 1,
+	.disable_hub_initiated_lpm = 1,
+};
+module_usb_driver(huawei_cdc_ncm_driver);
+MODULE_AUTHOR("Enrico Mioso <mrkiko.rs@gmail.com>");
+MODULE_DESCRIPTION("USB CDC NCM host driver with encapsulated protocol support");
+MODULE_LICENSE("GPL");
-- 
1.7.10.4

[PATCH RESEND V3 net-next 3/3] net: cdc_ncm: remove non-standard NCM device IDs

[Enrico Mioso]

Remove device IDs of NCM-like (but not NCM-conformant) devices, that are
handled by the huawwei_cdc_ncm driver now.

Signed-off-by: Enrico Mioso <mrkiko.rs@gmail.com>
---
 drivers/net/usb/cdc_ncm.c |   11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 62686be..31f43f7 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -1236,17 +1236,6 @@ static const struct usb_device_id cdc_devs[] = {
 	  .driver_info = (unsigned long)&wwan_info,
 	},
 
-	/* Huawei NCM devices disguised as vendor specific */
-	{ USB_VENDOR_AND_INTERFACE_INFO(0x12d1, 0xff, 0x02, 0x16),
-	  .driver_info = (unsigned long)&wwan_info,
-	},
-	{ USB_VENDOR_AND_INTERFACE_INFO(0x12d1, 0xff, 0x02, 0x46),
-	  .driver_info = (unsigned long)&wwan_info,
-	},
-	{ USB_VENDOR_AND_INTERFACE_INFO(0x12d1, 0xff, 0x02, 0x76),
-	  .driver_info = (unsigned long)&wwan_info,
-	},
-
 	/* Infineon(now Intel) HSPA Modem platform */
 	{ USB_DEVICE_AND_INTERFACE_INFO(0x1519, 0x0443,
 		USB_CLASS_COMM,
-- 
1.7.10.4

Re: [PATCH RESEND V3 net-next 2/3] net: huawei_cdc_ncm: Introduce the huawei_cdc_ncm driver

[David Miller]

From: Enrico Mioso <mrkiko.rs@gmail.com>
Date: Fri, 23 Aug 2013 09:56:29 +0200

> +	if ((on && atomic_add_return(1, &drvstate->pmcount) == 1) || (!on && atomic_dec_and_test(&drvstate->pmcount))) {

These line significantly exceeds 80 columns.

> +		subdriver = usb_cdc_wdm_register(ctx->control,
> +						 &usbnet_dev->status->desc,
> +						 256, /* CDC-WMC r1.1 requires wMaxCommand to be "at least 256 decimal (0x100)" */
> +						 huawei_cdc_ncm_wdm_manage_power);

Likewise.

> +	if (intf == ctx->control && drvstate->subdriver && drvstate->subdriver->suspend)

Likewise.

> +	int ret = 0;
> +	struct usbnet *usbnet_dev = usb_get_intfdata(intf);
> +	struct huawei_cdc_ncm_state *drvstate = (void *)&usbnet_dev->data;
> +	struct cdc_ncm_ctx *ctx = drvstate->ctx;
> +	bool callsub = (intf == ctx->control && drvstate->subdriver && drvstate->subdriver->resume); /* should we call subdriver's resume? ? */

Likewise, and order local function variable declarations by line
length, longest to shortest.

Re: [PATCH RESEND V3 net-next 2/3] net: huawei_cdc_ncm: Introduce the huawei_cdc_ncm driver

[Joe Perches]

On Mon, 2013-08-26 at 16:31 -0400, David Miller wrote:
> From: Enrico Mioso <mrkiko.rs@gmail.com>
[]
> > +     int ret = 0;
> > +     struct usbnet *usbnet_dev = usb_get_intfdata(intf);
> > +     struct huawei_cdc_ncm_state *drvstate = (void *)&usbnet_dev->data;
> > +     struct cdc_ncm_ctx *ctx = drvstate->ctx;
> > +     bool callsub = (intf == ctx->control && drvstate->subdriver && drvstate->subdriver->resume); /* should we call subdriver's resume? ? */
> 
> Likewise, and order local function variable declarations by line
> length, longest to shortest.

I think the premise of local variable
declaration by line length is flawed.

It can't work when local variables are
dependent on initialization order as
above.

int ret; could be moved down below callsub
if really desired though.

Re: [PATCH RESEND V3 net-next 2/3] net: huawei_cdc_ncm: Introduce the huawei_cdc_ncm driver

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Mon, 26 Aug 2013 13:45:13 -0700

> I think the premise of local variable
> declaration by line length is flawed.

I disagree.

> It can't work when local variables are
> dependent on initialization order as
> above.

Move the initialization below the declarations.

Re: [PATCH RESEND V3 net-next 2/3] net: huawei_cdc_ncm: Introduce the huawei_cdc_ncm driver

[Joe Perches]

On Mon, 2013-08-26 at 16:58 -0400, David Miller wrote:
> From: Joe Perches <joe@perches.com>
> Date: Mon, 26 Aug 2013 13:45:13 -0700
> 
> > I think the premise of local variable
> > declaration by line length is flawed.
> 
> I disagree.
> 
> > It can't work when local variables are
> > dependent on initialization order as
> > above.
> 
> Move the initialization below the declarations.

So all initializations should be on separate
lines from declarations?

ick.

It's possible, but it doubles the associated
line count and I still think it's a very
dubious premise/practice/style.

As a general rule, it's I suppose OK, but as
a hardened rule, it's like most all hard rules.
It's made to be broken.

Re: [PATCH RESEND V3 net-next 2/3] net: huawei_cdc_ncm: Introduce the huawei_cdc_ncm driver

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Mon, 26 Aug 2013 14:05:55 -0700

> On Mon, 2013-08-26 at 16:58 -0400, David Miller wrote:
>> From: Joe Perches <joe@perches.com>
>> Date: Mon, 26 Aug 2013 13:45:13 -0700
>> 
>> > I think the premise of local variable
>> > declaration by line length is flawed.
>> 
>> I disagree.
>> 
>> > It can't work when local variables are
>> > dependent on initialization order as
>> > above.
>> 
>> Move the initialization below the declarations.
> 
> So all initializations should be on separate
> lines from declarations?

No, you can often make it work out when the initialization
occurs on the same time.

Like everything else in life, you evaluate it on a case by
case basis.

Don't try to over-simplify things and act as if they are black
and white when they aren't.

Re: [PATCH RESEND V3 net-next 2/3] net: huawei_cdc_ncm: Introduce the huawei_cdc_ncm driver

[Joe Perches]

On Mon, 2013-08-26 at 17:09 -0400, David Miller wrote:
> From: Joe Perches <joe@perches.com>
[]
> Don't try to over-simplify things and act as if they are black
> and white when they aren't.

It wasn't me simplifying.

I commented on your request to:

"order local function variable declarations by line
 length, longest to shortest."

as it seemed a bit rigid.

And perhaps you didn't read the rest of my email.

>> As a general rule, it's I suppose OK, but as
>> a hardened rule, it's like most all hard rules.
>> It's made to be broken.

Re: [PATCH RESEND V3 net-next 2/3] net: huawei_cdc_ncm: Introduce the huawei_cdc_ncm driver

[Enrico Mioso]

Hi guys!! :)
First of all - I would like to thank both of you for your interest and time in 
my patches.

I agree with Joe's point of view, completely. The Coding style document tries 
to leverage on the developer's good sense, even when defining some rules.
Apart from that - checkpatch.po informed me about those very long lines, but I 
decided to leave them as they are due to the fact that they would look even 
more horrible than they look now. My braille display is 80-chars long (at 
least, the one I use normally), so I understand very well the problem of not 
passing that limit. Even so, the coding style says you might do so if you think 
the code is more readable this way, and that's why.
My git usage is very bad as you may have observed (and I'm working on improving 
myself of course), but this was something I took into consideration.
I remember when this cameto discussion:
http://lkml.org/lkml/2009/12/17/229
still I know perfectly that one of the line you're blaming is indeed 139 
characters.
I understand and appreciate the fact that we _shouldn't_ take as reference 
worst cases (but only bbetter cases) to improve our practice & life, but in 
various drivers you can find examples like those.
Is this still a problem?

I will re-work the code and send the patch again as soon as I can.

thank you again!