[PATCH V5 net-next 0/3] The huawei_cdc_ncm driver

[PATCH V5 net-next 0/3] The huawei_cdc_ncm driver

[Enrico Mioso]

So this is a new, revised, edition of the huawei_cdc_ncm.c driver, which 
supports devices resembling the NCM standard, but using it also as a mean 
to encapsulate other protocols, as is the case for the Huawei E3131 and
E3251 modem devices.
Some precisations are needed however - and I encourage discussion on this: and 
that's why I'm sending this message with a broader CC.
Merging those patches might change:
- the way Modem Manager interacts with those devices
- some regressions might be possible if there are some unknown firmware 
  variants around (Franko?)

First of all: I observed the behaviours of two devices.
Huawei E3131: this device doesn't accept NDIS setup requests unless they're 
sent via the embedded AT channel exposed by this driver.
So actually we gain funcionality in this case!

The second case, is the Huawei E3251: which works with standard NCM driver, 
still exposing an AT embedded channel. Whith this patch set applied, you gain 
some funcionality, loosing the ability to catch standard NCM events for now.
The device will work in both ways with no problems, but this has to be 
acknowledged and discussed. Might be we can develop this driver further to 
change this, when more devices are tested.

We where thinking Huawei changed their interfaces on new devices - but probably 
this driver only works around a nice firmware bug present in E3131, which 
prevented the modem from being used in NDIS mode.

I think committing this is definitely wortth-while, since it will allow for 
more Huawei devices to be used without serial connection. Some devices like the 
E3251 also, reports some status information only via the embedded AT channel, 
at least in my case.
Note: I'm not subscribed to any list except the Modem Manager's one, so please 
CC me, thanks!!


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          |  15 +++
 drivers/net/usb/Makefile         |   1 +
 drivers/net/usb/cdc_ncm.c        |  17 +--
 drivers/net/usb/huawei_cdc_ncm.c | 228 +++++++++++++++++++++++++++++++++++++++
 include/linux/usb/cdc_ncm.h      |   3 +
 5 files changed, 251 insertions(+), 13 deletions(-)
 create mode 100644 drivers/net/usb/huawei_cdc_ncm.c

-- 
1.8.4

[PATCH V5 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>

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

[PATCH V5 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/cdc_mbim approach & code model.

Suggested-by: Bjorn Mork <bjorn-yOkvZcmFvRU@public.gmane.org>
Signed-off-by: Enrico Mioso <mrkiko.rs-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

 create mode 100644 drivers/net/usb/huawei_cdc_ncm.c

diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
index 40db312..85e4a01 100644
--- a/drivers/net/usb/Kconfig
+++ b/drivers/net/usb/Kconfig
@@ -242,6 +242,21 @@ 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 NCM embedded AT channel support"
+	depends on USB_USBNET
+	select USB_WDM
+	select USB_NET_CDC_NCM
+	help
+		This driver supports huawei-style NCM devices, that use NCM as a
+		transport for other protocols, usually an embedded AT channel.
+		Good examples are:
+		* Huawei E3131
+		* Huawei E3251
+
+		To compile this driver as a module, choose M here: the module will be
+		called huawei_cdc_ncm.ko.
+
 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 8b342cf..b17b5e8 100644
--- a/drivers/net/usb/Makefile
+++ b/drivers/net/usb/Makefile
@@ -32,6 +32,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..ff07b18
--- /dev/null
+++ b/drivers/net/usb/huawei_cdc_ncm.c
@@ -0,0 +1,228 @@
+/* huawei_cdc_ncm.c - handles Huawei devices using the CDC NCM protocol as
+ * transport layer.
+ * Copyright (C) 2013	 Enrico Mioso <mrkiko.rs-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
+ *
+ *
+ * ABSTRACT:
+ * 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-yOkvZcmFvRU@public.gmane.org>
+ *
+ * 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;
+
+	/* altsetting should always be 1 for NCM devices - so we hard-coded
+	 * it here
+	 */
+	ret = cdc_ncm_bind_common(usbnet_dev, intf, 1);
+	if (ret)
+		goto err;
+
+	ctx = drvstate->ctx;
+
+	if (usbnet_dev->status)
+		/* CDC-WMC r1.1 requires wMaxCommand to be "at least 256
+		 * decimal (0x100)"
+		 */
+		subdriver = usb_cdc_wdm_register(ctx->control,
+						 &usbnet_dev->status->desc,
+						 256, /* wMaxCommand */
+						 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;
+	bool callsub;
+	struct cdc_ncm_ctx *ctx = drvstate->ctx;
+
+	/* should we call subdriver's resume function? */
+	callsub =
+		(intf == ctx->control &&
+		drvstate->subdriver &&
+		drvstate->subdriver->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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>");
+MODULE_DESCRIPTION("USB CDC NCM host driver with encapsulated protocol support");
+MODULE_LICENSE("GPL");
-- 
1.8.4

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

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

[Oliver Neukum]

On Mon, 2013-09-30 at 04:50 +0000, Enrico Mioso wrote:

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

The error case corrupts drvstate->pmcount

> +		usbnet_dev->intf->needs_remote_wakeup = on;
> +		usb_autopm_put_interface(usbnet_dev->intf);
> +	}
> +err:
> +	return rv;
> +}


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

That is not a valid way to indicate an error.

> +		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;
> +	bool callsub;
> +	struct cdc_ncm_ctx *ctx = drvstate->ctx;
> +
> +	/* should we call subdriver's resume function? */
> +	callsub =
> +		(intf == ctx->control &&
> +		drvstate->subdriver &&
> +		drvstate->subdriver->resume);
> +
> +	if (callsub)
> +		ret = drvstate->subdriver->resume(intf);
> +	if (ret < 0)
> +		goto err;
> +	ret = usbnet_resume(intf);
> +	if (ret < 0 && callsub && drvstate->subdriver->suspend)

You really want drivers with a resume() but no suspend() method?

> +		drvstate->subdriver->suspend(intf, PMSG_SUSPEND);
> +err:
> +	return ret;
> +}

	Regards
		Oliver

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

[Bjørn Mork]

Oliver Neukum <oneukum@suse.de> writes:
> On Mon, 2013-09-30 at 04:50 +0000, Enrico Mioso wrote:
>
>> +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;
>
> The error case corrupts drvstate->pmcount
>
>> +		usbnet_dev->intf->needs_remote_wakeup = on;
>> +		usb_autopm_put_interface(usbnet_dev->intf);
>> +	}
>> +err:
>> +	return rv;
>> +}

Hello Oliver,

I finally got around to looking closer at this and you are of course
correct.  This is all my fault when I initially wrapped the
needs_remote_wakeup update in usb_autopm_{get,put}_interface,

And the problem is not only here in this new driver, but *everywhere* I
did this, including in cdc-wdm.  That driver doesn't have the counter to
corrupt, but failing to update intf->needs_remote_wakeup if
usb_autopm_get_interface fails is still wrong. The get/put were only
added to make the change take effect immediately.  Things sort of worked
fine before that, and ignoring a get error would only revert to that
older behaviour.

So I believe we should do the update unconditionally, and but skip
usb_autopm_put_interface if the get failed.  Accordingly, these
functions should always return 0 (not that there is anything currently
checking the return anyway).

I'll prepare patches for cdc-wdm, qmi_wwan and cdc_mbim.


Bjørn

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

[Oliver Neukum]

On Fri, 2013-11-01 at 12:35 +0100, Bjørn Mork wrote:

> So I believe we should do the update unconditionally, and but skip
> usb_autopm_put_interface if the get failed.  Accordingly, these
> functions should always return 0 (not that there is anything currently
> checking the return anyway).
> 
> I'll prepare patches for cdc-wdm, qmi_wwan and cdc_mbim.

Good plan.

	Regards
		Oliver

[PATCH V5 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

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

Re: [PATCH V5 net-next 0/3] The huawei_cdc_ncm driver

[Bjørn Mork]

Enrico Mioso <mrkiko.rs@gmail.com> writes:

> So this is a new, revised, edition of the huawei_cdc_ncm.c driver, which 
> supports devices resembling the NCM standard, but using it also as a mean 
> to encapsulate other protocols, as is the case for the Huawei E3131 and
> E3251 modem devices.
> Some precisations are needed however - and I encourage discussion on this: and 
> that's why I'm sending this message with a broader CC.
> Merging those patches might change:
> - the way Modem Manager interacts with those devices
> - some regressions might be possible if there are some unknown firmware 
>   variants around (Franko?)
>
> First of all: I observed the behaviours of two devices.
> Huawei E3131: this device doesn't accept NDIS setup requests unless they're 
> sent via the embedded AT channel exposed by this driver.
> So actually we gain funcionality in this case!
>
> The second case, is the Huawei E3251: which works with standard NCM driver, 
> still exposing an AT embedded channel. Whith this patch set applied, you gain 
> some funcionality, loosing the ability to catch standard NCM events for now.
> The device will work in both ways with no problems, but this has to be 
> acknowledged and discussed. Might be we can develop this driver further to 
> change this, when more devices are tested.

Your driver, and the cdc-wdm subdriver API in general, could certainly
be extended to support standard NCM events.  There have been some
discussions in the linux-usb list already on how to best do this.  I
believe this message from Oliver is the current conclusion to that
discussion: http://www.spinics.net/lists/linux-usb/msg70140.html

I.e:
 - extend the cdc-wdm subdriver API by creating a struct holding all
   necessary callbacks, and use this struct instead of the current
   single "manage_power" callback, and
 - create one callback hook per notification you want to handle, with
   clear semantics and reasonable names

But I still believe your driver should go in as it is for now. As you
note, it is required for the E3131.  And the same is most likely the
case for other devices in the same family.

Handling the NCM notifications can always be added later. IMHO, they can
be considered optional given that a separate management channel is
required in any case to configure these devices and start/stop the
connection.  The NCM events you lose compared to cdc_ncm are
NETWORK_CONNECTION and SPEED_CHANGE. The first one is useful as it is
implemented in cdc_ncm because it controls the network device link
state, but it is still redundant for devices like these where a managing
userspace application is required and the connection events are
available via the management channel.  The implementation of
SPEED_CHANGE events is less useful. The cdc_ncm driver doesn't use the
received speed data for anything except printing an informational
message.  I don't think implementing it in your driver will gain you
anything.

> We where thinking Huawei changed their interfaces on new devices - but probably 
> this driver only works around a nice firmware bug present in E3131, which 
> prevented the modem from being used in NDIS mode.

I am not sure this is a firmware bug.  It could very well be by design,
and the differences in observed behaviour could be just artifacts of the
interface implementation on top of different chipsets and/or base
firmwares.  AFAIK, Huawei have never officially supported the serial
port for network device management on this class of devices. The
embedded AT channel is most likely the only AT command channel intended
for network device management, even on the devices with serial ports.

> I think committing this is definitely wortth-while, since it will allow for 
> more Huawei devices to be used without serial connection. Some devices like the 
> E3251 also, reports some status information only via the embedded AT channel, 
> at least in my case.
> Note: I'm not subscribed to any list except the Modem Manager's one, so please 
> CC me, thanks!!

Yes, this is most definitely a driver worth being added.  There are a
number of devices which just cannot be made working in Linux without it
because the embedded management channel is the only one available.

I don't know if you are aware of this, but I have pointed a few people
to your previous submission attempts and there are therefore some
success stories around already.  An example:
http://lists.freedesktop.org/archives/libqmi-devel/2013-September/000650.html


Bjørn