LinuxLists.cc - [PATCH v5 0/5] fix statistics and payload issues for error

2021-12-07 12:16:09

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

Important: this patch series depends on below patch:
https://lore.kernel.org/linux-can/[email protected]/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
--
2.32.0

2021-12-07 12:16:25

by Vincent MAILHOL

[permalink] [raw]

Subject: [PATCH v5 1/5] can: do not increase rx statistics when generating a CAN rx error message frame

The CAN error message frames (i.e. error skb) are an interface
specific to socket CAN. The payload of the CAN error message frames
does not correspond to any actual data sent on the wire. Only an error
flag and a delimiter are transmitted when an error occurs (c.f. ISO
11898-1 section 10.4.4.2 "Error flag").

For this reason, it makes no sense to increment the rx_packets and
rx_bytes fields of struct net_device_stats because no actual payload
were transmitted on the wire.

This patch fixes all the CAN drivers.

CC: Marc Kleine-Budde <[email protected]>
CC: Nicolas Ferre <[email protected]>
CC: Alexandre Belloni <[email protected]>
CC: Ludovic Desroches <[email protected]>
CC: Chandrasekar Ramakrishnan <[email protected]>
CC: Maxime Ripard <[email protected]>
CC: Chen-Yu Tsai <[email protected]>
CC: Jernej Skrabec <[email protected]>
CC: Appana Durga Kedareswara rao <[email protected]>
CC: Naga Sureshkumar Relli <[email protected]>
CC: Michal Simek <[email protected]>
CC: Stephane Grosjean <[email protected]>
Tested-by: Jimmy Assarsson <[email protected]> # kvaser
Signed-off-by: Vincent Mailhol <[email protected]>
---
drivers/net/can/at91_can.c | 6 ------
drivers/net/can/c_can/c_can_main.c | 5 -----
drivers/net/can/cc770/cc770.c | 3 ---
drivers/net/can/dev/dev.c | 4 ----
drivers/net/can/dev/rx-offload.c | 6 ++++--
drivers/net/can/ifi_canfd/ifi_canfd.c | 5 -----
drivers/net/can/kvaser_pciefd.c | 5 -----
drivers/net/can/m_can/m_can.c | 7 -------
drivers/net/can/mscan/mscan.c | 9 +++++----
drivers/net/can/pch_can.c | 3 ---
drivers/net/can/peak_canfd/peak_canfd.c | 4 ----
drivers/net/can/rcar/rcar_can.c | 6 +-----
drivers/net/can/rcar/rcar_canfd.c | 4 ----
drivers/net/can/sja1000/sja1000.c | 2 --
drivers/net/can/sun4i_can.c | 7 ++-----
drivers/net/can/usb/ems_usb.c | 2 --
drivers/net/can/usb/esd_usb2.c | 2 --
drivers/net/can/usb/etas_es58x/es58x_core.c | 7 -------
drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c | 2 --
drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c | 8 --------
drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 4 ----
drivers/net/can/usb/peak_usb/pcan_usb.c | 2 --
drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 3 ---
drivers/net/can/usb/peak_usb/pcan_usb_pro.c | 2 --
drivers/net/can/usb/ucan.c | 6 ++++--
drivers/net/can/usb/usb_8dev.c | 2 --
drivers/net/can/xilinx_can.c | 9 +--------
27 files changed, 17 insertions(+), 108 deletions(-)

diff --git a/drivers/net/can/at91_can.c b/drivers/net/can/at91_can.c
index 3aea32c9b108..3cd872cf9be6 100644
--- a/drivers/net/can/at91_can.c
+++ b/drivers/net/can/at91_can.c
@@ -553,8 +553,6 @@ static void at91_rx_overflow_err(struct net_device *dev)
cf->can_id |= CAN_ERR_CRTL;
cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;

- stats->rx_packets++;
- stats->rx_bytes += cf->len;
netif_receive_skb(skb);
}

@@ -779,8 +777,6 @@ static int at91_poll_err(struct net_device *dev, int quota, u32 reg_sr)

at91_poll_err_frame(dev, cf, reg_sr);

- dev->stats.rx_packets++;
- dev->stats.rx_bytes += cf->len;
netif_receive_skb(skb);

return 1;
@@ -1037,8 +1033,6 @@ static void at91_irq_err(struct net_device *dev)

at91_irq_err_state(dev, cf, new_state);

- dev->stats.rx_packets++;
- dev->stats.rx_bytes += cf->len;
netif_rx(skb);

priv->can.state = new_state;
diff --git a/drivers/net/can/c_can/c_can_main.c b/drivers/net/can/c_can/c_can_main.c
index 52671d1ea17d..670754a12984 100644
--- a/drivers/net/can/c_can/c_can_main.c
+++ b/drivers/net/can/c_can/c_can_main.c
@@ -920,7 +920,6 @@ static int c_can_handle_state_change(struct net_device *dev,
unsigned int reg_err_counter;
unsigned int rx_err_passive;
struct c_can_priv *priv = netdev_priv(dev);
- struct net_device_stats *stats = &dev->stats;
struct can_frame *cf;
struct sk_buff *skb;
struct can_berr_counter bec;
@@ -996,8 +995,6 @@ static int c_can_handle_state_change(struct net_device *dev,
break;
}

- stats->rx_packets++;
- stats->rx_bytes += cf->len;
netif_receive_skb(skb);

return 1;
@@ -1064,8 +1061,6 @@ static int c_can_handle_bus_err(struct net_device *dev,
break;
}

- stats->rx_packets++;
- stats->rx_bytes += cf->len;
netif_receive_skb(skb);
return 1;
}
diff --git a/drivers/net/can/cc770/cc770.c b/drivers/net/can/cc770/cc770.c
index f8a130f594e2..a5fd8ccedec2 100644
--- a/drivers/net/can/cc770/cc770.c
+++ b/drivers/net/can/cc770/cc770.c
@@ -499,7 +499,6 @@ static void cc770_rx(struct net_device *dev, unsigned int mo, u8 ctrl1)
static int cc770_err(struct net_device *dev, u8 status)
{
struct cc770_priv *priv = netdev_priv(dev);
- struct net_device_stats *stats = &dev->stats;
struct can_frame *cf;
struct sk_buff *skb;
u8 lec;
@@ -571,8 +570,6 @@ static int cc770_err(struct net_device *dev, u8 status)
}

- stats->rx_packets++;
- stats->rx_bytes += cf->len;
netif_rx(skb);

return 0;
diff --git a/drivers/net/can/dev/dev.c b/drivers/net/can/dev/dev.c
index e3d840b81357..4845ae6456e1 100644
--- a/drivers/net/can/dev/dev.c
+++ b/drivers/net/can/dev/dev.c
@@ -136,7 +136,6 @@ EXPORT_SYMBOL_GPL(can_change_state);
static void can_restart(struct net_device *dev)
{
struct can_priv *priv = netdev_priv(dev);
- struct net_device_stats *stats = &dev->stats;
struct sk_buff *skb;
struct can_frame *cf;
int err;
@@ -155,9 +154,6 @@ static void can_restart(struct net_device *dev)

cf->can_id |= CAN_ERR_RESTARTED;

- stats->rx_packets++;
- stats->rx_bytes += cf->len;
-
netif_rx_ni(skb);

restart:
diff --git a/drivers/net/can/dev/rx-offload.c b/drivers/net/can/dev/rx-offload.c
index 37b0cc65237b..7dbf46b9ca5d 100644
--- a/drivers/net/can/dev/rx-offload.c
+++ b/drivers/net/can/dev/rx-offload.c
@@ -54,8 +54,10 @@ static int can_rx_offload_napi_poll(struct napi_struct *napi, int quota)
struct can_frame *cf = (struct can_frame *)skb->data;

work_done++;
- stats->rx_packets++;
- stats->rx_bytes += cf->len;
+ if (!(cf->can_id & CAN_ERR_FLAG)) {
+ stats->rx_packets++;
+ stats->rx_bytes += cf->len;
+ }
netif_receive_skb(skb);
}

diff --git a/drivers/net/can/ifi_canfd/ifi_canfd.c b/drivers/net/can/ifi_canfd/ifi_canfd.c
index 5bb957a26bc6..e8318e984bf2 100644
--- a/drivers/net/can/ifi_canfd/ifi_canfd.c
+++ b/drivers/net/can/ifi_canfd/ifi_canfd.c
@@ -430,8 +430,6 @@ static int ifi_canfd_handle_lec_err(struct net_device *ndev)
priv->base + IFI_CANFD_INTERRUPT);
writel(IFI_CANFD_ERROR_CTR_ER_ENABLE, priv->base + IFI_CANFD_ERROR_CTR);

- stats->rx_packets++;
- stats->rx_bytes += cf->len;
netif_receive_skb(skb);

return 1;
@@ -456,7 +454,6 @@ static int ifi_canfd_handle_state_change(struct net_device *ndev,
enum can_state new_state)
{
struct ifi_canfd_priv *priv = netdev_priv(ndev);
- struct net_device_stats *stats = &ndev->stats;
struct can_frame *cf;
struct sk_buff *skb;
struct can_berr_counter bec;
@@ -522,8 +519,6 @@ static int ifi_canfd_handle_state_change(struct net_device *ndev,
break;
}

- stats->rx_packets++;
- stats->rx_bytes += cf->len;
netif_receive_skb(skb);

return 1;
diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c
index 74d9899fc904..483fbd9e6952 100644
--- a/drivers/net/can/kvaser_pciefd.c
+++ b/drivers/net/can/kvaser_pciefd.c
@@ -1304,9 +1304,6 @@ static int kvaser_pciefd_rx_error_frame(struct kvaser_pciefd_can *can,
cf->data[6] = bec.txerr;
cf->data[7] = bec.rxerr;

- stats->rx_packets++;
- stats->rx_bytes += cf->len;
-
netif_rx(skb);
return 0;
}
@@ -1504,8 +1501,6 @@ static void kvaser_pciefd_handle_nack_packet(struct kvaser_pciefd_can *can,

if (skb) {
cf->can_id |= CAN_ERR_BUSERROR;
- stats->rx_bytes += cf->len;
- stats->rx_packets++;
netif_rx(skb);
} else {
stats->rx_dropped++;
diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index f4f54012dea7..c33035e706bc 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -647,9 +647,6 @@ static int m_can_handle_lec_err(struct net_device *dev,
break;
}

- stats->rx_packets++;
- stats->rx_bytes += cf->len;
-
if (cdev->is_peripheral)
timestamp = m_can_get_timestamp(cdev);

@@ -706,7 +703,6 @@ static int m_can_handle_state_change(struct net_device *dev,
enum can_state new_state)
{
struct m_can_classdev *cdev = netdev_priv(dev);
- struct net_device_stats *stats = &dev->stats;
struct can_frame *cf;
struct sk_buff *skb;
struct can_berr_counter bec;
@@ -771,9 +767,6 @@ static int m_can_handle_state_change(struct net_device *dev,
break;
}

- stats->rx_packets++;
- stats->rx_bytes += cf->len;
-
if (cdev->is_peripheral)
timestamp = m_can_get_timestamp(cdev);

diff --git a/drivers/net/can/mscan/mscan.c b/drivers/net/can/mscan/mscan.c
index fa32e418eb29..9e1cce0260da 100644
--- a/drivers/net/can/mscan/mscan.c
+++ b/drivers/net/can/mscan/mscan.c
@@ -401,13 +401,14 @@ static int mscan_rx_poll(struct napi_struct *napi, int quota)
continue;
}

- if (canrflg & MSCAN_RXF)
+ if (canrflg & MSCAN_RXF) {
mscan_get_rx_frame(dev, frame);
- else if (canrflg & MSCAN_ERR_IF)
+ stats->rx_packets++;
+ stats->rx_bytes += frame->len;
+ } else if (canrflg & MSCAN_ERR_IF) {
mscan_get_err_frame(dev, frame, canrflg);
+ }

- stats->rx_packets++;
- stats->rx_bytes += frame->len;
work_done++;
netif_receive_skb(skb);
}
diff --git a/drivers/net/can/pch_can.c b/drivers/net/can/pch_can.c
index 964c8a09226a..6b45840db1f9 100644
--- a/drivers/net/can/pch_can.c
+++ b/drivers/net/can/pch_can.c
@@ -561,9 +561,6 @@ static void pch_can_error(struct net_device *ndev, u32 status)

priv->can.state = state;
netif_receive_skb(skb);
-
- stats->rx_packets++;
- stats->rx_bytes += cf->len;
}

static irqreturn_t pch_can_interrupt(int irq, void *dev_id)
diff --git a/drivers/net/can/peak_canfd/peak_canfd.c b/drivers/net/can/peak_canfd/peak_canfd.c
index d08718e98e11..d5b8bc6d2980 100644
--- a/drivers/net/can/peak_canfd/peak_canfd.c
+++ b/drivers/net/can/peak_canfd/peak_canfd.c
@@ -409,8 +409,6 @@ static int pucan_handle_status(struct peak_canfd_priv *priv,
return -ENOMEM;
}

- stats->rx_packets++;
- stats->rx_bytes += cf->len;
pucan_netif_rx(skb, msg->ts_low, msg->ts_high);

return 0;
@@ -438,8 +436,6 @@ static int pucan_handle_cache_critical(struct peak_canfd_priv *priv)
cf->data[6] = priv->bec.txerr;
cf->data[7] = priv->bec.rxerr;

- stats->rx_bytes += cf->len;
- stats->rx_packets++;
netif_rx(skb);

return 0;
diff --git a/drivers/net/can/rcar/rcar_can.c b/drivers/net/can/rcar/rcar_can.c
index 8999ec9455ec..f408ed9a6ccd 100644
--- a/drivers/net/can/rcar/rcar_can.c
+++ b/drivers/net/can/rcar/rcar_can.c
@@ -223,7 +223,6 @@ static void tx_failure_cleanup(struct net_device *ndev)
static void rcar_can_error(struct net_device *ndev)
{
struct rcar_can_priv *priv = netdev_priv(ndev);
- struct net_device_stats *stats = &ndev->stats;
struct can_frame *cf;
struct sk_buff *skb;
u8 eifr, txerr = 0, rxerr = 0;
@@ -362,11 +361,8 @@ static void rcar_can_error(struct net_device *ndev)
}
}

- if (skb) {
- stats->rx_packets++;
- stats->rx_bytes += cf->len;
+ if (skb)
netif_rx(skb);
- }
}

static void rcar_can_tx_done(struct net_device *ndev)
diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index ff9d0f5ae0dd..db9d62874e15 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -1033,8 +1033,6 @@ static void rcar_canfd_error(struct net_device *ndev, u32 cerfl,
/* Clear channel error interrupts that are handled */
rcar_canfd_write(priv->base, RCANFD_CERFL(ch),
RCANFD_CERFL_ERR(~cerfl));
- stats->rx_packets++;
- stats->rx_bytes += cf->len;
netif_rx(skb);
}

@@ -1174,8 +1172,6 @@ static void rcar_canfd_state_change(struct net_device *ndev,
rx_state = txerr <= rxerr ? state : 0;

can_change_state(ndev, cf, tx_state, rx_state);
- stats->rx_packets++;
- stats->rx_bytes += cf->len;
netif_rx(skb);
}
}
diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
index 3fad54646746..a65546ca9461 100644
--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -487,8 +487,6 @@ static int sja1000_err(struct net_device *dev, uint8_t isrc, uint8_t status)
can_bus_off(dev);
}

- stats->rx_packets++;
- stats->rx_bytes += cf->len;
netif_rx(skb);

return 0;
diff --git a/drivers/net/can/sun4i_can.c b/drivers/net/can/sun4i_can.c
index 54aa7c25c4de..599174098883 100644
--- a/drivers/net/can/sun4i_can.c
+++ b/drivers/net/can/sun4i_can.c
@@ -622,13 +622,10 @@ static int sun4i_can_err(struct net_device *dev, u8 isrc, u8 status)
can_bus_off(dev);
}

- if (likely(skb)) {
- stats->rx_packets++;
- stats->rx_bytes += cf->len;
+ if (likely(skb))
netif_rx(skb);
- } else {
+ else
return -ENOMEM;
- }

return 0;
}
diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
index 2b5302e72435..7cf65936d02e 100644
--- a/drivers/net/can/usb/ems_usb.c
+++ b/drivers/net/can/usb/ems_usb.c
@@ -397,8 +397,6 @@ static void ems_usb_rx_err(struct ems_usb *dev, struct ems_cpc_msg *msg)
stats->rx_errors++;
}

- stats->rx_packets++;
- stats->rx_bytes += cf->len;
netif_rx(skb);
}

diff --git a/drivers/net/can/usb/esd_usb2.c b/drivers/net/can/usb/esd_usb2.c
index c6068a251fbe..5f6915a27b3d 100644
--- a/drivers/net/can/usb/esd_usb2.c
+++ b/drivers/net/can/usb/esd_usb2.c
@@ -293,8 +293,6 @@ static void esd_usb2_rx_event(struct esd_usb2_net_priv *priv,
priv->bec.txerr = txerr;
priv->bec.rxerr = rxerr;

- stats->rx_packets++;
- stats->rx_bytes += cf->len;
netif_rx(skb);
}
}
diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c
index 8508a73d648e..2ed2370a3166 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_core.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
@@ -849,13 +849,6 @@ int es58x_rx_err_msg(struct net_device *netdev, enum es58x_err error,
break;
}

- /* driver/net/can/dev.c:can_restart() takes in account error
- * messages in the RX stats. Doing the same here for
- * consistency.
- */
- netdev->stats.rx_packets++;
- netdev->stats.rx_bytes += CAN_ERR_DLC;
-
if (cf) {
if (cf->data[1])
cf->can_id |= CAN_ERR_CRTL;
diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
index 0cc0fc866a2a..3e682ef43f8e 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
@@ -279,8 +279,6 @@ int kvaser_usb_can_rx_over_error(struct net_device *netdev)
cf->can_id |= CAN_ERR_CRTL;
cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;

- stats->rx_packets++;
- stats->rx_bytes += cf->len;
netif_rx(skb);

return 0;
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 dcee8dc828ec..3398da323126 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
@@ -869,7 +869,6 @@ static void kvaser_usb_hydra_update_state(struct kvaser_usb_net_priv *priv,
struct net_device *netdev = priv->netdev;
struct can_frame *cf;
struct sk_buff *skb;
- struct net_device_stats *stats;
enum can_state new_state, old_state;

old_state = priv->can.state;
@@ -919,9 +918,6 @@ static void kvaser_usb_hydra_update_state(struct kvaser_usb_net_priv *priv,
cf->data[6] = bec->txerr;
cf->data[7] = bec->rxerr;

- stats = &netdev->stats;
- stats->rx_packets++;
- stats->rx_bytes += cf->len;
netif_rx(skb);
}

@@ -1074,8 +1070,6 @@ kvaser_usb_hydra_error_frame(struct kvaser_usb_net_priv *priv,
cf->data[6] = bec.txerr;
cf->data[7] = bec.rxerr;

- stats->rx_packets++;
- stats->rx_bytes += cf->len;
netif_rx(skb);

priv->bec.txerr = bec.txerr;
@@ -1109,8 +1103,6 @@ static void kvaser_usb_hydra_one_shot_fail(struct kvaser_usb_net_priv *priv,
}

stats->tx_errors++;
- stats->rx_packets++;
- stats->rx_bytes += cf->len;
netif_rx(skb);
}

diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
index 59ba7c7beec0..4aebaab9ea9c 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
@@ -575,8 +575,6 @@ static void kvaser_usb_leaf_tx_acknowledge(const struct kvaser_usb *dev,
if (skb) {
cf->can_id |= CAN_ERR_RESTARTED;

- stats->rx_packets++;
- stats->rx_bytes += cf->len;
netif_rx(skb);
} else {
netdev_err(priv->netdev,
@@ -777,8 +775,6 @@ static void kvaser_usb_leaf_rx_error(const struct kvaser_usb *dev,
cf->data[6] = es->txerr;
cf->data[7] = es->rxerr;

- stats->rx_packets++;
- stats->rx_bytes += cf->len;
netif_rx(skb);
}

diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
index 876218752766..21b06a738595 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
@@ -520,8 +520,6 @@ static int pcan_usb_decode_error(struct pcan_usb_msg_context *mc, u8 n,
&hwts->hwtstamp);
}

- mc->netdev->stats.rx_packets++;
- mc->netdev->stats.rx_bytes += cf->len;
netif_rx(skb);

return 0;
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
index 6bd12549f101..185f5a98d217 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
@@ -577,9 +577,6 @@ static int pcan_usb_fd_decode_status(struct pcan_usb_fd_if *usb_if,
if (!skb)
return -ENOMEM;

- netdev->stats.rx_packets++;
- netdev->stats.rx_bytes += cf->len;
-
peak_usb_netif_rx_64(skb, le32_to_cpu(sm->ts_low),
le32_to_cpu(sm->ts_high));

diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
index 858ab22708fc..f6d19879bf40 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
@@ -660,8 +660,6 @@ static int pcan_usb_pro_handle_error(struct pcan_usb_pro_interface *usb_if,

hwts = skb_hwtstamps(skb);
peak_usb_get_ts_time(&usb_if->time_ref, le32_to_cpu(er->ts32), &hwts->hwtstamp);
- netdev->stats.rx_packets++;
- netdev->stats.rx_bytes += can_frame->len;
netif_rx(skb);

return 0;
diff --git a/drivers/net/can/usb/ucan.c b/drivers/net/can/usb/ucan.c
index 1679cbe45ded..d582c39fc8d0 100644
--- a/drivers/net/can/usb/ucan.c
+++ b/drivers/net/can/usb/ucan.c
@@ -621,8 +621,10 @@ static void ucan_rx_can_msg(struct ucan_priv *up, struct ucan_message_in *m)
memcpy(cf->data, m->msg.can_msg.data, cf->len);

/* don't count error frames as real packets */
- stats->rx_packets++;
- stats->rx_bytes += cf->len;
+ if (!(cf->can_id & CAN_ERR_FLAG)) {
+ stats->rx_packets++;
+ stats->rx_bytes += cf->len;
+ }

/* pass it to Linux */
netif_rx(skb);
diff --git a/drivers/net/can/usb/usb_8dev.c b/drivers/net/can/usb/usb_8dev.c
index d1b83bd1b3cb..040324362b26 100644
--- a/drivers/net/can/usb/usb_8dev.c
+++ b/drivers/net/can/usb/usb_8dev.c
@@ -449,8 +449,6 @@ static void usb_8dev_rx_err_msg(struct usb_8dev_priv *priv,
priv->bec.txerr = txerr;
priv->bec.rxerr = rxerr;

- stats->rx_packets++;
- stats->rx_bytes += cf->len;
netif_rx(skb);
}

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index e2b15d29d15e..275e240ab293 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -965,13 +965,8 @@ static void xcan_update_error_state_after_rxtx(struct net_device *ndev)

xcan_set_error_state(ndev, new_state, skb ? cf : NULL);

- if (skb) {
- struct net_device_stats *stats = &ndev->stats;
-
- stats->rx_packets++;
- stats->rx_bytes += cf->len;
+ if (skb)
netif_rx(skb);
- }
}
}

@@ -1095,8 +1090,6 @@ static void xcan_err_interrupt(struct net_device *ndev, u32 isr)
if (skb) {
skb_cf->can_id |= cf.can_id;
memcpy(skb_cf->data, cf.data, CAN_ERR_DLC);
- stats->rx_packets++;
- stats->rx_bytes += CAN_ERR_DLC;
netif_rx(skb);
}
}
--
2.32.0

2021-12-07 12:16:35

by Vincent MAILHOL

[permalink] [raw]

Subject: [PATCH v5 2/5] can: kvaser_usb: do not increase tx statistics when sending error message frames

2021-12-07 12:16:38

by Vincent MAILHOL

[permalink] [raw]

Subject: [PATCH v5 3/5] can: do not copy the payload of RTR frames

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.

For this reason, it is incorrect to copy the payload of RTR frames
(the payload buffer would only contain garbage data). This patch
encapsulates the payload copy in a check toward the RTR flag.

CC: Yasushi SHOJI <[email protected]>
Signed-off-by: Vincent Mailhol <[email protected]>
---
drivers/net/can/pch_can.c | 15 ++++++++-------
drivers/net/can/spi/mcp251x.c | 3 ++-
drivers/net/can/usb/mcba_usb.c | 8 ++++----
3 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/net/can/pch_can.c b/drivers/net/can/pch_can.c
index 6b45840db1f9..4bf9bfc4de72 100644
--- a/drivers/net/can/pch_can.c
+++ b/drivers/net/can/pch_can.c
@@ -677,16 +677,17 @@ static int pch_can_rx_normal(struct net_device *ndev, u32 obj_num, int quota)
cf->can_id = id;
}

- if (id2 & PCH_ID2_DIR)
- cf->can_id |= CAN_RTR_FLAG;
-
cf->len = can_cc_dlc2len((ioread32(&priv->regs->
ifregs[0].mcont)) & 0xF);

- for (i = 0; i < cf->len; i += 2) {
- data_reg = ioread16(&priv->regs->ifregs[0].data[i / 2]);
- cf->data[i] = data_reg;
- cf->data[i + 1] = data_reg >> 8;
+ if (id2 & PCH_ID2_DIR) {
+ cf->can_id |= CAN_RTR_FLAG;
+ } else {
+ for (i = 0; i < cf->len; i += 2) {
+ data_reg = ioread16(&priv->regs->ifregs[0].data[i / 2]);
+ cf->data[i] = data_reg;
+ cf->data[i + 1] = data_reg >> 8;
+ }
}

rcv_pkts++;
diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c
index 0579ab74f728..db3fa98569c4 100644
--- a/drivers/net/can/spi/mcp251x.c
+++ b/drivers/net/can/spi/mcp251x.c
@@ -733,7 +733,8 @@ static void mcp251x_hw_rx(struct spi_device *spi, int buf_idx)
}
/* Data length */
frame->len = can_cc_dlc2len(buf[RXBDLC_OFF] & RXBDLC_LEN_MASK);
- memcpy(frame->data, buf + RXBDAT_OFF, frame->len);
+ if (!(frame->can_id & CAN_RTR_FLAG))
+ memcpy(frame->data, buf + RXBDAT_OFF, frame->len);

priv->net->stats.rx_packets++;
priv->net->stats.rx_bytes += frame->len;
diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c
index a1a154c08b7f..162d2e11cadd 100644
--- a/drivers/net/can/usb/mcba_usb.c
+++ b/drivers/net/can/usb/mcba_usb.c
@@ -450,12 +450,12 @@ static void mcba_usb_process_can(struct mcba_priv *priv,
cf->can_id = (sid & 0xffe0) >> 5;
}

- if (msg->dlc & MCBA_DLC_RTR_MASK)
- cf->can_id |= CAN_RTR_FLAG;
-
cf->len = can_cc_dlc2len(msg->dlc & MCBA_DLC_MASK);

- memcpy(cf->data, msg->data, cf->len);
+ if (msg->dlc & MCBA_DLC_RTR_MASK)
+ cf->can_id |= CAN_RTR_FLAG;
+ else
+ memcpy(cf->data, msg->data, cf->len);

stats->rx_packets++;
stats->rx_bytes += cf->len;
--
2.32.0

2021-12-07 12:16:48

by Vincent MAILHOL

[permalink] [raw]

Subject: [PATCH v5 4/5] can: do not increase rx_bytes statistics for RTR frames

2021-12-07 12:17:09

by Vincent MAILHOL

[permalink] [raw]

Subject: [PATCH v5 5/5] can: do not increase tx_bytes statistics for RTR frames

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")

CC: Nicolas Ferre <[email protected]>
CC: Alexandre Belloni <[email protected]>
CC: Ludovic Desroches <[email protected]>
CC: Maxime Ripard <[email protected]>
CC: Chen-Yu Tsai <[email protected]>
CC: Jernej Skrabec <[email protected]>
CC: Yasushi SHOJI <[email protected]>
CC: Oliver Hartkopp <[email protected]>
CC: Stephane Grosjean <[email protected]>
Tested-by: Jimmy Assarsson <[email protected]> # kvaser
Signed-off-by: Vincent Mailhol <[email protected]>
---
drivers/net/can/at91_can.c | 8 ++--
drivers/net/can/c_can/c_can.h | 1 -
drivers/net/can/c_can/c_can_main.c | 7 +---
drivers/net/can/cc770/cc770.c | 8 +---
drivers/net/can/janz-ican3.c | 3 +-
drivers/net/can/mscan/mscan.c | 4 +-
drivers/net/can/pch_can.c | 9 ++--
drivers/net/can/peak_canfd/peak_canfd.c | 3 +-
drivers/net/can/rcar/rcar_can.c | 11 +++--
drivers/net/can/rcar/rcar_canfd.c | 6 +--
drivers/net/can/sja1000/sja1000.c | 4 +-
drivers/net/can/slcan.c | 3 +-
drivers/net/can/softing/softing_main.c | 8 ++--
drivers/net/can/spi/hi311x.c | 24 +++++------
drivers/net/can/spi/mcp251x.c | 25 +++++------
drivers/net/can/sun4i_can.c | 5 +--
drivers/net/can/usb/ems_usb.c | 7 +---
drivers/net/can/usb/esd_usb2.c | 6 +--
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 | 2 +-
.../net/can/usb/kvaser_usb/kvaser_usb_hydra.c | 42 +++++++++----------
.../net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 13 +++---
drivers/net/can/usb/mcba_usb.c | 12 ++----
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/ucan.c | 10 ++---
drivers/net/can/usb/usb_8dev.c | 6 +--
drivers/net/can/vcan.c | 7 ++--
drivers/net/can/vxcan.c | 2 +-
include/linux/can/skb.h | 5 ++-
31 files changed, 114 insertions(+), 160 deletions(-)

diff --git a/drivers/net/can/at91_can.c b/drivers/net/can/at91_can.c
index 97f1d08b4133..a00655ccda02 100644
--- a/drivers/net/can/at91_can.c
+++ b/drivers/net/can/at91_can.c
@@ -448,7 +448,6 @@ static void at91_chip_stop(struct net_device *dev, enum can_state state)
static netdev_tx_t at91_start_xmit(struct sk_buff *skb, struct net_device *dev)
{
struct at91_priv *priv = netdev_priv(dev);
- struct net_device_stats *stats = &dev->stats;
struct can_frame *cf = (struct can_frame *)skb->data;
unsigned int mb, prio;
u32 reg_mid, reg_mcr;
@@ -480,8 +479,6 @@ static netdev_tx_t at91_start_xmit(struct sk_buff *skb, struct net_device *dev)
/* This triggers transmission */
at91_write(priv, AT91_MCR(mb), reg_mcr);

- stats->tx_bytes += cf->len;
-
/* _NOTE_: subtract AT91_MB_TX_FIRST offset from mb! */
can_put_echo_skb(skb, dev, mb - get_mb_tx_first(priv), 0);

@@ -852,7 +849,10 @@ static void at91_irq_tx(struct net_device *dev, u32 reg_sr)
if (likely(reg_msr & AT91_MSR_MRDY &&
~reg_msr & AT91_MSR_MABT)) {
/* _NOTE_: subtract AT91_MB_TX_FIRST offset from mb! */
- can_get_echo_skb(dev, mb - get_mb_tx_first(priv), NULL);
+ dev->stats.tx_bytes +=
+ can_get_echo_skb(dev,
+ mb - get_mb_tx_first(priv),
+ NULL);
dev->stats.tx_packets++;
can_led_event(dev, CAN_LED_EVENT_TX);
}
diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
index 08b6efa7a1a7..bd2f6dc01194 100644
--- a/drivers/net/can/c_can/c_can.h
+++ b/drivers/net/can/c_can/c_can.h
@@ -211,7 +211,6 @@ struct c_can_priv {
struct c_can_raminit raminit_sys; /* RAMINIT via syscon regmap */
void (*raminit)(const struct c_can_priv *priv, bool enable);
u32 comm_rcv_high;
- u32 dlc[];
};

struct net_device *alloc_c_can_dev(int msg_obj_num);
diff --git a/drivers/net/can/c_can/c_can_main.c b/drivers/net/can/c_can/c_can_main.c
index 29e91804d81c..faa217f26771 100644
--- a/drivers/net/can/c_can/c_can_main.c
+++ b/drivers/net/can/c_can/c_can_main.c
@@ -477,7 +477,6 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
* transmit as we might race against do_tx().
*/
c_can_setup_tx_object(dev, IF_TX, frame, idx);
- priv->dlc[idx] = frame->len;
can_put_echo_skb(skb, dev, idx, 0);
obj = idx + priv->msg_obj_tx_first;
c_can_object_put(dev, IF_TX, obj, cmd);
@@ -742,8 +741,7 @@ static void c_can_do_tx(struct net_device *dev)
* NAPI. We are not transmitting.
*/
c_can_inval_tx_object(dev, IF_NAPI, obj);
- can_get_echo_skb(dev, idx, NULL);
- bytes += priv->dlc[idx];
+ bytes += can_get_echo_skb(dev, idx, NULL);
pkts++;
}

@@ -1227,8 +1225,7 @@ struct net_device *alloc_c_can_dev(int msg_obj_num)
struct c_can_priv *priv;
int msg_obj_tx_num = msg_obj_num / 2;

- dev = alloc_candev(struct_size(priv, dlc, msg_obj_tx_num),
- msg_obj_tx_num);
+ dev = alloc_candev(sizeof(*priv), msg_obj_tx_num);
if (!dev)
return NULL;

diff --git a/drivers/net/can/cc770/cc770.c b/drivers/net/can/cc770/cc770.c
index 994073c0a2f6..bb7224cfc6ab 100644
--- a/drivers/net/can/cc770/cc770.c
+++ b/drivers/net/can/cc770/cc770.c
@@ -664,7 +664,6 @@ static void cc770_tx_interrupt(struct net_device *dev, unsigned int o)
struct cc770_priv *priv = netdev_priv(dev);
struct net_device_stats *stats = &dev->stats;
unsigned int mo = obj2msgobj(o);
- struct can_frame *cf;
u8 ctrl1;

ctrl1 = cc770_read_reg(priv, msgobj[mo].ctrl1);
@@ -696,12 +695,9 @@ static void cc770_tx_interrupt(struct net_device *dev, unsigned int o)
return;
}

- cf = (struct can_frame *)priv->tx_skb->data;
- stats->tx_bytes += cf->len;
- stats->tx_packets++;
-
can_put_echo_skb(priv->tx_skb, dev, 0, 0);
- can_get_echo_skb(dev, 0, NULL);
+ stats->tx_bytes += can_get_echo_skb(dev, 0, NULL);
+ stats->tx_packets++;
priv->tx_skb = NULL;

netif_wake_queue(dev);
diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
index 5c589aa9dff8..5b677af5f2a4 100644
--- a/drivers/net/can/janz-ican3.c
+++ b/drivers/net/can/janz-ican3.c
@@ -1294,7 +1294,8 @@ static unsigned int ican3_get_echo_skb(struct ican3_dev *mod)
}

cf = (struct can_frame *)skb->data;
- dlc = cf->len;
+ if (!(cf->can_id & CAN_RTR_FLAG))
+ dlc = cf->len;

/* check flag whether this packet has to be looped back */
if (skb->pkt_type != PACKET_LOOPBACK) {
diff --git a/drivers/net/can/mscan/mscan.c b/drivers/net/can/mscan/mscan.c
index 59b8284d00e5..5b5802fac772 100644
--- a/drivers/net/can/mscan/mscan.c
+++ b/drivers/net/can/mscan/mscan.c
@@ -448,9 +448,9 @@ static irqreturn_t mscan_isr(int irq, void *dev_id)
continue;

out_8(&regs->cantbsel, mask);
- stats->tx_bytes += in_8(&regs->tx.dlr);
+ stats->tx_bytes += can_get_echo_skb(dev, entry->id,
+ NULL);
stats->tx_packets++;
- can_get_echo_skb(dev, entry->id, NULL);
priv->tx_active &= ~mask;
list_del(pos);
}
diff --git a/drivers/net/can/pch_can.c b/drivers/net/can/pch_can.c
index b46f9cfb9e0a..888bef03de09 100644
--- a/drivers/net/can/pch_can.c
+++ b/drivers/net/can/pch_can.c
@@ -707,16 +707,13 @@ static void pch_can_tx_complete(struct net_device *ndev, u32 int_stat)
{
struct pch_can_priv *priv = netdev_priv(ndev);
struct net_device_stats *stats = &(priv->ndev->stats);
- u32 dlc;

- can_get_echo_skb(ndev, int_stat - PCH_RX_OBJ_END - 1, NULL);
+ stats->tx_bytes += can_get_echo_skb(ndev, int_stat - PCH_RX_OBJ_END - 1,
+ NULL);
+ stats->tx_packets++;
iowrite32(PCH_CMASK_RX_TX_GET | PCH_CMASK_CLRINTPND,
&priv->regs->ifregs[1].cmask);
pch_can_rw_msg_obj(&priv->regs->ifregs[1].creq, int_stat);
- dlc = can_cc_dlc2len(ioread32(&priv->regs->ifregs[1].mcont) &
- PCH_IF_MCONT_DLC);
- stats->tx_bytes += dlc;
- stats->tx_packets++;
if (int_stat == PCH_TX_OBJ_END)
netif_wake_queue(ndev);
}
diff --git a/drivers/net/can/peak_canfd/peak_canfd.c b/drivers/net/can/peak_canfd/peak_canfd.c
index 216609198eac..b2dea360813d 100644
--- a/drivers/net/can/peak_canfd/peak_canfd.c
+++ b/drivers/net/can/peak_canfd/peak_canfd.c
@@ -266,10 +266,9 @@ static int pucan_handle_can_rx(struct peak_canfd_priv *priv,
unsigned long flags;

spin_lock_irqsave(&priv->echo_lock, flags);
- can_get_echo_skb(priv->ndev, msg->client, NULL);

/* count bytes of the echo instead of skb */
- stats->tx_bytes += cf_len;
+ stats->tx_bytes += can_get_echo_skb(priv->ndev, msg->client, NULL);
stats->tx_packets++;

/* restart tx queue (a slot is free) */
diff --git a/drivers/net/can/rcar/rcar_can.c b/drivers/net/can/rcar/rcar_can.c
index 62bbd58bfef8..33e37395379d 100644
--- a/drivers/net/can/rcar/rcar_can.c
+++ b/drivers/net/can/rcar/rcar_can.c
@@ -94,7 +94,6 @@ struct rcar_can_priv {
struct rcar_can_regs __iomem *regs;
struct clk *clk;
struct clk *can_clk;
- u8 tx_dlc[RCAR_CAN_FIFO_DEPTH];
u32 tx_head;
u32 tx_tail;
u8 clock_select;
@@ -379,10 +378,11 @@ static void rcar_can_tx_done(struct net_device *ndev)
if (priv->tx_head - priv->tx_tail <= unsent)
break;
stats->tx_packets++;
- stats->tx_bytes += priv->tx_dlc[priv->tx_tail %
- RCAR_CAN_FIFO_DEPTH];
- priv->tx_dlc[priv->tx_tail % RCAR_CAN_FIFO_DEPTH] = 0;
- can_get_echo_skb(ndev, priv->tx_tail % RCAR_CAN_FIFO_DEPTH, NULL);
+ stats->tx_bytes +=
+ can_get_echo_skb(ndev,
+ priv->tx_tail % RCAR_CAN_FIFO_DEPTH,
+ NULL);
+
priv->tx_tail++;
netif_wake_queue(ndev);
}
@@ -612,7 +612,6 @@ static netdev_tx_t rcar_can_start_xmit(struct sk_buff *skb,

writeb(cf->len, &priv->regs->mb[RCAR_CAN_TX_FIFO_MBX].dlc);

- priv->tx_dlc[priv->tx_head % RCAR_CAN_FIFO_DEPTH] = cf->len;
can_put_echo_skb(skb, ndev, priv->tx_head % RCAR_CAN_FIFO_DEPTH, 0);
priv->tx_head++;
/* Start Tx: write 0xff to the TFPCR register to increment
diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index b1eded2f2c5d..d0e795f60338 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -502,7 +502,6 @@ struct rcar_canfd_channel {
struct rcar_canfd_global *gpriv; /* Controller reference */
void __iomem *base; /* Register base address */
struct napi_struct napi;
- u8 tx_len[RCANFD_FIFO_DEPTH]; /* For net stats */
u32 tx_head; /* Incremented on xmit */
u32 tx_tail; /* Incremented on xmit done */
u32 channel; /* Channel number */
@@ -1049,9 +1048,7 @@ static void rcar_canfd_tx_done(struct net_device *ndev)

sent = priv->tx_tail % RCANFD_FIFO_DEPTH;
stats->tx_packets++;
- stats->tx_bytes += priv->tx_len[sent];
- priv->tx_len[sent] = 0;
- can_get_echo_skb(ndev, sent, NULL);
+ stats->tx_bytes += can_get_echo_skb(ndev, sent, NULL);

spin_lock_irqsave(&priv->tx_lock, flags);
priv->tx_tail++;
@@ -1461,7 +1458,6 @@ static netdev_tx_t rcar_canfd_start_xmit(struct sk_buff *skb,
RCANFD_C_CFDF(ch, RCANFD_CFFIFO_IDX, 0));
}

- priv->tx_len[priv->tx_head % RCANFD_FIFO_DEPTH] = cf->len;
can_put_echo_skb(skb, ndev, priv->tx_head % RCANFD_FIFO_DEPTH, 0);

spin_lock_irqsave(&priv->tx_lock, flags);
diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
index 4bf44d449987..966316479485 100644
--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -527,10 +527,8 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
can_free_echo_skb(dev, 0, NULL);
} else {
/* transmission complete */
- stats->tx_bytes +=
- priv->read_reg(priv, SJA1000_FI) & 0xf;
+ stats->tx_bytes += can_get_echo_skb(dev, 0, NULL);
stats->tx_packets++;
- can_get_echo_skb(dev, 0, NULL);
}
netif_wake_queue(dev);
can_led_event(dev, CAN_LED_EVENT_TX);
diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index 5cf03458e948..d4c7ce998a34 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -290,6 +290,8 @@ static void slc_encaps(struct slcan *sl, struct can_frame *cf)
if (!(cf->can_id & CAN_RTR_FLAG)) {
for (i = 0; i < cf->len; i++)
pos = hex_byte_pack_upper(pos, cf->data[i]);
+
+ sl->dev->stats.tx_bytes += cf->len;
}

*pos++ = '\r';
@@ -306,7 +308,6 @@ static void slc_encaps(struct slcan *sl, struct can_frame *cf)
actual = sl->tty->ops->write(sl->tty, sl->xbuff, pos - sl->xbuff);
sl->xleft = (pos - sl->xbuff) - actual;
sl->xhead = sl->xbuff + actual;
- sl->dev->stats.tx_bytes += cf->len;
}

/* Write out any remaining transmit buffer. Scheduled when tty is writable */
diff --git a/drivers/net/can/softing/softing_main.c b/drivers/net/can/softing/softing_main.c
index cfc1325aad10..d74e895bddf7 100644
--- a/drivers/net/can/softing/softing_main.c
+++ b/drivers/net/can/softing/softing_main.c
@@ -282,7 +282,10 @@ static int softing_handle_1(struct softing *card)
skb = priv->can.echo_skb[priv->tx.echo_get];
if (skb)
skb->tstamp = ktime;
- can_get_echo_skb(netdev, priv->tx.echo_get, NULL);
+ ++netdev->stats.tx_packets;
+ netdev->stats.tx_bytes +=
+ can_get_echo_skb(netdev, priv->tx.echo_get,
+ NULL);
++priv->tx.echo_get;
if (priv->tx.echo_get >= TX_ECHO_SKB_MAX)
priv->tx.echo_get = 0;
@@ -290,9 +293,6 @@ static int softing_handle_1(struct softing *card)
--priv->tx.pending;
if (card->tx.pending)
--card->tx.pending;
- ++netdev->stats.tx_packets;
- if (!(msg.can_id & CAN_RTR_FLAG))
- netdev->stats.tx_bytes += msg.len;
} else {
int ret;

diff --git a/drivers/net/can/spi/hi311x.c b/drivers/net/can/spi/hi311x.c
index 771a13945032..59e01c258490 100644
--- a/drivers/net/can/spi/hi311x.c
+++ b/drivers/net/can/spi/hi311x.c
@@ -153,7 +153,6 @@ struct hi3110_priv {
u8 *spi_rx_buf;

struct sk_buff *tx_skb;
- int tx_len;

struct workqueue_struct *wq;
struct work_struct tx_work;
@@ -166,6 +165,8 @@ struct hi3110_priv {
#define HI3110_AFTER_SUSPEND_POWER 4
#define HI3110_AFTER_SUSPEND_RESTART 8
int restart_tx;
+ bool tx_busy;
+
struct regulator *power;
struct regulator *transceiver;
struct clk *clk;
@@ -175,13 +176,13 @@ static void hi3110_clean(struct net_device *net)
{
struct hi3110_priv *priv = netdev_priv(net);

- if (priv->tx_skb || priv->tx_len)
+ if (priv->tx_skb || priv->tx_busy)
net->stats.tx_errors++;
dev_kfree_skb(priv->tx_skb);
- if (priv->tx_len)
+ if (priv->tx_busy)
can_free_echo_skb(priv->net, 0, NULL);
priv->tx_skb = NULL;
- priv->tx_len = 0;
+ priv->tx_busy = false;
}

/* Note about handling of error return of hi3110_spi_trans: accessing
@@ -369,7 +370,7 @@ static netdev_tx_t hi3110_hard_start_xmit(struct sk_buff *skb,
struct hi3110_priv *priv = netdev_priv(net);
struct spi_device *spi = priv->spi;

- if (priv->tx_skb || priv->tx_len) {
+ if (priv->tx_skb || priv->tx_busy) {
dev_err(&spi->dev, "hard_xmit called while tx busy\n");
return NETDEV_TX_BUSY;
}
@@ -586,7 +587,7 @@ static void hi3110_tx_work_handler(struct work_struct *ws)
} else {
frame = (struct can_frame *)priv->tx_skb->data;
hi3110_hw_tx(spi, frame);
- priv->tx_len = 1 + frame->len;
+ priv->tx_busy = true;
can_put_echo_skb(priv->tx_skb, net, 0, 0);
priv->tx_skb = NULL;
}
@@ -721,14 +722,11 @@ static irqreturn_t hi3110_can_ist(int irq, void *dev_id)
}
}

- if (priv->tx_len && statf & HI3110_STAT_TXMTY) {
+ if (priv->tx_busy && statf & HI3110_STAT_TXMTY) {
net->stats.tx_packets++;
- net->stats.tx_bytes += priv->tx_len - 1;
+ net->stats.tx_bytes += can_get_echo_skb(net, 0, NULL);
can_led_event(net, CAN_LED_EVENT_TX);
- if (priv->tx_len) {
- can_get_echo_skb(net, 0, NULL);
- priv->tx_len = 0;
- }
+ priv->tx_busy = false;
netif_wake_queue(net);
}

@@ -755,7 +753,7 @@ static int hi3110_open(struct net_device *net)

priv->force_quit = 0;
priv->tx_skb = NULL;
- priv->tx_len = 0;
+ priv->tx_busy = false;

ret = request_threaded_irq(spi->irq, NULL, hi3110_can_ist,
flags, DEVICE_NAME, priv);
diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c
index b46516b7d39f..69a3395385d3 100644
--- a/drivers/net/can/spi/mcp251x.c
+++ b/drivers/net/can/spi/mcp251x.c
@@ -237,7 +237,6 @@ struct mcp251x_priv {
u8 *spi_rx_buf;

struct sk_buff *tx_skb;
- int tx_len;

struct workqueue_struct *wq;
struct work_struct tx_work;
@@ -250,6 +249,8 @@ struct mcp251x_priv {
#define AFTER_SUSPEND_POWER 4
#define AFTER_SUSPEND_RESTART 8
int restart_tx;
+ bool tx_busy;
+
struct regulator *power;
struct regulator *transceiver;
struct clk *clk;
@@ -272,13 +273,13 @@ static void mcp251x_clean(struct net_device *net)
{
struct mcp251x_priv *priv = netdev_priv(net);

- if (priv->tx_skb || priv->tx_len)
+ if (priv->tx_skb || priv->tx_busy)
net->stats.tx_errors++;
dev_kfree_skb(priv->tx_skb);
- if (priv->tx_len)
+ if (priv->tx_busy)
can_free_echo_skb(priv->net, 0, NULL);
priv->tx_skb = NULL;
- priv->tx_len = 0;
+ priv->tx_busy = false;
}

/* Note about handling of error return of mcp251x_spi_trans: accessing
@@ -788,7 +789,7 @@ static netdev_tx_t mcp251x_hard_start_xmit(struct sk_buff *skb,
struct mcp251x_priv *priv = netdev_priv(net);
struct spi_device *spi = priv->spi;

- if (priv->tx_skb || priv->tx_len) {
+ if (priv->tx_skb || priv->tx_busy) {
dev_warn(&spi->dev, "hard_xmit called while tx busy\n");
return NETDEV_TX_BUSY;
}
@@ -1013,7 +1014,7 @@ static void mcp251x_tx_work_handler(struct work_struct *ws)
if (frame->len > CAN_FRAME_MAX_DATA_LEN)
frame->len = CAN_FRAME_MAX_DATA_LEN;
mcp251x_hw_tx(spi, frame, 0);
- priv->tx_len = 1 + frame->len;
+ priv->tx_busy = true;
can_put_echo_skb(priv->tx_skb, net, 0, 0);
priv->tx_skb = NULL;
}
@@ -1179,12 +1180,12 @@ static irqreturn_t mcp251x_can_ist(int irq, void *dev_id)
break;

if (intf & CANINTF_TX) {
- net->stats.tx_packets++;
- net->stats.tx_bytes += priv->tx_len - 1;
can_led_event(net, CAN_LED_EVENT_TX);
- if (priv->tx_len) {
- can_get_echo_skb(net, 0, NULL);
- priv->tx_len = 0;
+ if (priv->tx_busy) {
+ net->stats.tx_packets++;
+ net->stats.tx_bytes += can_get_echo_skb(net, 0,
+ NULL);
+ priv->tx_busy = false;
}
netif_wake_queue(net);
}
@@ -1211,7 +1212,7 @@ static int mcp251x_open(struct net_device *net)

priv->force_quit = 0;
priv->tx_skb = NULL;
- priv->tx_len = 0;
+ priv->tx_busy = false;

if (!dev_fwnode(&spi->dev))
flags = IRQF_TRIGGER_FALLING;
diff --git a/drivers/net/can/sun4i_can.c b/drivers/net/can/sun4i_can.c
index 6a7d89c4648c..81884ef5ceaf 100644
--- a/drivers/net/can/sun4i_can.c
+++ b/drivers/net/can/sun4i_can.c
@@ -650,11 +650,8 @@ static irqreturn_t sun4i_can_interrupt(int irq, void *dev_id)

if (isrc & SUN4I_INT_TBUF_VLD) {
/* transmission complete interrupt */
- stats->tx_bytes +=
- readl(priv->base +
- SUN4I_REG_RBUF_RBACK_START_ADDR) & 0xf;
+ stats->tx_bytes += can_get_echo_skb(dev, 0, NULL);
stats->tx_packets++;
- can_get_echo_skb(dev, 0, NULL);
netif_wake_queue(dev);
can_led_event(dev, CAN_LED_EVENT_TX);
}
diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
index c9b6adf5c1ec..7bedceffdfa3 100644
--- a/drivers/net/can/usb/ems_usb.c
+++ b/drivers/net/can/usb/ems_usb.c
@@ -230,7 +230,6 @@ struct ems_tx_urb_context {
struct ems_usb *dev;

u32 echo_index;
- u8 dlc;
};

struct ems_usb {
@@ -517,9 +516,8 @@ static void ems_usb_write_bulk_callback(struct urb *urb)

/* transmission complete interrupt */
netdev->stats.tx_packets++;
- netdev->stats.tx_bytes += context->dlc;
-
- can_get_echo_skb(netdev, context->echo_index, NULL);
+ netdev->stats.tx_bytes += can_get_echo_skb(netdev, context->echo_index,
+ NULL);

/* Release context */
context->echo_index = MAX_TX_URBS;
@@ -805,7 +803,6 @@ static netdev_tx_t ems_usb_start_xmit(struct sk_buff *skb, struct net_device *ne

context->dev = dev;
context->echo_index = i;
- context->dlc = cf->len;

usb_fill_bulk_urb(urb, dev->udev, usb_sndbulkpipe(dev->udev, 2), buf,
size, ems_usb_write_bulk_callback, context);
diff --git a/drivers/net/can/usb/esd_usb2.c b/drivers/net/can/usb/esd_usb2.c
index 9ac7ee44b6e3..286daaaea0b8 100644
--- a/drivers/net/can/usb/esd_usb2.c
+++ b/drivers/net/can/usb/esd_usb2.c
@@ -183,7 +183,6 @@ struct esd_usb2_net_priv;
struct esd_tx_urb_context {
struct esd_usb2_net_priv *priv;
u32 echo_index;
- int len; /* CAN payload length */
};

struct esd_usb2 {
@@ -357,8 +356,8 @@ static void esd_usb2_tx_done_msg(struct esd_usb2_net_priv *priv,

if (!msg->msg.txdone.status) {
stats->tx_packets++;
- stats->tx_bytes += context->len;
- can_get_echo_skb(netdev, context->echo_index, NULL);
+ stats->tx_bytes += can_get_echo_skb(netdev, context->echo_index,
+ NULL);
} else {
stats->tx_errors++;
can_free_echo_skb(netdev, context->echo_index, NULL);
@@ -783,7 +782,6 @@ static netdev_tx_t esd_usb2_start_xmit(struct sk_buff *skb,

context->priv = priv;
context->echo_index = i;
- context->len = cf->len;

/* hnd must not be 0 - MSB is stripped in txdone handling */
msg->msg.tx.hnd = 0x80000000 | i; /* returned in TX done message */
diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 1b400de00f51..03b012963c20 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -357,9 +357,6 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
goto resubmit_urb;
}

- netdev->stats.tx_packets++;
- netdev->stats.tx_bytes += hf->can_dlc;
-
txc = gs_get_tx_context(dev, hf->echo_id);

/* bad devices send bad echo_ids. */
@@ -370,7 +367,9 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
goto resubmit_urb;
}

- can_get_echo_skb(netdev, hf->echo_id, NULL);
+ netdev->stats.tx_packets++;
+ netdev->stats.tx_bytes += can_get_echo_skb(netdev, hf->echo_id,
+ NULL);

gs_free_tx_context(txc);

diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb.h b/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
index 390b6bde883c..3a49257f9fa6 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
@@ -77,7 +77,6 @@ struct kvaser_usb_dev_card_data {
struct kvaser_usb_tx_urb_context {
struct kvaser_usb_net_priv *priv;
u32 echo_index;
- int dlc;
};

struct kvaser_usb {
@@ -162,8 +161,8 @@ struct kvaser_usb_dev_ops {
void (*dev_read_bulk_callback)(struct kvaser_usb *dev, void *buf,
int len);
void *(*dev_frame_to_cmd)(const struct kvaser_usb_net_priv *priv,
- const struct sk_buff *skb, int *frame_len,
- int *cmd_len, u16 transid);
+ const struct sk_buff *skb, int *cmd_len,
+ u16 transid);
};

struct kvaser_usb_dev_cfg {
diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
index 3e682ef43f8e..c4b4d3d0a387 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
@@ -565,7 +565,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
goto freeurb;
}

- buf = dev->ops->dev_frame_to_cmd(priv, skb, &context->dlc, &cmd_len,
+ buf = dev->ops->dev_frame_to_cmd(priv, skb, &cmd_len,
context->echo_index);
if (!buf) {
stats->tx_dropped++;
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 47e3d3539e78..3cbb7f3051f1 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
@@ -1113,6 +1113,7 @@ static void kvaser_usb_hydra_tx_acknowledge(const struct kvaser_usb *dev,
struct kvaser_usb_tx_urb_context *context;
struct kvaser_usb_net_priv *priv;
unsigned long irq_flags;
+ unsigned int len;
bool one_shot_fail = false;
bool is_err_frame = false;
u16 transid = kvaser_usb_hydra_get_cmd_transid(cmd);
@@ -1139,21 +1140,22 @@ static void kvaser_usb_hydra_tx_acknowledge(const struct kvaser_usb *dev,
}

context = &priv->tx_contexts[transid % dev->max_tx_urbs];
- if (!one_shot_fail && !is_err_frame) {
- struct net_device_stats *stats = &priv->netdev->stats;
-
- stats->tx_packets++;
- stats->tx_bytes += can_fd_dlc2len(context->dlc);
- }

spin_lock_irqsave(&priv->tx_contexts_lock, irq_flags);

- can_get_echo_skb(priv->netdev, context->echo_index, NULL);
+ len = can_get_echo_skb(priv->netdev, context->echo_index, NULL);
context->echo_index = dev->max_tx_urbs;
--priv->active_tx_contexts;
netif_wake_queue(priv->netdev);

spin_unlock_irqrestore(&priv->tx_contexts_lock, irq_flags);
+
+ if (!one_shot_fail && !is_err_frame) {
+ struct net_device_stats *stats = &priv->netdev->stats;
+
+ stats->tx_packets++;
+ stats->tx_bytes += len;
+ }
}

static void kvaser_usb_hydra_rx_msg_std(const struct kvaser_usb *dev,
@@ -1372,8 +1374,8 @@ static void kvaser_usb_hydra_handle_cmd(const struct kvaser_usb *dev,

static void *
kvaser_usb_hydra_frame_to_cmd_ext(const struct kvaser_usb_net_priv *priv,
- const struct sk_buff *skb, int *frame_len,
- int *cmd_len, u16 transid)
+ const struct sk_buff *skb, int *cmd_len,
+ u16 transid)
{
struct kvaser_usb *dev = priv->dev;
struct kvaser_cmd_ext *cmd;
@@ -1385,8 +1387,6 @@ kvaser_usb_hydra_frame_to_cmd_ext(const struct kvaser_usb_net_priv *priv,
u32 kcan_id;
u32 kcan_header;

- *frame_len = nbr_of_bytes;
-
cmd = kcalloc(1, sizeof(struct kvaser_cmd_ext), GFP_ATOMIC);
if (!cmd)
return NULL;
@@ -1452,8 +1452,8 @@ kvaser_usb_hydra_frame_to_cmd_ext(const struct kvaser_usb_net_priv *priv,

static void *
kvaser_usb_hydra_frame_to_cmd_std(const struct kvaser_usb_net_priv *priv,
- const struct sk_buff *skb, int *frame_len,
- int *cmd_len, u16 transid)
+ const struct sk_buff *skb, int *cmd_len,
+ u16 transid)
{
struct kvaser_usb *dev = priv->dev;
struct kvaser_cmd *cmd;
@@ -1461,8 +1461,6 @@ kvaser_usb_hydra_frame_to_cmd_std(const struct kvaser_usb_net_priv *priv,
u32 flags;
u32 id;

- *frame_len = cf->len;
-
cmd = kcalloc(1, sizeof(struct kvaser_cmd), GFP_ATOMIC);
if (!cmd)
return NULL;
@@ -1496,7 +1494,7 @@ kvaser_usb_hydra_frame_to_cmd_std(const struct kvaser_usb_net_priv *priv,
cmd->tx_can.id = cpu_to_le32(id);
cmd->tx_can.flags = flags;

- memcpy(cmd->tx_can.data, cf->data, *frame_len);
+ memcpy(cmd->tx_can.data, cf->data, cf->len);

return cmd;
}
@@ -2004,17 +2002,17 @@ static void kvaser_usb_hydra_read_bulk_callback(struct kvaser_usb *dev,

static void *
kvaser_usb_hydra_frame_to_cmd(const struct kvaser_usb_net_priv *priv,
- const struct sk_buff *skb, int *frame_len,
- int *cmd_len, u16 transid)
+ const struct sk_buff *skb, int *cmd_len,
+ u16 transid)
{
void *buf;

if (priv->dev->card_data.capabilities & KVASER_USB_HYDRA_CAP_EXT_CMD)
- buf = kvaser_usb_hydra_frame_to_cmd_ext(priv, skb, frame_len,
- cmd_len, transid);
+ buf = kvaser_usb_hydra_frame_to_cmd_ext(priv, skb, cmd_len,
+ transid);
else
- buf = kvaser_usb_hydra_frame_to_cmd_std(priv, skb, frame_len,
- cmd_len, transid);
+ buf = kvaser_usb_hydra_frame_to_cmd_std(priv, skb, cmd_len,
+ transid);

return buf;
}
diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
index 14b445643554..47fa7f5a11c6 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
@@ -342,16 +342,14 @@ struct kvaser_usb_err_summary {

static void *
kvaser_usb_leaf_frame_to_cmd(const struct kvaser_usb_net_priv *priv,
- const struct sk_buff *skb, int *frame_len,
- int *cmd_len, u16 transid)
+ const struct sk_buff *skb, int *cmd_len,
+ u16 transid)
{
struct kvaser_usb *dev = priv->dev;
struct kvaser_cmd *cmd;
u8 *cmd_tx_can_flags = NULL; /* GCC */
struct can_frame *cf = (struct can_frame *)skb->data;

- *frame_len = cf->len;
-
cmd = kmalloc(sizeof(*cmd), GFP_ATOMIC);
if (cmd) {
cmd->u.tx_can.tid = transid & 0xff;
@@ -587,12 +585,11 @@ static void kvaser_usb_leaf_tx_acknowledge(const struct kvaser_usb *dev,
priv->can.state = CAN_STATE_ERROR_ACTIVE;
}

- stats->tx_packets++;
- stats->tx_bytes += context->dlc;
-
spin_lock_irqsave(&priv->tx_contexts_lock, flags);

- can_get_echo_skb(priv->netdev, context->echo_index, NULL);
+ stats->tx_packets++;
+ stats->tx_bytes += can_get_echo_skb(priv->netdev,
+ context->echo_index, NULL);
context->echo_index = dev->max_tx_urbs;
--priv->active_tx_contexts;
netif_wake_queue(priv->netdev);
diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c
index 4d20ea860ea8..77bddff86252 100644
--- a/drivers/net/can/usb/mcba_usb.c
+++ b/drivers/net/can/usb/mcba_usb.c
@@ -64,7 +64,6 @@
struct mcba_usb_ctx {
struct mcba_priv *priv;
u32 ndx;
- u8 dlc;
bool can;
};

@@ -184,13 +183,10 @@ static inline struct mcba_usb_ctx *mcba_usb_get_free_ctx(struct mcba_priv *priv,
ctx = &priv->tx_context[i];
ctx->ndx = i;

- if (cf) {
+ if (cf)
ctx->can = true;
- ctx->dlc = cf->len;
- } else {
+ else
ctx->can = false;
- ctx->dlc = 0;
- }

atomic_dec(&priv->free_ctx_cnt);
break;
@@ -236,10 +232,10 @@ static void mcba_usb_write_bulk_callback(struct urb *urb)
return;

netdev->stats.tx_packets++;
- netdev->stats.tx_bytes += ctx->dlc;
+ netdev->stats.tx_bytes += can_get_echo_skb(netdev, ctx->ndx,
+ NULL);

can_led_event(netdev, CAN_LED_EVENT_TX);
- can_get_echo_skb(netdev, ctx->ndx, NULL);
}

if (urb->status)
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
index 6107fef9f4a0..b850ff8fe4bd 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
@@ -291,6 +291,7 @@ static void peak_usb_write_bulk_callback(struct urb *urb)
struct peak_tx_urb_context *context = urb->context;
struct peak_usb_device *dev;
struct net_device *netdev;
+ int tx_bytes;

BUG_ON(!context);

@@ -305,10 +306,6 @@ static void peak_usb_write_bulk_callback(struct urb *urb)
/* check tx status */
switch (urb->status) {
case 0:
- /* transmission complete */
- netdev->stats.tx_packets++;
- netdev->stats.tx_bytes += context->data_len;
-
/* prevent tx timeout */
netif_trans_update(netdev);
break;
@@ -327,12 +324,17 @@ static void peak_usb_write_bulk_callback(struct urb *urb)
}

/* should always release echo skb and corresponding context */
- can_get_echo_skb(netdev, context->echo_index, NULL);
+ tx_bytes = can_get_echo_skb(netdev, context->echo_index, NULL);
context->echo_index = PCAN_USB_MAX_TX_URBS;

- /* do wakeup tx queue in case of success only */
- if (!urb->status)
+ if (!urb->status) {
+ /* transmission complete */
+ netdev->stats.tx_packets++;
+ netdev->stats.tx_bytes += tx_bytes;
+
+ /* do wakeup tx queue in case of success only */
netif_wake_queue(netdev);
+ }
}

/*
@@ -344,7 +346,6 @@ static netdev_tx_t peak_usb_ndo_start_xmit(struct sk_buff *skb,
struct peak_usb_device *dev = netdev_priv(netdev);
struct peak_tx_urb_context *context = NULL;
struct net_device_stats *stats = &netdev->stats;
- struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
struct urb *urb;
u8 *obuf;
int i, err;
@@ -378,9 +379,6 @@ static netdev_tx_t peak_usb_ndo_start_xmit(struct sk_buff *skb,

context->echo_index = i;

- /* Note: this works with CANFD frames too */
- context->data_len = cfd->len;
-
usb_anchor_urb(urb, &dev->tx_submitted);

can_put_echo_skb(skb, netdev, context->echo_index, 0);
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.h b/drivers/net/can/usb/peak_usb/pcan_usb_core.h
index daa19f57e742..f60af573a2e0 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_core.h
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.h
@@ -99,7 +99,6 @@ struct peak_time_ref {
struct peak_tx_urb_context {
struct peak_usb_device *dev;
u32 echo_index;
- u8 data_len;
struct urb *urb;
};

diff --git a/drivers/net/can/usb/ucan.c b/drivers/net/can/usb/ucan.c
index 388899019955..c7c41d1fd038 100644
--- a/drivers/net/can/usb/ucan.c
+++ b/drivers/net/can/usb/ucan.c
@@ -259,7 +259,6 @@ struct ucan_priv;
/* Context Information for transmission URBs */
struct ucan_urb_context {
struct ucan_priv *up;
- u8 dlc;
bool allocated;
};

@@ -637,7 +636,7 @@ static void ucan_tx_complete_msg(struct ucan_priv *up,
{
unsigned long flags;
u16 count, i;
- u8 echo_index, dlc;
+ u8 echo_index;
u16 len = le16_to_cpu(m->len);

struct ucan_urb_context *context;
@@ -661,7 +660,6 @@ static void ucan_tx_complete_msg(struct ucan_priv *up,

/* gather information from the context */
context = &up->context_array[echo_index];
- dlc = READ_ONCE(context->dlc);

/* Release context and restart queue if necessary.
* Also check if the context was allocated
@@ -674,8 +672,8 @@ static void ucan_tx_complete_msg(struct ucan_priv *up,
UCAN_TX_COMPLETE_SUCCESS) {
/* update statistics */
up->netdev->stats.tx_packets++;
- up->netdev->stats.tx_bytes += dlc;
- can_get_echo_skb(up->netdev, echo_index, NULL);
+ up->netdev->stats.tx_bytes +=
+ can_get_echo_skb(up->netdev, echo_index, NULL);
} else {
up->netdev->stats.tx_dropped++;
can_free_echo_skb(up->netdev, echo_index, NULL);
@@ -1089,8 +1087,6 @@ static struct urb *ucan_prepare_tx_urb(struct ucan_priv *up,
}
m->len = cpu_to_le16(mlen);

- context->dlc = cf->len;
-
m->subtype = echo_index;

/* build the urb */
diff --git a/drivers/net/can/usb/usb_8dev.c b/drivers/net/can/usb/usb_8dev.c
index 23cdc92fb007..3127a81f6c8c 100644
--- a/drivers/net/can/usb/usb_8dev.c
+++ b/drivers/net/can/usb/usb_8dev.c
@@ -114,7 +114,6 @@ struct usb_8dev_tx_urb_context {
struct usb_8dev_priv *priv;

u32 echo_index;
- u8 dlc;
};

/* Structure to hold all of our device specific stuff */
@@ -583,9 +582,7 @@ static void usb_8dev_write_bulk_callback(struct urb *urb)
urb->status);

netdev->stats.tx_packets++;
- netdev->stats.tx_bytes += context->dlc;
-
- can_get_echo_skb(netdev, context->echo_index, NULL);
+ netdev->stats.tx_bytes += can_get_echo_skb(netdev, context->echo_index, NULL);

can_led_event(netdev, CAN_LED_EVENT_TX);

@@ -656,7 +653,6 @@ static netdev_tx_t usb_8dev_start_xmit(struct sk_buff *skb,

context->priv = priv;
context->echo_index = i;
- context->dlc = cf->len;

usb_fill_bulk_urb(urb, priv->udev,
usb_sndbulkpipe(priv->udev, USB_8DEV_ENDP_DATA_TX),
diff --git a/drivers/net/can/vcan.c b/drivers/net/can/vcan.c
index 067705e2850b..c42f18845b02 100644
--- a/drivers/net/can/vcan.c
+++ b/drivers/net/can/vcan.c
@@ -87,13 +87,14 @@ static netdev_tx_t vcan_tx(struct sk_buff *skb, struct net_device *dev)
{
struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
struct net_device_stats *stats = &dev->stats;
- int loop;
+ int loop, len;

if (can_dropped_invalid_skb(dev, skb))
return NETDEV_TX_OK;

+ len = cfd->can_id & CAN_RTR_FLAG ? 0 : cfd->len;
stats->tx_packets++;
- stats->tx_bytes += cfd->len;
+ stats->tx_bytes += len;

/* set flag whether this packet has to be looped back */
loop = skb->pkt_type == PACKET_LOOPBACK;
@@ -105,7 +106,7 @@ static netdev_tx_t vcan_tx(struct sk_buff *skb, struct net_device *dev)
* CAN core already did the echo for us
*/
stats->rx_packets++;
- stats->rx_bytes += cfd->len;
+ stats->rx_bytes += len;
}
consume_skb(skb);
return NETDEV_TX_OK;
diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c
index 8861a7d875e7..47ccc15a3486 100644
--- a/drivers/net/can/vxcan.c
+++ b/drivers/net/can/vxcan.c
@@ -62,7 +62,7 @@ static netdev_tx_t vxcan_xmit(struct sk_buff *skb, struct net_device *dev)
skb->dev = peer;
skb->ip_summed = CHECKSUM_UNNECESSARY;

- len = cfd->len;
+ len = cfd->can_id & CAN_RTR_FLAG ? 0 : cfd->len;
if (netif_rx_ni(skb) == NET_RX_SUCCESS) {
srcstats->tx_packets++;
srcstats->tx_bytes += len;
diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
index d311bc369a39..fdb22b00674a 100644
--- a/include/linux/can/skb.h
+++ b/include/linux/can/skb.h
@@ -21,8 +21,9 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
unsigned int idx, unsigned int frame_len);
struct sk_buff *__can_get_echo_skb(struct net_device *dev, unsigned int idx,
u8 *len_ptr, unsigned int *frame_len_ptr);
-unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx,
- unsigned int *frame_len_ptr);
+unsigned int __must_check can_get_echo_skb(struct net_device *dev,
+ unsigned int idx,
+ unsigned int *frame_len_ptr);
void can_free_echo_skb(struct net_device *dev, unsigned int idx,
unsigned int *frame_len_ptr);
struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf);
--
2.32.0

2021-12-08 09:16:32

by Marc Kleine-Budde

[permalink] [raw]

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

On 07.12.2021 21:15:26, Vincent Mailhol wrote:
> Important: this patch series depends on below patch:
> https://lore.kernel.org/linux-can/[email protected]/T/#u

This series will go into net-next/master, I'll wait until net/master
(which includes the above mentioned patch) is merged into
net-next/master.

Marc

--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

Attachments:

(No filename) (610.00 B)
signature.asc (488.00 B)
Download all attachments

2021-12-09 23:21:56

by Stefan Mätje

[permalink] [raw]

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

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 <[email protected]>
Tested-by: Stefan Mätje <[email protected]>

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/[email protected]/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

2021-12-10 07:36:01

by Marc Kleine-Budde

[permalink] [raw]

Subject: Re: [PATCH v5 3/5] can: do not copy the payload of RTR frames

On 07.12.2021 21:15:29, 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,
^^^^^^^
I've changed this to "no payload" to make it more unambiguous.

> those RTR frames still use the DLC to indicate the length of the
> requested frame.
>
> For this reason, it is incorrect to copy the payload of RTR frames
> (the payload buffer would only contain garbage data). This patch
> encapsulates the payload copy in a check toward the RTR flag.
>
> CC: Yasushi SHOJI <[email protected]>
> Signed-off-by: Vincent Mailhol <[email protected]>

Marc

--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

Attachments:

(No filename) (960.00 B)
signature.asc (488.00 B)
Download all attachments

2021-12-10 08:02:15

by Marc Kleine-Budde

[permalink] [raw]

Subject: Re: [PATCH v5 3/5] can: do not copy the payload of RTR frames

On 10.12.2021 08:35:45, Marc Kleine-Budde wrote:
> On 07.12.2021 21:15:29, 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,
> ^^^^^^^
> I've changed this to "no payload" to make it more unambiguous.

Same for the other patches.

Marc

--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

Attachments:

(No filename) (649.00 B)
signature.asc (488.00 B)
Download all attachments

2021-12-10 08:57:49

by Yasushi SHOJI

[permalink] [raw]

Subject: Re: [PATCH v5 3/5] can: do not copy the payload of RTR frames

Hi Vincent,

On Tue, Dec 7, 2021 at 9:16 PM Vincent Mailhol
<[email protected]> 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.
>
> For this reason, it is incorrect to copy the payload of RTR frames
> (the payload buffer would only contain garbage data). This patch
> encapsulates the payload copy in a check toward the RTR flag.
>
> CC: Yasushi SHOJI <[email protected]>
> Signed-off-by: Vincent Mailhol <[email protected]>

It works and LGTM.

Tested-by: Yasushi SHOJI <[email protected]>
--
yashi