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

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.

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