Skip to contentSkip to conversation

dummy_can: fix packet statistics

PATCH Superseded Oliver Hartkopp 2 participants 3 messages Jan 24 – Jan 26 CAN network drivers

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 dummy_can: fix packet statistics

The former implementation was only counting the tx_packets value. Adopt the skb handling from vcan.c to correctly count the statistics and fix the packet flow for looped packets.
The CAN echo support (IFF_ECHO) is enabled in dummy_can_init() to provide a more realistic behaviour of real CAN hardware interfaces.

Fixes 816cf430e84b ("can: add dummy_can driver")
Cc Vincent Mailhol <[email protected]>
---
 drivers/net/can/dummy_can.c | 50 ++++++++++++++++++++++++++++++++++---
 1 file changed, 47 insertions(+), 3 deletions(-)
drivers/net/can/dummy_can.c +47 -3
index 41953655e3d3..a77f5fc5cfda 100644
@@ -207,17 +207,60 @@ static int dummy_can_netdev_close(struct net_device *dev)
 }
 
 static netdev_tx_t dummy_can_start_xmit(struct sk_buff *skb,
 					struct net_device *dev)
 {
+	struct net_device_stats *stats = &dev->stats;
+	unsigned int len;
+	bool loop;
+
 	if (can_dev_dropped_skb(dev, skb))
 		return NETDEV_TX_OK;
 
-	can_put_echo_skb(skb, dev, 0, 0);
-	dev->stats.tx_packets++;
-	dev->stats.tx_bytes += can_get_echo_skb(dev, 0, NULL);
+	len = can_skb_get_data_len(skb);
+	stats->tx_packets++;
+	stats->tx_bytes += len;
+
+	/* set flag whether this packet has to be looped back */
+	loop = skb->pkt_type == PACKET_LOOPBACK;
+
+	skb_tx_timestamp(skb);
+
+	/* driver supports echo handling - see dummy_can_init() */
+	if (!(dev->flags & IFF_ECHO)) {
+		/* no echo handling available inside this driver */
+		if (loop) {
+			/* only count the packets here, because the
+			 * CAN core already did the echo for us
+			 */
+			stats->rx_packets++;
+			stats->rx_bytes += len;
+		}
+		consume_skb(skb);
+		return NETDEV_TX_OK;
+	}
+
+	/* perform standard echo handling for CAN network interfaces */
+
+	if (loop) {
+		skb = can_create_echo_skb(skb);
+		if (!skb)
+			return NETDEV_TX_OK;
 
+		/* receive with packet counting */
+		stats->rx_packets++;
+		stats->rx_bytes += len;
+
+		skb->pkt_type  = PACKET_BROADCAST;
+		skb->dev       = dev;
+		skb->ip_summed = CHECKSUM_UNNECESSARY;
+
+		netif_rx(skb);
+	} else {
+		/* no looped packets => no counting */
+		consume_skb(skb);
+	}
 	return NETDEV_TX_OK;
 }
 
 static const struct net_device_ops dummy_can_netdev_ops = {
 	.ndo_open = dummy_can_netdev_open,
@@ -239,10 +282,11 @@ static int __init dummy_can_init(void)
 	if (!dev)
 		return -ENOMEM;
 
 	dev->netdev_ops = &dummy_can_netdev_ops;
 	dev->ethtool_ops = &dummy_can_ethtool_ops;
+	dev->flags |= IFF_ECHO; /* enable echo handling */
 	priv = netdev_priv(dev);
 	priv->can.bittiming_const = &dummy_can_bittiming_const;
 	priv->can.bitrate_max = 20 * MEGA /* BPS */;
 	priv->can.clock.freq = 160 * MEGA /* Hz */;
 	priv->can.fd.data_bittiming_const = &dummy_can_fd_databittiming_const;
Signature · 1 line
2.47.3
maintainer
#1 lore raw

Hi Oliver,

Quoted 3 lines · Oliver Hartkopp

The former implementation was only counting the tx_packets value. Adopt the skb handling from vcan.c to correctly count the statistics and fix the packet flow for looped packets.

My intent was to have the dummy_can's xmit() behave like the other hardware interface: just have the echo skb. I am not sure why a real vcan-style loop back would be needed here.

That also means that dummy_can would never be able to receive a packet, but I do not see the issue with that.

Quoted 2 lines

The CAN echo support (IFF_ECHO) is enabled in dummy_can_init() to provide a more realistic behaviour of real CAN hardware interfaces.

Ack. The IFF_ECHO is needed, otherwise the echo skb is discarded right away and the tx_bytes are never increased. Actually, this is not the only driver which forget to set up its IFF_ECHO flag.

So, for me, the fix would be to just add the IFF_ECHO. After doing just this locally, the statistics behave the same as with my real hardware
interfaces: when I send a packet only the TX stats are increased and the RX stats remains unchanged.

Yours sincerely,
Vincent Mailhol

#2 lore raw
Quoted 17 lines · Vincent Mailhol

Hi Oliver,

Quoted 3 lines · Oliver Hartkopp

The former implementation was only counting the tx_packets value. Adopt the skb handling from vcan.c to correctly count the statistics and fix the packet flow for looped packets.

My intent was to have the dummy_can's xmit() behave like the other hardware interface: just have the echo skb. I am not sure why a real vcan-style loop back would be needed here.

That also means that dummy_can would never be able to receive a packet, but I do not see the issue with that.

Quoted 2 lines

The CAN echo support (IFF_ECHO) is enabled in dummy_can_init() to provide a more realistic behaviour of real CAN hardware interfaces.

Quoted 8 lines

Ack. The IFF_ECHO is needed, otherwise the echo skb is discarded right away and the tx_bytes are never increased. Actually, this is not the only driver which forget to set up its IFF_ECHO flag.

So, for me, the fix would be to just add the IFF_ECHO. After doing just this locally, the statistics behave the same as with my real hardware
interfaces: when I send a packet only the TX stats are increased and the RX stats remains unchanged.

Correct!

I've sent a v2 patch.
Thanks for the review!

Best regards,
Oliver