Message ID | 1358519147-10073-1-git-send-email-bjorn@mork.no |
---|---|
State | Superseded, archived |
Delegated to: | David Miller |
Headers | show |
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
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
>+ 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
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
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
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 --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))))
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(+)