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

[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

[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

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