drivers/net/can/at91_can.c | 10 +++- drivers/net/can/dev/rx-offload.c | 88 +++++++++++++++++++++++++++----- drivers/net/can/usb/gs_usb.c | 5 +- include/linux/can/rx-offload.h | 2 +- 4 files changed, 87 insertions(+), 18 deletions(-)
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. 2. Fixes at91_can rx-offload teardown. 3. Checks the can_rx_offload_add_manual() return value in gs_usb. 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 (3): can: rx-offload: make skb_irq_queue per-CPU can: at91_can: fix rx-offload cleanup on unbind and probe errors can: gs_usb: check can_rx_offload_add_manual() return value drivers/net/can/at91_can.c | 10 +++- drivers/net/can/dev/rx-offload.c | 88 +++++++++++++++++++++++++++----- drivers/net/can/usb/gs_usb.c | 5 +- include/linux/can/rx-offload.h | 2 +- 4 files changed, 87 insertions(+), 18 deletions(-) -- 2.43.0
On 07.09.2026 12:49:39, Ciprian Costea wrote: > 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(). 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 |
On 9/7/2026 4:49 PM, Marc Kleine-Budde wrote: > On 07.09.2026 12:49:39, Ciprian Costea wrote: >> 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(). > > regards, > Marc > Hello Marc, 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 ? [1] https://lore.kernel.org/all/20260831143449.12828-1-ciprianmarian.costea@oss.nxp.com/T/#t Regards, Ciprian
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: > > On 07.09.2026 12:49:39, Ciprian Costea wrote: > > > 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 |
© 2016 - 2026 Red Hat, Inc.