[PATCH net-next 00/14] Adding a USB CDC MBIM driver

[PATCH net-next 00/14] Adding a USB CDC MBIM driver

[Bjørn Mork]

The USB Communications Device Class "Mobile Broadband Interface Model"
(MBIM) is the USB-IFs alternative to the current chipset/vendor
specific solutions to "Mobile Broadband" device management. The
specification, including the management protocol description, can be
downloaded from http://www.usb.org/developers/devclass_docs#approved

This driver implementing most MBIM features with the exception of
32bit NTB and NDP headers.

An important design principle has been reusing as much as possible of
existing kernel code, in particular the cdc_ncm and cdc_wdm drivers.
The CDC MBIM protocol is based on CDC NCM, and much of the setup and
framing logic is very similar.

One important addition in MBIM compared to NCM is the new control
protocol embedded in CDC commands.  This protocol is comprehensive and
support a number of policy decisions necessary for modern mobile
broadband devices often having multiple radio interfaces.  Based on
early comments and the experiences with the qmi_wwan driver, knowledge
of the control protocol has been kept completely out of the driver.
This is userspace material.  Like with qmi_wwan, a control protocol
interface is exported to userspace using the cdc_wdm subdriver API,
associating a /dev/cdc-wdmX character device with the network device
for the management application.

Patch 1 and 2 are independent of the rest and only required to make
test devices with very large buffers work.

Patch 3 adds new MBIM definitions to the cdc.h header file

Patches 4 to 9 refactor the cdc_ncm driver to enable reusing common
parts for MBIM.

Patches 10 and 11 add the new cdc_mbim driver

Patch 12 prevents cdc_ncm from binding to backward compatible MBIM
devices

Patches 13 and 14 extend the MBIM driver to support multiplexed
sessions

The changes to the cdc_ncm driver has been tested and verified to work
with an Ericsson F5521gw device.  The new cdc_mbim driver has been
tested with a Huawei E367u-2 device with MBIM firmware, and other
currently undisclosed devices.

Enjoy!


Bjørn Mork (10):
  net: usbnet: make sure the queue lenght is at least 1
  net: cdc_ncm: use device rx_max value if update failed
  net: cdc_ncm: process chained NDPs
  net: cdc_ncm: splitting rx_fixup for code reuse
  net: cdc_ncm: refactoring for tx multiplexing
  net: cdc_ncm: export shared symbols and definitions
  net: cdc_mbim: build the MBIM driver
  net: cdc_ncm: do not bind to NCM compatible MBIM devices
  net: cdc_ncm: map MBIM IPS SessionID to VLAN ID
  net: cdc_mbim: Device Service Stream support

Greg Suarez (4):
  USB: cdc: add MBIM constants and structures
  net: cdc_ncm: adding MBIM support to ncm_setup
  net: cdc_ncm: refactor bind preparing for MBIM support
  net: cdc_mbim: adding MBIM driver

 drivers/net/usb/Kconfig     |   18 ++
 drivers/net/usb/Makefile    |    1 +
 drivers/net/usb/cdc_mbim.c  |  412 ++++++++++++++++++++++++++++++++
 drivers/net/usb/cdc_ncm.c   |  546 +++++++++++++++++++++----------------------
 drivers/net/usb/usbnet.c    |    4 +-
 include/linux/usb/cdc.h     |   23 ++
 include/linux/usb/cdc_ncm.h |  134 +++++++++++
 7 files changed, 856 insertions(+), 282 deletions(-)
 create mode 100644 drivers/net/usb/cdc_mbim.c
 create mode 100644 include/linux/usb/cdc_ncm.h

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

[PATCH net-next 01/14] net: usbnet: make sure the queue lenght is at least 1

[Bjørn Mork]

Some usbnet based devices may want to use a rx_urb_size
greater than RX_MAX_QUEUE_MEMORY.  Prevent this from
creating a zero length rx queue.

Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
 drivers/net/usb/usbnet.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index f9819d1..34b5205 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -65,9 +65,9 @@
  */
 #define RX_MAX_QUEUE_MEMORY (60 * 1518)
 #define	RX_QLEN(dev) (((dev)->udev->speed == USB_SPEED_HIGH) ? \
-			(RX_MAX_QUEUE_MEMORY/(dev)->rx_urb_size) : 4)
+			(1 + RX_MAX_QUEUE_MEMORY/(dev)->rx_urb_size) : 4)
 #define	TX_QLEN(dev) (((dev)->udev->speed == USB_SPEED_HIGH) ? \
-			(RX_MAX_QUEUE_MEMORY/(dev)->hard_mtu) : 4)
+			(1 + RX_MAX_QUEUE_MEMORY/(dev)->hard_mtu) : 4)
 
 // reawaken network queue this soon after stopping; else watchdog barks
 #define TX_TIMEOUT_JIFFIES	(5*HZ)
-- 
1.7.10.4

[PATCH net-next 02/14] net: cdc_ncm: use device rx_max value if update failed

[Bjørn Mork]

If the device refuses our updated value, then we must be prepared
to receive URBs as big as the device wants to send.  Set rx_max
to the device provided value on error.

Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
 drivers/net/usb/cdc_ncm.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 4cd582a..6a65662 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -251,8 +251,11 @@ static u8 cdc_ncm_setup(struct cdc_ncm_ctx *ctx)
 			kfree(dwNtbInMaxSize);
 		}
 size_err:
-		if (err < 0)
-			pr_debug("Setting NTB Input Size failed\n");
+		if (err < 0) {
+			/* failed, so attempt to use the device suggested size */
+			ctx->rx_max = le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize);
+			pr_debug("Setting NTB Input Size failed, reverting to %u\n", ctx->rx_max);
+		}
 	}
 
 	/* verify maximum size of transmitted NTB in bytes */
-- 
1.7.10.4

[PATCH net-next 03/14] USB: cdc: add MBIM constants and structures

[Bjørn Mork]

From: Greg Suarez <gsuarez-AKjrjAf1O7qe8kRwQpwjMg@public.gmane.org>

Based on revision 1.0 of "Universal Serial Bus Communications
Class Subclass Specification for Mobile Broadband Interface
Model" available from www.usb.org

Signed-off-by: Greg Suarez <gsuarez-AKjrjAf1O7qe8kRwQpwjMg@public.gmane.org>
[bmork: added DSS defines]
Signed-off-by: Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org>
---
 include/linux/usb/cdc.h |   23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/include/linux/usb/cdc.h b/include/linux/usb/cdc.h
index 81a9279..f35aa0a 100644
--- a/include/linux/usb/cdc.h
+++ b/include/linux/usb/cdc.h
@@ -19,6 +19,7 @@
 #define USB_CDC_SUBCLASS_OBEX			0x0b
 #define USB_CDC_SUBCLASS_EEM			0x0c
 #define USB_CDC_SUBCLASS_NCM			0x0d
+#define USB_CDC_SUBCLASS_MBIM			0x0e
 
 #define USB_CDC_PROTO_NONE			0
 
@@ -33,6 +34,7 @@
 #define USB_CDC_PROTO_EEM			7
 
 #define USB_CDC_NCM_PROTO_NTB			1
+#define USB_CDC_MBIM_PROTO_NTB			2
 
 /*-------------------------------------------------------------------------*/
 
@@ -53,6 +55,7 @@
 #define USB_CDC_DMM_TYPE		0x14
 #define USB_CDC_OBEX_TYPE		0x15
 #define USB_CDC_NCM_TYPE		0x1a
+#define USB_CDC_MBIM_TYPE		0x1b
 
 /* "Header Functional Descriptor" from CDC spec  5.2.3.1 */
 struct usb_cdc_header_desc {
@@ -187,6 +190,21 @@ struct usb_cdc_ncm_desc {
 	__le16	bcdNcmVersion;
 	__u8	bmNetworkCapabilities;
 } __attribute__ ((packed));
+
+/* "MBIM Control Model Functional Descriptor" */
+struct usb_cdc_mbim_desc {
+	__u8	bLength;
+	__u8	bDescriptorType;
+	__u8	bDescriptorSubType;
+
+	__le16	bcdMBIMVersion;
+	__le16  wMaxControlMessage;
+	__u8    bNumberFilters;
+	__u8    bMaxFilterSize;
+	__le16  wMaxSegmentSize;
+	__u8    bmNetworkCapabilities;
+} __attribute__ ((packed));
+
 /*-------------------------------------------------------------------------*/
 
 /*
@@ -332,6 +350,11 @@ struct usb_cdc_ncm_nth32 {
 #define USB_CDC_NCM_NDP32_CRC_SIGN	0x316D636E /* ncm1 */
 #define USB_CDC_NCM_NDP32_NOCRC_SIGN	0x306D636E /* ncm0 */
 
+#define USB_CDC_MBIM_NDP16_IPS_SIGN     0x00535049 /* IPS<sessionID> : IPS0 for now */
+#define USB_CDC_MBIM_NDP32_IPS_SIGN     0x00737069 /* ips<sessionID> : ips0 for now */
+#define USB_CDC_MBIM_NDP16_DSS_SIGN     0x00535344 /* DSS<sessionID> */
+#define USB_CDC_MBIM_NDP32_DSS_SIGN     0x00737364 /* dss<sessionID> */
+
 /* 16-bit NCM Datagram Pointer Entry */
 struct usb_cdc_ncm_dpe16 {
 	__le16	wDatagramIndex;
-- 
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

[PATCH net-next 04/14] net: cdc_ncm: adding MBIM support to ncm_setup

[Bjørn Mork]

From: Greg Suarez <gsuarez@smithmicro.com>

MBIM and NCM are very similar, so we can reuse most of the
setup and bind logic in cdc_ncm for CDC MBIM devices.  Handle
a few minor differences in ncm_setup.

Signed-off-by: Greg Suarez <gsuarez@smithmicro.com>
Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
 drivers/net/usb/cdc_ncm.c |   51 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 36 insertions(+), 15 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 6a65662..bddb0e4 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -64,6 +64,9 @@
 /* Minimum value for MaxDatagramSize, ch. 6.2.9 */
 #define	CDC_NCM_MIN_DATAGRAM_SIZE		1514	/* bytes */
 
+/* Minimum value for MaxDatagramSize, ch. 8.1.3 */
+#define CDC_MBIM_MIN_DATAGRAM_SIZE              2048    /* bytes */
+
 #define	CDC_NCM_MIN_TX_PKT			512	/* bytes */
 
 /* Default value for MaxDatagramSize */
@@ -98,6 +101,7 @@ struct cdc_ncm_ctx {
 	struct tasklet_struct bh;
 
 	const struct usb_cdc_ncm_desc *func_desc;
+	const struct usb_cdc_mbim_desc *mbim_desc;
 	const struct usb_cdc_header_desc *header_desc;
 	const struct usb_cdc_union_desc *union_desc;
 	const struct usb_cdc_ether_desc *ether_desc;
@@ -158,7 +162,10 @@ static u8 cdc_ncm_setup(struct cdc_ncm_ctx *ctx)
 	u8 flags;
 	u8 iface_no;
 	int err;
+	int eth_hlen;
 	u16 ntb_fmt_supported;
+	u32 min_dgram_size;
+	u32 min_hdr_size;
 
 	iface_no = ctx->control->cur_altsetting->desc.bInterfaceNumber;
 
@@ -184,10 +191,19 @@ static u8 cdc_ncm_setup(struct cdc_ncm_ctx *ctx)
 	ctx->tx_max_datagrams = le16_to_cpu(ctx->ncm_parm.wNtbOutMaxDatagrams);
 	ntb_fmt_supported = le16_to_cpu(ctx->ncm_parm.bmNtbFormatsSupported);
 
-	if (ctx->func_desc != NULL)
+	eth_hlen = ETH_HLEN;
+	min_dgram_size = CDC_NCM_MIN_DATAGRAM_SIZE;
+	min_hdr_size = CDC_NCM_MIN_HDR_SIZE;
+	if (ctx->mbim_desc != NULL) {
+		flags = ctx->mbim_desc->bmNetworkCapabilities;
+		eth_hlen = 0;
+		min_dgram_size = CDC_MBIM_MIN_DATAGRAM_SIZE;
+		min_hdr_size = 0;
+	} else if (ctx->func_desc != NULL) {
 		flags = ctx->func_desc->bmNetworkCapabilities;
-	else
+	} else {
 		flags = 0;
+	}
 
 	pr_debug("dwNtbInMaxSize=%u dwNtbOutMaxSize=%u "
 		 "wNdpOutPayloadRemainder=%u wNdpOutDivisor=%u "
@@ -224,6 +240,7 @@ static u8 cdc_ncm_setup(struct cdc_ncm_ctx *ctx)
 				err = -ENOMEM;
 				goto size_err;
 			}
+			ndp_in_sz->dwNtbInMaxSize = cpu_to_le32(ctx->rx_max);
 
 			err = usb_control_msg(ctx->udev,
 					usb_sndctrlpipe(ctx->udev, 0),
@@ -260,7 +277,7 @@ size_err:
 
 	/* verify maximum size of transmitted NTB in bytes */
 	if ((ctx->tx_max <
-	    (CDC_NCM_MIN_HDR_SIZE + CDC_NCM_MIN_DATAGRAM_SIZE)) ||
+	    (min_hdr_size + min_dgram_size)) ||
 	    (ctx->tx_max > CDC_NCM_NTB_MAX_SIZE_TX)) {
 		pr_debug("Using default maximum transmit length=%d\n",
 						CDC_NCM_NTB_MAX_SIZE_TX);
@@ -302,8 +319,8 @@ size_err:
 	}
 
 	/* adjust TX-remainder according to NCM specification. */
-	ctx->tx_remainder = ((ctx->tx_remainder - ETH_HLEN) &
-						(ctx->tx_modulus - 1));
+	ctx->tx_remainder = ((ctx->tx_remainder - eth_hlen) &
+			     (ctx->tx_modulus - 1));
 
 	/* additional configuration */
 
@@ -330,12 +347,18 @@ size_err:
 			pr_debug("Setting NTB format to 16-bit failed\n");
 	}
 
-	ctx->max_datagram_size = CDC_NCM_MIN_DATAGRAM_SIZE;
+	ctx->max_datagram_size = min_dgram_size;
 
 	/* set Max Datagram Size (MTU) */
 	if (flags & USB_CDC_NCM_NCAP_MAX_DATAGRAM_SIZE) {
 		__le16 *max_datagram_size;
-		u16 eth_max_sz = le16_to_cpu(ctx->ether_desc->wMaxSegmentSize);
+		u16 eth_max_sz;
+		if (ctx->ether_desc != NULL)
+			eth_max_sz = le16_to_cpu(ctx->ether_desc->wMaxSegmentSize);
+		else if (ctx->mbim_desc != NULL)
+			eth_max_sz = le16_to_cpu(ctx->mbim_desc->wMaxSegmentSize);
+		else
+			goto max_dgram_err;
 
 		max_datagram_size = kzalloc(sizeof(*max_datagram_size),
 				GFP_KERNEL);
@@ -352,7 +375,7 @@ size_err:
 				2, 1000);
 		if (err < 0) {
 			pr_debug("GET_MAX_DATAGRAM_SIZE failed, use size=%u\n",
-						CDC_NCM_MIN_DATAGRAM_SIZE);
+				 min_dgram_size);
 		} else {
 			ctx->max_datagram_size =
 				le16_to_cpu(*max_datagram_size);
@@ -361,12 +384,10 @@ size_err:
 					ctx->max_datagram_size = eth_max_sz;
 
 			if (ctx->max_datagram_size > CDC_NCM_MAX_DATAGRAM_SIZE)
-				ctx->max_datagram_size =
-						CDC_NCM_MAX_DATAGRAM_SIZE;
+				ctx->max_datagram_size = CDC_NCM_MAX_DATAGRAM_SIZE;
 
-			if (ctx->max_datagram_size < CDC_NCM_MIN_DATAGRAM_SIZE)
-				ctx->max_datagram_size =
-					CDC_NCM_MIN_DATAGRAM_SIZE;
+			if (ctx->max_datagram_size < min_dgram_size)
+				ctx->max_datagram_size = min_dgram_size;
 
 			/* if value changed, update device */
 			if (ctx->max_datagram_size !=
@@ -387,8 +408,8 @@ size_err:
 	}
 
 max_dgram_err:
-	if (ctx->netdev->mtu != (ctx->max_datagram_size - ETH_HLEN))
-		ctx->netdev->mtu = ctx->max_datagram_size - ETH_HLEN;
+	if (ctx->netdev->mtu != (ctx->max_datagram_size - eth_hlen))
+		ctx->netdev->mtu = ctx->max_datagram_size - eth_hlen;
 
 	return 0;
 }
-- 
1.7.10.4

[PATCH net-next 05/14] net: cdc_ncm: refactor bind preparing for MBIM support

[Bjørn Mork]

From: Greg Suarez <gsuarez-AKjrjAf1O7qe8kRwQpwjMg@public.gmane.org>

NCM and MBIM can share most of the bind function.  Split
out the shareable part and add MBIM functional descriptor
parsing.

Signed-off-by: Greg Suarez <gsuarez-AKjrjAf1O7qe8kRwQpwjMg@public.gmane.org>
Signed-off-by: Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org>
---
 drivers/net/usb/cdc_ncm.c |   47 +++++++++++++++++++++++++++++++--------------
 1 file changed, 33 insertions(+), 14 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index bddb0e4..9d2e344 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -475,7 +475,7 @@ static const struct ethtool_ops cdc_ncm_ethtool_ops = {
 	.nway_reset = usbnet_nway_reset,
 };
 
-static int cdc_ncm_bind(struct usbnet *dev, struct usb_interface *intf)
+static int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8 data_altsetting)
 {
 	struct cdc_ncm_ctx *ctx;
 	struct usb_driver *driver;
@@ -549,6 +549,13 @@ static int cdc_ncm_bind(struct usbnet *dev, struct usb_interface *intf)
 			ctx->func_desc = (const struct usb_cdc_ncm_desc *)buf;
 			break;
 
+		case USB_CDC_MBIM_TYPE:
+			if (buf[0] < sizeof(*(ctx->mbim_desc)))
+				break;
+
+			ctx->mbim_desc = (const struct usb_cdc_mbim_desc *)buf;
+			break;
+
 		default:
 			break;
 		}
@@ -561,7 +568,7 @@ advance:
 
 	/* check if we got everything */
 	if ((ctx->control == NULL) || (ctx->data == NULL) ||
-	    (ctx->ether_desc == NULL) || (ctx->control != intf))
+	    ((!ctx->mbim_desc) && ((ctx->ether_desc == NULL) || (ctx->control != intf))))
 		goto error;
 
 	/* claim interfaces, if any */
@@ -581,7 +588,7 @@ advance:
 		goto error2;
 
 	/* configure data interface */
-	temp = usb_set_interface(dev->udev, iface_no, 1);
+	temp = usb_set_interface(dev->udev, iface_no, data_altsetting);
 	if (temp)
 		goto error2;
 
@@ -598,11 +605,13 @@ advance:
 	usb_set_intfdata(ctx->control, dev);
 	usb_set_intfdata(ctx->intf, dev);
 
-	temp = usbnet_get_ethernet_addr(dev, ctx->ether_desc->iMACAddress);
-	if (temp)
-		goto error2;
+	if (ctx->ether_desc) {
+		temp = usbnet_get_ethernet_addr(dev, ctx->ether_desc->iMACAddress);
+		if (temp)
+			goto error2;
+		dev_info(&dev->udev->dev, "MAC-Address: %pM\n", dev->net->dev_addr);
+	}
 
-	dev_info(&dev->udev->dev, "MAC-Address: %pM\n", dev->net->dev_addr);
 
 	dev->in = usb_rcvbulkpipe(dev->udev,
 		ctx->in_ep->desc.bEndpointAddress & USB_ENDPOINT_NUMBER_MASK);
@@ -611,13 +620,6 @@ advance:
 	dev->status = ctx->status_ep;
 	dev->rx_urb_size = ctx->rx_max;
 
-	/*
-	 * We should get an event when network connection is "connected" or
-	 * "disconnected". Set network connection in "disconnected" state
-	 * (carrier is OFF) during attach, so the IP network stack does not
-	 * start IPv6 negotiation and more.
-	 */
-	netif_carrier_off(dev->net);
 	ctx->tx_speed = ctx->rx_speed = 0;
 	return 0;
 
@@ -663,6 +665,23 @@ static void cdc_ncm_unbind(struct usbnet *dev, struct usb_interface *intf)
 	cdc_ncm_free(ctx);
 }
 
+static int cdc_ncm_bind(struct usbnet *dev, struct usb_interface *intf)
+{
+	int ret;
+
+	/* NCM data altsetting is always 1 */
+	ret = cdc_ncm_bind_common(dev, intf, 1);
+
+	/*
+	 * We should get an event when network connection is "connected" or
+	 * "disconnected". Set network connection in "disconnected" state
+	 * (carrier is OFF) during attach, so the IP network stack does not
+	 * start IPv6 negotiation and more.
+	 */
+	netif_carrier_off(dev->net);
+	return ret;
+}
+
 static void cdc_ncm_zero_fill(u8 *ptr, u32 first, u32 end, u32 max)
 {
 	if (first >= max)
-- 
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

[PATCH net-next 06/14] net: cdc_ncm: process chained NDPs

[Bjørn Mork]

The NCM 1.0 spefication makes provisions for linking more than
one NDP into a single NTB. This is important for MBIM support,
where these NDPs might be of different types.

Following the chain of NDPs is also correct for NCM, and will
not change anything in the common case where there is only
one NDP

Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
 drivers/net/usb/cdc_ncm.c |   35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 9d2e344..889969d 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -1001,6 +1001,8 @@ static int cdc_ncm_rx_fixup(struct usbnet *dev, struct sk_buff *skb_in)
 	struct usb_cdc_ncm_nth16 *nth16;
 	struct usb_cdc_ncm_ndp16 *ndp16;
 	struct usb_cdc_ncm_dpe16 *dpe16;
+	int ndpoffset;
+	int loopcount = 50; /* arbitrary max preventing infinite loop */
 
 	if (ctx == NULL)
 		goto error;
@@ -1034,25 +1036,24 @@ static int cdc_ncm_rx_fixup(struct usbnet *dev, struct sk_buff *skb_in)
 	}
 	ctx->rx_seq = le16_to_cpu(nth16->wSequence);
 
-	len = le16_to_cpu(nth16->wNdpIndex);
-	if ((len + sizeof(struct usb_cdc_ncm_ndp16)) > skb_in->len) {
-		pr_debug("invalid DPT16 index <%u>\n",
-					le16_to_cpu(nth16->wNdpIndex));
+	ndpoffset = le16_to_cpu(nth16->wNdpIndex);
+next_ndp:
+	if ((ndpoffset + sizeof(struct usb_cdc_ncm_ndp16)) > skb_in->len) {
+		pr_debug("invalid NDP offset  <%u>\n", ndpoffset);
 		goto error;
 	}
-
-	ndp16 = (struct usb_cdc_ncm_ndp16 *)(((u8 *)skb_in->data) + len);
+	ndp16 = (struct usb_cdc_ncm_ndp16 *)(skb_in->data + ndpoffset);
 
 	if (le32_to_cpu(ndp16->dwSignature) != USB_CDC_NCM_NDP16_NOCRC_SIGN) {
 		pr_debug("invalid DPT16 signature <%u>\n",
 					le32_to_cpu(ndp16->dwSignature));
-		goto error;
+		goto err_ndp;
 	}
 
 	if (le16_to_cpu(ndp16->wLength) < USB_CDC_NCM_NDP16_LENGTH_MIN) {
 		pr_debug("invalid DPT16 length <%u>\n",
 					le32_to_cpu(ndp16->dwSignature));
-		goto error;
+		goto err_ndp;
 	}
 
 	nframes = ((le16_to_cpu(ndp16->wLength) -
@@ -1060,15 +1061,15 @@ static int cdc_ncm_rx_fixup(struct usbnet *dev, struct sk_buff *skb_in)
 					sizeof(struct usb_cdc_ncm_dpe16));
 	nframes--; /* we process NDP entries except for the last one */
 
-	len += sizeof(struct usb_cdc_ncm_ndp16);
+	ndpoffset += sizeof(struct usb_cdc_ncm_ndp16);
 
-	if ((len + nframes * (sizeof(struct usb_cdc_ncm_dpe16))) >
+	if ((ndpoffset + nframes * (sizeof(struct usb_cdc_ncm_dpe16))) >
 								skb_in->len) {
 		pr_debug("Invalid nframes = %d\n", nframes);
-		goto error;
+		goto err_ndp;
 	}
 
-	dpe16 = (struct usb_cdc_ncm_dpe16 *)(((u8 *)skb_in->data) + len);
+	dpe16 = (struct usb_cdc_ncm_dpe16 *)(skb_in->data + ndpoffset);
 
 	for (x = 0; x < nframes; x++, dpe16++) {
 		offset = le16_to_cpu(dpe16->wDatagramIndex);
@@ -1080,7 +1081,7 @@ static int cdc_ncm_rx_fixup(struct usbnet *dev, struct sk_buff *skb_in)
 		 */
 		if ((offset == 0) || (len == 0)) {
 			if (!x)
-				goto error; /* empty NTB */
+				goto err_ndp; /* empty NTB */
 			break;
 		}
 
@@ -1091,7 +1092,7 @@ static int cdc_ncm_rx_fixup(struct usbnet *dev, struct sk_buff *skb_in)
 					"offset[%u]=%u, length=%u, skb=%p\n",
 					x, offset, len, skb_in);
 			if (!x)
-				goto error;
+				goto err_ndp;
 			break;
 
 		} else {
@@ -1104,6 +1105,12 @@ static int cdc_ncm_rx_fixup(struct usbnet *dev, struct sk_buff *skb_in)
 			usbnet_skb_return(dev, skb);
 		}
 	}
+err_ndp:
+	/* are there more NDPs to process? */
+	ndpoffset = le16_to_cpu(ndp16->wNextNdpIndex);
+	if (ndpoffset && loopcount--)
+		goto next_ndp;
+
 	return 1;
 error:
 	return 0;
-- 
1.7.10.4

[PATCH net-next 07/14] net: cdc_ncm: splitting rx_fixup for code reuse

[Bjørn Mork]

Verifying and handling received MBIM and NCM frames will need
to be different in three areas:
 - verifying the NDP signature
 - checking valid datagram length
 - datagram header manipulation

This makes it inconvenient to share rx_fixup in whole.  But
some verification parts are common.  Split these out in separate
functions.

Signed-off-by: Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org>
---
 drivers/net/usb/cdc_ncm.c |   83 ++++++++++++++++++++++++++++++---------------
 1 file changed, 55 insertions(+), 28 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 889969d..07d199a 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -990,19 +990,12 @@ error:
 	return NULL;
 }
 
-static int cdc_ncm_rx_fixup(struct usbnet *dev, struct sk_buff *skb_in)
+/* verify NTB header and return offset of first NDP, or negative error */
+static int cdc_ncm_rx_verify_nth16(struct cdc_ncm_ctx *ctx, struct sk_buff *skb_in)
 {
-	struct sk_buff *skb;
-	struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
-	int len;
-	int nframes;
-	int x;
-	int offset;
 	struct usb_cdc_ncm_nth16 *nth16;
-	struct usb_cdc_ncm_ndp16 *ndp16;
-	struct usb_cdc_ncm_dpe16 *dpe16;
-	int ndpoffset;
-	int loopcount = 50; /* arbitrary max preventing infinite loop */
+	int len;
+	int ret = -EINVAL;
 
 	if (ctx == NULL)
 		goto error;
@@ -1036,40 +1029,74 @@ static int cdc_ncm_rx_fixup(struct usbnet *dev, struct sk_buff *skb_in)
 	}
 	ctx->rx_seq = le16_to_cpu(nth16->wSequence);
 
-	ndpoffset = le16_to_cpu(nth16->wNdpIndex);
-next_ndp:
+	ret = le16_to_cpu(nth16->wNdpIndex);
+error:
+	return ret;
+}
+
+/* verify NDP header and return number of datagrams, or negative error */
+static int cdc_ncm_rx_verify_ndp16(struct sk_buff *skb_in, int ndpoffset)
+{
+	struct usb_cdc_ncm_ndp16 *ndp16;
+	int ret = -EINVAL;
+
 	if ((ndpoffset + sizeof(struct usb_cdc_ncm_ndp16)) > skb_in->len) {
 		pr_debug("invalid NDP offset  <%u>\n", ndpoffset);
 		goto error;
 	}
 	ndp16 = (struct usb_cdc_ncm_ndp16 *)(skb_in->data + ndpoffset);
 
-	if (le32_to_cpu(ndp16->dwSignature) != USB_CDC_NCM_NDP16_NOCRC_SIGN) {
-		pr_debug("invalid DPT16 signature <%u>\n",
-					le32_to_cpu(ndp16->dwSignature));
-		goto err_ndp;
-	}
-
 	if (le16_to_cpu(ndp16->wLength) < USB_CDC_NCM_NDP16_LENGTH_MIN) {
 		pr_debug("invalid DPT16 length <%u>\n",
 					le32_to_cpu(ndp16->dwSignature));
-		goto err_ndp;
+		goto error;
 	}
 
-	nframes = ((le16_to_cpu(ndp16->wLength) -
+	ret = ((le16_to_cpu(ndp16->wLength) -
 					sizeof(struct usb_cdc_ncm_ndp16)) /
 					sizeof(struct usb_cdc_ncm_dpe16));
-	nframes--; /* we process NDP entries except for the last one */
-
-	ndpoffset += sizeof(struct usb_cdc_ncm_ndp16);
+	ret--; /* we process NDP entries except for the last one */
 
-	if ((ndpoffset + nframes * (sizeof(struct usb_cdc_ncm_dpe16))) >
+	if ((sizeof(struct usb_cdc_ncm_ndp16) + ret * (sizeof(struct usb_cdc_ncm_dpe16))) >
 								skb_in->len) {
-		pr_debug("Invalid nframes = %d\n", nframes);
-		goto err_ndp;
+		pr_debug("Invalid nframes = %d\n", ret);
+		ret = -EINVAL;
 	}
 
-	dpe16 = (struct usb_cdc_ncm_dpe16 *)(skb_in->data + ndpoffset);
+error:
+	return ret;
+}
+
+static int cdc_ncm_rx_fixup(struct usbnet *dev, struct sk_buff *skb_in)
+{
+	struct sk_buff *skb;
+	struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
+	int len;
+	int nframes;
+	int x;
+	int offset;
+	struct usb_cdc_ncm_ndp16 *ndp16;
+	struct usb_cdc_ncm_dpe16 *dpe16;
+	int ndpoffset;
+	int loopcount = 50; /* arbitrary max preventing infinite loop */
+
+	ndpoffset = cdc_ncm_rx_verify_nth16(ctx, skb_in);
+	if (ndpoffset < 0)
+		goto error;
+
+next_ndp:
+	nframes = cdc_ncm_rx_verify_ndp16(skb_in, ndpoffset);
+	if (nframes < 0)
+		goto error;
+
+	ndp16 = (struct usb_cdc_ncm_ndp16 *)(skb_in->data + ndpoffset);
+
+	if (le32_to_cpu(ndp16->dwSignature) != USB_CDC_NCM_NDP16_NOCRC_SIGN) {
+		pr_debug("invalid DPT16 signature <%u>\n",
+			 le32_to_cpu(ndp16->dwSignature));
+		goto err_ndp;
+	}
+	dpe16 = ndp16->dpe16;
 
 	for (x = 0; x < nframes; x++, dpe16++) {
 		offset = le16_to_cpu(dpe16->wDatagramIndex);
-- 
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

[PATCH net-next 08/14] net: cdc_ncm: refactoring for tx multiplexing

[Bjørn Mork]

Adding multiplexed NDP support to cdc_ncm_fill_tx_frame, allowing
transmissions of multiple independent sessions within the same NTB.

Refactoring the code quite a bit to avoid having to store copies
of multiple NDPs being prepared for tx. The old code would still
reserve enough room for a maximum sized NDP in the skb so we might
as well keep them in the skb while they are being prepared.

Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
 drivers/net/usb/cdc_ncm.c |  247 +++++++++++++++++++--------------------------
 1 file changed, 102 insertions(+), 145 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 07d199a..aeebf7c 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -88,14 +88,11 @@
 	(sizeof(struct usb_cdc_ncm_nth16) + sizeof(struct usb_cdc_ncm_ndp16) + \
 	(CDC_NCM_DPT_DATAGRAMS_MAX + 1) * sizeof(struct usb_cdc_ncm_dpe16))
 
-struct cdc_ncm_data {
-	struct usb_cdc_ncm_nth16 nth16;
-	struct usb_cdc_ncm_ndp16 ndp16;
-	struct usb_cdc_ncm_dpe16 dpe16[CDC_NCM_DPT_DATAGRAMS_MAX + 1];
-};
+#define CDC_NCM_NDP_SIZE \
+	(sizeof(struct usb_cdc_ncm_ndp16) +				\
+	      (CDC_NCM_DPT_DATAGRAMS_MAX + 1) * sizeof(struct usb_cdc_ncm_dpe16))
 
 struct cdc_ncm_ctx {
-	struct cdc_ncm_data tx_ncm;
 	struct usb_cdc_ncm_ntb_parameters ncm_parm;
 	struct hrtimer tx_timer;
 	struct tasklet_struct bh;
@@ -117,13 +114,12 @@ struct cdc_ncm_ctx {
 
 	struct sk_buff *tx_curr_skb;
 	struct sk_buff *tx_rem_skb;
+	__le32 tx_rem_sign;
 
 	spinlock_t mtx;
 	atomic_t stop;
 
 	u32 tx_timer_pending;
-	u32 tx_curr_offset;
-	u32 tx_curr_last_offset;
 	u32 tx_curr_frame_num;
 	u32 rx_speed;
 	u32 tx_speed;
@@ -682,51 +678,75 @@ static int cdc_ncm_bind(struct usbnet *dev, struct usb_interface *intf)
 	return ret;
 }
 
-static void cdc_ncm_zero_fill(u8 *ptr, u32 first, u32 end, u32 max)
+static void cdc_ncm_align_tail(struct sk_buff *skb, size_t modulus, size_t remainder, size_t max)
 {
-	if (first >= max)
-		return;
-	if (first >= end)
-		return;
-	if (end > max)
-		end = max;
-	memset(ptr + first, 0, end - first);
+	size_t align = ALIGN(skb->len, modulus) - skb->len + remainder;
+
+	if (skb->len + align > max)
+		align = max - skb->len;
+	if (align && skb_tailroom(skb) >= align)
+		memset(skb_put(skb, align), 0, align);
+}
+
+/* return a pointer to a valid struct usb_cdc_ncm_ndp16 of type sign, possibly
+ * allocating a new one within skb
+ */
+static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp(struct cdc_ncm_ctx *ctx, struct sk_buff *skb, __le32 sign, size_t reserve)
+{
+	struct usb_cdc_ncm_ndp16 *ndp16 = NULL;
+	struct usb_cdc_ncm_nth16 *nth16 = (void *)skb->data;
+	size_t ndpoffset = le16_to_cpu(nth16->wNdpIndex);
+
+	/* follow the chain of NDPs, looking for a match */
+	while (ndpoffset) {
+		ndp16 = (struct usb_cdc_ncm_ndp16 *)(skb->data + ndpoffset);
+		if  (ndp16->dwSignature == sign)
+			return ndp16;
+		ndpoffset = le16_to_cpu(ndp16->wNextNdpIndex);
+	}
+
+	/* align new NDP */
+	cdc_ncm_align_tail(skb, ctx->tx_ndp_modulus, 0, ctx->tx_max);
+
+	/* verify that there is room for the NDP and the datagram (reserve) */
+	if ((ctx->tx_max - skb->len - reserve) < CDC_NCM_NDP_SIZE)
+		return NULL;
+
+	/* link to it */
+	if (ndp16)
+		ndp16->wNextNdpIndex = cpu_to_le16(skb->len);
+	else
+		nth16->wNdpIndex = cpu_to_le16(skb->len);
+
+	/* push a new empty NDP */
+	ndp16 = (struct usb_cdc_ncm_ndp16 *)memset(skb_put(skb, CDC_NCM_NDP_SIZE), 0, CDC_NCM_NDP_SIZE);
+	ndp16->dwSignature = sign;
+	ndp16->wLength = cpu_to_le16(sizeof(struct usb_cdc_ncm_ndp16) + sizeof(struct usb_cdc_ncm_dpe16));
+	return ndp16;
 }
 
 static struct sk_buff *
-cdc_ncm_fill_tx_frame(struct cdc_ncm_ctx *ctx, struct sk_buff *skb)
+cdc_ncm_fill_tx_frame(struct cdc_ncm_ctx *ctx, struct sk_buff *skb, __le32 sign)
 {
+	struct usb_cdc_ncm_nth16 *nth16;
+	struct usb_cdc_ncm_ndp16 *ndp16;
 	struct sk_buff *skb_out;
-	u32 rem;
-	u32 offset;
-	u32 last_offset;
-	u16 n = 0, index;
+	u16 n = 0, index, ndplen;
 	u8 ready2send = 0;
 
 	/* if there is a remaining skb, it gets priority */
-	if (skb != NULL)
+	if (skb != NULL) {
 		swap(skb, ctx->tx_rem_skb);
-	else
+		swap(sign, ctx->tx_rem_sign);
+	} else {
 		ready2send = 1;
-
-	/*
-	 * +----------------+
-	 * | skb_out        |
-	 * +----------------+
-	 *           ^ offset
-	 *        ^ last_offset
-	 */
+	}
 
 	/* check if we are resuming an OUT skb */
-	if (ctx->tx_curr_skb != NULL) {
-		/* pop variables */
-		skb_out = ctx->tx_curr_skb;
-		offset = ctx->tx_curr_offset;
-		last_offset = ctx->tx_curr_last_offset;
-		n = ctx->tx_curr_frame_num;
+	skb_out = ctx->tx_curr_skb;
 
-	} else {
-		/* reset variables */
+	/* allocate a new OUT skb */
+	if (!skb_out) {
 		skb_out = alloc_skb((ctx->tx_max + 1), GFP_ATOMIC);
 		if (skb_out == NULL) {
 			if (skb != NULL) {
@@ -735,35 +755,21 @@ cdc_ncm_fill_tx_frame(struct cdc_ncm_ctx *ctx, struct sk_buff *skb)
 			}
 			goto exit_no_skb;
 		}
+		/* fill out the initial 16-bit NTB header */
+		nth16 = (struct usb_cdc_ncm_nth16 *)memset(skb_put(skb_out, sizeof(struct usb_cdc_ncm_nth16)), 0, sizeof(struct usb_cdc_ncm_nth16));
+		nth16->dwSignature = cpu_to_le32(USB_CDC_NCM_NTH16_SIGN);
+		nth16->wHeaderLength = cpu_to_le16(sizeof(struct usb_cdc_ncm_nth16));
+		nth16->wSequence = cpu_to_le16(ctx->tx_seq++);
 
-		/* make room for NTH and NDP */
-		offset = ALIGN(sizeof(struct usb_cdc_ncm_nth16),
-					ctx->tx_ndp_modulus) +
-					sizeof(struct usb_cdc_ncm_ndp16) +
-					(ctx->tx_max_datagrams + 1) *
-					sizeof(struct usb_cdc_ncm_dpe16);
-
-		/* store last valid offset before alignment */
-		last_offset = offset;
-		/* align first Datagram offset correctly */
-		offset = ALIGN(offset, ctx->tx_modulus) + ctx->tx_remainder;
-		/* zero buffer till the first IP datagram */
-		cdc_ncm_zero_fill(skb_out->data, 0, offset, offset);
-		n = 0;
+		/* count total number of frames in this NTB */
 		ctx->tx_curr_frame_num = 0;
 	}
 
-	for (; n < ctx->tx_max_datagrams; n++) {
-		/* check if end of transmit buffer is reached */
-		if (offset >= ctx->tx_max) {
-			ready2send = 1;
-			break;
-		}
-		/* compute maximum buffer size */
-		rem = ctx->tx_max - offset;
-
+	for (n = ctx->tx_curr_frame_num; n < ctx->tx_max_datagrams; n++) {
+		/* send any remaining skb first */
 		if (skb == NULL) {
 			skb = ctx->tx_rem_skb;
+			sign = ctx->tx_rem_sign;
 			ctx->tx_rem_skb = NULL;
 
 			/* check for end of skb */
@@ -771,7 +777,14 @@ cdc_ncm_fill_tx_frame(struct cdc_ncm_ctx *ctx, struct sk_buff *skb)
 				break;
 		}
 
-		if (skb->len > rem) {
+		/* get the appropriate NDP for this skb */
+		ndp16 = cdc_ncm_ndp(ctx, skb_out, sign, skb->len + ctx->tx_modulus + ctx->tx_remainder);
+
+		/* align beginning of next frame */
+		cdc_ncm_align_tail(skb_out,  ctx->tx_modulus, ctx->tx_remainder, ctx->tx_max);
+
+		/* check if we had enough room left for both NDP and frame */
+		if (!ndp16 || skb_out->len + skb->len > ctx->tx_max) {
 			if (n == 0) {
 				/* won't fit, MTU problem? */
 				dev_kfree_skb_any(skb);
@@ -784,31 +797,30 @@ cdc_ncm_fill_tx_frame(struct cdc_ncm_ctx *ctx, struct sk_buff *skb)
 					ctx->netdev->stats.tx_dropped++;
 				}
 				ctx->tx_rem_skb = skb;
+				ctx->tx_rem_sign = sign;
 				skb = NULL;
 				ready2send = 1;
 			}
 			break;
 		}
 
-		memcpy(((u8 *)skb_out->data) + offset, skb->data, skb->len);
-
-		ctx->tx_ncm.dpe16[n].wDatagramLength = cpu_to_le16(skb->len);
-		ctx->tx_ncm.dpe16[n].wDatagramIndex = cpu_to_le16(offset);
-
-		/* update offset */
-		offset += skb->len;
+		/* calculate frame number withing this NDP */
+		ndplen = le16_to_cpu(ndp16->wLength);
+		index = (ndplen - sizeof(struct usb_cdc_ncm_ndp16)) / sizeof(struct usb_cdc_ncm_dpe16) - 1;
 
-		/* store last valid offset before alignment */
-		last_offset = offset;
-
-		/* align offset correctly */
-		offset = ALIGN(offset, ctx->tx_modulus) + ctx->tx_remainder;
-
-		/* zero padding */
-		cdc_ncm_zero_fill(skb_out->data, last_offset, offset,
-								ctx->tx_max);
+		/* OK, add this skb */
+		ndp16->dpe16[index].wDatagramLength = cpu_to_le16(skb->len);
+		ndp16->dpe16[index].wDatagramIndex = cpu_to_le16(skb_out->len);
+		ndp16->wLength = cpu_to_le16(ndplen + sizeof(struct usb_cdc_ncm_dpe16));
+		memcpy(skb_put(skb_out, skb->len), skb->data, skb->len);
 		dev_kfree_skb_any(skb);
 		skb = NULL;
+
+		/* send now if this NDP is full */
+		if (index >= CDC_NCM_DPT_DATAGRAMS_MAX) {
+			ready2send = 1;
+			break;
+		}
 	}
 
 	/* free up any dangling skb */
@@ -824,16 +836,12 @@ cdc_ncm_fill_tx_frame(struct cdc_ncm_ctx *ctx, struct sk_buff *skb)
 		/* wait for more frames */
 		/* push variables */
 		ctx->tx_curr_skb = skb_out;
-		ctx->tx_curr_offset = offset;
-		ctx->tx_curr_last_offset = last_offset;
 		goto exit_no_skb;
 
 	} else if ((n < ctx->tx_max_datagrams) && (ready2send == 0)) {
 		/* wait for more frames */
 		/* push variables */
 		ctx->tx_curr_skb = skb_out;
-		ctx->tx_curr_offset = offset;
-		ctx->tx_curr_last_offset = last_offset;
 		/* set the pending count */
 		if (n < CDC_NCM_RESTART_TIMER_DATAGRAM_CNT)
 			ctx->tx_timer_pending = CDC_NCM_TIMER_PENDING_CNT;
@@ -844,75 +852,24 @@ cdc_ncm_fill_tx_frame(struct cdc_ncm_ctx *ctx, struct sk_buff *skb)
 		/* variables will be reset at next call */
 	}
 
-	/* check for overflow */
-	if (last_offset > ctx->tx_max)
-		last_offset = ctx->tx_max;
-
-	/* revert offset */
-	offset = last_offset;
-
 	/*
 	 * If collected data size is less or equal CDC_NCM_MIN_TX_PKT bytes,
 	 * we send buffers as it is. If we get more data, it would be more
 	 * efficient for USB HS mobile device with DMA engine to receive a full
 	 * size NTB, than canceling DMA transfer and receiving a short packet.
 	 */
-	if (offset > CDC_NCM_MIN_TX_PKT)
-		offset = ctx->tx_max;
-
-	/* final zero padding */
-	cdc_ncm_zero_fill(skb_out->data, last_offset, offset, ctx->tx_max);
-
-	/* store last offset */
-	last_offset = offset;
-
-	if (((last_offset < ctx->tx_max) && ((last_offset %
-			le16_to_cpu(ctx->out_ep->desc.wMaxPacketSize)) == 0)) ||
-	    (((last_offset == ctx->tx_max) && ((ctx->tx_max %
-		le16_to_cpu(ctx->out_ep->desc.wMaxPacketSize)) == 0)) &&
-		(ctx->tx_max < le32_to_cpu(ctx->ncm_parm.dwNtbOutMaxSize)))) {
-		/* force short packet */
-		*(((u8 *)skb_out->data) + last_offset) = 0;
-		last_offset++;
-	}
+	if (skb_out->len > CDC_NCM_MIN_TX_PKT)
+		/* final zero padding */
+		memset(skb_put(skb_out, ctx->tx_max - skb_out->len), 0, ctx->tx_max - skb_out->len);
 
-	/* zero the rest of the DPEs plus the last NULL entry */
-	for (; n <= CDC_NCM_DPT_DATAGRAMS_MAX; n++) {
-		ctx->tx_ncm.dpe16[n].wDatagramLength = 0;
-		ctx->tx_ncm.dpe16[n].wDatagramIndex = 0;
-	}
-
-	/* fill out 16-bit NTB header */
-	ctx->tx_ncm.nth16.dwSignature = cpu_to_le32(USB_CDC_NCM_NTH16_SIGN);
-	ctx->tx_ncm.nth16.wHeaderLength =
-					cpu_to_le16(sizeof(ctx->tx_ncm.nth16));
-	ctx->tx_ncm.nth16.wSequence = cpu_to_le16(ctx->tx_seq);
-	ctx->tx_ncm.nth16.wBlockLength = cpu_to_le16(last_offset);
-	index = ALIGN(sizeof(struct usb_cdc_ncm_nth16), ctx->tx_ndp_modulus);
-	ctx->tx_ncm.nth16.wNdpIndex = cpu_to_le16(index);
-
-	memcpy(skb_out->data, &(ctx->tx_ncm.nth16), sizeof(ctx->tx_ncm.nth16));
-	ctx->tx_seq++;
-
-	/* fill out 16-bit NDP table */
-	ctx->tx_ncm.ndp16.dwSignature =
-				cpu_to_le32(USB_CDC_NCM_NDP16_NOCRC_SIGN);
-	rem = sizeof(ctx->tx_ncm.ndp16) + ((ctx->tx_curr_frame_num + 1) *
-					sizeof(struct usb_cdc_ncm_dpe16));
-	ctx->tx_ncm.ndp16.wLength = cpu_to_le16(rem);
-	ctx->tx_ncm.ndp16.wNextNdpIndex = 0; /* reserved */
-
-	memcpy(((u8 *)skb_out->data) + index,
-						&(ctx->tx_ncm.ndp16),
-						sizeof(ctx->tx_ncm.ndp16));
-
-	memcpy(((u8 *)skb_out->data) + index + sizeof(ctx->tx_ncm.ndp16),
-					&(ctx->tx_ncm.dpe16),
-					(ctx->tx_curr_frame_num + 1) *
-					sizeof(struct usb_cdc_ncm_dpe16));
+	/* do we need to prevent a ZLP? */
+	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 */
 
-	/* set frame length */
-	skb_put(skb_out, last_offset);
+	/* set final frame length */
+	nth16 = (struct usb_cdc_ncm_nth16 *)skb_out->data;
+	nth16->wBlockLength = cpu_to_le16(skb_out->len);
 
 	/* return skb */
 	ctx->tx_curr_skb = NULL;
@@ -979,7 +936,7 @@ cdc_ncm_tx_fixup(struct usbnet *dev, struct sk_buff *skb, gfp_t flags)
 		goto error;
 
 	spin_lock_bh(&ctx->mtx);
-	skb_out = cdc_ncm_fill_tx_frame(ctx, skb);
+	skb_out = cdc_ncm_fill_tx_frame(ctx, skb, cpu_to_le32(USB_CDC_NCM_NDP16_NOCRC_SIGN));
 	spin_unlock_bh(&ctx->mtx);
 	return skb_out;
 
-- 
1.7.10.4

[PATCH net-next 09/14] net: cdc_ncm: export shared symbols and definitions

[Bjørn Mork]

Move symbols and definitons which can be shared with a
MBIM driver in a new header.

Signed-off-by: Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org>
---
 drivers/net/usb/cdc_ncm.c   |   97 ++++-----------------------------
 include/linux/usb/cdc_ncm.h |  124 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 135 insertions(+), 86 deletions(-)
 create mode 100644 include/linux/usb/cdc_ncm.h

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index aeebf7c..0c941ef 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -51,90 +51,10 @@
 #include <linux/atomic.h>
 #include <linux/usb/usbnet.h>
 #include <linux/usb/cdc.h>
+#include <linux/usb/cdc_ncm.h>
 
 #define	DRIVER_VERSION				"14-Mar-2012"
 
-/* CDC NCM subclass 3.2.1 */
-#define USB_CDC_NCM_NDP16_LENGTH_MIN		0x10
-
-/* Maximum NTB length */
-#define	CDC_NCM_NTB_MAX_SIZE_TX			32768	/* bytes */
-#define	CDC_NCM_NTB_MAX_SIZE_RX			32768	/* bytes */
-
-/* Minimum value for MaxDatagramSize, ch. 6.2.9 */
-#define	CDC_NCM_MIN_DATAGRAM_SIZE		1514	/* bytes */
-
-/* Minimum value for MaxDatagramSize, ch. 8.1.3 */
-#define CDC_MBIM_MIN_DATAGRAM_SIZE              2048    /* bytes */
-
-#define	CDC_NCM_MIN_TX_PKT			512	/* bytes */
-
-/* Default value for MaxDatagramSize */
-#define	CDC_NCM_MAX_DATAGRAM_SIZE		8192	/* bytes */
-
-/*
- * Maximum amount of datagrams in NCM Datagram Pointer Table, not counting
- * the last NULL entry.
- */
-#define	CDC_NCM_DPT_DATAGRAMS_MAX		40
-
-/* Restart the timer, if amount of datagrams is less than given value */
-#define	CDC_NCM_RESTART_TIMER_DATAGRAM_CNT	3
-#define	CDC_NCM_TIMER_PENDING_CNT		2
-#define CDC_NCM_TIMER_INTERVAL			(400UL * NSEC_PER_USEC)
-
-/* The following macro defines the minimum header space */
-#define	CDC_NCM_MIN_HDR_SIZE \
-	(sizeof(struct usb_cdc_ncm_nth16) + sizeof(struct usb_cdc_ncm_ndp16) + \
-	(CDC_NCM_DPT_DATAGRAMS_MAX + 1) * sizeof(struct usb_cdc_ncm_dpe16))
-
-#define CDC_NCM_NDP_SIZE \
-	(sizeof(struct usb_cdc_ncm_ndp16) +				\
-	      (CDC_NCM_DPT_DATAGRAMS_MAX + 1) * sizeof(struct usb_cdc_ncm_dpe16))
-
-struct cdc_ncm_ctx {
-	struct usb_cdc_ncm_ntb_parameters ncm_parm;
-	struct hrtimer tx_timer;
-	struct tasklet_struct bh;
-
-	const struct usb_cdc_ncm_desc *func_desc;
-	const struct usb_cdc_mbim_desc *mbim_desc;
-	const struct usb_cdc_header_desc *header_desc;
-	const struct usb_cdc_union_desc *union_desc;
-	const struct usb_cdc_ether_desc *ether_desc;
-
-	struct net_device *netdev;
-	struct usb_device *udev;
-	struct usb_host_endpoint *in_ep;
-	struct usb_host_endpoint *out_ep;
-	struct usb_host_endpoint *status_ep;
-	struct usb_interface *intf;
-	struct usb_interface *control;
-	struct usb_interface *data;
-
-	struct sk_buff *tx_curr_skb;
-	struct sk_buff *tx_rem_skb;
-	__le32 tx_rem_sign;
-
-	spinlock_t mtx;
-	atomic_t stop;
-
-	u32 tx_timer_pending;
-	u32 tx_curr_frame_num;
-	u32 rx_speed;
-	u32 tx_speed;
-	u32 rx_max;
-	u32 tx_max;
-	u32 max_datagram_size;
-	u16 tx_max_datagrams;
-	u16 tx_remainder;
-	u16 tx_modulus;
-	u16 tx_ndp_modulus;
-	u16 tx_seq;
-	u16 rx_seq;
-	u16 connected;
-};
-
 static void cdc_ncm_txpath_bh(unsigned long param);
 static void cdc_ncm_tx_timeout_start(struct cdc_ncm_ctx *ctx);
 static enum hrtimer_restart cdc_ncm_tx_timer_cb(struct hrtimer *hr_timer);
@@ -471,7 +391,7 @@ static const struct ethtool_ops cdc_ncm_ethtool_ops = {
 	.nway_reset = usbnet_nway_reset,
 };
 
-static int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8 data_altsetting)
+int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8 data_altsetting)
 {
 	struct cdc_ncm_ctx *ctx;
 	struct usb_driver *driver;
@@ -629,8 +549,9 @@ error:
 	dev_info(&dev->udev->dev, "bind() failure\n");
 	return -ENODEV;
 }
+EXPORT_SYMBOL_GPL(cdc_ncm_bind_common);
 
-static void cdc_ncm_unbind(struct usbnet *dev, struct usb_interface *intf)
+void cdc_ncm_unbind(struct usbnet *dev, struct usb_interface *intf)
 {
 	struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
 	struct usb_driver *driver = driver_of(intf);
@@ -660,6 +581,7 @@ static void cdc_ncm_unbind(struct usbnet *dev, struct usb_interface *intf)
 	usb_set_intfdata(ctx->intf, NULL);
 	cdc_ncm_free(ctx);
 }
+EXPORT_SYMBOL_GPL(cdc_ncm_unbind);
 
 static int cdc_ncm_bind(struct usbnet *dev, struct usb_interface *intf)
 {
@@ -725,7 +647,7 @@ static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp(struct cdc_ncm_ctx *ctx, struct sk_
 	return ndp16;
 }
 
-static struct sk_buff *
+struct sk_buff *
 cdc_ncm_fill_tx_frame(struct cdc_ncm_ctx *ctx, struct sk_buff *skb, __le32 sign)
 {
 	struct usb_cdc_ncm_nth16 *nth16;
@@ -882,6 +804,7 @@ exit_no_skb:
 		cdc_ncm_tx_timeout_start(ctx);
 	return NULL;
 }
+EXPORT_SYMBOL_GPL(cdc_ncm_fill_tx_frame);
 
 static void cdc_ncm_tx_timeout_start(struct cdc_ncm_ctx *ctx)
 {
@@ -948,7 +871,7 @@ error:
 }
 
 /* verify NTB header and return offset of first NDP, or negative error */
-static int cdc_ncm_rx_verify_nth16(struct cdc_ncm_ctx *ctx, struct sk_buff *skb_in)
+int cdc_ncm_rx_verify_nth16(struct cdc_ncm_ctx *ctx, struct sk_buff *skb_in)
 {
 	struct usb_cdc_ncm_nth16 *nth16;
 	int len;
@@ -990,9 +913,10 @@ static int cdc_ncm_rx_verify_nth16(struct cdc_ncm_ctx *ctx, struct sk_buff *skb_
 error:
 	return ret;
 }
+EXPORT_SYMBOL_GPL(cdc_ncm_rx_verify_nth16);
 
 /* verify NDP header and return number of datagrams, or negative error */
-static int cdc_ncm_rx_verify_ndp16(struct sk_buff *skb_in, int ndpoffset)
+int cdc_ncm_rx_verify_ndp16(struct sk_buff *skb_in, int ndpoffset)
 {
 	struct usb_cdc_ncm_ndp16 *ndp16;
 	int ret = -EINVAL;
@@ -1023,6 +947,7 @@ static int cdc_ncm_rx_verify_ndp16(struct sk_buff *skb_in, int ndpoffset)
 error:
 	return ret;
 }
+EXPORT_SYMBOL_GPL(cdc_ncm_rx_verify_ndp16);
 
 static int cdc_ncm_rx_fixup(struct usbnet *dev, struct sk_buff *skb_in)
 {
diff --git a/include/linux/usb/cdc_ncm.h b/include/linux/usb/cdc_ncm.h
new file mode 100644
index 0000000..d1719a7
--- /dev/null
+++ b/include/linux/usb/cdc_ncm.h
@@ -0,0 +1,124 @@
+/*
+ * Copyright (C) ST-Ericsson 2010-2012
+ * Contact: Alexey Orishko <alexey.orishko-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org>
+ * Original author: Hans Petter Selasky <hans.petter.selasky@stericsson.com>
+ *
+ * USB Host Driver for Network Control Model (NCM)
+ * http://www.usb.org/developers/devclass_docs/NCM10.zip
+ *
+ * The NCM encoding, decoding and initialization logic
+ * derives from FreeBSD 8.x. if_cdce.c and if_cdcereg.h
+ *
+ * This software is available to you under a choice of one of two
+ * licenses. You may choose this file to be licensed under the terms
+ * of the GNU General Public License (GPL) Version 2 or the 2-clause
+ * BSD license listed below:
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED.  IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
+ * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
+ * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+ * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
+ * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+/* CDC NCM subclass 3.2.1 */
+#define USB_CDC_NCM_NDP16_LENGTH_MIN		0x10
+
+/* Maximum NTB length */
+#define	CDC_NCM_NTB_MAX_SIZE_TX			32768	/* bytes */
+#define	CDC_NCM_NTB_MAX_SIZE_RX			32768	/* bytes */
+
+/* Minimum value for MaxDatagramSize, ch. 6.2.9 */
+#define	CDC_NCM_MIN_DATAGRAM_SIZE		1514	/* bytes */
+
+/* Minimum value for MaxDatagramSize, ch. 8.1.3 */
+#define CDC_MBIM_MIN_DATAGRAM_SIZE		2048	/* bytes */
+
+#define	CDC_NCM_MIN_TX_PKT			512	/* bytes */
+
+/* Default value for MaxDatagramSize */
+#define	CDC_NCM_MAX_DATAGRAM_SIZE		8192	/* bytes */
+
+/*
+ * Maximum amount of datagrams in NCM Datagram Pointer Table, not counting
+ * the last NULL entry.
+ */
+#define	CDC_NCM_DPT_DATAGRAMS_MAX		40
+
+/* Restart the timer, if amount of datagrams is less than given value */
+#define	CDC_NCM_RESTART_TIMER_DATAGRAM_CNT	3
+#define	CDC_NCM_TIMER_PENDING_CNT		2
+#define CDC_NCM_TIMER_INTERVAL			(400UL * NSEC_PER_USEC)
+
+/* The following macro defines the minimum header space */
+#define	CDC_NCM_MIN_HDR_SIZE \
+	(sizeof(struct usb_cdc_ncm_nth16) + sizeof(struct usb_cdc_ncm_ndp16) + \
+	(CDC_NCM_DPT_DATAGRAMS_MAX + 1) * sizeof(struct usb_cdc_ncm_dpe16))
+
+#define CDC_NCM_NDP_SIZE \
+	(sizeof(struct usb_cdc_ncm_ndp16) +				\
+	      (CDC_NCM_DPT_DATAGRAMS_MAX + 1) * sizeof(struct usb_cdc_ncm_dpe16))
+
+struct cdc_ncm_ctx {
+	struct usb_cdc_ncm_ntb_parameters ncm_parm;
+	struct hrtimer tx_timer;
+	struct tasklet_struct bh;
+
+	const struct usb_cdc_ncm_desc *func_desc;
+	const struct usb_cdc_mbim_desc   *mbim_desc;
+	const struct usb_cdc_header_desc *header_desc;
+	const struct usb_cdc_union_desc *union_desc;
+	const struct usb_cdc_ether_desc *ether_desc;
+
+	struct net_device *netdev;
+	struct usb_device *udev;
+	struct usb_host_endpoint *in_ep;
+	struct usb_host_endpoint *out_ep;
+	struct usb_host_endpoint *status_ep;
+	struct usb_interface *intf;
+	struct usb_interface *control;
+	struct usb_interface *data;
+
+	struct sk_buff *tx_curr_skb;
+	struct sk_buff *tx_rem_skb;
+	__le32 tx_rem_sign;
+
+	spinlock_t mtx;
+	atomic_t stop;
+
+	u32 tx_timer_pending;
+	u32 tx_curr_frame_num;
+	u32 rx_speed;
+	u32 tx_speed;
+	u32 rx_max;
+	u32 tx_max;
+	u32 max_datagram_size;
+	u16 tx_max_datagrams;
+	u16 tx_remainder;
+	u16 tx_modulus;
+	u16 tx_ndp_modulus;
+	u16 tx_seq;
+	u16 rx_seq;
+	u16 connected;
+};
+
+extern int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8 data_altsetting);
+extern void cdc_ncm_unbind(struct usbnet *dev, struct usb_interface *intf);
+extern struct sk_buff *cdc_ncm_fill_tx_frame(struct cdc_ncm_ctx *ctx, struct sk_buff *skb, __le32 sign);
+extern int cdc_ncm_rx_verify_nth16(struct cdc_ncm_ctx *ctx, struct sk_buff *skb_in);
+extern int cdc_ncm_rx_verify_ndp16(struct sk_buff *skb_in, int ndpoffset);
-- 
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

[PATCH net-next 10/14] net: cdc_mbim: adding MBIM driver

[Bjørn Mork]

From: Greg Suarez <gsuarez-AKjrjAf1O7qe8kRwQpwjMg@public.gmane.org>

The CDC Mobile Broadband Interface Model (MBIM) specification
extends CDC NCM by
 - removing the redundant ethernet header from the point-to-point
   USB channel
 - adding support for multiple IP (v4 and/or v6) sessions multiplexed
   on the same USB channel
 - adding a MBIM control channel encapsulated in CDC
 - adding Device Service Streams (DSS), which are non IP generic data
   streams multiplexed on the same USB channel as the IP sessions

MBIM devices are managed using the dedicated control channel, and no
data will flow on the data channel until a control session has been
established.  This driver has no knowledge of MBIM control messages.
It just exports the control channel to a /dev/cdc-wdmX character
device for userspace management applications. Such an application is
therefore required to use this driver.

This patch implements basic MBIM support, reusing the NCM and WDM driver
APIs, currently limited to IP sessions with SessionID 0. DSS and
multiplexed IP sessions are not yet supported.

Signed-off-by: Greg Suarez <gsuarez-AKjrjAf1O7qe8kRwQpwjMg@public.gmane.org>
Signed-off-by: Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org>
---
 drivers/net/usb/cdc_mbim.c  |  365 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/usb/cdc_ncm.h |   10 ++
 2 files changed, 375 insertions(+)
 create mode 100644 drivers/net/usb/cdc_mbim.c

diff --git a/drivers/net/usb/cdc_mbim.c b/drivers/net/usb/cdc_mbim.c
new file mode 100644
index 0000000..2f21139
--- /dev/null
+++ b/drivers/net/usb/cdc_mbim.c
@@ -0,0 +1,365 @@
+/*
+ * Copyright (c) 2012  Smith Micro Software, Inc.
+ * Copyright (c) 2012  Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org>
+ *
+ * This driver is based on and reuse most of cdc_ncm, which is
+ * Copyright (C) ST-Ericsson 2010-2012
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/ethtool.h>
+#include <linux/ip.h>
+#include <linux/mii.h>
+#include <linux/usb.h>
+#include <linux/usb/cdc.h>
+#include <linux/usb/usbnet.h>
+#include <linux/usb/cdc-wdm.h>
+#include <linux/usb/cdc_ncm.h>
+
+/* driver specific data - must match cdc_ncm usage */
+struct cdc_mbim_state {
+	struct cdc_ncm_ctx *ctx;
+	atomic_t pmcount;
+	struct usb_driver *subdriver;
+	struct usb_interface *control;
+	struct usb_interface *data;
+};
+
+/* using a counter to merge subdriver requests with our own into a combined state */
+static int cdc_mbim_manage_power(struct usbnet *dev, int on)
+{
+	struct cdc_mbim_state *info = (void *)&dev->data;
+	int rv = 0;
+
+	dev_dbg(&dev->intf->dev, "%s() pmcount=%d, on=%d\n", __func__, atomic_read(&info->pmcount), on);
+
+	if ((on && atomic_add_return(1, &info->pmcount) == 1) || (!on && atomic_dec_and_test(&info->pmcount))) {
+		/* need autopm_get/put here to ensure the usbcore sees the new value */
+		rv = usb_autopm_get_interface(dev->intf);
+		if (rv < 0)
+			goto err;
+		dev->intf->needs_remote_wakeup = on;
+		usb_autopm_put_interface(dev->intf);
+	}
+err:
+	return rv;
+}
+
+static int cdc_mbim_wdm_manage_power(struct usb_interface *intf, int status)
+{
+	struct usbnet *dev = usb_get_intfdata(intf);
+
+	/* can be called while disconnecting */
+	if (!dev)
+		return 0;
+
+	return cdc_mbim_manage_power(dev, status);
+}
+
+
+static int cdc_mbim_bind(struct usbnet *dev, struct usb_interface *intf)
+{
+	struct cdc_ncm_ctx *ctx;
+	struct usb_driver *subdriver = ERR_PTR(-ENODEV);
+	int ret = -ENODEV;
+	u8 data_altsetting = CDC_NCM_DATA_ALTSETTING_NCM;
+	struct cdc_mbim_state *info = (void *)&dev->data;
+
+	/* see if interface supports MBIM alternate setting */
+	if (intf->num_altsetting == 2) {
+		if (!cdc_ncm_comm_intf_is_mbim(intf->cur_altsetting))
+			usb_set_interface(dev->udev,
+					  intf->cur_altsetting->desc.bInterfaceNumber,
+					  CDC_NCM_COMM_ALTSETTING_MBIM);
+		data_altsetting = CDC_NCM_DATA_ALTSETTING_MBIM;
+	}
+
+	/* Probably NCM, defer for cdc_ncm_bind */
+	if (!cdc_ncm_comm_intf_is_mbim(intf->cur_altsetting))
+		goto err;
+
+	ret = cdc_ncm_bind_common(dev, intf, data_altsetting);
+	if (ret)
+		goto err;
+
+	ctx = info->ctx;
+
+	/* The MBIM descriptor and the status endpoint are required */
+	if (ctx->mbim_desc && dev->status)
+		subdriver = usb_cdc_wdm_register(ctx->control,
+						 &dev->status->desc,
+						 le16_to_cpu(ctx->mbim_desc->wMaxControlMessage),
+						 cdc_mbim_wdm_manage_power);
+	if (IS_ERR(subdriver)) {
+		ret = PTR_ERR(subdriver);
+		cdc_ncm_unbind(dev, intf);
+		goto err;
+	}
+
+	/* can't let usbnet use the interrupt endpoint */
+	dev->status = NULL;
+	info->subdriver = subdriver;
+
+	/* MBIM cannot do ARP */
+	dev->net->flags |= IFF_NOARP;
+err:
+	return ret;
+}
+
+static void cdc_mbim_unbind(struct usbnet *dev, struct usb_interface *intf)
+{
+	struct cdc_mbim_state *info = (void *)&dev->data;
+	struct cdc_ncm_ctx *ctx = info->ctx;
+
+	/* disconnect subdriver from control interface */
+	if (info->subdriver && info->subdriver->disconnect)
+		info->subdriver->disconnect(ctx->control);
+	info->subdriver = NULL;
+
+	/* let NCM unbind clean up both control and data interface */
+	cdc_ncm_unbind(dev, intf);
+}
+
+
+static struct sk_buff *cdc_mbim_tx_fixup(struct usbnet *dev, struct sk_buff *skb, gfp_t flags)
+{
+	struct sk_buff *skb_out;
+	struct cdc_mbim_state *info = (void *)&dev->data;
+	struct cdc_ncm_ctx *ctx = info->ctx;
+
+	if (!ctx)
+		goto error;
+
+	if (skb) {
+		if (skb->len <= sizeof(ETH_HLEN))
+			goto error;
+
+		skb_reset_mac_header(skb);
+		switch (eth_hdr(skb)->h_proto) {
+		case htons(ETH_P_IP):
+		case htons(ETH_P_IPV6):
+			skb_pull(skb, ETH_HLEN);
+			break;
+		default:
+			goto error;
+		}
+	}
+
+	spin_lock_bh(&ctx->mtx);
+	skb_out = cdc_ncm_fill_tx_frame(ctx, skb, cpu_to_le32(USB_CDC_MBIM_NDP16_IPS_SIGN));
+	spin_unlock_bh(&ctx->mtx);
+	return skb_out;
+
+error:
+	if (skb)
+		dev_kfree_skb_any(skb);
+
+	return NULL;
+}
+
+static struct sk_buff *cdc_mbim_process_dgram(struct usbnet *dev, u8 *buf, size_t len)
+{
+	__be16 proto;
+	struct sk_buff *skb = NULL;
+
+	switch (*buf & 0xf0) {
+	case 0x40:
+		proto = htons(ETH_P_IP);
+		break;
+	case 0x60:
+		proto = htons(ETH_P_IPV6);
+		break;
+	default:
+		goto err;
+	}
+
+	skb = netdev_alloc_skb_ip_align(dev->net,  len + ETH_HLEN);
+	if (!skb)
+		goto err;
+
+	/* add an ethernet header */
+	skb_put(skb, ETH_HLEN);
+	skb_reset_mac_header(skb);
+	eth_hdr(skb)->h_proto = proto;
+	memset(eth_hdr(skb)->h_source, 0, ETH_ALEN);
+	memcpy(eth_hdr(skb)->h_dest, dev->net->dev_addr, ETH_ALEN);
+
+	/* add datagram */
+	memcpy(skb_put(skb, len), buf, len);
+err:
+	return skb;
+}
+
+static int cdc_mbim_rx_fixup(struct usbnet *dev, struct sk_buff *skb_in)
+{
+	struct sk_buff *skb;
+	struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
+	int len;
+	int nframes;
+	int x;
+	int offset;
+	struct usb_cdc_ncm_ndp16 *ndp16;
+	struct usb_cdc_ncm_dpe16 *dpe16;
+	int ndpoffset;
+	int loopcount = 50; /* arbitrary max preventing infinite loop */
+
+	ndpoffset = cdc_ncm_rx_verify_nth16(ctx, skb_in);
+	if (ndpoffset < 0)
+		goto error;
+
+next_ndp:
+	nframes = cdc_ncm_rx_verify_ndp16(skb_in, ndpoffset);
+	if (nframes < 0)
+		goto error;
+
+	ndp16 = (struct usb_cdc_ncm_ndp16 *)(skb_in->data + ndpoffset);
+
+	/* only supporting IPS Session #0 for now */
+	switch (ndp16->dwSignature) {
+	case cpu_to_le32(USB_CDC_MBIM_NDP16_IPS_SIGN):
+		break;
+	default:
+		netif_dbg(dev, rx_err, dev->net,
+			  "unsupported NDP signature <0x%08x>\n",
+			  le32_to_cpu(ndp16->dwSignature));
+		goto err_ndp;
+
+	}
+
+	dpe16 = ndp16->dpe16;
+	for (x = 0; x < nframes; x++, dpe16++) {
+		offset = le16_to_cpu(dpe16->wDatagramIndex);
+		len = le16_to_cpu(dpe16->wDatagramLength);
+
+		/*
+		 * CDC NCM ch. 3.7
+		 * All entries after first NULL entry are to be ignored
+		 */
+		if ((offset == 0) || (len == 0)) {
+			if (!x)
+				goto err_ndp; /* empty NTB */
+			break;
+		}
+
+		/* sanity checking */
+		if (((offset + len) > skb_in->len) || (len > ctx->rx_max) ||
+		    (len < sizeof(struct iphdr))) {
+			netif_dbg(dev, rx_err, dev->net,
+				  "invalid frame detected (ignored) offset[%u]=%u, length=%u, skb=%p\n",
+				  x, offset, len, skb_in);
+			if (!x)
+				goto err_ndp;
+			break;
+		} else {
+			skb = cdc_mbim_process_dgram(dev, skb_in->data + offset, len);
+			if (!skb)
+				goto error;
+			usbnet_skb_return(dev, skb);
+		}
+	}
+err_ndp:
+	/* are there more NDPs to process? */
+	ndpoffset = le16_to_cpu(ndp16->wNextNdpIndex);
+	if (ndpoffset && loopcount--)
+		goto next_ndp;
+
+	return 1;
+error:
+	return 0;
+}
+
+static int cdc_mbim_suspend(struct usb_interface *intf, pm_message_t message)
+{
+	int ret = 0;
+	struct usbnet *dev = usb_get_intfdata(intf);
+	struct cdc_mbim_state *info = (void *)&dev->data;
+	struct cdc_ncm_ctx *ctx = info->ctx;
+
+	if (ctx == NULL) {
+		ret = -1;
+		goto error;
+	}
+
+	ret = usbnet_suspend(intf, message);
+	if (ret < 0)
+		goto error;
+
+	if (intf == ctx->control && info->subdriver && info->subdriver->suspend)
+		ret = info->subdriver->suspend(intf, message);
+	if (ret < 0)
+		usbnet_resume(intf);
+
+error:
+	return ret;
+}
+
+static int cdc_mbim_resume(struct usb_interface *intf)
+{
+	int  ret = 0;
+	struct usbnet *dev = usb_get_intfdata(intf);
+	struct cdc_mbim_state *info = (void *)&dev->data;
+	struct cdc_ncm_ctx *ctx = info->ctx;
+	bool callsub = (intf == ctx->control && info->subdriver && info->subdriver->resume);
+
+	if (callsub)
+		ret = info->subdriver->resume(intf);
+	if (ret < 0)
+		goto err;
+	ret = usbnet_resume(intf);
+	if (ret < 0 && callsub && info->subdriver->suspend)
+		info->subdriver->suspend(intf, PMSG_SUSPEND);
+err:
+	return ret;
+}
+
+static const struct driver_info cdc_mbim_info = {
+	.description = "CDC MBIM",
+	.flags = FLAG_NO_SETINT | FLAG_MULTI_PACKET | FLAG_WWAN,
+	.bind = cdc_mbim_bind,
+	.unbind = cdc_mbim_unbind,
+	.manage_power = cdc_mbim_manage_power,
+	.rx_fixup = cdc_mbim_rx_fixup,
+	.tx_fixup = cdc_mbim_tx_fixup,
+};
+
+static const struct usb_device_id mbim_devs[] = {
+	/* This duplicate NCM entry is intentional. MBIM devices can
+	 * be disguised as NCM by default, and this is necessary to
+	 * allow us to bind the correct driver_info to such devices.
+	 *
+	 * bind() will sort out this for us, selecting the correct
+	 * entry and reject the other
+	 */
+	{ USB_INTERFACE_INFO(USB_CLASS_COMM, USB_CDC_SUBCLASS_NCM, USB_CDC_PROTO_NONE),
+	  .driver_info = (unsigned long)&cdc_mbim_info,
+	},
+	{ USB_INTERFACE_INFO(USB_CLASS_COMM, USB_CDC_SUBCLASS_MBIM, USB_CDC_PROTO_NONE),
+	  .driver_info = (unsigned long)&cdc_mbim_info,
+	},
+	{
+	},
+};
+MODULE_DEVICE_TABLE(usb, mbim_devs);
+
+static struct usb_driver cdc_mbim_driver = {
+	.name = "cdc_mbim",
+	.id_table = mbim_devs,
+	.probe = usbnet_probe,
+	.disconnect = usbnet_disconnect,
+	.suspend = cdc_mbim_suspend,
+	.resume = cdc_mbim_resume,
+	.reset_resume =	cdc_mbim_resume,
+	.supports_autosuspend = 1,
+	.disable_hub_initiated_lpm = 1,
+};
+module_usb_driver(cdc_mbim_driver);
+
+MODULE_AUTHOR("Greg Suarez <gsuarez-AKjrjAf1O7qe8kRwQpwjMg@public.gmane.org>");
+MODULE_AUTHOR("Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org>");
+MODULE_DESCRIPTION("USB CDC MBIM host driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/usb/cdc_ncm.h b/include/linux/usb/cdc_ncm.h
index d1719a7..3b8f9d4 100644
--- a/include/linux/usb/cdc_ncm.h
+++ b/include/linux/usb/cdc_ncm.h
@@ -36,6 +36,12 @@
  * SUCH DAMAGE.
  */
 
+#define CDC_NCM_COMM_ALTSETTING_NCM		0
+#define CDC_NCM_COMM_ALTSETTING_MBIM		1
+
+#define CDC_NCM_DATA_ALTSETTING_NCM		1
+#define CDC_NCM_DATA_ALTSETTING_MBIM		2
+
 /* CDC NCM subclass 3.2.1 */
 #define USB_CDC_NCM_NDP16_LENGTH_MIN		0x10
 
@@ -74,6 +80,10 @@
 	(sizeof(struct usb_cdc_ncm_ndp16) +				\
 	      (CDC_NCM_DPT_DATAGRAMS_MAX + 1) * sizeof(struct usb_cdc_ncm_dpe16))
 
+#define cdc_ncm_comm_intf_is_mbim(x)  ((x)->desc.bInterfaceSubClass == USB_CDC_SUBCLASS_MBIM && \
+				       (x)->desc.bInterfaceProtocol == USB_CDC_PROTO_NONE)
+#define cdc_ncm_data_intf_is_mbim(x)  ((x)->desc.bInterfaceProtocol == USB_CDC_MBIM_PROTO_NTB)
+
 struct cdc_ncm_ctx {
 	struct usb_cdc_ncm_ntb_parameters ncm_parm;
 	struct hrtimer tx_timer;
-- 
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

[PATCH net-next 11/14] net: cdc_mbim: build the MBIM driver

[Bjørn Mork]

Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
 drivers/net/usb/Kconfig  |   18 ++++++++++++++++++
 drivers/net/usb/Makefile |    1 +
 2 files changed, 19 insertions(+)

diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
index c1ae769..2ab8043 100644
--- a/drivers/net/usb/Kconfig
+++ b/drivers/net/usb/Kconfig
@@ -219,6 +219,24 @@ config USB_NET_CDC_NCM
 	    * ST-Ericsson M343 HSPA Mobile Broadband Modem (reference design)
 	    * Ericsson F5521gw Mobile Broadband Module
 
+config USB_NET_CDC_MBIM
+	tristate "CDC MBIM support"
+	depends on USB_USBNET
+	select USB_WDM
+	select USB_NET_CDC_NCM
+	help
+	  This driver provides support for CDC MBIM (Mobile Broadband
+	  Interface Model) devices. The CDC MBIM specification is
+	  available from <http://www.usb.org/>.
+
+	  MBIM devices require configuration using the management
+	  protocol defined by the MBIM specification.  This driver
+	  provides unfiltered access to the MBIM control channel
+	  through the associated /dev/cdc-wdmx character device.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called cdc_mbim.
+
 config USB_NET_DM9601
 	tristate "Davicom DM9601 based USB 1.1 10/100 ethernet devices"
 	depends on USB_USBNET
diff --git a/drivers/net/usb/Makefile b/drivers/net/usb/Makefile
index bf06300..4786913 100644
--- a/drivers/net/usb/Makefile
+++ b/drivers/net/usb/Makefile
@@ -31,4 +31,5 @@ obj-$(CONFIG_USB_NET_CX82310_ETH)	+= cx82310_eth.o
 obj-$(CONFIG_USB_NET_CDC_NCM)	+= cdc_ncm.o
 obj-$(CONFIG_USB_VL600)		+= lg-vl600.o
 obj-$(CONFIG_USB_NET_QMI_WWAN)	+= qmi_wwan.o
+obj-$(CONFIG_USB_NET_CDC_MBIM)	+= cdc_mbim.o
 
-- 
1.7.10.4

[PATCH net-next 12/14] net: cdc_ncm: do not bind to NCM compatible MBIM devices

[Bjørn Mork]

The MBIM specification allows a MBIM device to disguise
itself as NCM for backwards compatibility, using additional
altsettings with different subclass (control) or protocol
(data):

C:* #Ifs= 2 Cfg#= 1 Atr=80 MxPwr=100mA
I:  If#= 0 Alt= 0 #EPs= 1 Cls=02(comm.) Sub=0d Prot=00 Driver=cdc_mbim
E:  Ad=81(I) Atr=03(Int.) MxPS=  64 Ivl=7ms
I:* If#= 0 Alt= 1 #EPs= 1 Cls=02(comm.) Sub=0e Prot=00 Driver=cdc_mbim
E:  Ad=81(I) Atr=03(Int.) MxPS=  64 Ivl=7ms
I:  If#= 1 Alt= 0 #EPs= 0 Cls=0a(data ) Sub=00 Prot=01 Driver=cdc_mbim
I:  If#= 1 Alt= 1 #EPs= 2 Cls=0a(data ) Sub=00 Prot=01 Driver=cdc_mbim
E:  Ad=81(I) Atr=02(Bulk) MxPS=  64 Ivl=0ms
E:  Ad=02(O) Atr=02(Bulk) MxPS=  64 Ivl=0ms
I:* If#= 1 Alt= 2 #EPs= 2 Cls=0a(data ) Sub=00 Prot=02 Driver=cdc_mbim
E:  Ad=81(I) Atr=02(Bulk) MxPS=  64 Ivl=0ms
E:  Ad=02(O) Atr=02(Bulk) MxPS=  64 Ivl=0ms

If the MBIM driver is enabled then that should have priority
for devices providing such a NCM 1.0 backward compatibility
mode.

Signed-off-by: Bjørn Mork <bjorn@mork.no>
Signed-off-by: Greg Suarez <gsuarez@smithmicro.com>
---
 drivers/net/usb/cdc_ncm.c |   27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 0c941ef..a4d61a3 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -587,6 +587,33 @@ static int cdc_ncm_bind(struct usbnet *dev, struct usb_interface *intf)
 {
 	int ret;
 
+	/* The MBIM spec defines a NCM compatible default altsetting,
+	 * which we may have matched:
+	 *
+	 *  "Functions that implement both NCM 1.0 and MBIM (an
+	 *   “NCM/MBIM function”) according to this recommendation
+	 *   shall provide two alternate settings for the
+	 *   Communication Interface.  Alternate setting 0, and the
+	 *   associated class and endpoint descriptors, shall be
+	 *   constructed according to the rules given for the
+	 *   Communication Interface in section 5 of [USBNCM10].
+	 *   Alternate setting 1, and the associated class and
+	 *   endpoint descriptors, shall be constructed according to
+	 *   the rules given in section 6 (USB Device Model) of this
+	 *   specification."
+	 *
+	 * Do not bind to such interfaces, allowing cdc_mbim to handle
+	 * them
+	 */
+#if IS_ENABLED(CONFIG_USB_NET_CDC_MBIM)
+	if ((intf->num_altsetting == 2) &&
+	    !usb_set_interface(dev->udev,
+			       intf->cur_altsetting->desc.bInterfaceNumber,
+			       CDC_NCM_COMM_ALTSETTING_MBIM) &&
+	    cdc_ncm_comm_intf_is_mbim(intf->cur_altsetting))
+		return -ENODEV;
+#endif
+
 	/* NCM data altsetting is always 1 */
 	ret = cdc_ncm_bind_common(dev, intf, 1);
 
-- 
1.7.10.4

[PATCH net-next 13/14] net: cdc_ncm: map MBIM IPS SessionID to VLAN ID

[Bjørn Mork]

MBIM devices can support up to 256 independent IP Streams.
The main network device will only handle SessionID 0. Mapping
SessionIDs 1 to 255 to VLANs using the SessionID as VLAN ID
allow userspace to use these streams with traditional tools
like vconfig.

Signed-off-by: Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org>
---
 drivers/net/usb/cdc_mbim.c |   51 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 43 insertions(+), 8 deletions(-)

diff --git a/drivers/net/usb/cdc_mbim.c b/drivers/net/usb/cdc_mbim.c
index 2f21139..45f5f50 100644
--- a/drivers/net/usb/cdc_mbim.c
+++ b/drivers/net/usb/cdc_mbim.c
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <linux/netdevice.h>
 #include <linux/ethtool.h>
+#include <linux/if_vlan.h>
 #include <linux/ip.h>
 #include <linux/mii.h>
 #include <linux/usb.h>
@@ -107,6 +108,9 @@ static int cdc_mbim_bind(struct usbnet *dev, struct usb_interface *intf)
 
 	/* MBIM cannot do ARP */
 	dev->net->flags |= IFF_NOARP;
+
+	/* no need to put the VLAN tci in the packet headers */
+	dev->net->features |= NETIF_F_HW_VLAN_TX;
 err:
 	return ret;
 }
@@ -131,6 +135,9 @@ static struct sk_buff *cdc_mbim_tx_fixup(struct usbnet *dev, struct sk_buff *skb
 	struct sk_buff *skb_out;
 	struct cdc_mbim_state *info = (void *)&dev->data;
 	struct cdc_ncm_ctx *ctx = info->ctx;
+	__le32 sign = cpu_to_le32(USB_CDC_MBIM_NDP16_IPS_SIGN);
+	u16 tci = 0;
+	u8 *c;
 
 	if (!ctx)
 		goto error;
@@ -139,6 +146,24 @@ static struct sk_buff *cdc_mbim_tx_fixup(struct usbnet *dev, struct sk_buff *skb
 		if (skb->len <= sizeof(ETH_HLEN))
 			goto error;
 
+		/* mapping VLANs to MBIM sessions:
+		 *   no tag     => IPS session <0>
+		 *   1 - 255    => IPS session <vlanid>
+		 *   256 - 4095 => unsupported, drop
+		 */
+		vlan_get_tag(skb, &tci);
+
+		switch (tci & 0x0f00) {
+		case 0x0000: /* VLAN ID 0 - 255 */
+			c = (u8 *)&sign;
+			c[3] = tci;
+			break;
+		default:
+			netif_err(dev, tx_err, dev->net,
+				  "unsupported tci=0x%04x\n", tci);
+			goto error;
+		}
+
 		skb_reset_mac_header(skb);
 		switch (eth_hdr(skb)->h_proto) {
 		case htons(ETH_P_IP):
@@ -151,7 +176,7 @@ static struct sk_buff *cdc_mbim_tx_fixup(struct usbnet *dev, struct sk_buff *skb
 	}
 
 	spin_lock_bh(&ctx->mtx);
-	skb_out = cdc_ncm_fill_tx_frame(ctx, skb, cpu_to_le32(USB_CDC_MBIM_NDP16_IPS_SIGN));
+	skb_out = cdc_ncm_fill_tx_frame(ctx, skb, sign);
 	spin_unlock_bh(&ctx->mtx);
 	return skb_out;
 
@@ -162,11 +187,14 @@ error:
 	return NULL;
 }
 
-static struct sk_buff *cdc_mbim_process_dgram(struct usbnet *dev, u8 *buf, size_t len)
+static struct sk_buff *cdc_mbim_process_dgram(struct usbnet *dev, u8 *buf, size_t len, u16 tci)
 {
 	__be16 proto;
 	struct sk_buff *skb = NULL;
 
+	if (len < sizeof(struct iphdr))
+		goto err;
+
 	switch (*buf & 0xf0) {
 	case 0x40:
 		proto = htons(ETH_P_IP);
@@ -191,6 +219,10 @@ static struct sk_buff *cdc_mbim_process_dgram(struct usbnet *dev, u8 *buf, size_
 
 	/* add datagram */
 	memcpy(skb_put(skb, len), buf, len);
+
+	/* map MBIM session to VLAN */
+	if (tci)
+		vlan_put_tag(skb, tci);
 err:
 	return skb;
 }
@@ -198,7 +230,8 @@ err:
 static int cdc_mbim_rx_fixup(struct usbnet *dev, struct sk_buff *skb_in)
 {
 	struct sk_buff *skb;
-	struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
+	struct cdc_mbim_state *info = (void *)&dev->data;
+	struct cdc_ncm_ctx *ctx = info->ctx;
 	int len;
 	int nframes;
 	int x;
@@ -207,6 +240,8 @@ static int cdc_mbim_rx_fixup(struct usbnet *dev, struct sk_buff *skb_in)
 	struct usb_cdc_ncm_dpe16 *dpe16;
 	int ndpoffset;
 	int loopcount = 50; /* arbitrary max preventing infinite loop */
+	u8 *c;
+	u16 tci;
 
 	ndpoffset = cdc_ncm_rx_verify_nth16(ctx, skb_in);
 	if (ndpoffset < 0)
@@ -219,9 +254,10 @@ next_ndp:
 
 	ndp16 = (struct usb_cdc_ncm_ndp16 *)(skb_in->data + ndpoffset);
 
-	/* only supporting IPS Session #0 for now */
-	switch (ndp16->dwSignature) {
+	switch (ndp16->dwSignature & cpu_to_le32(0x00ffffff)) {
 	case cpu_to_le32(USB_CDC_MBIM_NDP16_IPS_SIGN):
+		c = (u8 *)&ndp16->dwSignature;
+		tci = c[3];
 		break;
 	default:
 		netif_dbg(dev, rx_err, dev->net,
@@ -247,8 +283,7 @@ next_ndp:
 		}
 
 		/* sanity checking */
-		if (((offset + len) > skb_in->len) || (len > ctx->rx_max) ||
-		    (len < sizeof(struct iphdr))) {
+		if (((offset + len) > skb_in->len) || (len > ctx->rx_max)) {
 			netif_dbg(dev, rx_err, dev->net,
 				  "invalid frame detected (ignored) offset[%u]=%u, length=%u, skb=%p\n",
 				  x, offset, len, skb_in);
@@ -256,7 +291,7 @@ next_ndp:
 				goto err_ndp;
 			break;
 		} else {
-			skb = cdc_mbim_process_dgram(dev, skb_in->data + offset, len);
+			skb = cdc_mbim_process_dgram(dev, skb_in->data + offset, len, tci);
 			if (!skb)
 				goto error;
 			usbnet_skb_return(dev, skb);
-- 
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

[PATCH net-next 14/14] net: cdc_mbim: Device Service Stream support

[Bjørn Mork]

MBIM devices can support up to 256 generic streams called
Device Service Streams (DSS). The MBIM spec says

   The format of the Device Service Stream payload depends
   on the device service (as identified by the corresponding
   UUID) that is used when opening the data stream.

Example use cases are serial AT command interfaces and NMEA
data streams. We cannot make any assumptions about these
device services.

Adding support for Device Service Stream by extending
the MBIM session to VLAN mapping scheme, allocating
VLAN IDs 256 to 511 for DSS, using the DSS SessionID
as the lower 8bit of the VLAN ID.

Using a netdev for DSS keeps the device framing intact and
allows userspace to do whatever it want with the streams.
For example, exporting an AT command interface using DSS
session #0 to a PTY for use with a terminal application like
minicom:

  vconfig add wwan0 256
  ip link set dev wwan0 up
  ip link set dev wwan0.256 up
  socat INTERFACE:wwan0.256,type=2 PTY:,echo=0,link=/tmp/modem

Device configuration must be done using MBIM control commands
over the /dev/cdc-wdmx device. The userspace management
application should coordinate host VLAN configuration and the
device MBIM configuration using the device capabilities to
find out if it needs to set up PTY mappings etc.

Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
 drivers/net/usb/cdc_mbim.c |   58 ++++++++++++++++++++++++++------------------
 1 file changed, 35 insertions(+), 23 deletions(-)

diff --git a/drivers/net/usb/cdc_mbim.c b/drivers/net/usb/cdc_mbim.c
index 45f5f50..42f51c7 100644
--- a/drivers/net/usb/cdc_mbim.c
+++ b/drivers/net/usb/cdc_mbim.c
@@ -149,12 +149,27 @@ static struct sk_buff *cdc_mbim_tx_fixup(struct usbnet *dev, struct sk_buff *skb
 		/* mapping VLANs to MBIM sessions:
 		 *   no tag     => IPS session <0>
 		 *   1 - 255    => IPS session <vlanid>
-		 *   256 - 4095 => unsupported, drop
+		 *   256 - 511  => DSS session <vlanid - 256>
+		 *   512 - 4095 => unsupported, drop
 		 */
 		vlan_get_tag(skb, &tci);
 
 		switch (tci & 0x0f00) {
 		case 0x0000: /* VLAN ID 0 - 255 */
+			/* verify that datagram is IPv4 or IPv6 */
+			skb_reset_mac_header(skb);
+			switch (eth_hdr(skb)->h_proto) {
+			case htons(ETH_P_IP):
+			case htons(ETH_P_IPV6):
+				break;
+			default:
+				goto error;
+			}
+			c = (u8 *)&sign;
+			c[3] = tci;
+			break;
+		case 0x0100: /* VLAN ID 256 - 511 */
+			sign = cpu_to_le32(USB_CDC_MBIM_NDP16_DSS_SIGN);
 			c = (u8 *)&sign;
 			c[3] = tci;
 			break;
@@ -163,16 +178,7 @@ static struct sk_buff *cdc_mbim_tx_fixup(struct usbnet *dev, struct sk_buff *skb
 				  "unsupported tci=0x%04x\n", tci);
 			goto error;
 		}
-
-		skb_reset_mac_header(skb);
-		switch (eth_hdr(skb)->h_proto) {
-		case htons(ETH_P_IP):
-		case htons(ETH_P_IPV6):
-			skb_pull(skb, ETH_HLEN);
-			break;
-		default:
-			goto error;
-		}
+		skb_pull(skb, ETH_HLEN);
 	}
 
 	spin_lock_bh(&ctx->mtx);
@@ -189,21 +195,23 @@ error:
 
 static struct sk_buff *cdc_mbim_process_dgram(struct usbnet *dev, u8 *buf, size_t len, u16 tci)
 {
-	__be16 proto;
+	__be16 proto = htons(ETH_P_802_3);
 	struct sk_buff *skb = NULL;
 
-	if (len < sizeof(struct iphdr))
-		goto err;
+	if (tci < 256) { /* IPS session? */
+		if (len < sizeof(struct iphdr))
+			goto err;
 
-	switch (*buf & 0xf0) {
-	case 0x40:
-		proto = htons(ETH_P_IP);
-		break;
-	case 0x60:
-		proto = htons(ETH_P_IPV6);
-		break;
-	default:
-		goto err;
+		switch (*buf & 0xf0) {
+		case 0x40:
+			proto = htons(ETH_P_IP);
+			break;
+		case 0x60:
+			proto = htons(ETH_P_IPV6);
+			break;
+		default:
+			goto err;
+		}
 	}
 
 	skb = netdev_alloc_skb_ip_align(dev->net,  len + ETH_HLEN);
@@ -259,6 +267,10 @@ next_ndp:
 		c = (u8 *)&ndp16->dwSignature;
 		tci = c[3];
 		break;
+	case cpu_to_le32(USB_CDC_MBIM_NDP16_DSS_SIGN):
+		c = (u8 *)&ndp16->dwSignature;
+		tci = c[3] + 256;
+		break;
 	default:
 		netif_dbg(dev, rx_err, dev->net,
 			  "unsupported NDP signature <0x%08x>\n",
-- 
1.7.10.4

Re: [PATCH net-next 00/14] Adding a USB CDC MBIM driver

[David Miller]

I didn't see patch #14, where is it?
--
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-next 00/14] Adding a USB CDC MBIM driver

[Bjørn Mork]

David Miller <davem@davemloft.net> writes:

> I didn't see patch #14, where is it?

It took an extra round in my mail queue due to a problem with one of the
receiving servers. Sorry about that. It will show up as soon as my mail
server decides to retry.


Bjørn

Re: [PATCH net-next 03/14] USB: cdc: add MBIM constants and structures

[Greg Kroah-Hartman]

On Thu, Oct 18, 2012 at 10:40:56PM +0200, Bjørn Mork wrote:
> From: Greg Suarez <gsuarez-AKjrjAf1O7qe8kRwQpwjMg@public.gmane.org>
> 
> Based on revision 1.0 of "Universal Serial Bus Communications
> Class Subclass Specification for Mobile Broadband Interface
> Model" available from www.usb.org
> 
> Signed-off-by: Greg Suarez <gsuarez-AKjrjAf1O7qe8kRwQpwjMg@public.gmane.org>
> [bmork: added DSS defines]
> Signed-off-by: Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org>
> ---

Acked-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
--
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-next 00/14] Adding a USB CDC MBIM driver

[Greg Kroah-Hartman]

On Thu, Oct 18, 2012 at 10:40:53PM +0200, Bjørn Mork wrote:
> The USB Communications Device Class "Mobile Broadband Interface Model"
> (MBIM) is the USB-IFs alternative to the current chipset/vendor
> specific solutions to "Mobile Broadband" device management. The
> specification, including the management protocol description, can be
> downloaded from http://www.usb.org/developers/devclass_docs#approved
> 
> This driver implementing most MBIM features with the exception of
> 32bit NTB and NDP headers.
> 
> An important design principle has been reusing as much as possible of
> existing kernel code, in particular the cdc_ncm and cdc_wdm drivers.
> The CDC MBIM protocol is based on CDC NCM, and much of the setup and
> framing logic is very similar.
> 
> One important addition in MBIM compared to NCM is the new control
> protocol embedded in CDC commands.  This protocol is comprehensive and
> support a number of policy decisions necessary for modern mobile
> broadband devices often having multiple radio interfaces.  Based on
> early comments and the experiences with the qmi_wwan driver, knowledge
> of the control protocol has been kept completely out of the driver.
> This is userspace material.  Like with qmi_wwan, a control protocol
> interface is exported to userspace using the cdc_wdm subdriver API,
> associating a /dev/cdc-wdmX character device with the network device
> for the management application.
> 
> Patch 1 and 2 are independent of the rest and only required to make
> test devices with very large buffers work.
> 
> Patch 3 adds new MBIM definitions to the cdc.h header file
> 
> Patches 4 to 9 refactor the cdc_ncm driver to enable reusing common
> parts for MBIM.
> 
> Patches 10 and 11 add the new cdc_mbim driver
> 
> Patch 12 prevents cdc_ncm from binding to backward compatible MBIM
> devices
> 
> Patches 13 and 14 extend the MBIM driver to support multiplexed
> sessions
> 
> The changes to the cdc_ncm driver has been tested and verified to work
> with an Ericsson F5521gw device.  The new cdc_mbim driver has been
> tested with a Huawei E367u-2 device with MBIM firmware, and other
> currently undisclosed devices.

Very nice work, I don't have any objections to any of this, but I just
did a very high-level code review:

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH net-next 02/14] net: cdc_ncm: use device rx_max value if update failed

[Oliver Neukum]

On Thursday 18 October 2012 22:40:55 Bjørn Mork wrote:
> If the device refuses our updated value, then we must be prepared
> to receive URBs as big as the device wants to send.  Set rx_max
> to the device provided value on error.

Problematic in principle. How do you allocate a buffer of arbitrary size?

	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-next 02/14] net: cdc_ncm: use device rx_max value if update failed

[Bjørn Mork]

Oliver Neukum <oliver@neukum.org> writes:
> On Thursday 18 October 2012 22:40:55 Bjørn Mork wrote:
>> If the device refuses our updated value, then we must be prepared
>> to receive URBs as big as the device wants to send.  Set rx_max
>> to the device provided value on error.
>
> Problematic in principle. How do you allocate a buffer of arbitrary size?

You cannot of course.  You can only try and give up if it doesn't work.
rx_submit would end up returning -ENOMEM, but we are not always checking
that so it will most likely fail silently.

But I don't think we can just continue with the smaller buffer size
without having the device agree to that either.  That is also likely to
fail silently.  Note that this patch was added exactly because one of
the MBIM test devices did refuse the lower rx_max we tried to enforce.
The device insists on using 128kB buffers.

Maybe we should cap it at some arbitrary reasonable value, and just bail
out from bind if the device insists on a larger buffer?  Would that be
OK?  How big buffers are (semi-)reasonable?


Bjørn

Re: [PATCH net-next 02/14] net: cdc_ncm: use device rx_max value if update failed

[Alexey Orishko]

On Fri, Oct 19, 2012 at 12:09 AM, Bjørn Mork <bjorn@mork.no> wrote:
> Oliver Neukum <oliver@neukum.org> writes:
>> On Thursday 18 October 2012 22:40:55 Bjørn Mork wrote:
>>> If the device refuses our updated value, then we must be prepared
>>> to receive URBs as big as the device wants to send.  Set rx_max
>>> to the device provided value on error.
>>
>> Problematic in principle. How do you allocate a buffer of arbitrary size?
>
> You cannot of course.  You can only try and give up if it doesn't work.
> rx_submit would end up returning -ENOMEM, but we are not always checking
> that so it will most likely fail silently.
>
> But I don't think we can just continue with the smaller buffer size
> without having the device agree to that either.  That is also likely to
> fail silently.  Note that this patch was added exactly because one of
> the MBIM test devices did refuse the lower rx_max we tried to enforce.
> The device insists on using 128kB buffers.
>
> Maybe we should cap it at some arbitrary reasonable value, and just bail
> out from bind if the device insists on a larger buffer?  Would that be
> OK?  How big buffers are (semi-)reasonable?
>

I recommend to drop this.Vendor has to fix firmware.
Current version of the driver supports 16-bit NTB, which means you can address
(64K only - NTB header). So, how do you plan to use 64K-128K buffer space,
if it can't be addressed by 16 bit offset?
Another angle to big buffers, even while using 64K buffers your TCP connection
will suffer, so what's the point making huge buffers?

/alexey

Re: [PATCH net-next 02/14] net: cdc_ncm: use device rx_max value if update failed

[Bjørn Mork]

Alexey Orishko <alexey.orishko@gmail.com> writes:
> On Fri, Oct 19, 2012 at 12:09 AM, Bjørn Mork <bjorn@mork.no> wrote:
>> Oliver Neukum <oliver@neukum.org> writes:
>>> On Thursday 18 October 2012 22:40:55 Bjørn Mork wrote:
>>>> If the device refuses our updated value, then we must be prepared
>>>> to receive URBs as big as the device wants to send.  Set rx_max
>>>> to the device provided value on error.
>>>
>>> Problematic in principle. How do you allocate a buffer of arbitrary size?
>>
>> You cannot of course.  You can only try and give up if it doesn't work.
>> rx_submit would end up returning -ENOMEM, but we are not always checking
>> that so it will most likely fail silently.
>>
>> But I don't think we can just continue with the smaller buffer size
>> without having the device agree to that either.  That is also likely to
>> fail silently.  Note that this patch was added exactly because one of
>> the MBIM test devices did refuse the lower rx_max we tried to enforce.
>> The device insists on using 128kB buffers.
>>
>> Maybe we should cap it at some arbitrary reasonable value, and just bail
>> out from bind if the device insists on a larger buffer?  Would that be
>> OK?  How big buffers are (semi-)reasonable?
>>
>
> I recommend to drop this.

OK, will drop patch 01 (only necessary if some usbnet minidriver uses
buffers > 60 * 1518) and 02 from the next version of this series.

> Vendor has to fix firmware.

I agree in principle, and I'll report the problem to them.  But as usual
I believe we have to support any weird firmware we encounter, if at all
possible.

> Current version of the driver supports 16-bit NTB, which means you can address
> (64K only - NTB header). So, how do you plan to use 64K-128K buffer space,
> if it can't be addressed by 16 bit offset?

This is of course true.  The device does obey the 16bit header choice,
so I would hope that it does not send us buffers it can't use.  But it
does send buffers in the range 32K-64K, which makes the current driver
fail in a rather ugly way.

I assume the current CDC_NCM_NTB_MAX_SIZE_RX is set to 32K as a sane
default, but how about allowing up to 64K for devices which does not
accept this?  The other options are
 - silently failing, or
 - refusing to load with an error the user cannot do anything with,
and I don't think either of those are wanted, even if the NCM spec is
quite clear that the device is wrong here.

> Another angle to big buffers, even while using 64K buffers your TCP connection
> will suffer, so what's the point making huge buffers?

Agreed.  There is no point.  It's bloat.  Which makes you kind of wonder
why they bothered to define a separate 32bit header format at all,
complicating the protocol quite a lot...  Or why those writing the MBIM
spec didn't take their opportunity to remove this useless complication?
I am not holding it against you though ;-)

A nice side effect of the refactoring done to support MBIM is that most
of the 16bit header parsing has been isolated in separate functions,
making it trivial to implement the missing 32bit header support.  Maybe
we should do that?



Bjørn

Re: [PATCH net-next 02/14] net: cdc_ncm: use device rx_max value if update failed

[Bjørn Mork]

Bjørn Mork <bjorn@mork.no> writes:
> Alexey Orishko <alexey.orishko@gmail.com> writes:
>
>> Vendor has to fix firmware.
>
> I agree in principle, and I'll report the problem to them.  But as usual
> I believe we have to support any weird firmware we encounter, if at all
> possible.

OK, I did some more experiments, and I am wondering if the real firmware
problem is in the MBIM descriptor.  It is

      CDC MBIM:
        bcdMBIMVersion       1.00
        wMaxControlMessage   1536
        bNumberFilters       16
        bMaxFilterSize       40
        wMaxSegmentSize      4096
        bmNetworkCapabilities 0x20
          8-byte ntb input size


so we use the 8-byte version of USB_CDC_SET_NTB_INPUT_SIZE, which fails
with -EPIPE.  But forcing the 4-byte version seems to work. Hmm, I also
see that the device returns 4 bytes in response to at
USB_CDC_GET_NTB_INPUT_SIZE with an 8-byte buffer.  Maybe we can
auto-quirk based on that?  I.e., if USB_CDC_GET_NTB_INPUT_SIZE returns
only 4 bytes then we assume that the bmNetworkCapabilities flag is
wrong.

Is that acceptable?  Then it seems we are able to inform this device of
the reduced buffer, and the other problems can be ignored.


Bjørn

Re: [PATCH net-next 02/14] net: cdc_ncm: use device rx_max value if update failed

[Alexey Orishko]

On Fri, Oct 19, 2012 at 11:30 AM, Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org> wrote:
> Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org> writes:
>> Alexey Orishko <alexey.orishko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
>>
> OK, I did some more experiments, and I am wondering if the real firmware
> problem is in the MBIM descriptor.  It is
>
>       CDC MBIM:
>         bcdMBIMVersion       1.00
>         wMaxControlMessage   1536
>         bNumberFilters       16
>         bMaxFilterSize       40
>         wMaxSegmentSize      4096
>         bmNetworkCapabilities 0x20
>           8-byte ntb input size
>
>
> so we use the 8-byte version of USB_CDC_SET_NTB_INPUT_SIZE, which fails
> with -EPIPE.  But forcing the 4-byte version seems to work. Hmm, I also
> see that the device returns 4 bytes in response to at
> USB_CDC_GET_NTB_INPUT_SIZE with an 8-byte buffer.  Maybe we can
> auto-quirk based on that?  I.e., if USB_CDC_GET_NTB_INPUT_SIZE returns
> only 4 bytes then we assume that the bmNetworkCapabilities flag is
> wrong.
>
> Is that acceptable?  Then it seems we are able to inform this device of
> the reduced buffer, and the other problems can be ignored.
>

Based on NCM errata, NCM functional descriptor: if Bit 5 is set, then
device can (must) handle 8-byte form of Set/GetNtbInputSize.
If they set a flag, but don't support the feature, hm.. is it a
prototype device or
commercially available one?

At least device must support Set request, but for GetNtbInputSize we
could happily
live without wNtbInMaxDatagrams (i.e. use 4 byte variant) on Linux.
Since we must anyway receive a complete NTB and making any number of skb
buffers from received NTB is not a problem at all.

regards,
alexey
--
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-next 02/14] net: cdc_ncm: use device rx_max value if update failed

[Bjørn Mork]

Alexey Orishko <alexey.orishko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
> On Fri, Oct 19, 2012 at 11:30 AM, Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org> wrote:
>> Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org> writes:
>>> Alexey Orishko <alexey.orishko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
>>>
>> OK, I did some more experiments, and I am wondering if the real firmware
>> problem is in the MBIM descriptor.  It is
>>
>>       CDC MBIM:
>>         bcdMBIMVersion       1.00
>>         wMaxControlMessage   1536
>>         bNumberFilters       16
>>         bMaxFilterSize       40
>>         wMaxSegmentSize      4096
>>         bmNetworkCapabilities 0x20
>>           8-byte ntb input size
>>
>>
>> so we use the 8-byte version of USB_CDC_SET_NTB_INPUT_SIZE, which fails
>> with -EPIPE.  But forcing the 4-byte version seems to work. Hmm, I also
>> see that the device returns 4 bytes in response to at
>> USB_CDC_GET_NTB_INPUT_SIZE with an 8-byte buffer.  Maybe we can
>> auto-quirk based on that?  I.e., if USB_CDC_GET_NTB_INPUT_SIZE returns
>> only 4 bytes then we assume that the bmNetworkCapabilities flag is
>> wrong.
>>
>> Is that acceptable?  Then it seems we are able to inform this device of
>> the reduced buffer, and the other problems can be ignored.
>>
>
> Based on NCM errata, NCM functional descriptor: if Bit 5 is set, then
> device can (must) handle 8-byte form of Set/GetNtbInputSize.

Yes, and this flag is copied with the same requirements in MBIM.  So it
is definitely a firmware bug.

> If they set a flag, but don't support the feature, hm.. is it a
> prototype device or
> commercially available one?

The firmware is not commercially availabe AFAIK, but based on experience
I believe we should assume that any firmware bug ever seen will live
forever.  There are likely other devices with the same bug even if this
firmware and this device, and all other devices from the same vendor,
are fixed.

I believe the problem here is that the USB descriptors are somewhat
decoupled from the firmware functions.  The firmware functions are
usually implemented in firmware originating from the chipset vendor,
while the descriptors are up to the device vendor. This has led to
interesting situations before.  But for us, I believe it means that we
should put more trust in the responses to control messages than in
functional descriptors.  So if we can detect a mismatch like this one,
then we do that and use the control message.

> At least device must support Set request, but for GetNtbInputSize we
> could happily
> live without wNtbInMaxDatagrams (i.e. use 4 byte variant) on Linux.
> Since we must anyway receive a complete NTB and making any number of skb
> buffers from received NTB is not a problem at all.

Yes, it doesn't matter to us whether we get the 8 byte variant or
not. We are prepared to handle the 4 byte variant in any case, if the
functional descriptor flag is not set.  So I believe a fallback to 4
byte should not pose any problem.  The only difference will be that we
need to do the USB_CDC_GET_NTB_INPUT_SIZE to detect the bug in the first
place.

I'll cook up an alternative patch implementing this so you can evaluate
the impact.


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

[RFC] net: cdc_ncm: workaround NTB input size firmware bug

[Bjørn Mork]

Some devices do not support the 8 byte variants of
the NTB input size control messages despite announcing
such support in the functional descriptor. This can be
detected by reading the current input size, looking
at how many bytes the device returns.

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

> I'll cook up an alternative patch implementing this so you can evaluate
> the impact.

So how about something like this?

I couldn't help myself not combining the two paths here while at it.
But I have intentionally not done the other obvious cleanups, like using
the standard timeout constants and using dev_xxx for instead of pr_xxx,
because they must and should be done separately over the whole driver.

This works for me:

Oct 19 13:19:43 nemi kernel: [304987.350138] dwNtbInMaxSize=131072 dwNtbOutMaxSize=32768 wNdpOutPayloadRemainder=0 wNdpOutDivisor=4 wNdpOutAlignment=4 wNtbOutMaxDatagrams=0 flags=0x20
Oct 19 13:19:43 nemi kernel: [304987.350144] Using default maximum receive length=32768
Oct 19 13:19:43 nemi kernel: [304987.350507] firmware bug: flags=0x20, but USB_CDC_GET_NTB_INPUT_SIZE returned 4 bytes


Bjørn

 drivers/net/usb/cdc_ncm.c |   64 +++++++++++++++++++++++----------------------
 1 file changed, 33 insertions(+), 31 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 4cd582a..26d31d7 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -215,44 +215,46 @@ static u8 cdc_ncm_setup(struct cdc_ncm_ctx *ctx)
 
 	/* inform device about NTB input size changes */
 	if (ctx->rx_max != le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize)) {
+		struct usb_cdc_ncm_ndp_input_size *ndp_in_sz;
+		size_t request_len = 4;
 
+		ndp_in_sz = kzalloc(sizeof(*ndp_in_sz), GFP_KERNEL);
+		if (!ndp_in_sz) {
+			err = -ENOMEM;
+			goto size_err;
+		}
 		if (flags & USB_CDC_NCM_NCAP_NTB_INPUT_SIZE) {
-			struct usb_cdc_ncm_ndp_input_size *ndp_in_sz;
-
-			ndp_in_sz = kzalloc(sizeof(*ndp_in_sz), GFP_KERNEL);
-			if (!ndp_in_sz) {
-				err = -ENOMEM;
-				goto size_err;
-			}
-
 			err = usb_control_msg(ctx->udev,
-					usb_sndctrlpipe(ctx->udev, 0),
-					USB_CDC_SET_NTB_INPUT_SIZE,
-					USB_TYPE_CLASS | USB_DIR_OUT
-					 | USB_RECIP_INTERFACE,
-					0, iface_no, ndp_in_sz, 8, 1000);
-			kfree(ndp_in_sz);
-		} else {
-			__le32 *dwNtbInMaxSize;
-			dwNtbInMaxSize = kzalloc(sizeof(*dwNtbInMaxSize),
-					GFP_KERNEL);
-			if (!dwNtbInMaxSize) {
-				err = -ENOMEM;
-				goto size_err;
+					      usb_rcvctrlpipe(ctx->udev, 0),
+					      USB_CDC_GET_NTB_INPUT_SIZE,
+					      USB_TYPE_CLASS | USB_DIR_OUT
+					      | USB_RECIP_INTERFACE,
+					      0, iface_no, ndp_in_sz, 8, 1000);
+			switch (err) {
+			case 4:
+				pr_debug("firmware bug: flags=0x%02x, but USB_CDC_GET_NTB_INPUT_SIZE returned 4 bytes\n", flags);
+				break;
+			case 8:
+				/* bmNetworkCapabilities is correct */
+				request_len = 8;
+				break;
+			default:
+				goto size_err_free;
 			}
-			*dwNtbInMaxSize = cpu_to_le32(ctx->rx_max);
-
-			err = usb_control_msg(ctx->udev,
-					usb_sndctrlpipe(ctx->udev, 0),
-					USB_CDC_SET_NTB_INPUT_SIZE,
-					USB_TYPE_CLASS | USB_DIR_OUT
-					 | USB_RECIP_INTERFACE,
-					0, iface_no, dwNtbInMaxSize, 4, 1000);
-			kfree(dwNtbInMaxSize);
 		}
+		ndp_in_sz->dwNtbInMaxSize = cpu_to_le32(ctx->rx_max);
+		ndp_in_sz->wNtbInMaxDatagrams = 0;	/* unlimited */
+		err = usb_control_msg(ctx->udev,
+				      usb_sndctrlpipe(ctx->udev, 0),
+				      USB_CDC_SET_NTB_INPUT_SIZE,
+				      USB_TYPE_CLASS | USB_DIR_OUT
+				      | USB_RECIP_INTERFACE,
+				      0, iface_no, ndp_in_sz, request_len, 1000);
+size_err_free:
+		kfree(ndp_in_sz);
 size_err:
 		if (err < 0)
-			pr_debug("Setting NTB Input Size failed\n");
+			pr_debug("Setting NTB Input Size failed: %d\n", err);
 	}
 
 	/* verify maximum size of transmitted NTB in bytes */
-- 
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-next 02/14] net: cdc_ncm: use device rx_max value if update failed

[Bjørn Mork]

Alexey Orishko <alexey.orishko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:

> Based on NCM errata, NCM functional descriptor: if Bit 5 is set, then
> device can (must) handle 8-byte form of Set/GetNtbInputSize.
> If they set a flag, but don't support the feature, hm.. is it a
> prototype device or
> commercially available one?
>
> At least device must support Set request, but for GetNtbInputSize we
> could happily
> live without wNtbInMaxDatagrams (i.e. use 4 byte variant) on Linux.
> Since we must anyway receive a complete NTB and making any number of skb
> buffers from received NTB is not a problem at all.

OK, I may have misunderstood you here.  Quoting the errata text:

<quote>
  If bit D5 is set in the bmNetworkCapabilities field of function’s NCM
  Functional Descriptor, the host may set wLength either to 4 or to
  8. If wLength is 4, the function shall assume that wNtbInMaxDatagrams
  is to be set to zero. If wLength is 8, then the function shall use the
  provided value as the limit. The function shall return an error
  response (a STALL PID) if wLength is set to any other value.
</quote>

So the 4 byte variant is always supported and we might as well just use
it unconditionally because we don't set, or need to set, the
wNtbInMaxDatagrams.

Is that right?  It will simplify the code even more without any loss of
functionality, except for the possibility of failing on some other buggy
device not supporting the 4 byte variant...


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-next 02/14] net: cdc_ncm: use device rx_max value if update failed

[Alexey Orishko]

On Fri, Oct 19, 2012 at 2:18 PM, Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org> wrote:
>
> OK, I may have misunderstood you here.  Quoting the errata text:
>
> <quote>
>   If bit D5 is set in the bmNetworkCapabilities field of function’s NCM
>   Functional Descriptor, the host may set wLength either to 4 or to
>   8. If wLength is 4, the function shall assume that wNtbInMaxDatagrams
>   is to be set to zero. If wLength is 8, then the function shall use the
>   provided value as the limit. The function shall return an error
>   response (a STALL PID) if wLength is set to any other value.
> </quote>
>
> So the 4 byte variant is always supported and we might as well just use
> it unconditionally because we don't set, or need to set, the
> wNtbInMaxDatagrams.
>
> Is that right?  It will simplify the code even more without any loss of
> functionality, except for the possibility of failing on some other buggy
> device not supporting the 4 byte variant...

Agree, since 4-byte version must be supported by all devices,
we can drop 8-byte variant

/alexey
--
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-next 02/14] net: cdc_ncm: use device rx_max value if update failed

[Bjørn Mork]

Alexey Orishko <alexey.orishko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
> On Fri, Oct 19, 2012 at 2:18 PM, Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org> wrote:
>>
>> OK, I may have misunderstood you here.  Quoting the errata text:
>>
>> <quote>
>>   If bit D5 is set in the bmNetworkCapabilities field of function’s NCM
>>   Functional Descriptor, the host may set wLength either to 4 or to
>>   8. If wLength is 4, the function shall assume that wNtbInMaxDatagrams
>>   is to be set to zero. If wLength is 8, then the function shall use the
>>   provided value as the limit. The function shall return an error
>>   response (a STALL PID) if wLength is set to any other value.
>> </quote>
>>
>> So the 4 byte variant is always supported and we might as well just use
>> it unconditionally because we don't set, or need to set, the
>> wNtbInMaxDatagrams.
>>
>> Is that right?  It will simplify the code even more without any loss of
>> functionality, except for the possibility of failing on some other buggy
>> device not supporting the 4 byte variant...
>
> Agree, since 4-byte version must be supported by all devices,
> we can drop 8-byte variant

Thanks.  I'll implement that in the next version then, and drop the
first patch as it is no longer needed.

But I will hold off posting the updated series for a few more days to
allow you all some time to digest the rest of this. There should be more
issues to comment on here than this simple firmware bug workaround ;-)


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