diff mbox series

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

Message ID 20200818165848.4117493-3-lorenzo@google.com (mailing list archive)
State Superseded
Headers show
Series [v2,1/3] usb: gadget: f_ncm: fix ncm_bitrate for SuperSpeed and above. | expand

Commit Message

Lorenzo Colitti Aug. 18, 2020, 4:58 p.m. UTC 
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(-)

Comments

Maciej Żenczykowski Aug. 18, 2020, 10:40 p.m. UTC | #1 
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
>
Lorenzo Colitti Aug. 19, 2020, 1:39 p.m. UTC | #2 
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).
diff mbox series

Patch

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;