Re: [PATCH] usb: gadget: ncm: Fix endianness of wMaxSegmentSize variable in ecm_desc

[Maciej Żenczykowski]

On Thu, Jan 18, 2024 at 7:49 AM Krishna Kurapati
<quic_kriskura@xxxxxxxxxxx> wrote:
>
> Recent commit [1] added support for changing max segment size of the NCM
> interface via configfs. But the value of segment size value stored in
> ncm_opts need to be converted to little endian before saving it in
> ecm_desc. Also while initialising the value of segment size in opts during
> instance allocation, the value ETH_FRAME_LEN needs to be assigned directly
> without any conversion as ETH_FRAME_LEN and the variable max_segment_size
> are native endian. The current implementaion modifies it into little endian
> thus breaking things for big endian targets.
>
> Fix endianness while assigning these variables.
> While at it, fix up some stray spaces in comments added in code.
>
> [1]: https://lore.kernel.org/all/20231221153216.18657-1-quic_kriskura@xxxxxxxxxxx/
>
> Fixes: 1900daeefd3e ("usb: gadget: ncm: Add support to update wMaxSegmentSize via configfs")
> Signed-off-by: Krishna Kurapati <quic_kriskura@xxxxxxxxxxx>
> ---
> Since the patch was tested on ARM based QC devices, no issues were seen
> as these devices were little endian. Thanks to Maciej Żenczykowski for
> pointing it out offline over ACK that the patch breaks functionality for
> big endian devices.
>
>  drivers/usb/gadget/function/f_ncm.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
> index a1575a0ca568..ca5d5f564998 100644
> --- a/drivers/usb/gadget/function/f_ncm.c
> +++ b/drivers/usb/gadget/function/f_ncm.c
> @@ -105,8 +105,8 @@ static inline struct f_ncm *func_to_ncm(struct usb_function *f)
>
>  /*
>   * Although max mtu as dictated by u_ether is 15412 bytes, setting
> - * max_segment_sizeto 15426 would not be efficient. If user chooses segment
> - * size to be (>= 8192), then we can't aggregate more than one  buffer in each
> + * max_segment_size to 15426 would not be efficient. If user chooses segment
> + * size to be (>= 8192), then we can't aggregate more than one buffer in each
>   * NTB (assuming each packet coming from network layer is >= 8192 bytes) as ep
>   * maxpacket limit is 16384. So let max_segment_size be limited to 8000 to allow
>   * at least 2 packets to be aggregated reducing wastage of NTB buffer space
> @@ -1489,7 +1489,7 @@ static int ncm_bind(struct usb_configuration *c, struct usb_function *f)
>         ncm_data_intf.bInterfaceNumber = status;
>         ncm_union_desc.bSlaveInterface0 = status;
>
> -       ecm_desc.wMaxSegmentSize = ncm_opts->max_segment_size;
> +       ecm_desc.wMaxSegmentSize = cpu_to_le16(ncm_opts->max_segment_size);
>
>         status = -ENODEV;
>
> @@ -1685,7 +1685,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);
> +       opts->max_segment_size = ETH_FRAME_LEN;
>         INIT_LIST_HEAD(&opts->ncm_os_desc.ext_prop);
>
>         descs[0] = &opts->ncm_os_desc;
> --
> 2.42.0
>
Reviewed-by: Maciej Żenczykowski <maze@xxxxxxxxxx>