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