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

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

Pre-existing issues:
- [High] Concurrent execution of separate hardware interrupt handlers (`dev->irq` and `priv->irq_err`) modifies the lockless `can_rx_offload` IRQ queue (`skb_irq_queue`), leading to list corruption.
--

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

can: flexcan: add FLEXCAN_QUIRK_IRQ_BERR quirk

Introduce FLEXCAN_QUIRK_IRQ_BERR quirk to handle hardware integration
where the FlexCAN module has a dedicated interrupt line for signaling
bus errors and device state changes. This adds the flexcan_irq_esr() handler
and 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);
> +
> +	return handled;
> +}

[Severity: High]
This is a pre-existing issue, but could this lead to list corruption
if these separate IRQs run concurrently on different CPUs?

Since flexcan_irq_esr() and flexcan_irq_mb() are registered as separate
interrupts, they might fire at the same time. If flexcan_irq_esr()
processes a bus error and flexcan_irq_mb() processes a mailbox interrupt,
they both end up modifying the lockless offload->skb_irq_queue.

CPU1 handling dev->irq (flexcan_irq_mb):
flexcan_irq_mb()
  can_rx_offload_irq_offload_timestamp()
    __skb_queue_add_sort(&offload->skb_irq_queue, ...)

CPU2 handling priv->irq_err (flexcan_irq_esr):
flexcan_irq_esr()
  flexcan_do_berr()
    flexcan_irq_bus_err()
      can_rx_offload_queue_timestamp()
        __skb_queue_add_sort(&offload->skb_irq_queue, ...)

Furthermore, both handlers unconditionally call can_rx_offload_irq_finish(),
which locklessly splices the skb_irq_queue list:

can_rx_offload_irq_finish()
  skb_queue_splice_tail_init(&offload->skb_irq_queue, ...)

Are there existing synchronization mechanisms preventing these IRQ
handlers from running simultaneously and corrupting the lockless
skb_irq_queue list pointers?

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