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

[PATCH v2 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 about 3.86 Gbps for gen1x1 and
9.35 Gbps for gen2x1, and I have seen gadget->host speeds
>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 always report a speed of 4 Gbps, which is
close enough to the technical maximum of a 5 Gbps link.

This results in:

[96033.958723] cdc_ncm 8-2:1.0 enx4643f5db6f40: renamed from usb0
[96033.997136] cdc_ncm 8-2:1.0 enx4643f5db6f40: 4000 mbit/s downlink 4000 mbit/s uplink

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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
index 1d900081b1..0c073df225 100644
--- a/drivers/usb/gadget/function/f_ncm.c
+++ b/drivers/usb/gadget/function/f_ncm.c
@@ -85,8 +85,8 @@ 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)
+		return 4000000000;
 	else if (gadget_is_dualspeed(g) && g->speed == USB_SPEED_HIGH)
 		return 13 * 512 * 8 * 1000 * 8;
 	else
-- 
2.28.0.220.ged08abb693-goog

[PATCH v2 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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
index 0c073df225..57ccf30c6c 100644
--- a/drivers/usb/gadget/function/f_ncm.c
+++ b/drivers/usb/gadget/function/f_ncm.c
@@ -348,7 +348,7 @@ static struct usb_ss_ep_comp_descriptor ss_ncm_notify_comp_desc = {
 	.bDescriptorType =	USB_DT_SS_ENDPOINT_COMP,
 
 	/* the following 3 values can be tweaked if necessary */
-	/* .bMaxBurst =		0, */
+	.bMaxBurst =		15,
 	/* .bmAttributes =	0, */
 	.wBytesPerInterval =	cpu_to_le16(NCM_STATUS_BYTECOUNT),
 };
@@ -376,7 +376,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.220.ged08abb693-goog

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

[Lorenzo Colitti]

Currently, using f_ncm in a SuperSpeed Plus gadget results in
an oops in config_ep_by_speed because ncm_set_alt passes in NULL
ssp_descriptors. Fix this by defining new descriptors for
SuperSpeed Plus. (We cannot re-use the existing definitions for
the SuperSpeed descriptors, even though they are mostly the same,
because they are not fixed initializers).

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 | 76 ++++++++++++++++++++++++++++-
 1 file changed, 75 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
index 57ccf30c6c..01242454d5 100644
--- a/drivers/usb/gadget/function/f_ncm.c
+++ b/drivers/usb/gadget/function/f_ncm.c
@@ -400,6 +400,75 @@ static struct usb_descriptor_header *ncm_ss_function[] = {
 	NULL,
 };
 
+/* super speed plus support: */
+
+static struct usb_endpoint_descriptor ssp_ncm_notify_desc = {
+	.bLength =		USB_DT_ENDPOINT_SIZE,
+	.bDescriptorType =	USB_DT_ENDPOINT,
+
+	.bEndpointAddress =	USB_DIR_IN,
+	.bmAttributes =		USB_ENDPOINT_XFER_INT,
+	.wMaxPacketSize =	cpu_to_le16(NCM_STATUS_BYTECOUNT),
+	.bInterval =		USB_MS_TO_HS_INTERVAL(NCM_STATUS_INTERVAL_MS)
+};
+
+static struct usb_ss_ep_comp_descriptor ssp_ncm_notify_comp_desc = {
+	.bLength =		sizeof(ssp_ncm_notify_comp_desc),
+	.bDescriptorType =	USB_DT_SS_ENDPOINT_COMP,
+
+	/* the following 3 values can be tweaked if necessary */
+	.bMaxBurst =		15,
+	/* .bmAttributes =	0, */
+	.wBytesPerInterval =	cpu_to_le16(NCM_STATUS_BYTECOUNT),
+};
+
+static struct usb_endpoint_descriptor ssp_ncm_in_desc = {
+	.bLength =		USB_DT_ENDPOINT_SIZE,
+	.bDescriptorType =	USB_DT_ENDPOINT,
+
+	.bEndpointAddress =	USB_DIR_IN,
+	.bmAttributes =		USB_ENDPOINT_XFER_BULK,
+	.wMaxPacketSize =	cpu_to_le16(1024),
+};
+
+static struct usb_endpoint_descriptor ssp_ncm_out_desc = {
+	.bLength =		USB_DT_ENDPOINT_SIZE,
+	.bDescriptorType =	USB_DT_ENDPOINT,
+
+	.bEndpointAddress =	USB_DIR_OUT,
+	.bmAttributes =		USB_ENDPOINT_XFER_BULK,
+	.wMaxPacketSize =	cpu_to_le16(1024),
+};
+
+static struct usb_ss_ep_comp_descriptor ssp_ncm_bulk_comp_desc = {
+	.bLength =		sizeof(ssp_ncm_bulk_comp_desc),
+	.bDescriptorType =	USB_DT_SS_ENDPOINT_COMP,
+
+	/* the following 2 values can be tweaked if necessary */
+	.bMaxBurst =		15,
+	/* .bmAttributes =	0, */
+};
+
+static struct usb_descriptor_header *ncm_ssp_function[] = {
+	(struct usb_descriptor_header *) &ncm_iad_desc,
+	/* CDC NCM control descriptors */
+	(struct usb_descriptor_header *) &ncm_control_intf,
+	(struct usb_descriptor_header *) &ncm_header_desc,
+	(struct usb_descriptor_header *) &ncm_union_desc,
+	(struct usb_descriptor_header *) &ecm_desc,
+	(struct usb_descriptor_header *) &ncm_desc,
+	(struct usb_descriptor_header *) &ssp_ncm_notify_desc,
+	(struct usb_descriptor_header *) &ssp_ncm_notify_comp_desc,
+	/* data interface, altsettings 0 and 1 */
+	(struct usb_descriptor_header *) &ncm_data_nop_intf,
+	(struct usb_descriptor_header *) &ncm_data_intf,
+	(struct usb_descriptor_header *) &ssp_ncm_in_desc,
+	(struct usb_descriptor_header *) &ssp_ncm_bulk_comp_desc,
+	(struct usb_descriptor_header *) &ssp_ncm_out_desc,
+	(struct usb_descriptor_header *) &ssp_ncm_bulk_comp_desc,
+	NULL,
+};
+
 /* string descriptors: */
 
 #define STRING_CTRL_IDX	0
@@ -1502,8 +1571,13 @@ static int ncm_bind(struct usb_configuration *c, struct usb_function *f)
 	ss_ncm_notify_desc.bEndpointAddress =
 		fs_ncm_notify_desc.bEndpointAddress;
 
+	ssp_ncm_in_desc.bEndpointAddress = fs_ncm_in_desc.bEndpointAddress;
+	ssp_ncm_out_desc.bEndpointAddress = fs_ncm_out_desc.bEndpointAddress;
+	ssp_ncm_notify_desc.bEndpointAddress =
+		fs_ncm_notify_desc.bEndpointAddress;
+
 	status = usb_assign_descriptors(f, ncm_fs_function, ncm_hs_function,
-			ncm_ss_function, NULL);
+			ncm_ss_function, ncm_ssp_function);
 	if (status)
 		goto fail;
 
-- 
2.28.0.220.ged08abb693-goog

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

[Maciej Żenczykowski]

On Tue, Aug 18, 2020 at 9:59 AM 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 about 3.86 Gbps for gen1x1 and
> 9.35 Gbps for gen2x1, and I have seen gadget->host speeds
> >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 always report a speed of 4 Gbps, which is
> close enough to the technical maximum of a 5 Gbps link.
>
> This results in:
>
> [96033.958723] cdc_ncm 8-2:1.0 enx4643f5db6f40: renamed from usb0
> [96033.997136] cdc_ncm 8-2:1.0 enx4643f5db6f40: 4000 mbit/s downlink 4000 mbit/s uplink
>
> 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 | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
> index 1d900081b1..0c073df225 100644
> --- a/drivers/usb/gadget/function/f_ncm.c
> +++ b/drivers/usb/gadget/function/f_ncm.c
> @@ -85,8 +85,8 @@ 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)
> +               return 4000000000;
>         else if (gadget_is_dualspeed(g) && g->speed == USB_SPEED_HIGH)
>                 return 13 * 512 * 8 * 1000 * 8;
>         else
> --
> 2.28.0.220.ged08abb693-goog
>

Do you know what this actually affects besides the display?
My cursory investigation shows it gets printed to kernel log and sent
over some sort of ncm notification to the other side...

Is it better to underestimate or overestimate?
(ie. would it be better to report 3.5 gbps for super and max out at
4.2 gbps for super plus instead?)

However, since this is obviously better than the utterly bogus 851,968,000:

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

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

[Maciej Żenczykowski]

On Tue, Aug 18, 2020 at 9:59 AM Lorenzo Colitti <lorenzo@google.com> wrote:
>
> 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 | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
> index 0c073df225..57ccf30c6c 100644
> --- a/drivers/usb/gadget/function/f_ncm.c
> +++ b/drivers/usb/gadget/function/f_ncm.c
> @@ -348,7 +348,7 @@ static struct usb_ss_ep_comp_descriptor ss_ncm_notify_comp_desc = {
>         .bDescriptorType =      USB_DT_SS_ENDPOINT_COMP,
>
>         /* the following 3 values can be tweaked if necessary */
> -       /* .bMaxBurst =         0, */
> +       .bMaxBurst =            15,
>         /* .bmAttributes =      0, */
>         .wBytesPerInterval =    cpu_to_le16(NCM_STATUS_BYTECOUNT),
>  };
> @@ -376,7 +376,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.220.ged08abb693-goog
>

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

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

[Maciej Żenczykowski]

I'm very confused by the location of the MaxBurst parameter.
All the dongles I've looked at seem to place MaxBurst just after MaxPacketSize,
and not in a separate descriptor (and don't place burst on the
status/config channel).

wMaxPacketSize     0x0400  1x 1024 bytes
bInterval               0
bMaxBurst               3

What does "lsusb -d ... -v" show from the host side?

Maybe things are pretty misconfigured?  Or maybe the dongles don't
match the spec...

On Tue, Aug 18, 2020 at 9:59 AM Lorenzo Colitti <lorenzo@google.com> wrote:
>
> Currently, using f_ncm in a SuperSpeed Plus gadget results in
> an oops in config_ep_by_speed because ncm_set_alt passes in NULL
> ssp_descriptors. Fix this by defining new descriptors for
> SuperSpeed Plus. (We cannot re-use the existing definitions for
> the SuperSpeed descriptors, even though they are mostly the same,
> because they are not fixed initializers).
>
> 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 | 76 ++++++++++++++++++++++++++++-
>  1 file changed, 75 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
> index 57ccf30c6c..01242454d5 100644
> --- a/drivers/usb/gadget/function/f_ncm.c
> +++ b/drivers/usb/gadget/function/f_ncm.c
> @@ -400,6 +400,75 @@ static struct usb_descriptor_header *ncm_ss_function[] = {
>         NULL,
>  };
>
> +/* super speed plus support: */
> +
> +static struct usb_endpoint_descriptor ssp_ncm_notify_desc = {
> +       .bLength =              USB_DT_ENDPOINT_SIZE,
> +       .bDescriptorType =      USB_DT_ENDPOINT,
> +
> +       .bEndpointAddress =     USB_DIR_IN,
> +       .bmAttributes =         USB_ENDPOINT_XFER_INT,
> +       .wMaxPacketSize =       cpu_to_le16(NCM_STATUS_BYTECOUNT),
> +       .bInterval =            USB_MS_TO_HS_INTERVAL(NCM_STATUS_INTERVAL_MS)
> +};
> +
> +static struct usb_ss_ep_comp_descriptor ssp_ncm_notify_comp_desc = {
> +       .bLength =              sizeof(ssp_ncm_notify_comp_desc),
> +       .bDescriptorType =      USB_DT_SS_ENDPOINT_COMP,
> +
> +       /* the following 3 values can be tweaked if necessary */
> +       .bMaxBurst =            15,
> +       /* .bmAttributes =      0, */
> +       .wBytesPerInterval =    cpu_to_le16(NCM_STATUS_BYTECOUNT),
> +};
> +
> +static struct usb_endpoint_descriptor ssp_ncm_in_desc = {
> +       .bLength =              USB_DT_ENDPOINT_SIZE,
> +       .bDescriptorType =      USB_DT_ENDPOINT,
> +
> +       .bEndpointAddress =     USB_DIR_IN,
> +       .bmAttributes =         USB_ENDPOINT_XFER_BULK,
> +       .wMaxPacketSize =       cpu_to_le16(1024),
> +};
> +
> +static struct usb_endpoint_descriptor ssp_ncm_out_desc = {
> +       .bLength =              USB_DT_ENDPOINT_SIZE,
> +       .bDescriptorType =      USB_DT_ENDPOINT,
> +
> +       .bEndpointAddress =     USB_DIR_OUT,
> +       .bmAttributes =         USB_ENDPOINT_XFER_BULK,
> +       .wMaxPacketSize =       cpu_to_le16(1024),
> +};
> +
> +static struct usb_ss_ep_comp_descriptor ssp_ncm_bulk_comp_desc = {
> +       .bLength =              sizeof(ssp_ncm_bulk_comp_desc),
> +       .bDescriptorType =      USB_DT_SS_ENDPOINT_COMP,
> +
> +       /* the following 2 values can be tweaked if necessary */
> +       .bMaxBurst =            15,
> +       /* .bmAttributes =      0, */
> +};
> +
> +static struct usb_descriptor_header *ncm_ssp_function[] = {
> +       (struct usb_descriptor_header *) &ncm_iad_desc,
> +       /* CDC NCM control descriptors */
> +       (struct usb_descriptor_header *) &ncm_control_intf,
> +       (struct usb_descriptor_header *) &ncm_header_desc,
> +       (struct usb_descriptor_header *) &ncm_union_desc,
> +       (struct usb_descriptor_header *) &ecm_desc,
> +       (struct usb_descriptor_header *) &ncm_desc,
> +       (struct usb_descriptor_header *) &ssp_ncm_notify_desc,
> +       (struct usb_descriptor_header *) &ssp_ncm_notify_comp_desc,
> +       /* data interface, altsettings 0 and 1 */
> +       (struct usb_descriptor_header *) &ncm_data_nop_intf,
> +       (struct usb_descriptor_header *) &ncm_data_intf,
> +       (struct usb_descriptor_header *) &ssp_ncm_in_desc,
> +       (struct usb_descriptor_header *) &ssp_ncm_bulk_comp_desc,
> +       (struct usb_descriptor_header *) &ssp_ncm_out_desc,
> +       (struct usb_descriptor_header *) &ssp_ncm_bulk_comp_desc,
> +       NULL,
> +};
> +
>  /* string descriptors: */
>
>  #define STRING_CTRL_IDX        0
> @@ -1502,8 +1571,13 @@ static int ncm_bind(struct usb_configuration *c, struct usb_function *f)
>         ss_ncm_notify_desc.bEndpointAddress =
>                 fs_ncm_notify_desc.bEndpointAddress;
>
> +       ssp_ncm_in_desc.bEndpointAddress = fs_ncm_in_desc.bEndpointAddress;
> +       ssp_ncm_out_desc.bEndpointAddress = fs_ncm_out_desc.bEndpointAddress;
> +       ssp_ncm_notify_desc.bEndpointAddress =
> +               fs_ncm_notify_desc.bEndpointAddress;
> +
>         status = usb_assign_descriptors(f, ncm_fs_function, ncm_hs_function,
> -                       ncm_ss_function, NULL);
> +                       ncm_ss_function, ncm_ssp_function);
>         if (status)
>                 goto fail;
>
> --
> 2.28.0.220.ged08abb693-goog
>

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

[kernel test robot]

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

Hi Lorenzo,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on balbi-usb/testing/next]
[also build test WARNING on usb/usb-testing peter.chen-usb/ci-for-usb-next v5.9-rc1 next-20200818]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Lorenzo-Colitti/usb-gadget-f_ncm-fix-ncm_bitrate-for-SuperSpeed-and-above/20200819-010231
base:   https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git testing/next
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/linux/kernel.h:11,
                    from drivers/usb/gadget/function/f_ncm.c:14:
   include/linux/scatterlist.h: In function 'sg_set_buf':
   arch/m68k/include/asm/page_mm.h:169:49: warning: ordered comparison of pointer with null pointer [-Wextra]
     169 | #define virt_addr_valid(kaddr) ((void *)(kaddr) >= (void *)PAGE_OFFSET && (void *)(kaddr) < high_memory)
         |                                                 ^~
   include/linux/compiler.h:78:42: note: in definition of macro 'unlikely'
      78 | # define unlikely(x) __builtin_expect(!!(x), 0)
         |                                          ^
   include/linux/scatterlist.h:143:2: note: in expansion of macro 'BUG_ON'
     143 |  BUG_ON(!virt_addr_valid(buf));
         |  ^~~~~~
   include/linux/scatterlist.h:143:10: note: in expansion of macro 'virt_addr_valid'
     143 |  BUG_ON(!virt_addr_valid(buf));
         |          ^~~~~~~~~~~~~~~
   drivers/usb/gadget/function/f_ncm.c: In function 'ncm_bitrate':
>> drivers/usb/gadget/function/f_ncm.c:89:3: warning: this decimal constant is unsigned only in ISO C90
      89 |   return 4000000000;
         |   ^~~~~~

# https://github.com/0day-ci/linux/commit/e5b632f5aecf9c4d7d1bd3e17cf3ff699c1c3201
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Lorenzo-Colitti/usb-gadget-f_ncm-fix-ncm_bitrate-for-SuperSpeed-and-above/20200819-010231
git checkout e5b632f5aecf9c4d7d1bd3e17cf3ff699c1c3201
vim +89 drivers/usb/gadget/function/f_ncm.c

    84	
    85	/* peak (theoretical) bulk transfer rate in bits-per-second */
    86	static inline unsigned ncm_bitrate(struct usb_gadget *g)
    87	{
    88		if (gadget_is_superspeed(g) && g->speed >= USB_SPEED_SUPER)
  > 89			return 4000000000;
    90		else if (gadget_is_dualspeed(g) && g->speed == USB_SPEED_HIGH)
    91			return 13 * 512 * 8 * 1000 * 8;
    92		else
    93			return 19 *  64 * 1 * 1000 * 8;
    94	}
    95	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 57743 bytes --]

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

[kernel test robot]

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

Hi Lorenzo,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on balbi-usb/testing/next]
[also build test WARNING on usb/usb-testing peter.chen-usb/ci-for-usb-next v5.9-rc1 next-20200818]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Lorenzo-Colitti/usb-gadget-f_ncm-fix-ncm_bitrate-for-SuperSpeed-and-above/20200819-010231
base:   https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git testing/next
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/linux/kernel.h:11,
                    from drivers/usb/gadget/function/f_ncm.c:14:
   include/linux/scatterlist.h: In function 'sg_set_buf':
   arch/m68k/include/asm/page_mm.h:169:49: warning: ordered comparison of pointer with null pointer [-Wextra]
     169 | #define virt_addr_valid(kaddr) ((void *)(kaddr) >= (void *)PAGE_OFFSET && (void *)(kaddr) < high_memory)
         |                                                 ^~
   include/linux/compiler.h:78:42: note: in definition of macro 'unlikely'
      78 | # define unlikely(x) __builtin_expect(!!(x), 0)
         |                                          ^
   include/linux/scatterlist.h:143:2: note: in expansion of macro 'BUG_ON'
     143 |  BUG_ON(!virt_addr_valid(buf));
         |  ^~~~~~
   include/linux/scatterlist.h:143:10: note: in expansion of macro 'virt_addr_valid'
     143 |  BUG_ON(!virt_addr_valid(buf));
         |          ^~~~~~~~~~~~~~~
   drivers/usb/gadget/function/f_ncm.c: In function 'ncm_bitrate':
>> drivers/usb/gadget/function/f_ncm.c:89:3: warning: this decimal constant is unsigned only in ISO C90
      89 |   return 4000000000;
         |   ^~~~~~

# https://github.com/0day-ci/linux/commit/e5b632f5aecf9c4d7d1bd3e17cf3ff699c1c3201
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Lorenzo-Colitti/usb-gadget-f_ncm-fix-ncm_bitrate-for-SuperSpeed-and-above/20200819-010231
git checkout e5b632f5aecf9c4d7d1bd3e17cf3ff699c1c3201
vim +89 drivers/usb/gadget/function/f_ncm.c

    84	
    85	/* peak (theoretical) bulk transfer rate in bits-per-second */
    86	static inline unsigned ncm_bitrate(struct usb_gadget *g)
    87	{
    88		if (gadget_is_superspeed(g) && g->speed >= USB_SPEED_SUPER)
  > 89			return 4000000000;
    90		else if (gadget_is_dualspeed(g) && g->speed == USB_SPEED_HIGH)
    91			return 13 * 512 * 8 * 1000 * 8;
    92		else
    93			return 19 *  64 * 1 * 1000 * 8;
    94	}
    95	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 57743 bytes --]

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

[Lorenzo Colitti]

On Wed, Aug 19, 2020 at 7:40 AM Maciej Żenczykowski
<zenczykowski@gmail.com> wrote:
> All the dongles I've looked at seem to place MaxBurst just after MaxPacketSize,
> and not in a separate descriptor (and don't place burst on the
> status/config channel).

You're right, that looks wrong. In my patches, the companion
descriptor sets max burst of 15 on ss_ncm_notify_comp_desc (and
ss_ncm_notify_comp_desc), which is an interrupt IN endpoint with a max
packet size of 16. This probably doesn't make sense, and in any case
is prohibited by the spec:

======
The only allowable maximum data payload size for interrupt endpoints
is 1024 bytes for interrupt endpoints that support a burst size
greater than one and can be any size from 1 to 1024 for an interrupt
endpoint with a burst size equal to one. The maximum allowable burst
size for interrupt endpoints is three.
======

Will respin this series to leave ss_ncm_notify_comp_desc and
ssp_ncm_notify_comp_desc at a burst of 0.

> What does "lsusb -d ... -v" show from the host side?

Output when on a 10 Gbps link:

    Interface Association:
      bLength                 8
      bDescriptorType        11
      bFirstInterface         0
      bInterfaceCount         2
      bFunctionClass          2 Communications
      bFunctionSubClass      13
      bFunctionProtocol       0
      iFunction               7 CDC NCM
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        0
      bAlternateSetting       0
      bNumEndpoints           1
      bInterfaceClass         2 Communications
      bInterfaceSubClass     13
      bInterfaceProtocol      0
      iInterface              4 CDC Network Control Model (NCM)
      CDC Header:
        bcdCDC               1.10
      CDC Union:
        bMasterInterface        0
        bSlaveInterface         1
      CDC Ethernet:
        iMacAddress                      5 fab33fdead72
        bmEthernetStatistics    0x00000000
        wMaxSegmentSize               1514
        wNumberMCFilters            0x0000
        bNumberPowerFilters              0
      CDC NCM:
        bcdNcmVersion        1.00
        bmNetworkCapabilities 0x11
          crc mode
          packet filter
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x82  EP 2 IN
        bmAttributes            3
          Transfer Type            Interrupt
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0010  1x 16 bytes
        bInterval               9
        bMaxBurst              15
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This looks wrong, see above.


    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        1
      bAlternateSetting       0
      bNumEndpoints           0
      bInterfaceClass        10 CDC Data
      bInterfaceSubClass      0
      bInterfaceProtocol      1
      iInterface              6 CDC Network Data
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        1
      bAlternateSetting       1
      bNumEndpoints           2
      bInterfaceClass        10 CDC Data
      bInterfaceSubClass      0
      bInterfaceProtocol      1
      iInterface              6 CDC Network Data
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x81  EP 1 IN
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0400  1x 1024 bytes
        bInterval               0
        bMaxBurst              15
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This looks correct (max burst of 15 on the bulk in endpoint).

      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x01  EP 1 OUT
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0400  1x 1024 bytes
        bInterval               0
        bMaxBurst              15
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This looks correct (max burst of 15 on the bulk out endpoint).

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

[Lorenzo Colitti]

On Wed, Aug 19, 2020 at 6:39 AM Maciej Żenczykowski
<zenczykowski@gmail.com> wrote:
> > -       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)
> > +               return 4000000000;

Will respin to change this to 4000000000U to address the warning
reported by the kernel test robot.

> Do you know what this actually affects besides the display?
> My cursory investigation shows it gets printed to kernel log and sent
> over some sort of ncm notification to the other side...

Yes, it's sent in the ConnectionSpeedChange notifications which are
intended to inform the host about how fast the link is. For a direct
connection over a USB cable this does not make much sense, but for,
say, a Gigabit Ethernet dongle that uses NCM, you'd probably want to
inform the host of whether the connection is 10, 100, or 1000M. This
is not what the code does now, obviously.

> Is it better to underestimate or overestimate?
> (ie. would it be better to report 3.5 gbps for super and max out at
> 4.2 gbps for super plus instead?)

I don't think it matters much. I'm happy to put 3860000000 for
SuperSpeed and 4200000000 for SuperSpeed Plus, or whatever else we
think makes sense. The speed is theoretical anyway. I suppose
reporting different speeds might be useful to debug whether the
connection is using 5G or >= 10G.

Felipe, any opinions?

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

[Maciej Żenczykowski]

On Wed, Aug 19, 2020 at 6:56 AM Lorenzo Colitti <lorenzo@google.com> wrote:
>
> On Wed, Aug 19, 2020 at 6:39 AM Maciej Żenczykowski
> <zenczykowski@gmail.com> wrote:
> > > -       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)
> > > +               return 4000000000;
>
> Will respin to change this to 4000000000U to address the warning
> reported by the kernel test robot.
>
> > Do you know what this actually affects besides the display?
> > My cursory investigation shows it gets printed to kernel log and sent
> > over some sort of ncm notification to the other side...
>
> Yes, it's sent in the ConnectionSpeedChange notifications which are
> intended to inform the host about how fast the link is. For a direct
> connection over a USB cable this does not make much sense, but for,
> say, a Gigabit Ethernet dongle that uses NCM, you'd probably want to
> inform the host of whether the connection is 10, 100, or 1000M. This
> is not what the code does now, obviously.
>
> > Is it better to underestimate or overestimate?
> > (ie. would it be better to report 3.5 gbps for super and max out at
> > 4.2 gbps for super plus instead?)
>
> I don't think it matters much. I'm happy to put 3860000000 for
> SuperSpeed and 4200000000 for SuperSpeed Plus, or whatever else we
> think makes sense. The speed is theoretical anyway. I suppose
> reporting different speeds might be useful to debug whether the
> connection is using 5G or >= 10G.
>
> Felipe, any opinions?

Oh reporting diff speeds to make it more obvious what happened and
whether SS+ is in use... that does seem like a win.