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

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

[Marc Kleine-Budde]

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_*().

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/m_can/m_can.c | 134 +++++++++++-------------------------------
 drivers/net/can/m_can/m_can.h |   1 -
 2 files changed, 34 insertions(+), 101 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index eb856547ae7d..8175b030257b 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);
+	if (err)
+		stats->rx_fifo_errors++;
 }
 
 static int m_can_read_fifo(struct net_device *dev, u32 fgi)
@@ -678,8 +669,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 +740,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 +872,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 +961,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);
 
@@ -1065,32 +1052,6 @@ static int m_can_rx_handler(struct net_device *dev, int quota, u32 irqstatus)
 	return work_done;
 }
 
-static int m_can_poll(struct napi_struct *napi, int quota)
-{
-	struct net_device *dev = napi->dev;
-	struct m_can_classdev *cdev = netdev_priv(dev);
-	int work_done;
-	u32 irqstatus;
-
-	irqstatus = cdev->irqstatus | m_can_read(cdev, M_CAN_IR);
-
-	work_done = m_can_rx_handler(dev, quota, irqstatus);
-
-	/* Don't re-enable interrupts if the driver had a fatal error
-	 * (e.g., FIFO read failure).
-	 */
-	if (work_done >= 0 && work_done < quota) {
-		napi_complete_done(napi, work_done);
-		m_can_enable_all_interrupts(cdev);
-	}
-
-	return work_done;
-}
-
-/* Echo tx skb and update net stats. Peripherals use rx-offload for
- * echo. timestamp is used for peripherals to ensure correct ordering
- * by rx-offload, and is ignored for non-peripherals.
- */
 static unsigned int m_can_tx_update_stats(struct m_can_classdev *cdev,
 					  unsigned int msg_mark, u32 timestamp)
 {
@@ -1098,14 +1059,11 @@ static unsigned int m_can_tx_update_stats(struct m_can_classdev *cdev,
 	struct net_device_stats *stats = &dev->stats;
 	unsigned int frame_len;
 
-	if (cdev->is_peripheral)
-		stats->tx_bytes +=
-			can_rx_offload_get_echo_skb_queue_timestamp(&cdev->offload,
-								    msg_mark,
-								    timestamp,
-								    &frame_len);
-	else
-		stats->tx_bytes += can_get_echo_skb(dev, msg_mark, &frame_len);
+	stats->tx_bytes +=
+		can_rx_offload_get_echo_skb_queue_timestamp(&cdev->offload,
+							    msg_mark,
+							    timestamp,
+							    &frame_len);
 
 	stats->tx_packets++;
 
@@ -1265,21 +1223,10 @@ static int m_can_interrupt_handler(struct m_can_classdev *cdev)
 	if (cdev->ops->clear_interrupts)
 		cdev->ops->clear_interrupts(cdev);
 
-	/* schedule NAPI in case of
-	 * - rx IRQ
-	 * - state change IRQ
-	 * - bus error IRQ and bus error reporting
-	 */
 	if (ir & (IR_RF0N | IR_RF0W | IR_ERR_ALL_30X)) {
-		cdev->irqstatus = ir;
-		if (!cdev->is_peripheral) {
-			m_can_disable_all_interrupts(cdev);
-			napi_schedule(&cdev->napi);
-		} else {
-			ret = m_can_rx_handler(dev, NAPI_POLL_WEIGHT, ir);
-			if (ret < 0)
-				return ret;
-		}
+		ret = m_can_rx_handler(dev, NAPI_POLL_WEIGHT, ir);
+		if (ret < 0)
+			return ret;
 	}
 
 	if (cdev->version == 30) {
@@ -1288,8 +1235,7 @@ static int m_can_interrupt_handler(struct m_can_classdev *cdev)
 			u32 timestamp = 0;
 			unsigned int frame_len;
 
-			if (cdev->is_peripheral)
-				timestamp = m_can_get_timestamp(cdev);
+			timestamp = m_can_get_timestamp(cdev);
 			frame_len = m_can_tx_update_stats(cdev, 0, timestamp);
 			m_can_finish_tx(cdev, 1, frame_len);
 		}
@@ -1304,6 +1250,8 @@ static int m_can_interrupt_handler(struct m_can_classdev *cdev)
 
 	if (cdev->is_peripheral)
 		can_rx_offload_threaded_irq_finish(&cdev->offload);
+	else
+		can_rx_offload_irq_finish(&cdev->offload);
 
 	return IRQ_HANDLED;
 }
@@ -1752,9 +1700,6 @@ static int m_can_dev_setup(struct m_can_classdev *cdev)
 	if (err)
 		return err;
 
-	if (!cdev->is_peripheral)
-		netif_napi_add(dev, &cdev->napi, m_can_poll);
-
 	/* Shared properties of all M_CAN versions */
 	cdev->version = m_can_version;
 	cdev->can.do_set_mode = m_can_set_mode;
@@ -1846,11 +1791,10 @@ static int m_can_close(struct net_device *dev)
 	if (cdev->is_peripheral) {
 		destroy_workqueue(cdev->tx_wq);
 		cdev->tx_wq = NULL;
-		can_rx_offload_disable(&cdev->offload);
-	} else {
-		napi_disable(&cdev->napi);
 	}
 
+	can_rx_offload_disable(&cdev->offload);
+
 	close_candev(dev);
 
 	reset_control_assert(cdev->rst);
@@ -2069,8 +2013,8 @@ static enum hrtimer_restart m_can_polling_timer(struct hrtimer *timer)
 
 	ret = m_can_interrupt_handler(cdev);
 
-	/* On error or if napi is scheduled to read, stop the timer */
-	if (ret < 0 || napi_is_scheduled(&cdev->napi))
+	/* On error stop the timer */
+	if (ret < 0)
 		return HRTIMER_NORESTART;
 
 	hrtimer_forward_now(timer, ms_to_ktime(HRTIMER_POLL_INTERVAL_MS));
@@ -2102,10 +2046,7 @@ static int m_can_open(struct net_device *dev)
 		goto out_reset_control_assert;
 	}
 
-	if (cdev->is_peripheral)
-		can_rx_offload_enable(&cdev->offload);
-	else
-		napi_enable(&cdev->napi);
+	can_rx_offload_enable(&cdev->offload);
 
 	/* register interrupt handler */
 	if (cdev->is_peripheral) {
@@ -2144,16 +2085,13 @@ static int m_can_open(struct net_device *dev)
 	return 0;
 
 exit_start_fail:
-	if (cdev->is_peripheral || dev->irq)
+	if (dev->irq)
 		free_irq(dev->irq, dev);
 exit_irq_fail:
 	if (cdev->is_peripheral)
 		destroy_workqueue(cdev->tx_wq);
 out_wq_fail:
-	if (cdev->is_peripheral)
-		can_rx_offload_disable(&cdev->offload);
-	else
-		napi_disable(&cdev->napi);
+	can_rx_offload_disable(&cdev->offload);
 	close_candev(dev);
 out_reset_control_assert:
 	reset_control_assert(cdev->rst);
@@ -2533,12 +2471,10 @@ int m_can_class_register(struct m_can_classdev *cdev)
 	if (ret)
 		goto clk_disable;
 
-	if (cdev->is_peripheral) {
-		ret = can_rx_offload_add_manual(cdev->net, &cdev->offload,
-						NAPI_POLL_WEIGHT);
-		if (ret)
-			goto out_reset_control_assert;
-	}
+	ret = can_rx_offload_add_manual(cdev->net, &cdev->offload,
+					NAPI_POLL_WEIGHT);
+	if (ret)
+		goto out_reset_control_assert;
 
 	if (!cdev->net->irq) {
 		netdev_dbg(cdev->net, "Polling enabled, initialize hrtimer");
@@ -2575,8 +2511,7 @@ int m_can_class_register(struct m_can_classdev *cdev)
 	return 0;
 
 rx_offload_del:
-	if (cdev->is_peripheral)
-		can_rx_offload_del(&cdev->offload);
+	can_rx_offload_del(&cdev->offload);
 out_reset_control_assert:
 	reset_control_assert(cdev->rst);
 clk_disable:
@@ -2589,8 +2524,7 @@ EXPORT_SYMBOL_GPL(m_can_class_register);
 void m_can_class_unregister(struct m_can_classdev *cdev)
 {
 	unregister_candev(cdev->net);
-	if (cdev->is_peripheral)
-		can_rx_offload_del(&cdev->offload);
+	can_rx_offload_del(&cdev->offload);
 }
 EXPORT_SYMBOL_GPL(m_can_class_unregister);
 
diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
index 4743342b2fba..ca5833650dbf 100644
--- a/drivers/net/can/m_can/m_can.h
+++ b/drivers/net/can/m_can/m_can.h
@@ -96,7 +96,6 @@ struct m_can_classdev {
 	const struct m_can_ops *ops;
 
 	int version;
-	u32 irqstatus;
 
 	int pm_clock_support;
 	int pm_wake_source;

---
base-commit: 08030ddb87b4c6c6a2c03c82731b5e188f02f5b9
change-id: 20260709-m_can-rx-offload-a228500d9f3a

Best regards,
--  
Marc Kleine-Budde <mkl@pengutronix.de>

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [High] Double counting of received packets and bytes, alongside a data race on network statistics between HardIRQ and SoftIRQ contexts.
- [High] Processing RX frames directly in hardirq with a strict, non-replenishable quota can leave packets stranded indefinitely in the hardware FIFO.
- [Low] The napi_struct is no longer used but was left in struct m_can_classdev.

Pre-existing issues:
- [High] The driver teardown path uses the non-blocking `hrtimer_try_to_cancel()` to stop the polling/coalescing timer, leading to unclocked hardware accesses and Use-After-Free vulnerabilities.
- [High] The teardown sequence in m_can_close leaks socket buffers and creates a guaranteed NULL pointer dereference when flushing the transmission workqueue.
- [High] Uninitialized stack variable `frame_len` corrupts Byte Queue Limits (BQL) accounting.
--

--- Patch [1]: [PATCH can-next] can: m_can: switch to rx-offload implementation ---
commit 7f375f1ac288dad1360ddf2dd262a4c708abc626
Author: Marc Kleine-Budde <mkl@pengutronix.de>

can: m_can: switch to rx-offload implementation

This patch converts the m_can driver to unconditionally use rx-offload
for all devices, calling m_can_rx_handler() directly from the IRQ
handler instead of using NAPI.

> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index eb856547ae7df..8175b030257b4 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)
[ ... ]
>  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);

[Severity: High]
Does this change introduce a data race and double counting for network stats?

When the mmio devices are switched to use rx-offload, m_can_read_fifo()
manually increments the stats in hardirq context before calling
m_can_receive_skb():

m_can_read_fifo()
  stats->rx_bytes += cf->len;
  stats->rx_packets++;
  m_can_receive_skb(..., skb, ...);

The SKB is then queued to the offload queue. When the rx-offload NAPI poll
runs in softirq context, it unconditionally increments the exact same
counters again:

can_rx_offload_napi_poll()
  stats->rx_packets++;
  stats->rx_bytes += cf->len;

Does this completely inflate the RX statistics, as well as cause a concurrent
data race between hardirq and softirq contexts updating dev->stats?

Additionally, wouldn't TX echo packets queued via
can_rx_offload_get_echo_skb_queue_timestamp() also be dequeued by the poll
routine, erroneously counting transmitted frames as RX packets?

[ ... ]
> @@ -1065,32 +1052,6 @@ static int m_can_rx_handler(struct net_device *dev, int quota, u32 irqstatus)
[ ... ]
>  static unsigned int m_can_tx_update_stats(struct m_can_classdev *cdev,
>  					  unsigned int msg_mark, u32 timestamp)
>  {
> @@ -1098,14 +1059,11 @@ static unsigned int m_can_tx_update_stats(struct m_can_classdev *cdev,
>  	struct net_device_stats *stats = &dev->stats;
>  	unsigned int frame_len;
>  
> -	if (cdev->is_peripheral)
> -		stats->tx_bytes +=
> -			can_rx_offload_get_echo_skb_queue_timestamp(&cdev->offload,
> -								    msg_mark,
> -								    timestamp,
> -								    &frame_len);
> -	else
> -		stats->tx_bytes += can_get_echo_skb(dev, msg_mark, &frame_len);
> +	stats->tx_bytes +=
> +		can_rx_offload_get_echo_skb_queue_timestamp(&cdev->offload,
> +							    msg_mark,
> +							    timestamp,
> +							    &frame_len);
>  
>  	stats->tx_packets++;
>  
>  	return frame_len;

[Severity: High]
This is a pre-existing issue, but is it possible for an uninitialized stack
variable to corrupt BQL accounting?

In m_can_tx_update_stats(), frame_len is passed by reference to
can_rx_offload_get_echo_skb_queue_timestamp(). If the echo SKB is missing
or invalid, the helper function returns 0 immediately and does not initialize
frame_len.

Will this return uninitialized stack garbage that gets passed to
netdev_completed_queue() in m_can_finish_tx(), potentially stalling the TX
queue by corrupting Byte Queue Limits accounting?

[ ... ]
> @@ -1265,21 +1223,10 @@ static int m_can_interrupt_handler(struct m_can_classdev *cdev)
[ ... ]
>  	if (ir & (IR_RF0N | IR_RF0W | IR_ERR_ALL_30X)) {
> -		cdev->irqstatus = ir;
> -		if (!cdev->is_peripheral) {
> -			m_can_disable_all_interrupts(cdev);
> -			napi_schedule(&cdev->napi);
> -		} else {
> -			ret = m_can_rx_handler(dev, NAPI_POLL_WEIGHT, ir);
> -			if (ret < 0)
> -				return ret;
> -		}
> +		ret = m_can_rx_handler(dev, NAPI_POLL_WEIGHT, ir);
> +		if (ret < 0)
> +			return ret;
>  	}

[Severity: High]
Could directly calling m_can_rx_handler() from the hardirq handler cause
packets to be stranded indefinitely?

The handler is now called with a strict quota of NAPI_POLL_WEIGHT. If the FIFO
contains more messages than the remaining quota (which can shrink if work_done
is consumed by state errors), the read loop in m_can_do_rx_poll() breaks
early.

Because the IR_RF0N hardware interrupt is edge-triggered (only set when a new
message is written) and the NAPI automatic rescheduling is removed, will the
remaining packets sit in the hardware FIFO indefinitely until another frame
happens to arrive on the bus?

[ ... ]
> @@ -1846,11 +1791,10 @@ static int m_can_close(struct net_device *dev)
>  	if (cdev->is_peripheral) {
>  		destroy_workqueue(cdev->tx_wq);
>  		cdev->tx_wq = NULL;
> -		can_rx_offload_disable(&cdev->offload);
> -	} else {
> -		napi_disable(&cdev->napi);
>  	}

[Severity: High]
This is a pre-existing issue, but can the teardown sequence in m_can_close()
trigger a null pointer dereference?

During m_can_close(), m_can_clean() iterates through the tx_ops and sets
the skb pointers to NULL:

m_can_clean()
  cdev->tx_ops[i].skb = NULL;

Immediately after, destroy_workqueue(cdev->tx_wq) is called, which drains the
queue and executes pending tasks. When the flushed m_can_tx_work_queue() task
runs, it passes the nullified skb pointer to the tx handler:

m_can_tx_work_queue()
  struct sk_buff *skb = op->skb;
  m_can_tx_handler(cdev, skb);

Will this result in a null pointer dereference since the tx handler
unconditionally accesses skb->data? Also, is there a memory leak since
m_can_clean() clears the pointer without freeing the SKB memory?

[Severity: High]
This is a pre-existing issue, but does the teardown path safely stop the
polling timer?

Looking at the teardown sequence in m_can_close(), the interface is shut
down and interrupts are disabled, which relies on hrtimer_try_to_cancel() in
m_can_disable_all_interrupts():

m_can_close()
  m_can_stop()
    m_can_disable_all_interrupts()
      hrtimer_try_to_cancel(&cdev->hrtimer);

If the timer callback is actively running on another CPU,
hrtimer_try_to_cancel() returns -1 without waiting for the callback to finish.
The teardown path then continues to free data structures, disable hardware
clocks, and power off the PHY.

Could this lead to a use-after-free and synchronous bus fault when the
concurrently executing timer callback accesses unmapped or unclocked MMIO
regions?

Also, m_can_class_unregister() appears to lack explicit timer teardown, which
might leave the timer active during module unload.

[ ... ]
> diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
> index 4743342b2fba3..ca5833650dbfb 100644
> --- a/drivers/net/can/m_can/m_can.h
> +++ b/drivers/net/can/m_can/m_can.h
> @@ -96,7 +96,6 @@ struct m_can_classdev {
>  	const struct m_can_ops *ops;
>  
>  	int version;
> -	u32 irqstatus;
>  
>  	int pm_clock_support;
>  	int pm_wake_source;

[Severity: Low]
Is it intentional to keep the napi structure in m_can_classdev?

The patch removes all manual napi handling from the driver, but it looks like
struct napi_struct napi; is still present in the struct.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-m_can-rx-offload-v1-1-af3efa8e4272@pengutronix.de?part=1