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

[sashiko-bot@kernel.org]

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