Thread (7 messages) flat view 7 messages, 2 authors, 15d ago

Re: [PATCH v5 0/3] can_rx_offload keeps a lockless irq_queue that the IRQ handlers fill and that is later spliced under skb_queue.lock into the NAPI-facing skb_queue. This works as long as a single context fills the irq_queue. flexcan with FLEXCAN_QUIRK_SECONDARY_MB_IRQ and mcf5441x use two mailbox IRQ lines. When those are affined to different CPUs the two handlers can enqueue into the same list at the same time and corrupt it.

From: Marc Kleine-Budde <mkl@pengutronix.de>
Date: 2026-09-08 08:58:30
Also in: imx, linux-can, lkml

FYI: somehow the series description ended up in the subject

On 07.09.2026 18:07:42, Ciprian Marian Costea wrote:
On 9/7/2026 4:49 PM, Marc Kleine-Budde wrote:
quoted
On 07.09.2026 12:49:39, Ciprian Costea wrote:
quoted
From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>

This series:
   1. Makes the irq_queue per-CPU so the handlers no longer share a list.
As sashiko pointed out, using per-CPU variables in a preemptible context
doesn't work. When proposing to use per-CPU variables I haven't thought
that far. So in hindsight this approach is not good.

What about following what NAPI does. Have a dedicated data structure per
IRQ. I think these ones are needed:

| 	struct sk_buff_head skb_irq_queue;
| 	u32 skb_queue_len_max;
|
| 	unsigned int mb_first;
| 	unsigned int mb_last;

And pass them to can_rx_offload_queue_timestamp() and
can_rx_offload_irq_finish().
If the RX-Offload knows the number of concurrent IRQs, you can optimize
for the single IRQ case and keep skb_queue_splice_tail_init(), while for
the other case you should sort the skbs into the offload->skb_queue
list.
Thanks for replying.
Indeed the per-CPU approach should be dropped. I've also missed the
PREEMPT_RT case.
Your proposal seems better than even having a dedicated spinlock for
skb_irq_queue operations as I've originally thought.

One thing to confirm before I go ahead and implement this approach for V6:
Having the following connected patchset in mind [1] which separates the IRQ
handlers, both flexcan MB IRQs currently run flexcan_do_mb() over the full
iflag. Therefore, to actually separate the producers I'll have each IRQ
drain only its own mb_first..mb_last range into its own queue - is this the
right approach ?
Yes!

Please look out for shared ressources (read-modify-write registers,
etc..) when accessing the mailboxes concurrently from 2 (or more) IRQs.
After a quick look I haven't found any.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

Attachments

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help