[PATCH] net: can: Convert to DEFINE_SIMPLE_DEV_PM_OPS()

[PATCH] net: can: Convert to DEFINE_SIMPLE_DEV_PM_OPS()

[Triet Hoang]

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.

Signed-off-by: Triet Hoang <triet.hoang.dev@gmail.com>
---
 drivers/net/can/bxcan.c                      | 8 ++++----
 drivers/net/can/ctucanfd/ctucanfd.h          | 4 ++--
 drivers/net/can/ctucanfd/ctucanfd_pci.c      | 4 ++--
 drivers/net/can/ctucanfd/ctucanfd_platform.c | 4 ++--
 drivers/net/can/m_can/m_can_pci.c            | 8 ++++----
 drivers/net/can/spi/hi311x.c                 | 8 ++++----
 drivers/net/can/spi/mcp251x.c                | 8 ++++----
 7 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/net/can/bxcan.c b/drivers/net/can/bxcan.c
index baf494d20bef..b2a0ff8c033c 100644
--- a/drivers/net/can/bxcan.c
+++ b/drivers/net/can/bxcan.c
@@ -1042,7 +1042,7 @@ static void bxcan_remove(struct platform_device *pdev)
 	free_candev(ndev);
 }
 
-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);
@@ -1059,7 +1059,7 @@ static int __maybe_unused bxcan_suspend(struct device *dev)
 	return 0;
 }
 
-static int __maybe_unused bxcan_resume(struct device *dev)
+static int bxcan_resume(struct device *dev)
 {
 	struct net_device *ndev = dev_get_drvdata(dev);
 	struct bxcan_priv *priv = netdev_priv(ndev);
@@ -1076,7 +1076,7 @@ static int __maybe_unused bxcan_resume(struct device *dev)
 	return 0;
 }
 
-static SIMPLE_DEV_PM_OPS(bxcan_pm_ops, bxcan_suspend, bxcan_resume);
+static DEFINE_SIMPLE_DEV_PM_OPS(bxcan_pm_ops, bxcan_suspend, bxcan_resume);
 
 static const struct of_device_id bxcan_of_match[] = {
 	{.compatible = "st,stm32f4-bxcan"},
@@ -1087,7 +1087,7 @@ MODULE_DEVICE_TABLE(of, bxcan_of_match);
 static struct platform_driver bxcan_driver = {
 	.driver = {
 		.name = KBUILD_MODNAME,
-		.pm = &bxcan_pm_ops,
+		.pm = pm_sleep_ptr(&bxcan_pm_ops),
 		.of_match_table = bxcan_of_match,
 	},
 	.probe = bxcan_probe,
diff --git a/drivers/net/can/ctucanfd/ctucanfd.h b/drivers/net/can/ctucanfd/ctucanfd.h
index 0e9904f6a05d..7dcd7dc507e4 100644
--- a/drivers/net/can/ctucanfd/ctucanfd.h
+++ b/drivers/net/can/ctucanfd/ctucanfd.h
@@ -76,7 +76,7 @@ int ctucan_probe_common(struct device *dev, void __iomem *addr,
 			void (*set_drvdata_fnc)(struct device *dev,
 						struct net_device *ndev));
 
-int ctucan_suspend(struct device *dev) __maybe_unused;
-int ctucan_resume(struct device *dev) __maybe_unused;
+int ctucan_suspend(struct device *dev);
+int ctucan_resume(struct device *dev);
 
 #endif /*__CTUCANFD__*/
diff --git a/drivers/net/can/ctucanfd/ctucanfd_pci.c b/drivers/net/can/ctucanfd/ctucanfd_pci.c
index 4b6db28f7b67..9c9c41c3058c 100644
--- a/drivers/net/can/ctucanfd/ctucanfd_pci.c
+++ b/drivers/net/can/ctucanfd/ctucanfd_pci.c
@@ -259,7 +259,7 @@ static void ctucan_pci_remove(struct pci_dev *pdev)
 	kfree(bdata);
 }
 
-static SIMPLE_DEV_PM_OPS(ctucan_pci_pm_ops, ctucan_suspend, ctucan_resume);
+static DEFINE_SIMPLE_DEV_PM_OPS(ctucan_pci_pm_ops, ctucan_suspend, ctucan_resume);
 
 static const struct pci_device_id ctucan_pci_tbl[] = {
 	{PCI_DEVICE_DATA(TEDIA, CTUCAN_VER21,
@@ -273,7 +273,7 @@ static struct pci_driver ctucan_pci_driver = {
 	.id_table = ctucan_pci_tbl,
 	.probe = ctucan_pci_probe,
 	.remove = ctucan_pci_remove,
-	.driver.pm = &ctucan_pci_pm_ops,
+	.driver.pm = pm_sleep_ptr(&ctucan_pci_pm_ops),
 };
 
 module_pci_driver(ctucan_pci_driver);
diff --git a/drivers/net/can/ctucanfd/ctucanfd_platform.c b/drivers/net/can/ctucanfd/ctucanfd_platform.c
index 70e2577c8541..4962b00ee8ac 100644
--- a/drivers/net/can/ctucanfd/ctucanfd_platform.c
+++ b/drivers/net/can/ctucanfd/ctucanfd_platform.c
@@ -99,7 +99,7 @@ static void ctucan_platform_remove(struct platform_device *pdev)
 	free_candev(ndev);
 }
 
-static SIMPLE_DEV_PM_OPS(ctucan_platform_pm_ops, ctucan_suspend, ctucan_resume);
+static DEFINE_SIMPLE_DEV_PM_OPS(ctucan_platform_pm_ops, ctucan_suspend, ctucan_resume);
 
 /* Match table for OF platform binding */
 static const struct of_device_id ctucan_of_match[] = {
@@ -114,7 +114,7 @@ static struct platform_driver ctucanfd_driver = {
 	.remove = ctucan_platform_remove,
 	.driver	= {
 		.name = DRV_NAME,
-		.pm = &ctucan_platform_pm_ops,
+		.pm = pm_sleep_ptr(&ctucan_platform_pm_ops),
 		.of_match_table	= ctucan_of_match,
 	},
 };
diff --git a/drivers/net/can/m_can/m_can_pci.c b/drivers/net/can/m_can/m_can_pci.c
index d11a7c88fc32..d31179312026 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)
 	pci_free_irq_vectors(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);
 }
 
-static __maybe_unused int m_can_pci_resume(struct device *dev)
+static int m_can_pci_resume(struct device *dev)
 {
 	return m_can_class_resume(dev);
 }
 
-static SIMPLE_DEV_PM_OPS(m_can_pci_pm_ops,
+static DEFINE_SIMPLE_DEV_PM_OPS(m_can_pci_pm_ops,
 			 m_can_pci_suspend, m_can_pci_resume);
 
 static const struct pci_device_id m_can_pci_id_table[] = {
@@ -195,7 +195,7 @@ static struct pci_driver m_can_pci_driver = {
 	.remove = m_can_pci_remove,
 	.id_table = m_can_pci_id_table,
 	.driver = {
-		.pm = &m_can_pci_pm_ops,
+		.pm = pm_sleep_ptr(&m_can_pci_pm_ops),
 	},
 };
 
diff --git a/drivers/net/can/spi/hi311x.c b/drivers/net/can/spi/hi311x.c
index ae90e6716de5..076a3efa1bff 100644
--- a/drivers/net/can/spi/hi311x.c
+++ b/drivers/net/can/spi/hi311x.c
@@ -972,7 +972,7 @@ static void hi3110_can_remove(struct spi_device *spi)
 	free_candev(net);
 }
 
-static int __maybe_unused hi3110_can_suspend(struct device *dev)
+static int hi3110_can_suspend(struct device *dev)
 {
 	struct spi_device *spi = to_spi_device(dev);
 	struct hi3110_priv *priv = spi_get_drvdata(spi);
@@ -1002,7 +1002,7 @@ static int __maybe_unused hi3110_can_suspend(struct device *dev)
 	return 0;
 }
 
-static int __maybe_unused hi3110_can_resume(struct device *dev)
+static int hi3110_can_resume(struct device *dev)
 {
 	struct spi_device *spi = to_spi_device(dev);
 	struct hi3110_priv *priv = spi_get_drvdata(spi);
@@ -1022,13 +1022,13 @@ static int __maybe_unused hi3110_can_resume(struct device *dev)
 	return 0;
 }
 
-static SIMPLE_DEV_PM_OPS(hi3110_can_pm_ops, hi3110_can_suspend, hi3110_can_resume);
+static DEFINE_SIMPLE_DEV_PM_OPS(hi3110_can_pm_ops, hi3110_can_suspend, hi3110_can_resume);
 
 static struct spi_driver hi3110_can_driver = {
 	.driver = {
 		.name = DEVICE_NAME,
 		.of_match_table = hi3110_of_match,
-		.pm = &hi3110_can_pm_ops,
+		.pm = pm_sleep_ptr(&hi3110_can_pm_ops),
 	},
 	.id_table = hi3110_id_table,
 	.probe = hi3110_can_probe,
diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c
index 0d0190ae094a..d971890487b6 100644
--- a/drivers/net/can/spi/mcp251x.c
+++ b/drivers/net/can/spi/mcp251x.c
@@ -1490,7 +1490,7 @@ static void mcp251x_can_remove(struct spi_device *spi)
 	free_candev(net);
 }
 
-static int __maybe_unused mcp251x_can_suspend(struct device *dev)
+static int mcp251x_can_suspend(struct device *dev)
 {
 	struct spi_device *spi = to_spi_device(dev);
 	struct mcp251x_priv *priv = spi_get_drvdata(spi);
@@ -1517,7 +1517,7 @@ static int __maybe_unused mcp251x_can_suspend(struct device *dev)
 	return 0;
 }
 
-static int __maybe_unused mcp251x_can_resume(struct device *dev)
+static int mcp251x_can_resume(struct device *dev)
 {
 	struct spi_device *spi = to_spi_device(dev);
 	struct mcp251x_priv *priv = spi_get_drvdata(spi);
@@ -1551,14 +1551,14 @@ static int __maybe_unused mcp251x_can_resume(struct device *dev)
 	return 0;
 }
 
-static SIMPLE_DEV_PM_OPS(mcp251x_can_pm_ops, mcp251x_can_suspend,
+static DEFINE_SIMPLE_DEV_PM_OPS(mcp251x_can_pm_ops, mcp251x_can_suspend,
 	mcp251x_can_resume);
 
 static struct spi_driver mcp251x_can_driver = {
 	.driver = {
 		.name = DEVICE_NAME,
 		.of_match_table = mcp251x_of_match,
-		.pm = &mcp251x_can_pm_ops,
+		.pm = pm_sleep_ptr(&mcp251x_can_pm_ops),
 	},
 	.id_table = mcp251x_id_table,
 	.probe = mcp251x_can_probe,
-- 
2.53.0

Re: [PATCH] net: can: Convert to DEFINE_SIMPLE_DEV_PM_OPS()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing synchronous cancellation of the polling hrtimer during device teardown leads to a use-after-free.
- [High] Workqueue is flushed after the device's power has been disabled, causing pending works to access unpowered hardware.
- [High] Missing IRQ synchronization in bxcan_suspend causes a system crash when a shared interrupt fires.
- [High] Missing NAPI synchronization in m_can_class_suspend allows concurrent access to unclocked registers.
- [High] Missing NAPI and IRQ synchronization in ctucan_suspend leads to use-after-suspend register access.
--

--- Patch [1]: [PATCH] net: can: Convert to DEFINE_SIMPLE_DEV_PM_OPS() ---
commit a15538c2b4418f8d2120cca5fc551587851481fe
Author: Triet Hoang <triet.hoang.dev@gmail.com>

net: 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.

> diff --git a/drivers/net/can/bxcan.c b/drivers/net/can/bxcan.c
> index baf494d20bef4..b2a0ff8c033cc 100644
> --- a/drivers/net/can/bxcan.c
> +++ b/drivers/net/can/bxcan.c
[ ... ]
> -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);

[Severity: High]
This is a pre-existing issue, but does this suspend logic correctly handle
shared interrupts?

In bxcan_suspend(), the device clock is turned off:

bxcan_suspend() {
	...
	bxcan_enter_sleep_mode(priv);
	priv->can.state = CAN_STATE_SLEEPING;
	clk_disable_unprepare(priv->clk);
	return 0;
}

If another device sharing the same IRQ line triggers an interrupt while the
clock is disabled, the kernel will execute bxcan_rx_isr() and unconditionally
attempt to read regs->rf0r. Would reading from this unclocked peripheral
register cause a fatal bus fault?

[ ... ]
> diff --git a/drivers/net/can/ctucanfd/ctucanfd_pci.c b/drivers/net/can/ctucanfd/ctucanfd_pci.c
> index 4b6db28f7b670..9c9c41c3058c0 100644
> --- a/drivers/net/can/ctucanfd/ctucanfd_pci.c
> +++ b/drivers/net/can/ctucanfd/ctucanfd_pci.c
[ ... ]
> -static SIMPLE_DEV_PM_OPS(ctucan_pci_pm_ops, ctucan_suspend, ctucan_resume);
> +static DEFINE_SIMPLE_DEV_PM_OPS(ctucan_pci_pm_ops, ctucan_suspend, ctucan_resume);

[Severity: High]
This isn't a bug introduced by this patch, but does ctucan_suspend() properly
disable hardware interrupts and NAPI?

It transitions the device state to CAN_STATE_SLEEPING, but leaves interrupts
and NAPI active before returning.

When the parent PCI bus transitions the device to a low-power state and MMIO
access is disabled, could a shared interrupt or scheduled NAPI handler read
from the powered-down MMIO space?

Reading from powered-down MMIO typically returns 0xFFFFFFFF, which might cause
the ISR to assume all interrupts are active, scheduling NAPI again and
potentially entering an infinite polling loop.

[ ... ]
> 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)
>  	pci_free_irq_vectors(pci);
>  }

[Severity: High]
This is a pre-existing issue, but could this lead to a use-after-free during
device teardown?

In m_can_pci_remove(), we call m_can_class_free_dev(), which frees the memory
backing the device:

m_can_pci_remove() {
	...
	m_can_class_unregister(mcan_class);
	m_can_class_free_dev(mcan_class->net);
	pci_free_irq_vectors(pci);
}

However, earlier in the unregister path (m_can_close() calling
m_can_disable_all_interrupts()), it uses hrtimer_try_to_cancel().

Because hrtimer_try_to_cancel() does not wait for the timer callback to finish
if it's currently executing on another CPU, if the timer is still running,
could the callback subsequently access the freed cdev->can.state?

> -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]
This isn't a bug introduced by this patch, but can this suspend flow lead to a
system crash due to a NAPI race?

m_can_class_suspend() disables device clocks via m_can_clk_stop() without
explicitly disabling NAPI:

m_can_class_suspend() {
	...
		} else {
			m_can_stop(ndev);
		}

		m_can_clk_stop(cdev);
		cdev->can.state = CAN_STATE_SLEEPING;
	}
	...
}

If an interrupt fires just before or during suspend, napi_schedule() can queue
the NAPI poll function (m_can_poll). Could this asynchronous softirq execute
after the clocks are turned off and read the M_CAN_IR register from the
unclocked peripheral, triggering a bus fault?

[ ... ]
> diff --git a/drivers/net/can/spi/hi311x.c b/drivers/net/can/spi/hi311x.c
> index ae90e6716de5b..076a3efa1bfff 100644
> --- a/drivers/net/can/spi/hi311x.c
> +++ b/drivers/net/can/spi/hi311x.c
> @@ -972,7 +972,7 @@ static void hi3110_can_remove(struct spi_device *spi)
>  	free_candev(net);
>  }

[Severity: High]
This is a pre-existing issue, but does this workqueue teardown order cause
pending works to access unpowered hardware?

In hi3110_can_remove() (and similarly in mcp251x_can_remove()), the device
power regulators are disabled before calling destroy_workqueue(priv->wq):

hi3110_can_remove() {
	...
	hi3110_power_enable(priv->power, 0);

	destroy_workqueue(priv->wq);
	priv->wq = NULL;
	...
}

Since destroy_workqueue() synchronously flushes any pending work items, could a
pending restart_work execute and attempt to communicate with the SPI device
while its power regulators are already turned off?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260822074701.22856-1-triet.hoang.dev@gmail.com?part=1