Re: [PATCH v5 2/8] can: flexcan: disable all IRQ lines in flexcan_chip_interrupts_enable()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The bus-off recovery worker (`can_restart_work`) can execute concurrently with the hardware teardown in `flexcan_close()`, leading to an unhandled interrupt storm on a shared IRQ line.
--

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

can: flexcan: disable all IRQ lines in flexcan_chip_interrupts_enable()

This commit modifies flexcan_chip_interrupts_enable() to disable all registered
IRQ lines around IMASK and CTRL register writes. Previously, only the primary
IRQ line was disabled, which left additional multi-IRQ lines active and could
allow handlers to fire while registers were inconsistent.

> diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
> index f73ff442d5304..7dde2e623defe 100644
> --- a/drivers/net/can/flexcan/flexcan-core.c
> +++ b/drivers/net/can/flexcan/flexcan-core.c
> @@ -1519,14 +1519,28 @@ static void flexcan_chip_interrupts_enable(const struct net_device *dev)
>  {
>  	const struct flexcan_priv *priv = netdev_priv(dev);
>  	struct flexcan_regs __iomem *regs = priv->regs;
> +	u32 quirks = priv->devtype_data.quirks;
>  	u64 reg_imask;
>  
>  	disable_irq(dev->irq);
> +	if (quirks & FLEXCAN_QUIRK_NR_IRQ_3) {
> +		disable_irq(priv->irq_boff);
> +		disable_irq(priv->irq_err);
> +	}
> +	if (quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ)
> +		disable_irq(priv->irq_secondary_mb);
> +
>  	priv->write(priv->reg_ctrl_default, &regs->ctrl);
>  	reg_imask = priv->rx_mask | priv->tx_mask;
>  	priv->write(upper_32_bits(reg_imask), &regs->imask2);
>  	priv->write(lower_32_bits(reg_imask), &regs->imask1);
>  	enable_irq(dev->irq);
> +	if (quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ)
> +		enable_irq(priv->irq_secondary_mb);
> +	if (quirks & FLEXCAN_QUIRK_NR_IRQ_3) {
> +		enable_irq(priv->irq_boff);
> +		enable_irq(priv->irq_err);
> +	}
>  }

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for
flexcan_chip_interrupts_enable() to be called after flexcan_close() has
already freed the IRQs?

If the bus-off recovery worker (can_restart_work) runs concurrently with the
hardware teardown in flexcan_close(), could it trigger an interrupt storm?

flexcan_close() frees the requested IRQs and stops the chip, and then calls
close_candev():

drivers/net/can/flexcan/flexcan-core.c:flexcan_close() {
	...
	free_irq(dev->irq, dev);
	can_rx_offload_disable(&priv->offload);
	flexcan_chip_stop_disable_on_error(dev);

	can_rx_offload_del(&priv->offload);
	flexcan_transceiver_disable(priv);
	close_candev(dev);
	...
}

Because close_candev() is where cancel_delayed_work_sync(&priv->restart_work)
is called, the worker can execute while flexcan_close() is running.

If the worker executes its hardware initialization after free_irq() is called
but before close_candev() cancels it:

can_restart_work()->flexcan_set_mode()->flexcan_chip_interrupts_enable()

Will this re-enable chip-level interrupts while no handler is registered,
permanently disabling the shared IRQ line via the kernel's spurious interrupt
detector?

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