[PATCH net,stable] net: cdc_mbim: add "NDP to end" quirk for Huawei E3372

[PATCH net,stable] net: cdc_mbim: add "NDP to end" quirk for Huawei E3372

[Bjørn Mork]

The Huawei E3372 (12d1:157d) needs this quirk in MBIM mode
as well. Allow this by forcing the NTB to contain only a
single NDP, and add a device specific entry for this ID.

Due to the way Huawei use device IDs, this might be applied
to other modems as well.  It is assumed that those modems
will be based on the same firmware and will need this quirk
too.  If not, it will still not harm normal usage, although
multiplexing performance could be impacted.

Cc: Enrico Mioso <mrkiko.rs@gmail.com>
Reported-by: Sami Farin <hvtaifwkbgefbaei@gmail.com>
Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
 drivers/net/usb/cdc_mbim.c | 26 +++++++++++++++++++++++++-
 drivers/net/usb/cdc_ncm.c  | 10 +++++++++-
 2 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/cdc_mbim.c b/drivers/net/usb/cdc_mbim.c
index bbde9884ab8a..8973abdec9f6 100644
--- a/drivers/net/usb/cdc_mbim.c
+++ b/drivers/net/usb/cdc_mbim.c
@@ -158,7 +158,7 @@ static int cdc_mbim_bind(struct usbnet *dev, struct usb_interface *intf)
 	if (!cdc_ncm_comm_intf_is_mbim(intf->cur_altsetting))
 		goto err;
 
-	ret = cdc_ncm_bind_common(dev, intf, data_altsetting, 0);
+	ret = cdc_ncm_bind_common(dev, intf, data_altsetting, dev->driver_info->data);
 	if (ret)
 		goto err;
 
@@ -582,6 +582,26 @@ static const struct driver_info cdc_mbim_info_zlp = {
 	.tx_fixup = cdc_mbim_tx_fixup,
 };
 
+/* The spefication explicitly allows NDPs to be placed anywhere in the
+ * frame, but some devices fail unless the NDP is placed after the IP
+ * packets.  Using the CDC_NCM_FLAG_NDP_TO_END flags to force this
+ * behaviour.
+ *
+ * Note: The current implementation of this feature restricts each NTB
+ * to a single NDP, implying that multiplexed sessions cannot share an
+ * NTB. This might affect performace for multiplexed sessions.
+ */
+static const struct driver_info cdc_mbim_info_ndp_to_end = {
+	.description = "CDC MBIM",
+	.flags = FLAG_NO_SETINT | FLAG_MULTI_PACKET | FLAG_WWAN,
+	.bind = cdc_mbim_bind,
+	.unbind = cdc_mbim_unbind,
+	.manage_power = cdc_mbim_manage_power,
+	.rx_fixup = cdc_mbim_rx_fixup,
+	.tx_fixup = cdc_mbim_tx_fixup,
+	.data = CDC_NCM_FLAG_NDP_TO_END,
+};
+
 static const struct usb_device_id mbim_devs[] = {
 	/* This duplicate NCM entry is intentional. MBIM devices can
 	 * be disguised as NCM by default, and this is necessary to
@@ -597,6 +617,10 @@ static const struct usb_device_id mbim_devs[] = {
 	{ USB_VENDOR_AND_INTERFACE_INFO(0x0bdb, USB_CLASS_COMM, USB_CDC_SUBCLASS_MBIM, USB_CDC_PROTO_NONE),
 	  .driver_info = (unsigned long)&cdc_mbim_info,
 	},
+	/* Huawei E3372 fails unless NDP comes after the IP packets */
+	{ USB_DEVICE_AND_INTERFACE_INFO(0x12d1, 0x157d, USB_CLASS_COMM, USB_CDC_SUBCLASS_MBIM, USB_CDC_PROTO_NONE),
+	  .driver_info = (unsigned long)&cdc_mbim_info_ndp_to_end,
+	},
 	/* default entry */
 	{ USB_INTERFACE_INFO(USB_CLASS_COMM, USB_CDC_SUBCLASS_MBIM, USB_CDC_PROTO_NONE),
 	  .driver_info = (unsigned long)&cdc_mbim_info_zlp,
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 3b1ba8237768..1e9843a41168 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -955,10 +955,18 @@ static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp(struct cdc_ncm_ctx *ctx, struct sk_
 	* NTH16 header as we would normally do. NDP isn't written to the SKB yet, and
 	* the wNdpIndex field in the header is actually not consistent with reality. It will be later.
 	*/
-	if (ctx->drvflags & CDC_NCM_FLAG_NDP_TO_END)
+	if (ctx->drvflags & CDC_NCM_FLAG_NDP_TO_END) {
 		if (ctx->delayed_ndp16->dwSignature == sign)
 			return ctx->delayed_ndp16;
 
+		/* We can only push a single NDP to the end. Return
+		 * NULL to send what we've already got and queue this
+		 * skb for later.
+		 */
+		else if (ctx->delayed_ndp16->dwSignature)
+			return NULL;
+	}
+
 	/* follow the chain of NDPs, looking for a match */
 	while (ndpoffset) {
 		ndp16 = (struct usb_cdc_ncm_ndp16 *)(skb->data + ndpoffset);
-- 
2.1.4

Re: [PATCH net,stable] net: cdc_mbim: add "NDP to end" quirk for Huawei E3372

[Enrico Mioso]

Hello guys.
I am sorry the way my feature is actually implemented is limiting MUX 
performances in mbim case, even with good devices. When I developed it, I also 
probably tought this wasn't affecting MBIM devices.

Thank you Bjorn, great work in handling it.
Thank you for all of you; Thank you for reporting this Sami.

Acked-By: Enrico Mioso <mrkiko.rs-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
--
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 net,stable] net: cdc_mbim: add "NDP to end" quirk for Huawei E3372

[Mrkiko Rs]

Unfortunately I found a typo: spefication should be specification. Sorry.

Inviato da iPhone

> Il giorno 05/dic/2015, alle ore 13:02, "Bjørn Mork" <bjorn@mork.no> ha scritto:
>
> The Huawei E3372 (12d1:157d) needs this quirk in MBIM mode
> as well. Allow this by forcing the NTB to contain only a
> single NDP, and add a device specific entry for this ID.
>
> Due to the way Huawei use device IDs, this might be applied
> to other modems as well.  It is assumed that those modems
> will be based on the same firmware and will need this quirk
> too.  If not, it will still not harm normal usage, although
> multiplexing performance could be impacted.
>
> Cc: Enrico Mioso <mrkiko.rs@gmail.com>
> Reported-by: Sami Farin <hvtaifwkbgefbaei@gmail.com>
> Signed-off-by: Bjørn Mork <bjorn@mork.no>
> ---
> drivers/net/usb/cdc_mbim.c | 26 +++++++++++++++++++++++++-
> drivers/net/usb/cdc_ncm.c  | 10 +++++++++-
> 2 files changed, 34 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/usb/cdc_mbim.c b/drivers/net/usb/cdc_mbim.c
> index bbde9884ab8a..8973abdec9f6 100644
> --- a/drivers/net/usb/cdc_mbim.c
> +++ b/drivers/net/usb/cdc_mbim.c
> @@ -158,7 +158,7 @@ static int cdc_mbim_bind(struct usbnet *dev, struct usb_interface *intf)
>    if (!cdc_ncm_comm_intf_is_mbim(intf->cur_altsetting))
>        goto err;
>
> -    ret Íc_ncm_bind_common(dev, intf, data_altsetting, 0);
> +    ret Íc_ncm_bind_common(dev, intf, data_altsetting, dev->driver_info->data);
>    if (ret)
>        goto err;
>
> @@ -582,6 +582,26 @@ static const struct driver_info cdc_mbim_info_zlp ={
>    .tx_fixup = cdc_mbim_tx_fixup,
> };
>
> +/* The spefication explicitly allows NDPs to be placed anywhere in the
> + * frame, but some devices fail unless the NDP is placed after the IP
> + * packets.  Using the CDC_NCM_FLAG_NDP_TO_END flags to force this
> + * behaviour.
> + *
> + * Note: The current implementation of this feature restricts each NTB
> + * to a single NDP, implying that multiplexed sessions cannot share an
> + * NTB. This might affect performace for multiplexed sessions.
> + */
> +static const struct driver_info cdc_mbim_info_ndp_to_end = {
> +    .description = "CDC MBIM",
> +    .flags = FLAG_NO_SETINT | FLAG_MULTI_PACKET | FLAG_WWAN,
> +    .bind = cdc_mbim_bind,
> +    .unbind = cdc_mbim_unbind,
> +    .manage_power = cdc_mbim_manage_power,
> +    .rx_fixup = cdc_mbim_rx_fixup,
> +    .tx_fixup = cdc_mbim_tx_fixup,
> +    .data = CDC_NCM_FLAG_NDP_TO_END,
> +};
> +
> static const struct usb_device_id mbim_devs[] = {
>    /* This duplicate NCM entry is intentional. MBIM devices can
>     * be disguised as NCM by default, and this is necessary to
> @@ -597,6 +617,10 @@ static const struct usb_device_id mbim_devs[] = {
>    { USB_VENDOR_AND_INTERFACE_INFO(0x0bdb, USB_CLASS_COMM, USB_CDC_SUBCLASS_MBIM, USB_CDC_PROTO_NONE),
>      .driver_info = (unsigned long)&cdc_mbim_info,
>    },
> +    /* Huawei E3372 fails unless NDP comes after the IP packets */
> +    { USB_DEVICE_AND_INTERFACE_INFO(0x12d1, 0x157d, USB_CLASS_COMM, USB_CDC_SUBCLASS_MBIM, USB_CDC_PROTO_NONE),
> +      .driver_info = (unsigned long)&cdc_mbim_info_ndp_to_end,
> +    },
>    /* default entry */
>    { USB_INTERFACE_INFO(USB_CLASS_COMM, USB_CDC_SUBCLASS_MBIM, USB_CDC_PROTO_NONE),
>      .driver_info = (unsigned long)&cdc_mbim_info_zlp,
> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> index 3b1ba8237768..1e9843a41168 100644
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -955,10 +955,18 @@ static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp(struct cdc_ncm_ctx *ctx, struct sk_
>    * NTH16 header as we would normally do. NDP isn't written to the SKB yet, and
>    * the wNdpIndex field in the header is actually not consistent with reality. It will be later.
>    */
> -    if (ctx->drvflags & CDC_NCM_FLAG_NDP_TO_END)
> +    if (ctx->drvflags & CDC_NCM_FLAG_NDP_TO_END) {
>        if (ctx->delayed_ndp16->dwSignature == sign)
>            return ctx->delayed_ndp16;
>
> +        /* We can only push a single NDP to the end. Return
> +         * NULL to send what we've already got and queue this
> +         * skb for later.
> +         */
> +        else if (ctx->delayed_ndp16->dwSignature)
> +            return NULL;
> +    }
> +
>    /* follow the chain of NDPs, looking for a match */
>    while (ndpoffset) {
>        ndp16 = (struct usb_cdc_ncm_ndp16 *)(skb->data + ndpoffset);
> --
> 2.1.4
>

Re: [PATCH net,stable] net: cdc_mbim: add "NDP to end" quirk for Huawei E3372

[David Miller]

From: Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org>
Date: Sat,  5 Dec 2015 13:01:50 +0100

> The Huawei E3372 (12d1:157d) needs this quirk in MBIM mode
> as well. Allow this by forcing the NTB to contain only a
> single NDP, and add a device specific entry for this ID.
> 
> Due to the way Huawei use device IDs, this might be applied
> to other modems as well.  It is assumed that those modems
> will be based on the same firmware and will need this quirk
> too.  If not, it will still not harm normal usage, although
> multiplexing performance could be impacted.
> 
> Cc: Enrico Mioso <mrkiko.rs-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Reported-by: Sami Farin <hvtaifwkbgefbaei-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org>

Applied and queued up for -stable, thanks.
--
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