Re: [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] 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 <[email protected]>

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 <[email protected]>
Reviewed-by: Vincent Mailhol <[email protected]>
Reviewed-and-tested-by: Haibo Chen <[email protected]>
Tested-by: Enric Balletbo i Serra <[email protected]>

> 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?

> +
> +	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?

> +
> +	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?

>  		if (err)
>  			goto out_free_irq_err;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1