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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Concurrent multi-IRQ execution corrupts `can_rx_offload` lockless SKB queues.
- [High] Wake-on-CAN is broken on multi-IRQ platforms because `enable_irq_wake` is not called for the dedicated state/error IRQ line.
--

--- Patch [5]: [RESEND PATCH v6 5/6] can: flexcan: add FLEXCAN_QUIRK_IRQ_BERR quirk ---
commit ec638df99febf4382988e54906857f08c9de34b2
Author: Ciprian Marian Costea <[email protected]>

can: flexcan: add FLEXCAN_QUIRK_IRQ_BERR quirk
    
This commit introduces the FLEXCAN_QUIRK_IRQ_BERR quirk for platforms with a
dedicated IRQ line for bus errors and state changes. It adds the
flexcan_irq_esr() handler for these events and updates interrupt enable and
disable paths to handle the new IRQ line. The change 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);

[Severity: High]
This is a pre-existing issue, but does the concurrent execution of multiple
IRQ handlers corrupt the lockless SKB queues?

In an SMP system, if flexcan_irq_mb() (processing a mailbox message) and
flexcan_irq_esr() (processing a bus error) execute concurrently on different
CPUs, they will both push to the same lockless offload queue.

Since the can_rx_offload framework relies on the implicit synchronization
of a single IRQ line, executing these handlers on distinct IRQs could
cause simultaneous lockless modifications to offload->skb_irq_queue, leading
to linked list corruption.

> +
> +	return handled;
> +}

[ ... ]

> @@ -1902,6 +1919,13 @@ static int flexcan_open(struct net_device *dev)
>  			goto out_free_irq_boff;
>  	}
>  
> +	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_IRQ_BERR) {
> +		err = request_irq(priv->irq_err,
> +				  flexcan_irq_esr, IRQF_SHARED, dev->name, dev);

[Severity: High]
This isn't a bug introduced by this patch, but does Wake-on-CAN fail to wake
the system on multi-IRQ platforms?

Looking at flexcan_suspend(), it only arms the primary mailbox IRQ for wakeup:

drivers/net/can/flexcan/flexcan-core.c:flexcan_suspend() {
    ...
    if (device_may_wakeup(device)) {
        enable_irq_wake(dev->irq);
        err = flexcan_enter_stop_mode(priv);
    ...
}

However, wake events (FLEXCAN_ESR_WAK_INT) are processed by
flexcan_do_state(), which is routed through this dedicated state/error IRQ
line (priv->irq_err). Because enable_irq_wake() is not called on the
secondary IRQ lines, will the interrupt controller ignore the wakeup signal?

> +		if (err)
> +			goto out_free_irq_boff;
> +	}

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