Re: [PATCH can-next v2] can: m_can: switch to rx-offload implementation

[Markus Schneider-Pargmann]

Hi Marc,

sorry for the delay, I was on summer vacation.

On Fri Jul 10, 2026 at 1:50 PM CEST, Marc Kleine-Budde wrote:
> The current m_can driver uses NAPI for mmio devices to handle RX'ed CAN
> frames, the RX IRQ is disabled and a NAPI poll is scheduled. Then in
> m_can_poll() the RX'ed CAN frames are read from the device.
>
> The driver already uses rx-offload for SPI devices like the tcan4x5x,
> indicated by struct m_can_classdev::is_peripheral being set.
>
> This approach has 2 drawbacks:
>
> - Under high system load it might take too long from the initial RX IRQ to
>   the NAPI poll function to run. This causes RX buffer overflows.
> - The driver contains several checks if it handles a peripheral or a memory
>   mapped device, that makes maintenance harder.
>
> Convert the driver to unconditionally call m_can_rx_handler() from the IRQ
> handler (m_can_interrupt_handler()), which reads the RX'ed CAN frames from
> the hardware and adds it to a list sorted by RX timestamp. This list of
> RX'ed SKBs is then passed to the networking stack in a later NAPI context.
>
> Remove all manual napi handling from the driver and keep the
> can_rx_offload_*().

Thanks for your work, this is a nice improvement. Comments below.

>
> Signed-off-by: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
> ---
> Changes in v2:
> - m_can_receive_skb(): remove double accounting of RX packets (found by sashiko)
> - remove obsolete struct m_can_classdev::napi (found by sashiko)
> - m_can_do_rx_poll(): remove quota, read all RX'ed messages (found by sashiko)
> - Link to v1: https://patch.msgid.link/20260709-m_can-rx-offload-v1-1-af3efa8e4272@xxxxxxxxxxxxxx
>
> To: Markus Schneider-Pargmann <msp@xxxxxxxxxxxx>
> To: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
> To: Vincent Mailhol <mailhol@xxxxxxxxxx>
> Cc: linux-can@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> ---
>  drivers/net/can/m_can/m_can.c | 151 +++++++++++-------------------------------
>  drivers/net/can/m_can/m_can.h |   2 -
>  2 files changed, 38 insertions(+), 115 deletions(-)
>
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index eb856547ae7d..866c4b501dad 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -530,26 +530,17 @@ static void m_can_clean(struct net_device *net)
>  	spin_unlock_irqrestore(&cdev->tx_handling_spinlock, irqflags);
>  }
>  
> -/* For peripherals, pass skb to rx-offload, which will push skb from
> - * napi. For non-peripherals, RX is done in napi already, so push
> - * directly. timestamp is used to ensure good skb ordering in
> - * rx-offload and is ignored for non-peripherals.
> - */
>  static void m_can_receive_skb(struct m_can_classdev *cdev,
>  			      struct sk_buff *skb,
>  			      u32 timestamp)
>  {
> -	if (cdev->is_peripheral) {
> -		struct net_device_stats *stats = &cdev->net->stats;
> -		int err;
> +	struct net_device_stats *stats = &cdev->net->stats;
> +	int err;
>  
> -		err = can_rx_offload_queue_timestamp(&cdev->offload, skb,
> -						     timestamp);
> -		if (err)
> -			stats->rx_fifo_errors++;
> -	} else {
> -		netif_receive_skb(skb);
> -	}
> +	err = can_rx_offload_queue_timestamp(&cdev->offload, skb,
> +					     timestamp);

An AI noticed (and I checked) in can_rx_offload_queue_timestamp() it
checks the queue length of skb_queue and drops packets if that queue is
already full. But the function actually adds it to the skb_irq_queue
which length is not checked at all in this function. skb_irq_queue is
only much later added to the skb_queue, at which point it doesn't drop
any packets over the limit. Shouldn't it be checking the sum of the
lengths of both queues? Or is this intentional?

> +	if (err)
> +		stats->rx_fifo_errors++;
>  }
>  
>  static int m_can_read_fifo(struct net_device *dev, u32 fgi)
> @@ -600,10 +591,7 @@ static int m_can_read_fifo(struct net_device *dev, u32 fgi)
>  				      cf->data, DIV_ROUND_UP(cf->len, 4));
>  		if (err)
>  			goto out_free_skb;
> -
> -		stats->rx_bytes += cf->len;
>  	}
> -	stats->rx_packets++;
>  
>  	timestamp = FIELD_GET(RX_BUF_RXTS_MASK, fifo_header.dlc) << 16;
>  
> @@ -618,10 +606,9 @@ static int m_can_read_fifo(struct net_device *dev, u32 fgi)
>  	return err;
>  }
>  
> -static int m_can_do_rx_poll(struct net_device *dev, int quota)
> +static int m_can_do_rx_poll(struct net_device *dev)

This function doesn't have anything to do with polling anymore. Maybe it
would better to rename this?

>  {
>  	struct m_can_classdev *cdev = netdev_priv(dev);
> -	u32 pkts = 0;
>  	u32 rxfs;
>  	u32 rx_count;
>  	u32 fgi;
> @@ -638,13 +625,11 @@ static int m_can_do_rx_poll(struct net_device *dev, int quota)
>  	rx_count = FIELD_GET(RXFS_FFL_MASK, rxfs);
>  	fgi = FIELD_GET(RXFS_FGI_MASK, rxfs);
>  
> -	for (i = 0; i < rx_count && quota > 0; ++i) {
> +	for (i = 0; i < rx_count; ++i) {

If I understand correctly you are removing any limit on how much is
retrieved and you are doing this in the main irq handler now, not
threaded. Did you measure how long the non-threaded irq-handler actually
takes with this change and if the CAN bus is under pressure? Would it
make sense to do this in a threaded irq handler?

>  		err = m_can_read_fifo(dev, fgi);
>  		if (err)
>  			break;
>  
> -		quota--;
> -		pkts++;

You are not counting or returning the number of packets anymore but use
it in the calling code. Is this intended?

>  		ack_fgi = fgi;
>  		fgi = (++fgi >= cdev->mcfg[MRAM_RXF0].num ? 0 : fgi);
>  	}
> @@ -652,10 +637,7 @@ static int m_can_do_rx_poll(struct net_device *dev, int quota)
>  	if (ack_fgi != -1)
>  		m_can_write(cdev, M_CAN_RXF0A, ack_fgi);
>  
> -	if (err)
> -		return err;
> -
> -	return pkts;
> +	return err;
>  }
>  
>  static int m_can_handle_lost_msg(struct net_device *dev)
> @@ -678,8 +660,7 @@ static int m_can_handle_lost_msg(struct net_device *dev)
>  	frame->can_id |= CAN_ERR_CRTL;
>  	frame->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
>  
> -	if (cdev->is_peripheral)
> -		timestamp = m_can_get_timestamp(cdev);
> +	timestamp = m_can_get_timestamp(cdev);
>  
>  	m_can_receive_skb(cdev, skb, timestamp);
>  
> @@ -750,8 +731,7 @@ static int m_can_handle_lec_err(struct net_device *dev,
>  	if (unlikely(!skb))
>  		return 0;
>  
> -	if (cdev->is_peripheral)
> -		timestamp = m_can_get_timestamp(cdev);
> +	timestamp = m_can_get_timestamp(cdev);
>  
>  	m_can_receive_skb(cdev, skb, timestamp);
>  
> @@ -883,8 +863,7 @@ static int m_can_handle_state_change(struct net_device *dev,
>  		break;
>  	}
>  
> -	if (cdev->is_peripheral)
> -		timestamp = m_can_get_timestamp(cdev);
> +	timestamp = m_can_get_timestamp(cdev);
>  
>  	m_can_receive_skb(cdev, skb, timestamp);
>  
> @@ -973,8 +952,7 @@ static int m_can_handle_protocol_error(struct net_device *dev, u32 irqstatus)
>  		return 0;
>  	}
>  
> -	if (cdev->is_peripheral)
> -		timestamp = m_can_get_timestamp(cdev);
> +	timestamp = m_can_get_timestamp(cdev);
>  
>  	m_can_receive_skb(cdev, skb, timestamp);
>  
> @@ -1055,7 +1033,7 @@ static int m_can_rx_handler(struct net_device *dev, int quota, u32 irqstatus)

This function still has the argument quota but it seems unused now.

>  						     m_can_read(cdev, M_CAN_PSR));
>  
>  	if (irqstatus & IR_RF0N) {
> -		rx_work_or_err = m_can_do_rx_poll(dev, (quota - work_done));
> +		rx_work_or_err = m_can_do_rx_poll(dev);

You are only returning error from m_can_do_rx_poll now. work_done will
always be 0 now. The work_done += line is not useful anymore.

Looking more closely I think work_done in m_can_rx_handler is completely
useless now and could be removed here and in the called functions?

Best
Markus

Attachment: signature.asc
Description: PGP signature