Re: [RESEND PATCH v6 2/6] can: flexcan: disable all IRQ lines in flexcan_chip_interrupts_enable()

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

Pre-existing issues:
- [High] Prematurely exposing the device to the network stack during resume, and delaying teardown during suspend, causes a race condition where ndo_start_xmit can interact with powered-down or partially initialized hardware.
- [Medium] PM runtime usage count underflow (double-put) in the probe error path.
--

--- Patch [2]: [RESEND PATCH v6 2/6] can: flexcan: disable all IRQ lines in flexcan_chip_interrupts_enable() ---
commit 51a052134a222a737e75062314cf44dff20da306
Author: Ciprian Marian Costea <[email protected]>

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

flexcan_chip_interrupts_enable() disables only the primary IRQ line while
writing to the IMASK and CTRL registers.

On multi-IRQ platforms (S32G2, MCF5441X), the additional IRQ lines (boff,
err, secondary-mb) remain active so their handlers can fire while
registers are inconsistent.

Disable all registered IRQ lines around the IMASK/CTRL writes. This
also fixes the resume path, which calls this function.

> 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;

[ ... ]

This isn't a bug introduced by this patch, but while reviewing flexcan-core.c
I noticed a PM runtime count underflow in the probe error path:

drivers/net/can/flexcan/flexcan-core.c:flexcan_probe() {
    ...
	err = register_flexcandev(dev);
	if (err) {
		dev_err(&pdev->dev, "registering netdev failed\n");
		goto failed_register;
	}

	err = flexcan_setup_stop_mode(pdev);
	if (err < 0) {
		dev_err_probe(&pdev->dev, err, "setup stop mode failed\n");
		goto failed_setup_stop_mode;
	}
    ...
 failed_setup_stop_mode:
	unregister_flexcandev(dev);
 failed_register:
	pm_runtime_put_noidle(&pdev->dev);
    ...
}

[Severity: Medium]
Since register_flexcandev() successfully drops the PM runtime reference via
pm_runtime_put() on success, doesn't jumping to failed_setup_stop_mode and
falling through to failed_register result in a double-put when
pm_runtime_put_noidle() is called?


This is also a pre-existing issue, but the suspend/resume handling appears
to have a race window with the network stack:

drivers/net/can/flexcan/flexcan-core.c:flexcan_resume() {
    ...
	if (netif_running(dev)) {
		netif_device_attach(dev);
		netif_start_queue(dev);
		if (device_may_wakeup(device)) {
			disable_irq_wake(dev->irq);
			err = flexcan_exit_stop_mode(priv);
    ...
}

[Severity: High]
Could this sequence prematurely expose the device to the network stack?
Calling netif_device_attach() and netif_start_queue() before hardware
initialization like flexcan_exit_stop_mode() or flexcan_chip_start()
completes could allow ndo_start_xmit to write to device registers while the
device is not powered.

Similarly in suspend:

drivers/net/can/flexcan/flexcan-core.c:flexcan_suspend() {
    ...
		} else {
			err = flexcan_chip_stop(dev);
			if (err)
				return err;
			flexcan_chip_interrupts_disable(dev);
			err = flexcan_transceiver_disable(priv);
			if (err)
				return err;
			err = pinctrl_pm_select_sleep_state(device);
			if (err)
				return err;
		}
		netif_stop_queue(dev);
		netif_device_detach(dev);
    ...
}

[Severity: High]
Does this tear down the hardware before stopping the queue? netif_stop_queue()
and netif_device_detach() occur after the hardware is suspended via
flexcan_chip_stop(), creating a similar race window.

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