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

On Fri, Aug 14, 2026 at 09:20:27AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>
> New issues:
> - [High] The split IRQ handlers break the FLEXCAN_QUIRK_BROKEN_PERR_STATE workaround on multi-IRQ platforms.

Please check this one, You can fix pre-existing issue later.

Frank

>
> Pre-existing issues:
> - [Critical] Concurrent execution of split IRQ handlers locklessly corrupts the shared `skb_irq_queue`.
> - [High] The patch fails to fix duplicate frame processing for platforms with multiple mailbox IRQs.
> --
>
> --- Patch [1]: [RESEND PATCH v6 1/6] can: flexcan: use dedicated IRQ handlers for multi-IRQ platforms ---
> commit 4696683063946bcc95216f931b7fa509b55610f8
> Author: Ciprian Marian Costea <[email protected]>
> Date:   Fri Aug 14 09:03:18 2026 +0000
>
>     can: flexcan: use dedicated IRQ handlers for multi-IRQ platforms
>
> This commit splits the monolithic flexcan IRQ handler into dedicated
> handlers for mailbox, bus error, and state change events. This is intended
> to prevent duplicate frame processing on multi-IRQ platforms where distinct
> IRQ lines can dispatch concurrently.
>
> > 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
> [ ... ]
> > @@ -1131,25 +1129,57 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
> >  		netif_wake_queue(dev);
> >  	}
> >
> > +	return handled;
> > +}
> > +
> > +/* Process bus error events */
> > +static irqreturn_t flexcan_do_berr(struct net_device *dev)
> > +{
> > +	struct flexcan_priv *priv = netdev_priv(dev);
> > +	struct flexcan_regs __iomem *regs = priv->regs;
> > +	irqreturn_t handled = IRQ_NONE;
> > +	u32 reg_esr;
> > +
> >  	reg_esr = priv->read(&regs->esr);
> >
> > -	/* ACK all bus error, state change and wake IRQ sources */
> > -	if (reg_esr & (FLEXCAN_ESR_ALL_INT | FLEXCAN_ESR_WAK_INT)) {
> > +	/* ACK bus error interrupt source */
> > +	if (reg_esr & FLEXCAN_ESR_ERR_INT) {
> >  		handled = IRQ_HANDLED;
> > -		priv->write(reg_esr & (FLEXCAN_ESR_ALL_INT | FLEXCAN_ESR_WAK_INT), &regs->esr);
> > +		priv->write(FLEXCAN_ESR_ERR_INT, &regs->esr);
> >  	}
> >
> > -	/* state change interrupt or broken error state quirk fix is enabled */
> > -	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);
> > -
> >  	/* 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);
> >
> > +	return handled;
> > +}
>
> [Severity: High]
> Does this break the FLEXCAN_QUIRK_BROKEN_PERR_STATE workaround?
>
> Before this patch, the monolithic flexcan_irq() checked this quirk and called
> flexcan_irq_state() on any interrupt.
>
> The FLEXCAN_QUIRK_BROKEN_PERR_STATE workaround relies on enabling the bus error
> interrupt to catch state changes on platforms where the state interrupt is
> broken.
>
> Now that the bus error interrupt is routed exclusively to flexcan_do_berr(),
> which never invokes flexcan_irq_state(), won't state transitions (like bus
> warning or bus-off) be silently ignored on affected platforms?
>
> [ ... ]
> > +/* 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]
> This is a pre-existing issue, but does this still allow duplicate frame
> processing?
>
> If a platform has a secondary mailbox IRQ (FLEXCAN_QUIRK_SECONDARY_MB_IRQ),
> flexcan_open() registers flexcan_irq_mb() for the secondary IRQ, and the
> primary IRQ registers either flexcan_irq() or flexcan_irq_mb().
>
> Because these can run concurrently on different CPUs, and flexcan_do_mb()
> unconditionally processes all active mailboxes globally, won't both threads
> still race and process the same frames?
>
> > +
> > +	if (handled)
> > +		can_rx_offload_irq_finish(&priv->offload);
>
> [Severity: Critical]
> This is a pre-existing issue, but does splitting the IRQ handlers make
> skb_irq_queue vulnerable to lockless corruption?
>
> When flexcan_irq_mb(), flexcan_irq_boff(), and flexcan_irq_berr() run
> concurrently on different CPUs, they can independently call the can_rx_offload
> API (like can_rx_offload_irq_offload_timestamp() and
> can_rx_offload_queue_timestamp()).
>
> These functions insert SKBs into offload->skb_irq_queue using the lockless
> __skb_queue_add_sort().
>
> Additionally, they concurrently call can_rx_offload_irq_finish(), which
> splices the list locklessly:
>
> drivers/net/can/dev/rx-offload.c:can_rx_offload_irq_finish() {
>     ...
>     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);
>     ...
> }
>
> Will this concurrent lockless access corrupt the skb->next / skb->prev pointers
> leading to a kernel panic?
>
> > +
> > +	return handled;
> > +}
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1