Re: [PATCH net-next 23/37] can: Convert to DEFINE_SIMPLE_DEV_PM_OPS()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] can: m_can/bxcan: bus fault on shared IRQ during suspend
--

commit 67e577fb01461ec0237b4358b708fb235942190b
Author: Triet Hoang <triet.hoang.dev@gmail.com>

    can: Convert to DEFINE_SIMPLE_DEV_PM_OPS()
    
    Convert the deprecated SIMPLE_DEV_PM_OPS to DEFINE_SIMPLE_DEV_PM_OPS and
    pm_sleep_ptr().
    
    This lets us drop the __maybe_unused annotations from the suspend and
    resume callbacks, also reduces kernel size in case CONFIG_PM or
    CONFIG_PM_SLEEP is disabled.
    
    Link: https://patch.msgid.link/20260822074701.22856-1-triet.hoang.dev@gmail.com

> diff --git a/drivers/net/can/bxcan.c b/drivers/net/can/bxcan.c
> index 4bddd015775bc..622c5d73e33a8 100644
> --- a/drivers/net/can/bxcan.c
> +++ b/drivers/net/can/bxcan.c
> @@ -1042,20 +1042,19 @@ static void bxcan_remove(struct platform_device *pdev)
[ ... ]
> -static int __maybe_unused bxcan_suspend(struct device *dev)
> +static int bxcan_suspend(struct device *dev)
>  {
>  	struct net_device *ndev = dev_get_drvdata(dev);
>  	struct bxcan_priv *priv = netdev_priv(ndev);
>  
>  	if (!netif_running(ndev))
>  		return 0;
>  
>  	netif_stop_queue(ndev);
>  	netif_device_detach(ndev);
>  
>  	bxcan_enter_sleep_mode(priv);
>  	priv->can.state = CAN_STATE_SLEEPING;
>  	clk_disable_unprepare(priv->clk);
        ^^^^

[Severity: High]
Is it possible for the shared IRQ to fire after the clocks are disabled here?

If another device sharing the same IRQ line triggers an interrupt before the
suspend_noirq phase, or if the interrupt is configured as a wakeup source, the
kernel will invoke bxcan_rx_isr().

Since bxcan_rx_isr() reads MMIO registers (like rf0r = readl(&regs->rf0r))
unconditionally when called, could this cause a bus fault and system crash if
the clock is stopped?

>  	return 0;
>  }

> diff --git a/drivers/net/can/m_can/m_can_pci.c b/drivers/net/can/m_can/m_can_pci.c
> index d11a7c88fc32f..d31179312026b 100644
> --- a/drivers/net/can/m_can/m_can_pci.c
> +++ b/drivers/net/can/m_can/m_can_pci.c
> @@ -169,17 +169,17 @@ static void m_can_pci_remove(struct pci_dev *pci)
[ ... ]
> -static __maybe_unused int m_can_pci_suspend(struct device *dev)
> +static int m_can_pci_suspend(struct device *dev)
>  {
>  	return m_can_class_suspend(dev);
        ^^^^

[Severity: High]
Does this suspend path have a similar regression with shared IRQs?

m_can_class_suspend() calls m_can_clk_stop() which disables the peripheral
clocks, but the shared IRQ is left registered. If the interrupt fires
subsequently, m_can_interrupt_handler() will execute.

While m_can_interrupt_handler() checks pm_runtime_suspended(), this evaluates
to false during system suspend if the device was active prior to sleep. The
handler will then attempt an MMIO read via m_can_read(cdev, M_CAN_IR), which
could cause a bus fault on an unclocked peripheral.

>  }

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