Re: [PATCH net 12/22] can: m_can: m_can_class_suspend(): fix suspend deinit() error path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] can: m_can: data race on active_interrupts between suspend error path and ISR
- [Medium] can: tcan4x5x: unbalanced enable_irq_wake() when m_can_class_suspend() fails
- [Medium] can: m_can: polling hrtimer is permanently disabled on suspend failure
--

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

can: m_can: m_can_class_suspend(): fix suspend deinit() error path

This commit modifies m_can_class_suspend() to restore the device to its
previous state and return an error if deinit() fails. Previously, a
deinit() failure would return an error but not restore the interface.

Link: https://patch.msgid.link/20260918-v7-3-topic-mcan-suspend-fix-fix-v1-1-e24fa70c754e@baylibre.com

> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index 16f80607e150e..91a0c5eca260c 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_restore_interface;
> +				}
> +			}

[Severity: Medium]
Will this error return cause issues for callers that blindly return the result
without cleaning up on error?

For example, in drivers/net/can/m_can/tcan4x5x-core.c:tcan4x5x_suspend():

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

    return m_can_class_suspend(dev);

If m_can_class_suspend() returns an error here, the PM core aborts the system
suspend and skips calling tcan4x5x_resume() for this device. Does this leave
the IRQ wake_depth counter unbalanced since disable_irq_wake() is never
called, potentially triggering a WARN_ON in the IRQ core and breaking system
wakeup functionality?

[ ... ]

> +	return 0;
> +
> +err_restore_interface:
> +	if (netif_running(ndev)) {
> +		if (cdev->pm_wake_source) {
> +			/* Enable interrupts that trigger immediately if
> +			 * something is there and keep the hrtimer off
> +			 */
> +			cdev->active_interrupts |= IR_RF0N | IR_TEFN;

[Severity: High]
Can this read-modify-write operation corrupt the software-cached interrupt mask
cdev->active_interrupts?

Because suspend_device_irqs() is not called by the PM core until the
suspend_late phase, hardware interrupts are still fully enabled here (and
IR_RF0N was just explicitly re-enabled earlier in this function).

If a hardware interrupt fires concurrently, m_can_isr() runs and calls
m_can_interrupt_enable(), which performs an unprotected assignment:

drivers/net/can/m_can/m_can.c:m_can_interrupt_enable() {
    ...
    cdev->active_interrupts = interrupts;
    ...
}

Will this concurrent modification result in a data race, leading to permanent
misconfiguration of the hardware M_CAN_IE register (such as permanently lost
RX/TX interrupts)?

> +			m_can_write(cdev, M_CAN_IE, cdev->active_interrupts);
> +		}
> +		netif_device_attach(ndev);
> +		netif_start_queue(ndev);
> +	}
> +
>  	return ret;
>  }

[Severity: Medium]
Does this error path need to restart the polling hrtimer?

If a device operates in polling mode (!cdev->net->irq) but has wakeup-source
set in DT, cdev->pm_wake_source is true.

Earlier in this function, the hrtimer is cancelled:

    hrtimer_cancel(&cdev->hrtimer);

However, this error path restores the network queues but completely fails to
restart the hrtimer. Will this cause the CAN interface to completely stall
with no TX/RX processing?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928193312.553632-1-mkl@pengutronix.de?part=12