Re: [PATCH v5 0/5] fix statistics and payload issues for error

["Stefan Mätje" <Stefan.Maetje@esd.eu>]

Hello Vincent,

I now had some time to test the patches [1/5], [4/5] and [5/5] for esd_usb2.c 
and they work as expected. You may add

For esd_usb2.c
Acked-by: Stefan Mätje <stefan.maetje@esd.eu>
Tested-by: Stefan Mätje <stefan.maetje@esd.eu>

Thanks for the work changing all the drivers.

Best regards,
    Stefan Mätje


Am Dienstag, den 07.12.2021, 21:15 +0900 schrieb Vincent Mailhol:
> Important: this patch series depends on below patch:
> https://lore.kernel.org/linux-can/20211123111654.621610-1-mailhol.vincent@wanadoo.fr/T/#u
> 
> There are some common errors which are made when updating the network
> statistics or processing the CAN payload:
> 
>   1. Incrementing the "normal" stats when generating or sending a CAN
>   error message frame. Error message frames are an abstraction of
>   Socket CAN and do not exist on the wire. The first patch of this
>   series fixes the RX stats for 22 different drivers, the second one
>   fixes the TX stasts for the kvaser driver (N.B. only this driver is
>   capable of sending error on the bus).
> 
>   2. Copying the payload of RTR frames: RTR frames have no payload and
>   the data buffer only contains garbage. The DLC/length should not be
>   used to do a memory copy. The third patch of this series address
>   this issue for 3 different drivers.
> 
>   3. Counting the length of the Remote Transmission Frames (RTR). The
>   length of an RTR frame is the length of the requested frame not the
>   actual payload. In reality the payload of an RTR frame is always 0
>   bytes long. The fourth patch of this series fixes the RX stats for
>   27 different drivers and the fifth one fixes the TX stats for 25
>   different ones.
> 
> 
> * Changelog *
> 
> v4 -> v5:
> 
>   * at91_can: netdev_tx_t at91_start_xmit: replace
>     | dev->stats.tx_bytes = ...
>     by
>     | dev->stats.tx_bytes += ...
> 
> v3 -> v4:
> 
>   * patch 2/5: kvaser: include the suggestion from Jimmy Assarsson so
>     that we don't need to get the original CAN frame anymore to
>     determine whether or not it was an error frame. patch 5/5 is
>     rebased accordingly.
> 
>   * patch 5/5: kvaser: kvaser_usb_hydra_frame_to_cmd_std: remove
>     unrelated change.
> 
>   * patch 5/5: slcan: better factorize code for the tx RTR frames
>     (reorder the line instead of adding a new "if" branch).
> 
> 
> v2 -> v3:
> 
>   * Fix an issue in the fourth patch ("do not increase rx_bytes
>     statistics for RTR frames"). In ucan_rx_can_msg() of the ucan
>     driver, the changes in v2 made no sense. Reverted it to v1.
> 
> 
> v1 -> v2:
> 
>   * can_rx_offload_napi_poll: v1 used CAN_ERR_MASK instead of
>     CAN_ERR_FLAG. Fixed the issue.
> 
>   * use correct vocabulary. The correct term to designate the Socket
>     CAN specific error skb is "error message frames" not "error
>     frames". "error frames" is used in the standard and has a
>     different meaning.
> 
>   * better factorize code for the rx RTR frames. Most of the drivers
>     already have a switch to check if the frame is a RTR. Moved the
>     instruction to increase net_device_stats:rx_bytes inside the else
>     branch of those switches whenever possible (for some drivers with
>     some complex logic, putting and additional RTR check was easier).
> 
>   * add a patch which prevent drivers to copy the payload of RTR
>     frames.
> 
>   * add a patch to cover the tx RTR frames (the fifth patch of
>     v2). The tx RTR frames issue was supposedly covered by the
>     can_get_echo_skb() function which returns the correct length for
>     drivers to increase their stats. However, the reality is that most
>     of the drivers do not check this value and instead use a local
>     copy of the length/dlc.
> 
> Vincent Mailhol (5):
>   can: do not increase rx statistics when generating a CAN rx error
>     message frame
>   can: kvaser_usb: do not increase tx statistics when sending error
>     message frames
>   can: do not copy the payload of RTR frames
>   can: do not increase rx_bytes statistics for RTR frames
>   can: do not increase tx_bytes statistics for RTR frames
> 
>  drivers/net/can/at91_can.c                    | 18 ++---
>  drivers/net/can/c_can/c_can.h                 |  1 -
>  drivers/net/can/c_can/c_can_main.c            | 16 ++---
>  drivers/net/can/cc770/cc770.c                 | 16 ++---
>  drivers/net/can/dev/dev.c                     |  4 --
>  drivers/net/can/dev/rx-offload.c              |  7 +-
>  drivers/net/can/grcan.c                       |  6 +-
>  drivers/net/can/ifi_canfd/ifi_canfd.c         | 11 +--
>  drivers/net/can/janz-ican3.c                  |  6 +-
>  drivers/net/can/kvaser_pciefd.c               | 16 ++---
>  drivers/net/can/m_can/m_can.c                 | 13 +---
>  drivers/net/can/mscan/mscan.c                 | 14 ++--
>  drivers/net/can/pch_can.c                     | 33 ++++-----
>  drivers/net/can/peak_canfd/peak_canfd.c       | 14 ++--
>  drivers/net/can/rcar/rcar_can.c               | 22 +++---
>  drivers/net/can/rcar/rcar_canfd.c             | 13 +---
>  drivers/net/can/sja1000/sja1000.c             | 11 ++-
>  drivers/net/can/slcan.c                       |  7 +-
>  drivers/net/can/softing/softing_main.c        |  8 +--
>  drivers/net/can/spi/hi311x.c                  | 31 ++++----
>  drivers/net/can/spi/mcp251x.c                 | 31 ++++----
>  drivers/net/can/sun4i_can.c                   | 22 +++---
>  drivers/net/can/usb/ems_usb.c                 | 14 ++--
>  drivers/net/can/usb/esd_usb2.c                | 13 ++--
>  drivers/net/can/usb/etas_es58x/es58x_core.c   |  7 --
>  drivers/net/can/usb/gs_usb.c                  |  7 +-
>  drivers/net/can/usb/kvaser_usb/kvaser_usb.h   |  5 +-
>  .../net/can/usb/kvaser_usb/kvaser_usb_core.c  |  4 +-
>  .../net/can/usb/kvaser_usb/kvaser_usb_hydra.c | 71 +++++++++----------
>  .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c  | 20 ++----
>  drivers/net/can/usb/mcba_usb.c                | 23 +++---
>  drivers/net/can/usb/peak_usb/pcan_usb.c       |  9 ++-
>  drivers/net/can/usb/peak_usb/pcan_usb_core.c  | 20 +++---
>  drivers/net/can/usb/peak_usb/pcan_usb_core.h  |  1 -
>  drivers/net/can/usb/peak_usb/pcan_usb_fd.c    | 11 ++-
>  drivers/net/can/usb/peak_usb/pcan_usb_pro.c   | 12 ++--
>  drivers/net/can/usb/ucan.c                    | 17 +++--
>  drivers/net/can/usb/usb_8dev.c                | 17 ++---
>  drivers/net/can/vcan.c                        |  7 +-
>  drivers/net/can/vxcan.c                       |  2 +-
>  drivers/net/can/xilinx_can.c                  | 19 +++--
>  include/linux/can/skb.h                       |  5 +-
>  42 files changed, 254 insertions(+), 350 deletions(-)
> 
> 
> base-commit: 11f2c3c57f37befb1af6ccac0defacb813411d9d