On Mon, Mar 17, 2014 at 03:23:22PM +0100, Bjørn Mork wrote:
> Pasi Kärkkäinen <pasik@xxxxxx> 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@xxxxxx>
-- Pasi
>
> Bjørn
>
> From 2ad87cde1d386acc31ac3caf66a24d24423ca721 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Bj=C3=B8rn=20Mork?= <bjorn@xxxxxxx>
> 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@xxxxxx>
> Reported-by: Thomas Schäfer <tschaefer@xxxxxxxxxxx>
> Signed-off-by: Bjørn Mork <bjorn@xxxxxxx>
> ---
> 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 linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
