[PATCH 0/2] DisplayLink USB-ethernet improvements.

[PATCH 0/2] DisplayLink USB-ethernet improvements.

[Łukasz Spintzyk]

Hi all,
This are two patches to cdc_ncm driver used in our DisplayLink USB docking stations.
They are independent, however both of them are improving performance and stability so it matches Windows experience.

Please take a look.
Also I want to ask for forgiveness if there is something wrong with the contribution flow.

Regards,
Łukasz Spintzyk

Software Developer at Synaptics Inc.

Dominik Czerwik (1):
  net/cdc_ncm: Enable ZLP for DisplayLink ethernet devices

Łukasz Spintzyk (1):
  net/cdc_ncm: Add ntb_max_rx,ntb_max_tx cdc_ncm module parameters

 drivers/net/usb/cdc_ncm.c | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

-- 
2.36.1

[PATCH 1/2] net/cdc_ncm: Enable ZLP for DisplayLink ethernet devices

[Łukasz Spintzyk]

From: Dominik Czerwik <dominik.czerwik@synaptics.com>

This improves performance and stability of
DL-3xxx/DL-5xxx/DL-6xxx device series.

Signed-off-by: Dominik Czerwik <dominik.czerwik@synaptics.com>
Signed-off-by: Łukasz Spintzyk <lukasz.spintzyk@synaptics.com>
---
 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 d55f59ce4a31..4594bf2982ee 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -2,6 +2,7 @@
  * cdc_ncm.c
  *
  * Copyright (C) ST-Ericsson 2010-2012
+ * Copyright (C) 2022 Synaptics Incorporated. All rights reserved.
  * Contact: Alexey Orishko <alexey.orishko@stericsson.com>
  * Original author: Hans Petter Selasky <hans.petter.selasky@stericsson.com>
  *
@@ -1904,6 +1905,19 @@ static const struct driver_info cdc_ncm_info = {
 	.set_rx_mode = usbnet_cdc_update_filter,
 };
 
+/* Same as cdc_ncm_info, but with FLAG_SEND_ZLP  */
+static const struct driver_info cdc_ncm_zlp_info = {
+	.description = "CDC NCM (SEND ZLP)",
+	.flags = FLAG_POINTTOPOINT | FLAG_NO_SETINT | FLAG_MULTI_PACKET
+			| FLAG_LINK_INTR | FLAG_ETHER | FLAG_SEND_ZLP,
+	.bind = cdc_ncm_bind,
+	.unbind = cdc_ncm_unbind,
+	.manage_power = usbnet_manage_power,
+	.status = cdc_ncm_status,
+	.rx_fixup = cdc_ncm_rx_fixup,
+	.tx_fixup = cdc_ncm_tx_fixup,
+};
+
 /* Same as cdc_ncm_info, but with FLAG_WWAN */
 static const struct driver_info wwan_info = {
 	.description = "Mobile Broadband Network Device",
@@ -2010,6 +2024,16 @@ static const struct usb_device_id cdc_devs[] = {
 	  .driver_info = (unsigned long)&wwan_info,
 	},
 
+	/* DisplayLink docking stations */
+	{ .match_flags = USB_DEVICE_ID_MATCH_INT_INFO
+		| USB_DEVICE_ID_MATCH_VENDOR,
+	  .idVendor = 0x17e9,
+	  .bInterfaceClass = USB_CLASS_COMM,
+	  .bInterfaceSubClass = USB_CDC_SUBCLASS_NCM,
+	  .bInterfaceProtocol = USB_CDC_PROTO_NONE,
+	  .driver_info = (unsigned long)&cdc_ncm_zlp_info,
+	},
+
 	/* Generic CDC-NCM devices */
 	{ USB_INTERFACE_INFO(USB_CLASS_COMM,
 		USB_CDC_SUBCLASS_NCM, USB_CDC_PROTO_NONE),
-- 
2.36.1

[PATCH 2/2] net/cdc_ncm: Add ntb_max_rx,ntb_max_tx cdc_ncm module parameters

[Łukasz Spintzyk]

This change allows to optionally adjust maximum RX and TX NTB size
to better match specific device capabilities, leading to
higher achievable Ethernet bandwidth.

Signed-off-by: Łukasz Spintzyk <lukasz.spintzyk@synaptics.com>
---
 drivers/net/usb/cdc_ncm.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 4594bf2982ee..1ea1949c6014 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -61,6 +61,12 @@ static bool prefer_mbim;
 #endif
 module_param(prefer_mbim, bool, 0644);
 MODULE_PARM_DESC(prefer_mbim, "Prefer MBIM setting on dual NCM/MBIM functions");
+static unsigned int ntb_max_tx = CDC_NCM_NTB_MAX_SIZE_TX;
+static unsigned int ntb_max_rx = CDC_NCM_NTB_MAX_SIZE_RX;
+module_param(ntb_max_tx, uint, 0644);
+MODULE_PARM_DESC(ntb_max_rx, "Maximum allowed NTB RX size");
+module_param(ntb_max_rx, uint, 0644);
+MODULE_PARM_DESC(ntb_max_tx, "Maximum allowed NTB TX size");
 
 static void cdc_ncm_txpath_bh(struct tasklet_struct *t);
 static void cdc_ncm_tx_timeout_start(struct cdc_ncm_ctx *ctx);
@@ -154,7 +160,7 @@ static u32 cdc_ncm_check_rx_max(struct usbnet *dev, u32 new_rx)
 
 	/* clamp new_rx to sane values */
 	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));
+	max = min_t(u32, ntb_max_rx, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize));
 
 	/* dwNtbInMaxSize spec violation? Use MIN size for both limits */
 	if (max < min) {
@@ -181,7 +187,7 @@ static u32 cdc_ncm_check_tx_max(struct usbnet *dev, u32 new_tx)
 	else
 		min = ctx->max_datagram_size + ctx->max_ndp_size + sizeof(struct usb_cdc_ncm_nth32);
 
-	max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_TX, le32_to_cpu(ctx->ncm_parm.dwNtbOutMaxSize));
+	max = min_t(u32, ntb_max_tx, le32_to_cpu(ctx->ncm_parm.dwNtbOutMaxSize));
 	if (max == 0)
 		max = CDC_NCM_NTB_MAX_SIZE_TX; /* dwNtbOutMaxSize not set */
 
-- 
2.36.1

Re: [PATCH 2/2] net/cdc_ncm: Add ntb_max_rx,ntb_max_tx cdc_ncm module parameters

[Oliver Neukum]

On 13.06.22 10:02, Łukasz Spintzyk wrote:
> This change allows to optionally adjust maximum RX and TX NTB size
> to better match specific device capabilities, leading to
> higher achievable Ethernet bandwidth.
>
Hi,

this is awkward a patch. If some devices need bigger buffers, the
driver should grow its buffers for them without administrative
intervention.

	Regards
		Oliver

RE: [PATCH 2/2] net/cdc_ncm: Add ntb_max_rx,ntb_max_tx cdc_ncm module parameters

[Lukasz Spintzyk]

This Is true, 
Some of Synaptics USB eth devices require values of TX and RX NTB size higher then 32kb and this is more then CDC_NCM_NTB_MAX_SIZE_TX/RX
I wanted to be careful an not increase limit of NTB size for all devices.  But it would also by ok to me if we could increase CDC_NCM_NTB_MAX_SIZE_TX/RX to 64kb.

Regards
Lukasz


-----Original Message-----
From: Oliver Neukum <oneukum@suse.com> 
Sent: Monday, June 13, 2022 4:54 PM
To: Lukasz Spintzyk <Lukasz.Spintzyk@synaptics.com>; netdev@vger.kernel.org
Cc: PPD-POSIX <PPD-POSIX@synaptics.com>
Subject: Re: [PATCH 2/2] net/cdc_ncm: Add ntb_max_rx,ntb_max_tx cdc_ncm module parameters

CAUTION: Email originated externally, do not click links or open attachments unless you recognize the sender and know the content is safe.


On 13.06.22 10:02, Łukasz Spintzyk wrote:
> This change allows to optionally adjust maximum RX and TX NTB size to 
> better match specific device capabilities, leading to higher 
> achievable Ethernet bandwidth.
>
Hi,

this is awkward a patch. If some devices need bigger buffers, the driver should grow its buffers for them without administrative intervention.

        Regards
                Oliver

Re: [PATCH 2/2] net/cdc_ncm: Add ntb_max_rx,ntb_max_tx cdc_ncm module parameters

[Lukasz Spintzyk]

On 13/06/2022 16:54, Oliver Neukum wrote:
> CAUTION: Email originated externally, do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> 
> On 13.06.22 10:02, Łukasz Spintzyk wrote:
>> This change allows to optionally adjust maximum RX and TX NTB size
>> to better match specific device capabilities, leading to
>> higher achievable Ethernet bandwidth.
>>
> Hi,
> 
> this is awkward a patch. If some devices need bigger buffers, the
> driver should grow its buffers for them without administrative
> intervention.
> 
>          Regards
>                  Oliver
> 

This is true,
Some of DisplayLink USB ethernet devices require values of TX and RX NTB 
size higher then 32kb and this is more then defined 
CDC_NCM_NTB_MAX_SIZE_TX/RX
I wanted to be careful and not increase limit of NTB size for all 
devices. But it would also by ok to me if we could increase 
CDC_NCM_NTB_MAX_SIZE_TX/RX to 64kb.

Regards
Lukasz Spintzyk

Re: [PATCH 2/2] net/cdc_ncm: Add ntb_max_rx,ntb_max_tx cdc_ncm module parameters

[Lukasz Spintzyk]

On 14/06/2022 12:24, Lukasz Spintzyk wrote:
> On 13/06/2022 16:54, Oliver Neukum wrote:
>> CAUTION: Email originated externally, do not click links or open 
>> attachments unless you recognize the sender and know the content is safe.
>>
>>
>> On 13.06.22 10:02, Łukasz Spintzyk wrote:
>>> This change allows to optionally adjust maximum RX and TX NTB size
>>> to better match specific device capabilities, leading to
>>> higher achievable Ethernet bandwidth.
>>>
>> Hi,
>>
>> this is awkward a patch. If some devices need bigger buffers, the
>> driver should grow its buffers for them without administrative
>> intervention.
>>
>>          Regards
>>                  Oliver
>>
> 
> This is true,
> Some of DisplayLink USB ethernet devices require values of TX and RX NTB 
> size higher then 32kb and this is more then defined 
> CDC_NCM_NTB_MAX_SIZE_TX/RX
> I wanted to be careful and not increase limit of NTB size for all 
> devices. But it would also by ok to me if we could increase 
> CDC_NCM_NTB_MAX_SIZE_TX/RX to 64kb.
> 
> Regards
> Lukasz Spintzyk
This whole patch could be changed to that two line fix:

diff --git a/include/linux/usb/cdc_ncm.h b/include/linux/usb/cdc_ncm.h
index f7cb3ddce7fb..2d207cb4837d 100644
--- a/include/linux/usb/cdc_ncm.h
+++ b/include/linux/usb/cdc_ncm.h
@@ -53,8 +53,8 @@
  #define USB_CDC_NCM_NDP32_LENGTH_MIN           0x20

  /* Maximum NTB length */
-#define        CDC_NCM_NTB_MAX_SIZE_TX                 32768   /* bytes */
-#define        CDC_NCM_NTB_MAX_SIZE_RX                 32768   /* bytes */
+#define        CDC_NCM_NTB_MAX_SIZE_TX                 65536   /* bytes */
+#define        CDC_NCM_NTB_MAX_SIZE_RX                 65536   /* bytes */

  /* Initial NTB length */
  #define        CDC_NCM_NTB_DEF_SIZE_TX                 16384   /* bytes */

Olivier, would it be acceptable to increase max NTB RX/TX size to 64kb?

regards
Lukasz Spintzyk

Re: [PATCH 2/2] net/cdc_ncm: Add ntb_max_rx,ntb_max_tx cdc_ncm module parameters

[Lukasz Spintzyk]

On 14/06/2022 12:24, Lukasz Spintzyk wrote:
> On 13/06/2022 16:54, Oliver Neukum wrote:
>> CAUTION: Email originated externally, do not click links or open 
>> attachments unless you recognize the sender and know the content is safe.
>>
>>
>> On 13.06.22 10:02, Łukasz Spintzyk wrote:
>>> This change allows to optionally adjust maximum RX and TX NTB size
>>> to better match specific device capabilities, leading to
>>> higher achievable Ethernet bandwidth.
>>>
>> Hi,
>>
>> this is awkward a patch. If some devices need bigger buffers, the
>> driver should grow its buffers for them without administrative
>> intervention.
>>
>>          Regards
>>                  Oliver
>>
> 
> This is true,
> Some of DisplayLink USB ethernet devices require values of TX and RX NTB 
> size higher then 32kb and this is more then defined 
> CDC_NCM_NTB_MAX_SIZE_TX/RX
> I wanted to be careful and not increase limit of NTB size for all 
> devices. But it would also by ok to me if we could increase 
> CDC_NCM_NTB_MAX_SIZE_TX/RX to 64kb.
> 
> Regards
> Lukasz Spintzyk

Hi guys,

Is there anything I could do to proceed with that patchset?

It is improving the experience for users of millions of 
DisplayLink-based docking stations that are in the wild.
That tweaks of NTB TX/RX results in approximately 4-5x available 
bandwidth improvement in extreme cases.

I think this is worth to have it upstream.

regards
Lukasz Spintzyk

[PATCH 0/2] DisplayLink USB-ethernet improvements

[Łukasz Spintzyk]

Hi all,

I am resending that two changes again.

This are two patches to cdc_ncm driver used in our DisplayLink USB docking stations.
They are independent, however both of them are improving performance and stability so it matches Windows experience.

It is improving the experience for users of millions of DisplayLink-based docking stations that are in the wild.
That tweaks of NTB TX/RX results in approximately 4-5x available bandwidth improvement in extreme cases.


Please take a look. 

Regards,
Łukasz Spintzyk 

Software Developer at Synaptics Inc.


Dominik Czerwik (1):
  net/cdc_ncm: Enable ZLP for DisplayLink ethernet devices

Łukasz Spintzyk (1):
  net/cdc_ncm: Increase NTB max RX/TX values to 64kb

 drivers/net/usb/cdc_ncm.c   | 24 ++++++++++++++++++++++++
 include/linux/usb/cdc_ncm.h |  4 ++--
 2 files changed, 26 insertions(+), 2 deletions(-)

-- 
2.36.1

[PATCH 1/2] net/cdc_ncm: Enable ZLP for DisplayLink ethernet devices

[Łukasz Spintzyk]

From: Dominik Czerwik <dominik.czerwik@synaptics.com>

This improves performance and stability of
DL-3xxx/DL-5xxx/DL-6xxx device series.

Signed-off-by: Dominik Czerwik <dominik.czerwik@synaptics.com>
Signed-off-by: Łukasz Spintzyk <lukasz.spintzyk@synaptics.com>
---
 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 d55f59ce4a31..4594bf2982ee 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -2,6 +2,7 @@
  * cdc_ncm.c
  *
  * Copyright (C) ST-Ericsson 2010-2012
+ * Copyright (C) 2022 Synaptics Incorporated. All rights reserved.
  * Contact: Alexey Orishko <alexey.orishko@stericsson.com>
  * Original author: Hans Petter Selasky <hans.petter.selasky@stericsson.com>
  *
@@ -1904,6 +1905,19 @@ static const struct driver_info cdc_ncm_info = {
 	.set_rx_mode = usbnet_cdc_update_filter,
 };
 
+/* Same as cdc_ncm_info, but with FLAG_SEND_ZLP  */
+static const struct driver_info cdc_ncm_zlp_info = {
+	.description = "CDC NCM (SEND ZLP)",
+	.flags = FLAG_POINTTOPOINT | FLAG_NO_SETINT | FLAG_MULTI_PACKET
+			| FLAG_LINK_INTR | FLAG_ETHER | FLAG_SEND_ZLP,
+	.bind = cdc_ncm_bind,
+	.unbind = cdc_ncm_unbind,
+	.manage_power = usbnet_manage_power,
+	.status = cdc_ncm_status,
+	.rx_fixup = cdc_ncm_rx_fixup,
+	.tx_fixup = cdc_ncm_tx_fixup,
+};
+
 /* Same as cdc_ncm_info, but with FLAG_WWAN */
 static const struct driver_info wwan_info = {
 	.description = "Mobile Broadband Network Device",
@@ -2010,6 +2024,16 @@ static const struct usb_device_id cdc_devs[] = {
 	  .driver_info = (unsigned long)&wwan_info,
 	},
 
+	/* DisplayLink docking stations */
+	{ .match_flags = USB_DEVICE_ID_MATCH_INT_INFO
+		| USB_DEVICE_ID_MATCH_VENDOR,
+	  .idVendor = 0x17e9,
+	  .bInterfaceClass = USB_CLASS_COMM,
+	  .bInterfaceSubClass = USB_CDC_SUBCLASS_NCM,
+	  .bInterfaceProtocol = USB_CDC_PROTO_NONE,
+	  .driver_info = (unsigned long)&cdc_ncm_zlp_info,
+	},
+
 	/* Generic CDC-NCM devices */
 	{ USB_INTERFACE_INFO(USB_CLASS_COMM,
 		USB_CDC_SUBCLASS_NCM, USB_CDC_PROTO_NONE),
-- 
2.36.1

Re: [PATCH 1/2] net/cdc_ncm: Enable ZLP for DisplayLink ethernet devices

[Greg KH]

On Mon, Jul 04, 2022 at 09:04:06AM +0200, Łukasz Spintzyk wrote:
> From: Dominik Czerwik <dominik.czerwik@synaptics.com>
> 
> This improves performance and stability of
> DL-3xxx/DL-5xxx/DL-6xxx device series.
> 
> Signed-off-by: Dominik Czerwik <dominik.czerwik@synaptics.com>
> Signed-off-by: Łukasz Spintzyk <lukasz.spintzyk@synaptics.com>
> ---
>  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 d55f59ce4a31..4594bf2982ee 100644
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -2,6 +2,7 @@
>   * cdc_ncm.c
>   *
>   * Copyright (C) ST-Ericsson 2010-2012
> + * Copyright (C) 2022 Synaptics Incorporated. All rights reserved.

As I ask many times for other copyright additions, when making a change
like this, for such a tiny patch, I want to see a lawyer from your
company also sign off on the patch proving that they agree that this
line should be added.

thanks,

greg k-h

Re: [PATCH 1/2] net/cdc_ncm: Enable ZLP for DisplayLink ethernet devices

[Lukasz Spintzyk]

On 04/07/2022 09:25, Greg KH wrote:
> CAUTION: Email originated externally, do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> 
> On Mon, Jul 04, 2022 at 09:04:06AM +0200, Łukasz Spintzyk wrote:
>> From: Dominik Czerwik <dominik.czerwik@synaptics.com>
>>
>> This improves performance and stability of
>> DL-3xxx/DL-5xxx/DL-6xxx device series.
>>
>> Signed-off-by: Dominik Czerwik <dominik.czerwik@synaptics.com>
>> Signed-off-by: Łukasz Spintzyk <lukasz.spintzyk@synaptics.com>
>> ---
>>   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 d55f59ce4a31..4594bf2982ee 100644
>> --- a/drivers/net/usb/cdc_ncm.c
>> +++ b/drivers/net/usb/cdc_ncm.c
>> @@ -2,6 +2,7 @@
>>    * cdc_ncm.c
>>    *
>>    * Copyright (C) ST-Ericsson 2010-2012
>> + * Copyright (C) 2022 Synaptics Incorporated. All rights reserved.
> 
> As I ask many times for other copyright additions, when making a change
> like this, for such a tiny patch, I want to see a lawyer from your
> company also sign off on the patch proving that they agree that this
> line should be added.
> 
> thanks,
> 
> greg k-h

Ok, I will contact company's layer and will be back with updated patches 
if needed.
thanks
Łukasz Spintzyk