Commit cc4b08c3 authored by Vincent Mailhol's avatar Vincent Mailhol Committed by Marc Kleine-Budde
Browse files

can: do not increase tx_bytes statistics for RTR frames 

The actual payload length of the CAN Remote Transmission Request (RTR)
frames is always 0, i.e. 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 ed3320ce ("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 <nicolas.ferre@microchip.com>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Ludovic Desroches <ludovic.desroches@microchip.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: Oliver Hartkopp <socketcan@hartkopp.net>
Cc: Stephane Grosjean <s.grosjean@peak-system.com>
Cc: Andreas Larsson <andreas@gaisler.com>
Tested-by: Jimmy Assarsson <extja@kvaser.com> # kvaser
Signed-off-by: default avatarVincent Mailhol <mailhol.vincent@wanadoo.fr>
Acked-by: Stefan Mätje <stefan.maetje@esd.eu> # esd_usb2
Tested-by: Stefan Mätje <stefan.maetje@esd.eu> # esd_usb2
[mkl: add conversion for grcan]
Signed-off-by: default avatarMarc Kleine-Budde <mkl@pengutronix.de>
parent 8e674ca7

drivers/net/can/at91_can.c

+4 −4
Original line number Diff line number Diff line
@@ -448,7 +448,6 @@ static void at91_chip_stop(struct net_device *dev, enum can_state state) 
static netdev_tx_t at91_start_xmit(struct sk_buff *skb, struct net_device *dev)

{

	struct at91_priv *priv = netdev_priv(dev);

	struct net_device_stats *stats = &dev->stats;

	struct can_frame *cf = (struct can_frame *)skb->data;

	unsigned int mb, prio;

	u32 reg_mid, reg_mcr;
@@ -480,8 +479,6 @@ static netdev_tx_t at91_start_xmit(struct sk_buff *skb, struct net_device *dev) 
	/* This triggers transmission */

	at91_write(priv, AT91_MCR(mb), reg_mcr);



	stats->tx_bytes += cf->len;



	/* _NOTE_: subtract AT91_MB_TX_FIRST offset from mb! */

	can_put_echo_skb(skb, dev, mb - get_mb_tx_first(priv), 0);
@@ -852,7 +849,10 @@ static void at91_irq_tx(struct net_device *dev, u32 reg_sr) 
		if (likely(reg_msr & AT91_MSR_MRDY &&

			   ~reg_msr & AT91_MSR_MABT)) {

			/* _NOTE_: subtract AT91_MB_TX_FIRST offset from mb! */

			can_get_echo_skb(dev, mb - get_mb_tx_first(priv), NULL);

			dev->stats.tx_bytes +=

				can_get_echo_skb(dev,

						 mb - get_mb_tx_first(priv),

						 NULL);

			dev->stats.tx_packets++;

			can_led_event(dev, CAN_LED_EVENT_TX);

		}

drivers/net/can/c_can/c_can.h

+0 −1
Original line number Diff line number Diff line
@@ -211,7 +211,6 @@ struct c_can_priv { 
	struct c_can_raminit raminit_sys;	/* RAMINIT via syscon regmap */

	void (*raminit)(const struct c_can_priv *priv, bool enable);

	u32 comm_rcv_high;

	u32 dlc[];

};



struct net_device *alloc_c_can_dev(int msg_obj_num);

drivers/net/can/c_can/c_can_main.c

+2 −5
Original line number Diff line number Diff line
@@ -477,7 +477,6 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb, 
	 * transmit as we might race against do_tx().

	 */

	c_can_setup_tx_object(dev, IF_TX, frame, idx);

	priv->dlc[idx] = frame->len;

	can_put_echo_skb(skb, dev, idx, 0);

	obj = idx + priv->msg_obj_tx_first;

	c_can_object_put(dev, IF_TX, obj, cmd);
@@ -742,8 +741,7 @@ static void c_can_do_tx(struct net_device *dev) 
		 * NAPI. We are not transmitting.

		 */

		c_can_inval_tx_object(dev, IF_NAPI, obj);

		can_get_echo_skb(dev, idx, NULL);

		bytes += priv->dlc[idx];

		bytes += can_get_echo_skb(dev, idx, NULL);

		pkts++;

	}
@@ -1227,8 +1225,7 @@ struct net_device *alloc_c_can_dev(int msg_obj_num) 
	struct c_can_priv *priv;

	int msg_obj_tx_num = msg_obj_num / 2;



	dev = alloc_candev(struct_size(priv, dlc, msg_obj_tx_num),

			   msg_obj_tx_num);

	dev = alloc_candev(sizeof(*priv), msg_obj_tx_num);

	if (!dev)

		return NULL;

drivers/net/can/cc770/cc770.c

+2 −6
Original line number Diff line number Diff line
@@ -664,7 +664,6 @@ static void cc770_tx_interrupt(struct net_device *dev, unsigned int o) 
	struct cc770_priv *priv = netdev_priv(dev);

	struct net_device_stats *stats = &dev->stats;

	unsigned int mo = obj2msgobj(o);

	struct can_frame *cf;

	u8 ctrl1;



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

	}



	cf = (struct can_frame *)priv->tx_skb->data;

	stats->tx_bytes += cf->len;

	stats->tx_packets++;



	can_put_echo_skb(priv->tx_skb, dev, 0, 0);

	can_get_echo_skb(dev, 0, NULL);

	stats->tx_bytes += can_get_echo_skb(dev, 0, NULL);

	stats->tx_packets++;

	priv->tx_skb = NULL;



	netif_wake_queue(dev);

drivers/net/can/grcan.c

+2 −15
Original line number Diff line number Diff line
@@ -255,7 +255,6 @@ struct grcan_priv { 
	struct grcan_dma dma;



	struct sk_buff **echo_skb;	/* We allocate this on our own */

	u8 *txdlc;			/* Length of queued frames */



	/* The echo skb pointer, pointing into echo_skb and indicating which

	 * frames can be echoed back. See the "Notes on the tx cyclic buffer
@@ -515,9 +514,7 @@ static int catch_up_echo_skb(struct net_device *dev, int budget, bool echo) 
		if (echo) {

			/* Normal echo of messages */

			stats->tx_packets++;

			stats->tx_bytes += priv->txdlc[i];

			priv->txdlc[i] = 0;

			can_get_echo_skb(dev, i, NULL);

			stats->tx_bytes += can_get_echo_skb(dev, i, NULL);

		} else {

			/* For cleanup of untransmitted messages */

			can_free_echo_skb(dev, i, NULL);
@@ -1062,16 +1059,10 @@ static int grcan_open(struct net_device *dev) 
	priv->can.echo_skb_max = dma->tx.size;

	priv->can.echo_skb = priv->echo_skb;



	priv->txdlc = kcalloc(dma->tx.size, sizeof(*priv->txdlc), GFP_KERNEL);

	if (!priv->txdlc) {

		err = -ENOMEM;

		goto exit_free_echo_skb;

	}



	/* Get can device up */

	err = open_candev(dev);

	if (err)

		goto exit_free_txdlc;

		goto exit_free_echo_skb;



	err = request_irq(dev->irq, grcan_interrupt, IRQF_SHARED,

			  dev->name, dev);
@@ -1093,8 +1084,6 @@ static int grcan_open(struct net_device *dev) 


exit_close_candev:

	close_candev(dev);

exit_free_txdlc:

	kfree(priv->txdlc);

exit_free_echo_skb:

	kfree(priv->echo_skb);

exit_free_dma_buffers:
@@ -1129,7 +1118,6 @@ static int grcan_close(struct net_device *dev) 
	priv->can.echo_skb_max = 0;

	priv->can.echo_skb = NULL;

	kfree(priv->echo_skb);

	kfree(priv->txdlc);



	return 0;

}
@@ -1447,7 +1435,6 @@ static netdev_tx_t grcan_start_xmit(struct sk_buff *skb, 
	 * can_put_echo_skb would be an error unless other measures are

	 * taken.

	 */

	priv->txdlc[slotindex] = cf->len; /* Store dlc for statistics */

	can_put_echo_skb(skb, dev, slotindex, 0);



	/* Make sure everything is written before allowing hardware to