On Mon, Oct 9, 2023 at 5:17 PM Maciej Żenczykowski <maze@xxxxxxxxxx> wrote:
>
> 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...
Also
static struct usb_cdc_ether_desc ecm_desc = {
...
.wMaxSegmentSize = cpu_to_le16(ETH_FRAME_LEN),
^ I think this should be deleted now, right? since it's always overwritten?
And if it isn't always overwritten, that would be a bug I think, cause
what happens if you bring up 2 ncm devices and only change the setting
on the 1st?
