[PATCH net-next 0/2] cdc_mbim workarounds for firmware bugs

[PATCH net-next 0/2] cdc_mbim workarounds for firmware bugs

[Bjørn Mork]

The first patch is an attempt to work around a bug which prevents
some devices from working with IPv6. I guess handling ND in a driver
can be considered controversial, but it was the best alternative I
came up with.

The second is the "final solution" to the well known ZLP bug, which
as we feared is present in a number of devices using different
vendor IDs. Enabling this is definitely controversial, but I do not
see any other possible solution.  The blacklist has become
unmanagable, and I am pretty sure it is very incomplete due to
under-reporting.  Devices known to work without ZLPs can be added
to the whitelist.

Bjørn Mork (2):
  net: cdc_mbim: handle IPv6 Neigbor Solicitations
  net: cdc_mbim: change the default to send ZLPs

 drivers/net/usb/cdc_mbim.c |   86 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 75 insertions(+), 11 deletions(-)

-- 
1.7.10.4

[PATCH net-next 1/2] net: cdc_mbim: handle IPv6 Neigbor Solicitations

[Bjørn Mork]

MBIM is a point-to-point protocol transporting raw IP packets
with no L2 headers. Only IPv4 and IPv6 are supported. ARP in
particular is not, which is quite logical given the lack of
L2 headers.

The driver still emulates an ethernet interface, dropping all
unsupported protocols, and avoiding neigbour resolving by
setting the IFF_NOARP flag.

The MBIM specification does not explicitly forbid IPv6 Neighbor
Discovery, and it seems the other OS support will respond to
Neighbor Solicitations on MBIM links. There are therefore
buggy devices out there, which despite the pointlessness, still
require Neighbor Discovery for IPv6 over MBIM.

This is incompatible with the IFF_NOARP flag which disables
both ARP and ND.  We cannot support ARP in any case, so we
have to keep that flag. This patch implements a workaround
for the buggy devices, letting the driver respond directly
to Neighbor Solicitations from the device.

This is not optimal, but will have minimal effect on any sane
device.

Cc: Greg Suarez <gsuarez@smithmicro.com>
Reported-and-tested-by: Thomas Schäfer <tschaefer@t-online.de>
Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
 drivers/net/usb/cdc_mbim.c |   58 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/drivers/net/usb/cdc_mbim.c b/drivers/net/usb/cdc_mbim.c
index 25ba7ec..6533a9f 100644
--- a/drivers/net/usb/cdc_mbim.c
+++ b/drivers/net/usb/cdc_mbim.c
@@ -21,6 +21,8 @@
 #include <linux/usb/usbnet.h>
 #include <linux/usb/cdc-wdm.h>
 #include <linux/usb/cdc_ncm.h>
+#include <net/ipv6.h>
+#include <net/addrconf.h>
 
 /* driver specific data - must match cdc_ncm usage */
 struct cdc_mbim_state {
@@ -184,6 +186,60 @@ error:
 	return NULL;
 }
 
+/* Some devices are known to send Neigbor Solicitation messages and
+ * require Neigbor Advertisement replies.  The IPv6 core will not
+ * respond since IFF_NOARP is set, so we must handle them ourselves.
+ */
+static void do_neigh_solicit(struct usbnet *dev, u8 *buf, u16 tci)
+{
+	struct ipv6hdr *iph = (void *)buf;
+	struct nd_msg *msg = (void *)(iph + 1);
+	struct net_device *netdev;
+	struct inet6_dev *in6_dev;
+	bool is_router;
+
+	/* we'll only respond to requests from unicast addresses to
+	 * our solicited node addresses.
+	 */
+	if (!ipv6_addr_is_solict_mult(&iph->daddr) ||
+	    !(ipv6_addr_type(&iph->saddr) & IPV6_ADDR_UNICAST))
+		return;
+
+	/* need to send the NA on the VLAN dev, if any */
+	if (tci)
+		netdev = __vlan_find_dev_deep(dev->net, htons(ETH_P_8021Q),
+					      tci);
+	else
+		netdev = dev->net;
+	if (!netdev)
+		return;
+
+	in6_dev = in6_dev_get(netdev);
+	if (!in6_dev)
+		return;
+	is_router = !!in6_dev->cnf.forwarding;
+	in6_dev_put(in6_dev);
+
+	/* ipv6_stub != NULL if in6_dev_get returned an inet6_dev */
+	ipv6_stub->ndisc_send_na(netdev, NULL, &iph->saddr, &msg->target,
+				 is_router /* router */,
+				 true /* solicited */,
+				 false /* override */,
+				 true /* inc_opt */);
+}
+
+static bool is_neigh_solicit(u8 *buf, size_t len)
+{
+	struct ipv6hdr *iph = (void *)buf;
+	struct nd_msg *msg = (void *)(iph + 1);
+
+	return (len >= sizeof(struct ipv6hdr) + sizeof(struct nd_msg) &&
+		iph->nexthdr == IPPROTO_ICMPV6 &&
+		msg->icmph.icmp6_code == 0 &&
+		msg->icmph.icmp6_type == NDISC_NEIGHBOUR_SOLICITATION);
+}
+
+
 static struct sk_buff *cdc_mbim_process_dgram(struct usbnet *dev, u8 *buf, size_t len, u16 tci)
 {
 	__be16 proto = htons(ETH_P_802_3);
@@ -198,6 +254,8 @@ static struct sk_buff *cdc_mbim_process_dgram(struct usbnet *dev, u8 *buf, size_
 			proto = htons(ETH_P_IP);
 			break;
 		case 0x60:
+			if (is_neigh_solicit(buf, len))
+				do_neigh_solicit(dev, buf, tci);
 			proto = htons(ETH_P_IPV6);
 			break;
 		default:
-- 
1.7.10.4

[PATCH net-next 2/2] net: cdc_mbim: change the default to send ZLPs

[Bjørn Mork]

A number of devices in the wild have turned out to require ZLPs.
Even if this is a spec violation, our priority is to make any
device work as good as possible. Devices needing ZLPs will fail
to receive any full sized frame we send. On the other hand,
devices which do not need the ZLP will still work if we send
them.

This gives us no other option than sending ZLPs by default.

This will prevent devices conforming to the spec from making the
optimizations which are possible without ZLPs.  Adding known
such devices to a whitelist, to avoid the possible negative
impact of the new spec violating default.

Cc: Greg Suarez <gsuarez@smithmicro.com>
Cc: Alexey Orishko <alexey.orishko@gmail.com>
Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
 drivers/net/usb/cdc_mbim.c |   28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/net/usb/cdc_mbim.c b/drivers/net/usb/cdc_mbim.c
index 6533a9f..253472a 100644
--- a/drivers/net/usb/cdc_mbim.c
+++ b/drivers/net/usb/cdc_mbim.c
@@ -429,9 +429,18 @@ static const struct driver_info cdc_mbim_info = {
 };
 
 /* MBIM and NCM devices should not need a ZLP after NTBs with
- * dwNtbOutMaxSize length. This driver_info is for the exceptional
- * devices requiring it anyway, allowing them to be supported without
- * forcing the performance penalty on all the sane devices.
+ * dwNtbOutMaxSize length. Nevertheless, a number of devices from
+ * different vendor IDs will fail unless we send ZLPs, forcing us
+ * to make this the default.
+ *
+ * This default may cause a performance penalty for spec conforming
+ * devices wanting to take advantage of optimizations possible without
+ * ZLPs.  A whitelist is added in an attempt to avoid this for devices
+ * known to conform to the MBIM specification.
+ *
+ * All known devices supporting NCM compatibility mode are also
+ * conforming to the NCM and MBIM specifications. For this reason, the
+ * NCM subclass entry is also in the ZLP whitelist.
  */
 static const struct driver_info cdc_mbim_info_zlp = {
 	.description = "CDC MBIM",
@@ -454,16 +463,13 @@ static const struct usb_device_id mbim_devs[] = {
 	{ USB_INTERFACE_INFO(USB_CLASS_COMM, USB_CDC_SUBCLASS_NCM, USB_CDC_PROTO_NONE),
 	  .driver_info = (unsigned long)&cdc_mbim_info,
 	},
-	/* Sierra Wireless MC7710 need ZLPs */
-	{ USB_DEVICE_AND_INTERFACE_INFO(0x1199, 0x68a2, USB_CLASS_COMM, USB_CDC_SUBCLASS_MBIM, USB_CDC_PROTO_NONE),
-	  .driver_info = (unsigned long)&cdc_mbim_info_zlp,
-	},
-	/* HP hs2434 Mobile Broadband Module needs ZLPs */
-	{ USB_DEVICE_AND_INTERFACE_INFO(0x3f0, 0x4b1d, USB_CLASS_COMM, USB_CDC_SUBCLASS_MBIM, USB_CDC_PROTO_NONE),
-	  .driver_info = (unsigned long)&cdc_mbim_info_zlp,
+	/* ZLP conformance whitelist: All Ericsson MBIM devices */
+	{ USB_VENDOR_AND_INTERFACE_INFO(0x0bdb, USB_CLASS_COMM, USB_CDC_SUBCLASS_MBIM, USB_CDC_PROTO_NONE),
+	  .driver_info = (unsigned long)&cdc_mbim_info,
 	},
+	/* default entry */
 	{ USB_INTERFACE_INFO(USB_CLASS_COMM, USB_CDC_SUBCLASS_MBIM, USB_CDC_PROTO_NONE),
-	  .driver_info = (unsigned long)&cdc_mbim_info,
+	  .driver_info = (unsigned long)&cdc_mbim_info_zlp,
 	},
 	{
 	},
-- 
1.7.10.4

Re: [PATCH net-next 0/2] cdc_mbim workarounds for firmware bugs

[David Miller]

From: Bjørn Mork <bjorn@mork.no>
Date: Thu, 31 Oct 2013 15:56:09 +0100

> The first patch is an attempt to work around a bug which prevents
> some devices from working with IPv6. I guess handling ND in a driver
> can be considered controversial, but it was the best alternative I
> came up with.
> 
> The second is the "final solution" to the well known ZLP bug, which
> as we feared is present in a number of devices using different
> vendor IDs. Enabling this is definitely controversial, but I do not
> see any other possible solution.  The blacklist has become
> unmanagable, and I am pretty sure it is very incomplete due to
> under-reporting.  Devices known to work without ZLPs can be added
> to the whitelist.

This seems fine, both applied, thanks.