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

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Wakeup interrupt is not configured for the new dedicated ESR IRQ during suspend.

Pre-existing issues:
- [Critical] Concurrent execution of separate IRQ handlers corrupts the lockless rx-offload SKB queue.
--

commit 53e92dd64634f7bc681782eddd379791f6533960
Author: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>

can: flexcan: add FLEXCAN_QUIRK_IRQ_BERR quirk

This commit introduces the FLEXCAN_QUIRK_IRQ_BERR quirk for platforms where bus
errors and state changes share a dedicated IRQ line. It adds a combined IRQ
handler flexcan_irq_esr() and updates interrupt setup and teardown to manage
the new line.

> diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
> index f28ff0e95b74f..601d88bfcfeb2 100644
> --- a/drivers/net/can/flexcan/flexcan-core.c
> +++ b/drivers/net/can/flexcan/flexcan-core.c

[ ... ]

> @@ -1303,6 +1303,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);
> +
> +	return handled;
> +}

[Severity: Critical]
This is a pre-existing issue, but adding this separate IRQ handler highlights
a race condition. Can this new handler and the main mailbox IRQ handler
execute concurrently on different CPUs?

If dev->irq and priv->irq_err are dispatched to different CPUs simultaneously,
both handlers can call into the Rx offload functions in rx-offload.c. For
example:

CPU1 (in flexcan_irq_mb):
  can_rx_offload_irq_offload_timestamp()
    __skb_queue_add_sort(&offload->skb_irq_queue, ...)

CPU2 (in flexcan_irq_esr):
  handled |= flexcan_do_berr(dev);
    flexcan_irq_bus_err()
      can_rx_offload_queue_timestamp()
        __skb_queue_add_sort(&offload->skb_irq_queue, ...)

Since the Rx offload queue manipulation in __skb_queue_add_sort() is lockless,
could this concurrent execution corrupt the skb_irq_queue list and crash the
kernel? Similarly, both paths call can_rx_offload_irq_finish() which splices
the queue locklessly from the IRQ queue side without adequate mutual exclusion
against other HardIRQs.

[ ... ]

> @@ -1905,6 +1922,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);
> +		if (err)
> +			goto out_free_irq_boff;
> +	}
> +

[Severity: Medium]
Since this quirk routes the wakeup event (WAK_INT) to this new dedicated ESR
IRQ line, should priv->irq_err be configured for wakeup during suspend?

Currently, flexcan_suspend() calls enable_irq_wake(dev->irq), but it does
not arm priv->irq_err. Will the system fail to wake up from sleep upon CAN
activity on platforms using this quirk because the interrupt controller isn't
armed for the correct line?

Should flexcan_suspend() and flexcan_resume() be updated to handle
priv->irq_err as well?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831143449.12828-1-ciprianmarian.costea@oss.nxp.com?part=5