[PATCH v2 net-next 0/8] cdc_ncm: fixes and conversion to sysfs API

[PATCH v2 net-next 0/8] cdc_ncm: fixes and conversion to sysfs API

[Bjørn Mork]

After considering the comments received after the ethtool coalesce
support was commited, I have ended up concluding that we should
remove it again, while we can, before it hits a release. The idea
was not well enough thought through, and all comments received
pointed to advantages of using a sysfs based API instead.

This series removes the ethtool coalesce support and replaces it
with sysfs attributes in a driver specific group under the netdev.

The first 3 patches are unrelated fixes:

patch 1: reducing truesize as discussed
patch 2: fixing a potentional buffer overrun when changing tx_max
patch 3: prevent framing errors when changing rx_max


Changes v2:
 - minor editorial changes to patch 8, as suggested by Peter Stuge


Bjørn Mork (8):
  net: cdc_ncm: reduce skb truesize in rx path
  net: cdc_ncm: always reallocate tx_curr_skb when tx_max increases
  net: cdc_ncm: inform usbnet when rx buffers are reduced
  net: cdc_ncm: use sysfs for rx/tx aggregation tuning
  net: cdc_ncm: drop ethtool coalesce support
  net: cdc_ncm: export NCM Transfer Block (NTB) parameters
  net: cdc_ncm: allow tuning min_tx_pkt
  net: cdc_ncm: document the sysfs API

 Documentation/ABI/testing/sysfs-class-net-cdc_ncm | 149 ++++++++++++
 drivers/net/usb/cdc_ncm.c                         | 268 ++++++++++++++++------
 2 files changed, 341 insertions(+), 76 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-net-cdc_ncm

-- 
2.0.0.rc4

--
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 v2 net-next 1/8] net: cdc_ncm: reduce skb truesize in rx path

[Bjørn Mork]

Cloning the big skbs we use for USB buffering chokes up TCP and
SCTP because the socket memory limits are hitting earlier than
they should. It is better to unconditionally copy the unwrapped
packets to freshly allocated skbs.

Reported-by: Jim Baxter <jim_baxter@mentor.com>
Acked-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
 drivers/net/usb/cdc_ncm.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 93c9ca9924eb..2bbbd65591c7 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -1289,12 +1289,11 @@ next_ndp:
 			break;
 
 		} else {
-			skb = skb_clone(skb_in, GFP_ATOMIC);
+			/* create a fresh copy to reduce truesize */
+			skb = netdev_alloc_skb_ip_align(dev->net,  len);
 			if (!skb)
 				goto error;
-			skb->len = len;
-			skb->data = ((u8 *)skb_in->data) + offset;
-			skb_set_tail_pointer(skb, len);
+			memcpy(skb_put(skb, len), skb_in->data + offset, len);
 			usbnet_skb_return(dev, skb);
 			payload += len;	/* count payload bytes in this NTB */
 		}
-- 
2.0.0.rc4

[PATCH v2 net-next 2/8] net: cdc_ncm: always reallocate tx_curr_skb when tx_max increases

[Bjørn Mork]

We are calling usbnet_start_xmit() to flush any remaining data,
depending on the side effect that tx_curr_skb is set to NULL,
ensuring a new allocation using the updated tx_max.  But this
side effect will only happen if there were any cached data ready
to transmit. If not, then an empty tx_curr_skb is still allocated
using the old tx_max size. Free it to avoid a buffer overrun.

Fixes: 68864abf08f0 ("net: cdc_ncm: support rx_max/tx_max updates when running")
Signed-off-by: Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org>
---
 drivers/net/usb/cdc_ncm.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 2bbbd65591c7..ff5b3a854898 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -268,6 +268,11 @@ static void cdc_ncm_update_rxtx_max(struct usbnet *dev, u32 new_rx, u32 new_tx)
 	if (netif_running(dev->net) && val > ctx->tx_max) {
 		netif_tx_lock_bh(dev->net);
 		usbnet_start_xmit(NULL, dev->net);
+		/* make sure tx_curr_skb is reallocated if it was empty */
+		if (ctx->tx_curr_skb) {
+			dev_kfree_skb_any(ctx->tx_curr_skb);
+			ctx->tx_curr_skb = NULL;
+		}
 		ctx->tx_max = val;
 		netif_tx_unlock_bh(dev->net);
 	} else {
-- 
2.0.0.rc4

--
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 v2 net-next 4/8] net: cdc_ncm: use sysfs for rx/tx aggregation tuning

[Bjørn Mork]

Attach a driver specific sysfs group to the netdev, and use it
for the rx/tx aggregation variables.

The datagram aggregation defined by the CDC NCM specification is
specific to this device class (including CDC MBIM). Using the
ethtool interrupt coalesce API as an interface to the aggregation
parameters redefined that API in a driver specific and confusing
way.  A sysfs group
 - makes it clear that this is a driver specific userspace API, and
 - allows us to export the real values instead of some translated
   version, and
 - lets us include more aggregation variables which were impossible
   to force into the ethtool API.

Additionally, using sysfs allows tuning the driver on space
constrained hosts where userspace tools like ethtool are undesired.

Suggested-by: Peter Stuge <peter-Y+HMSxxDrH8@public.gmane.org>
Signed-off-by: Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org>
---
 drivers/net/usb/cdc_ncm.c | 144 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 125 insertions(+), 19 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 5bce86a0d063..631741c8ff22 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -191,11 +191,9 @@ static const struct ethtool_ops cdc_ncm_ethtool_ops = {
 	.set_coalesce      = cdc_ncm_set_coalesce,
 };
 
-/* handle rx_max and tx_max changes */
-static void cdc_ncm_update_rxtx_max(struct usbnet *dev, u32 new_rx, u32 new_tx)
+static u32 cdc_ncm_check_rx_max(struct usbnet *dev, u32 new_rx)
 {
 	struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
-	u8 iface_no = ctx->control->cur_altsetting->desc.bInterfaceNumber;
 	u32 val, max, min;
 
 	/* clamp new_rx to sane values */
@@ -210,10 +208,126 @@ static void cdc_ncm_update_rxtx_max(struct usbnet *dev, u32 new_rx, u32 new_tx)
 	}
 
 	val = clamp_t(u32, new_rx, min, max);
-	if (val != new_rx) {
-		dev_dbg(&dev->intf->dev, "rx_max must be in the [%u, %u] range. Using %u\n",
-			min, max, val);
-	}
+	if (val != new_rx)
+		dev_dbg(&dev->intf->dev, "rx_max must be in the [%u, %u] range\n", min, max);
+
+	return val;
+}
+
+static u32 cdc_ncm_check_tx_max(struct usbnet *dev, u32 new_tx)
+{
+	struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
+	u32 val, max, min;
+
+	/* clamp new_tx to sane values */
+	min = ctx->max_datagram_size + ctx->max_ndp_size + sizeof(struct usb_cdc_ncm_nth16);
+	max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_TX, le32_to_cpu(ctx->ncm_parm.dwNtbOutMaxSize));
+
+	/* some devices set dwNtbOutMaxSize too low for the above default */
+	min = min(min, max);
+
+	val = clamp_t(u32, new_tx, min, max);
+	if (val != new_tx)
+		dev_dbg(&dev->intf->dev, "tx_max must be in the [%u, %u] range\n", min, max);
+
+	return val;
+}
+
+static ssize_t cdc_ncm_show_rx_max(struct device *d, struct device_attribute *attr, char *buf)
+{
+	struct usbnet *dev = netdev_priv(to_net_dev(d));
+	struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
+
+	return sprintf(buf, "%u\n", ctx->rx_max);
+}
+
+static ssize_t cdc_ncm_show_tx_max(struct device *d, struct device_attribute *attr, char *buf)
+{
+	struct usbnet *dev = netdev_priv(to_net_dev(d));
+	struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
+
+	return sprintf(buf, "%u\n", ctx->tx_max);
+}
+
+static ssize_t cdc_ncm_show_tx_timer_usecs(struct device *d, struct device_attribute *attr, char *buf)
+{
+	struct usbnet *dev = netdev_priv(to_net_dev(d));
+	struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
+
+	return sprintf(buf, "%u\n", ctx->timer_interval / (u32)NSEC_PER_USEC);
+}
+
+static ssize_t cdc_ncm_store_rx_max(struct device *d,  struct device_attribute *attr, const char *buf, size_t len)
+{
+	struct usbnet *dev = netdev_priv(to_net_dev(d));
+	struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
+	unsigned long val;
+
+	if (kstrtoul(buf, 0, &val) || cdc_ncm_check_rx_max(dev, val) != val)
+		return -EINVAL;
+
+	cdc_ncm_update_rxtx_max(dev, val, ctx->tx_max);
+	return len;
+}
+
+static ssize_t cdc_ncm_store_tx_max(struct device *d,  struct device_attribute *attr, const char *buf, size_t len)
+{
+	struct usbnet *dev = netdev_priv(to_net_dev(d));
+	struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
+	unsigned long val;
+
+	if (kstrtoul(buf, 0, &val) || cdc_ncm_check_tx_max(dev, val) != val)
+		return -EINVAL;
+
+	cdc_ncm_update_rxtx_max(dev, ctx->rx_max, val);
+	return len;
+}
+
+static ssize_t cdc_ncm_store_tx_timer_usecs(struct device *d,  struct device_attribute *attr, const char *buf, size_t len)
+{
+	struct usbnet *dev = netdev_priv(to_net_dev(d));
+	struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
+	ssize_t ret;
+	unsigned long val;
+
+	ret = kstrtoul(buf, 0, &val);
+	if (ret)
+		return ret;
+	if (val && (val < CDC_NCM_TIMER_INTERVAL_MIN || val > CDC_NCM_TIMER_INTERVAL_MAX))
+		return -EINVAL;
+
+	spin_lock_bh(&ctx->mtx);
+	ctx->timer_interval = val * NSEC_PER_USEC;
+	if (!ctx->timer_interval)
+		ctx->tx_timer_pending = 0;
+	spin_unlock_bh(&ctx->mtx);
+	return len;
+}
+
+static DEVICE_ATTR(rx_max, S_IRUGO | S_IWUSR, cdc_ncm_show_rx_max, cdc_ncm_store_rx_max);
+static DEVICE_ATTR(tx_max, S_IRUGO | S_IWUSR, cdc_ncm_show_tx_max, cdc_ncm_store_tx_max);
+static DEVICE_ATTR(tx_timer_usecs, S_IRUGO | S_IWUSR, cdc_ncm_show_tx_timer_usecs, cdc_ncm_store_tx_timer_usecs);
+
+static struct attribute *cdc_ncm_sysfs_attrs[] = {
+	&dev_attr_rx_max.attr,
+	&dev_attr_tx_max.attr,
+	&dev_attr_tx_timer_usecs.attr,
+	NULL,
+};
+
+static struct attribute_group cdc_ncm_sysfs_attr_group = {
+	.name = "cdc_ncm",
+	.attrs = cdc_ncm_sysfs_attrs,
+};
+
+/* handle rx_max and tx_max changes */
+static void cdc_ncm_update_rxtx_max(struct usbnet *dev, u32 new_rx, u32 new_tx)
+{
+	struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
+	u8 iface_no = ctx->control->cur_altsetting->desc.bInterfaceNumber;
+	u32 val;
+
+	val = cdc_ncm_check_rx_max(dev, new_rx);
 
 	/* inform device about NTB input size changes */
 	if (val != ctx->rx_max) {
@@ -238,18 +352,7 @@ static void cdc_ncm_update_rxtx_max(struct usbnet *dev, u32 new_rx, u32 new_tx)
 			usbnet_unlink_rx_urbs(dev);
 	}
 
-	/* clamp new_tx to sane values */
-	min = ctx->max_datagram_size + ctx->max_ndp_size + sizeof(struct usb_cdc_ncm_nth16);
-	max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_TX, le32_to_cpu(ctx->ncm_parm.dwNtbOutMaxSize));
-
-	/* some devices set dwNtbOutMaxSize too low for the above default */
-	min = min(min, max);

[PATCH v2 net-next 3/8] net: cdc_ncm: inform usbnet when rx buffers are reduced

[Bjørn Mork]

It doesn't matter whether the buffer size goes up or down.  We have to
keep usbnet and device syncronized to be able to split transfers at the
correct boundaries. The spec allow skipping short packets when using
max sized transfers.  If we don't tell usbnet about our new expected rx
buffer size, then it will merge and/or split NTBs.  The driver does not
support this, and the result will be lots of framing errors.

Fix by always reallocating usbnet rx buffers when the rx_max value
changes.

Fixes: 68864abf08f0 ("net: cdc_ncm: support rx_max/tx_max updates when running")
Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
 drivers/net/usb/cdc_ncm.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index ff5b3a854898..5bce86a0d063 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -215,19 +215,12 @@ static void cdc_ncm_update_rxtx_max(struct usbnet *dev, u32 new_rx, u32 new_tx)
 			min, max, val);
 	}
 
-	/* usbnet use these values for sizing rx queues */
-	dev->rx_urb_size = val;
-
 	/* inform device about NTB input size changes */
 	if (val != ctx->rx_max) {
 		__le32 dwNtbInMaxSize = cpu_to_le32(val);
 
 		dev_info(&dev->intf->dev, "setting rx_max = %u\n", val);
 
-		/* need to unlink rx urbs before increasing buffer size */
-		if (netif_running(dev->net) && dev->rx_urb_size > ctx->rx_max)
-			usbnet_unlink_rx_urbs(dev);
-
 		/* tell device to use new size */
 		if (usbnet_write_cmd(dev, USB_CDC_SET_NTB_INPUT_SIZE,
 				     USB_TYPE_CLASS | USB_DIR_OUT
@@ -238,6 +231,13 @@ static void cdc_ncm_update_rxtx_max(struct usbnet *dev, u32 new_rx, u32 new_tx)
 			ctx->rx_max = val;
 	}
 
+	/* usbnet use these values for sizing rx queues */
+	if (dev->rx_urb_size != ctx->rx_max) {
+		dev->rx_urb_size = ctx->rx_max;
+		if (netif_running(dev->net))
+			usbnet_unlink_rx_urbs(dev);
+	}
+
 	/* clamp new_tx to sane values */
 	min = ctx->max_datagram_size + ctx->max_ndp_size + sizeof(struct usb_cdc_ncm_nth16);
 	max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_TX, le32_to_cpu(ctx->ncm_parm.dwNtbOutMaxSize));
-- 
2.0.0.rc4

RE: [PATCH v2 net-next 3/8] net: cdc_ncm: inform usbnet when rx buffers are reduced

[David Laight]

From: Bjørn Mork 
> It doesn't matter whether the buffer size goes up or down.  We have to
> keep usbnet and device syncronized to be able to split transfers at the
> correct boundaries. The spec allow skipping short packets when using
> max sized transfers.  If we don't tell usbnet about our new expected rx
> buffer size, then it will merge and/or split NTBs.  The driver does not
> support this, and the result will be lots of framing errors.
> 
> Fix by always reallocating usbnet rx buffers when the rx_max value
> changes.

I'm guessing that the rx_max value is the maximum size of the USB bulk
data 'message' that the device generates?

As such the URB only need to be longer that it.
(Or multiples of the USB packet size, and the driver then merge URB
when generating skbs.)

Since you are now copying the data out of the URB's skb before
passing the ethernet packet upstream, is there ever any real
requirement to use a small rx_max? or ever change rx_max?

	David

> Fixes: 68864abf08f0 ("net: cdc_ncm: support rx_max/tx_max updates when running")
> Signed-off-by: Bjørn Mork <bjorn@mork.no>
> ---
>  drivers/net/usb/cdc_ncm.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> index ff5b3a854898..5bce86a0d063 100644
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -215,19 +215,12 @@ static void cdc_ncm_update_rxtx_max(struct usbnet *dev, u32 new_rx, u32 new_tx)
>  			min, max, val);
>  	}
> 
> -	/* usbnet use these values for sizing rx queues */
> -	dev->rx_urb_size = val;
> -
>  	/* inform device about NTB input size changes */
>  	if (val != ctx->rx_max) {
>  		__le32 dwNtbInMaxSize = cpu_to_le32(val);
> 
>  		dev_info(&dev->intf->dev, "setting rx_max = %u\n", val);
> 
> -		/* need to unlink rx urbs before increasing buffer size */
> -		if (netif_running(dev->net) && dev->rx_urb_size > ctx->rx_max)
> -			usbnet_unlink_rx_urbs(dev);
> -
>  		/* tell device to use new size */
>  		if (usbnet_write_cmd(dev, USB_CDC_SET_NTB_INPUT_SIZE,
>  				     USB_TYPE_CLASS | USB_DIR_OUT
> @@ -238,6 +231,13 @@ static void cdc_ncm_update_rxtx_max(struct usbnet *dev, u32 new_rx, u32 new_tx)
>  			ctx->rx_max = val;
>  	}
> 
> +	/* usbnet use these values for sizing rx queues */
> +	if (dev->rx_urb_size != ctx->rx_max) {
> +		dev->rx_urb_size = ctx->rx_max;
> +		if (netif_running(dev->net))
> +			usbnet_unlink_rx_urbs(dev);
> +	}
> +
>  	/* clamp new_tx to sane values */
>  	min = ctx->max_datagram_size + ctx->max_ndp_size + sizeof(struct usb_cdc_ncm_nth16);
>  	max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_TX, le32_to_cpu(ctx->ncm_parm.dwNtbOutMaxSize));
> --
> 2.0.0.rc4

Re: [PATCH v2 net-next 3/8] net: cdc_ncm: inform usbnet when rx buffers are reduced

[Bjørn Mork]

David Laight <David.Laight@ACULAB.COM> writes:
> From: Bjørn Mork 
>> It doesn't matter whether the buffer size goes up or down.  We have to
>> keep usbnet and device syncronized to be able to split transfers at the
>> correct boundaries. The spec allow skipping short packets when using
>> max sized transfers.  If we don't tell usbnet about our new expected rx
>> buffer size, then it will merge and/or split NTBs.  The driver does not
>> support this, and the result will be lots of framing errors.
>> 
>> Fix by always reallocating usbnet rx buffers when the rx_max value
>> changes.
>
> I'm guessing that the rx_max value is the maximum size of the USB bulk
> data 'message' that the device generates?
>
> As such the URB only need to be longer that it.

So did I think too at first.  That's how I added the bug fixed by this
commit :-)

The problem with NCM is that it explicitly allows (and encourage) using
transfers which are multiples of the USB packet size, *without* any
terminating short packet (0 or 1 byte). This means that the USB core
won't know or care about the end of one transfer and the beginning of
the next.  Which is fine.  But the cdc_ncm driver has to know, because
it must split the transfers into frames it can decode.

Now, the current cdc_ncm implementation has a built-in assumption that
the size of the URB == rx_max.  This lets it simplify the splitting into
frames to nearly nothing: Any received URB contains exactly one frame.
Therefore we need to keep the rx URB size strictly syncronized the
rx_max.

> (Or multiples of the USB packet size, and the driver then merge URB
> when generating skbs.)
>
> Since you are now copying the data out of the URB's skb before
> passing the ethernet packet upstream, is there ever any real
> requirement to use a small rx_max? or ever change rx_max?

Yes.  usbnet doesn't currently recycle skbs.  Continuously allocating 32
kB skbs (or even 64 kB truesize, which is the current cdc_ncm worst
case) on memory constrained hosts is bound to fail eventually.  Reducing
rx_max will reduce the order of the allocations and thereby the
probability of failing.

I've had reports from OpenWRT users with Huawei modems (which tend to
use extreme buffers), constantly hitting the usbnet allocation failure
event 'EVENT_RX_MEMORY'.  The result is of course terrible performance,
where usbnet gets really busy trying to allocate buffers which just
cannot be allocated...  Reducing rx_max by simply rebuilding the old
driver with a lower hard limit has pretty much eliminated these issues,
but is not generally a solution because it makes some modem firmwares
fail.

True, copying instead of cloning is probably going to help this problem
too, but I believe there are still reasons why you do not want to allow
the 64 kB skb abuse.



Bjørn

RE: [PATCH v2 net-next 3/8] net: cdc_ncm: inform usbnet when rx buffers are reduced

[David Laight]

From: Bjørn Mork
> David Laight <David.Laight@ACULAB.COM> writes:
> > From: Bjørn Mork
> >> It doesn't matter whether the buffer size goes up or down.  We have to
> >> keep usbnet and device syncronized to be able to split transfers at the
> >> correct boundaries. The spec allow skipping short packets when using
> >> max sized transfers.  If we don't tell usbnet about our new expected rx
> >> buffer size, then it will merge and/or split NTBs.  The driver does not
> >> support this, and the result will be lots of framing errors.
> >>
> >> Fix by always reallocating usbnet rx buffers when the rx_max value
> >> changes.
> >
> > I'm guessing that the rx_max value is the maximum size of the USB bulk
> > data 'message' that the device generates?
> >
> > As such the URB only need to be longer that it.
> 
> So did I think too at first.  That's how I added the bug fixed by this
> commit :-)
> 
> The problem with NCM is that it explicitly allows (and encourage) using
> transfers which are multiples of the USB packet size, *without* any
> terminating short packet (0 or 1 byte). This means that the USB core
> won't know or care about the end of one transfer and the beginning of
> the next.  Which is fine.  But the cdc_ncm driver has to know, because
> it must split the transfers into frames it can decode.
> 
> Now, the current cdc_ncm implementation has a built-in assumption that
> the size of the URB == rx_max.  This lets it simplify the splitting into
> frames to nearly nothing: Any received URB contains exactly one frame.
> Therefore we need to keep the rx URB size strictly syncronized the
> rx_max.

Hmmm....
So there is an ethernet packet size somewhere near 500 that exactly fills
a 512 byte USB2 frame.
If you receive one of those does the hardware send a 0 length terminating
fragment?
If not the then URB won't complete until after the next ethernet frame
arrives.
Receive three of them followed by a longer frame and you'll overrun the
end of the URB.

At least one path through usbnet ensures that the rx buffers aren't
a multiple of the USB frame size. I'm not sure whether that really helps.

Sounds like the hardware expects you to receive each USB bulk buffer
separately.

	David

Re: [PATCH v2 net-next 3/8] net: cdc_ncm: inform usbnet when rx buffers are reduced

[Bjørn Mork]

David Laight <David.Laight@ACULAB.COM> writes:

> From: Bjørn Mork
>> David Laight <David.Laight@ACULAB.COM> writes:
>> > From: Bjørn Mork
>> >> It doesn't matter whether the buffer size goes up or down.  We have to
>> >> keep usbnet and device syncronized to be able to split transfers at the
>> >> correct boundaries. The spec allow skipping short packets when using
>> >> max sized transfers.  If we don't tell usbnet about our new expected rx
>> >> buffer size, then it will merge and/or split NTBs.  The driver does not
>> >> support this, and the result will be lots of framing errors.
>> >>
>> >> Fix by always reallocating usbnet rx buffers when the rx_max value
>> >> changes.
>> >
>> > I'm guessing that the rx_max value is the maximum size of the USB bulk
>> > data 'message' that the device generates?
>> >
>> > As such the URB only need to be longer that it.
>> 
>> So did I think too at first.  That's how I added the bug fixed by this
>> commit :-)
>> 
>> The problem with NCM is that it explicitly allows (and encourage) using
>> transfers which are multiples of the USB packet size, *without* any
>> terminating short packet (0 or 1 byte). This means that the USB core
>> won't know or care about the end of one transfer and the beginning of
>> the next.  Which is fine.  But the cdc_ncm driver has to know, because
>> it must split the transfers into frames it can decode.
>> 
>> Now, the current cdc_ncm implementation has a built-in assumption that
>> the size of the URB == rx_max.  This lets it simplify the splitting into
>> frames to nearly nothing: Any received URB contains exactly one frame.
>> Therefore we need to keep the rx URB size strictly syncronized the
>> rx_max.
>
> Hmmm....
> So there is an ethernet packet size somewhere near 500 that exactly fills
> a 512 byte USB2 frame.
> If you receive one of those does the hardware send a 0 length terminating
> fragment?
> If not the then URB won't complete until after the next ethernet frame
> arrives.
> Receive three of them followed by a longer frame and you'll overrun the
> end of the URB.
>
> At least one path through usbnet ensures that the rx buffers aren't
> a multiple of the USB frame size. I'm not sure whether that really helps.
>
> Sounds like the hardware expects you to receive each USB bulk buffer
> separately.

Please, go read the NCM spec.


Bjørn

RE: [PATCH v2 net-next 3/8] net: cdc_ncm: inform usbnet when rx buffers are reduced

[David Laight]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 759 bytes --]

From: Bjørn Mork
...
> > Since you are now copying the data out of the URB's skb before
> > passing the ethernet packet upstream, is there ever any real
> > requirement to use a small rx_max? or ever change rx_max?
> 
> Yes.  usbnet doesn't currently recycle skbs.  Continuously allocating 32
> kB skbs (or even 64 kB truesize, which is the current cdc_ncm worst
> case) on memory constrained hosts is bound to fail eventually.

Getting usbnet to recycle skb shouldn't be too hard - and will
be an immediate gain for most of the drivers.
I think they would all benefit from copying frames to new skb.

	David

N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±ºÆâžØ^n‡r¡ö¦zË\x1aëh™¨è­Ú&¢îý»\x05ËÛÔØï¦v¬Îf\x1dp)¹¹br	šê+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹\x1e®w¥¢¸?™¨è­Ú&¢)ߢ^[f

Re: [PATCH v2 net-next 3/8] net: cdc_ncm: inform usbnet when rx buffers are reduced

[Bjørn Mork]

David Laight <David.Laight@ACULAB.COM> writes:

> From: Bjørn Mork
> ...
>> > Since you are now copying the data out of the URB's skb before
>> > passing the ethernet packet upstream, is there ever any real
>> > requirement to use a small rx_max? or ever change rx_max?
>> 
>> Yes.  usbnet doesn't currently recycle skbs.  Continuously allocating 32
>> kB skbs (or even 64 kB truesize, which is the current cdc_ncm worst
>> case) on memory constrained hosts is bound to fail eventually.
>
> Getting usbnet to recycle skb shouldn't be too hard - and will
> be an immediate gain for most of the drivers.

Yes.  This is one of those problems that really isn't that hard to
solve.  Until you start hitting all the corner cases...  But I'm sure it
is doable.

Anyway, it is not going to be part of this patch series, which is merely
touching the cdc_ncm minidriver within the existing usbnet framework.


Bjørn

[PATCH v2 net-next 5/8] net: cdc_ncm: drop ethtool coalesce support

[Bjørn Mork]

The ethtool coalesce API is not applicable for this driver. Forcing
it to fit the NCM aggregation redefined the API in a driver specific
way, which is much worse than defining a clean new API. These ethtool
coalesce functions have therefore been replaced by a new sysfs API.

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

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 631741c8ff22..aaa440d892b8 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -127,54 +127,8 @@ static void cdc_ncm_get_strings(struct net_device __always_unused *netdev, u32 s
 	}
 }
 
-static int cdc_ncm_get_coalesce(struct net_device *netdev,
-				struct ethtool_coalesce *ec)
-{
-	struct usbnet *dev = netdev_priv(netdev);
-	struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
-
-	/* assuming maximum sized dgrams and ignoring NDPs */
-	ec->rx_max_coalesced_frames = ctx->rx_max / ctx->max_datagram_size;
-	ec->tx_max_coalesced_frames = ctx->tx_max / ctx->max_datagram_size;
-
-	/* the timer will fire CDC_NCM_TIMER_PENDING_CNT times in a row */
-	ec->tx_coalesce_usecs = ctx->timer_interval / (NSEC_PER_USEC / CDC_NCM_TIMER_PENDING_CNT);
-	return 0;
-}
-
 static void cdc_ncm_update_rxtx_max(struct usbnet *dev, u32 new_rx, u32 new_tx);
 
-static int cdc_ncm_set_coalesce(struct net_device *netdev,
-				struct ethtool_coalesce *ec)
-{
-	struct usbnet *dev = netdev_priv(netdev);
-	struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
-	u32 new_rx_max = ctx->rx_max;
-	u32 new_tx_max = ctx->tx_max;
-
-	/* assuming maximum sized dgrams and a single NDP */
-	if (ec->rx_max_coalesced_frames)
-		new_rx_max = ec->rx_max_coalesced_frames * ctx->max_datagram_size;
-	if (ec->tx_max_coalesced_frames)
-		new_tx_max = ec->tx_max_coalesced_frames * ctx->max_datagram_size;
-
-	if (ec->tx_coalesce_usecs &&
-	    (ec->tx_coalesce_usecs < CDC_NCM_TIMER_INTERVAL_MIN * CDC_NCM_TIMER_PENDING_CNT ||
-	     ec->tx_coalesce_usecs > CDC_NCM_TIMER_INTERVAL_MAX * CDC_NCM_TIMER_PENDING_CNT))
-		return -EINVAL;
-
-	spin_lock_bh(&ctx->mtx);
-	ctx->timer_interval = ec->tx_coalesce_usecs * (NSEC_PER_USEC / CDC_NCM_TIMER_PENDING_CNT);
-	if (!ctx->timer_interval)
-		ctx->tx_timer_pending = 0;
-	spin_unlock_bh(&ctx->mtx);
-
-	/* inform device of new values */
-	if (new_rx_max != ctx->rx_max || new_tx_max != ctx->tx_max)
-		cdc_ncm_update_rxtx_max(dev, new_rx_max, new_tx_max);
-	return 0;
-}
-
 static const struct ethtool_ops cdc_ncm_ethtool_ops = {
 	.get_settings      = usbnet_get_settings,
 	.set_settings      = usbnet_set_settings,
@@ -187,8 +141,6 @@ static const struct ethtool_ops cdc_ncm_ethtool_ops = {
 	.get_sset_count    = cdc_ncm_get_sset_count,
 	.get_strings       = cdc_ncm_get_strings,
 	.get_ethtool_stats = cdc_ncm_get_ethtool_stats,
-	.get_coalesce      = cdc_ncm_get_coalesce,
-	.set_coalesce      = cdc_ncm_set_coalesce,
 };
 
 static u32 cdc_ncm_check_rx_max(struct usbnet *dev, u32 new_rx)
-- 
2.0.0.rc4

[PATCH v2 net-next 6/8] net: cdc_ncm: export NCM Transfer Block (NTB) parameters

[Bjørn Mork]

The mandatory GetNtbParameters control request is an important part of
the host <-> device protocol negotiation in CDC NCM (and CDC MBIM). It
gives device limits which the host must obey when configuring the
protocol aggregation variables. The driver will enforce this by
rejecting attempts to set any of the tunable variables to a value
which is not supported by the device.  Exporting the parameter block
helps userspace decide which values are allowed without resorting
to trial and error.

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

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index aaa440d892b8..98c3adb5aea3 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -260,10 +260,40 @@ static DEVICE_ATTR(rx_max, S_IRUGO | S_IWUSR, cdc_ncm_show_rx_max, cdc_ncm_store
 static DEVICE_ATTR(tx_max, S_IRUGO | S_IWUSR, cdc_ncm_show_tx_max, cdc_ncm_store_tx_max);
 static DEVICE_ATTR(tx_timer_usecs, S_IRUGO | S_IWUSR, cdc_ncm_show_tx_timer_usecs, cdc_ncm_store_tx_timer_usecs);
 
+#define NCM_PARM_ATTR(name, format, tocpu)				\
+static ssize_t cdc_ncm_show_##name(struct device *d, struct device_attribute *attr, char *buf) \
+{ \
+	struct usbnet *dev = netdev_priv(to_net_dev(d)); \
+	struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0]; \
+	return sprintf(buf, format "\n", tocpu(ctx->ncm_parm.name));	\
+} \
+static DEVICE_ATTR(name, S_IRUGO, cdc_ncm_show_##name, NULL)
+
+NCM_PARM_ATTR(bmNtbFormatsSupported, "0x%04x", le16_to_cpu);
+NCM_PARM_ATTR(dwNtbInMaxSize, "%u", le32_to_cpu);
+NCM_PARM_ATTR(wNdpInDivisor, "%u", le16_to_cpu);
+NCM_PARM_ATTR(wNdpInPayloadRemainder, "%u", le16_to_cpu);
+NCM_PARM_ATTR(wNdpInAlignment, "%u", le16_to_cpu);
+NCM_PARM_ATTR(dwNtbOutMaxSize, "%u", le32_to_cpu);
+NCM_PARM_ATTR(wNdpOutDivisor, "%u", le16_to_cpu);
+NCM_PARM_ATTR(wNdpOutPayloadRemainder, "%u", le16_to_cpu);
+NCM_PARM_ATTR(wNdpOutAlignment, "%u", le16_to_cpu);
+NCM_PARM_ATTR(wNtbOutMaxDatagrams, "%u", le16_to_cpu);
+
 static struct attribute *cdc_ncm_sysfs_attrs[] = {
 	&dev_attr_rx_max.attr,
 	&dev_attr_tx_max.attr,
 	&dev_attr_tx_timer_usecs.attr,
+	&dev_attr_bmNtbFormatsSupported.attr,
+	&dev_attr_dwNtbInMaxSize.attr,
+	&dev_attr_wNdpInDivisor.attr,
+	&dev_attr_wNdpInPayloadRemainder.attr,
+	&dev_attr_wNdpInAlignment.attr,
+	&dev_attr_dwNtbOutMaxSize.attr,
+	&dev_attr_wNdpOutDivisor.attr,
+	&dev_attr_wNdpOutPayloadRemainder.attr,
+	&dev_attr_wNdpOutAlignment.attr,
+	&dev_attr_wNtbOutMaxDatagrams.attr,
 	NULL,
 };
 
-- 
2.0.0.rc4

[PATCH v2 net-next 7/8] net: cdc_ncm: allow tuning min_tx_pkt

[Bjørn Mork]

The min_tx_pkt variable decides the cutoff point where the driver
will stop padding out NTBs to maximum size. The padding is a tradeoff
where we use some USB bus bandwidth to allow the device to receive
fixed size buffers. Different devices will have different optimal
settings, spanning from no padding at all to padding every NTB.
There is no way to automatically figure out which setting is best
for a specific device.

The default value is a reasonable tradeoff, calculated based on the
USB packet size and out NTB max size. This may have to be changed
along with any tx_max changes.

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

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 98c3adb5aea3..80a844e0ae03 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -185,6 +185,14 @@ static u32 cdc_ncm_check_tx_max(struct usbnet *dev, u32 new_tx)
 	return val;
 }
 
+static ssize_t cdc_ncm_show_min_tx_pkt(struct device *d, struct device_attribute *attr, char *buf)
+{
+	struct usbnet *dev = netdev_priv(to_net_dev(d));
+	struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
+
+	return sprintf(buf, "%u\n", ctx->min_tx_pkt);
+}
+
 static ssize_t cdc_ncm_show_rx_max(struct device *d, struct device_attribute *attr, char *buf)
 {
 	struct usbnet *dev = netdev_priv(to_net_dev(d));
@@ -209,6 +217,20 @@ static ssize_t cdc_ncm_show_tx_timer_usecs(struct device *d, struct device_attri
 	return sprintf(buf, "%u\n", ctx->timer_interval / (u32)NSEC_PER_USEC);
 }
 
+static ssize_t cdc_ncm_store_min_tx_pkt(struct device *d,  struct device_attribute *attr, const char *buf, size_t len)
+{
+	struct usbnet *dev = netdev_priv(to_net_dev(d));
+	struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
+	unsigned long val;
+
+	/* no need to restrict values - anything from 0 to infinity is OK */
+	if (kstrtoul(buf, 0, &val))
+		return -EINVAL;
+
+	ctx->min_tx_pkt = val;
+	return len;
+}
+
 static ssize_t cdc_ncm_store_rx_max(struct device *d,  struct device_attribute *attr, const char *buf, size_t len)
 {
 	struct usbnet *dev = netdev_priv(to_net_dev(d));
@@ -256,6 +278,7 @@ static ssize_t cdc_ncm_store_tx_timer_usecs(struct device *d,  struct device_att
 	return len;
 }
 
+static DEVICE_ATTR(min_tx_pkt, S_IRUGO | S_IWUSR, cdc_ncm_show_min_tx_pkt, cdc_ncm_store_min_tx_pkt);
 static DEVICE_ATTR(rx_max, S_IRUGO | S_IWUSR, cdc_ncm_show_rx_max, cdc_ncm_store_rx_max);
 static DEVICE_ATTR(tx_max, S_IRUGO | S_IWUSR, cdc_ncm_show_tx_max, cdc_ncm_store_tx_max);
 static DEVICE_ATTR(tx_timer_usecs, S_IRUGO | S_IWUSR, cdc_ncm_show_tx_timer_usecs, cdc_ncm_store_tx_timer_usecs);
@@ -281,6 +304,7 @@ NCM_PARM_ATTR(wNdpOutAlignment, "%u", le16_to_cpu);
 NCM_PARM_ATTR(wNtbOutMaxDatagrams, "%u", le16_to_cpu);
 
 static struct attribute *cdc_ncm_sysfs_attrs[] = {
+	&dev_attr_min_tx_pkt.attr,
 	&dev_attr_rx_max.attr,
 	&dev_attr_tx_max.attr,
 	&dev_attr_tx_timer_usecs.attr,
-- 
2.0.0.rc4

[PATCH v2 net-next 8/8] net: cdc_ncm: document the sysfs API

[Bjørn Mork]

Adding documentation for all the driver specific sysfs attributes.

Cc: Peter Stuge <peter@stuge.se>
Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
 Documentation/ABI/testing/sysfs-class-net-cdc_ncm | 149 ++++++++++++++++++++++
 1 file changed, 149 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-net-cdc_ncm

diff --git a/Documentation/ABI/testing/sysfs-class-net-cdc_ncm b/Documentation/ABI/testing/sysfs-class-net-cdc_ncm
new file mode 100644
index 000000000000..5cedf72df358
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-net-cdc_ncm
@@ -0,0 +1,149 @@
+What:		/sys/class/net/<iface>/cdc_ncm/min_tx_pkt
+Date:		May 2014
+KernelVersion:	3.16
+Contact:	Bjørn Mork <bjorn@mork.no>
+Description:
+		The driver will pad NCM Transfer Blocks (NTBs) longer
+		than this to tx_max, allowing the device to receive
+		tx_max sized frames with no terminating short
+		packet. NTBs shorter than this limit are transmitted
+		as-is, without any padding, and are terminated with a
+		short USB packet.
+
+		Padding to tx_max allows the driver to transmit NTBs
+		back-to-back without any interleaving short USB
+		packets.  This reduces the number of short packet
+		interrupts in the device, and represents a tradeoff
+		between USB bus bandwidth and device DMA optimization.
+
+		Set to 0 to pad all frames. Set greater than tx_max to
+		disable all padding.
+
+What:		/sys/class/net/<iface>/cdc_ncm/rx_max
+Date:		May 2014
+KernelVersion:	3.16
+Contact:	Bjørn Mork <bjorn@mork.no>
+Description:
+		The maximum NTB size for RX.  Cannot exceed the
+		maximum value supported by the device. Must allow at
+		least one max sized datagram plus headers.
+
+		The actual limits are device dependent.  See
+		dwNtbInMaxSize.
+
+		Note: Some devices will silently ignore changes to
+		this value, resulting in oversized NTBs and
+		corresponding framing errors.
+
+What:		/sys/class/net/<iface>/cdc_ncm/tx_max
+Date:		May 2014
+KernelVersion:	3.16
+Contact:	Bjørn Mork <bjorn@mork.no>
+Description:
+		The maximum NTB size for TX.  Cannot exceed the
+		maximum value supported by the device.  Must allow at
+		least one max sized datagram plus headers.
+
+		The actual limits are device dependent.  See
+		dwNtbOutMaxSize.
+
+What:		/sys/class/net/<iface>/cdc_ncm/tx_timer_usecs
+Date:		May 2014
+KernelVersion:	3.16
+Contact:	Bjørn Mork <bjorn@mork.no>
+Description:
+		Datagram aggregation timeout in µs. The driver will
+		wait up to 3 times this timeout for more datagrams to
+		aggregate before transmitting an NTB frame.
+
+		Valid range: 5 to 4000000
+
+		Set to 0 to disable aggregation.
+
+The following read-only attributes all represent fields of the
+structure defined in section 6.2.1 "GetNtbParameters" of "Universal
+Serial Bus Communications Class Subclass Specifications for Network
+Control Model Devices" (CDC NCM), Revision 1.0 (Errata 1), November
+24, 2010 from USB Implementers Forum, Inc.  The descriptions are
+quoted from table 6-3 of CDC NCM: "NTB Parameter Structure".
+
+What:		/sys/class/net/<iface>/cdc_ncm/bmNtbFormatsSupported
+Date:		May 2014
+KernelVersion:	3.16
+Contact:	Bjørn Mork <bjorn@mork.no>
+Description:
+		Bit 0: 16-bit NTB supported (set to 1)
+		Bit 1: 32-bit NTB supported
+		Bits 2 – 15: reserved (reset to zero; must be ignored by host)
+
+What:		/sys/class/net/<iface>/cdc_ncm/dwNtbInMaxSize
+Date:		May 2014
+KernelVersion:	3.16
+Contact:	Bjørn Mork <bjorn@mork.no>
+Description:
+		IN NTB Maximum Size in bytes
+
+What:		/sys/class/net/<iface>/cdc_ncm/wNdpInDivisor
+Date:		May 2014
+KernelVersion:	3.16
+Contact:	Bjørn Mork <bjorn@mork.no>
+Description:
+		Divisor used for IN NTB Datagram payload alignment
+
+What:		/sys/class/net/<iface>/cdc_ncm/wNdpInPayloadRemainder
+Date:		May 2014
+KernelVersion:	3.16
+Contact:	Bjørn Mork <bjorn@mork.no>
+Description:
+		Remainder used to align input datagram payload within
+		the NTB: (Payload Offset) mod (wNdpInDivisor) =
+		wNdpInPayloadRemainder
+
+What:		/sys/class/net/<iface>/cdc_ncm/wNdpInAlignment
+Date:		May 2014
+KernelVersion:	3.16
+Contact:	Bjørn Mork <bjorn@mork.no>
+Description:
+		NDP alignment modulus for NTBs on the IN pipe. Shall
+		be a power of 2, and shall be at least 4.
+
+What:		/sys/class/net/<iface>/cdc_ncm/dwNtbOutMaxSize
+Date:		May 2014
+KernelVersion:	3.16
+Contact:	Bjørn Mork <bjorn@mork.no>
+Description:
+		OUT NTB Maximum Size
+
+What:		/sys/class/net/<iface>/cdc_ncm/wNdpOutDivisor
+Date:		May 2014
+KernelVersion:	3.16
+Contact:	Bjørn Mork <bjorn@mork.no>
+Description:
+		OUT NTB Datagram alignment modulus
+
+What:		/sys/class/net/<iface>/cdc_ncm/wNdpOutPayloadRemainder
+Date:		May 2014
+KernelVersion:	3.16
+Contact:	Bjørn Mork <bjorn@mork.no>
+Description:
+		Remainder used to align output datagram payload
+		offsets within the NTB: Padding, shall be transmitted
+		as zero by function, and ignored by host.  (Payload
+		Offset) mod (wNdpOutDivisor) = wNdpOutPayloadRemainder
+
+What:		/sys/class/net/<iface>/cdc_ncm/wNdpOutAlignment
+Date:		May 2014
+KernelVersion:	3.16
+Contact:	Bjørn Mork <bjorn@mork.no>
+Description:
+		NDP alignment modulus for use in NTBs on the OUT
+		pipe. Shall be a power of 2, and shall be at least 4.
+
+What:		/sys/class/net/<iface>/cdc_ncm/wNtbOutMaxDatagrams
+Date:		May 2014
+KernelVersion:	3.16
+Contact:	Bjørn Mork <bjorn@mork.no>
+Description:
+		Maximum number of datagrams that the host may pack
+		into a single OUT NTB. Zero means that the device
+		imposes no limit.
-- 
2.0.0.rc4

Re: [PATCH v2 net-next 0/8] cdc_ncm: fixes and conversion to sysfs API

[David Miller]

From: Bjørn Mork <bjorn@mork.no>
Date: Fri, 30 May 2014 09:31:02 +0200

> After considering the comments received after the ethtool coalesce
> support was commited, I have ended up concluding that we should
> remove it again, while we can, before it hits a release. The idea
> was not well enough thought through, and all comments received
> pointed to advantages of using a sysfs based API instead.
> 
> This series removes the ethtool coalesce support and replaces it
> with sysfs attributes in a driver specific group under the netdev.
> 
> The first 3 patches are unrelated fixes:
> 
> patch 1: reducing truesize as discussed
> patch 2: fixing a potentional buffer overrun when changing tx_max
> patch 3: prevent framing errors when changing rx_max
> 
> Changes v2:
>  - minor editorial changes to patch 8, as suggested by Peter Stuge

Generally speaking I wouldn't be OK with using sysfs for this stuff,
but you make a very convincing argument for doing so in this specific
case.

Series applied, thanks.