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

[Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>]

On 10/10/2023 6:07 AM, Maciej Żenczykowski wrote:
> On Mon, Oct 9, 2023 at 5:20 PM Maciej Żenczykowski <maze@google.com> wrote:
>>
>> On Mon, Oct 9, 2023 at 5:17 PM Maciej Żenczykowski <maze@google.com> wrote:
>>>
>>> On Mon, Oct 9, 2023 at 7:20 AM Krishna Kurapati
>>> <quic_kriskura@quicinc.com> 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@quicinc.com>
>>>> ---
>>>>   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?
> 
> One last thing...
> 
> static int ncm_unwrap_ntb(struct gether *port,
> ...
> unsigned frame_max = le16_to_cpu(ecm_desc.wMaxSegmentSize);
> 
> ^ is this a problem now if we have >1 gadget?
> how does it work then?


You are right. This would effect unwrap call and the wMaxSegmentSize is 
used directly. Thanks for the catch. I didn't test with 2 NCM interfaces 
and hence I wasn't able to find this bug. Perhaps changing this to 
opts->max_segment_size would fix the implementation as unwrap would 
anyways be called after bind.

Regards,
Krishna,