Re: [PATCH net-next 01/11] net: cdc_ncm: split out rx_max/tx_max update of setup

Re: [PATCH net-next 01/11] net: cdc_ncm: split out rx_max/tx_max update of setup

[Enrico Mioso]

Note: I am re-sending the mail, since I sent it privately to Bj*rn only, by 
mistake. Sorry. And sorry also for continously mistyping your name: I would 
like to learn writing it right! :)

I would also propose some polite warning message regarding what's happening.
Ok, I know it may simply be SPAM for users, but it may help identifying devices
not behaving properly in regard to specifications.
I know - it doesn't make much sense, even considering there are simply devices
that will act like NCM but use AT for signaling reasons. But with this message,
the kernle may inform users about a situation like:
2oh - I am trying to do my best. If performances are not as good as they might
be, proceed with ..."
This is only a proposal, nothing more.

Enrico

[PATCH net-next 00/11] cdc_ncm: add buffer tuning and stats using ethtool

[Bjørn Mork]

The first 9 patches of this set are unchanged from the RFC I
sent more than a week ago.  The 2 last patches are leftover
minor cleanups, which I found laying around from last years
cleaning job.

David Laight was the only one commenting on the RFC, and I
have interpreted his comment as a wish for further work on
the usbnet framework, and not a request for any changes to
this patch set.  Hence this submission of without changes to
patch 1-9, which I described like this for the RFC:

"I have got quite a few reports from frustrated users of OpenWRT
hosts trying to use some powerful LTE modem, but not achieving
full speed.  This is typically caused by a combination of
big buffers and little memory, giving in allocation errors and
bad performance as a result.

This series is an attempt to let users adjust the size of these
buffers without having to rebuild the driver.

Patches 1 - 4 are mostly rearranging existing code, in preparing
for the dynamic buffer size changes.

Patch 5 adds userspace control (ab)using the ethtool coalescing
API. This isn't a perfect match, which is the main reason why I
post this series as a RFC.

Patch 6 is an unrelated framing optimization, reducing the
overhead quite a bit and allowing for better use of smaller
buffers.

Patch 7 changes the way we calculate frame padding cutoff. The
problem with big buffers is made much worse by the current padding
strategy where zero padding often can account for more than 90% of
the frames.

Patch 8 add some counters giving some insight into how well the
NCM/MBIM protocol works, supporting further tuning.

Patch 9 reduce the initial maximum buffer size from 32kB to 16kB
in an attempt to make the default better suit all. It is still
possible to tune this up again to the old fixed max, using the
new tuning knobs.

I must admit that I had higher hopes for this series before I
tested it on my own modems.  One really unexpected result was
that one of the MBIM modems accepted the new rx buffer size we
set, but happily continued sending buffers of the same size as
before.  Needless to say:  This did not work very well...

So don't really expect to be able to use any values with any
given device. Firmware implementations are still... I don't
think I have words suitable for a public mailing list.

But I am hoping this will help the many users who have had success
rebuilding the driver with lower fixed limits.

Please test and/or comment!"

And a short description of the 2 new patches since the RFC:

Patch 10 is a follow-up to a comment Joe Perches made in November
2013.  I don't always forget :-)

Patch 11 removes the redundant "connected" driver state, and the
associated .check_connect callbacks.


Bjørn Mork (11):
  net: cdc_ncm: split out rx_max/tx_max update of setup
  net: cdc_ncm: factor out one-time device initialization
  net: cdc_ncm: split .bind device initialization
  net: cdc_ncm: support rx_max/tx_max updates when running
  net: cdc_ncm: use ethtool to tune coalescing settings
  net: cdc_ncm: use true max dgram count for header estimates
  net: cdc_ncm: set reasonable padding limits
  net: cdc_ncm/cdc_mbim: adding NCM protocol statiscics
  net: cdc_ncm: use sane defaults for rx/tx buffers
  net: cdc_ncm: fix argument alignment
  net: cdc_ncm: remove redundant "disconnected" flag

 drivers/net/usb/cdc_mbim.c       |   6 +
 drivers/net/usb/cdc_ncm.c        | 576 ++++++++++++++++++++++++++++-----------
 drivers/net/usb/huawei_cdc_ncm.c |  13 -
 include/linux/usb/cdc_ncm.h      |  33 ++-
 4 files changed, 442 insertions(+), 186 deletions(-)

-- 
2.0.0.rc2

[PATCH net-next 01/11] net: cdc_ncm: split out rx_max/tx_max update of setup

[Bjørn Mork]

Split out the part of setup dealing with updating the rx_max
and tx_max buffer sizes so that this code can be reused for
dynamically updating the limits.

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

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 549dbac710ed..87a32edf7ea5 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -65,6 +65,54 @@ 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);
 static struct usb_driver cdc_ncm_driver;
 
+/* 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, max, min;
+
+	/* clamp new_rx to sane values */
+	min = min_t(u32, USB_CDC_NCM_NTB_MIN_IN_SIZE, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize));
+	max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_RX, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize));
+
+	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);
+	}
+
+	/* 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);
+		if (usbnet_write_cmd(dev, USB_CDC_SET_NTB_INPUT_SIZE,
+				     USB_TYPE_CLASS | USB_DIR_OUT
+				     | USB_RECIP_INTERFACE,
+				     0, iface_no, &dwNtbInMaxSize, 4) < 0)
+			dev_dbg(&dev->intf->dev, "Setting NTB Input Size failed\n");
+		else
+			ctx->rx_max = val;
+	}
+
+	/* clamp new_tx to sane values */
+	min = CDC_NCM_MIN_HDR_SIZE + ctx->max_datagram_size;
+	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. Using %u\n",
+			min, max, val);
+	}
+	if (val != ctx->tx_max)
+		dev_info(&dev->intf->dev, "setting tx_max = %u\n", val);
+	ctx->tx_max = val;
+}
+
 static int cdc_ncm_setup(struct usbnet *dev)
 {
 	struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
@@ -132,37 +180,8 @@ static int cdc_ncm_setup(struct usbnet *dev)
 			(ctx->tx_max_datagrams > CDC_NCM_DPT_DATAGRAMS_MAX))
 		ctx->tx_max_datagrams = CDC_NCM_DPT_DATAGRAMS_MAX;
 
-	/* verify maximum size of received NTB in bytes */
-	if (ctx->rx_max < USB_CDC_NCM_NTB_MIN_IN_SIZE) {
-		dev_dbg(&dev->intf->dev, "Using min receive length=%d\n",
-			USB_CDC_NCM_NTB_MIN_IN_SIZE);
-		ctx->rx_max = USB_CDC_NCM_NTB_MIN_IN_SIZE;
-	}
-
-	if (ctx->rx_max > CDC_NCM_NTB_MAX_SIZE_RX) {
-		dev_dbg(&dev->intf->dev, "Using default maximum receive length=%d\n",
-			CDC_NCM_NTB_MAX_SIZE_RX);
-		ctx->rx_max = CDC_NCM_NTB_MAX_SIZE_RX;
-	}
-
-	/* inform device about NTB input size changes */
-	if (ctx->rx_max != le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize)) {
-		__le32 dwNtbInMaxSize = cpu_to_le32(ctx->rx_max);
-
-		err = usbnet_write_cmd(dev, USB_CDC_SET_NTB_INPUT_SIZE,
-				       USB_TYPE_CLASS | USB_DIR_OUT
-				       | USB_RECIP_INTERFACE,
-				       0, iface_no, &dwNtbInMaxSize, 4);
-		if (err < 0)
-			dev_dbg(&dev->intf->dev, "Setting NTB Input Size failed\n");
-	}

Re: [PATCH net-next 01/11] net: cdc_ncm: split out rx_max/tx_max update of setup

[Oliver Neukum]

On Sat, 2014-05-10 at 17:41 +0200, Bjørn Mork wrote:
> Split out the part of setup dealing with updating the rx_max
> and tx_max buffer sizes so that this code can be reused for
> dynamically updating the limits.
> 
> Signed-off-by: Bjørn Mork <bjorn@mork.no>
> ---
>  drivers/net/usb/cdc_ncm.c | 81 +++++++++++++++++++++++++++++------------------
>  1 file changed, 50 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> index 549dbac710ed..87a32edf7ea5 100644
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -65,6 +65,54 @@ 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);
>  static struct usb_driver cdc_ncm_driver;
>  
> +/* 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, max, min;
> +
> +	/* clamp new_rx to sane values */
> +	min = min_t(u32, USB_CDC_NCM_NTB_MIN_IN_SIZE, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize));
> +	max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_RX, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize));

Are you sure this makes sense? min_t both times?

	Regards
		Oliver

Re: [PATCH net-next 01/11] net: cdc_ncm: split out rx_max/tx_max update of setup

[Bjørn Mork]

Oliver Neukum <oneukum@suse.de> writes:

> On Sat, 2014-05-10 at 17:41 +0200, Bjørn Mork wrote:
>> Split out the part of setup dealing with updating the rx_max
>> and tx_max buffer sizes so that this code can be reused for
>> dynamically updating the limits.
>> 
>> Signed-off-by: Bjørn Mork <bjorn@mork.no>
>> ---
>>  drivers/net/usb/cdc_ncm.c | 81 +++++++++++++++++++++++++++++------------------
>>  1 file changed, 50 insertions(+), 31 deletions(-)
>> 
>> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
>> index 549dbac710ed..87a32edf7ea5 100644
>> --- a/drivers/net/usb/cdc_ncm.c
>> +++ b/drivers/net/usb/cdc_ncm.c
>> @@ -65,6 +65,54 @@ 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);
>>  static struct usb_driver cdc_ncm_driver;
>>  
>> +/* 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, max, min;
>> +
>> +	/* clamp new_rx to sane values */
>> +	min = min_t(u32, USB_CDC_NCM_NTB_MIN_IN_SIZE, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize));
>> +	max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_RX, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize));
>
> Are you sure this makes sense? min_t both times?

Yes, I am sure.  At least it made sense when I wrote it.  I am more in
doubt now.

I guess you don't question the max calculation, but just so everyone
else gets the idea: dwNtbInMaxSize is the buffer size suggested by the
device. Some devices just specify an insanely large value (132kB has
been observed). So we need to cap that to CDC_NCM_NTB_MAX_SIZE_RX, which
is the absolutely largest buffer size we are prepared to support.

USB_CDC_NCM_NTB_MIN_IN_SIZE is the minimum acceptable buffer size
according to the spec. dwNtbInMaxSize is not allowed to be smaller than
this.  So if we assume that no device violates the spec, then the above
should simple be

	min = USB_CDC_NCM_NTB_MIN_IN_SIZE;
	max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_RX, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize));

which is the result for all spec conforming devices.

The reason I put that min_t() there instead was an attempt to deal with
the (not unlikely) event that some buggy device set dwNtbInMaxSize lower
than this required minimum value.  We then have the choices:

 a) fail to support the buggy device
 b) attempt to set a larger buffer size than the device supports
 c) accept the lower size

So I chose c) in an attempt to be as gentle as possible.  But I am open
to go for a) instead if you think that is better. After all
USB_CDC_NCM_NTB_MIN_IN_SIZE is as low as 2048, so it doesn't fit much
more than the headers and a single full size ethernet frame.  And I see
now that we fail to do further sanity checking after this.  What if
dwNtbInMaxSize is 0? Or smaller than the necessary headers?

Should I rewrite the above to do a) instead?  I.e.

	min = USB_CDC_NCM_NTB_MIN_IN_SIZE;
	max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_RX, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize));
        if (min > max)
           fail;

I don't think b) is a good idea.  It might work, but it might also fail
in surprising ways making it hard to debug.


Bjørn

Re: [PATCH net-next 01/11] net: cdc_ncm: split out rx_max/tx_max update of setup

[Oliver Neukum]

On Tue, 2014-05-13 at 10:49 +0200, Bjørn Mork wrote:
> Oliver Neukum <oneukum@suse.de> writes:
> 
> > On Sat, 2014-05-10 at 17:41 +0200, Bjørn Mork wrote:

> >> +	/* clamp new_rx to sane values */
> >> +	min = min_t(u32, USB_CDC_NCM_NTB_MIN_IN_SIZE, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize));
> >> +	max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_RX, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize));
> >
> > Are you sure this makes sense? min_t both times?
> 
> Yes, I am sure.  At least it made sense when I wrote it.  I am more in
> doubt now.

I actually suspected a copy n' paste error; thence the formulation.

> I guess you don't question the max calculation, but just so everyone
> else gets the idea: dwNtbInMaxSize is the buffer size suggested by the
> device. Some devices just specify an insanely large value (132kB has
> been observed). So we need to cap that to CDC_NCM_NTB_MAX_SIZE_RX, which
> is the absolutely largest buffer size we are prepared to support.

Good

> USB_CDC_NCM_NTB_MIN_IN_SIZE is the minimum acceptable buffer size
> according to the spec. dwNtbInMaxSize is not allowed to be smaller than
> this.  So if we assume that no device violates the spec, then the above
> should simple be
> 
> 	min = USB_CDC_NCM_NTB_MIN_IN_SIZE;
> 	max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_RX, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize));
> 
> which is the result for all spec conforming devices.
> 
> The reason I put that min_t() there instead was an attempt to deal with
> the (not unlikely) event that some buggy device set dwNtbInMaxSize lower
> than this required minimum value.  We then have the choices:
> 
>  a) fail to support the buggy device
>  b) attempt to set a larger buffer size than the device supports
>  c) accept the lower size

My preference would be b) > a) > c)
It seems to me that would should respect the spec and if the spec sets
a lower limit then we don't go lower.

> So I chose c) in an attempt to be as gentle as possible.  But I am open
> to go for a) instead if you think that is better. After all
> USB_CDC_NCM_NTB_MIN_IN_SIZE is as low as 2048, so it doesn't fit much
> more than the headers and a single full size ethernet frame.  And I see
> now that we fail to do further sanity checking after this.  What if
> dwNtbInMaxSize is 0? Or smaller than the necessary headers?

Exactly. Some fool may simply overlook setting it at all.

> Should I rewrite the above to do a) instead?  I.e.
> 
> 	min = USB_CDC_NCM_NTB_MIN_IN_SIZE;
> 	max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_RX, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize));
>         if (min > max)
>            fail;
> 
> I don't think b) is a good idea.  It might work, but it might also fail
> in surprising ways making it hard to debug.

Users may prefer working devices to clean failures, but
I primarily care about conforming to spec. We just shouldn't
do such violations in a general case.

	Regards
		Oliver

Re: [PATCH net-next 01/11] net: cdc_ncm: split out rx_max/tx_max update of setup

[Bjørn Mork]

Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> writes:

>> The reason I put that min_t() there instead was an attempt to deal with
>> the (not unlikely) event that some buggy device set dwNtbInMaxSize lower
>> than this required minimum value.  We then have the choices:
>> 
>>  a) fail to support the buggy device
>>  b) attempt to set a larger buffer size than the device supports
>>  c) accept the lower size
>
> My preference would be b) > a) > c)
> It seems to me that would should respect the spec and if the spec sets
> a lower limit then we don't go lower.
>
>> So I chose c) in an attempt to be as gentle as possible.  But I am open
>> to go for a) instead if you think that is better. After all
>> USB_CDC_NCM_NTB_MIN_IN_SIZE is as low as 2048, so it doesn't fit much
>> more than the headers and a single full size ethernet frame.  And I see
>> now that we fail to do further sanity checking after this.  What if
>> dwNtbInMaxSize is 0? Or smaller than the necessary headers?
>
> Exactly. Some fool may simply overlook setting it at all.
>
>> Should I rewrite the above to do a) instead?  I.e.
>> 
>> 	min = USB_CDC_NCM_NTB_MIN_IN_SIZE;
>> 	max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_RX, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize));
>>         if (min > max)
>>            fail;
>> 
>> I don't think b) is a good idea.  It might work, but it might also fail
>> in surprising ways making it hard to debug.
>
> Users may prefer working devices to clean failures, but
> I primarily care about conforming to spec. We just shouldn't
> do such violations in a general case.

Yes, I agree.  Will change this. Let's try to go for b) then. I.e.

 	min = USB_CDC_NCM_NTB_MIN_IN_SIZE;
 	max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_RX, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize));
        if (max < min)
              max = min;


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 01/11] net: cdc_ncm: split out rx_max/tx_max update of setup

[Oliver Neukum]

On Tue, 2014-05-13 at 11:25 +0200, Bjørn Mork wrote:
> Oliver Neukum <oneukum@suse.de> writes:

> Yes, I agree.  Will change this. Let's try to go for b) then. I.e.
> 
>  	min = USB_CDC_NCM_NTB_MIN_IN_SIZE;
>  	max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_RX, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize));
>         if (max < min)
>               max = min;

Thanks

	Regards
		Oliver