Re: [PATCH v2 2/2] can: tcan4x5x: Add suspend error handling

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The Fixes: tag points at the wrong commit.
- [Low] tcan4x5x_suspend() ignores the return value of
  enable_irq_wake(spi->irq).

--

can: tcan4x5x: Add suspend error handling

This patch adds an error path to tcan4x5x_suspend(). When
m_can_class_suspend() fails, the wake IRQ armed by enable_irq_wake() is
released with disable_irq_wake() before the error is returned.

> Fixes: b6b640c04446 ("can: tcan4x5x: support resuming from rx interrupt signal")

[Severity: Low]

Is this the right commit for the Fixes tag?

At b6b640c04446, m_can_class_suspend() ends with an unconditional
"return 0;". The new err_disable_irq path in tcan4x5x_suspend() could
never run against that version.

m_can_class_suspend() first became able to fail in ad1ddb3bfb0c ("can:
m_can: call deinit/init callback when going into suspend/resume"). That
commit added:

    ret = cdev->ops->deinit(cdev);

and changed the final statement to "return ret;". tcan4x5x is the only
m_can driver that sets .deinit (.deinit = tcan4x5x_deinit).

The previous patch in this series, "can: m_can: Fix suspend deinit()
error path", already uses Fixes: ad1ddb3bfb0c. With different tags,
stable tooling could pick this patch for trees that have b6b640c04446
but not ad1ddb3bfb0c. In those trees the new branch is dead code, and the
companion patch would not be picked.

Should this be Fixes: ad1ddb3bfb0c ("can: m_can: call deinit/init
callback when going into suspend/resume") instead?

Also, the commit message says "restore the previous state". Could it say
that the state being restored is only the enable_irq_wake() reference?

> diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
> index a5b8829aa5193..b69312413d627 100644
> --- a/drivers/net/can/m_can/tcan4x5x-core.c
> +++ b/drivers/net/can/m_can/tcan4x5x-core.c
> @@ -575,11 +575,20 @@ static int __maybe_unused tcan4x5x_suspend(struct device *dev)
>  {
>  	struct m_can_classdev *cdev = dev_get_drvdata(dev);
>  	struct spi_device *spi = to_spi_device(dev);
> +	int ret;
>  
>  	if (cdev->pm_wake_source)
>  		enable_irq_wake(spi->irq);
>  
> -	return m_can_class_suspend(dev);
> +	ret = m_can_class_suspend(dev);
> +	if (ret)
> +		goto err_disable_irq;
> +	return 0;
> +
> +err_disable_irq:
> +	if (cdev->pm_wake_source)
> +		disable_irq_wake(spi->irq);
> +	return ret;

[Severity: Low]

Can this disable_irq_wake() be unbalanced against the enable_irq_wake()
above it?

The return value of enable_irq_wake(spi->irq) is ignored. Suppose the
parent irqchip has no irq_set_wake callback and does not set
IRQCHIP_SKIP_SET_WAKE. Then set_irq_wake_real() returns -ENXIO, and
irq_set_irq_wake() resets the depth:

kernel/irq/manage.c:irq_set_irq_wake() {
    ...
		if (desc->wake_depth++ == 0) {
			ret = set_irq_wake_real(irq, on);
			if (ret)
				desc->wake_depth = 0;
    ...
}

m_can_class_suspend() can then also fail, for example when an SPI access
in tcan4x5x_deinit() fails. In that case this error path calls
disable_irq_wake() with wake_depth == 0, which hits:

			if (desc->wake_depth == 0) {
				WARN(1, "Unbalanced IRQ %d wake disable\n", irq);

So the rollback drops a wake reference that was never taken.

Ignoring the enable failure has a second effect. Suspend carries on with
the chip in RX-wake mode even though the SoC IRQ line was never armed as
a wake source.

tcan4x5x_resume() already calls disable_irq_wake() unconditionally, so
the pattern is older than this patch. This patch copies it into the new
rollback path.

Would it work to record whether enable_irq_wake() succeeded, or to
return its error before calling m_can_class_suspend()? Then
disable_irq_wake() would only be called when the wake reference was
actually taken.

>  }
>  
>  static int __maybe_unused tcan4x5x_resume(struct device *dev)

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