[PATCH v3 0/3] usb: gadget: f_ncm: support SuperSpeed Plus, improve on SuperSpeed

[PATCH v3 0/3] usb: gadget: f_ncm: support SuperSpeed Plus, improve on SuperSpeed

[Lorenzo Colitti]

This patch series makes the NCM gadget usable at SuperSpeed Plus
speeds (currently, it crashes with an oops). It also improves the
behaviour on SuperSpeed and above by making simple performance
improvements and by fixing the speeds that are reported to the
host (currently 851 Mbps, which is much below actual throughput).

Tested on a gadget directly connected to a Linux laptop running
5.6.14 and cdc_ncm, using both 5 Gbps and 10 Gbps cables. iperf3
single TCP connection throughput (gadget to host) is > 2 Gbps on
SuperSpeed and > 4 Gbps on SuperSpeed Plus.

Changes since v1:
- Set bMaxBurst to 15 on endpoints.
- Report more realistic speeds than 851 Mbps.

Changes since v2:
- Remove the separate SuperSpeed Plus descriptors and function
  which were just a copy of the SuperSpeed descriptors and
  function. Instead, just pass the SuperSpeed function to
  usb_assign_descriptors for both SuperSpeed and SuperSpeed
  Plus.
- Don't set bMaxBurst on the interrupt endpoint. This is
  incorrect/useless and forbidden by the spec.
- Make the speed constants unsigned literals.

[PATCH v3 1/3] usb: gadget: f_ncm: fix ncm_bitrate for SuperSpeed and above.

[Lorenzo Colitti]

Currently, SuperSpeed NCM gadgets report a speed of 851 Mbps
in USB_CDC_NOTIFY_SPEED_CHANGE. But the calculation appears to
assume 16 packets per microframe, and USB 3 and above no longer
use microframes.

Maximum speed is actually much higher. On a direct connection,
theoretical throughput is at most 3.86 Gbps for gen1x1 and
9.36 Gbps for gen2x1, and I have seen gadget->host iperf
throughput of >2 Gbps for gen1x1 and >4 Gbps for gen2x1.

Unfortunately the ConnectionSpeedChange defined in the CDC spec
only uses 32-bit values, so we can't report accurate numbers for
10Gbps and above. So, report 3.75Gbps for SuperSpeed (which is
roughly maximum theoretical performance) and 4.25Gbps for
SuperSpeed Plus (which is close to the maximum that we can report
in a 32-bit unsigned integer).

This results in:

[50879.191272] cdc_ncm 2-2:1.0 enx228b127e050c: renamed from usb0
[50879.234778] cdc_ncm 2-2:1.0 enx228b127e050c: 3750 mbit/s downlink 3750 mbit/s uplink

on SuperSpeed and:

[50798.434527] cdc_ncm 8-2:1.0 enx228b127e050c: renamed from usb0
[50798.524278] cdc_ncm 8-2:1.0 enx228b127e050c: 4250 mbit/s downlink 4250 mbit/s uplink

on SuperSpeed Plus.

Fixes: 1650113888fe ("usb: gadget: f_ncm: add SuperSpeed descriptors for CDC NCM")
Signed-off-by: Lorenzo Colitti <lorenzo@google.com>
---
 drivers/usb/gadget/function/f_ncm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
index 1d900081b1..5b9266a87f 100644
--- a/drivers/usb/gadget/function/f_ncm.c
+++ b/drivers/usb/gadget/function/f_ncm.c
@@ -85,8 +85,10 @@ static inline struct f_ncm *func_to_ncm(struct usb_function *f)
 /* peak (theoretical) bulk transfer rate in bits-per-second */
 static inline unsigned ncm_bitrate(struct usb_gadget *g)
 {
-	if (gadget_is_superspeed(g) && g->speed == USB_SPEED_SUPER)
-		return 13 * 1024 * 8 * 1000 * 8;
+	if (gadget_is_superspeed(g) && g->speed >= USB_SPEED_SUPER_PLUS)
+		return 4250000000U;
+	else if (gadget_is_superspeed(g) && g->speed == USB_SPEED_SUPER)
+		return 3750000000U;
 	else if (gadget_is_dualspeed(g) && g->speed == USB_SPEED_HIGH)
 		return 13 * 512 * 8 * 1000 * 8;
 	else
-- 
2.28.0.297.g1956fa8f8d-goog

[PATCH v3 2/3] usb: gadget: f_ncm: set SuperSpeed bulk descriptor bMaxBurst to 15

[Lorenzo Colitti]

This improves performance on fast connections. When directly
connecting to a Linux laptop running 5.6, single-stream iperf3
goes from ~1.7Gbps to ~2.3Gbps out, and from ~620Mbps to ~720Mbps
in.

Signed-off-by: Lorenzo Colitti <lorenzo@google.com>
---
 drivers/usb/gadget/function/f_ncm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
index 5b9266a87f..cad3c9dd61 100644
--- a/drivers/usb/gadget/function/f_ncm.c
+++ b/drivers/usb/gadget/function/f_ncm.c
@@ -378,7 +378,7 @@ static struct usb_ss_ep_comp_descriptor ss_ncm_bulk_comp_desc = {
 	.bDescriptorType =	USB_DT_SS_ENDPOINT_COMP,
 
 	/* the following 2 values can be tweaked if necessary */
-	/* .bMaxBurst =		0, */
+	.bMaxBurst =		15,
 	/* .bmAttributes =	0, */
 };
 
-- 
2.28.0.297.g1956fa8f8d-goog

[PATCH v3 3/3] usb: gadget: f_ncm: allow using NCM in SuperSpeed Plus gadgets.

[Lorenzo Colitti]

Currently, enabling f_ncm at SuperSpeed Plus speeds results in an
oops in config_ep_by_speed because ncm_set_alt passes in NULL
ssp_descriptors. Fix this by re-using the SuperSpeed descriptors.
This is safe because usb_assign_descriptors calls
usb_copy_descriptors.

Tested: enabled f_ncm on a dwc3 gadget and 10Gbps link, ran iperf
Signed-off-by: Lorenzo Colitti <lorenzo@google.com>
---
 drivers/usb/gadget/function/f_ncm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
index cad3c9dd61..87f548bd97 100644
--- a/drivers/usb/gadget/function/f_ncm.c
+++ b/drivers/usb/gadget/function/f_ncm.c
@@ -1505,7 +1505,7 @@ static int ncm_bind(struct usb_configuration *c, struct usb_function *f)
 		fs_ncm_notify_desc.bEndpointAddress;
 
 	status = usb_assign_descriptors(f, ncm_fs_function, ncm_hs_function,
-			ncm_ss_function, NULL);
+			ncm_ss_function, ncm_ss_function);
 	if (status)
 		goto fail;
 
-- 
2.28.0.297.g1956fa8f8d-goog

Re: [PATCH v3 1/3] usb: gadget: f_ncm: fix ncm_bitrate for SuperSpeed and above.

[Maciej Żenczykowski]

On Mon, Aug 24, 2020 at 10:55 PM Lorenzo Colitti <lorenzo@google.com> wrote:
>
> Currently, SuperSpeed NCM gadgets report a speed of 851 Mbps
> in USB_CDC_NOTIFY_SPEED_CHANGE. But the calculation appears to
> assume 16 packets per microframe, and USB 3 and above no longer
> use microframes.
>
> Maximum speed is actually much higher. On a direct connection,
> theoretical throughput is at most 3.86 Gbps for gen1x1 and
> 9.36 Gbps for gen2x1, and I have seen gadget->host iperf
> throughput of >2 Gbps for gen1x1 and >4 Gbps for gen2x1.
>
> Unfortunately the ConnectionSpeedChange defined in the CDC spec
> only uses 32-bit values, so we can't report accurate numbers for
> 10Gbps and above. So, report 3.75Gbps for SuperSpeed (which is
> roughly maximum theoretical performance) and 4.25Gbps for
> SuperSpeed Plus (which is close to the maximum that we can report
> in a 32-bit unsigned integer).
>
> This results in:
>
> [50879.191272] cdc_ncm 2-2:1.0 enx228b127e050c: renamed from usb0
> [50879.234778] cdc_ncm 2-2:1.0 enx228b127e050c: 3750 mbit/s downlink 3750 mbit/s uplink
>
> on SuperSpeed and:
>
> [50798.434527] cdc_ncm 8-2:1.0 enx228b127e050c: renamed from usb0
> [50798.524278] cdc_ncm 8-2:1.0 enx228b127e050c: 4250 mbit/s downlink 4250 mbit/s uplink
>
> on SuperSpeed Plus.
>
> Fixes: 1650113888fe ("usb: gadget: f_ncm: add SuperSpeed descriptors for CDC NCM")
> Signed-off-by: Lorenzo Colitti <lorenzo@google.com>
> ---
>  drivers/usb/gadget/function/f_ncm.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
> index 1d900081b1..5b9266a87f 100644
> --- a/drivers/usb/gadget/function/f_ncm.c
> +++ b/drivers/usb/gadget/function/f_ncm.c
> @@ -85,8 +85,10 @@ static inline struct f_ncm *func_to_ncm(struct usb_function *f)
>  /* peak (theoretical) bulk transfer rate in bits-per-second */
>  static inline unsigned ncm_bitrate(struct usb_gadget *g)
>  {
> -       if (gadget_is_superspeed(g) && g->speed == USB_SPEED_SUPER)
> -               return 13 * 1024 * 8 * 1000 * 8;
> +       if (gadget_is_superspeed(g) && g->speed >= USB_SPEED_SUPER_PLUS)
> +               return 4250000000U;
> +       else if (gadget_is_superspeed(g) && g->speed == USB_SPEED_SUPER)
> +               return 3750000000U;
>         else if (gadget_is_dualspeed(g) && g->speed == USB_SPEED_HIGH)
>                 return 13 * 512 * 8 * 1000 * 8;
>         else
> --
> 2.28.0.297.g1956fa8f8d-goog
>

Reviewed-by: Maciej Żenczykowski <maze@google.com>

Re: [PATCH v3 3/3] usb: gadget: f_ncm: allow using NCM in SuperSpeed Plus gadgets.

[Maciej Żenczykowski]

On Mon, Aug 24, 2020 at 10:55 PM Lorenzo Colitti <lorenzo@google.com> wrote:
>
> Currently, enabling f_ncm at SuperSpeed Plus speeds results in an
> oops in config_ep_by_speed because ncm_set_alt passes in NULL
> ssp_descriptors. Fix this by re-using the SuperSpeed descriptors.
> This is safe because usb_assign_descriptors calls
> usb_copy_descriptors.
>
> Tested: enabled f_ncm on a dwc3 gadget and 10Gbps link, ran iperf
> Signed-off-by: Lorenzo Colitti <lorenzo@google.com>
> ---
>  drivers/usb/gadget/function/f_ncm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
> index cad3c9dd61..87f548bd97 100644
> --- a/drivers/usb/gadget/function/f_ncm.c
> +++ b/drivers/usb/gadget/function/f_ncm.c
> @@ -1505,7 +1505,7 @@ static int ncm_bind(struct usb_configuration *c, struct usb_function *f)
>                 fs_ncm_notify_desc.bEndpointAddress;
>
>         status = usb_assign_descriptors(f, ncm_fs_function, ncm_hs_function,
> -                       ncm_ss_function, NULL);
> +                       ncm_ss_function, ncm_ss_function);
>         if (status)
>                 goto fail;
>
> --
> 2.28.0.297.g1956fa8f8d-goog


Reviewed-by: Maciej Żenczykowski <maze@google.com>

Re: [PATCH v3 0/3] usb: gadget: f_ncm: support SuperSpeed Plus, improve on SuperSpeed

[Greg KH]

On Tue, Aug 25, 2020 at 02:55:02PM +0900, Lorenzo Colitti wrote:
> This patch series makes the NCM gadget usable at SuperSpeed Plus
> speeds (currently, it crashes with an oops). It also improves the
> behaviour on SuperSpeed and above by making simple performance
> improvements and by fixing the speeds that are reported to the
> host (currently 851 Mbps, which is much below actual throughput).
> 
> Tested on a gadget directly connected to a Linux laptop running
> 5.6.14 and cdc_ncm, using both 5 Gbps and 10 Gbps cables. iperf3
> single TCP connection throughput (gadget to host) is > 2 Gbps on
> SuperSpeed and > 4 Gbps on SuperSpeed Plus.
> 
> Changes since v1:
> - Set bMaxBurst to 15 on endpoints.
> - Report more realistic speeds than 851 Mbps.
> 
> Changes since v2:
> - Remove the separate SuperSpeed Plus descriptors and function
>   which were just a copy of the SuperSpeed descriptors and
>   function. Instead, just pass the SuperSpeed function to
>   usb_assign_descriptors for both SuperSpeed and SuperSpeed
>   Plus.
> - Don't set bMaxBurst on the interrupt endpoint. This is
>   incorrect/useless and forbidden by the spec.
> - Make the speed constants unsigned literals.
> 
> 

Felipe, did you miss this series, or is it still in your to-review queue
you are working your way through?

thanks,

greg k-h

Re: [PATCH v3 0/3] usb: gadget: f_ncm: support SuperSpeed Plus, improve on SuperSpeed

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1474 bytes --]


Hi Greg,

Greg KH <gregkh@linuxfoundation.org> writes:
> On Tue, Aug 25, 2020 at 02:55:02PM +0900, Lorenzo Colitti wrote:
>> This patch series makes the NCM gadget usable at SuperSpeed Plus
>> speeds (currently, it crashes with an oops). It also improves the
>> behaviour on SuperSpeed and above by making simple performance
>> improvements and by fixing the speeds that are reported to the
>> host (currently 851 Mbps, which is much below actual throughput).
>> 
>> Tested on a gadget directly connected to a Linux laptop running
>> 5.6.14 and cdc_ncm, using both 5 Gbps and 10 Gbps cables. iperf3
>> single TCP connection throughput (gadget to host) is > 2 Gbps on
>> SuperSpeed and > 4 Gbps on SuperSpeed Plus.
>> 
>> Changes since v1:
>> - Set bMaxBurst to 15 on endpoints.
>> - Report more realistic speeds than 851 Mbps.
>> 
>> Changes since v2:
>> - Remove the separate SuperSpeed Plus descriptors and function
>>   which were just a copy of the SuperSpeed descriptors and
>>   function. Instead, just pass the SuperSpeed function to
>>   usb_assign_descriptors for both SuperSpeed and SuperSpeed
>>   Plus.
>> - Don't set bMaxBurst on the interrupt endpoint. This is
>>   incorrect/useless and forbidden by the spec.
>> - Make the speed constants unsigned literals.
>> 
>> 
>
> Felipe, did you miss this series, or is it still in your to-review queue
> you are working your way through?

Just got to this one :-)


-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

Re: [PATCH v3 1/3] usb: gadget: f_ncm: fix ncm_bitrate for SuperSpeed and above.

[Sid Spry]

On Tue, Aug 25, 2020, at 12:55 AM, Lorenzo Colitti wrote:
> Currently, SuperSpeed NCM gadgets report a speed of 851 Mbps

I apologize for hijacking your patch message, but on what hardware
are you testing? I've been trying to find USB3 device-mode-capable
hardware for ages.

Re: [PATCH v3 1/3] usb: gadget: f_ncm: fix ncm_bitrate for SuperSpeed and above.

[Lorenzo Colitti]

On Tue, Sep 29, 2020 at 11:52 AM Sid Spry <sid@aeam.us> wrote:
> I apologize for hijacking your patch message, but on what hardware
> are you testing? I've been trying to find USB3 device-mode-capable
> hardware for ages.

I'm using a rooted Pixel phone. Not great as a development platform though.

I think my colleague Maciej (CCd) has a better option involving a
developer board.

Re: [PATCH v3 1/3] usb: gadget: f_ncm: fix ncm_bitrate for SuperSpeed and above.

[Maciej Żenczykowski]

On Mon, Sep 28, 2020 at 9:30 PM Lorenzo Colitti <lorenzo@google.com> wrote:
> On Tue, Sep 29, 2020 at 11:52 AM Sid Spry <sid@aeam.us> wrote:
> > I apologize for hijacking your patch message, but on what hardware
> > are you testing? I've been trying to find USB3 device-mode-capable
> > hardware for ages.
>
> I'm using a rooted Pixel phone. Not great as a development platform though.
>
> I think my colleague Maciej (CCd) has a better option involving a
> developer board.

I spent some time trying to figure out how to test gadget stuff on x86
with recent kernels.
This is so that I don't have to futz about with all sorts of out of
tree patches and crazy arm dev boards that usually don't just work
with vanilla kernels.
I also wanted to use x86 because I understand the x86 memory
model/assembly/etc a lot better, and can directly compare against
'standard' server grade nics (like the Ethernet controller [0200]:
Intel Corporation Ethernet Controller 10G X550T [8086:1563]) and
various other 1/2.5/5gbps ethernet dongles.

This is all still very much a work in progress...

I found two kind-of solutions:

(a) USB3.1 10gbps

https://www.datapro.net/products/usb-3-0-super-speed-a-a-debugging-cable.html

Notes:
- this cost 14.95$ + 9.50$ S&H = 24.45$ (to the bay area)
- my understanding is the debug cable does not include the USB2 data
connection that a normal USB3 cable would have
- this allows connecting two USB3 A ports to each other (or I guess
USB-C ports with C2A adapters).
  (Maybe a C-C cable or C-A cable would also work, not sure? I don't
remember what I already tried...)
- AFAIK the longer cables won't maintain 10gbps speeds, the shortest
<1m one should be able to on a good day

This is not a real 'gadget' solution... but could probably be made
into one with 'just' some kernel code (any takers?).
It probably should be sufficient to enable some sort of decent
networking over it.

Here's how it works:

# lspci -nn | egrep -i USB
00:14.0 USB controller [0c03]: Intel Corporation Cannon Lake PCH USB
3.1 xHCI Host Controller [8086:a36d] (rev 10)

# cat /sys/bus/pci/devices/0000:00:14.0/dbc
disabled

# echo enable > /sys/bus/pci/devices/0000:00:14.0/dbc

# cat /sys/bus/pci/devices/0000:00:14.0/dbc
configured
(due to previously plugged in cable it immediately moves to
'configured' instead of 'enabled')

Related dmesg:
...(lots of spewing about bad cable prior to dbc enablement)...
[765163.777035] usb usb2-port4: Cannot enable. Maybe the USB cable is bad?
[765163.777094] usb usb2-port3: config error
[765167.833012] usb usb2-port3: Cannot enable. Maybe the USB cable is bad?
[765167.833063] usb usb2-port4: config error
[765167.941988] xhci_hcd 0000:00:14.0: DbC connected   (this is where
the above echo happened)
[765168.561094] usb usb2-port3: config error
[765168.783042] usb 2-3: new SuperSpeed Gen 1 USB device number 3 using xhci_hcd
[765168.795266] usb 2-3: LPM exit latency is zeroed, disabling LPM.
[765168.795506] usb 2-3: New USB device found, idVendor=1d6b,
idProduct=0010, bcdDevice= 0.10
[765168.795515] usb 2-3: New USB device strings: Mfr=1, Product=2,
SerialNumber=3
[765168.795522] usb 2-3: Product: Linux USB Debug Target
[765168.795527] usb 2-3: Manufacturer: Linux Foundation
[765168.795532] usb 2-3: SerialNumber: 0001
[765168.797997] xhci_hcd 0000:00:14.0: DbC configured
[765168.798300] xhci_hcd 0000:00:14.0: DbC now attached to /dev/ttyDBC0
[765168.804804] usbcore: registered new interface driver usb_debug
[765168.804810] usbserial: USB Serial support registered for debug
[765168.804814] usbserial: USB Serial support registered for xhci_dbc
[765168.804823] usb_debug 2-3:1.0: xhci_dbc converter detected
[765168.804877] usb 2-3: xhci_dbc converter now attached to ttyUSB0
(I think what would normally be 2-4 ended up being the 'device' side
and 2-3 the 'host' side, but both ports are on the same pci device
which makes this more confusing)

echo hi > /dev/ttyDBC0; cat /dev/ttyUSB0
hi

# lsusb -t
/:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/10p, 10000M
    |__ Port 3: Dev 3, If 0, Class=Diagnostic, Driver=usb_debug, 5000M

(so it connected at 5gbps, I think I've seen it connect at 10gbps as
well, though using a pair of computers instead of loopback on one...)

There's some docs at Documentation/driver-api/usb/usb3-debug-port.rst,
and it looks like it might be possible to adjust this to be a more
generic usb3-only gadget...

drivers/usb/host/xhci-dbgcap.c - seems to have a lot of the
configuration hardcoded, but in ways that make it seem like it might
be possible to use it in a more generic fashion...

(b) USB3.0 5gbps

http://www.hwtools.net/Adapter/USB3380EVB.html  (not the RC 'root
complex' version)
http://www.bplus.com.tw/Adapter/USB3380EVB.html  [156$ + 45$ S&H to
bay area = 201$ USD total via paypal]

This is a gadget capable usb3 capable chipset in a x86 compatible form
factor with linux gadget drivers (USB_NET2280 Kconfig option).
It shows up as:
02:00.0 USB controller [0c03]: PLX Technology, Inc. Device [10b5:3380] (rev ab)

They don't appear to have any of the nice pcie adapters that expose
the port available (in stock) any more.

The following:
  https://www.startech.com/en-us/cards-adapters/pex2mpex
works, but you end up with a port inside your case...

I didn't have a free pcie slot on my motherboard, so I ended up going
via an on-the-motherboard SSD slot.
You need to have an NGFF(M.2) socket with the key in the 'M' position,
so it can serve as a mPCIe x4 slot.

Then a https://www.amazon.com/gp/product/B08D3N2284/ref=ppx_yo_dt_b_asin_title_o00_s00?ie=UTF8&psc=1
converter works.
(presumably https://www.amazon.com/gp/product/B07BWQTQG2/ref=ppx_yo_dt_b_asin_title_o01_s00?ie=UTF8&psc=1
+ the above pex2mpex adapter would also work, but why bother...)

With this approach I'm seeing pci errors:

[768796.304490] pcieport 0000:00:1b.0: AER: Corrected error received:
0000:00:1b.0
[768796.304495] pcieport 0000:00:1b.0: AER: PCIe Bus Error:
severity=Corrected, type=Data Link Layer, (Transmitter ID)
[768796.304496] pcieport 0000:00:1b.0: AER:   device [8086:a340] error
status/mask=00001000/00002000
[768796.304497] pcieport 0000:00:1b.0: AER:    [12] Timeout

but I do get:

[768797.724834] usb 2-7: new SuperSpeed Gen 1 USB device number 8 using xhci_hcd

--

Not sure how much help this will be.  I'm still poking at it, this is
a relatively fresh setup, I'm trying to run both device and host sides
as Fedora VMs (on a Fedora host) with KVM & VFIO and running into all
sorts of unrelated problems (like VM kernel not finding rootfs and
panicking).