diff mbox series

usb: gadget: ncm: Fix endianness of wMaxSegmentSize variable in ecm_desc

Message ID 20240118154910.8765-1-quic_kriskura@quicinc.com (mailing list archive)
State Accepted
Commit 9dc292413c56a2d01e34787d3fc4a76635e4a498
Headers show
Series usb: gadget: ncm: Fix endianness of wMaxSegmentSize variable in ecm_desc | expand

Commit Message

Krishna Kurapati Jan. 18, 2024, 3:48 p.m. UTC
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@quicinc.com/

Fixes: 1900daeefd3e ("usb: gadget: ncm: Add support to update wMaxSegmentSize via configfs")
Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
---
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(-)

Comments

Maciej Żenczykowski Jan. 18, 2024, 6:30 p.m. UTC | #1
On Thu, Jan 18, 2024 at 7:49 AM Krishna Kurapati
<quic_kriskura@quicinc.com> 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@quicinc.com/
>
> Fixes: 1900daeefd3e ("usb: gadget: ncm: Add support to update wMaxSegmentSize via configfs")
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> ---
> 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@google.com>
diff mbox series

Patch

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;