Re: [PATCH v6 5/6] can: flexcan: add FLEXCAN_QUIRK_IRQ_BERR quirk

[Bough Chen <haibo.chen@oss.nxp.com>]

On Mon, Jul 13, 2026 at 09:19:50AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Concurrent execution of separate hardware interrupt handlers (`dev->irq` and `priv->irq_err`) modifies the lockless `can_rx_offload` IRQ queue (`skb_irq_queue`), leading to list corruption.
> --
> 
> --- Patch [5]: [PATCH v6 5/6] can: flexcan: add FLEXCAN_QUIRK_IRQ_BERR quirk ---
> commit 97d482f1edd202225930276cf268822cd49ea3d2
> Author: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> 
> can: flexcan: add FLEXCAN_QUIRK_IRQ_BERR quirk
> 
> Introduce FLEXCAN_QUIRK_IRQ_BERR quirk to handle hardware integration
> where the FlexCAN module has a dedicated interrupt line for signaling
> bus errors and device state changes. This adds the flexcan_irq_esr() handler
> and is required for NXP S32N79 SoC support.
> 
> > diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
> > index 8b70952f7f05d..e7e3fb41f55e7 100644
> > --- a/drivers/net/can/flexcan/flexcan-core.c
> > +++ b/drivers/net/can/flexcan/flexcan-core.c
> > @@ -1300,6 +1300,22 @@ static irqreturn_t flexcan_irq_boff(int irq, void *dev_id)
> >  	return handled;
> >  }
> >  
> > +/* Combined bus error and state change IRQ handler */
> > +static irqreturn_t flexcan_irq_esr(int irq, void *dev_id)
> > +{
> > +	struct net_device *dev = dev_id;
> > +	struct flexcan_priv *priv = netdev_priv(dev);
> > +	irqreturn_t handled;
> > +
> > +	handled = flexcan_do_state(dev);
> > +	handled |= flexcan_do_berr(dev);
> > +
> > +	if (handled)
> > +		can_rx_offload_irq_finish(&priv->offload);
> > +
> > +	return handled;
> > +}
> 
> [Severity: High]
> This is a pre-existing issue, but could this lead to list corruption
> if these separate IRQs run concurrently on different CPUs?
> 
> Since flexcan_irq_esr() and flexcan_irq_mb() are registered as separate
> interrupts, they might fire at the same time. If flexcan_irq_esr()
> processes a bus error and flexcan_irq_mb() processes a mailbox interrupt,
> they both end up modifying the lockless offload->skb_irq_queue.
> 
> CPU1 handling dev->irq (flexcan_irq_mb):
> flexcan_irq_mb()
>   can_rx_offload_irq_offload_timestamp()
>     __skb_queue_add_sort(&offload->skb_irq_queue, ...)
> 
> CPU2 handling priv->irq_err (flexcan_irq_esr):
> flexcan_irq_esr()
>   flexcan_do_berr()
>     flexcan_irq_bus_err()
>       can_rx_offload_queue_timestamp()
>         __skb_queue_add_sort(&offload->skb_irq_queue, ...)
> 
> Furthermore, both handlers unconditionally call can_rx_offload_irq_finish(),
> which locklessly splices the skb_irq_queue list:
> 
> can_rx_offload_irq_finish()
>   skb_queue_splice_tail_init(&offload->skb_irq_queue, ...)
> 
> Are there existing synchronization mechanisms preventing these IRQ
> handlers from running simultaneously and corrupting the lockless
> skb_irq_queue list pointers?

Yes, same with the review of PATCH3.

Ciprian Marian Costea, please address this.

Regards
Haibo Chen
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260713085306.2643794-1-ciprianmarian.costea@oss.nxp.com?part=5