Re: [PATCH v1 2/2] can: do not increase rx_bytes statistics for RTR frames

[Vincent MAILHOL]

Hi Stefan,

On Thu. 25 Nov 2021 at 02:59, Stefan Mätje <Stefan.Maetje@xxxxxx> wrote:
> Hi Vincent,
>
> I would like to suggest a slightly different patch for the esd_usb2.c (as
> added below).
>
> Best regards,
>     Stefan Mätje
>
> Am Dienstag, den 23.11.2021, 20:53 +0900 schrieb Vincent Mailhol:
> > 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 uses the DLC to indicate the length of the
> > requested frame.
> >
> > As such, net_device_stats:rx_bytes should not be increased for the RTR
> > frames.
> >
> > This patch fixes all the CAN drivers.
> >
> > CC: Jimmy Assarsson <extja@xxxxxxxxxx>
> > CC: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
> > CC: Nicolas Ferre <nicolas.ferre@xxxxxxxxxxxxx>
> > CC: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx>
> > CC: Ludovic Desroches <ludovic.desroches@xxxxxxxxxxxxx>
> > CC: Chandrasekar Ramakrishnan <rcsekar@xxxxxxxxxxx>
> > CC: Maxime Ripard <mripard@xxxxxxxxxx>
> > CC: Chen-Yu Tsai <wens@xxxxxxxx>
> > CC: Jernej Skrabec <jernej.skrabec@xxxxxxxxx>
> > CC: Yasushi SHOJI <yashi@xxxxxxxxxxxxxxx>
> > CC: Appana Durga Kedareswara rao <appana.durga.rao@xxxxxxxxxx>
> > CC: Naga Sureshkumar Relli <naga.sureshkumar.relli@xxxxxxxxxx>
> > CC: Michal Simek <michal.simek@xxxxxxxxxx>
> > CC: Stephane Grosjean <s.grosjean@xxxxxxxxxxxxxxx>
> > Signed-off-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx>
> > ---
> >  drivers/net/can/at91_can.c                        | 3 ++-
> >  drivers/net/can/c_can/c_can_main.c                | 3 ++-
> >  drivers/net/can/cc770/cc770.c                     | 3 ++-
> >  drivers/net/can/dev/rx-offload.c                  | 3 ++-
> >  drivers/net/can/grcan.c                           | 3 ++-
> >  drivers/net/can/ifi_canfd/ifi_canfd.c             | 3 ++-
> >  drivers/net/can/janz-ican3.c                      | 3 ++-
> >  drivers/net/can/kvaser_pciefd.c                   | 3 ++-
> >  drivers/net/can/m_can/m_can.c                     | 3 ++-
> >  drivers/net/can/mscan/mscan.c                     | 3 ++-
> >  drivers/net/can/pch_can.c                         | 3 ++-
> >  drivers/net/can/peak_canfd/peak_canfd.c           | 3 ++-
> >  drivers/net/can/rcar/rcar_can.c                   | 3 ++-
> >  drivers/net/can/rcar/rcar_canfd.c                 | 3 ++-
> >  drivers/net/can/sja1000/sja1000.c                 | 3 ++-
> >  drivers/net/can/slcan.c                           | 3 ++-
> >  drivers/net/can/spi/hi311x.c                      | 3 ++-
> >  drivers/net/can/spi/mcp251x.c                     | 3 ++-
> >  drivers/net/can/sun4i_can.c                       | 3 ++-
> >  drivers/net/can/usb/ems_usb.c                     | 3 ++-
> >  drivers/net/can/usb/esd_usb2.c                    | 3 ++-
> >  drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c | 6 ++++--
> >  drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c  | 3 ++-
> >  drivers/net/can/usb/mcba_usb.c                    | 3 ++-
> >  drivers/net/can/usb/peak_usb/pcan_usb.c           | 3 ++-
> >  drivers/net/can/usb/peak_usb/pcan_usb_fd.c        | 8 ++++----
> >  drivers/net/can/usb/peak_usb/pcan_usb_pro.c       | 9 +++++----
> >  drivers/net/can/usb/ucan.c                        | 3 ++-
> >  drivers/net/can/usb/usb_8dev.c                    | 8 ++++----
> >  drivers/net/can/xilinx_can.c                      | 8 +++++---
> >  30 files changed, 72 insertions(+), 42 deletions(-)
> >
...
> >
> > diff --git a/drivers/net/can/usb/esd_usb2.c b/drivers/net/can/usb/esd_usb2.c
> > index 5f6915a27b3d..ac65ddfe814d 100644
> > --- a/drivers/net/can/usb/esd_usb2.c
> > +++ b/drivers/net/can/usb/esd_usb2.c
> > @@ -335,7 +335,8 @@ static void esd_usb2_rx_can_msg(struct esd_usb2_net_priv *priv,
> >               }
> >
> >               stats->rx_packets++;
> > -             stats->rx_bytes += cf->len;
> > +             if (!(cf->can_id & CAN_RTR_FLAG))
> > +                     stats->rx_bytes += cf->len;
> >               netif_rx(skb);
> >       }
> >
>
> The version below would save us adding another if() statement to check for RTR or
> normal frame that is already tested in the if() statement directly before.
>
> diff --git a/drivers/net/can/usb/esd_usb2.c b/drivers/net/can/usb/esd_usb2.c
> index c6068a251fbe..2f91a18fe592 100644
> --- a/drivers/net/can/usb/esd_usb2.c
> +++ b/drivers/net/can/usb/esd_usb2.c
> @@ -332,14 +332,14 @@ static void esd_usb2_rx_can_msg(struct esd_usb2_net_priv *priv,
>                 if (msg->msg.rx.dlc & ESD_RTR) {
>                         cf->can_id |= CAN_RTR_FLAG;
>                 } else {
>                         for (i = 0; i < cf->len; i++)
>                                 cf->data[i] = msg->msg.rx.data[i];
> +                       stats->rx_bytes += cf->len;
>                 }
> -
>                 stats->rx_packets++;
> -               stats->rx_bytes += cf->len;
> +
>                 netif_rx(skb);
>         }
>
>         return;
>  }

Work for me! I will add this in the v2 (under preparation).

Yours sincerely,
Vincent Mailhol