[net-next,v6,0/3] The huawei_cdc_ncm driver / E3276 problem

Pasi Kärkkäinen <pasik@iki.fi> writes:
> On Mon, Mar 17, 2014 at 02:15:23PM +0100, Bjørn Mork wrote:
>
>> I still don't know for sure, but I do hope this bug is the real cause of
>> the problems you are having.  I'll send you a patch for testing as soon
>> as it is ready.
>> 
>
> Sure. I'll be happy to test the patch!

I ended up with a simple revert of the buggy commit, except for a
conflict due to unrelated context changes.  This seemed like the
cleanest approach given that this fix also needs to go to stable.

Attaching the first version.  Please give it a try if you can. I've
tested it somewhat myself and it doesn't seem to break anything.  Since
it's a simple revert, there isn't really that much that could go wrong
here...


Bjørn

I admite you, Bjorn: that's talent.
Let's see how it goes.
And congratulations to Pasik also.




On Mon, 17 Mar 2014, Bjørn Mork wrote:

==Date: Mon, 17 Mar 2014 15:23:22
==From: Bjørn Mork <bjorn@mork.no>
==To: Pasi Kärkkäinen <pasik@iki.fi>
==Cc: Thomas Schäfer <tschaefer@t-online.de>, Dan Williams <dcbw@redhat.com>,
==    netdev@vger.kernel.org, linux-usb@vger.kernel.org,
==    Enrico Mioso <mrkiko.rs@gmail.com>, Oliver Neukum <oliver@neukum.org>
==Subject: Re: [PATCH net-next v6 0/3] The huawei_cdc_ncm driver / E3276 problem
==
==Pasi Kärkkäinen <pasik@iki.fi> writes:
==> On Mon, Mar 17, 2014 at 02:15:23PM +0100, Bjørn Mork wrote:
==>
==>> I still don't know for sure, but I do hope this bug is the real cause of
==>> the problems you are having.  I'll send you a patch for testing as soon
==>> as it is ready.
==>> 
==>
==> Sure. I'll be happy to test the patch!
==
==I ended up with a simple revert of the buggy commit, except for a
==conflict due to unrelated context changes.  This seemed like the
==cleanest approach given that this fix also needs to go to stable.
==
==Attaching the first version.  Please give it a try if you can. I've
==tested it somewhat myself and it doesn't seem to break anything.  Since
==it's a simple revert, there isn't really that much that could go wrong
==here...
==
==
==Bjørn
==
==

On Mon, Mar 17, 2014 at 03:23:22PM +0100, Bjørn Mork wrote:
> Pasi Kärkkäinen <pasik@iki.fi> writes:
> > On Mon, Mar 17, 2014 at 02:15:23PM +0100, Bjørn Mork wrote:
> >
> >> I still don't know for sure, but I do hope this bug is the real cause of
> >> the problems you are having.  I'll send you a patch for testing as soon
> >> as it is ready.
> >> 
> >
> > Sure. I'll be happy to test the patch!
> 
> I ended up with a simple revert of the buggy commit, except for a
> conflict due to unrelated context changes.  This seemed like the
> cleanest approach given that this fix also needs to go to stable.
> 
> Attaching the first version.  Please give it a try if you can. I've
> tested it somewhat myself and it doesn't seem to break anything.  Since
> it's a simple revert, there isn't really that much that could go wrong
> here...
> 

I applied the patch on top of Linux 3.13.6 kernel and now I'm able to use the wwan0 NCM interface successfully! 
I do get an IP with a dhcp client (this failed earlier without the patch), and Internet seems to work OK. 
So the patch definitely fixes the problem for me with Huawei E3276 4G/LTE USB dongle. 

Thanks a lot!

Tested-by: Pasi Kärkkäinen <pasik@iki.fi>


-- Pasi

> 
> Bjørn
> 

> From 2ad87cde1d386acc31ac3caf66a24d24423ca721 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Bj=C3=B8rn=20Mork?= <bjorn@mork.no>
> Date: Mon, 17 Mar 2014 14:58:06 +0100
> Subject: [PATCH] net: cdc_ncm: fix control message ordering
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> Commit 6a9612e2cb22 ("net: cdc_ncm: remove ncm_parm field")
> introduced a specification violation, which caused setup
> errors for some devices. In some cases, these errors
> resulted in the device and host disagreeing about shared
> settings, with complete failure to communicate as the end
> result.
> 
> The NCM specification require that some commands are sent
> only while the NCM Data Interface is in alternate setting 0.
> Reverting the commit ensures that we follow this requirement.
> 
> Fixes: 6a9612e2cb22 ("net: cdc_ncm: remove ncm_parm field")
> Reported-by: Pasi Kärkkäinen <pasik@iki.fi>
> Reported-by: Thomas Schäfer <tschaefer@t-online.de>
> Signed-off-by: Bjørn Mork <bjorn@mork.no>
> ---
>  drivers/net/usb/cdc_ncm.c   | 48 ++++++++++++++++++++++-----------------------
>  include/linux/usb/cdc_ncm.h |  1 +
>  2 files changed, 24 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> index dbff290ed0e4..d350d2795e10 100644
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -68,7 +68,6 @@ static struct usb_driver cdc_ncm_driver;
>  static int cdc_ncm_setup(struct usbnet *dev)
>  {
>  	struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
> -	struct usb_cdc_ncm_ntb_parameters ncm_parm;
>  	u32 val;
>  	u8 flags;
>  	u8 iface_no;
> @@ -82,22 +81,22 @@ static int cdc_ncm_setup(struct usbnet *dev)
>  	err = usbnet_read_cmd(dev, USB_CDC_GET_NTB_PARAMETERS,
>  			      USB_TYPE_CLASS | USB_DIR_IN
>  			      |USB_RECIP_INTERFACE,
> -			      0, iface_no, &ncm_parm,
> -			      sizeof(ncm_parm));
> +			      0, iface_no, &ctx->ncm_parm,
> +			      sizeof(ctx->ncm_parm));
>  	if (err < 0) {
>  		dev_err(&dev->intf->dev, "failed GET_NTB_PARAMETERS\n");
>  		return err; /* GET_NTB_PARAMETERS is required */
>  	}
>  
>  	/* read correct set of parameters according to device mode */
> -	ctx->rx_max = le32_to_cpu(ncm_parm.dwNtbInMaxSize);
> -	ctx->tx_max = le32_to_cpu(ncm_parm.dwNtbOutMaxSize);
> -	ctx->tx_remainder = le16_to_cpu(ncm_parm.wNdpOutPayloadRemainder);
> -	ctx->tx_modulus = le16_to_cpu(ncm_parm.wNdpOutDivisor);
> -	ctx->tx_ndp_modulus = le16_to_cpu(ncm_parm.wNdpOutAlignment);
> +	ctx->rx_max = le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize);
> +	ctx->tx_max = le32_to_cpu(ctx->ncm_parm.dwNtbOutMaxSize);
> +	ctx->tx_remainder = le16_to_cpu(ctx->ncm_parm.wNdpOutPayloadRemainder);
> +	ctx->tx_modulus = le16_to_cpu(ctx->ncm_parm.wNdpOutDivisor);
> +	ctx->tx_ndp_modulus = le16_to_cpu(ctx->ncm_parm.wNdpOutAlignment);
>  	/* devices prior to NCM Errata shall set this field to zero */
> -	ctx->tx_max_datagrams = le16_to_cpu(ncm_parm.wNtbOutMaxDatagrams);
> -	ntb_fmt_supported = le16_to_cpu(ncm_parm.bmNtbFormatsSupported);
> +	ctx->tx_max_datagrams = le16_to_cpu(ctx->ncm_parm.wNtbOutMaxDatagrams);
> +	ntb_fmt_supported = le16_to_cpu(ctx->ncm_parm.bmNtbFormatsSupported);
>  
>  	/* there are some minor differences in NCM and MBIM defaults */
>  	if (cdc_ncm_comm_intf_is_mbim(ctx->control->cur_altsetting)) {
> @@ -146,7 +145,7 @@ static int cdc_ncm_setup(struct usbnet *dev)
>  	}
>  
>  	/* inform device about NTB input size changes */
> -	if (ctx->rx_max != le32_to_cpu(ncm_parm.dwNtbInMaxSize)) {
> +	if (ctx->rx_max != le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize)) {
>  		__le32 dwNtbInMaxSize = cpu_to_le32(ctx->rx_max);
>  
>  		err = usbnet_write_cmd(dev, USB_CDC_SET_NTB_INPUT_SIZE,
> @@ -162,14 +161,6 @@ static int cdc_ncm_setup(struct usbnet *dev)
>  		dev_dbg(&dev->intf->dev, "Using default maximum transmit length=%d\n",
>  			CDC_NCM_NTB_MAX_SIZE_TX);
>  		ctx->tx_max = CDC_NCM_NTB_MAX_SIZE_TX;
> -
> -		/* Adding a pad byte here simplifies the handling in
> -		 * cdc_ncm_fill_tx_frame, by making tx_max always
> -		 * represent the real skb max size.
> -		 */
> -		if (ctx->tx_max % usb_maxpacket(dev->udev, dev->out, 1) == 0)
> -			ctx->tx_max++;
> -
>  	}
>  
>  	/*
> @@ -439,6 +430,10 @@ advance:
>  		goto error2;
>  	}
>  
> +	/* initialize data interface */
> +	if (cdc_ncm_setup(dev))
> +		goto error2;
> +
>  	/* configure data interface */
>  	temp = usb_set_interface(dev->udev, iface_no, data_altsetting);
>  	if (temp) {
> @@ -453,12 +448,6 @@ advance:
>  		goto error2;
>  	}
>  
> -	/* initialize data interface */
> -	if (cdc_ncm_setup(dev))	{
> -		dev_dbg(&intf->dev, "cdc_ncm_setup() failed\n");
> -		goto error2;
> -	}
> -
>  	usb_set_intfdata(ctx->data, dev);
>  	usb_set_intfdata(ctx->control, dev);
>  
> @@ -475,6 +464,15 @@ advance:
>  	dev->hard_mtu = ctx->tx_max;
>  	dev->rx_urb_size = ctx->rx_max;
>  
> +	/* cdc_ncm_setup will override dwNtbOutMaxSize if it is
> +	 * outside the sane range. Adding a pad byte here if necessary
> +	 * simplifies the handling in cdc_ncm_fill_tx_frame, making
> +	 * tx_max always represent the real skb max size.
> +	 */
> +	if (ctx->tx_max != le32_to_cpu(ctx->ncm_parm.dwNtbOutMaxSize) &&
> +	    ctx->tx_max % usb_maxpacket(dev->udev, dev->out, 1) == 0)
> +		ctx->tx_max++;
> +
>  	return 0;
>  
>  error2:
> diff --git a/include/linux/usb/cdc_ncm.h b/include/linux/usb/cdc_ncm.h
> index c3fa80745996..2c14d9cdd57a 100644
> --- a/include/linux/usb/cdc_ncm.h
> +++ b/include/linux/usb/cdc_ncm.h
> @@ -88,6 +88,7 @@
>  #define cdc_ncm_data_intf_is_mbim(x)  ((x)->desc.bInterfaceProtocol == USB_CDC_MBIM_PROTO_NTB)
>  
>  struct cdc_ncm_ctx {
> +	struct usb_cdc_ncm_ntb_parameters ncm_parm;
>  	struct hrtimer tx_timer;
>  	struct tasklet_struct bh;
>  
> -- 
> 1.9.0
> 

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

Pasi Kärkkäinen <pasik@iki.fi> writes:

> I applied the patch on top of Linux 3.13.6 kernel and now I'm able to use the wwan0 NCM interface successfully! 
> I do get an IP with a dhcp client (this failed earlier without the patch), and Internet seems to work OK. 
> So the patch definitely fixes the problem for me with Huawei E3276 4G/LTE USB dongle. 
>
> Thanks a lot!
>
> Tested-by: Pasi Kärkkäinen <pasik@iki.fi>

Great!  Thanks a lot for quick testing too.  I'll submit this now.
Might be a bit late for 3.14, but should go to stable the next time
davem submits a batch there.


Bjørn
--
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