On 2021-12-03 02:05, Vincent MAILHOL wrote:

On Fri. 3 Dec. 2021 at 08:35, Jimmy Assarsson [off-list ref] wrote:

quoted

On 2021-11-28 13:37, Vincent Mailhol wrote:

quoted

The actual payload length of the CAN Remote Transmission Request (RTR)
frames is always 0, i.e. nothing is transmitted on the wire. However,
those RTR frames still use the DLC to indicate the length of the
requested frame.

As such, net_device_stats:tx_bytes should not be increased when
sending RTR frames.

The function can_get_echo_skb() already returns the correct length,
even for RTR frames (c.f. [1]). However, for historical reasons, the
drivers do not use can_get_echo_skb()'s return value and instead, most
of them store a temporary length (or dlc) in some local structure or
array. Using the return value of can_get_echo_skb() solves the
issue. After doing this, such length/dlc fields become unused and so
this patch does the adequate cleaning when needed.

This patch fixes all the CAN drivers.

Finally, can_get_echo_skb() is decorated with the __must_check
attribute in order to force future drivers to correctly use its return
value (else the compiler would emit a warning).

[1] commit ed3320cec279 ("can: dev: __can_get_echo_skb():
fix real payload length return value for RTR frames")

Hi Vincent!

Thanks for the patch!
I've reviewed and tested the changes affecting kvaser_usb.
Looks good to me, only a minor nitpick inline :)

...

quoted

quoted

diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
index 17fabd3d0613..9f423a5fb63f 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c

...

quoted

quoted

@@ -1493,13 +1489,13 @@ kvaser_usb_hydra_frame_to_cmd_std(const struct kvaser_usb_net_priv *priv,
       if (cf->can_id & CAN_RTR_FLAG)
               flags |= KVASER_USB_HYDRA_CF_FLAG_REMOTE_FRAME;

-     flags |= (cf->can_id & CAN_ERR_FLAG ?
-               KVASER_USB_HYDRA_CF_FLAG_ERROR_FRAME : 0);
+     if (cf->can_id & CAN_ERR_FLAG)
+             flags |= KVASER_USB_HYDRA_CF_FLAG_ERROR_FRAME;

This has nothing to do with RTR. Maybe put it in a separate patch?

Arg... You are right. This should not be here. I saw it in my
final check, removed it in my tree and forgot to redo a "git
format-patch".

This is some leftover of a previous version in which I did more
heavy changes to kvaser_usb_hydra_frame_to_cmd_std(). This is
purely cosmetic though. I am not willing to go into a clean up
crusade of all CAN drivers so I will just leave the ternary
operator untouched. Free to you to reuse it if you want to do a
clean up later on.

Sure, I'll save it for later :) If you got more changes, feel free to share
them.

Best regards,
jimmy

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel