From 676068db69b847f06fe054fca15bf6b107bd24da Mon Sep 17 00:00:00 2001 From: Vincent Mailhol Date: Tue, 7 Dec 2021 21:15:27 +0900 Subject: can: do not increase rx statistics when generating a CAN rx error message frame MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. Link: https://lore.kernel.org/all/20211207121531.42941-2-mailhol.vincent@wanadoo.fr CC: Marc Kleine-Budde CC: Nicolas Ferre CC: Alexandre Belloni CC: Ludovic Desroches CC: Chandrasekar Ramakrishnan CC: Maxime Ripard CC: Chen-Yu Tsai CC: Jernej Skrabec CC: Appana Durga Kedareswara rao CC: Naga Sureshkumar Relli CC: Michal Simek CC: Stephane Grosjean Tested-by: Jimmy Assarsson # kvaser Signed-off-by: Vincent Mailhol Acked-by: Stefan Mätje # esd_usb2 Tested-by: Stefan Mätje # esd_usb2 Signed-off-by: Marc Kleine-Budde --- drivers/net/can/rcar/rcar_can.c | 6 +----- drivers/net/can/rcar/rcar_canfd.c | 4 ---- 2 files changed, 1 insertion(+), 9 deletions(-) (limited to 'drivers/net/can/rcar') 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); } } -- cgit v1.2.3 From 8e674ca74244eac1cd85c6e9a89b588a03c55ff7 Mon Sep 17 00:00:00 2001 From: Vincent Mailhol Date: Tue, 7 Dec 2021 21:15:30 +0900 Subject: can: do not increase rx_bytes statistics for RTR frames MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The actual payload length of the CAN Remote Transmission Request (RTR) frames is always 0, i.e. no payload 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::rx_bytes should not be increased for the RTR frames. This patch fixes all the CAN drivers. Link: https://lore.kernel.org/all/20211207121531.42941-5-mailhol.vincent@wanadoo.fr Cc: Marc Kleine-Budde Cc: Nicolas Ferre Cc: Alexandre Belloni Cc: Ludovic Desroches Cc: Chandrasekar Ramakrishnan Cc: Maxime Ripard Cc: Chen-Yu Tsai Cc: Jernej Skrabec Cc: Yasushi SHOJI Cc: Appana Durga Kedareswara rao Cc: Naga Sureshkumar Relli Cc: Michal Simek Cc: Stephane Grosjean Tested-by: Jimmy Assarsson # kvaser Signed-off-by: Vincent Mailhol Acked-by: Stefan Mätje # esd_usb2 Tested-by: Stefan Mätje # esd_usb2 Signed-off-by: Marc Kleine-Budde --- drivers/net/can/rcar/rcar_can.c | 5 +++-- drivers/net/can/rcar/rcar_canfd.c | 3 ++- 2 files changed, 5 insertions(+), 3 deletions(-) (limited to 'drivers/net/can/rcar') diff --git a/drivers/net/can/rcar/rcar_can.c b/drivers/net/can/rcar/rcar_can.c index f408ed9a6ccd..62bbd58bfef8 100644 --- a/drivers/net/can/rcar/rcar_can.c +++ b/drivers/net/can/rcar/rcar_can.c @@ -662,12 +662,13 @@ static void rcar_can_rx_pkt(struct rcar_can_priv *priv) for (dlc = 0; dlc < cf->len; dlc++) cf->data[dlc] = readb(&priv->regs->mb[RCAR_CAN_RX_FIFO_MBX].data[dlc]); + + stats->rx_bytes += cf->len; } + stats->rx_packets++; can_led_event(priv->ndev, CAN_LED_EVENT_RX); - 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); } -- cgit v1.2.3 From cc4b08c31b5c51352f258032cc65e884b3e61e6a Mon Sep 17 00:00:00 2001 From: Vincent Mailhol Date: Tue, 7 Dec 2021 21:15:31 +0900 Subject: can: do not increase tx_bytes statistics for RTR frames MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The actual payload length of the CAN Remote Transmission Request (RTR) frames is always 0, i.e. no payload 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") Link: https://lore.kernel.org/all/20211207121531.42941-6-mailhol.vincent@wanadoo.fr Cc: Nicolas Ferre Cc: Alexandre Belloni Cc: Ludovic Desroches Cc: Maxime Ripard Cc: Chen-Yu Tsai Cc: Jernej Skrabec Cc: Yasushi SHOJI Cc: Oliver Hartkopp Cc: Stephane Grosjean Cc: Andreas Larsson Tested-by: Jimmy Assarsson # kvaser Signed-off-by: Vincent Mailhol Acked-by: Stefan Mätje # esd_usb2 Tested-by: Stefan Mätje # esd_usb2 [mkl: add conversion for grcan] Signed-off-by: Marc Kleine-Budde --- drivers/net/can/rcar/rcar_can.c | 11 +++++------ drivers/net/can/rcar/rcar_canfd.c | 6 +----- 2 files changed, 6 insertions(+), 11 deletions(-) (limited to 'drivers/net/can/rcar') 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); -- cgit v1.2.3 From 7d4a101c0bd3c6e5c6e45c705a54f7bc8f6c128d Mon Sep 17 00:00:00 2001 From: Vincent Mailhol Date: Tue, 14 Dec 2021 01:02:24 +0900 Subject: can: dev: add sanity check in can_set_static_ctrlmode() Previous patch removed can_priv::ctrlmode_static to replace it with can_get_static_ctrlmode(). A condition sine qua non for this to work is that the controller static modes should never be set in can_priv::ctrlmode_supported (c.f. the comment on can_priv::ctrlmode_supported which states that it is for "options that can be *modified* by netlink"). Also, this condition is already correctly fulfilled by all existing drivers which rely on the ctrlmode_static feature. Nonetheless, we added an extra safeguard in can_set_static_ctrlmode() to return an error value and to warn the developer who would be adventurous enough to set to static a given feature that is already set to supported. The drivers which rely on the static controller mode are then updated to check the return value of can_set_static_ctrlmode(). Link: https://lore.kernel.org/all/20211213160226.56219-3-mailhol.vincent@wanadoo.fr Signed-off-by: Vincent Mailhol Signed-off-by: Marc Kleine-Budde --- drivers/net/can/rcar/rcar_canfd.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'drivers/net/can/rcar') diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c index d0e795f60338..82cc71d5ee10 100644 --- a/drivers/net/can/rcar/rcar_canfd.c +++ b/drivers/net/can/rcar/rcar_canfd.c @@ -1699,7 +1699,9 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch, &rcar_canfd_data_bittiming_const; /* Controller starts in CAN FD only mode */ - can_set_static_ctrlmode(ndev, CAN_CTRLMODE_FD); + err = can_set_static_ctrlmode(ndev, CAN_CTRLMODE_FD); + if (err) + goto fail; priv->can.ctrlmode_supported = CAN_CTRLMODE_BERR_REPORTING; } else { /* Controller starts in Classical CAN only mode */ -- cgit v1.2.3