[PATCH] can: dev: do not increment rx stats when generating a CAN error skb

[PATCH] can: dev: do not increment rx stats when generating a CAN error skb

[Vincent Mailhol]

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 is 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.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/net/can/dev/dev.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/can/dev/dev.c b/drivers/net/can/dev/dev.c
index d9281ae853f8..6855cbe3cae7 100644
--- a/drivers/net/can/dev/dev.c
+++ b/drivers/net/can/dev/dev.c
@@ -135,7 +135,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;
@@ -154,9 +153,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:
-- 
2.26.2

Re: [PATCH] can: dev: do not increment rx stats when generating a CAN error skb

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 812 bytes --]

On 17.03.2021 17:52:23, 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 is 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.

I think we increment the rx packets and bytes (more or less
consistently) in all drivers for CAN error skbs.

Marc

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] can: dev: do not increment rx stats when generating a CAN error skb

[Vincent MAILHOL]

On Fri. 19 Mar 2021 at 17:29, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 17.03.2021 17:52:23, 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 is 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.
>
> I think we increment the rx packets and bytes (more or less
> consistently) in all drivers for CAN error skbs.

If I understand correctly your comment, you mean that this patch
alone is not good because it will create a discrepancy with the
drivers. This is fair enough.

The real question is what is the meaning of rx_packets and
rx_bytes? My understanding is that those statistics should
represent what is sent on the wire.  Do we agree? If not, what is
the rationale for counting error skb in the device statistics?
struct can_device_stats already keeps track of the errors
statistics so incrementing the struct net_device_stats adds no
information here.

I am fine to send a more complete patch which will also do the
cleanup on all drivers. But before starting that work, I prefer
to confirm that we are aligned.


Yours sincerely,
Vincent