Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
MIME-Version: 1.0
References: <20231009142005.21338-1-quic_kriskura@quicinc.com>
 <20231009142005.21338-2-quic_kriskura@quicinc.com> <CANP3RGfEk2DqZ3biyN78ycQYbDxCEG+H1me2vnEYuwXkNdXnTA@mail.gmail.com>
 <CANP3RGcCpNOuVpdV9n0AFxZo-wsfwi8OfYgBk1WHNHaEd-4V-Q@mail.gmail.com>
 <CANP3RGdY4LsOA6U5kuccApHCzL0_jBnY=pLOYrUuYtMZFTvnbw@mail.gmail.com>
 <d19d9d08-c119-4991-b460-49925f601d15@quicinc.com> <fad5a7fb-cce1-46bc-a0af-72405c76d107@quicinc.com>
In-Reply-To: <fad5a7fb-cce1-46bc-a0af-72405c76d107@quicinc.com>
From:   =?UTF-8?Q?Maciej_=C5=BBenczykowski?= <maze@google.com>
Date:   Thu, 12 Oct 2023 05:32:22 -0700
Message-ID: <CANP3RGcqWBYd9FqAX47rE9pFgBTB8=0CGdwkScm-OH1epHcVWQ@mail.gmail.com>
Subject: Re: [PATCH 2/2] usb: gadget: ncm: Add support to update
 wMaxSegmentSize via configfs
To:     Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        onathan Corbet <corbet@lwn.net>,
        Linyu Yuan <quic_linyyuan@quicinc.com>,
        linux-usb@vger.kernel.org, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org, quic_ppratap@quicinc.com,
        quic_wcheng@quicinc.com, quic_jackp@quicinc.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Oct 12, 2023 at 1:48=E2=80=AFAM Krishna Kurapati PSSNV
<quic_kriskura@quicinc.com> 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 interface=
s
> > 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 =3D {
>          /* this descriptor actually adds value, surprise! */
>          /* .iMACAddress =3D DYNAMIC */
>          .bmEthernetStatistics =3D cpu_to_le32(0), /* no statistics */
> -       .wMaxSegmentSize =3D      cpu_to_le16(ETH_FRAME_LEN),
>          .wNumberMCFilters =3D     cpu_to_le16(0),
>          .bNumberPowerFilters =3D  0,
>   };
> @@ -1180,10 +1185,15 @@ static int ncm_unwrap_ntb(struct gether *port,
>          struct sk_buff  *skb2;
>          int             ret =3D -EINVAL;
>          unsigned        ntb_max =3D
> le32_to_cpu(ntb_parameters.dwNtbOutMaxSize);
> -       unsigned        frame_max =3D le16_to_cpu(ecm_desc.wMaxSegmentSiz=
e);
> +       unsigned int    frame_max;
>          const struct ndp_parser_opts *opts =3D ncm->parser_opts;
>          unsigned        crc_len =3D ncm->is_crc ? sizeof(uint32_t) : 0;
>          int             dgram_counter;
> +       struct f_ncm_opts *ncm_opts;
> +       const struct usb_function_instance *fi =3D port->func.fi;
> +
> +       ncm_opts =3D container_of(fi, struct f_ncm_opts, func_inst);
> +       frame_max =3D ncm_opts->max_segment_size;
>
>          /* dwSignature */
>          if (get_unaligned_le32(tmp) !=3D 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 =3D (ncm_opts->max_segment_size -
> ETH_HLEN);
>                  gether_set_gadget(ncm_opts->net, cdev->gadget);
>                  status =3D 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 =3D -ENODEV;
>
> +       ecm_desc.wMaxSegmentSize =3D (__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)