Re: [PATCH RESEND v4 3/8] can: flexcan: split rx/tx masks per mailbox IRQ line

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Concurrent execution of mailbox IRQ handlers corrupts the lockless `skb_irq_queue` list and tears network statistics.
--

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

can: flexcan: split rx/tx masks per mailbox IRQ line

This patch splits rx and tx masks per mailbox IRQ line for platforms like
S32G2 which have multiple mailbox IRQs. It introduces a struct to hold these
masks and updates the IRQ handlers to use the correct subset, preventing both
handlers from processing the full mask range.

> diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
> index 7dde2e623defe..32e4d4da00a1c 100644
> --- a/drivers/net/can/flexcan/flexcan-core.c
> +++ b/drivers/net/can/flexcan/flexcan-core.c
[ ... ]
> @@ -1243,9 +1247,15 @@ static irqreturn_t flexcan_irq_mb(int irq, void *dev_id)
>  {
>  	struct net_device *dev = dev_id;
>  	struct flexcan_priv *priv = netdev_priv(dev);
> +	const struct flexcan_mb_irq *mb_irq;
>  	irqreturn_t handled;
> +	int idx;
>  
> -	handled = flexcan_do_mb(dev);
> +	idx = (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ &&
> +	       irq == priv->irq_secondary_mb) ? 1 : 0;
> +	mb_irq = &priv->mb_irq[idx];
> +
> +	handled = flexcan_do_mb(dev, mb_irq);

[Severity: High]
This isn't a bug introduced by this patch, but since the S32G2 platform
supports multiple mailbox IRQ lines (mb-0 and mb-1), can these handlers execute
concurrently on different CPUs?

Both handlers invoke flexcan_do_mb(), which eventually calls
can_rx_offload_irq_offload_timestamp() using the shared priv->offload
structure. This performs a lockless list insertion:

drivers/net/can/dev/rx-offload.c:can_rx_offload_irq_offload_timestamp() {
    ...
    __skb_queue_add_sort(&offload->skb_irq_queue, skb,
                         can_rx_offload_compare);
    ...
}

Because __skb_queue_add_sort() does not take internal locks, concurrent
execution by multiple hardirq handlers could corrupt the list pointers.

>  
>  	if (handled)
>  		can_rx_offload_irq_finish(&priv->offload);

Furthermore, one handler might be concurrently splicing the list while the
other is inserting:

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);
    ...
}

Does the driver need to ensure mutual exclusion between the mailbox IRQ
handlers when accessing the shared skb_irq_queue?

>  
>  	return handled;
>  }

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