Re: [PATCH 2/2] usb: gadget: ncm: Add support to update wMaxSegmentSize via configfs

[Maciej Żenczykowski]

On Mon, Oct 9, 2023 at 7:20 AM Krishna Kurapati
<quic_kriskura@xxxxxxxxxxx> wrote:
>
> Currently the NCM driver restricts wMaxSegmentSize that indicates
> the datagram size coming from network layer to 1514. However the
> spec doesn't have any limitation. For P2P connections over NCM,
> increasing MTU helps increasing throughput.
>
> Add support to configure this value before configfs symlink is
> created. Also since the NTB Out/In buffer sizes are fixed at 16384
> bytes, limit the segment size to an upper cap of 15014. Set the
> default MTU size for the ncm interface during function bind before
> network interface is registered allowing MTU to be set in parity
> with wMaxSegmentSize.
>
> Signed-off-by: Krishna Kurapati <quic_kriskura@xxxxxxxxxxx>
> ---
>  drivers/usb/gadget/function/f_ncm.c | 51 +++++++++++++++++++++++++++++
>  drivers/usb/gadget/function/u_ncm.h |  2 ++
>  2 files changed, 53 insertions(+)
>
> diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
> index feccf4c8cc4f..eab297b22200 100644
> --- a/drivers/usb/gadget/function/f_ncm.c
> +++ b/drivers/usb/gadget/function/f_ncm.c
> @@ -103,6 +103,8 @@ static inline struct f_ncm *func_to_ncm(struct usb_function *f)
>  /* Delay for the transmit to wait before sending an unfilled NTB frame. */
>  #define TX_TIMEOUT_NSECS       300000
>
> +#define MAX_DATAGRAM_SIZE      15014
> +
>  #define FORMATS_SUPPORTED      (USB_CDC_NCM_NTB16_SUPPORTED |  \
>                                  USB_CDC_NCM_NTB32_SUPPORTED)
>
> @@ -1408,6 +1410,7 @@ static int ncm_bind(struct usb_configuration *c, struct usb_function *f)
>         ncm_opts = container_of(f->fi, struct f_ncm_opts, func_inst);
>
>         if (cdev->use_os_string) {
> +               ncm_opts->net->mtu = (ncm_opts->max_segment_size - ETH_HLEN);
>                 f->os_desc_table = kzalloc(sizeof(*f->os_desc_table),
>                                            GFP_KERNEL);
>                 if (!f->os_desc_table)
> @@ -1469,6 +1472,8 @@ static int ncm_bind(struct usb_configuration *c, struct usb_function *f)
>
>         status = -ENODEV;
>
> +       ecm_desc.wMaxSegmentSize = ncm_opts->max_segment_size;

I think you need byte swap here.

> +
>         /* allocate instance-specific endpoints */
>         ep = usb_ep_autoconfig(cdev->gadget, &fs_ncm_in_desc);
>         if (!ep)
> @@ -1569,11 +1574,56 @@ USB_ETHERNET_CONFIGFS_ITEM_ATTR_QMULT(ncm);
>  /* f_ncm_opts_ifname */
>  USB_ETHERNET_CONFIGFS_ITEM_ATTR_IFNAME(ncm);
>
> +static ssize_t ncm_opts_max_segment_size_show(struct config_item *item,
> +                                             char *page)
> +{
> +       struct f_ncm_opts *opts = to_f_ncm_opts(item);
> +       u32 segment_size;
> +
> +       mutex_lock(&opts->lock);
> +       segment_size = opts->max_segment_size;
> +       mutex_unlock(&opts->lock);
> +
> +       return sprintf(page, "%u\n", segment_size);
> +}
> +
> +static ssize_t ncm_opts_max_segment_size_store(struct config_item *item,
> +                                              const char *page, size_t len)
> +{
> +       struct f_ncm_opts *opts = to_f_ncm_opts(item);
> +       int ret;
> +       u32 segment_size;
> +
> +       mutex_lock(&opts->lock);
> +       if (opts->refcnt) {
> +               ret = -EBUSY;
> +               goto out;
> +       }
> +
> +       ret = kstrtou32(page, 0, &segment_size);
> +       if (ret)
> +               goto out;
> +
> +       if (segment_size > MAX_DATAGRAM_SIZE) {
> +               ret = -EINVAL;
> +               goto out;
> +       }
> +
> +       opts->max_segment_size = segment_size;
> +       ret = len;
> +out:
> +       mutex_unlock(&opts->lock);
> +       return ret;
> +}
> +
> +CONFIGFS_ATTR(ncm_opts_, max_segment_size);
> +
>  static struct configfs_attribute *ncm_attrs[] = {
>         &ncm_opts_attr_dev_addr,
>         &ncm_opts_attr_host_addr,
>         &ncm_opts_attr_qmult,
>         &ncm_opts_attr_ifname,
> +       &ncm_opts_attr_max_segment_size,
>         NULL,
>  };
>
> @@ -1616,6 +1666,7 @@ static struct usb_function_instance *ncm_alloc_inst(void)
>                 kfree(opts);
>                 return ERR_CAST(net);
>         }
> +       opts->max_segment_size = cpu_to_le16(ETH_FRAME_LEN);

and not here.  ie. max_segment_size should be native endian

>         INIT_LIST_HEAD(&opts->ncm_os_desc.ext_prop);
>
>         descs[0] = &opts->ncm_os_desc;
> diff --git a/drivers/usb/gadget/function/u_ncm.h b/drivers/usb/gadget/function/u_ncm.h
> index 5408854d8407..d3403cf13f17 100644
> --- a/drivers/usb/gadget/function/u_ncm.h
> +++ b/drivers/usb/gadget/function/u_ncm.h
> @@ -31,6 +31,8 @@ struct f_ncm_opts {
>          */
>         struct mutex                    lock;
>         int                             refcnt;
> +
> +       u32                             max_segment_size;
>  };
>
>  #endif /* U_NCM_H */
> --
> 2.42.0
>

That said, I don't really follow what this is doing...

Maciej Żenczykowski, Kernel Networking Developer @ Google