Re: [PATCH 2/2] usb: gadget: ncm: Add support to update wMaxSegmentSize via configfs
From: Krishna Kurapati PSSNV
Date: Tue Oct 10 2023 - 00:34:36 EST
On 10/10/2023 5:50 AM, Maciej Żenczykowski wrote:
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?
Actually, keeping it or removing it doesn't affect because the default
value of this will be set in bind call via opts->max_segment_size. And
this max_segment_size variable is defaulted to ETH_FRAME_LEN in
alloc_inst. After bind, writing to this value doesn't take effect.
Before the netdev interface was registered, if this value is updated
then MTU is changed automatically for this interface. If there are two
NCM devices, then each of them will have their own instance of
wMaxSegmentSize property in their usb_gadget/g1/functions/ncm.X
directories and both of them are defaulted ETH_FRAME_LEN if the configfs
property is not disturbed. In case only one needs to change, we can
change via configfs and the other stays as ETH_FRAME_LEN only.
Regards,
Krishna,