[PATCH v7 0/4] can: rx-offload: allow more than one receive source

[PATCH v7 0/4] can: rx-offload: allow more than one receive source

[Ciprian Costea]

From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>

On S32G2, flexcan uses the same IRQ handler for all four interrupt
lines, so the handlers can run concurrently on different CPUs. They all
fill the same lockless skb_irq_queue, which can get corrupted.

This series gives each IRQ line its own rx-offload source with its own
skb_irq_queue. struct can_rx_offload now describes one source, and the
skb_queue and NAPI shared by all sources move into the new struct
can_rx_offload_queue. The primary source is added as before and embeds
the shared queue. Extra sources are attached to it with
can_rx_offload_add_source().

The IRQ side stays lockless, as each source has a single producer. The
finish helpers move the skbs into the shared skb_queue under its lock.
With more than one source in timestamp mode they are sorted in, so the
timestamp order is kept across IRQ lines.

The rx-offload API is unchanged, so only flexcan is converted.

Patch 1 is an independent at91_can fix. Patch 2 moves skb_queue and napi
into struct can_rx_offload_queue, with no functional change. Patch 3
adds support for more than one source, and patch 4 uses it in flexcan.

Tested on S32G274A-RDB2 (can0 <-> can1, CAN FD + classic, IRQ lines pinned
to different CPUs).

Changes since v6:

- Moved skb_queue and napi into struct can_rx_offload_queue instead of
  adding struct can_rx_offload_irq, so other drivers are not touched
  (Haibo).
- Renamed "per-IRQ context" to "source", added
  can_rx_offload_add_source() and reworked the comments (Max).
- Split the rx-offload change into two patches.
- at91_can: add Acked-by: Max Staudt <max@enpas.org>

Changes since v5:

- Replaced the per-CPU skb_irq_queue, which does not work in preemptible
  context, with a per-IRQ receive context, as suggested by Marc Kleine-Budde.
- Added patch 3 with the flexcan conversion. The bus off and error lines get
  their own context too, not only the mailbox lines.
- Moved the at91_can fix to its own patch.
- Dropped the gs_usb can_rx_offload_add_manual() return value check, the
  NULL pointer dereference it guarded against went away with the per-CPU
  allocation.
- Dropped Haibo's Reviewed-by from the rx-offload patch, since it was
  rewritten.
- Added Assisted-by tags.

Changes since v4:
- rx-offload: expand the comment above the for_each_possible_cpu() loop
  in can_rx_offload_threaded_irq_finish() to add the single-producer
  assumption (IRQ requested with IRQF_ONESHOT / handler non-reentrant).
  Suggested by Haibo Chen.
- rx-offload: add Reviewed-by: Haibo Chen <haibo.chen@nxp.com>

Changes since v3:

- In gs_usb driver, check the can_rx_offload_add_manual() return value,
  the same NULL-deref the per-CPU change exposes.

Changes since v2:

- at91_can: also add can_rx_offload_del() on the register_candev() error
  path and check the can_rx_offload_add_timestamp() return value.

Changes since v1:

- The enqueue helpers used this_cpu_ptr() without disabling preemption.
  All four enqueue helpers now use get_cpu_ptr()/put_cpu_ptr().
- Guard can_rx_offload_del() against skb_irq_queue == NULL.
- Fix 'at91_can' memory leak by adding missing 'can_rx_offload_del'.

Ciprian Marian Costea (4):
  can: at91_can: release the rx-offload on teardown
  can: rx-offload: move skb_queue and napi into struct
    can_rx_offload_queue
  can: rx-offload: allow more than one receive source
  can: flexcan: use one rx-offload source per IRQ line

 drivers/net/can/at91_can.c             |  12 +-
 drivers/net/can/dev/rx-offload.c       | 185 +++++++++++++++++++------
 drivers/net/can/flexcan/flexcan-core.c |  86 +++++++++---
 drivers/net/can/flexcan/flexcan.h      |   3 +
 include/linux/can/rx-offload.h         |  27 +++-
 5 files changed, 252 insertions(+), 61 deletions(-)

-- 
2.43.0

[PATCH v7 1/4] can: at91_can: release the rx-offload on teardown

[Ciprian Costea]

From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>

at91_can_probe() ignores the return value of can_rx_offload_add_timestamp()
and can_rx_offload_del() is never called, neither on the register_candev()
error path nor in at91_can_remove(). Any skbs left in the offload queues
are leaked.

Check the return value and call can_rx_offload_del() on both paths.

Fixes: 137f59d5dab4 ("can: at91_can: switch to rx-offload implementation")
Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
Acked-by: Max Staudt <max@enpas.org>
---
 drivers/net/can/at91_can.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/at91_can.c b/drivers/net/can/at91_can.c
index 58da323f14d7..09aa676a03fb 100644
--- a/drivers/net/can/at91_can.c
+++ b/drivers/net/can/at91_can.c
@@ -1123,7 +1123,11 @@ static int at91_can_probe(struct platform_device *pdev)
 	priv->offload.mb_first = devtype_data->rx_first;
 	priv->offload.mb_last = devtype_data->rx_last;
 
-	can_rx_offload_add_timestamp(dev, &priv->offload);
+	err = can_rx_offload_add_timestamp(dev, &priv->offload);
+	if (err) {
+		dev_err(&pdev->dev, "can_rx_offload_add_timestamp() failed\n");
+		goto exit_free;
+	}
 
 	if (transceiver)
 		priv->can.bitrate_max = transceiver->attrs.max_link_rate;
@@ -1137,7 +1141,7 @@ static int at91_can_probe(struct platform_device *pdev)
 	err = register_candev(dev);
 	if (err) {
 		dev_err(&pdev->dev, "registering netdev failed\n");
-		goto exit_free;
+		goto exit_offload;
 	}
 
 	dev_info(&pdev->dev, "device registered (reg_base=%p, irq=%d)\n",
@@ -1145,6 +1149,8 @@ static int at91_can_probe(struct platform_device *pdev)
 
 	return 0;
 
+ exit_offload:
+	can_rx_offload_del(&priv->offload);
  exit_free:
 	free_candev(dev);
  exit_iounmap:
@@ -1165,6 +1171,8 @@ static void at91_can_remove(struct platform_device *pdev)
 
 	unregister_netdev(dev);
 
+	can_rx_offload_del(&priv->offload);
+
 	iounmap(priv->reg_base);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-- 
2.43.0

[PATCH v7 2/4] can: rx-offload: move skb_queue and napi into struct can_rx_offload_queue

[Ciprian Costea]

From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>

Currently, struct can_rx_offload holds both the state filled by the
driver's IRQ handler (skb_irq_queue, mailbox range) and the state it
feeds (skb_queue, napi). To let more than one source of skbs feed the
same skb_queue and NAPI, the latter has to be separate from the
former.

Move skb_queue and napi into a new struct can_rx_offload_queue. For now
every struct can_rx_offload uses its own embedded instance, reached
through the queue pointer.

The NAPI poll function gets the netdev from napi->dev.

No functional change. The rx-offload API and its users are unchanged.

Suggested-by: Haibo Chen <haibo.chen@nxp.com>
Assisted-by: LLM
Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
---
 drivers/net/can/dev/rx-offload.c | 58 +++++++++++++++++---------------
 include/linux/can/rx-offload.h   | 14 +++++---
 2 files changed, 41 insertions(+), 31 deletions(-)

diff --git a/drivers/net/can/dev/rx-offload.c b/drivers/net/can/dev/rx-offload.c
index 46e7b6db4a1e..92309ed47bac 100644
--- a/drivers/net/can/dev/rx-offload.c
+++ b/drivers/net/can/dev/rx-offload.c
@@ -41,16 +41,15 @@ can_rx_offload_inc(struct can_rx_offload *offload, unsigned int *val)
 
 static int can_rx_offload_napi_poll(struct napi_struct *napi, int quota)
 {
-	struct can_rx_offload *offload = container_of(napi,
-						      struct can_rx_offload,
-						      napi);
-	struct net_device *dev = offload->dev;
-	struct net_device_stats *stats = &dev->stats;
+	struct can_rx_offload_queue *queue = container_of(napi,
+							  struct can_rx_offload_queue,
+							  napi);
+	struct net_device_stats *stats = &napi->dev->stats;
 	struct sk_buff *skb;
 	int work_done = 0;
 
 	while ((work_done < quota) &&
-	       (skb = skb_dequeue(&offload->skb_queue))) {
+	       (skb = skb_dequeue(&queue->skb_queue))) {
 		struct can_frame *cf = (struct can_frame *)skb->data;
 
 		work_done++;
@@ -66,8 +65,8 @@ static int can_rx_offload_napi_poll(struct napi_struct *napi, int quota)
 		napi_complete_done(napi, work_done);
 
 		/* Check if there was another interrupt */
-		if (!skb_queue_empty(&offload->skb_queue))
-			napi_schedule(&offload->napi);
+		if (!skb_queue_empty(&queue->skb_queue))
+			napi_schedule(&queue->napi);
 	}
 
 	return work_done;
@@ -125,7 +124,7 @@ static int can_rx_offload_compare(struct sk_buff *a, struct sk_buff *b)
  * from the device and return the mailbox's content as a struct
  * sk_buff.
  *
- * If the struct can_rx_offload::skb_queue exceeds the maximal queue
+ * If the struct can_rx_offload_queue::skb_queue exceeds the maximal queue
  * length (struct can_rx_offload::skb_queue_len_max) or no skb can be
  * allocated, the mailbox contents is discarded by reading it into an
  * overflow buffer. This way the mailbox is marked as free by the
@@ -146,7 +145,7 @@ can_rx_offload_offload_one(struct can_rx_offload *offload, unsigned int n)
 	u32 timestamp;
 
 	/* If queue is full drop frame */
-	if (unlikely(skb_queue_len(&offload->skb_queue) >
+	if (unlikely(skb_queue_len(&offload->queue->skb_queue) >
 		     offload->skb_queue_len_max))
 		drop = true;
 
@@ -224,7 +223,7 @@ int can_rx_offload_queue_timestamp(struct can_rx_offload *offload,
 {
 	struct can_rx_offload_cb *cb;
 
-	if (skb_queue_len(&offload->skb_queue) >
+	if (skb_queue_len(&offload->queue->skb_queue) >
 	    offload->skb_queue_len_max) {
 		dev_kfree_skb_any(skb);
 		return -ENOBUFS;
@@ -268,7 +267,7 @@ EXPORT_SYMBOL_GPL(can_rx_offload_get_echo_skb_queue_timestamp);
 int can_rx_offload_queue_tail(struct can_rx_offload *offload,
 			      struct sk_buff *skb)
 {
-	if (skb_queue_len(&offload->skb_queue) >
+	if (skb_queue_len(&offload->queue->skb_queue) >
 	    offload->skb_queue_len_max) {
 		dev_kfree_skb_any(skb);
 		return -ENOBUFS;
@@ -307,44 +306,46 @@ EXPORT_SYMBOL_GPL(can_rx_offload_get_echo_skb_queue_tail);
 
 void can_rx_offload_irq_finish(struct can_rx_offload *offload)
 {
+	struct can_rx_offload_queue *queue = offload->queue;
 	unsigned long flags;
 	int queue_len;
 
 	if (skb_queue_empty_lockless(&offload->skb_irq_queue))
 		return;
 
-	spin_lock_irqsave(&offload->skb_queue.lock, flags);
-	skb_queue_splice_tail_init(&offload->skb_irq_queue, &offload->skb_queue);
-	spin_unlock_irqrestore(&offload->skb_queue.lock, flags);
+	spin_lock_irqsave(&queue->skb_queue.lock, flags);
+	skb_queue_splice_tail_init(&offload->skb_irq_queue, &queue->skb_queue);
+	spin_unlock_irqrestore(&queue->skb_queue.lock, flags);
 
-	queue_len = skb_queue_len(&offload->skb_queue);
+	queue_len = skb_queue_len(&queue->skb_queue);
 	if (queue_len > offload->skb_queue_len_max / 8)
 		netdev_dbg(offload->dev, "%s: queue_len=%d\n",
 			   __func__, queue_len);
 
-	napi_schedule(&offload->napi);
+	napi_schedule(&queue->napi);
 }
 EXPORT_SYMBOL_GPL(can_rx_offload_irq_finish);
 
 void can_rx_offload_threaded_irq_finish(struct can_rx_offload *offload)
 {
+	struct can_rx_offload_queue *queue = offload->queue;
 	unsigned long flags;
 	int queue_len;
 
 	if (skb_queue_empty_lockless(&offload->skb_irq_queue))
 		return;
 
-	spin_lock_irqsave(&offload->skb_queue.lock, flags);
-	skb_queue_splice_tail_init(&offload->skb_irq_queue, &offload->skb_queue);
-	spin_unlock_irqrestore(&offload->skb_queue.lock, flags);
+	spin_lock_irqsave(&queue->skb_queue.lock, flags);
+	skb_queue_splice_tail_init(&offload->skb_irq_queue, &queue->skb_queue);
+	spin_unlock_irqrestore(&queue->skb_queue.lock, flags);
 
-	queue_len = skb_queue_len(&offload->skb_queue);
+	queue_len = skb_queue_len(&queue->skb_queue);
 	if (queue_len > offload->skb_queue_len_max / 8)
 		netdev_dbg(offload->dev, "%s: queue_len=%d\n",
 			   __func__, queue_len);
 
 	local_bh_disable();
-	napi_schedule(&offload->napi);
+	napi_schedule(&queue->napi);
 	local_bh_enable();
 }
 EXPORT_SYMBOL_GPL(can_rx_offload_threaded_irq_finish);
@@ -353,15 +354,18 @@ static int can_rx_offload_init_queue(struct net_device *dev,
 				     struct can_rx_offload *offload,
 				     unsigned int weight)
 {
+	struct can_rx_offload_queue *queue = &offload->own_queue;
+
 	offload->dev = dev;
+	offload->queue = queue;
 
 	/* Limit queue len to 4x the weight (rounded to next power of two) */
 	offload->skb_queue_len_max = 2 << fls(weight);
 	offload->skb_queue_len_max *= 4;
-	skb_queue_head_init(&offload->skb_queue);
+	skb_queue_head_init(&queue->skb_queue);
 	__skb_queue_head_init(&offload->skb_irq_queue);
 
-	netif_napi_add_weight(dev, &offload->napi, can_rx_offload_napi_poll,
+	netif_napi_add_weight(dev, &queue->napi, can_rx_offload_napi_poll,
 			      weight);
 
 	dev_dbg(dev->dev.parent, "%s: skb_queue_len_max=%d\n",
@@ -414,14 +418,14 @@ EXPORT_SYMBOL_GPL(can_rx_offload_add_manual);
 
 void can_rx_offload_enable(struct can_rx_offload *offload)
 {
-	napi_enable(&offload->napi);
+	napi_enable(&offload->queue->napi);
 }
 EXPORT_SYMBOL_GPL(can_rx_offload_enable);
 
 void can_rx_offload_del(struct can_rx_offload *offload)
 {
-	netif_napi_del(&offload->napi);
-	skb_queue_purge(&offload->skb_queue);
+	netif_napi_del(&offload->queue->napi);
+	skb_queue_purge(&offload->queue->skb_queue);
 	__skb_queue_purge(&offload->skb_irq_queue);
 }
 EXPORT_SYMBOL_GPL(can_rx_offload_del);
diff --git a/include/linux/can/rx-offload.h b/include/linux/can/rx-offload.h
index d29bb4521947..fafc00fc3700 100644
--- a/include/linux/can/rx-offload.h
+++ b/include/linux/can/rx-offload.h
@@ -12,6 +12,11 @@
 #include <linux/netdevice.h>
 #include <linux/can.h>
 
+struct can_rx_offload_queue {
+	struct sk_buff_head skb_queue;
+	struct napi_struct napi;
+};
+
 struct can_rx_offload {
 	struct net_device *dev;
 
@@ -19,16 +24,17 @@ struct can_rx_offload {
 					unsigned int mb, u32 *timestamp,
 					bool drop);
 
-	struct sk_buff_head skb_queue;
 	struct sk_buff_head skb_irq_queue;
 	u32 skb_queue_len_max;
 
 	unsigned int mb_first;
 	unsigned int mb_last;
 
-	struct napi_struct napi;
-
 	bool inc;
+
+	/* Points to own_queue. */
+	struct can_rx_offload_queue *queue;
+	struct can_rx_offload_queue own_queue;
 };
 
 int can_rx_offload_add_timestamp(struct net_device *dev,
@@ -59,7 +65,7 @@ void can_rx_offload_enable(struct can_rx_offload *offload);
 
 static inline void can_rx_offload_disable(struct can_rx_offload *offload)
 {
-	napi_disable(&offload->napi);
+	napi_disable(&offload->queue->napi);
 }
 
 #endif /* !_CAN_RX_OFFLOAD_H */
-- 
2.43.0

[PATCH v7 3/4] can: rx-offload: allow more than one receive source

[Ciprian Costea]

From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>

Currently, the IRQ handler fills skb_irq_queue without a lock and the
finish helpers then splice it into skb_queue under skb_queue.lock. This
only works with a single producer. It breaks when a driver uses the
helpers from more than one IRQ line: on NXP S32G2 the flexcan handlers
can run at the same time on different CPUs and corrupt skb_irq_queue.

Let each producer have its own struct can_rx_offload, called a source,
while all sources of a device share one struct can_rx_offload_queue.

The source registered with can_rx_offload_add_timestamp(),
can_rx_offload_add_fifo() or can_rx_offload_add_manual() is the primary
source and owns the queue, as before. The new can_rx_offload_add_source()
attaches another source to the primary's queue. That source inherits the
mode, netdev, queue length limit and mailbox_read() of the primary, and
in timestamp mode scans its own mailbox range. can_rx_offload_del() on
the primary removes all sources, so they can be added again on the next
open.

With a single source the finish helpers still splice skb_irq_queue into
skb_queue. With more than one source in timestamp mode they merge it
into skb_queue sorted by timestamp. Otherwise they append it.

The existing API is unchanged, so drivers with a single producer need no
change.

Suggested-by: Marc Kleine-Budde <mkl@pengutronix.de>
Suggested-by: Haibo Chen <haibo.chen@nxp.com>
Assisted-by: LLM
Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
---
 drivers/net/can/dev/rx-offload.c | 145 ++++++++++++++++++++++++++-----
 include/linux/can/rx-offload.h   |  15 +++-
 2 files changed, 138 insertions(+), 22 deletions(-)

diff --git a/drivers/net/can/dev/rx-offload.c b/drivers/net/can/dev/rx-offload.c
index 92309ed47bac..5352d70b6e93 100644
--- a/drivers/net/can/dev/rx-offload.c
+++ b/drivers/net/can/dev/rx-offload.c
@@ -304,18 +304,40 @@ can_rx_offload_get_echo_skb_queue_tail(struct can_rx_offload *offload,
 }
 EXPORT_SYMBOL_GPL(can_rx_offload_get_echo_skb_queue_tail);
 
-void can_rx_offload_irq_finish(struct can_rx_offload *offload)
+/* Sources may run on different CPUs at the same time, skb_queue.lock
+ * serializes them.
+ */
+static void can_rx_offload_move_to_skb_queue(struct can_rx_offload *offload)
 {
 	struct can_rx_offload_queue *queue = offload->queue;
 	unsigned long flags;
+
+	spin_lock_irqsave(&queue->skb_queue.lock, flags);
+
+	if (queue->source_cnt > 1 && queue->sort) {
+		/* keep the timestamp order over all sources */
+		struct sk_buff *skb;
+
+		while ((skb = __skb_dequeue(&offload->skb_irq_queue)))
+			__skb_queue_add_sort(&queue->skb_queue, skb,
+					     can_rx_offload_compare);
+	} else {
+		skb_queue_splice_tail_init(&offload->skb_irq_queue,
+					   &queue->skb_queue);
+	}
+
+	spin_unlock_irqrestore(&queue->skb_queue.lock, flags);
+}
+
+void can_rx_offload_irq_finish(struct can_rx_offload *offload)
+{
+	struct can_rx_offload_queue *queue = offload->queue;
 	int queue_len;
 
 	if (skb_queue_empty_lockless(&offload->skb_irq_queue))
 		return;
 
-	spin_lock_irqsave(&queue->skb_queue.lock, flags);
-	skb_queue_splice_tail_init(&offload->skb_irq_queue, &queue->skb_queue);
-	spin_unlock_irqrestore(&queue->skb_queue.lock, flags);
+	can_rx_offload_move_to_skb_queue(offload);
 
 	queue_len = skb_queue_len(&queue->skb_queue);
 	if (queue_len > offload->skb_queue_len_max / 8)
@@ -329,15 +351,12 @@ EXPORT_SYMBOL_GPL(can_rx_offload_irq_finish);
 void can_rx_offload_threaded_irq_finish(struct can_rx_offload *offload)
 {
 	struct can_rx_offload_queue *queue = offload->queue;
-	unsigned long flags;
 	int queue_len;
 
 	if (skb_queue_empty_lockless(&offload->skb_irq_queue))
 		return;
 
-	spin_lock_irqsave(&queue->skb_queue.lock, flags);
-	skb_queue_splice_tail_init(&offload->skb_irq_queue, &queue->skb_queue);
-	spin_unlock_irqrestore(&queue->skb_queue.lock, flags);
+	can_rx_offload_move_to_skb_queue(offload);
 
 	queue_len = skb_queue_len(&queue->skb_queue);
 	if (queue_len > offload->skb_queue_len_max / 8)
@@ -350,6 +369,16 @@ void can_rx_offload_threaded_irq_finish(struct can_rx_offload *offload)
 }
 EXPORT_SYMBOL_GPL(can_rx_offload_threaded_irq_finish);
 
+static void can_rx_offload_link_source(struct can_rx_offload_queue *queue,
+				       struct can_rx_offload *offload)
+{
+	offload->queue = queue;
+	__skb_queue_head_init(&offload->skb_irq_queue);
+
+	list_add_tail(&offload->node, &queue->sources);
+	queue->source_cnt++;
+}
+
 static int can_rx_offload_init_queue(struct net_device *dev,
 				     struct can_rx_offload *offload,
 				     unsigned int weight)
@@ -357,41 +386,65 @@ static int can_rx_offload_init_queue(struct net_device *dev,
 	struct can_rx_offload_queue *queue = &offload->own_queue;
 
 	offload->dev = dev;
-	offload->queue = queue;
 
 	/* Limit queue len to 4x the weight (rounded to next power of two) */
 	offload->skb_queue_len_max = 2 << fls(weight);
 	offload->skb_queue_len_max *= 4;
 	skb_queue_head_init(&queue->skb_queue);
-	__skb_queue_head_init(&offload->skb_irq_queue);
+
+	INIT_LIST_HEAD(&queue->sources);
+	queue->source_cnt = 0;
+	queue->sort = false;
 
 	netif_napi_add_weight(dev, &queue->napi, can_rx_offload_napi_poll,
 			      weight);
 
+	can_rx_offload_link_source(queue, offload);
+
 	dev_dbg(dev->dev.parent, "%s: skb_queue_len_max=%d\n",
 		__func__, offload->skb_queue_len_max);
 
 	return 0;
 }
 
-int can_rx_offload_add_timestamp(struct net_device *dev,
-				 struct can_rx_offload *offload)
+static int can_rx_offload_init_mb_range(struct can_rx_offload *offload,
+					unsigned int *weight)
 {
-	unsigned int weight;
-
 	if (offload->mb_first > BITS_PER_LONG_LONG ||
-	    offload->mb_last > BITS_PER_LONG_LONG || !offload->mailbox_read)
+	    offload->mb_last > BITS_PER_LONG_LONG)
 		return -EINVAL;
 
 	if (offload->mb_first < offload->mb_last) {
 		offload->inc = true;
-		weight = offload->mb_last - offload->mb_first;
+		*weight = offload->mb_last - offload->mb_first;
 	} else {
 		offload->inc = false;
-		weight = offload->mb_first - offload->mb_last;
+		*weight = offload->mb_first - offload->mb_last;
 	}
 
-	return can_rx_offload_init_queue(dev, offload, weight);
+	return 0;
+}
+
+int can_rx_offload_add_timestamp(struct net_device *dev,
+				 struct can_rx_offload *offload)
+{
+	unsigned int weight;
+	int err;
+
+	if (!offload->mailbox_read)
+		return -EINVAL;
+
+	err = can_rx_offload_init_mb_range(offload, &weight);
+	if (err)
+		return err;
+
+	err = can_rx_offload_init_queue(dev, offload, weight);
+	if (err)
+		return err;
+
+	offload->queue->sort = true;
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(can_rx_offload_add_timestamp);
 
@@ -416,6 +469,46 @@ int can_rx_offload_add_manual(struct net_device *dev,
 }
 EXPORT_SYMBOL_GPL(can_rx_offload_add_manual);
 
+/**
+ * can_rx_offload_add_source() - Add a source to the queue of @primary
+ * @primary: source added with can_rx_offload_add_timestamp(),
+ *	     can_rx_offload_add_fifo() or can_rx_offload_add_manual()
+ * @source: source to add
+ *
+ * @source inherits the mode, netdev, queue length limit and mailbox_read()
+ * of @primary. In timestamp mode the driver must set the mailbox range of
+ * @source first. mailbox_read() is called with @source, so it must use
+ * netdev_priv(offload->dev) and not container_of() to get its private data.
+ *
+ * Return: 0 on success, -EINVAL on invalid arguments.
+ */
+int can_rx_offload_add_source(struct can_rx_offload *primary,
+			      struct can_rx_offload *source)
+{
+	struct can_rx_offload_queue *queue = primary->queue;
+	unsigned int weight;
+	int err;
+
+	if (queue != &primary->own_queue || !queue->source_cnt ||
+	    source == primary)
+		return -EINVAL;
+
+	if (queue->sort) {
+		err = can_rx_offload_init_mb_range(source, &weight);
+		if (err)
+			return err;
+	}
+
+	source->dev = primary->dev;
+	source->mailbox_read = primary->mailbox_read;
+	source->skb_queue_len_max = primary->skb_queue_len_max;
+
+	can_rx_offload_link_source(queue, source);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(can_rx_offload_add_source);
+
 void can_rx_offload_enable(struct can_rx_offload *offload)
 {
 	napi_enable(&offload->queue->napi);
@@ -424,8 +517,18 @@ EXPORT_SYMBOL_GPL(can_rx_offload_enable);
 
 void can_rx_offload_del(struct can_rx_offload *offload)
 {
-	netif_napi_del(&offload->queue->napi);
-	skb_queue_purge(&offload->queue->skb_queue);
-	__skb_queue_purge(&offload->skb_irq_queue);
+	struct can_rx_offload_queue *queue = offload->queue;
+	struct can_rx_offload *source, *tmp;
+
+	netif_napi_del(&queue->napi);
+	skb_queue_purge(&queue->skb_queue);
+
+	/* the sources are added again on the next open */
+	list_for_each_entry_safe(source, tmp, &queue->sources, node) {
+		__skb_queue_purge(&source->skb_irq_queue);
+		list_del_init(&source->node);
+	}
+	queue->source_cnt = 0;
+	queue->sort = false;
 }
 EXPORT_SYMBOL_GPL(can_rx_offload_del);
diff --git a/include/linux/can/rx-offload.h b/include/linux/can/rx-offload.h
index fafc00fc3700..06c2fb71cf7b 100644
--- a/include/linux/can/rx-offload.h
+++ b/include/linux/can/rx-offload.h
@@ -15,8 +15,16 @@
 struct can_rx_offload_queue {
 	struct sk_buff_head skb_queue;
 	struct napi_struct napi;
+
+	struct list_head sources;
+	unsigned int source_cnt;
+	bool sort;
 };
 
+/* One struct can_rx_offload per source of RX skbs, e.g. per IRQ line.
+ * Sources added with can_rx_offload_add_source() share the queue of the
+ * primary source.
+ */
 struct can_rx_offload {
 	struct net_device *dev;
 
@@ -32,9 +40,12 @@ struct can_rx_offload {
 
 	bool inc;
 
-	/* Points to own_queue. */
+	/* Points to own_queue of the primary source. own_queue is unused
+	 * in the other sources.
+	 */
 	struct can_rx_offload_queue *queue;
 	struct can_rx_offload_queue own_queue;
+	struct list_head node;
 };
 
 int can_rx_offload_add_timestamp(struct net_device *dev,
@@ -45,6 +56,8 @@ int can_rx_offload_add_fifo(struct net_device *dev,
 int can_rx_offload_add_manual(struct net_device *dev,
 			      struct can_rx_offload *offload,
 			      unsigned int weight);
+int can_rx_offload_add_source(struct can_rx_offload *primary,
+			      struct can_rx_offload *source);
 int can_rx_offload_irq_offload_timestamp(struct can_rx_offload *offload,
 					 u64 reg);
 int can_rx_offload_irq_offload_fifo(struct can_rx_offload *offload);
-- 
2.43.0

Re: [PATCH v7 3/4] can: rx-offload: allow more than one receive source

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 4 (2)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In can_rx_offload_move_to_skb_queue() the comment says the
  sorted merge will 'keep the timestamp order over all sources', but it…
- [Low] Concern: the commit message describes a live S32G2 flexcan bug,
  but the patch only adds can_rx_offload_add_source() and nothing calls…
- [Low] can_rx_offload_add_source() calls
  can_rx_offload_init_mb_range(source, &weight), then never uses the…
- [Low] queue->sort is set only by can_rx_offload_add_timestamp(), so
  with several FIFO-mode or manual-mode sources each source's…
- [Low] can_rx_offload_add_source() does not check whether @source is
  already linked.

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but
  can_rx_offload_init_mb_range() (previously inline in…
- [Low] This problem wasn't introduced by this patch, but several sources
  share dev->stats, and can_rx_offload_offload_one(),…

--

can: rx-offload: allow more than one receive source

This commit lets a CAN device have several rx-offload sources, one per
producer such as an IRQ line. All of them share one can_rx_offload_queue,
which the primary source owns. A new can_rx_offload_add_source() attaches
extra sources, and the IRQ finish helpers move each source's skb_irq_queue
into the shared skb_queue under skb_queue.lock, sorting by timestamp when
several sources run in timestamp mode.

> diff --git a/drivers/net/can/dev/rx-offload.c b/drivers/net/can/dev/rx-offload.c
> index 92309ed47bac5..5352d70b6e93c 100644
> --- a/drivers/net/can/dev/rx-offload.c
> +++ b/drivers/net/can/dev/rx-offload.c
> @@ -304,18 +304,40 @@ can_rx_offload_get_echo_skb_queue_tail(struct can_rx_offload *offload,
>  }
>  EXPORT_SYMBOL_GPL(can_rx_offload_get_echo_skb_queue_tail);
>  
> -void can_rx_offload_irq_finish(struct can_rx_offload *offload)
> +/* Sources may run on different CPUs at the same time, skb_queue.lock
> + * serializes them.
> + */

[Severity: Low]
This is a pre-existing issue, but the sources also share
offload->dev->stats. can_rx_offload_offload_one(),
can_rx_offload_get_echo_skb_queue_timestamp() and
can_rx_offload_get_echo_skb_queue_tail() update these counters with plain
increments from IRQ context:

drivers/net/can/dev/rx-offload.c:can_rx_offload_offload_one() {
    ...
	if (IS_ERR(skb)) {
		offload->dev->stats.rx_dropped++;
		offload->dev->stats.rx_fifo_errors++;
    ...
}

When sources run on different CPUs, can these increments get lost?

flexcan already ran these helpers from several IRQ lines at once before
this series. The only effect is slightly wrong drop and error counters.

> +static void can_rx_offload_move_to_skb_queue(struct can_rx_offload *offload)
>  {
>  	struct can_rx_offload_queue *queue = offload->queue;
>  	unsigned long flags;
> +
> +	spin_lock_irqsave(&queue->skb_queue.lock, flags);
> +
> +	if (queue->source_cnt > 1 && queue->sort) {
> +		/* keep the timestamp order over all sources */
> +		struct sk_buff *skb;
> +
> +		while ((skb = __skb_dequeue(&offload->skb_irq_queue)))
> +			__skb_queue_add_sort(&queue->skb_queue, skb,
> +					     can_rx_offload_compare);

[Severity: Medium]
Does this really keep the timestamp order over all sources?

__skb_queue_add_sort() only orders the frames that are in queue->skb_queue
at the moment of the merge. can_rx_offload_napi_poll() drains that queue
independently of the producers:

drivers/net/can/dev/rx-offload.c:can_rx_offload_napi_poll() {
    ...
	while ((work_done < quota) &&
	       (skb = skb_dequeue(&queue->skb_queue))) {
		...
		netif_receive_skb(skb);
	}
    ...
}

With two sources on different CPUs:

CPU0 (source A)                  CPU1 (source B)
                                 reads frame ts=10 into skb_irq_queue
                                 still busy in RX loop, TX complete or
                                 ESR handling
reads frame ts=20
can_rx_offload_irq_finish()
  merges ts=20, napi_schedule()
NAPI delivers ts=20
                                 can_rx_offload_irq_finish()
                                   merges ts=10
NAPI delivers ts=10

Can CAN frames reach the stack out of timestamp order this way?

With the next patch in the series, S32G2 in timestamp mode runs all four
flexcan sources through flexcan_irq() concurrently. That configuration
meets the queue->source_cnt > 1 && queue->sort condition.

skb_queue.lock only serializes the list manipulation. Nothing holds back
delivery until the other sources have merged their older frames.

If full ordering isn't achievable here, should the comment and the commit
message describe the narrower guarantee?

> +	} else {
> +		skb_queue_splice_tail_init(&offload->skb_irq_queue,
> +					   &queue->skb_queue);
> +	}

[Severity: Low]
Only can_rx_offload_add_timestamp() sets queue->sort. With several
FIFO-mode or manual-mode sources, each skb_irq_queue is therefore just
appended here. That happens even when a driver ordered its frames with
can_rx_offload_queue_timestamp().

The commit message documents this ("Otherwise they append it").

At the end of the series, the only in-tree FIFO multi-source user is
flexcan on mcf5441x (FLEXCAN_QUIRK_NR_IRQ_3). That part is uniprocessor
m68k, so its handlers never run in parallel.

Would it make sense to state in the kerneldoc of
can_rx_offload_add_source() that ordering across sources is not kept in
FIFO and manual mode, for future SMP users?

[ ... ]

> @@ -350,6 +369,16 @@ void can_rx_offload_threaded_irq_finish(struct can_rx_offload *offload)
>  }
>  EXPORT_SYMBOL_GPL(can_rx_offload_threaded_irq_finish);
>  
> +static void can_rx_offload_link_source(struct can_rx_offload_queue *queue,
> +				       struct can_rx_offload *offload)
> +{
> +	offload->queue = queue;
> +	__skb_queue_head_init(&offload->skb_irq_queue);
> +
> +	list_add_tail(&offload->node, &queue->sources);
> +	queue->source_cnt++;
> +}

[Severity: Low]
Nothing here checks whether offload is already on a sources list.

Suppose can_rx_offload_add_source() is called twice for the same source
with no can_rx_offload_del() in between. Wouldn't that reinitialize a live
skb_irq_queue, count the source twice in source_cnt, and call
list_add_tail() on a node that is already linked? The node could end up
pointing at itself, and then the list_for_each_entry_safe() loop in
can_rx_offload_del() would never finish.

No in-tree path does this. At the end of the series, flexcan calls
can_rx_offload_del(&priv->offload) in flexcan_close() and on every failure
path before flexcan_open() adds the sources again. can_rx_offload_init_queue()
also resets the list.

Would a defensive check against adding the same source twice be worth
having?

[ ... ]

> @@ -357,41 +386,65 @@ static int can_rx_offload_init_queue(struct net_device *dev,

[ ... ]

> -int can_rx_offload_add_timestamp(struct net_device *dev,
> -				 struct can_rx_offload *offload)
> +static int can_rx_offload_init_mb_range(struct can_rx_offload *offload,
> +					unsigned int *weight)
>  {
> -	unsigned int weight;
> -
>  	if (offload->mb_first > BITS_PER_LONG_LONG ||
> -	    offload->mb_last > BITS_PER_LONG_LONG || !offload->mailbox_read)
> +	    offload->mb_last > BITS_PER_LONG_LONG)
>  		return -EINVAL;

[Severity: Low]
This isn't a bug introduced by this patch, but the bound check moved here
accepts mb_first or mb_last equal to BITS_PER_LONG_LONG (64).
can_rx_offload_irq_offload_timestamp() walks the range inclusively and
does:

		if (!(pending & BIT_ULL(i)))
			continue;

so i == 64 would be an out-of-range shift. Should these checks use >=?

No in-tree driver passes 64 today. flexcan uses mb_count - 2, at91 uses
rx_last, and ti_hecc uses at most HECC_MAX_MAILBOXES - 1.

[ ... ]

> @@ -416,6 +469,46 @@ int can_rx_offload_add_manual(struct net_device *dev,
>  }
>  EXPORT_SYMBOL_GPL(can_rx_offload_add_manual);
>  
> +/**

[ ... ]

> +int can_rx_offload_add_source(struct can_rx_offload *primary,
> +			      struct can_rx_offload *source)
> +{

[Severity: Low]
This isn't a bug, but the commit message describes a live S32G2 flexcan
problem, while nothing in this commit calls can_rx_offload_add_source()
and there is no Fixes: tag.

This is resolved later in the series. The next patch, "can: flexcan: use
one rx-offload source per IRQ line", does the following:

- adds the extra sources in flexcan_rx_offload_setup()
- picks the source per IRQ via flexcan_get_offload()
- switches flexcan_mailbox_read() to netdev_priv(offload->dev)
- carries Fixes: 8503a4b1a24d ("can: flexcan: add NXP S32G2/S32G3 SoC
  support")

So adding the infrastructure first and the user next looks fine.

> +	struct can_rx_offload_queue *queue = primary->queue;
> +	unsigned int weight;
> +	int err;
> +
> +	if (queue != &primary->own_queue || !queue->source_cnt ||
> +	    source == primary)
> +		return -EINVAL;
> +
> +	if (queue->sort) {
> +		err = can_rx_offload_init_mb_range(source, &weight);
> +		if (err)
> +			return err;
> +	}
> +
> +	source->dev = primary->dev;
> +	source->mailbox_read = primary->mailbox_read;
> +	source->skb_queue_len_max = primary->skb_queue_len_max;

[Severity: Low]
The weight that can_rx_offload_init_mb_range() computes for source is
never used. skb_queue_len_max is copied from the primary, and the shared
NAPI weight is not adjusted. So the drop limit and the NAPI weight depend
only on the primary's mailbox range.

This matches the commit message ("inherits the mode, netdev, queue length
limit and mailbox_read() of the primary"). Later in the series,
flexcan_rx_offload_add_source() copies the primary's mb_first and mb_last
into every source, so the limit fits there.

A driver that split its mailboxes into a small primary range and a larger
secondary range would get an undersized shared limit, though.

Should the unused weight be dropped, or is it meant to feed into
skb_queue_len_max?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002071203.1287650-1-ciprianmarian.costea%40oss.nxp.com

[PATCH v7 4/4] can: flexcan: use one rx-offload source per IRQ line

[Ciprian Costea]

From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>

Currently, flexcan_irq() is requested on every IRQ line of the
controller: the mailbox line, the bus off and error lines
(FLEXCAN_QUIRK_NR_IRQ_3) and a second mailbox line
(FLEXCAN_QUIRK_SECONDARY_MB_IRQ). On NXP S32G2, which has all four, the
handlers can run at the same time on different CPUs and corrupt the
rx-offload queue they share.

Add an rx-offload source for each extra line with
can_rx_offload_add_source(), pick the source in flexcan_irq() based on
the IRQ number and pass it to the functions that queue skbs.

flexcan_mailbox_read() is now called with any of the sources, so get the
private data with netdev_priv() instead of container_of().

This only fixes the queue corruption. All handlers still process the
whole mailbox range and the same ESR events. A dedicated handler per line
and splitting the mailbox range between the two mailbox lines will follow
in a separate series, which also removes the lookup added here.

Fixes: 8503a4b1a24d ("can: flexcan: add NXP S32G2/S32G3 SoC support")
Assisted-by: LLM
Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
---
 drivers/net/can/flexcan/flexcan-core.c | 86 +++++++++++++++++++++-----
 drivers/net/can/flexcan/flexcan.h      |  3 +
 2 files changed, 73 insertions(+), 16 deletions(-)

diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
index f5d22c61503f..2ef06ed0c744 100644
--- a/drivers/net/can/flexcan/flexcan-core.c
+++ b/drivers/net/can/flexcan/flexcan-core.c
@@ -828,7 +828,8 @@ static netdev_tx_t flexcan_start_xmit(struct sk_buff *skb, struct net_device *de
 	return NETDEV_TX_OK;
 }
 
-static void flexcan_irq_bus_err(struct net_device *dev, u32 reg_esr)
+static void flexcan_irq_bus_err(struct net_device *dev,
+				struct can_rx_offload *offload, u32 reg_esr)
 {
 	struct flexcan_priv *priv = netdev_priv(dev);
 	struct flexcan_regs __iomem *regs = priv->regs;
@@ -885,12 +886,13 @@ static void flexcan_irq_bus_err(struct net_device *dev, u32 reg_esr)
 	if (tx_errors)
 		dev->stats.tx_errors++;
 
-	err = can_rx_offload_queue_timestamp(&priv->offload, skb, timestamp);
+	err = can_rx_offload_queue_timestamp(offload, skb, timestamp);
 	if (err)
 		dev->stats.rx_fifo_errors++;
 }
 
-static void flexcan_irq_state(struct net_device *dev, u32 reg_esr)
+static void flexcan_irq_state(struct net_device *dev,
+			      struct can_rx_offload *offload, u32 reg_esr)
 {
 	struct flexcan_priv *priv = netdev_priv(dev);
 	struct flexcan_regs __iomem *regs = priv->regs;
@@ -932,7 +934,7 @@ static void flexcan_irq_state(struct net_device *dev, u32 reg_esr)
 	if (unlikely(new_state == CAN_STATE_BUS_OFF))
 		can_bus_off(dev);
 
-	err = can_rx_offload_queue_timestamp(&priv->offload, skb, timestamp);
+	err = can_rx_offload_queue_timestamp(offload, skb, timestamp);
 	if (err)
 		dev->stats.rx_fifo_errors++;
 }
@@ -967,16 +969,12 @@ static inline u64 flexcan_read_reg_iflag_tx(struct flexcan_priv *priv)
 	return flexcan_read64_mask(priv, &priv->regs->iflag1, priv->tx_mask);
 }
 
-static inline struct flexcan_priv *rx_offload_to_priv(struct can_rx_offload *offload)
-{
-	return container_of(offload, struct flexcan_priv, offload);
-}
-
 static struct sk_buff *flexcan_mailbox_read(struct can_rx_offload *offload,
 					    unsigned int n, u32 *timestamp,
 					    bool drop)
 {
-	struct flexcan_priv *priv = rx_offload_to_priv(offload);
+	/* offload may be any of the sources, see can_rx_offload_add_source() */
+	struct flexcan_priv *priv = netdev_priv(offload->dev);
 	struct flexcan_regs __iomem *regs = priv->regs;
 	struct flexcan_mb __iomem *mb;
 	struct sk_buff *skb;
@@ -1070,11 +1068,34 @@ static struct sk_buff *flexcan_mailbox_read(struct can_rx_offload *offload,
 	return skb;
 }
 
+/* The same handler is requested on every IRQ line, the line it is called
+ * for selects the rx-offload source to queue into.
+ */
+static struct can_rx_offload *
+flexcan_get_offload(struct flexcan_priv *priv, int irq)
+{
+	const u32 quirks = priv->devtype_data.quirks;
+
+	if (quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ &&
+	    irq == priv->irq_secondary_mb)
+		return &priv->offload_secondary_mb;
+
+	if (quirks & FLEXCAN_QUIRK_NR_IRQ_3) {
+		if (irq == priv->irq_boff)
+			return &priv->offload_boff;
+		if (irq == priv->irq_err)
+			return &priv->offload_err;
+	}
+
+	return &priv->offload;
+}
+
 static irqreturn_t flexcan_irq(int irq, void *dev_id)
 {
 	struct net_device *dev = dev_id;
 	struct net_device_stats *stats = &dev->stats;
 	struct flexcan_priv *priv = netdev_priv(dev);
+	struct can_rx_offload *offload = flexcan_get_offload(priv, irq);
 	struct flexcan_regs __iomem *regs = priv->regs;
 	irqreturn_t handled = IRQ_NONE;
 	u64 reg_iflag_tx;
@@ -1088,7 +1109,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 
 		while ((reg_iflag_rx = flexcan_read_reg_iflag_rx(priv))) {
 			handled = IRQ_HANDLED;
-			ret = can_rx_offload_irq_offload_timestamp(&priv->offload,
+			ret = can_rx_offload_irq_offload_timestamp(offload,
 								   reg_iflag_rx);
 			if (!ret)
 				break;
@@ -1099,7 +1120,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 		reg_iflag1 = priv->read(&regs->iflag1);
 		if (reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_AVAILABLE) {
 			handled = IRQ_HANDLED;
-			can_rx_offload_irq_offload_fifo(&priv->offload);
+			can_rx_offload_irq_offload_fifo(offload);
 		}
 
 		/* FIFO overflow interrupt */
@@ -1120,7 +1141,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 
 		handled = IRQ_HANDLED;
 		stats->tx_bytes +=
-			can_rx_offload_get_echo_skb_queue_timestamp(&priv->offload, 0,
+			can_rx_offload_get_echo_skb_queue_timestamp(offload, 0,
 								    reg_ctrl << 16, NULL);
 		stats->tx_packets++;
 
@@ -1143,12 +1164,12 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 	if ((reg_esr & FLEXCAN_ESR_ERR_STATE) ||
 	    (priv->devtype_data.quirks & (FLEXCAN_QUIRK_BROKEN_WERR_STATE |
 					   FLEXCAN_QUIRK_BROKEN_PERR_STATE)))
-		flexcan_irq_state(dev, reg_esr);
+		flexcan_irq_state(dev, offload, reg_esr);
 
 	/* bus error IRQ - handle if bus error reporting is activated */
 	if ((reg_esr & FLEXCAN_ESR_ERR_BUS) &&
 	    (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING))
-		flexcan_irq_bus_err(dev, reg_esr);
+		flexcan_irq_bus_err(dev, offload, reg_esr);
 
 	/* availability of error interrupt among state transitions in case
 	 * bus error reporting is de-activated and
@@ -1189,7 +1210,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 	}
 
 	if (handled)
-		can_rx_offload_irq_finish(&priv->offload);
+		can_rx_offload_irq_finish(offload);
 
 	return handled;
 }
@@ -1381,6 +1402,15 @@ static void flexcan_ram_init(struct net_device *dev)
 	priv->write(reg_ctrl2, &regs->ctrl2);
 }
 
+static int flexcan_rx_offload_add_source(struct flexcan_priv *priv,
+					 struct can_rx_offload *source)
+{
+	source->mb_first = priv->offload.mb_first;
+	source->mb_last = priv->offload.mb_last;
+
+	return can_rx_offload_add_source(&priv->offload, source);
+}
+
 static int flexcan_rx_offload_setup(struct net_device *dev)
 {
 	struct flexcan_priv *priv = netdev_priv(dev);
@@ -1422,6 +1452,30 @@ static int flexcan_rx_offload_setup(struct net_device *dev)
 		err = can_rx_offload_add_fifo(dev, &priv->offload,
 					      FLEXCAN_NAPI_WEIGHT);
 	}
+	if (err)
+		return err;
+
+	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
+		err = flexcan_rx_offload_add_source(priv,
+						    &priv->offload_secondary_mb);
+		if (err)
+			goto out_can_rx_offload_del;
+	}
+
+	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_NR_IRQ_3) {
+		err = flexcan_rx_offload_add_source(priv, &priv->offload_boff);
+		if (err)
+			goto out_can_rx_offload_del;
+
+		err = flexcan_rx_offload_add_source(priv, &priv->offload_err);
+		if (err)
+			goto out_can_rx_offload_del;
+	}
+
+	return 0;
+
+out_can_rx_offload_del:
+	can_rx_offload_del(&priv->offload);
 
 	return err;
 }
diff --git a/drivers/net/can/flexcan/flexcan.h b/drivers/net/can/flexcan/flexcan.h
index 16692a2502eb..0b09e94d0010 100644
--- a/drivers/net/can/flexcan/flexcan.h
+++ b/drivers/net/can/flexcan/flexcan.h
@@ -88,6 +88,9 @@ struct flexcan_stop_mode {
 struct flexcan_priv {
 	struct can_priv can;
 	struct can_rx_offload offload;
+	struct can_rx_offload offload_secondary_mb;
+	struct can_rx_offload offload_boff;
+	struct can_rx_offload offload_err;
 	struct device *dev;
 
 	struct flexcan_regs __iomem *regs;
-- 
2.43.0

Re: [PATCH v7 4/4] can: flexcan: use one rx-offload source per IRQ line

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 (1) · Medium: 1 (1) · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Concurrent handlers can still corrupt the shared rx-offload
  queue through the TX-completion path, which is the bug class the…
- [Medium] In flexcan_irq() (drivers/net/can/flexcan/flexcan-core.c), the
  per-line source's skb_irq_queue is only flushed when `handled` is set…
- [Low] The patch has `Fixes: 8503a4b1a24d ("can: flexcan: add NXP
  S32G2/S32G3 SoC support")`, so stable trees that carry S32G2 support…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it remains
  after it.
- [Medium] This problem wasn't introduced by this patch, but concurrent
  S32G2 handlers still race on shared CAN state and statistics.

--

can: flexcan: use one rx-offload source per IRQ line

This patch adds an rx-offload source with can_rx_offload_add_source() for
each extra FlexCAN IRQ line: bus off, error and the secondary mailbox line.
flexcan_irq() picks the source from the IRQ number and passes it to the
functions that queue skbs, so concurrent handlers on S32G2 no longer corrupt
the shared rx-offload queue. flexcan_mailbox_read() now gets the private
data through netdev_priv() instead of container_of().

> diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
> index f5d22c61503fa..2ef06ed0c7449 100644
> --- a/drivers/net/can/flexcan/flexcan-core.c
> +++ b/drivers/net/can/flexcan/flexcan-core.c

[ ... ]

> @@ -932,7 +934,7 @@ static void flexcan_irq_state(struct net_device *dev, u32 reg_esr)
>  	if (unlikely(new_state == CAN_STATE_BUS_OFF))
>  		can_bus_off(dev);
>  
> -	err = can_rx_offload_queue_timestamp(&priv->offload, skb, timestamp);
> +	err = can_rx_offload_queue_timestamp(offload, skb, timestamp);
>  	if (err)
>  		dev->stats.rx_fifo_errors++;
>  }

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch.
Concurrent S32G2 handlers still race on the shared CAN state here.

With FLEXCAN_QUIRK_BROKEN_PERR_STATE, flexcan_irq_state() runs on every
call on every line. It compares the new_state from its own ESR snapshot
with priv->can.state, without a lock:

	/* state hasn't changed */
	if (likely(new_state == priv->can.state))
		return;

can_change_state() then does a plain priv->state = new_state.

Can two CPUs see the same transition? If so, both would queue an error
frame, both would bump can_stats and both would call can_bus_off().

Could a handler with an older ESR snapshot replace a newer
CAN_STATE_BUS_OFF with CAN_STATE_ERROR_WARNING?

The BROKEN_PERR_STATE block at the end of flexcan_irq() has a similar
problem. It calls flexcan_error_irq_enable() or flexcan_error_irq_disable()
based on unlocked snapshots of last_state and priv->can.state. Both
functions write a full CTRL value built from reg_ctrl_default. Can the last
writer leave ERR_MSK out of step with the final state?

The dev->stats and can_stats counters (tx_bytes, tx_packets, rx_errors,
rx_fifo_errors, rx_dropped, bus_error) are also plain read-modify-write
updates from several hardirq contexts, so increments can be lost.

[ ... ]

> @@ -1120,7 +1141,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>  
>  		handled = IRQ_HANDLED;
>  		stats->tx_bytes +=
> -			can_rx_offload_get_echo_skb_queue_timestamp(&priv->offload, 0,
> +			can_rx_offload_get_echo_skb_queue_timestamp(offload, 0,
>  								    reg_ctrl << 16, NULL);
>  		stats->tx_packets++;
>  

[Severity: High]
Can two handlers still race on the echo skb here?

flexcan_open() requests flexcan_irq() on dev->irq, irq_boff, irq_err and
irq_secondary_mb. Every call reads flexcan_read_reg_iflag_tx(). The TX
iflag is cleared only after the echo skb has been taken:

		flexcan_write64(priv, priv->tx_mask, &regs->iflag1);
		netif_wake_queue(dev);

So two handlers on different CPUs can both see the TX bit, and both go
down this path:

flexcan_irq()
  can_rx_offload_get_echo_skb_queue_timestamp(offload, 0, ...)
    __can_get_echo_skb(dev, 0, ...)

__can_get_echo_skb() checks priv->echo_skb[idx] and clears the slot only
later, with no lock or xchg. Both CPUs can therefore get the same skb.

For a PACKET_LOOPBACK skb, each CPU adds it with __skb_queue_add_sort() to
its own source's skb_irq_queue. can_rx_offload_move_to_skb_queue() later
merges both into the shared queue->skb_queue. Would that link the same skb
into one list twice, so that NAPI passes a freed skb to
netif_receive_skb()?

For an echo skb that is not PACKET_LOOPBACK, both CPUs would call
dev_consume_skb_any() on it.

There is a related interleaving. The late handler could pick up a new echo
skb that flexcan_start_xmit() queued after the first handler's
netif_wake_queue(). It would then write FLEXCAN_MB_CODE_TX_INACTIVE to
tx_mb, aborting a transmission that was already reported as sent.

The echo handling itself is older than this patch. However, the commit
message says this fixes concurrent handlers corrupting the shared
rx-offload queue. Should the TX completion handling (or the whole handler)
be serialized across lines, rather than only splitting the staging queues?

[ ... ]

> @@ -1143,12 +1164,12 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>  	if ((reg_esr & FLEXCAN_ESR_ERR_STATE) ||
>  	    (priv->devtype_data.quirks & (FLEXCAN_QUIRK_BROKEN_WERR_STATE |
>  					   FLEXCAN_QUIRK_BROKEN_PERR_STATE)))
> -		flexcan_irq_state(dev, reg_esr);
> +		flexcan_irq_state(dev, offload, reg_esr);
>  

[ ... ]

> @@ -1189,7 +1210,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>  	}
>  
>  	if (handled)
> -		can_rx_offload_irq_finish(&priv->offload);
> +		can_rx_offload_irq_finish(offload);
>  
>  	return handled;
>  }

[Severity: Medium]
Can a state-change skb get stuck in a per-line source here?

nxp_s32g2_devtype_data has FLEXCAN_QUIRK_BROKEN_PERR_STATE, so the
flexcan_irq_state() call above runs on every call, whatever ESR contains.
If new_state differs from priv->can.state, flexcan_irq_state() queues an
error skb with can_rx_offload_queue_timestamp(offload, ...). It does this
even when handled is still IRQ_NONE.

handled can stay IRQ_NONE when a handler on another line has already acked
all RX, TX and ESR interrupt bits. Two cases reach this path:

  - a passive to warning/active transition, which raises no interrupt
  - a concurrent handler that has not updated can.state yet

Before this patch, the skb sat in priv->offload.skb_irq_queue, and the next
handled interrupt on any line flushed it.

Now flexcan_get_offload() sends it to offload_boff, offload_err or
offload_secondary_mb. Only can_rx_offload_irq_finish() on that same source
drains it, or can_rx_offload_del() purges it at close.

On the bus-off or error line, could the state-change frame stay stuck for
an arbitrarily long time? When it is finally merged, its stale timestamp
would sort it against frames that were already delivered, so it would
arrive out of order.

can_rx_offload_irq_finish() already returns early when skb_irq_queue is
empty. Would calling it unconditionally avoid this?

[ ... ]

> @@ -1381,6 +1402,15 @@ static void flexcan_ram_init(struct net_device *dev)
>  	priv->write(reg_ctrl2, &regs->ctrl2);
>  }
>  
> +static int flexcan_rx_offload_add_source(struct flexcan_priv *priv,
> +					 struct can_rx_offload *source)
> +{
> +	source->mb_first = priv->offload.mb_first;
> +	source->mb_last = priv->offload.mb_last;

[Severity: High]
This isn't a bug introduced by this patch, but it remains after it. Every
extra source gets the full primary mailbox range here, and the RX loop in
flexcan_irq() runs on every line:

		while ((reg_iflag_rx = flexcan_read_reg_iflag_rx(priv))) {
			...
			ret = can_rx_offload_irq_offload_timestamp(offload,
								   reg_iflag_rx);

Nothing serializes flexcan_mailbox_read() between CPUs. The FlexCAN read
sequence assumes a single reader:

  - reading C/S locks the MB
  - the ID and data are copied
  - IFLAG is cleared
  - reading the free running timer unlocks the MB, globally

Suppose two handlers both have bit n in their IFLAG snapshot. Can both of
them read MB n? The CODE stays RX_FULL after a read, so the second reader
passes this check and delivers the frame a second time:

		if ((code != FLEXCAN_MB_CODE_RX_FULL) &&
		    (code != FLEXCAN_MB_CODE_RX_OVERRUN))
			return NULL;

Another CPU can read priv->read(&regs->timer) from flexcan_mailbox_read(),
flexcan_irq_state() or flexcan_irq_bus_err(). Could that unlock the MB
while this CPU is still copying it, so the hardware overwrites it mid-copy?

Could the second reader's late clear

	flexcan_write64(priv, FLEXCAN_IFLAG_MB(n), &regs->iflag1);

wipe the IFLAG of a newer frame that arrived after the first reader
unlocked the MB? Later scans would then miss that frame.

The commit message defers this to a follow-up series ("All handlers still
process the whole mailbox range"). Until then, the S32G2 support named in
the Fixes: tag can deliver duplicated, torn or lost CAN frames.

> +
> +	return can_rx_offload_add_source(&priv->offload, source);

[Severity: Low]
can_rx_offload_add_source() only exists once these earlier patches in the
series are applied:

  can: rx-offload: move skb_queue and napi into struct can_rx_offload_queue
  can: rx-offload: allow more than one receive source

The fix also relies on two things from those patches: the per-source
offload->queue linkage, and the locked merge in
can_rx_offload_move_to_skb_queue().

Neither of those patches has a Fixes: or Cc: stable tag. This commit has:

  Fixes: 8503a4b1a24d ("can: flexcan: add NXP S32G2/S32G3 SoC support")

If stable selects this patch only through the Fixes: tag, will the build
fail? A hand backport could also leave out the merge logic that actually
stops the corruption.

Would it help to add Cc: stable@vger.kernel.org, with dependency notes for
both rx-offload patches?

> +}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002071203.1287650-1-ciprianmarian.costea%40oss.nxp.com