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

[Jimmy Assarsson]

On 2021-12-03 02:05, Vincent MAILHOL wrote:
> On Fri. 3 Dec. 2021 at 08:35, Jimmy Assarsson <extja@xxxxxxxxxx> 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