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

[Krishna Kurapati PSSNV]

On 10/10/2023 10:08 AM, Krishna Kurapati PSSNV wrote:
>

> > ^ 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.

Hi Maciej,

 How about the below diff:

---------

+/*
+ * Allow max segment size to be in parity with max_mtu possible
+ * for the interface.
+ */
+#define MAX_DATAGRAM_SIZE      GETHER_MAX_ETH_FRAME_LEN
+
 #define FORMATS_SUPPORTED      (USB_CDC_NCM_NTB16_SUPPORTED |  \
                                 USB_CDC_NCM_NTB32_SUPPORTED)

@@ -194,7 +200,6 @@ static struct usb_cdc_ether_desc ecm_desc = {
        /* this descriptor actually adds value, surprise! */
        /* .iMACAddress = DYNAMIC */
        .bmEthernetStatistics = cpu_to_le32(0), /* no statistics */
-       .wMaxSegmentSize =      cpu_to_le16(ETH_FRAME_LEN),
        .wNumberMCFilters =     cpu_to_le16(0),
        .bNumberPowerFilters =  0,
 };
@@ -1180,10 +1185,15 @@ static int ncm_unwrap_ntb(struct gether *port,
        struct sk_buff  *skb2;
        int             ret = -EINVAL;
        unsigned        ntb_max = 
le32_to_cpu(ntb_parameters.dwNtbOutMaxSize);
-       unsigned        frame_max = le16_to_cpu(ecm_desc.wMaxSegmentSize);
+       unsigned int    frame_max;
        const struct ndp_parser_opts *opts = ncm->parser_opts;
        unsigned        crc_len = ncm->is_crc ? sizeof(uint32_t) : 0;
        int             dgram_counter;
+       struct f_ncm_opts *ncm_opts;
+       const struct usb_function_instance *fi = port->func.fi;
+
+       ncm_opts = container_of(fi, struct f_ncm_opts, func_inst);
+       frame_max = ncm_opts->max_segment_size;

        /* dwSignature */
        if (get_unaligned_le32(tmp) != opts->nth_sign) {
@@ -1440,6 +1450,7 @@ static int ncm_bind(struct usb_configuration *c, 
struct usb_function *f)
         */
        if (!ncm_opts->bound) {
                mutex_lock(&ncm_opts->lock);
+               ncm_opts->net->mtu = (ncm_opts->max_segment_size - 
ETH_HLEN);
                gether_set_gadget(ncm_opts->net, cdev->gadget);
                status = gether_register_netdev(ncm_opts->net);
                mutex_unlock(&ncm_opts->lock);
@@ -1484,6 +1495,8 @@ static int ncm_bind(struct usb_configuration *c, 
struct usb_function *f)

        status = -ENODEV;

+       ecm_desc.wMaxSegmentSize = (__le16)ncm_opts->max_segment_size;
+

------

I can limit the max segment size to (Max MTU + ETH_HELN) and this would 
be logical to do. Also we can set the frame_max from ncm_opts itself 
while initializing it to 1514 (default value) during alloc_inst callback 
and nothing would break while still being backward compatible.

Let me know your thoughts on this.

Regards,
Krishna,