On Thu, Oct 12, 2023 at 1:48 AM Krishna Kurapati PSSNV
<quic_kriskura@xxxxxxxxxxx> wrote:
>
>
>
> 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;
this looks wrong. pretty sure this should be some form of cpu_to_le16
> +
>
> ------
>
> 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,
Could you paste the full patch?
This is hard to review without looking at much more context then email
is providing
(or, even better, send me a link to a CL in gerrit somewhere - for
example aosp ACK mainline tree)
