Re: [PATCH v6 1/6] can: flexcan: use dedicated IRQ handlers for multi-IRQ platforms

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

On Mon, Jul 13, 2026 at 09:07:46AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> New issues:
> - [High] Splitting the IRQ handler breaks the FLEXCAN_QUIRK_BROKEN_PERR_STATE workaround on multi-IRQ platforms.
> 
> Pre-existing issues:
> - [Critical] Concurrent lockless modifications of `skb_irq_queue` by multiple IRQ handlers causes list corruption.
> - [High] Incomplete fix for duplicate frame processing on platforms with secondary mailbox IRQs.
> --
> 
> --- Patch [1]: [PATCH v6 1/6] can: flexcan: use dedicated IRQ handlers for multi-IRQ platforms ---
> commit b6f6f5d373c333a28a06c2ed84ea386f9dc5b122
> Author: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> 
> can: flexcan: use dedicated IRQ handlers for multi-IRQ platforms
>     
> On platforms with multiple IRQ lines (S32G2, MCF5441X), all lines are
> registered to the same flexcan_irq() handler. Since these are distinct IRQ
> numbers, they can be dispatched concurrently on different CPUs. Both
> instances then read the same iflag and ESR registers unconditionally,
> leading to duplicate frame processing.
> 
> Fix this by splitting the monolithic handler into focused parts:
> - flexcan_do_mb(): processes mailbox events
> - flexcan_do_state(): processes device state change events
> - flexcan_do_berr(): processes bus error events
>     
> Introduce dedicated IRQ handlers for multi-IRQ platforms:
> - flexcan_irq_mb(): mailbox-only, used for mb-0, mb-1 IRQ lines
> - flexcan_irq_boff(): state-change-only, used for boff/state IRQ line
> - flexcan_irq_berr(): bus-error-only, used for berr IRQ line
>     
> The combined flexcan_irq() handler is preserved for single-IRQ
> platforms with no functional change.
> 
> Fixes: d9cead75b1c6 ("can: flexcan: add mcf5441x support")
> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> Reviewed-by: Vincent Mailhol <mailhol@kernel.org>
> Reviewed-and-tested-by: Haibo Chen <haibo.chen@nxp.com>
> Tested-by: Enric Balletbo i Serra <eballetb@redhat.com>
> 
> > diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
> > index f5d22c61503fa..f73ff442d5304 100644
> > --- a/drivers/net/can/flexcan/flexcan-core.c
> > +++ b/drivers/net/can/flexcan/flexcan-core.c
> > @@ -1188,6 +1218,65 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
> >  		}
> >  	}
> >  
> > +	return handled;
> > +}
> > +
> > +/* Combined IRQ handler for single-IRQ platforms */
> > +static irqreturn_t flexcan_irq(int irq, void *dev_id)
> > +{
> > +	struct net_device *dev = dev_id;
> > +	struct flexcan_priv *priv = netdev_priv(dev);
> > +	irqreturn_t handled;
> > +
> > +	handled = flexcan_do_mb(dev);
> > +	handled |= flexcan_do_state(dev);
> > +	handled |= flexcan_do_berr(dev);
> > +
> > +	if (handled)
> > +		can_rx_offload_irq_finish(&priv->offload);
> > +
> > +	return handled;
> > +}
> > +
> > +/* Mailbox IRQ handler for multi-IRQ platforms */
> > +static irqreturn_t flexcan_irq_mb(int irq, void *dev_id)
> > +{
> > +	struct net_device *dev = dev_id;
> > +	struct flexcan_priv *priv = netdev_priv(dev);
> > +	irqreturn_t handled;
> > +
> > +	handled = flexcan_do_mb(dev);
> 
> [Severity: High]
> Does isolating mailbox handling into flexcan_irq_mb() break the
> FLEXCAN_QUIRK_BROKEN_PERR_STATE workaround?
> 
> On multi-IRQ platforms with this quirk (such as MCF5441X and S32G2), the
> hardware fails to trigger a state change interrupt, meaning flexcan_irq_boff()
> won't fire. The baseline driver worked around this by polling the state via
> flexcan_do_state() during every mailbox interrupt in the monolithic handler.
> 
> Since flexcan_irq_mb() completely bypasses flexcan_do_state(), could this cause
> the driver to silently miss CAN error state transitions (e.g., Error Passive)
> when the error rate increases?

AI is correct, need to fix this in next version.

> 
> > +
> > +	if (handled)
> > +		can_rx_offload_irq_finish(&priv->offload);
> > +
> > +	return handled;
> > +}
> > +
> > +/* Bus error IRQ handler for multi-IRQ platforms */
> > +static irqreturn_t flexcan_irq_berr(int irq, void *dev_id)
> > +{
> > +	struct net_device *dev = dev_id;
> > +	struct flexcan_priv *priv = netdev_priv(dev);
> > +	irqreturn_t handled;
> > +
> > +	handled = flexcan_do_berr(dev);
> 
> [Severity: Critical]
> This is a pre-existing issue, but does having dedicated IRQ handlers
> running concurrently on different CPUs cause a race condition in the rx offload
> framework?
> 
> If flexcan_irq_mb() and flexcan_irq_berr() fire simultaneously, both will
> eventually call into the CAN rx offload framework (e.g., via
> can_rx_offload_queue_timestamp()):
> 
> drivers/net/can/dev/rx-offload.c:can_rx_offload_queue_timestamp() {
>     ...
>     __skb_queue_add_sort(&offload->skb_irq_queue, skb,
>                          can_rx_offload_compare);
>     ...
> }
> 
> Since __skb_queue_add_sort() locklessly modifies skb_irq_queue, could
> concurrent execution of these handlers corrupt the sk_buff linked lists?

Yes, should be handled in another patch.

> 
> > +
> > +	if (handled)
> > +		can_rx_offload_irq_finish(&priv->offload);
> > +
> > +	return handled;
> > +}
> > +
> > +/* Device state change IRQ handler for multi-IRQ platforms */
> > +static irqreturn_t flexcan_irq_boff(int irq, void *dev_id)
> > +{
> 
> [ ... ]
> 
> > @@ -1761,25 +1850,30 @@ static int flexcan_open(struct net_device *dev)
> >  
> >  	can_rx_offload_enable(&priv->offload);
> >  
> > -	err = request_irq(dev->irq, flexcan_irq, IRQF_SHARED, dev->name, dev);
> > +	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_NR_IRQ_3)
> > +		err = request_irq(dev->irq, flexcan_irq_mb,
> > +				  IRQF_SHARED, dev->name, dev);
> > +	else
> > +		err = request_irq(dev->irq, flexcan_irq,
> > +				  IRQF_SHARED, dev->name, dev);
> 
> [ ... ]
> 
> >  	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
> >  		err = request_irq(priv->irq_secondary_mb,
> > -				  flexcan_irq, IRQF_SHARED, dev->name, dev);
> > +				  flexcan_irq_mb, IRQF_SHARED, dev->name, dev);
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does registering the same
> flexcan_irq_mb() handler for both primary and secondary MB IRQ lines leave the
> duplicate frame processing bug unfixed on platforms with secondary mailboxes?
> 
> If both IRQ lines fire concurrently on S32G2, flexcan_irq_mb() is dispatched to
> both CPUs. Since flexcan_do_mb() doesn't check which IRQ line triggered it,
> won't both CPUs unconditionally process all mailboxes, resulting in every
> frame being processed twice?

I think this is fixed by PATCH3.

Regards
Haibo Chen
> 
> >  		if (err)
> >  			goto out_free_irq_err;
> >  	}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260713085306.2643794-1-ciprianmarian.costea@oss.nxp.com?part=1