[PATCH 0/1] NET: usb: cdc_ncm: adding MBIM RESET_FUNCTION request and modifying ncm bind common code

[PATCH 0/1] NET: usb: cdc_ncm: adding MBIM RESET_FUNCTION request and modifying ncm bind common code

[Daniele Palmas]

Some latest QC based modems seem not to properly accept altsetting
toggling in cdc_ncm_bind_common, making them to fail. The workaround
was to introduce an empirically decided pause to avoid the failure.

This patch introduces a different approach: for MBIM devices, instead
of toggling interfaces, the MBIM class-specific request code
RESET_FUNCTION is used in order to reset the function to its initial
state, removing the need for the pause.

Patch has been tested with a few Telit QC and Intel based MBIM modems.

Patch has also been tested with an Intel NCM based device, for
regression checking.

Daniele Palmas (1):
  NET: usb: cdc_ncm: adding MBIM RESET_FUNCTION request and modifying
    ncm bind common code

 drivers/net/usb/cdc_ncm.c    | 39 +++++++++++++++++++++++++++------------
 include/uapi/linux/usb/cdc.h |  1 +
 2 files changed, 28 insertions(+), 12 deletions(-)

-- 
2.7.4

[PATCH 1/1] NET: usb: cdc_ncm: adding MBIM RESET_FUNCTION request and modifying ncm bind common code

[Daniele Palmas]

Some latest QC based modems seem not to properly accept altsetting
toggling in cdc_ncm_bind_common, making them to fail. The workaround
was to introduce an empirically decided pause to avoid the failure.

This patch introduces a different approach: for MBIM devices, instead
of toggling interfaces, the MBIM class-specific request code
RESET_FUNCTION is used in order to reset the function to its initial
state, removing the need for the pause.

Signed-off-by: Daniele Palmas <dnlplm@gmail.com>
---
 drivers/net/usb/cdc_ncm.c    | 39 +++++++++++++++++++++++++++------------
 include/uapi/linux/usb/cdc.h |  1 +
 2 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 877c951..e8a7a76 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -498,6 +498,22 @@ static int cdc_ncm_init(struct usbnet *dev)
 		return err; /* GET_NTB_PARAMETERS is required */
 	}
 
+	/* Some modems (e.g. Telit LE922A6) need to reset the MBIM function
+	 * or they will fail to work properly.
+	 * For details on RESET_FUNCTION request see document
+	 * "USB Communication Class Subclass Specification for MBIM"
+	 * RESET_FUNCTION should be harmless for all the other MBIM modems
+	 */
+	if (cdc_ncm_comm_intf_is_mbim(ctx->control->cur_altsetting)) {
+		err = usbnet_write_cmd(dev, USB_CDC_RESET_FUNCTION,
+				       USB_TYPE_CLASS | USB_DIR_OUT
+				       | USB_RECIP_INTERFACE,
+				       0, iface_no, NULL, 0);
+		if (err < 0) {
+			dev_err(&dev->intf->dev, "failed RESET_FUNCTION\n");
+		}
+	}
+
 	/* set CRC Mode */
 	if (cdc_ncm_flags(dev) & USB_CDC_NCM_NCAP_CRC_MODE) {
 		dev_dbg(&dev->intf->dev, "Setting CRC mode off\n");
@@ -842,25 +858,24 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8 data_
 	/* Reset data interface. Some devices will not reset properly
 	 * unless they are configured first.  Toggle the altsetting to
 	 * force a reset
+	 * This is applied only to ncm devices, since it has been verified
+	 * to cause issues with some MBIM modems (e.g. Telit LE922A6).
+	 * MBIM devices reset is achieved using MBIM request RESET_FUNCTION
+	 * in cdc_ncm_init
 	 */
-	usb_set_interface(dev->udev, iface_no, data_altsetting);
-	temp = usb_set_interface(dev->udev, iface_no, 0);
-	if (temp) {
-		dev_dbg(&intf->dev, "set interface failed\n");
-		goto error2;
+	if (!cdc_ncm_comm_intf_is_mbim(intf->cur_altsetting)) {
+		usb_set_interface(dev->udev, iface_no, data_altsetting);
+		temp = usb_set_interface(dev->udev, iface_no, 0);
+		if (temp) {
+			dev_dbg(&intf->dev, "set interface failed\n");
+			goto error2;
+		}
 	}
 
 	/* initialize basic device settings */
 	if (cdc_ncm_init(dev))
 		goto error2;
 
-	/* Some firmwares need a pause here or they will silently fail
-	 * to set up the interface properly.  This value was decided
-	 * empirically on a Sierra Wireless MC7455 running 02.08.02.00
-	 * firmware.
-	 */
-	usleep_range(10000, 20000);
-
 	/* configure data interface */
 	temp = usb_set_interface(dev->udev, iface_no, data_altsetting);
 	if (temp) {
diff --git a/include/uapi/linux/usb/cdc.h b/include/uapi/linux/usb/cdc.h
index e2bc417..30258fb 100644
--- a/include/uapi/linux/usb/cdc.h
+++ b/include/uapi/linux/usb/cdc.h
@@ -231,6 +231,7 @@ struct usb_cdc_mbim_extended_desc {
 
 #define USB_CDC_SEND_ENCAPSULATED_COMMAND	0x00
 #define USB_CDC_GET_ENCAPSULATED_RESPONSE	0x01
+#define USB_CDC_RESET_FUNCTION			0x05
 #define USB_CDC_REQ_SET_LINE_CODING		0x20
 #define USB_CDC_REQ_GET_LINE_CODING		0x21
 #define USB_CDC_REQ_SET_CONTROL_LINE_STATE	0x22
-- 
2.7.4

Re: [PATCH 1/1] NET: usb: cdc_ncm: adding MBIM RESET_FUNCTION request and modifying ncm bind common code

[Bjørn Mork]

Finally, I found my modems (or at least a number of them) again today.
But I'm sorry to say, that the troublesome Huawei E3372h-153 is still
giving us a hard time.  It does not work with your patch. The symptom is
the same as earlier:  The modem returns MBIM frames with 32bit headers.

So for now, I have to NAK this patch.

I am sure we can find a good solution that makes all of these modems
work, but I cannot support a patch that breaks previously working
configurations. Sorry.  I'll do a few experiments and see if there is a
simple fix for this.  Otherwise we'll probably have to do the quirk
game.


Bjørn

Re: [PATCH 1/1] NET: usb: cdc_ncm: adding MBIM RESET_FUNCTION request and modifying ncm bind common code

[Bjørn Mork]

Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org> writes:

> Finally, I found my modems (or at least a number of them) again today.
> But I'm sorry to say, that the troublesome Huawei E3372h-153 is still
> giving us a hard time.  It does not work with your patch. The symptom is
> the same as earlier:  The modem returns MBIM frames with 32bit headers.
>
> So for now, I have to NAK this patch.
>
> I am sure we can find a good solution that makes all of these modems
> work, but I cannot support a patch that breaks previously working
> configurations. Sorry.  I'll do a few experiments and see if there is a
> simple fix for this.  Otherwise we'll probably have to do the quirk
> game.


This is a proof-of-concept only, but it appears to be working.  Please
test with your device(s) too.  It's still mostly your code, as you can
see.

If this turns out to work, then I believe we should refactor
cdc_ncm_init() and cdc_ncm_bind_common() to make the whole
initialisation sequence a bit cleaner.  And maybe also include
cdc_mbim_bind().  Ideally, the MBIM specific RESET should happen there
instead of "polluting" the NCM driver with MBIM specific code.

But anyway:  The sequence that seems to work for both the  E3372h-153
and the EM7455 is

 USB_CDC_GET_NTB_PARAMETERS
 USB_CDC_RESET_FUNCTION
 usb_set_interface(dev->udev, 'data interface no', 0);
 remaining parts of cdc_ncm_init(), excluding USB_CDC_GET_NTB_PARAMETERS
 usb_set_interface(dev->udev, 'data interface no', 'data alt setting');

without any additional delay between the two usb_set_interface() calls.
So the major difference from your patch is that I moved the two control
requests out of cdc_ncm_init() to allow running them _before_ setting
the data interface to altsetting 0.

But maybe I was just lucky.  This was barely proof tested.  Needs a lot
more testing and cleanups as suggested.  I'd appreciate it if you
continued that, as I don't really have any time for it...

FWIW, I also ran a quick test with a D-Link DWM-156A7 (Mediatek MBIM
firmware) and a Huawei E367 (Qualcomm device with early Huawei MBIM
firmware, distinctly different from the E3372h-153 and most other
MBIM devices I've seen)



Bjørn

---
 drivers/net/usb/cdc_ncm.c    | 48 ++++++++++++++++++++++++++++----------------
 include/uapi/linux/usb/cdc.h |  1 +
 2 files changed, 32 insertions(+), 17 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 877c9516e781..be019cbf1719 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -488,16 +488,6 @@ static int cdc_ncm_init(struct usbnet *dev)
 	u8 iface_no = ctx->control->cur_altsetting->desc.bInterfaceNumber;
 	int err;
 
-	err = usbnet_read_cmd(dev, USB_CDC_GET_NTB_PARAMETERS,
-			      USB_TYPE_CLASS | USB_DIR_IN
-			      |USB_RECIP_INTERFACE,
-			      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 */
-	}
-
 	/* set CRC Mode */
 	if (cdc_ncm_flags(dev) & USB_CDC_NCM_NCAP_CRC_MODE) {
 		dev_dbg(&dev->intf->dev, "Setting CRC mode off\n");
@@ -837,12 +827,43 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8 data_
 		}
 	}
 
+	iface_no = ctx->control->cur_altsetting->desc.bInterfaceNumber;
+	temp = usbnet_read_cmd(dev, USB_CDC_GET_NTB_PARAMETERS,
+			       USB_TYPE_CLASS | USB_DIR_IN
+			       | USB_RECIP_INTERFACE,
+			       0, iface_no, &ctx->ncm_parm,
+			       sizeof(ctx->ncm_parm));
+	if (temp < 0) {
+		dev_err(&dev->intf->dev, "failed GET_NTB_PARAMETERS\n");
+		goto error; /* GET_NTB_PARAMETERS is required */
+	}
+
+	/* Some modems (e.g. Telit LE922A6) need to reset the MBIM function
+	 * or they will fail to work properly.
+	 * For details on RESET_FUNCTION request see document
+	 * "USB Communication Class Subclass Specification for MBIM"
+	 * RESET_FUNCTION should be harmless for all the other MBIM modems
+	 */
+	if (cdc_ncm_comm_intf_is_mbim(ctx->control->cur_altsetting)) {
+		temp = usbnet_write_cmd(dev, USB_CDC_RESET_FUNCTION,
+					USB_TYPE_CLASS | USB_DIR_OUT
+					| USB_RECIP_INTERFACE,
+					0, iface_no, NULL, 0);
+		if (temp < 0)
+			dev_err(&dev->intf->dev, "failed RESET_FUNCTION\n");
+	}
+
 	iface_no = ctx->data->cur_altsetting->desc.bInterfaceNumber;
 
 	/* Reset data interface. Some devices will not reset properly
 	 * unless they are configured first.  Toggle the altsetting to
 	 * force a reset
+	 * This is applied only to ncm devices, since it has been verified
+	 * to cause issues with some MBIM modems (e.g. Telit LE922A6).
+	 * MBIM devices reset is achieved using MBIM request RESET_FUNCTION
+	 * in cdc_ncm_init
 	 */
+
 	usb_set_interface(dev->udev, iface_no, data_altsetting);
 	temp = usb_set_interface(dev->udev, iface_no, 0);
 	if (temp) {
@@ -854,13 +875,6 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8 data_
 	if (cdc_ncm_init(dev))
 		goto error2;
 
-	/* Some firmwares need a pause here or they will silently fail
-	 * to set up the interface properly.  This value was decided
-	 * empirically on a Sierra Wireless MC7455 running 02.08.02.00
-	 * firmware.
-	 */
-	usleep_range(10000, 20000);

Re: [PATCH 1/1] NET: usb: cdc_ncm: adding MBIM RESET_FUNCTION request and modifying ncm bind common code

[Daniele Palmas]

2016-11-26 22:17 GMT+01:00 Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org>:
> Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org> writes:
>
>> Finally, I found my modems (or at least a number of them) again today.
>> But I'm sorry to say, that the troublesome Huawei E3372h-153 is still
>> giving us a hard time.  It does not work with your patch. The symptom is
>> the same as earlier:  The modem returns MBIM frames with 32bit headers.
>>
>> So for now, I have to NAK this patch.
>>
>> I am sure we can find a good solution that makes all of these modems
>> work, but I cannot support a patch that breaks previously working
>> configurations. Sorry.  I'll do a few experiments and see if there is a
>> simple fix for this.  Otherwise we'll probably have to do the quirk
>> game.
>
>
> This is a proof-of-concept only, but it appears to be working.  Please
> test with your device(s) too.  It's still mostly your code, as you can
> see.

Sorry, this does not work :-(

The problem is always in the altsetting toggle: if I comment that
part, your patch is working fine.

I'm now wondering if the safest thing is to add a very simple quirk in
cdc_mbim that makes this toggle not to be applied with the buggy
modems and then unconditionally add the RESET_FUNCTION request in
cdc_mbim_bind after cdc_ncm_bind_common has been executed.

Daniele

>
> If this turns out to work, then I believe we should refactor
> cdc_ncm_init() and cdc_ncm_bind_common() to make the whole
> initialisation sequence a bit cleaner.  And maybe also include
> cdc_mbim_bind().  Ideally, the MBIM specific RESET should happen there
> instead of "polluting" the NCM driver with MBIM specific code.
>
> But anyway:  The sequence that seems to work for both the  E3372h-153
> and the EM7455 is
>
>  USB_CDC_GET_NTB_PARAMETERS
>  USB_CDC_RESET_FUNCTION
>  usb_set_interface(dev->udev, 'data interface no', 0);
>  remaining parts of cdc_ncm_init(), excluding USB_CDC_GET_NTB_PARAMETERS
>  usb_set_interface(dev->udev, 'data interface no', 'data alt setting');
>
> without any additional delay between the two usb_set_interface() calls.
> So the major difference from your patch is that I moved the two control
> requests out of cdc_ncm_init() to allow running them _before_ setting
> the data interface to altsetting 0.
>
> But maybe I was just lucky.  This was barely proof tested.  Needs a lot
> more testing and cleanups as suggested.  I'd appreciate it if you
> continued that, as I don't really have any time for it...
>
> FWIW, I also ran a quick test with a D-Link DWM-156A7 (Mediatek MBIM
> firmware) and a Huawei E367 (Qualcomm device with early Huawei MBIM
> firmware, distinctly different from the E3372h-153 and most other
> MBIM devices I've seen)
>
>
>
> Bjørn
>
> ---
>  drivers/net/usb/cdc_ncm.c    | 48 ++++++++++++++++++++++++++++----------------
>  include/uapi/linux/usb/cdc.h |  1 +
>  2 files changed, 32 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> index 877c9516e781..be019cbf1719 100644
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -488,16 +488,6 @@ static int cdc_ncm_init(struct usbnet *dev)
>         u8 iface_no = ctx->control->cur_altsetting->desc.bInterfaceNumber;
>         int err;
>
> -       err = usbnet_read_cmd(dev, USB_CDC_GET_NTB_PARAMETERS,
> -                             USB_TYPE_CLASS | USB_DIR_IN
> -                             |USB_RECIP_INTERFACE,
> -                             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 */
> -       }
> -
>         /* set CRC Mode */
>         if (cdc_ncm_flags(dev) & USB_CDC_NCM_NCAP_CRC_MODE) {
>                 dev_dbg(&dev->intf->dev, "Setting CRC mode off\n");
> @@ -837,12 +827,43 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8 data_
>                 }
>         }
>
> +       iface_no = ctx->control->cur_altsetting->desc.bInterfaceNumber;
> +       temp = usbnet_read_cmd(dev, USB_CDC_GET_NTB_PARAMETERS,
> +                              USB_TYPE_CLASS | USB_DIR_IN
> +                              | USB_RECIP_INTERFACE,
> +                              0, iface_no, &ctx->ncm_parm,
> +                              sizeof(ctx->ncm_parm));
> +       if (temp < 0) {
> +               dev_err(&dev->intf->dev, "failed GET_NTB_PARAMETERS\n");
> +               goto error; /* GET_NTB_PARAMETERS is required */
> +       }
> +
> +       /* Some modems (e.g. Telit LE922A6) need to reset the MBIM function
> +        * or they will fail to work properly.
> +        * For details on RESET_FUNCTION request see document
> +        * "USB Communication Class Subclass Specification for MBIM"
> +        * RESET_FUNCTION should be harmless for all the other MBIM modems
> +        */
> +       if (cdc_ncm_comm_intf_is_mbim(ctx->control->cur_altsetting)) {
> +               temp = usbnet_write_cmd(dev, USB_CDC_RESET_FUNCTION,
> +                                       USB_TYPE_CLASS | USB_DIR_OUT
> +                                       | USB_RECIP_INTERFACE,
> +                                       0, iface_no, NULL, 0);
> +               if (temp < 0)
> +                       dev_err(&dev->intf->dev, "failed RESET_FUNCTION\n");
> +       }
> +
>         iface_no = ctx->data->cur_altsetting->desc.bInterfaceNumber;
>
>         /* Reset data interface. Some devices will not reset properly
>          * unless they are configured first.  Toggle the altsetting to
>          * force a reset
> +        * This is applied only to ncm devices, since it has been verified
> +        * to cause issues with some MBIM modems (e.g. Telit LE922A6).
> +        * MBIM devices reset is achieved using MBIM request RESET_FUNCTION
> +        * in cdc_ncm_init
>          */
> +
>         usb_set_interface(dev->udev, iface_no, data_altsetting);
>         temp = usb_set_interface(dev->udev, iface_no, 0);
>         if (temp) {
> @@ -854,13 +875,6 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8 data_
>         if (cdc_ncm_init(dev))
>                 goto error2;
>
> -       /* Some firmwares need a pause here or they will silently fail
> -        * to set up the interface properly.  This value was decided
> -        * empirically on a Sierra Wireless MC7455 running 02.08.02.00
> -        * firmware.
> -        */
> -       usleep_range(10000, 20000);
> -
>         /* configure data interface */
>         temp = usb_set_interface(dev->udev, iface_no, data_altsetting);
>         if (temp) {
> diff --git a/include/uapi/linux/usb/cdc.h b/include/uapi/linux/usb/cdc.h
> index e2bc417b243b..30258fb229e6 100644
> --- a/include/uapi/linux/usb/cdc.h
> +++ b/include/uapi/linux/usb/cdc.h
> @@ -231,6 +231,7 @@ struct usb_cdc_mbim_extended_desc {
>
>  #define USB_CDC_SEND_ENCAPSULATED_COMMAND      0x00
>  #define USB_CDC_GET_ENCAPSULATED_RESPONSE      0x01
> +#define USB_CDC_RESET_FUNCTION                 0x05
>  #define USB_CDC_REQ_SET_LINE_CODING            0x20
>  #define USB_CDC_REQ_GET_LINE_CODING            0x21
>  #define USB_CDC_REQ_SET_CONTROL_LINE_STATE     0x22
> --
> 2.10.2
>
>
>
--
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 1/1] NET: usb: cdc_ncm: adding MBIM RESET_FUNCTION request and modifying ncm bind common code

[Daniele Palmas]

Hi,

2016-11-28 12:23 GMT+01:00 Daniele Palmas <dnlplm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> 2016-11-26 22:17 GMT+01:00 Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org>:
>> Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org> writes:
>>
>>> Finally, I found my modems (or at least a number of them) again today.
>>> But I'm sorry to say, that the troublesome Huawei E3372h-153 is still
>>> giving us a hard time.  It does not work with your patch. The symptom is
>>> the same as earlier:  The modem returns MBIM frames with 32bit headers.
>>>
>>> So for now, I have to NAK this patch.
>>>
>>> I am sure we can find a good solution that makes all of these modems
>>> work, but I cannot support a patch that breaks previously working
>>> configurations. Sorry.  I'll do a few experiments and see if there is a
>>> simple fix for this.  Otherwise we'll probably have to do the quirk
>>> game.
>>
>>
>> This is a proof-of-concept only, but it appears to be working.  Please
>> test with your device(s) too.  It's still mostly your code, as you can
>> see.
>
> Sorry, this does not work :-(
>
> The problem is always in the altsetting toggle: if I comment that
> part, your patch is working fine.
>
> I'm now wondering if the safest thing is to add a very simple quirk in
> cdc_mbim that makes this toggle not to be applied with the buggy
> modems and then unconditionally add the RESET_FUNCTION request in
> cdc_mbim_bind after cdc_ncm_bind_common has been executed.
>

I went back to this and further checking revealed that MBIM function
reset is not needed and the only problem is related to commit
48906f62c96cc2cd35753e59310cb70eb08cc6a5 cdc_ncm: toggle altsetting to
force reset before setup, that, however, is mandatory for some Huawei
modems.

My proposal is to add something like
CDC_MBIM_FLAG_AVOID_ALTSETTING_TOGGLE quirk to avoid the toggle.

To have a conservative approach, I would add only Telit LE922 to this
quirk and keep also the usleep_range delay, since I do not have a
clear view on all the VIDs/PIDs affected by this quirk. Then other
modems, once tested, can be added to the quirk if the delay is not
enough.

If this approach, though ugly, has chances to be accepted I can write
a new patch.

Thanks,
Daniele

> Daniele
>
>>
>> If this turns out to work, then I believe we should refactor
>> cdc_ncm_init() and cdc_ncm_bind_common() to make the whole
>> initialisation sequence a bit cleaner.  And maybe also include
>> cdc_mbim_bind().  Ideally, the MBIM specific RESET should happen there
>> instead of "polluting" the NCM driver with MBIM specific code.
>>
>> But anyway:  The sequence that seems to work for both the  E3372h-153
>> and the EM7455 is
>>
>>  USB_CDC_GET_NTB_PARAMETERS
>>  USB_CDC_RESET_FUNCTION
>>  usb_set_interface(dev->udev, 'data interface no', 0);
>>  remaining parts of cdc_ncm_init(), excluding USB_CDC_GET_NTB_PARAMETERS
>>  usb_set_interface(dev->udev, 'data interface no', 'data alt setting');
>>
>> without any additional delay between the two usb_set_interface() calls.
>> So the major difference from your patch is that I moved the two control
>> requests out of cdc_ncm_init() to allow running them _before_ setting
>> the data interface to altsetting 0.
>>
>> But maybe I was just lucky.  This was barely proof tested.  Needs a lot
>> more testing and cleanups as suggested.  I'd appreciate it if you
>> continued that, as I don't really have any time for it...
>>
>> FWIW, I also ran a quick test with a D-Link DWM-156A7 (Mediatek MBIM
>> firmware) and a Huawei E367 (Qualcomm device with early Huawei MBIM
>> firmware, distinctly different from the E3372h-153 and most other
>> MBIM devices I've seen)
>>
>>
>>
>> Bjørn
>>
>> ---
>>  drivers/net/usb/cdc_ncm.c    | 48 ++++++++++++++++++++++++++++----------------
>>  include/uapi/linux/usb/cdc.h |  1 +
>>  2 files changed, 32 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
>> index 877c9516e781..be019cbf1719 100644
>> --- a/drivers/net/usb/cdc_ncm.c
>> +++ b/drivers/net/usb/cdc_ncm.c
>> @@ -488,16 +488,6 @@ static int cdc_ncm_init(struct usbnet *dev)
>>         u8 iface_no = ctx->control->cur_altsetting->desc.bInterfaceNumber;
>>         int err;
>>
>> -       err = usbnet_read_cmd(dev, USB_CDC_GET_NTB_PARAMETERS,
>> -                             USB_TYPE_CLASS | USB_DIR_IN
>> -                             |USB_RECIP_INTERFACE,
>> -                             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 */
>> -       }
>> -
>>         /* set CRC Mode */
>>         if (cdc_ncm_flags(dev) & USB_CDC_NCM_NCAP_CRC_MODE) {
>>                 dev_dbg(&dev->intf->dev, "Setting CRC mode off\n");
>> @@ -837,12 +827,43 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8 data_
>>                 }
>>         }
>>
>> +       iface_no = ctx->control->cur_altsetting->desc.bInterfaceNumber;
>> +       temp = usbnet_read_cmd(dev, USB_CDC_GET_NTB_PARAMETERS,
>> +                              USB_TYPE_CLASS | USB_DIR_IN
>> +                              | USB_RECIP_INTERFACE,
>> +                              0, iface_no, &ctx->ncm_parm,
>> +                              sizeof(ctx->ncm_parm));
>> +       if (temp < 0) {
>> +               dev_err(&dev->intf->dev, "failed GET_NTB_PARAMETERS\n");
>> +               goto error; /* GET_NTB_PARAMETERS is required */
>> +       }
>> +
>> +       /* Some modems (e.g. Telit LE922A6) need to reset the MBIM function
>> +        * or they will fail to work properly.
>> +        * For details on RESET_FUNCTION request see document
>> +        * "USB Communication Class Subclass Specification for MBIM"
>> +        * RESET_FUNCTION should be harmless for all the other MBIM modems
>> +        */
>> +       if (cdc_ncm_comm_intf_is_mbim(ctx->control->cur_altsetting)) {
>> +               temp = usbnet_write_cmd(dev, USB_CDC_RESET_FUNCTION,
>> +                                       USB_TYPE_CLASS | USB_DIR_OUT
>> +                                       | USB_RECIP_INTERFACE,
>> +                                       0, iface_no, NULL, 0);
>> +               if (temp < 0)
>> +                       dev_err(&dev->intf->dev, "failed RESET_FUNCTION\n");
>> +       }
>> +
>>         iface_no = ctx->data->cur_altsetting->desc.bInterfaceNumber;
>>
>>         /* Reset data interface. Some devices will not reset properly
>>          * unless they are configured first.  Toggle the altsetting to
>>          * force a reset
>> +        * This is applied only to ncm devices, since it has been verified
>> +        * to cause issues with some MBIM modems (e.g. Telit LE922A6).
>> +        * MBIM devices reset is achieved using MBIM request RESET_FUNCTION
>> +        * in cdc_ncm_init
>>          */
>> +
>>         usb_set_interface(dev->udev, iface_no, data_altsetting);
>>         temp = usb_set_interface(dev->udev, iface_no, 0);
>>         if (temp) {
>> @@ -854,13 +875,6 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8 data_
>>         if (cdc_ncm_init(dev))
>>                 goto error2;
>>
>> -       /* Some firmwares need a pause here or they will silently fail
>> -        * to set up the interface properly.  This value was decided
>> -        * empirically on a Sierra Wireless MC7455 running 02.08.02.00
>> -        * firmware.
>> -        */
>> -       usleep_range(10000, 20000);
>> -
>>         /* configure data interface */
>>         temp = usb_set_interface(dev->udev, iface_no, data_altsetting);
>>         if (temp) {
>> diff --git a/include/uapi/linux/usb/cdc.h b/include/uapi/linux/usb/cdc.h
>> index e2bc417b243b..30258fb229e6 100644
>> --- a/include/uapi/linux/usb/cdc.h
>> +++ b/include/uapi/linux/usb/cdc.h
>> @@ -231,6 +231,7 @@ struct usb_cdc_mbim_extended_desc {
>>
>>  #define USB_CDC_SEND_ENCAPSULATED_COMMAND      0x00
>>  #define USB_CDC_GET_ENCAPSULATED_RESPONSE      0x01
>> +#define USB_CDC_RESET_FUNCTION                 0x05
>>  #define USB_CDC_REQ_SET_LINE_CODING            0x20
>>  #define USB_CDC_REQ_GET_LINE_CODING            0x21
>>  #define USB_CDC_REQ_SET_CONTROL_LINE_STATE     0x22
>> --
>> 2.10.2
>>
>>
>>
--
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 1/1] NET: usb: cdc_ncm: adding MBIM RESET_FUNCTION request and modifying ncm bind common code

[Bjørn Mork]

Daniele Palmas <dnlplm@gmail.com> writes:

> I went back to this and further checking revealed that MBIM function
> reset is not needed and the only problem is related to commit
> 48906f62c96cc2cd35753e59310cb70eb08cc6a5 cdc_ncm: toggle altsetting to
> force reset before setup, that, however, is mandatory for some Huawei
> modems.

Interesting.  That does sound like an odd firmware bug to me. I have a
bit of a hard time understanding how this can be.  Using data interface
altsetting 0 to reset the function is documented in the NCM spec:

"
  7.2 Using Alternate Settings to Reset an NCM Function

  To place the network aspects of a function in a known state, the host
  shall:
  • select alternate setting 0 of the NCM Data Interface (this is the
    setting with no endpoints). This can be done explicitly using
    SetInterface, or implicitly using SetConfiguration. See [USB30] for
    details.
  • select the NCM operational parameters by sending commands to the NCM
    Communication Interface, then
  • select the second alternate interface setting of the NCM Data
    Interface (this is the setting with a bulk IN endpoint and a bulk
    OUT endpoint).

  Whenever alternate setting 0 is selected by the host, the function
  shall:
  • flush function buffers
  • reset the packet filter to its default state
  • clear all multicast address filters
  • clear all power filters set using
    SetEthernetPowerManagementPatternFilter
  • reset statistics counters to zero
  • restore its Ethernet address to its default state
  • reset its IN NTB size to the value given by field dwNtbInMaxSize
    from the NTB Parameter Struc- ture
  • reset the NTB format to NTB-16
  • reset the current Maximum Datagram Size to a function-specific
    default. If SetMaxDatagramSize is not supported, then the maximum
    datagram size shall be the same as the value in wMaxSegmentSize of
    the Ethernet Networking Functional Descriptor. If SetMaxDatagramSize
    is supported by the function, then the host must either query the
    function to determine the current effective maximum datagram size,
    or must explicitly set the maximum datagram size. If the host wishes
    to set the Maximum Datagram Size, it may do so prior to selecting
    the second alternate interface set- ting of the data
    interface. Doing so will ensure that the change takes effect prior
    to send or receiving data.
  • reset CRC mode so that the function will not append CRCs to
    datagrams sent on the IN pipe
  • reset NTB sequence numbers to zero

  When the host selects the second alternate interface setting of the
  NCM Data Interface, the function shall perform the following actions
  in the following order.
  • If connected to the network, the function shall send a
    ConnectionSpeedChange notification to the host indicating the
    current connection speed.
  • Whether connected or not, the function shall then send a
    NetworkConnection notification to the host, with wValue indicating
    the current state of network connectivity
"

The addition of the "RESET" request in MBIM did not change these
requirements AFAICS.  Supporting NCM style "altsetting 0 reset" is
required by default inheritance.  The description of dual NCM/MBIM
functions in the MBIM spec further emphasizes this:

"
  Regardless of the previous sequence of SET_INTERFACE commands targeting
  the Communication Interface, the host is able to put the function into
  a defined state by the following sequence:
  
  1. SET_INTERFACE(Data Interface, 0)
  2. SET_INTERFACE(Communication Interface, desired alternate setting)
  3. Sending the required class commands for the targeted alternate
    setting
  4. SET_INTERFACE(Data Interface, 1 if Communication Interface,0 and
    Data Interface,2 Communication Interface 1)
"


This, along with the lack of *any* sort of discussion of the
relation/interfaction between "MBIM RESET" and "data interface
altsetting 0" makes the MBIM RESET control request either ambigious or
redundant.  Or both...

FWIW, I've always considered RESET a symptom of the sloppy MBIM spec
development.  It did not exactly work to their advantage that the first
(and only?) published errata simply was an excuse to add new features
(the "MBIM extended functional descriptor" and "MBIM loopback testmode")
without updating the revision number.  Causing even more confusion,
since we now have two different MBIMv1.0 specs.

Well, whatever.  No need to get all frustrated about that. We have to
deal with whatever the firmware developers serve us.  And,
unfortunately, it is not surprising that unclear and ambigious specs
leads to incompatible firmware implementations.

I wonder, what happens if you unload the MBIM driver and let the USB
core reset the data interface to altsetting 0?  Will the device then
fail when the driver is loaded again?  If not, then where is the
difference?  And can we possibly reorder the driver set_interface
requests to make them similar to the USB core reset

Until now, I was under the impression that this was the
only documented way to reset both NCM and MBIM functions (since the
RESET is MBIM specific),

> My proposal is to add something like
> CDC_MBIM_FLAG_AVOID_ALTSETTING_TOGGLE quirk to avoid the toggle.
>
> To have a conservative approach, I would add only Telit LE922 to this
> quirk and keep also the usleep_range delay, since I do not have a
> clear view on all the VIDs/PIDs affected by this quirk. Then other
> modems, once tested, can be added to the quirk if the delay is not
> enough.
>
> If this approach, though ugly, has chances to be accepted I can write
> a new patch.

Yes, that sounds like the best approach at the moment.  I have
unfortunately not as much time to experiment with this as I seem to
need.  And the EM7455 workaround (the delay) is a bit hard to verify,
since it is easily affected by small additional delays caused by the
debugging itself.  This can cause bogus test results.


Bjørn

Re: [PATCH 1/1] NET: usb: cdc_ncm: adding MBIM RESET_FUNCTION request and modifying ncm bind common code

[Daniele Palmas]

Hi,

2016-12-05 11:10 GMT+01:00 Bjørn Mork <bjorn@mork.no>:
> Daniele Palmas <dnlplm@gmail.com> writes:
>
>> I went back to this and further checking revealed that MBIM function
>> reset is not needed and the only problem is related to commit
>> 48906f62c96cc2cd35753e59310cb70eb08cc6a5 cdc_ncm: toggle altsetting to
>> force reset before setup, that, however, is mandatory for some Huawei
>> modems.
>
> Interesting.  That does sound like an odd firmware bug to me. I have a
> bit of a hard time understanding how this can be.  Using data interface
> altsetting 0 to reset the function is documented in the NCM spec:
>

Agree, this is likely a firmware bug and it is also proved by all the
other modems that accept the procedure without issues.

> "
>   7.2 Using Alternate Settings to Reset an NCM Function
>
>   To place the network aspects of a function in a known state, the host
>   shall:
>   • select alternate setting 0 of the NCM Data Interface (this is the
>     setting with no endpoints). This can be done explicitly using
>     SetInterface, or implicitly using SetConfiguration. See [USB30] for
>     details.
>   • select the NCM operational parameters by sending commands to the NCM
>     Communication Interface, then
>   • select the second alternate interface setting of the NCM Data
>     Interface (this is the setting with a bulk IN endpoint and a bulk
>     OUT endpoint).
>
>   Whenever alternate setting 0 is selected by the host, the function
>   shall:
>   • flush function buffers
>   • reset the packet filter to its default state
>   • clear all multicast address filters
>   • clear all power filters set using
>     SetEthernetPowerManagementPatternFilter
>   • reset statistics counters to zero
>   • restore its Ethernet address to its default state
>   • reset its IN NTB size to the value given by field dwNtbInMaxSize
>     from the NTB Parameter Struc- ture
>   • reset the NTB format to NTB-16
>   • reset the current Maximum Datagram Size to a function-specific
>     default. If SetMaxDatagramSize is not supported, then the maximum
>     datagram size shall be the same as the value in wMaxSegmentSize of
>     the Ethernet Networking Functional Descriptor. If SetMaxDatagramSize
>     is supported by the function, then the host must either query the
>     function to determine the current effective maximum datagram size,
>     or must explicitly set the maximum datagram size. If the host wishes
>     to set the Maximum Datagram Size, it may do so prior to selecting
>     the second alternate interface set- ting of the data
>     interface. Doing so will ensure that the change takes effect prior
>     to send or receiving data.
>   • reset CRC mode so that the function will not append CRCs to
>     datagrams sent on the IN pipe
>   • reset NTB sequence numbers to zero
>
>   When the host selects the second alternate interface setting of the
>   NCM Data Interface, the function shall perform the following actions
>   in the following order.
>   • If connected to the network, the function shall send a
>     ConnectionSpeedChange notification to the host indicating the
>     current connection speed.
>   • Whether connected or not, the function shall then send a
>     NetworkConnection notification to the host, with wValue indicating
>     the current state of network connectivity
> "
>
> The addition of the "RESET" request in MBIM did not change these
> requirements AFAICS.  Supporting NCM style "altsetting 0 reset" is
> required by default inheritance.  The description of dual NCM/MBIM
> functions in the MBIM spec further emphasizes this:
>
> "
>   Regardless of the previous sequence of SET_INTERFACE commands targeting
>   the Communication Interface, the host is able to put the function into
>   a defined state by the following sequence:
>
>   1. SET_INTERFACE(Data Interface, 0)
>   2. SET_INTERFACE(Communication Interface, desired alternate setting)
>   3. Sending the required class commands for the targeted alternate
>     setting
>   4. SET_INTERFACE(Data Interface, 1 if Communication Interface,0 and
>     Data Interface,2 Communication Interface 1)
> "
>
>
> This, along with the lack of *any* sort of discussion of the
> relation/interfaction between "MBIM RESET" and "data interface
> altsetting 0" makes the MBIM RESET control request either ambigious or
> redundant.  Or both...
>
> FWIW, I've always considered RESET a symptom of the sloppy MBIM spec
> development.  It did not exactly work to their advantage that the first
> (and only?) published errata simply was an excuse to add new features
> (the "MBIM extended functional descriptor" and "MBIM loopback testmode")
> without updating the revision number.  Causing even more confusion,
> since we now have two different MBIMv1.0 specs.
>
> Well, whatever.  No need to get all frustrated about that. We have to
> deal with whatever the firmware developers serve us.  And,
> unfortunately, it is not surprising that unclear and ambigious specs
> leads to incompatible firmware implementations.
>
> I wonder, what happens if you unload the MBIM driver and let the USB
> core reset the data interface to altsetting 0?  Will the device then
> fail when the driver is loaded again?  If not, then where is the
> difference?  And can we possibly reorder the driver set_interface
> requests to make them similar to the USB core reset
>

I will check this.

Thanks,
Daniele

> Until now, I was under the impression that this was the
> only documented way to reset both NCM and MBIM functions (since the
> RESET is MBIM specific),
>
>> My proposal is to add something like
>> CDC_MBIM_FLAG_AVOID_ALTSETTING_TOGGLE quirk to avoid the toggle.
>>
>> To have a conservative approach, I would add only Telit LE922 to this
>> quirk and keep also the usleep_range delay, since I do not have a
>> clear view on all the VIDs/PIDs affected by this quirk. Then other
>> modems, once tested, can be added to the quirk if the delay is not
>> enough.
>>
>> If this approach, though ugly, has chances to be accepted I can write
>> a new patch.
>
> Yes, that sounds like the best approach at the moment.  I have
> unfortunately not as much time to experiment with this as I seem to
> need.  And the EM7455 workaround (the delay) is a bit hard to verify,
> since it is easily affected by small additional delays caused by the
> debugging itself.  This can cause bogus test results.
>
>
> Bjørn

Re: [PATCH 1/1] NET: usb: cdc_ncm: adding MBIM RESET_FUNCTION request and modifying ncm bind common code

[Daniele Palmas]

2016-12-05 14:04 GMT+01:00 Daniele Palmas <dnlplm@gmail.com>:
> Hi,
>
> 2016-12-05 11:10 GMT+01:00 Bjørn Mork <bjorn@mork.no>:
>> Daniele Palmas <dnlplm@gmail.com> writes:
>>
>>> I went back to this and further checking revealed that MBIM function
>>> reset is not needed and the only problem is related to commit
>>> 48906f62c96cc2cd35753e59310cb70eb08cc6a5 cdc_ncm: toggle altsetting to
>>> force reset before setup, that, however, is mandatory for some Huawei
>>> modems.
>>
>> Interesting.  That does sound like an odd firmware bug to me. I have a
>> bit of a hard time understanding how this can be.  Using data interface
>> altsetting 0 to reset the function is documented in the NCM spec:
>>
>
> Agree, this is likely a firmware bug and it is also proved by all the
> other modems that accept the procedure without issues.
>
>> "
>>   7.2 Using Alternate Settings to Reset an NCM Function
>>
>>   To place the network aspects of a function in a known state, the host
>>   shall:
>>   • select alternate setting 0 of the NCM Data Interface (this is the
>>     setting with no endpoints). This can be done explicitly using
>>     SetInterface, or implicitly using SetConfiguration. See [USB30] for
>>     details.
>>   • select the NCM operational parameters by sending commands to the NCM
>>     Communication Interface, then
>>   • select the second alternate interface setting of the NCM Data
>>     Interface (this is the setting with a bulk IN endpoint and a bulk
>>     OUT endpoint).
>>
>>   Whenever alternate setting 0 is selected by the host, the function
>>   shall:
>>   • flush function buffers
>>   • reset the packet filter to its default state
>>   • clear all multicast address filters
>>   • clear all power filters set using
>>     SetEthernetPowerManagementPatternFilter
>>   • reset statistics counters to zero
>>   • restore its Ethernet address to its default state
>>   • reset its IN NTB size to the value given by field dwNtbInMaxSize
>>     from the NTB Parameter Struc- ture
>>   • reset the NTB format to NTB-16
>>   • reset the current Maximum Datagram Size to a function-specific
>>     default. If SetMaxDatagramSize is not supported, then the maximum
>>     datagram size shall be the same as the value in wMaxSegmentSize of
>>     the Ethernet Networking Functional Descriptor. If SetMaxDatagramSize
>>     is supported by the function, then the host must either query the
>>     function to determine the current effective maximum datagram size,
>>     or must explicitly set the maximum datagram size. If the host wishes
>>     to set the Maximum Datagram Size, it may do so prior to selecting
>>     the second alternate interface set- ting of the data
>>     interface. Doing so will ensure that the change takes effect prior
>>     to send or receiving data.
>>   • reset CRC mode so that the function will not append CRCs to
>>     datagrams sent on the IN pipe
>>   • reset NTB sequence numbers to zero
>>
>>   When the host selects the second alternate interface setting of the
>>   NCM Data Interface, the function shall perform the following actions
>>   in the following order.
>>   • If connected to the network, the function shall send a
>>     ConnectionSpeedChange notification to the host indicating the
>>     current connection speed.
>>   • Whether connected or not, the function shall then send a
>>     NetworkConnection notification to the host, with wValue indicating
>>     the current state of network connectivity
>> "
>>
>> The addition of the "RESET" request in MBIM did not change these
>> requirements AFAICS.  Supporting NCM style "altsetting 0 reset" is
>> required by default inheritance.  The description of dual NCM/MBIM
>> functions in the MBIM spec further emphasizes this:
>>
>> "
>>   Regardless of the previous sequence of SET_INTERFACE commands targeting
>>   the Communication Interface, the host is able to put the function into
>>   a defined state by the following sequence:
>>
>>   1. SET_INTERFACE(Data Interface, 0)
>>   2. SET_INTERFACE(Communication Interface, desired alternate setting)
>>   3. Sending the required class commands for the targeted alternate
>>     setting
>>   4. SET_INTERFACE(Data Interface, 1 if Communication Interface,0 and
>>     Data Interface,2 Communication Interface 1)
>> "
>>
>>
>> This, along with the lack of *any* sort of discussion of the
>> relation/interfaction between "MBIM RESET" and "data interface
>> altsetting 0" makes the MBIM RESET control request either ambigious or
>> redundant.  Or both...
>>
>> FWIW, I've always considered RESET a symptom of the sloppy MBIM spec
>> development.  It did not exactly work to their advantage that the first
>> (and only?) published errata simply was an excuse to add new features
>> (the "MBIM extended functional descriptor" and "MBIM loopback testmode")
>> without updating the revision number.  Causing even more confusion,
>> since we now have two different MBIMv1.0 specs.
>>
>> Well, whatever.  No need to get all frustrated about that. We have to
>> deal with whatever the firmware developers serve us.  And,
>> unfortunately, it is not surprising that unclear and ambigious specs
>> leads to incompatible firmware implementations.
>>
>> I wonder, what happens if you unload the MBIM driver and let the USB
>> core reset the data interface to altsetting 0?  Will the device then
>> fail when the driver is loaded again?  If not, then where is the
>> difference?  And can we possibly reorder the driver set_interface
>> requests to make them similar to the USB core reset
>>
>
> I will check this.
>

I verified that the behavior does not change and data connection still
does not work, so I'm going to submit the new patch.

Regards,
Daniele

> Thanks,
> Daniele
>
>> Until now, I was under the impression that this was the
>> only documented way to reset both NCM and MBIM functions (since the
>> RESET is MBIM specific),
>>
>>> My proposal is to add something like
>>> CDC_MBIM_FLAG_AVOID_ALTSETTING_TOGGLE quirk to avoid the toggle.
>>>
>>> To have a conservative approach, I would add only Telit LE922 to this
>>> quirk and keep also the usleep_range delay, since I do not have a
>>> clear view on all the VIDs/PIDs affected by this quirk. Then other
>>> modems, once tested, can be added to the quirk if the delay is not
>>> enough.
>>>
>>> If this approach, though ugly, has chances to be accepted I can write
>>> a new patch.
>>
>> Yes, that sounds like the best approach at the moment.  I have
>> unfortunately not as much time to experiment with this as I seem to
>> need.  And the EM7455 workaround (the delay) is a bit hard to verify,
>> since it is easily affected by small additional delays caused by the
>> debugging itself.  This can cause bogus test results.
>>
>>
>> Bjørn