Re: [PATCH v3 5/5] can: do not increase tx_bytes statistics for RTR frames

[Jimmy Assarsson <extja@kvaser.com>]

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

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