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

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

[Markus Schneider-Pargmann (TI)]

Hi,

this small series tries to make the suspend error path of m_can a bit
better.

Best,
Markus

Signed-off-by: Markus Schneider-Pargmann (TI) <msp@baylibre.com>
---
Changes in v2:
- Add a patch for tcan suspend
- Start the hrtimer if coalescing is active and suspend is cancelled
  instead of modifying active_interrupts
- Link to v1: https://patch.msgid.link/20260918-v7-3-topic-mcan-suspend-fix-fix-v1-1-e24fa70c754e@baylibre.com

To: Markus Schneider-Pargmann <msp@baylibre.com>
To: Marc Kleine-Budde <mkl@pengutronix.de>
To: Vincent Mailhol <mailhol@kernel.org>
To: Sean Nyekjaer <sean@geanix.com>
To: Martin Hundebøll <martin@geanix.com>
Cc: linux-can@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---
Markus Schneider-Pargmann (1):
      can: tcan4x5x: Add suspend error handling

Markus Schneider-Pargmann (TI) (1):
      can: m_can: Fix suspend deinit() error path

 drivers/net/can/m_can/m_can.c         | 25 ++++++++++++++++++++++++-
 drivers/net/can/m_can/tcan4x5x-core.c | 11 ++++++++++-
 2 files changed, 34 insertions(+), 2 deletions(-)
---
base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
change-id: 20260914-v7-3-topic-mcan-suspend-fix-fix-b943929d8fef

Best regards,
--  
Markus Schneider-Pargmann <msp@baylibre.com>

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

[Markus Schneider-Pargmann (TI)]

When suspending mcan, deinit is called and its return value is returned,
but nothing is restored. Returning an error in the suspend function will
stop suspending and resume the system immediately. So on error the
device should be restored to its previous state. If coalescing is
active, the hrtimer is restarted.

Fixes: ad1ddb3bfb0c ("can: m_can: call deinit/init callback when going into suspend/resume")
Signed-off-by: Markus Schneider-Pargmann (TI) <msp@baylibre.com>
---
 drivers/net/can/m_can/m_can.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 16f80607e150..02a95a769a78 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);
 		}
@@ -2625,6 +2631,23 @@ int m_can_class_suspend(struct device *dev)
 	if (!m_can_class_wakeup_pinctrl_enabled(cdev))
 		pinctrl_pm_select_sleep_state(dev);
 
+	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);
+		}
+		netif_device_attach(ndev);
+		netif_start_queue(ndev);
+	}
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(m_can_class_suspend);

-- 
2.55.0

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

[netdev-bot+sashiko]

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

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

[Markus Schneider-Pargmann]

m_can_class_suspend() may return errors. On error we should first
restore the previous state and then pass the error code on. Otherwise we
keep the device in a misconfigured state for the running state.

Fixes: b6b640c04446 ("can: tcan4x5x: support resuming from rx interrupt signal")
Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
 drivers/net/can/m_can/tcan4x5x-core.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
index a5b8829aa519..b69312413d62 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;
 }
 
 static int __maybe_unused tcan4x5x_resume(struct device *dev)

-- 
2.55.0

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

[Martin Hundebøll]

Hi Markus,

On Fri, 2026-10-16 at 11:57 +0200, Markus Schneider-Pargmann wrote:
> diff --git a/drivers/net/can/m_can/tcan4x5x-core.c
> b/drivers/net/can/m_can/tcan4x5x-core.c
> index a5b8829aa519..b69312413d62 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;

Wouldn't this be better written in the shorter form:

	ret = m_can_class_suspend(dev);
	if (ret && dev->pm_wake_source)
		disable_irq_wake(spi->irq);

	return ret;

?

// Martin

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

[netdev-bot+sashiko]

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