diff mbox

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

Message ID 1358519147-10073-1-git-send-email-bjorn@mork.no
State Superseded, archived
Delegated to: David Miller
Headers show

Commit Message

Bjørn Mork Jan. 18, 2013, 2:25 p.m. UTC 
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@smithmicro.com>
Cc: Alexey Orishko <alexey.orishko@stericsson.com>
Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
 drivers/net/usb/cdc_ncm.c |   25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

Comments

Oliver Neukum Jan. 18, 2013, 7:11 p.m. UTC | #1 
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 netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjørn Mork Jan. 18, 2013, 9:17 p.m. UTC | #2 
Oliver Neukum <oliver@neukum.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 netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjørn Mork Jan. 19, 2013, 12:18 p.m. UTC | #3 
>+	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

--
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
Alexey Orishko Jan. 20, 2013, 12:21 a.m. UTC | #4 
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
--
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
Bjørn Mork Jan. 21, 2013, 8:31 a.m. UTC | #5 
Alexey Orishko <alexey.orishko@gmail.com> writes:
> 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."

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 netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjørn Mork Jan. 21, 2013, 2:08 p.m. UTC | #6 
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
--
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
diff mbox

Patch

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