* [PATCH v1 0/2] fix statistics for CAN RTR and Error frames
@ 2021-11-23 11:53 Vincent Mailhol
2021-11-23 11:53 ` [PATCH v1 1/2] can: do not increase rx statistics when receiving CAN error frames Vincent Mailhol
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Vincent Mailhol @ 2021-11-23 11:53 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can
Cc: netdev, linux-kernel, linux-arm-kernel, linux-sunxi, Vincent Mailhol
There are two common errors which are made when reporting the CAN RX
statistics:
1. Incrementing the "normal" RX stats when receiving an Error
frame. Error frames is an abstraction of Socket CAN and does not
exist on the wire.
2. 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.
This patch series fix those two issues for all CAN drivers.
Vincent Mailhol (2):
can: do not increase rx statistics when receiving CAN error frames
can: do not increase rx_bytes statistics for RTR frames
drivers/net/can/at91_can.c | 9 ++-------
drivers/net/can/c_can/c_can_main.c | 8 ++------
drivers/net/can/cc770/cc770.c | 6 ++----
drivers/net/can/dev/dev.c | 4 ----
drivers/net/can/dev/rx-offload.c | 7 +++++--
drivers/net/can/grcan.c | 3 ++-
drivers/net/can/ifi_canfd/ifi_canfd.c | 8 ++------
drivers/net/can/janz-ican3.c | 3 ++-
drivers/net/can/kvaser_pciefd.c | 8 ++------
drivers/net/can/m_can/m_can.c | 10 ++--------
drivers/net/can/mscan/mscan.c | 10 ++++++----
drivers/net/can/pch_can.c | 6 ++----
drivers/net/can/peak_canfd/peak_canfd.c | 7 ++-----
drivers/net/can/rcar/rcar_can.c | 9 +++------
drivers/net/can/rcar/rcar_canfd.c | 7 ++-----
drivers/net/can/sja1000/sja1000.c | 5 ++---
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 | 10 ++++------
drivers/net/can/usb/ems_usb.c | 5 ++---
drivers/net/can/usb/esd_usb2.c | 5 ++---
drivers/net/can/usb/etas_es58x/es58x_core.c | 7 -------
.../net/can/usb/kvaser_usb/kvaser_usb_core.c | 2 --
.../net/can/usb/kvaser_usb/kvaser_usb_hydra.c | 14 ++++----------
.../net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 7 ++-----
drivers/net/can/usb/mcba_usb.c | 3 ++-
drivers/net/can/usb/peak_usb/pcan_usb.c | 5 ++---
drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 11 ++++-------
drivers/net/can/usb/peak_usb/pcan_usb_pro.c | 11 +++++------
drivers/net/can/usb/ucan.c | 7 +++++--
drivers/net/can/usb/usb_8dev.c | 10 ++++------
drivers/net/can/xilinx_can.c | 17 ++++++-----------
33 files changed, 86 insertions(+), 147 deletions(-)
--
2.32.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v1 1/2] can: do not increase rx statistics when receiving CAN error frames
2021-11-23 11:53 [PATCH v1 0/2] fix statistics for CAN RTR and Error frames Vincent Mailhol
@ 2021-11-23 11:53 ` Vincent Mailhol
2021-11-23 21:01 ` Oliver Hartkopp
2021-11-23 11:53 ` [PATCH v1 2/2] can: do not increase rx_bytes statistics for RTR frames Vincent Mailhol
2021-11-23 21:10 ` [PATCH v1 0/2] fix statistics for CAN RTR and Error frames Oliver Hartkopp
2 siblings, 1 reply; 10+ messages in thread
From: Vincent Mailhol @ 2021-11-23 11:53 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can
Cc: netdev, linux-kernel, linux-arm-kernel, linux-sunxi,
Vincent Mailhol, Jimmy Assarsson, Nicolas Ferre,
Alexandre Belloni, Ludovic Desroches, Chandrasekar Ramakrishnan,
Maxime Ripard, Chen-Yu Tsai, Jernej Skrabec,
Appana Durga Kedareswara rao, Naga Sureshkumar Relli,
Michal Simek, Stephane Grosjean
CAN error skb is an interface specific to socket CAN. The CAN error
skb does not correspond to any actual CAN frame 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: Jimmy Assarsson <extja@kvaser.com>
CC: Marc Kleine-Budde <mkl@pengutronix.de>
CC: Nicolas Ferre <nicolas.ferre@microchip.com>
CC: Alexandre Belloni <alexandre.belloni@bootlin.com>
CC: Ludovic Desroches <ludovic.desroches@microchip.com>
CC: Chandrasekar Ramakrishnan <rcsekar@samsung.com>
CC: Maxime Ripard <mripard@kernel.org>
CC: Chen-Yu Tsai <wens@csie.org>
CC: Jernej Skrabec <jernej.skrabec@gmail.com>
CC: Appana Durga Kedareswara rao <appana.durga.rao@xilinx.com>
CC: Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com>
CC: Michal Simek <michal.simek@xilinx.com>
CC: Stephane Grosjean <s.grosjean@peak-system.com>
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
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..bb47e9a49240 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_MASK)) {
+ 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v1 2/2] can: do not increase rx_bytes statistics for RTR frames
2021-11-23 11:53 [PATCH v1 0/2] fix statistics for CAN RTR and Error frames Vincent Mailhol
2021-11-23 11:53 ` [PATCH v1 1/2] can: do not increase rx statistics when receiving CAN error frames Vincent Mailhol
@ 2021-11-23 11:53 ` Vincent Mailhol
2021-11-23 18:10 ` Vincent MAILHOL
2021-11-24 17:59 ` Stefan Mätje
2021-11-23 21:10 ` [PATCH v1 0/2] fix statistics for CAN RTR and Error frames Oliver Hartkopp
2 siblings, 2 replies; 10+ messages in thread
From: Vincent Mailhol @ 2021-11-23 11:53 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can
Cc: netdev, linux-kernel, linux-arm-kernel, linux-sunxi,
Vincent Mailhol, Jimmy Assarsson, Nicolas Ferre,
Alexandre Belloni, Ludovic Desroches, Chandrasekar Ramakrishnan,
Maxime Ripard, Chen-Yu Tsai, Jernej Skrabec, Yasushi SHOJI,
Appana Durga Kedareswara rao, Naga Sureshkumar Relli,
Michal Simek, Stephane Grosjean
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@kvaser.com>
CC: Marc Kleine-Budde <mkl@pengutronix.de>
CC: Nicolas Ferre <nicolas.ferre@microchip.com>
CC: Alexandre Belloni <alexandre.belloni@bootlin.com>
CC: Ludovic Desroches <ludovic.desroches@microchip.com>
CC: Chandrasekar Ramakrishnan <rcsekar@samsung.com>
CC: Maxime Ripard <mripard@kernel.org>
CC: Chen-Yu Tsai <wens@csie.org>
CC: Jernej Skrabec <jernej.skrabec@gmail.com>
CC: Yasushi SHOJI <yashi@spacecubics.com>
CC: Appana Durga Kedareswara rao <appana.durga.rao@xilinx.com>
CC: Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com>
CC: Michal Simek <michal.simek@xilinx.com>
CC: Stephane Grosjean <s.grosjean@peak-system.com>
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
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/at91_can.c b/drivers/net/can/at91_can.c
index 3cd872cf9be6..89b22c3f9a01 100644
--- a/drivers/net/can/at91_can.c
+++ b/drivers/net/can/at91_can.c
@@ -617,7 +617,8 @@ static void at91_read_msg(struct net_device *dev, unsigned int mb)
at91_read_mb(dev, mb, cf);
stats->rx_packets++;
- stats->rx_bytes += cf->len;
+ if (!(cf->can_id & CAN_RTR_FLAG))
+ stats->rx_bytes += cf->len;
netif_receive_skb(skb);
can_led_event(dev, CAN_LED_EVENT_RX);
diff --git a/drivers/net/can/c_can/c_can_main.c b/drivers/net/can/c_can/c_can_main.c
index 670754a12984..f2eb9ebc875f 100644
--- a/drivers/net/can/c_can/c_can_main.c
+++ b/drivers/net/can/c_can/c_can_main.c
@@ -406,7 +406,8 @@ static int c_can_read_msg_object(struct net_device *dev, int iface, u32 ctrl)
}
stats->rx_packets++;
- stats->rx_bytes += frame->len;
+ if (!(frame->can_id & CAN_RTR_FLAG))
+ stats->rx_bytes += frame->len;
netif_receive_skb(skb);
return 0;
diff --git a/drivers/net/can/cc770/cc770.c b/drivers/net/can/cc770/cc770.c
index a5fd8ccedec2..7c703bcbb433 100644
--- a/drivers/net/can/cc770/cc770.c
+++ b/drivers/net/can/cc770/cc770.c
@@ -492,7 +492,8 @@ static void cc770_rx(struct net_device *dev, unsigned int mo, u8 ctrl1)
}
stats->rx_packets++;
- stats->rx_bytes += cf->len;
+ if (!(id & CAN_RTR_FLAG))
+ stats->rx_bytes += cf->len;
netif_rx(skb);
}
diff --git a/drivers/net/can/dev/rx-offload.c b/drivers/net/can/dev/rx-offload.c
index bb47e9a49240..b10b73e5ed87 100644
--- a/drivers/net/can/dev/rx-offload.c
+++ b/drivers/net/can/dev/rx-offload.c
@@ -56,7 +56,8 @@ static int can_rx_offload_napi_poll(struct napi_struct *napi, int quota)
work_done++;
if (!(cf->can_id & CAN_ERR_MASK)) {
stats->rx_packets++;
- stats->rx_bytes += cf->len;
+ if (!(cf->can_id & CAN_RTR_FLAG))
+ stats->rx_bytes += cf->len;
}
netif_receive_skb(skb);
}
diff --git a/drivers/net/can/grcan.c b/drivers/net/can/grcan.c
index 78e27940b2af..2f8a08321b41 100644
--- a/drivers/net/can/grcan.c
+++ b/drivers/net/can/grcan.c
@@ -1215,7 +1215,8 @@ static int grcan_receive(struct net_device *dev, int budget)
/* Update statistics and read pointer */
stats->rx_packets++;
- stats->rx_bytes += cf->len;
+ if (!(cf->can_id & CAN_RTR_FLAG))
+ stats->rx_bytes += cf->len;
netif_receive_skb(skb);
rd = grcan_ring_add(rd, GRCAN_MSG_SIZE, dma->rx.size);
diff --git a/drivers/net/can/ifi_canfd/ifi_canfd.c b/drivers/net/can/ifi_canfd/ifi_canfd.c
index e8318e984bf2..1f90c829321e 100644
--- a/drivers/net/can/ifi_canfd/ifi_canfd.c
+++ b/drivers/net/can/ifi_canfd/ifi_canfd.c
@@ -316,7 +316,8 @@ static void ifi_canfd_read_fifo(struct net_device *ndev)
writel(rx_irq_mask, priv->base + IFI_CANFD_INTERRUPT);
stats->rx_packets++;
- stats->rx_bytes += cf->len;
+ if (!(cf->can_id & CAN_RTR_FLAG))
+ stats->rx_bytes += cf->len;
netif_receive_skb(skb);
}
diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
index 32006dbf5abd..5c589aa9dff8 100644
--- a/drivers/net/can/janz-ican3.c
+++ b/drivers/net/can/janz-ican3.c
@@ -1421,7 +1421,8 @@ static int ican3_recv_skb(struct ican3_dev *mod)
/* update statistics, receive the skb */
stats->rx_packets++;
- stats->rx_bytes += cf->len;
+ if (!(cf->can_id & CAN_RTR_FLAG))
+ stats->rx_bytes += cf->len;
netif_receive_skb(skb);
err_noalloc:
diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c
index 483fbd9e6952..88a570476500 100644
--- a/drivers/net/can/kvaser_pciefd.c
+++ b/drivers/net/can/kvaser_pciefd.c
@@ -1193,7 +1193,8 @@ static int kvaser_pciefd_handle_data_packet(struct kvaser_pciefd *pcie,
ns_to_ktime(div_u64(p->timestamp * 1000,
pcie->freq_to_ticks_div));
- stats->rx_bytes += cf->len;
+ if (!(cf->can_id & CAN_RTR_FLAG))
+ stats->rx_bytes += cf->len;
stats->rx_packets++;
return netif_rx(skb);
diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index c33035e706bc..eedaf66bff31 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -524,7 +524,8 @@ static int m_can_read_fifo(struct net_device *dev, u32 rxfs)
m_can_write(cdev, M_CAN_RXF0A, fgi);
stats->rx_packets++;
- stats->rx_bytes += cf->len;
+ if (!(cf->can_id & CAN_RTR_FLAG))
+ stats->rx_bytes += cf->len;
timestamp = FIELD_GET(RX_BUF_RXTS_MASK, fifo_header.dlc);
diff --git a/drivers/net/can/mscan/mscan.c b/drivers/net/can/mscan/mscan.c
index 9e1cce0260da..59b8284d00e5 100644
--- a/drivers/net/can/mscan/mscan.c
+++ b/drivers/net/can/mscan/mscan.c
@@ -404,7 +404,8 @@ static int mscan_rx_poll(struct napi_struct *napi, int quota)
if (canrflg & MSCAN_RXF) {
mscan_get_rx_frame(dev, frame);
stats->rx_packets++;
- stats->rx_bytes += frame->len;
+ if (!(frame->can_id & CAN_RTR_FLAG))
+ stats->rx_bytes += frame->len;
} else if (canrflg & MSCAN_ERR_IF) {
mscan_get_err_frame(dev, frame, canrflg);
}
diff --git a/drivers/net/can/pch_can.c b/drivers/net/can/pch_can.c
index 6b45840db1f9..8fbe3560216b 100644
--- a/drivers/net/can/pch_can.c
+++ b/drivers/net/can/pch_can.c
@@ -692,7 +692,8 @@ static int pch_can_rx_normal(struct net_device *ndev, u32 obj_num, int quota)
rcv_pkts++;
stats->rx_packets++;
quota--;
- stats->rx_bytes += cf->len;
+ if (!(cf->can_id & CAN_RTR_FLAG))
+ stats->rx_bytes += cf->len;
netif_receive_skb(skb);
pch_fifo_thresh(priv, obj_num);
diff --git a/drivers/net/can/peak_canfd/peak_canfd.c b/drivers/net/can/peak_canfd/peak_canfd.c
index d5b8bc6d2980..baae39fa14fc 100644
--- a/drivers/net/can/peak_canfd/peak_canfd.c
+++ b/drivers/net/can/peak_canfd/peak_canfd.c
@@ -315,7 +315,8 @@ static int pucan_handle_can_rx(struct peak_canfd_priv *priv,
else
memcpy(cf->data, msg->d, cf->len);
- stats->rx_bytes += cf->len;
+ if (!(cf->can_id & CAN_RTR_FLAG))
+ stats->rx_bytes += cf->len;
stats->rx_packets++;
pucan_netif_rx(skb, msg->ts_low, msg->ts_high);
diff --git a/drivers/net/can/rcar/rcar_can.c b/drivers/net/can/rcar/rcar_can.c
index f408ed9a6ccd..237a97cd2f78 100644
--- a/drivers/net/can/rcar/rcar_can.c
+++ b/drivers/net/can/rcar/rcar_can.c
@@ -666,7 +666,8 @@ static void rcar_can_rx_pkt(struct rcar_can_priv *priv)
can_led_event(priv->ndev, CAN_LED_EVENT_RX);
- stats->rx_bytes += cf->len;
+ if (!(cf->can_id & CAN_RTR_FLAG))
+ stats->rx_bytes += cf->len;
stats->rx_packets++;
netif_receive_skb(skb);
}
diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index db9d62874e15..b1eded2f2c5d 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -1550,7 +1550,8 @@ static void rcar_canfd_rx_pkt(struct rcar_canfd_channel *priv)
can_led_event(priv->ndev, CAN_LED_EVENT_RX);
- stats->rx_bytes += cf->len;
+ if (!(cf->can_id & CAN_RTR_FLAG))
+ stats->rx_bytes += cf->len;
stats->rx_packets++;
netif_receive_skb(skb);
}
diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
index a65546ca9461..d2e444968087 100644
--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -380,7 +380,8 @@ static void sja1000_rx(struct net_device *dev)
sja1000_write_cmdreg(priv, CMD_RRB);
stats->rx_packets++;
- stats->rx_bytes += cf->len;
+ if (!(id & CAN_RTR_FLAG))
+ stats->rx_bytes += cf->len;
netif_rx(skb);
can_led_event(dev, CAN_LED_EVENT_RX);
diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index 9a4ebda30510..c53ad59763e0 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -218,7 +218,8 @@ static void slc_bump(struct slcan *sl)
skb_put_data(skb, &cf, sizeof(struct can_frame));
sl->dev->stats.rx_packets++;
- sl->dev->stats.rx_bytes += cf.len;
+ if (!(cf.can_id & CAN_RTR_FLAG))
+ sl->dev->stats.rx_bytes += cf.len;
netif_rx_ni(skb);
}
diff --git a/drivers/net/can/spi/hi311x.c b/drivers/net/can/spi/hi311x.c
index 89d9c986a229..1e2929a9521b 100644
--- a/drivers/net/can/spi/hi311x.c
+++ b/drivers/net/can/spi/hi311x.c
@@ -350,7 +350,8 @@ static void hi3110_hw_rx(struct spi_device *spi)
frame->len);
priv->net->stats.rx_packets++;
- priv->net->stats.rx_bytes += frame->len;
+ if (!(frame->can_id & CAN_RTR_FLAG))
+ priv->net->stats.rx_bytes += frame->len;
can_led_event(priv->net, CAN_LED_EVENT_RX);
diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c
index 0579ab74f728..1f47c588b69f 100644
--- a/drivers/net/can/spi/mcp251x.c
+++ b/drivers/net/can/spi/mcp251x.c
@@ -736,7 +736,8 @@ static void mcp251x_hw_rx(struct spi_device *spi, int buf_idx)
memcpy(frame->data, buf + RXBDAT_OFF, frame->len);
priv->net->stats.rx_packets++;
- priv->net->stats.rx_bytes += frame->len;
+ if (!(frame->can_id & CAN_RTR_FLAG))
+ priv->net->stats.rx_bytes += frame->len;
can_led_event(priv->net, CAN_LED_EVENT_RX);
diff --git a/drivers/net/can/sun4i_can.c b/drivers/net/can/sun4i_can.c
index 599174098883..cd3278e5d4f6 100644
--- a/drivers/net/can/sun4i_can.c
+++ b/drivers/net/can/sun4i_can.c
@@ -501,7 +501,8 @@ static void sun4i_can_rx(struct net_device *dev)
sun4i_can_write_cmdreg(priv, SUN4I_CMD_RELEASE_RBUF);
stats->rx_packets++;
- stats->rx_bytes += cf->len;
+ if (!(id & CAN_RTR_FLAG))
+ stats->rx_bytes += cf->len;
netif_rx(skb);
can_led_event(dev, CAN_LED_EVENT_RX);
diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
index 7cf65936d02e..f4288f98c006 100644
--- a/drivers/net/can/usb/ems_usb.c
+++ b/drivers/net/can/usb/ems_usb.c
@@ -323,7 +323,8 @@ static void ems_usb_rx_can_msg(struct ems_usb *dev, struct ems_cpc_msg *msg)
}
stats->rx_packets++;
- stats->rx_bytes += cf->len;
+ if (!(cf->can_id & CAN_RTR_FLAG))
+ 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 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);
}
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 3398da323126..adab5c2a8b99 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
@@ -1206,7 +1206,8 @@ static void kvaser_usb_hydra_rx_msg_std(const struct kvaser_usb *dev,
memcpy(cf->data, cmd->rx_can.data, cf->len);
stats->rx_packets++;
- stats->rx_bytes += cf->len;
+ if (!(cf->can_id & CAN_RTR_FLAG))
+ stats->rx_bytes += cf->len;
netif_rx(skb);
}
@@ -1284,7 +1285,8 @@ static void kvaser_usb_hydra_rx_msg_ext(const struct kvaser_usb *dev,
memcpy(cf->data, cmd->rx_can.kcan_payload, cf->len);
stats->rx_packets++;
- stats->rx_bytes += cf->len;
+ if (!(cf->can_id & CAN_RTR_FLAG))
+ 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 4aebaab9ea9c..14b445643554 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
@@ -1001,7 +1001,8 @@ static void kvaser_usb_leaf_rx_can_msg(const struct kvaser_usb *dev,
}
stats->rx_packets++;
- stats->rx_bytes += cf->len;
+ if (!(cf->can_id & CAN_RTR_FLAG))
+ stats->rx_bytes += cf->len;
netif_rx(skb);
}
diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c
index a1a154c08b7f..372f0a5ebb9f 100644
--- a/drivers/net/can/usb/mcba_usb.c
+++ b/drivers/net/can/usb/mcba_usb.c
@@ -458,7 +458,8 @@ static void mcba_usb_process_can(struct mcba_priv *priv,
memcpy(cf->data, msg->data, cf->len);
stats->rx_packets++;
- stats->rx_bytes += cf->len;
+ if (!(cf->can_id & CAN_RTR_FLAG))
+ stats->rx_bytes += cf->len;
can_led_event(priv->netdev, CAN_LED_EVENT_RX);
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 21b06a738595..d1a4f847f310 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
@@ -684,7 +684,8 @@ static int pcan_usb_decode_data(struct pcan_usb_msg_context *mc, u8 status_len)
/* update statistics */
mc->netdev->stats.rx_packets++;
- mc->netdev->stats.rx_bytes += cf->len;
+ if (!(cf->can_id & CAN_RTR_FLAG))
+ mc->netdev->stats.rx_bytes += cf->len;
/* push the skb */
netif_rx(skb);
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 185f5a98d217..65487ec33566 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
@@ -507,13 +507,13 @@ static int pcan_usb_fd_decode_canmsg(struct pcan_usb_fd_if *usb_if,
if (rx_msg_flags & PUCAN_MSG_EXT_ID)
cfd->can_id |= CAN_EFF_FLAG;
- if (rx_msg_flags & PUCAN_MSG_RTR)
+ if (rx_msg_flags & PUCAN_MSG_RTR) {
cfd->can_id |= CAN_RTR_FLAG;
- else
+ } else {
memcpy(cfd->data, rm->d, cfd->len);
-
+ netdev->stats.rx_bytes += cfd->len;
+ }
netdev->stats.rx_packets++;
- netdev->stats.rx_bytes += cfd->len;
peak_usb_netif_rx_64(skb, le32_to_cpu(rm->ts_low),
le32_to_cpu(rm->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 f6d19879bf40..69fdddcbba98 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
@@ -536,17 +536,18 @@ static int pcan_usb_pro_handle_canmsg(struct pcan_usb_pro_interface *usb_if,
if (rx->flags & PCAN_USBPRO_EXT)
can_frame->can_id |= CAN_EFF_FLAG;
- if (rx->flags & PCAN_USBPRO_RTR)
+ if (rx->flags & PCAN_USBPRO_RTR) {
can_frame->can_id |= CAN_RTR_FLAG;
- else
+ } else {
memcpy(can_frame->data, rx->data, can_frame->len);
+ netdev->stats.rx_bytes += can_frame->len;
+ }
+ netdev->stats.rx_packets++;
hwts = skb_hwtstamps(skb);
peak_usb_get_ts_time(&usb_if->time_ref, le32_to_cpu(rx->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 d582c39fc8d0..388899019955 100644
--- a/drivers/net/can/usb/ucan.c
+++ b/drivers/net/can/usb/ucan.c
@@ -623,7 +623,8 @@ static void ucan_rx_can_msg(struct ucan_priv *up, struct ucan_message_in *m)
/* don't count error frames as real packets */
if (!(cf->can_id & CAN_ERR_FLAG)) {
stats->rx_packets++;
- stats->rx_bytes += cf->len;
+ if (!(cf->can_id & CAN_RTR_FLAG))
+ stats->rx_bytes += cf->len;
}
/* pass it to Linux */
diff --git a/drivers/net/can/usb/usb_8dev.c b/drivers/net/can/usb/usb_8dev.c
index 040324362b26..14ae8ed85f80 100644
--- a/drivers/net/can/usb/usb_8dev.c
+++ b/drivers/net/can/usb/usb_8dev.c
@@ -474,13 +474,13 @@ static void usb_8dev_rx_can_msg(struct usb_8dev_priv *priv,
if (msg->flags & USB_8DEV_EXTID)
cf->can_id |= CAN_EFF_FLAG;
- if (msg->flags & USB_8DEV_RTR)
+ if (msg->flags & USB_8DEV_RTR) {
cf->can_id |= CAN_RTR_FLAG;
- else
+ } else {
memcpy(cf->data, msg->data, cf->len);
-
+ stats->rx_bytes += cf->len;
+ }
stats->rx_packets++;
- stats->rx_bytes += cf->len;
netif_rx(skb);
can_led_event(priv->netdev, CAN_LED_EVENT_RX);
diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 275e240ab293..7d7239ec9920 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -787,9 +787,9 @@ static int xcan_rx(struct net_device *ndev, int frame_base)
*(__be32 *)(cf->data) = cpu_to_be32(data[0]);
if (cf->len > 4)
*(__be32 *)(cf->data + 4) = cpu_to_be32(data[1]);
- }
- stats->rx_bytes += cf->len;
+ stats->rx_bytes += cf->len;
+ }
stats->rx_packets++;
netif_receive_skb(skb);
@@ -871,7 +871,9 @@ static int xcanfd_rx(struct net_device *ndev, int frame_base)
*(__be32 *)(cf->data + i) = cpu_to_be32(data[0]);
}
}
- stats->rx_bytes += cf->len;
+
+ if (!(cf->can_id & CAN_RTR_FLAG))
+ stats->rx_bytes += cf->len;
stats->rx_packets++;
netif_receive_skb(skb);
--
2.32.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v1 2/2] can: do not increase rx_bytes statistics for RTR frames
2021-11-23 11:53 ` [PATCH v1 2/2] can: do not increase rx_bytes statistics for RTR frames Vincent Mailhol
@ 2021-11-23 18:10 ` Vincent MAILHOL
2021-11-24 17:59 ` Stefan Mätje
1 sibling, 0 replies; 10+ messages in thread
From: Vincent MAILHOL @ 2021-11-23 18:10 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can
Cc: netdev, linux-kernel, linux-arm-kernel, linux-sunxi,
Jimmy Assarsson, Nicolas Ferre, Alexandre Belloni,
Ludovic Desroches, Chandrasekar Ramakrishnan, Maxime Ripard,
Chen-Yu Tsai, Jernej Skrabec, Yasushi SHOJI,
Appana Durga Kedareswara rao, Naga Sureshkumar Relli,
Michal Simek, Stephane Grosjean
On Tue. 23 Nov. 2021 at 20:53, Vincent Mailhol
<mailhol.vincent@wanadoo.fr> 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 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.
Actually, I just realized that we also need to fix the tx path.
Since [1], can_get_echo_skb() returns the correct length (even
for RTR frames). So as long as the drivers use this function,
everything should be fine. But the fact is that the majority do
not (probably for historical reasons). Long story short, I will
send a v2 in which there will be an additional third patch to
address the tx_bytes statistics of the RTR frames in the tx
path.
[1] commit 59d24425c93d ("can: dev: replace can_priv::ctrlmode_static
by can_get_static_ctrlmode()")
https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/commit/?h=testing&id=ed3320cec279407a86bc4c72edc4a39eb49165ec
Yours sincerely,
Vincent Mailhol
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 1/2] can: do not increase rx statistics when receiving CAN error frames
2021-11-23 11:53 ` [PATCH v1 1/2] can: do not increase rx statistics when receiving CAN error frames Vincent Mailhol
@ 2021-11-23 21:01 ` Oliver Hartkopp
2021-11-23 23:21 ` Vincent MAILHOL
0 siblings, 1 reply; 10+ messages in thread
From: Oliver Hartkopp @ 2021-11-23 21:01 UTC (permalink / raw)
To: Vincent Mailhol, Marc Kleine-Budde, linux-can
Cc: netdev, linux-kernel, linux-arm-kernel, linux-sunxi,
Jimmy Assarsson, Nicolas Ferre, Alexandre Belloni,
Ludovic Desroches, Chandrasekar Ramakrishnan, Maxime Ripard,
Chen-Yu Tsai, Jernej Skrabec, Appana Durga Kedareswara rao,
Naga Sureshkumar Relli, Michal Simek, Stephane Grosjean
On 23.11.21 12:53, Vincent Mailhol wrote:
> CAN error skb is an interface specific to socket CAN. The CAN error
> skb does not correspond to any actual CAN frame 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.
>
(..)
> diff --git a/drivers/net/can/dev/rx-offload.c b/drivers/net/can/dev/rx-offload.c
> index 37b0cc65237b..bb47e9a49240 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_MASK)) {
This looks wrong.
Did you think of CAN_ERR_FLAG ??
> + stats->rx_packets++;
> + stats->rx_bytes += cf->len;
> + }
> netif_receive_skb(skb);
(..)
> 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)) {
Ah, here we are :-)
> + stats->rx_packets++;
> + stats->rx_bytes += cf->len;
> + }
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 0/2] fix statistics for CAN RTR and Error frames
2021-11-23 11:53 [PATCH v1 0/2] fix statistics for CAN RTR and Error frames Vincent Mailhol
2021-11-23 11:53 ` [PATCH v1 1/2] can: do not increase rx statistics when receiving CAN error frames Vincent Mailhol
2021-11-23 11:53 ` [PATCH v1 2/2] can: do not increase rx_bytes statistics for RTR frames Vincent Mailhol
@ 2021-11-23 21:10 ` Oliver Hartkopp
2021-11-23 23:35 ` Vincent MAILHOL
2 siblings, 1 reply; 10+ messages in thread
From: Oliver Hartkopp @ 2021-11-23 21:10 UTC (permalink / raw)
To: Vincent Mailhol, Marc Kleine-Budde, linux-can
Cc: netdev, linux-kernel, linux-arm-kernel, linux-sunxi
On 23.11.21 12:53, Vincent Mailhol wrote:
> There are two common errors which are made when reporting the CAN RX
> statistics:
>
> 1. Incrementing the "normal" RX stats when receiving an Error
> frame. Error frames is an abstraction of Socket CAN and does not
> exist on the wire.
>
> 2. 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.
>
> This patch series fix those two issues for all CAN drivers.
>
> Vincent Mailhol (2):
> can: do not increase rx statistics when receiving CAN error frames
> can: do not increase rx_bytes statistics for RTR frames
I would suggest to upstream this change without bringing it to older
(stable) trees.
It doesn't fix any substantial flaw which needs to be backported IMHO.
Btw. can you please change 'error frames' to 'error message frames'?
We had a discussion some years ago that the 'error frames' are used as
term inside the CAN protocol.
Thanks,
Oliver
>
> drivers/net/can/at91_can.c | 9 ++-------
> drivers/net/can/c_can/c_can_main.c | 8 ++------
> drivers/net/can/cc770/cc770.c | 6 ++----
> drivers/net/can/dev/dev.c | 4 ----
> drivers/net/can/dev/rx-offload.c | 7 +++++--
> drivers/net/can/grcan.c | 3 ++-
> drivers/net/can/ifi_canfd/ifi_canfd.c | 8 ++------
> drivers/net/can/janz-ican3.c | 3 ++-
> drivers/net/can/kvaser_pciefd.c | 8 ++------
> drivers/net/can/m_can/m_can.c | 10 ++--------
> drivers/net/can/mscan/mscan.c | 10 ++++++----
> drivers/net/can/pch_can.c | 6 ++----
> drivers/net/can/peak_canfd/peak_canfd.c | 7 ++-----
> drivers/net/can/rcar/rcar_can.c | 9 +++------
> drivers/net/can/rcar/rcar_canfd.c | 7 ++-----
> drivers/net/can/sja1000/sja1000.c | 5 ++---
> 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 | 10 ++++------
> drivers/net/can/usb/ems_usb.c | 5 ++---
> drivers/net/can/usb/esd_usb2.c | 5 ++---
> drivers/net/can/usb/etas_es58x/es58x_core.c | 7 -------
> .../net/can/usb/kvaser_usb/kvaser_usb_core.c | 2 --
> .../net/can/usb/kvaser_usb/kvaser_usb_hydra.c | 14 ++++----------
> .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 7 ++-----
> drivers/net/can/usb/mcba_usb.c | 3 ++-
> drivers/net/can/usb/peak_usb/pcan_usb.c | 5 ++---
> drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 11 ++++-------
> drivers/net/can/usb/peak_usb/pcan_usb_pro.c | 11 +++++------
> drivers/net/can/usb/ucan.c | 7 +++++--
> drivers/net/can/usb/usb_8dev.c | 10 ++++------
> drivers/net/can/xilinx_can.c | 17 ++++++-----------
> 33 files changed, 86 insertions(+), 147 deletions(-)
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 1/2] can: do not increase rx statistics when receiving CAN error frames
2021-11-23 21:01 ` Oliver Hartkopp
@ 2021-11-23 23:21 ` Vincent MAILHOL
0 siblings, 0 replies; 10+ messages in thread
From: Vincent MAILHOL @ 2021-11-23 23:21 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: Marc Kleine-Budde, linux-can, netdev, linux-kernel,
linux-arm-kernel, linux-sunxi, Jimmy Assarsson, Nicolas Ferre,
Alexandre Belloni, Ludovic Desroches, Chandrasekar Ramakrishnan,
Maxime Ripard, Chen-Yu Tsai, Jernej Skrabec,
Appana Durga Kedareswara rao, Naga Sureshkumar Relli,
Michal Simek, Stephane Grosjean
On Wed. 24 Nov. 2021 à 06:01, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> On 23.11.21 12:53, Vincent Mailhol wrote:
> > CAN error skb is an interface specific to socket CAN. The CAN error
> > skb does not correspond to any actual CAN frame 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.
> >
>
> (..)
>
> > diff --git a/drivers/net/can/dev/rx-offload.c b/drivers/net/can/dev/rx-offload.c
> > index 37b0cc65237b..bb47e9a49240 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_MASK)) {
>
> This looks wrong.
And it is.
> Did you think of CAN_ERR_FLAG ??
Yes, I will fix this in v2.
> > + stats->rx_packets++;
> > + stats->rx_bytes += cf->len;
> > + }
> > netif_receive_skb(skb);
>
> (..)
>
> > 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)) {
>
> Ah, here we are :-)
Thank you!
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 0/2] fix statistics for CAN RTR and Error frames
2021-11-23 21:10 ` [PATCH v1 0/2] fix statistics for CAN RTR and Error frames Oliver Hartkopp
@ 2021-11-23 23:35 ` Vincent MAILHOL
0 siblings, 0 replies; 10+ messages in thread
From: Vincent MAILHOL @ 2021-11-23 23:35 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: Marc Kleine-Budde, linux-can, netdev, linux-kernel,
linux-arm-kernel, linux-sunxi
On Wed. 24 Nov. 2021 at 06:10, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> On 23.11.21 12:53, Vincent Mailhol wrote:
> > There are two common errors which are made when reporting the CAN RX
> > statistics:
> >
> > 1. Incrementing the "normal" RX stats when receiving an Error
> > frame. Error frames is an abstraction of Socket CAN and does not
> > exist on the wire.
> >
> > 2. 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.
> >
> > This patch series fix those two issues for all CAN drivers.
> >
> > Vincent Mailhol (2):
> > can: do not increase rx statistics when receiving CAN error frames
> > can: do not increase rx_bytes statistics for RTR frames
>
> I would suggest to upstream this change without bringing it to older
> (stable) trees.
>
> It doesn't fix any substantial flaw which needs to be backported IMHO.
I fully agree. Bringing it to the stable trees would be a
considerable effort and was not my intent either (thus the
absence of "Fixes" tags).
> Btw. can you please change 'error frames' to 'error message frames'?
>
> We had a discussion some years ago that the 'error frames' are used as
> term inside the CAN protocol.
ACK. Thanks for the clarification on the vocabulary.
Yours sincerely,
Vincent Mailhol
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1 2/2] can: do not increase rx_bytes statistics for RTR frames
2021-11-23 11:53 ` [PATCH v1 2/2] can: do not increase rx_bytes statistics for RTR frames Vincent Mailhol
2021-11-23 18:10 ` Vincent MAILHOL
@ 2021-11-24 17:59 ` Stefan Mätje
2021-11-24 23:55 ` Vincent MAILHOL
1 sibling, 1 reply; 10+ messages in thread
From: Stefan Mätje @ 2021-11-24 17:59 UTC (permalink / raw)
To: linux-can, mailhol.vincent, mkl
Cc: linux-kernel, alexandre.belloni, appana.durga.rao, michal.simek,
mripard, wens, jernej.skrabec, yashi, rcsekar,
naga.sureshkumar.relli, linux-arm-kernel, netdev, linux-sunxi,
s.grosjean, ludovic.desroches, extja, nicolas.ferre
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@kvaser.com>
> CC: Marc Kleine-Budde <mkl@pengutronix.de>
> CC: Nicolas Ferre <nicolas.ferre@microchip.com>
> CC: Alexandre Belloni <alexandre.belloni@bootlin.com>
> CC: Ludovic Desroches <ludovic.desroches@microchip.com>
> CC: Chandrasekar Ramakrishnan <rcsekar@samsung.com>
> CC: Maxime Ripard <mripard@kernel.org>
> CC: Chen-Yu Tsai <wens@csie.org>
> CC: Jernej Skrabec <jernej.skrabec@gmail.com>
> CC: Yasushi SHOJI <yashi@spacecubics.com>
> CC: Appana Durga Kedareswara rao <appana.durga.rao@xilinx.com>
> CC: Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com>
> CC: Michal Simek <michal.simek@xilinx.com>
> CC: Stephane Grosjean <s.grosjean@peak-system.com>
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
> 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/at91_can.c b/drivers/net/can/at91_can.c
> index 3cd872cf9be6..89b22c3f9a01 100644
> --- a/drivers/net/can/at91_can.c
> +++ b/drivers/net/can/at91_can.c
> @@ -617,7 +617,8 @@ static void at91_read_msg(struct net_device *dev, unsigned int mb)
> at91_read_mb(dev, mb, cf);
>
> stats->rx_packets++;
> - stats->rx_bytes += cf->len;
> + if (!(cf->can_id & CAN_RTR_FLAG))
> + stats->rx_bytes += cf->len;
> netif_receive_skb(skb);
>
> can_led_event(dev, CAN_LED_EVENT_RX);
> diff --git a/drivers/net/can/c_can/c_can_main.c b/drivers/net/can/c_can/c_can_main.c
> index 670754a12984..f2eb9ebc875f 100644
> --- a/drivers/net/can/c_can/c_can_main.c
> +++ b/drivers/net/can/c_can/c_can_main.c
> @@ -406,7 +406,8 @@ static int c_can_read_msg_object(struct net_device *dev, int iface, u32 ctrl)
> }
>
> stats->rx_packets++;
> - stats->rx_bytes += frame->len;
> + if (!(frame->can_id & CAN_RTR_FLAG))
> + stats->rx_bytes += frame->len;
>
> netif_receive_skb(skb);
> return 0;
> diff --git a/drivers/net/can/cc770/cc770.c b/drivers/net/can/cc770/cc770.c
> index a5fd8ccedec2..7c703bcbb433 100644
> --- a/drivers/net/can/cc770/cc770.c
> +++ b/drivers/net/can/cc770/cc770.c
> @@ -492,7 +492,8 @@ static void cc770_rx(struct net_device *dev, unsigned int mo, u8 ctrl1)
> }
>
> stats->rx_packets++;
> - stats->rx_bytes += cf->len;
> + if (!(id & CAN_RTR_FLAG))
> + stats->rx_bytes += cf->len;
> netif_rx(skb);
> }
>
> diff --git a/drivers/net/can/dev/rx-offload.c b/drivers/net/can/dev/rx-offload.c
> index bb47e9a49240..b10b73e5ed87 100644
> --- a/drivers/net/can/dev/rx-offload.c
> +++ b/drivers/net/can/dev/rx-offload.c
> @@ -56,7 +56,8 @@ static int can_rx_offload_napi_poll(struct napi_struct *napi, int quota)
> work_done++;
> if (!(cf->can_id & CAN_ERR_MASK)) {
> stats->rx_packets++;
> - stats->rx_bytes += cf->len;
> + if (!(cf->can_id & CAN_RTR_FLAG))
> + stats->rx_bytes += cf->len;
> }
> netif_receive_skb(skb);
> }
> diff --git a/drivers/net/can/grcan.c b/drivers/net/can/grcan.c
> index 78e27940b2af..2f8a08321b41 100644
> --- a/drivers/net/can/grcan.c
> +++ b/drivers/net/can/grcan.c
> @@ -1215,7 +1215,8 @@ static int grcan_receive(struct net_device *dev, int budget)
>
> /* Update statistics and read pointer */
> stats->rx_packets++;
> - stats->rx_bytes += cf->len;
> + if (!(cf->can_id & CAN_RTR_FLAG))
> + stats->rx_bytes += cf->len;
> netif_receive_skb(skb);
>
> rd = grcan_ring_add(rd, GRCAN_MSG_SIZE, dma->rx.size);
> diff --git a/drivers/net/can/ifi_canfd/ifi_canfd.c b/drivers/net/can/ifi_canfd/ifi_canfd.c
> index e8318e984bf2..1f90c829321e 100644
> --- a/drivers/net/can/ifi_canfd/ifi_canfd.c
> +++ b/drivers/net/can/ifi_canfd/ifi_canfd.c
> @@ -316,7 +316,8 @@ static void ifi_canfd_read_fifo(struct net_device *ndev)
> writel(rx_irq_mask, priv->base + IFI_CANFD_INTERRUPT);
>
> stats->rx_packets++;
> - stats->rx_bytes += cf->len;
> + if (!(cf->can_id & CAN_RTR_FLAG))
> + stats->rx_bytes += cf->len;
>
> netif_receive_skb(skb);
> }
> diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
> index 32006dbf5abd..5c589aa9dff8 100644
> --- a/drivers/net/can/janz-ican3.c
> +++ b/drivers/net/can/janz-ican3.c
> @@ -1421,7 +1421,8 @@ static int ican3_recv_skb(struct ican3_dev *mod)
>
> /* update statistics, receive the skb */
> stats->rx_packets++;
> - stats->rx_bytes += cf->len;
> + if (!(cf->can_id & CAN_RTR_FLAG))
> + stats->rx_bytes += cf->len;
> netif_receive_skb(skb);
>
> err_noalloc:
> diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c
> index 483fbd9e6952..88a570476500 100644
> --- a/drivers/net/can/kvaser_pciefd.c
> +++ b/drivers/net/can/kvaser_pciefd.c
> @@ -1193,7 +1193,8 @@ static int kvaser_pciefd_handle_data_packet(struct kvaser_pciefd *pcie,
> ns_to_ktime(div_u64(p->timestamp * 1000,
> pcie->freq_to_ticks_div));
>
> - stats->rx_bytes += cf->len;
> + if (!(cf->can_id & CAN_RTR_FLAG))
> + stats->rx_bytes += cf->len;
> stats->rx_packets++;
>
> return netif_rx(skb);
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index c33035e706bc..eedaf66bff31 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -524,7 +524,8 @@ static int m_can_read_fifo(struct net_device *dev, u32 rxfs)
> m_can_write(cdev, M_CAN_RXF0A, fgi);
>
> stats->rx_packets++;
> - stats->rx_bytes += cf->len;
> + if (!(cf->can_id & CAN_RTR_FLAG))
> + stats->rx_bytes += cf->len;
>
> timestamp = FIELD_GET(RX_BUF_RXTS_MASK, fifo_header.dlc);
>
> diff --git a/drivers/net/can/mscan/mscan.c b/drivers/net/can/mscan/mscan.c
> index 9e1cce0260da..59b8284d00e5 100644
> --- a/drivers/net/can/mscan/mscan.c
> +++ b/drivers/net/can/mscan/mscan.c
> @@ -404,7 +404,8 @@ static int mscan_rx_poll(struct napi_struct *napi, int quota)
> if (canrflg & MSCAN_RXF) {
> mscan_get_rx_frame(dev, frame);
> stats->rx_packets++;
> - stats->rx_bytes += frame->len;
> + if (!(frame->can_id & CAN_RTR_FLAG))
> + stats->rx_bytes += frame->len;
> } else if (canrflg & MSCAN_ERR_IF) {
> mscan_get_err_frame(dev, frame, canrflg);
> }
> diff --git a/drivers/net/can/pch_can.c b/drivers/net/can/pch_can.c
> index 6b45840db1f9..8fbe3560216b 100644
> --- a/drivers/net/can/pch_can.c
> +++ b/drivers/net/can/pch_can.c
> @@ -692,7 +692,8 @@ static int pch_can_rx_normal(struct net_device *ndev, u32 obj_num, int quota)
> rcv_pkts++;
> stats->rx_packets++;
> quota--;
> - stats->rx_bytes += cf->len;
> + if (!(cf->can_id & CAN_RTR_FLAG))
> + stats->rx_bytes += cf->len;
> netif_receive_skb(skb);
>
> pch_fifo_thresh(priv, obj_num);
> diff --git a/drivers/net/can/peak_canfd/peak_canfd.c b/drivers/net/can/peak_canfd/peak_canfd.c
> index d5b8bc6d2980..baae39fa14fc 100644
> --- a/drivers/net/can/peak_canfd/peak_canfd.c
> +++ b/drivers/net/can/peak_canfd/peak_canfd.c
> @@ -315,7 +315,8 @@ static int pucan_handle_can_rx(struct peak_canfd_priv *priv,
> else
> memcpy(cf->data, msg->d, cf->len);
>
> - stats->rx_bytes += cf->len;
> + if (!(cf->can_id & CAN_RTR_FLAG))
> + stats->rx_bytes += cf->len;
> stats->rx_packets++;
>
> pucan_netif_rx(skb, msg->ts_low, msg->ts_high);
> diff --git a/drivers/net/can/rcar/rcar_can.c b/drivers/net/can/rcar/rcar_can.c
> index f408ed9a6ccd..237a97cd2f78 100644
> --- a/drivers/net/can/rcar/rcar_can.c
> +++ b/drivers/net/can/rcar/rcar_can.c
> @@ -666,7 +666,8 @@ static void rcar_can_rx_pkt(struct rcar_can_priv *priv)
>
> can_led_event(priv->ndev, CAN_LED_EVENT_RX);
>
> - stats->rx_bytes += cf->len;
> + if (!(cf->can_id & CAN_RTR_FLAG))
> + stats->rx_bytes += cf->len;
> stats->rx_packets++;
> netif_receive_skb(skb);
> }
> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> index db9d62874e15..b1eded2f2c5d 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c
> @@ -1550,7 +1550,8 @@ static void rcar_canfd_rx_pkt(struct rcar_canfd_channel *priv)
>
> can_led_event(priv->ndev, CAN_LED_EVENT_RX);
>
> - stats->rx_bytes += cf->len;
> + if (!(cf->can_id & CAN_RTR_FLAG))
> + stats->rx_bytes += cf->len;
> stats->rx_packets++;
> netif_receive_skb(skb);
> }
> diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
> index a65546ca9461..d2e444968087 100644
> --- a/drivers/net/can/sja1000/sja1000.c
> +++ b/drivers/net/can/sja1000/sja1000.c
> @@ -380,7 +380,8 @@ static void sja1000_rx(struct net_device *dev)
> sja1000_write_cmdreg(priv, CMD_RRB);
>
> stats->rx_packets++;
> - stats->rx_bytes += cf->len;
> + if (!(id & CAN_RTR_FLAG))
> + stats->rx_bytes += cf->len;
> netif_rx(skb);
>
> can_led_event(dev, CAN_LED_EVENT_RX);
> diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
> index 9a4ebda30510..c53ad59763e0 100644
> --- a/drivers/net/can/slcan.c
> +++ b/drivers/net/can/slcan.c
> @@ -218,7 +218,8 @@ static void slc_bump(struct slcan *sl)
> skb_put_data(skb, &cf, sizeof(struct can_frame));
>
> sl->dev->stats.rx_packets++;
> - sl->dev->stats.rx_bytes += cf.len;
> + if (!(cf.can_id & CAN_RTR_FLAG))
> + sl->dev->stats.rx_bytes += cf.len;
> netif_rx_ni(skb);
> }
>
> diff --git a/drivers/net/can/spi/hi311x.c b/drivers/net/can/spi/hi311x.c
> index 89d9c986a229..1e2929a9521b 100644
> --- a/drivers/net/can/spi/hi311x.c
> +++ b/drivers/net/can/spi/hi311x.c
> @@ -350,7 +350,8 @@ static void hi3110_hw_rx(struct spi_device *spi)
> frame->len);
>
> priv->net->stats.rx_packets++;
> - priv->net->stats.rx_bytes += frame->len;
> + if (!(frame->can_id & CAN_RTR_FLAG))
> + priv->net->stats.rx_bytes += frame->len;
>
> can_led_event(priv->net, CAN_LED_EVENT_RX);
>
> diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c
> index 0579ab74f728..1f47c588b69f 100644
> --- a/drivers/net/can/spi/mcp251x.c
> +++ b/drivers/net/can/spi/mcp251x.c
> @@ -736,7 +736,8 @@ static void mcp251x_hw_rx(struct spi_device *spi, int buf_idx)
> memcpy(frame->data, buf + RXBDAT_OFF, frame->len);
>
> priv->net->stats.rx_packets++;
> - priv->net->stats.rx_bytes += frame->len;
> + if (!(frame->can_id & CAN_RTR_FLAG))
> + priv->net->stats.rx_bytes += frame->len;
>
> can_led_event(priv->net, CAN_LED_EVENT_RX);
>
> diff --git a/drivers/net/can/sun4i_can.c b/drivers/net/can/sun4i_can.c
> index 599174098883..cd3278e5d4f6 100644
> --- a/drivers/net/can/sun4i_can.c
> +++ b/drivers/net/can/sun4i_can.c
> @@ -501,7 +501,8 @@ static void sun4i_can_rx(struct net_device *dev)
> sun4i_can_write_cmdreg(priv, SUN4I_CMD_RELEASE_RBUF);
>
> stats->rx_packets++;
> - stats->rx_bytes += cf->len;
> + if (!(id & CAN_RTR_FLAG))
> + stats->rx_bytes += cf->len;
> netif_rx(skb);
>
> can_led_event(dev, CAN_LED_EVENT_RX);
> diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
> index 7cf65936d02e..f4288f98c006 100644
> --- a/drivers/net/can/usb/ems_usb.c
> +++ b/drivers/net/can/usb/ems_usb.c
> @@ -323,7 +323,8 @@ static void ems_usb_rx_can_msg(struct ems_usb *dev, struct ems_cpc_msg *msg)
> }
>
> stats->rx_packets++;
> - stats->rx_bytes += cf->len;
> + if (!(cf->can_id & CAN_RTR_FLAG))
> + 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 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;
}
>
> 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 3398da323126..adab5c2a8b99 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
> @@ -1206,7 +1206,8 @@ static void kvaser_usb_hydra_rx_msg_std(const struct kvaser_usb *dev,
> memcpy(cf->data, cmd->rx_can.data, cf->len);
>
> stats->rx_packets++;
> - stats->rx_bytes += cf->len;
> + if (!(cf->can_id & CAN_RTR_FLAG))
> + stats->rx_bytes += cf->len;
> netif_rx(skb);
> }
>
> @@ -1284,7 +1285,8 @@ static void kvaser_usb_hydra_rx_msg_ext(const struct kvaser_usb *dev,
> memcpy(cf->data, cmd->rx_can.kcan_payload, cf->len);
>
> stats->rx_packets++;
> - stats->rx_bytes += cf->len;
> + if (!(cf->can_id & CAN_RTR_FLAG))
> + 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 4aebaab9ea9c..14b445643554 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> @@ -1001,7 +1001,8 @@ static void kvaser_usb_leaf_rx_can_msg(const struct kvaser_usb *dev,
> }
>
> stats->rx_packets++;
> - stats->rx_bytes += cf->len;
> + if (!(cf->can_id & CAN_RTR_FLAG))
> + stats->rx_bytes += cf->len;
> netif_rx(skb);
> }
>
> diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c
> index a1a154c08b7f..372f0a5ebb9f 100644
> --- a/drivers/net/can/usb/mcba_usb.c
> +++ b/drivers/net/can/usb/mcba_usb.c
> @@ -458,7 +458,8 @@ static void mcba_usb_process_can(struct mcba_priv *priv,
> memcpy(cf->data, msg->data, cf->len);
>
> stats->rx_packets++;
> - stats->rx_bytes += cf->len;
> + if (!(cf->can_id & CAN_RTR_FLAG))
> + stats->rx_bytes += cf->len;
>
> can_led_event(priv->netdev, CAN_LED_EVENT_RX);
> 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 21b06a738595..d1a4f847f310 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
> @@ -684,7 +684,8 @@ static int pcan_usb_decode_data(struct pcan_usb_msg_context *mc, u8 status_len)
>
> /* update statistics */
> mc->netdev->stats.rx_packets++;
> - mc->netdev->stats.rx_bytes += cf->len;
> + if (!(cf->can_id & CAN_RTR_FLAG))
> + mc->netdev->stats.rx_bytes += cf->len;
> /* push the skb */
> netif_rx(skb);
>
> 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 185f5a98d217..65487ec33566 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> @@ -507,13 +507,13 @@ static int pcan_usb_fd_decode_canmsg(struct pcan_usb_fd_if *usb_if,
> if (rx_msg_flags & PUCAN_MSG_EXT_ID)
> cfd->can_id |= CAN_EFF_FLAG;
>
> - if (rx_msg_flags & PUCAN_MSG_RTR)
> + if (rx_msg_flags & PUCAN_MSG_RTR) {
> cfd->can_id |= CAN_RTR_FLAG;
> - else
> + } else {
> memcpy(cfd->data, rm->d, cfd->len);
> -
> + netdev->stats.rx_bytes += cfd->len;
> + }
> netdev->stats.rx_packets++;
> - netdev->stats.rx_bytes += cfd->len;
>
> peak_usb_netif_rx_64(skb, le32_to_cpu(rm->ts_low),
> le32_to_cpu(rm->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 f6d19879bf40..69fdddcbba98 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
> @@ -536,17 +536,18 @@ static int pcan_usb_pro_handle_canmsg(struct pcan_usb_pro_interface *usb_if,
> if (rx->flags & PCAN_USBPRO_EXT)
> can_frame->can_id |= CAN_EFF_FLAG;
>
> - if (rx->flags & PCAN_USBPRO_RTR)
> + if (rx->flags & PCAN_USBPRO_RTR) {
> can_frame->can_id |= CAN_RTR_FLAG;
> - else
> + } else {
> memcpy(can_frame->data, rx->data, can_frame->len);
> + netdev->stats.rx_bytes += can_frame->len;
> + }
> + netdev->stats.rx_packets++;
>
> hwts = skb_hwtstamps(skb);
> peak_usb_get_ts_time(&usb_if->time_ref, le32_to_cpu(rx->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 d582c39fc8d0..388899019955 100644
> --- a/drivers/net/can/usb/ucan.c
> +++ b/drivers/net/can/usb/ucan.c
> @@ -623,7 +623,8 @@ static void ucan_rx_can_msg(struct ucan_priv *up, struct ucan_message_in *m)
> /* don't count error frames as real packets */
> if (!(cf->can_id & CAN_ERR_FLAG)) {
> stats->rx_packets++;
> - stats->rx_bytes += cf->len;
> + if (!(cf->can_id & CAN_RTR_FLAG))
> + stats->rx_bytes += cf->len;
> }
>
> /* pass it to Linux */
> diff --git a/drivers/net/can/usb/usb_8dev.c b/drivers/net/can/usb/usb_8dev.c
> index 040324362b26..14ae8ed85f80 100644
> --- a/drivers/net/can/usb/usb_8dev.c
> +++ b/drivers/net/can/usb/usb_8dev.c
> @@ -474,13 +474,13 @@ static void usb_8dev_rx_can_msg(struct usb_8dev_priv *priv,
> if (msg->flags & USB_8DEV_EXTID)
> cf->can_id |= CAN_EFF_FLAG;
>
> - if (msg->flags & USB_8DEV_RTR)
> + if (msg->flags & USB_8DEV_RTR) {
> cf->can_id |= CAN_RTR_FLAG;
> - else
> + } else {
> memcpy(cf->data, msg->data, cf->len);
> -
> + stats->rx_bytes += cf->len;
> + }
> stats->rx_packets++;
> - stats->rx_bytes += cf->len;
> netif_rx(skb);
>
> can_led_event(priv->netdev, CAN_LED_EVENT_RX);
> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> index 275e240ab293..7d7239ec9920 100644
> --- a/drivers/net/can/xilinx_can.c
> +++ b/drivers/net/can/xilinx_can.c
> @@ -787,9 +787,9 @@ static int xcan_rx(struct net_device *ndev, int frame_base)
> *(__be32 *)(cf->data) = cpu_to_be32(data[0]);
> if (cf->len > 4)
> *(__be32 *)(cf->data + 4) = cpu_to_be32(data[1]);
> - }
>
> - stats->rx_bytes += cf->len;
> + stats->rx_bytes += cf->len;
> + }
> stats->rx_packets++;
> netif_receive_skb(skb);
>
> @@ -871,7 +871,9 @@ static int xcanfd_rx(struct net_device *ndev, int frame_base)
> *(__be32 *)(cf->data + i) = cpu_to_be32(data[0]);
> }
> }
> - stats->rx_bytes += cf->len;
> +
> + if (!(cf->can_id & CAN_RTR_FLAG))
> + stats->rx_bytes += cf->len;
> stats->rx_packets++;
> netif_receive_skb(skb);
>
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v1 2/2] can: do not increase rx_bytes statistics for RTR frames
2021-11-24 17:59 ` Stefan Mätje
@ 2021-11-24 23:55 ` Vincent MAILHOL
0 siblings, 0 replies; 10+ messages in thread
From: Vincent MAILHOL @ 2021-11-24 23:55 UTC (permalink / raw)
To: Stefan Mätje
Cc: linux-can, mkl, linux-kernel, alexandre.belloni,
appana.durga.rao, michal.simek, mripard, wens, jernej.skrabec,
yashi, rcsekar, naga.sureshkumar.relli, linux-arm-kernel, netdev,
linux-sunxi, s.grosjean, ludovic.desroches, extja, nicolas.ferre
Hi Stefan,
On Thu. 25 Nov 2021 at 02:59, Stefan Mätje <Stefan.Maetje@esd.eu> 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@kvaser.com>
> > CC: Marc Kleine-Budde <mkl@pengutronix.de>
> > CC: Nicolas Ferre <nicolas.ferre@microchip.com>
> > CC: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > CC: Ludovic Desroches <ludovic.desroches@microchip.com>
> > CC: Chandrasekar Ramakrishnan <rcsekar@samsung.com>
> > CC: Maxime Ripard <mripard@kernel.org>
> > CC: Chen-Yu Tsai <wens@csie.org>
> > CC: Jernej Skrabec <jernej.skrabec@gmail.com>
> > CC: Yasushi SHOJI <yashi@spacecubics.com>
> > CC: Appana Durga Kedareswara rao <appana.durga.rao@xilinx.com>
> > CC: Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com>
> > CC: Michal Simek <michal.simek@xilinx.com>
> > CC: Stephane Grosjean <s.grosjean@peak-system.com>
> > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > ---
> > 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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-11-24 23:55 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-23 11:53 [PATCH v1 0/2] fix statistics for CAN RTR and Error frames Vincent Mailhol
2021-11-23 11:53 ` [PATCH v1 1/2] can: do not increase rx statistics when receiving CAN error frames Vincent Mailhol
2021-11-23 21:01 ` Oliver Hartkopp
2021-11-23 23:21 ` Vincent MAILHOL
2021-11-23 11:53 ` [PATCH v1 2/2] can: do not increase rx_bytes statistics for RTR frames Vincent Mailhol
2021-11-23 18:10 ` Vincent MAILHOL
2021-11-24 17:59 ` Stefan Mätje
2021-11-24 23:55 ` Vincent MAILHOL
2021-11-23 21:10 ` [PATCH v1 0/2] fix statistics for CAN RTR and Error frames Oliver Hartkopp
2021-11-23 23:35 ` Vincent MAILHOL
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).