[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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] can: m_can: suspend error path permanently stalls polling mode
- [Medium] can: m_can: race in suspend error path leads to permanent rx stall
--

commit 483c34ecb4b606d88b805473f5b905d713840725
Author: Markus Schneider-Pargmann (TI) <msp@baylibre.com>

can: m_can: Fix suspend deinit() error path

When suspending mcan, deinit is called and its return value is returned,
but nothing is restored. This commit adds an error path to restore the
device to its previous state, including restarting the coalescing hrtimer.

> 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
[ ... ]
> @@ -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);

[Severity: High]
If the device operates in polling mode (!cdev->net->irq) and acts as a wake
source (cdev->pm_wake_source == true), does this error path permanently
stall the polling mode?

The tcan4x5x driver allows pm_wake_source to be true even if spi->irq is 0
(polling mode). In polling mode, all active interrupt bits (IR_ALL_INT) are
retained, so coalescing_active will evaluate to false. As a result, the
polling timer cancelled earlier in suspend will never be restarted here.

Additionally, if the timer were to be restarted, would using the coalescing
parameters (cdev->irq_timer_wait, HRTIMER_MODE_REL) be correct, or should it
use the polling parameters (HRTIMER_POLL_INTERVAL_MS,
HRTIMER_MODE_REL_PINNED)?

> +			m_can_write(cdev, M_CAN_IE, cdev->active_interrupts);

[Severity: Medium]
Could this non-atomic read and write introduce a race condition leading to
a permanent RX stall?

Earlier in m_can_class_suspend(), m_can_write(cdev, M_CAN_IE, IR_RF0N)
enables RX wakeups, which allows a concurrent threaded ISR to run if an
RX packet arrives.

If the ISR coalescing timer expires exactly between evaluating
coalescing_active (reading cdev->active_interrupts) and writing it to
M_CAN_IE here, the hardware could be overwritten with a stale mask that
leaves RX disabled.

If the error path then restarts the timer, the ISR's next execution
might see the software state already has RX enabled and skip updating the
hardware, leaving the hardware and software states permanently out of sync.

> +			if (coalescing_active)
> +				hrtimer_start(&cdev->hrtimer,
> +					      cdev->irq_timer_wait,
> +					      HRTIMER_MODE_REL);
> +		}
> +		netif_device_attach(ndev);
> +		netif_start_queue(ndev);
> +	}
> +
>  	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016-v7-3-topic-mcan-suspend-fix-fix-v2-0-7efa2b172cce@baylibre.com?part=1

[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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] irq: unbalanced IRQ wake disable on suspend error path
--

commit 35cb9abaa9dd12aabf550562b5fe36f26129d629
Author: Markus Schneider-Pargmann <msp@baylibre.com>

can: tcan4x5x: Add suspend error handling

This commit adds error handling to tcan4x5x_suspend() by checking the
return value of m_can_class_suspend(). If the suspend fails, it restores
the IRQ wake state before returning the error.

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

[Severity: Low]
Will this trigger an unbalanced IRQ wake disable warning if
enable_irq_wake() fails?

The original code calls enable_irq_wake() but ignores its return value.
If the underlying IRQ controller does not support wake settings (e.g.
returning -ENXIO), wake_depth is not incremented.

If m_can_class_suspend() subsequently fails, the new error path calls
disable_irq_wake() unconditionally.  If enable_irq_wake() had previously
failed, wouldn't this underflow wake_depth and trigger a WARN from the IRQ
core?

Could we check the return value of enable_irq_wake() and only call
disable_irq_wake() if it actually succeeded?

> +	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016-v7-3-topic-mcan-suspend-fix-fix-v2-0-7efa2b172cce@baylibre.com?part=2