Messages in this thread | | | Subject | Re: [PATCH v3 5/5] can: do not increase tx_bytes statistics for RTR frames | From | Jimmy Assarsson <> | Date | Fri, 3 Dec 2021 09:44:51 +0100 |
| |
On 2021-12-03 02:05, Vincent MAILHOL wrote: > On Fri. 3 Dec. 2021 at 08:35, Jimmy Assarsson <extja@kvaser.com> wrote: >> On 2021-11-28 13:37, Vincent Mailhol wrote: >>> 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 :)
...
>>> 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
...
>>> @@ -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
| |