Re: [PATCH v2 1/2] can: m_can: Fix suspend deinit() error path

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new err_pm_wake_source restore path in
  m_can_class_suspend() (drivers/net/can/m_can/m_can.c:2638-2645) has no…
- [Medium] The new error path in m_can_class_suspend() restores M_CAN_IE
  and the coalescing hrtimer, reattaches the netdev and restarts the TX…

Pre-existing issues:
- [Low] Unbalanced enable_irq_wake() when suspend fails.

--

can: m_can: Fix suspend deinit() error path

This patch handles a failure of the deinit() callback in
m_can_class_suspend() when the device is a wake source. On failure it
writes the cached interrupt enable mask back to M_CAN_IE and restarts the
coalescing hrtimer if coalescing was active. It then reattaches the netdev
and restarts the TX queue.

> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index 16f80607e150e..02a95a769a784 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -2612,8 +2612,14 @@ int m_can_class_suspend(struct device *dev)
>  			hrtimer_cancel(&cdev->hrtimer);
>  			m_can_write(cdev, M_CAN_IE, IR_RF0N);
>  
> -			if (cdev->ops->deinit)
> +			if (cdev->ops->deinit) {
>  				ret = cdev->ops->deinit(cdev);
> +				if (ret) {
> +					netdev_err(cdev->net, "failed to deinit device while suspending %pe\n",
> +						   ERR_PTR(ret));
> +					goto err_pm_wake_source;
> +				}
> +			}
>  		} else {
>  			m_can_stop(ndev);
>  		}

[ ... ]

> +	return 0;
> +
> +err_pm_wake_source:
> +	if (netif_running(ndev)) {
> +		if (cdev->pm_wake_source) {
> +			bool coalescing_active = (cdev->active_interrupts & (IR_RF0N | IR_TEFN))
> +						 != (IR_RF0N | IR_TEFN);
> +			m_can_write(cdev, M_CAN_IE, cdev->active_interrupts);
> +			if (coalescing_active)
> +				hrtimer_start(&cdev->hrtimer,
> +					      cdev->irq_timer_wait,
> +					      HRTIMER_MODE_REL);

[Severity: Medium]
Can this restore race with the threaded IRQ handler?

The forward path leaves M_CAN_IE set to IR_RF0N, and the IRQ is still live
during ->suspend. In that state m_can_coalescing_update() does a lockless
read-modify-write of the same cache, the same register and the same
hrtimer:

m_can_coalescing_update() {
	u32 new_interrupts = cdev->active_interrupts;
	...
	if (!enable_rx_timer && !hrtimer_active(&cdev->hrtimer))
		new_interrupts |= IR_RF0N;
	if (!enable_tx_timer && !hrtimer_active(&cdev->hrtimer))
		new_interrupts |= IR_TEFN;

	m_can_interrupt_enable(cdev, new_interrupts);
	...
}

With the tcan4x5x SPI regmap, one possible interleaving is:

suspend error path                  IRQ thread
reads active_interrupts (A_old,
  RF0N/TEFN cleared)
m_can_write(IE, A_old) waits on
  the regmap lock
                                    m_can_coalescing_update()
                                      timer was cancelled, so
                                      RF0N/TEFN are ORed in (A_new)
                                      writes IE = A_new
                                      active_interrupts = A_new
m_can_write(IE, A_old) completes

After this, the hardware masks RF0N or TEFN, but the cache says they are
enabled. m_can_interrupt_enable() then short-circuits:

	if (cdev->active_interrupts == interrupts)
		return;

So later updates that compute A_new, including the one woken by the
restarted hrtimer through m_can_coalescing_timer(), never rewrite IE.

Could RX FIFO or TX event interrupts stay masked after the netdev is
reattached here, until something else changes the cached mask?

> +		}
> +		netif_device_attach(ndev);
> +		netif_start_queue(ndev);

[Severity: Medium]
Does this rollback leave the TCAN4x5x in standby mode?

tcan4x5x_deinit() is a single read-modify-write on the SPI regmap:

	return regmap_update_bits(tcan4x5x->regmap, TCAN4X5X_CONFIG,
				  TCAN4X5X_MODE_SEL_MASK, TCAN4X5X_MODE_STANDBY);

Suppose the read succeeds and the write reports an error after the frame
has already been clocked out to the chip. The device may then have latched
STANDBY even though deinit() returned an error.

The error path restores IE, the hrtimer and the netdev state, but it never
sets TCAN4X5X_MODE_NORMAL again the way tcan4x5x_init() does:

	ret = regmap_update_bits(tcan4x5x->regmap, TCAN4X5X_CONFIG,
				 TCAN4X5X_MODE_SEL_MASK, TCAN4X5X_MODE_NORMAL);

In that case the interface would look operational, but frames would sit in
the TX FIFO until the next ifdown/ifup or suspend/resume.

This depends on whether a failed SPI write can still reach the device. If
the read fails, no write is issued and the mode is unchanged.

> +	}
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(m_can_class_suspend);

[Severity: Low]
This isn't a bug introduced by this patch, but returning an error here
still leaves the wake IRQ armed on tcan4x5x. tcan4x5x_suspend() does:

	if (cdev->pm_wake_source)
		enable_irq_wake(spi->irq);

	return m_can_class_suspend(dev);

If m_can_class_suspend() fails, the PM core does not call
tcan4x5x_resume() for this device. That is the only place that calls
disable_irq_wake(), so each failed suspend leaks one wake reference on
spi->irq.

The same imbalance existed before this patch, because the deinit() error
was already returned without this cleanup.

The next patch in this series, "can: tcan4x5x: Add suspend error
handling", adds an err_disable_irq label to tcan4x5x_suspend() that calls
disable_irq_wake() when m_can_class_suspend() fails, which looks like it
fixes this.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261016-v7-3-topic-mcan-suspend-fix-fix-v2-0-7efa2b172cce%40baylibre.com