[PATCH net] net: cdc_ncm: workaround for missing CDC Union

[PATCH net] net: cdc_ncm: workaround for missing CDC Union

[Bjørn Mork]

Adding support for the MBIM mode in some Sierra Wireless devices.

Some Sierra Wireless firmwares support CDC MBIM but have no CDC
Union funtional descriptor. This violates the MBIM specification,
but we can easily work around the bug by looking at the Interface
Association Descriptor instead.  This is most likely what
Windows uses too, which explains how the firmware bug has gone
unnoticed until now.

This change will not affect any currently supported device
conforming to the NCM or MBIM specifications, as they must have
the CDC Union descriptor.

Cc: Greg Suarez <gsuarez-AKjrjAf1O7qe8kRwQpwjMg@public.gmane.org>
Cc: Alexey Orishko <alexey.orishko-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org>
Signed-off-by: Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org>
---
 drivers/net/usb/cdc_ncm.c |   25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 71b6e92..5c1210f 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -344,6 +344,21 @@ static const struct ethtool_ops cdc_ncm_ethtool_ops = {
 	.nway_reset = usbnet_nway_reset,
 };
 
+/* return first slave interface if an IAD matches the given master */
+static struct usb_interface *get_iad_slave(struct usb_device *udev,
+					   struct usb_interface *master) {
+	int i;
+	struct usb_interface_assoc_descriptor *iad;
+	u8 mnum = master->cur_altsetting->desc.bInterfaceNumber;
+
+	for (i = 0; i < USB_MAXIADS; i++) {
+		iad = udev->actconfig->intf_assoc[i];
+		if (iad->bFirstInterface == mnum && iad->bInterfaceCount == 2)
+			return usb_ifnum_to_if(udev, mnum + 1);
+	}
+	return NULL;
+}
+
 int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8 data_altsetting)
 {
 	struct cdc_ncm_ctx *ctx;
@@ -435,6 +450,16 @@ advance:
 		len -= temp;
 	}
 
+	/* some buggy devices have an IAD but no CDC Union */
+	if (!ctx->union_desc) {
+		dev_dbg(&intf->dev, "missing CDC Union descriptor\n");
+		ctx->data = get_iad_slave(dev->udev, intf);
+		if (ctx->data) {
+			ctx->control = intf;
+			dev_dbg(&intf->dev, "got slave from IAD\n");
+		}
+	}
+
 	/* check if we got everything */
 	if ((ctx->control == NULL) || (ctx->data == NULL) ||
 	    ((!ctx->mbim_desc) && ((ctx->ether_desc == NULL) || (ctx->control != intf))))
-- 
1.7.10.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 net] net: cdc_ncm: workaround for missing CDC Union

[Oliver Neukum]

On Friday 18 January 2013 15:25:47 Bjørn Mork wrote:
> Adding support for the MBIM mode in some Sierra Wireless devices.
> 
> Some Sierra Wireless firmwares support CDC MBIM but have no CDC
> Union funtional descriptor. This violates the MBIM specification,
> but we can easily work around the bug by looking at the Interface
> Association Descriptor instead.  This is most likely what
> Windows uses too, which explains how the firmware bug has gone
> unnoticed until now.

Should we do this for everything CDC?

	Regards
		Oliver

--
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] net: cdc_ncm: workaround for missing CDC Union

[Bjørn Mork]

Oliver Neukum <oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org> writes:
> On Friday 18 January 2013 15:25:47 Bjørn Mork wrote:
>> Adding support for the MBIM mode in some Sierra Wireless devices.
>> 
>> Some Sierra Wireless firmwares support CDC MBIM but have no CDC
>> Union funtional descriptor. This violates the MBIM specification,
>> but we can easily work around the bug by looking at the Interface
>> Association Descriptor instead.  This is most likely what
>> Windows uses too, which explains how the firmware bug has gone
>> unnoticed until now.
>
> Should we do this for everything CDC?

I don't know.  There are workarounds for missing CDC Union descriptors
on RNDIS devices in cdc_ether.c, but I have no idea if those devices in
provide an IAD instead.  I don't think this bug is a very common.

MBIM devices are likely to have IADs because Microsoft let you skip some
of the more hairy vendor specific control message parts if you provide
one:
http://msdn.microsoft.com/en-us/library/windows/hardware/mbim-based-mobile-broadband-requirements-for-windows.aspx

But they should of course provide the CDC Union functional descriptor as
well, even if that is "just" a USB-IF requirement and not a Windows
requirement ;-)


Bjørn
--
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] net: cdc_ncm: workaround for missing CDC Union

[Alexey Orishko]

On Fri, Jan 18, 2013 at 10:17 PM, Bjørn Mork <bjorn@mork.no> wrote:

>>> Some Sierra Wireless firmwares support CDC MBIM but have no CDC
>>> Union funtional descriptor. This violates the MBIM specification,

I don't believe Sierra Wireless violates MBIM specification.
See in the specification: "there are two ways to group interfaces: the
WHCM Union functional descriptor and IAD."

Regards,
Alexey

Re: [PATCH net] net: cdc_ncm: workaround for missing CDC Union

[Bjørn Mork]

Alexey Orishko <alexey.orishko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
> On Fri, Jan 18, 2013 at 10:17 PM, Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org> wrote:
>
>>>> Some Sierra Wireless firmwares support CDC MBIM but have no CDC
>>>> Union funtional descriptor. This violates the MBIM specification,
>
> I don't believe Sierra Wireless violates MBIM specification.
> See in the specification: "there are two ways to group interfaces: the
> WHCM Union functional descriptor and IAD."

I disagree.  This is not about the WHCM Union descriptor, it's about the
CDC Union descriptor.  WHCM is of course not mandatory.

Quoting that whole section from the MBIM specification:

<quote>
  6.1 OVERVIEW
  
  A USB MBIM function is implemented as a USB CDC function with two
  interfaces.  Functions shall provide a CDC Union functional descriptor
  to group these two interfaces.  See [USBWMC11].
  
  A Communication Class interface, with class 02h and subclass 0Eh, and
  a Data Class interface combine to form a single functional unit
  representing the USB MBIM device.  The Communication Class interface
  includes a single endpoint for event notification; it also uses the
  device’s default pipe for control messages.  The Data Class interface
  includes two bulk endpoints for data traffic.
  
  There are two ways to group interfaces: the WHCM Union Functional
  Descriptor (see [USBWMC11]) and the IAD.  Devices may also provide an
  IAD.  If an IAD is provided, the information in the IAD for MBIM
  functions shall be consistent with the information in the CDC Union
  descriptor and Communication Class interface descriptor.
</quote>


The "Functions shall provide a CDC Union functional descriptor to group
these two interfaces." is pretty clear IMHO.  You also have table 6‐2
listing the HEADER, UNION and MBIM functional descriptors as "Required".

There is absolutely not way to make this anything but a firmware bug.

But I am all for working around it, of course.  There also seems to be a
couple more oddities with the MBIM mode of this device compared to the
other I've seen, but those are more likely revealing driver bugs.  I
intend to fix them as well.



Bjørn
--
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] net: cdc_ncm: workaround for missing CDC Union

[Bjørn Mork]

Hello Alexey,

I have another issue with the Sierra firmware which I hope you can help
me with: The MC7710 device requires at ZLP even if we send
dwNtbOutMaxSize sized NTBs. This is a problem because the current code
explicitly prevents this.

The following code in the v3.8 cdc-ncm was written to keep existing
behaviour from the pre-v3.8 driver:

	if (((skb_out->len % le16_to_cpu(ctx->out_ep->desc.wMaxPacketSize)) == 0) &&
	    (skb_out->len < le32_to_cpu(ctx->ncm_parm.dwNtbOutMaxSize)) && skb_tailroom(skb_out))
		*skb_put(skb_out, 1) = 0;	/* force short packet */



The previous implementaion looked like this:

6c60408e (Alexey Orishko     2011-05-06 03:01:30 +0000  832)    if (((last_offset < ctx->tx_max) && ((last_offset %
6c60408e (Alexey Orishko     2011-05-06 03:01:30 +0000  833)                    le16_to_cpu(ctx->out_ep->desc.wMaxPacketSize)) == 0)) ||
6c60408e (Alexey Orishko     2011-05-06 03:01:30 +0000  834)        (((last_offset == ctx->tx_max) && ((ctx->tx_max %
6c60408e (Alexey Orishko     2011-05-06 03:01:30 +0000  835)            le16_to_cpu(ctx->out_ep->desc.wMaxPacketSize)) == 0)) &&
6c60408e (Alexey Orishko     2011-05-06 03:01:30 +0000  836)            (ctx->tx_max < le32_to_cpu(ctx->ncm_parm.dwNtbOutMaxSize)))) {
900d495a (Alexey Orishko     2010-11-29 23:23:28 +0000  837)            /* force short packet */
900d495a (Alexey Orishko     2010-11-29 23:23:28 +0000  838)            *(((u8 *)skb_out->data) + last_offset) = 0;
900d495a (Alexey Orishko     2010-11-29 23:23:28 +0000  839)            last_offset++;
900d495a (Alexey Orishko     2010-11-29 23:23:28 +0000  840)    }


The effect is the same: We add a 0 byte if the NTB length is a multiplum
of wMaxPacketSize *except* if the length is equal to dwNtbOutMaxSize.
This exception will happen very often because of the way we pad NTBs.

Now, I have tried to find what the above code was based on, and my guess
is that it is this note in table 3-1 in the CDC NCM spec:

   If wBlockLength = 0x0000, the block is terminated by a
   short packet. In this case, the USB transfer must still
   be shorter than dwNtbInMaxSize or dwNtbOutMax-
   Size. If exactly dwNtbInMaxSize or dwNtbOutMaxSize
   bytes are sent, and the size is a multiple of wMax-
   PacketSize for the given pipe, then no ZLP shall be
   sent.

Is that correct?  I cannot find any special ZLP handling mentioned
anywhere else in the standard.

If so, then I believe it is a misinterpretation.  The above text deals
only with the exceptional case of wBlockLength = 0x0000, which we do not
do.  As long as wBlockLength > 0 then I believe the device is in its
full right to expect a ZLP if wBlockLength % wMaxPacketSize is 0.

Would you feel comfortable dropping the additional condition and going
with

	if (((skb_out->len % le16_to_cpu(ctx->out_ep->desc.wMaxPacketSize)) == 0) &&
	    skb_tailroom(skb_out))
		*skb_put(skb_out, 1) = 0;	/* force short packet */


? I have verified that this is sufficient to make the Sierra device
work.  I will of course test it with the other NCM and MBIM devices I've
got, but that is a very limited set...

The other option I see is making a device specific quirk for this.  But
I suspect that Sierra is using the current Qualcomm MBIM implemetation
here, and if so then we are likely to see a large number of similar
devices in the near future.  I'd really like to avoid having device
specific quirks for all of them if at all possible.



Bjørn

Re: [PATCH net] net: cdc_ncm: workaround for missing CDC Union

[Bjørn Mork]

Bjørn Mork <bjorn@mork.no> writes:

> I have another issue with the Sierra firmware which I hope you can help
> me with: The MC7710 device requires at ZLP even if we send
> dwNtbOutMaxSize sized NTBs. This is a problem because the current code
> explicitly prevents this.

If anyone found this more than normally confused, then that is
correct...

What the NCM code does is that it sends a *short* packet whenever the
usbnet would normally have done so, except if the NTP length >=
dwNtbOutMaxSize.

The problem is that we do not set 

	urb->transfer_flags |= URB_ZERO_PACKET;

either in this special case, resulting in neither a short nor a zero
length packet being sent.  I believe this is what the Sierra firmware
chokes at. This seems to fix the issue, and is IMHO correct:

diff --git a/drivers/net/usb/cdc_mbim.c b/drivers/net/usb/cdc_mbim.c
index 42f51c7..3a5673a 100644
--- a/drivers/net/usb/cdc_mbim.c
+++ b/drivers/net/usb/cdc_mbim.c
@@ -366,7 +366,7 @@ err:
 
 static const struct driver_info cdc_mbim_info = {
        .description = "CDC MBIM",
-       .flags = FLAG_NO_SETINT | FLAG_MULTI_PACKET | FLAG_WWAN,
+       .flags = FLAG_NO_SETINT | FLAG_MULTI_PACKET | FLAG_WWAN | FLAG_SEND_ZLP,
        .bind = cdc_mbim_bind,
        .unbind = cdc_mbim_unbind,
        .manage_power = cdc_mbim_manage_power,



But I wonder if this isn't really a generic problem in usbnet.  The
FLAG_MULTI_PACKET test here seems completely bogus:

	if (length % dev->maxpacket == 0) {
		if (!(info->flags & FLAG_SEND_ZLP)) {
			if (!(info->flags & FLAG_MULTI_PACKET)) {
				urb->transfer_buffer_length++;
				if (skb_tailroom(skb)) {
					skb->data[skb->len] = 0;
					__skb_put(skb, 1);
				}
			}
		} else
			urb->transfer_flags |= URB_ZERO_PACKET;
	}

Either the FLAG_MULTI_PACKET minidriver will have already padded the
buffer so that we do not hit (length % dev->maxpacket == 0), or we
should choose one of the alternatives: ZLP or padding.

Minidrivers not wanting the short packets (like cdc_ncm) must set
FLAG_SEND_ZLP.


Bjørn

Re: [PATCH net] net: cdc_ncm: workaround for missing CDC Union

[Oliver Neukum]

On Monday 21 January 2013 15:47:13 Bjørn Mork wrote:
> But I wonder if this isn't really a generic problem in usbnet.  The
> FLAG_MULTI_PACKET test here seems completely bogus:
> 
>         if (length % dev->maxpacket == 0) {
>                 if (!(info->flags & FLAG_SEND_ZLP)) {
>                         if (!(info->flags & FLAG_MULTI_PACKET)) {
>                                 urb->transfer_buffer_length++;
>                                 if (skb_tailroom(skb)) {
>                                         skb->data[skb->len] = 0;
>                                         __skb_put(skb, 1);
>                                 }
>                         }
>                 } else
>                         urb->transfer_flags |= URB_ZERO_PACKET;
>         }
> 
> Either the FLAG_MULTI_PACKET minidriver will have already padded the
> buffer so that we do not hit (length % dev->maxpacket == 0), or we
> should choose one of the alternatives: ZLP or padding.

But we cannot simply call __skb_put for a complicated data frame.
Besides you may want the current behavior.

	Regards
		Oliver

--
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] net: cdc_ncm: workaround for missing CDC Union

[Bjørn Mork]

Oliver Neukum <oneukum@suse.de> writes:
> On Monday 21 January 2013 15:47:13 Bjørn Mork wrote:
>> But I wonder if this isn't really a generic problem in usbnet.  The
>> FLAG_MULTI_PACKET test here seems completely bogus:
>> 
>>         if (length % dev->maxpacket == 0) {
>>                 if (!(info->flags & FLAG_SEND_ZLP)) {
>>                         if (!(info->flags & FLAG_MULTI_PACKET)) {
>>                                 urb->transfer_buffer_length++;
>>                                 if (skb_tailroom(skb)) {
>>                                         skb->data[skb->len] = 0;
>>                                         __skb_put(skb, 1);
>>                                 }
>>                         }
>>                 } else
>>                         urb->transfer_flags |= URB_ZERO_PACKET;
>>         }
>> 
>> Either the FLAG_MULTI_PACKET minidriver will have already padded the
>> buffer so that we do not hit (length % dev->maxpacket == 0), or we
>> should choose one of the alternatives: ZLP or padding.
>
> But we cannot simply call __skb_put for a complicated data frame.

Agreed.  But I believe the condition should be 

 if (!(info->flags & FLAG_SEND_ZLP) && !(info->flags & FLAG_MULTI_PACKET)) {
     ..
 } else {
     urb->transfer_flags |= URB_ZERO_PACKET;
 }

to ensure that we send the ZLP in this case.

> Besides you may want the current behavior.

Why? Does it ever make sense to prevent both the short packet and the
ZLP?


Bjørn

Re: [PATCH net] net: cdc_ncm: workaround for missing CDC Union

[Oliver Neukum]

On Monday 21 January 2013 16:28:48 Bjørn Mork wrote:

> Agreed.  But I believe the condition should be 
> 
>  if (!(info->flags & FLAG_SEND_ZLP) && !(info->flags & FLAG_MULTI_PACKET)) {
>      ..
>  } else {
>      urb->transfer_flags |= URB_ZERO_PACKET;
>  }
> 
> to ensure that we send the ZLP in this case.

Why? If a driver wants ZLP, it can set FLAG_SEND_ZLP. Your proposed change
would take away an option from drivers without any gain.

> > Besides you may want the current behavior.
> 
> Why? Does it ever make sense to prevent both the short packet and the
> ZLP?

Why not? It is possible and a driver may want it, so why forbid it?
Especially, as in theory it takes least bandwidth as a solution.

	Regards
		Oliver


--
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] net: cdc_ncm: workaround for missing CDC Union

[Bjørn Mork]

Oliver Neukum <oneukum@suse.de> writes:
> On Monday 21 January 2013 16:28:48 Bjørn Mork wrote:
>
>> Agreed.  But I believe the condition should be 
>> 
>>  if (!(info->flags & FLAG_SEND_ZLP) && !(info->flags & FLAG_MULTI_PACKET)) {
>>      ..
>>  } else {
>>      urb->transfer_flags |= URB_ZERO_PACKET;
>>  }
>> 
>> to ensure that we send the ZLP in this case.
>
> Why? If a driver wants ZLP, it can set FLAG_SEND_ZLP. Your proposed change
> would take away an option from drivers without any gain.

OK.


Bjørn

RE: [PATCH net] net: cdc_ncm: workaround for missing CDC Union

[Alexey ORISHKO]

> -----Original Message-----
> From: Oliver Neukum [mailto:oneukum@suse.de]
> On Monday 21 January 2013 15:47:13 Bjørn Mork wrote:
> > But I wonder if this isn't really a generic problem in usbnet.  The
> > FLAG_MULTI_PACKET test here seems completely bogus:
> >
> >         if (length % dev->maxpacket == 0) {
> >                 if (!(info->flags & FLAG_SEND_ZLP)) {
> >                         if (!(info->flags & FLAG_MULTI_PACKET)) {
> >                                 urb->transfer_buffer_length++;
> >                                 if (skb_tailroom(skb)) {
> >                                         skb->data[skb->len] = 0;
> >                                         __skb_put(skb, 1);
> >                                 }
> >                         }
> >                 } else
> >                         urb->transfer_flags |= URB_ZERO_PACKET;
> >         }
> >
> > Either the FLAG_MULTI_PACKET minidriver will have already padded the
> > buffer so that we do not hit (length % dev->maxpacket == 0), or we
> > should choose one of the alternatives: ZLP or padding.
> 
> But we cannot simply call __skb_put for a complicated data frame.
> Besides you may want the current behavior.
> 

Specification says: 
NCM/MBIM shall not send ZLP if buffer size is dwNtbOutMaxSize.

The problem is: 
dwNtbOutMaxSize value is negotiated between host and device NCM/MBIM
entities and usbnet has no knowledge about it.

Adding one byte to make buffer looking like a short packet was most
simple approach instead of inventing a way to communicate 
dwNtbOutMaxSize to usbnet.

You could drop short packet approach if dwNtbOutMaxSize is provided
to usbnet and decision is made accordingly to NCM/MBIM spec (with
exception to faulty devices).

Regards,
Alexey

Re: [PATCH net] net: cdc_ncm: workaround for missing CDC Union

[Oliver Neukum]

On Monday 21 January 2013 18:59:05 Alexey ORISHKO wrote:
> Specification says: 
> NCM/MBIM shall not send ZLP if buffer size is dwNtbOutMaxSize.
> 
> The problem is: 
> dwNtbOutMaxSize value is negotiated between host and device NCM/MBIM
> entities and usbnet has no knowledge about it.
> 
> Adding one byte to make buffer looking like a short packet was most
> simple approach instead of inventing a way to communicate 
> dwNtbOutMaxSize to usbnet.
> 
> You could drop short packet approach if dwNtbOutMaxSize is provided
> to usbnet and decision is made accordingly to NCM/MBIM spec (with
> exception to faulty devices).

We could do that. Feel free to send a patch. ;-)

	Regards
		Oliver

Re: [PATCH net] net: cdc_ncm: workaround for missing CDC Union

[Oliver Neukum]

On Monday 21 January 2013 18:59:05 Alexey ORISHKO wrote:

> > > Either the FLAG_MULTI_PACKET minidriver will have already padded the
> > > buffer so that we do not hit (length % dev->maxpacket == 0), or we
> > > should choose one of the alternatives: ZLP or padding.
> > 
> > But we cannot simply call __skb_put for a complicated data frame.
> > Besides you may want the current behavior.
> > 
> 
> Specification says: 
> NCM/MBIM shall not send ZLP if buffer size is dwNtbOutMaxSize.

Hi,

one thing on a generic level, which I hope you don't take wrong, but I need
to make it clear. Usbnet is for every driver. What a spec says
or what the reality of implementations of that spec looks like, determine
what usbnet needs to support.

But what is not needed to implement that spec doesn't matter in deciding
what usbnet needs not support. Drivers may or may not need ZLPs.

	Regards
		Oliver
 

Re: [PATCH net] net: cdc_ncm: workaround for missing CDC Union

[Bjørn Mork]

>+	for (i = 0; i < USB_MAXIADS; i++) {
>+		iad = udev->actconfig->intf_assoc[i];
>+		if (iad->bFirstInterface == mnum && iad->bInterfaceCount == 2)
>+			return usb_ifnum_to_if(udev, mnum + 1);

Ouch.  This looks buggy. Please do not apply. I will send a fixed version later. 

Bjørn