Re: [PATCH v5 1/8] can: flexcan: use dedicated IRQ handlers for multi-IRQ platforms

[sashiko-bot@kernel.org]

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

New issues:
- [High] Splitting the IRQ handlers breaks `FLEXCAN_QUIRK_BROKEN_PERR_STATE` polling and strands state change SKBs.

Pre-existing issues:
- [Critical] Concurrent execution of split IRQ handlers corrupts the lockless CAN `rx_offload` queue.
--

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

can: flexcan: use dedicated IRQ handlers for multi-IRQ platforms

This patch splits the monolithic flexcan IRQ handler into focused parts
and introduces dedicated IRQ handlers for multi-IRQ platforms to prevent
duplicate frame processing.

> 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)
[ ... ]
> +/* 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]
By splitting the handler, does this break the FLEXCAN_QUIRK_BROKEN_PERR_STATE
polling mechanism? Before this patch, the monolithic flexcan_irq() naturally
polled flexcan_do_state() during mailbox interrupts. Now that flexcan_irq_mb()
bypasses flexcan_do_state(), it seems state transitions might no longer be
polled during active CAN traffic.

> +
> +	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 assigning dedicated handlers to
different IRQ lines lead to concurrent modifications of the lockless
rx_offload queue?

Since flexcan_irq_mb() and flexcan_irq_berr() can run concurrently on
different CPUs, flexcan_do_mb() and flexcan_do_berr() might concurrently
call can_rx_offload_queue_timestamp().

That function uses __skb_queue_add_sort(), which is a lockless primitive
that assumes strictly serialized execution. Could this lead to doubly-linked
list corruption and kernel panics, further exacerbated by concurrent unlocked
splices in can_rx_offload_irq_finish()?

> +
> +	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)
> +{
> +	struct net_device *dev = dev_id;
> +	struct flexcan_priv *priv = netdev_priv(dev);
> +	irqreturn_t handled;
> +
> +	handled = flexcan_do_state(dev);
> +
> +	if (handled)
> +		can_rx_offload_irq_finish(&priv->offload);

[Severity: High]
If flexcan_do_state() queues an SKB but returns IRQ_NONE (e.g., if no ESR
bits are set), will this skip the can_rx_offload_irq_finish() call and
strand the state change SKB in the lockless queue?

> +
> +	return handled;
> +}

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