[PATCH] spi: amlogic-spisg: Balance runtime PM and clock cleanup

[PATCH] spi: amlogic-spisg: Balance runtime PM and clock cleanup

[Myeonghun Pak]

The core and pclk clocks are acquired with devm_clk_get_enabled(), but
pclk is immediately disabled and core is also disabled by runtime PM and
teardown. The managed cleanup can disable them again. Meanwhile, probe
unwind and active removal disable pclk instead of the enabled child sclk,
leaving the divider's own clock references unbalanced.

Keep managed clock handles but explicitly manage the core and sclk enable
references. Let the clock framework account for pclk through its sclk
child, and unwind core if divider setup or sclk enable fails.

After clock setup, check pm_runtime_set_active() and hold a no-resume
reference before enabling runtime PM. Balance that reference on probe
failure, disable runtime PM during teardown and free the IRQ before the
explicit clock shutdown. On removal, only disable clocks that runtime
suspend has not already disabled.

This issue was identified during our ongoing static-analysis research while
reviewing kernel code.

Fixes: cef9991e04ae ("spi: Add Amlogic SPISG driver")
Cc: stable@vger.kernel.org
Assisted-by: OpenAI:GPT-5.6
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---
Based on SPI for-next at 98100d83adc8e17606605d131e685b1fbedfe092,
which includes Li Youhong's clock-enable error handling patch:
https://lore.kernel.org/r/20260831094553.2247003-1-dayou5941@163.com/

 drivers/spi/spi-amlogic-spisg.c | 55 +++++++++++++++++++++++++------------
 1 file changed, 38 insertions(+), 17 deletions(-)

diff --git a/drivers/spi/spi-amlogic-spisg.c b/drivers/spi/spi-amlogic-spisg.c
index 1b00f34..33c5cbf 100644
--- a/drivers/spi/spi-amlogic-spisg.c
+++ b/drivers/spi/spi-amlogic-spisg.c
@@ -165,6 +165,7 @@ struct spisg_device {
 	struct clk			*sclk;
 	struct clk_div_table		*tbl;
 	struct completion		completion;
+	int				irq;
 	const struct aml_spisg_data	*data;
 	u32				status;
 	u32				speed_hz;
@@ -692,13 +693,13 @@ static int aml_spisg_clk_init(struct spisg_device *spisg, void __iomem *base)
 	char name[32];
 	int ret, i;
 
-	spisg->core = devm_clk_get_enabled(dev, "core");
+	spisg->core = devm_clk_get(dev, "core");
 	if (IS_ERR(spisg->core)) {
 		dev_err(dev, "core clock request failed\n");
 		return PTR_ERR(spisg->core);
 	}
 
-	spisg->pclk = devm_clk_get_enabled(dev, "pclk");
+	spisg->pclk = devm_clk_get(dev, "pclk");
 	if (IS_ERR(spisg->pclk)) {
 		dev_err(dev, "pclk clock request failed\n");
 		return PTR_ERR(spisg->pclk);
@@ -706,8 +707,6 @@ static int aml_spisg_clk_init(struct spisg_device *spisg, void __iomem *base)
 
 	clk_set_min_rate(spisg->pclk, SPISG_PCLK_RATE_MIN);
 
-	clk_disable_unprepare(spisg->pclk);
-
 	tbl = devm_kcalloc(dev, (DIV_NUM + 1), sizeof(*tbl), GFP_KERNEL);
 	if (!tbl)
 		return -ENOMEM;
@@ -728,6 +727,10 @@ static int aml_spisg_clk_init(struct spisg_device *spisg, void __iomem *base)
 	div->width = CLK_DIV_WIDTH;
 	div->table = tbl;
 
+	ret = clk_prepare_enable(spisg->core);
+	if (ret)
+		return ret;
+
 	/* Register value should not be outside of the table */
 	regmap_update_bits(spisg->map, SPISG_REG_CFG_BUS, CFG_CLK_DIV,
 			   FIELD_PREP(CFG_CLK_DIV, SPISG_CLK_DIV_MIN - 1));
@@ -745,20 +748,25 @@ static int aml_spisg_clk_init(struct spisg_device *spisg, void __iomem *base)
 	ret = devm_clk_hw_register(dev, &div->hw);
 	if (ret) {
 		dev_err(dev, "clock registration failed\n");
-		return ret;
+		goto out_core;
 	}
 
 	spisg->sclk = devm_clk_hw_get_clk(dev, &div->hw, NULL);
 	if (IS_ERR(spisg->sclk)) {
 		dev_err(dev, "get clock failed\n");
-		return PTR_ERR(spisg->sclk);
+		ret = PTR_ERR(spisg->sclk);
+		goto out_core;
 	}
 
 	ret = clk_prepare_enable(spisg->sclk);
 	if (ret)
-		return ret;
+		goto out_core;
 
 	return 0;
+
+out_core:
+	clk_disable_unprepare(spisg->core);
+	return ret;
 }
 
 static int aml_spisg_probe(struct platform_device *pdev)
@@ -802,6 +810,7 @@ static int aml_spisg_probe(struct platform_device *pdev)
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)
 		return irq;
+	spisg->irq = irq;
 
 	ret = device_reset_optional(dev);
 	if (ret)
@@ -832,9 +841,13 @@ static int aml_spisg_probe(struct platform_device *pdev)
 	/* default pending */
 	spisg->cfg_start = FIELD_PREP(CFG_PEND, 1);
 
-	pm_runtime_set_active(&spisg->pdev->dev);
-	pm_runtime_enable(&spisg->pdev->dev);
-	pm_runtime_resume_and_get(&spisg->pdev->dev);
+	ret = pm_runtime_set_active(dev);
+	if (ret)
+		goto out_clk;
+
+	/* Clock initialization has already powered up the controller. */
+	pm_runtime_get_noresume(dev);
+	pm_runtime_enable(dev);
 
 	ctlr->num_chipselect = 4;
 	ctlr->mode_bits = SPI_CPHA | SPI_CPOL | SPI_LSB_FIRST |
@@ -856,22 +869,26 @@ static int aml_spisg_probe(struct platform_device *pdev)
 	ret = devm_request_irq(&pdev->dev, irq, aml_spisg_irq, 0, NULL, spisg);
 	if (ret) {
 		dev_err(&pdev->dev, "irq request failed\n");
-		goto out_clk;
+		goto out_pm;
 	}
 
 	ret = spi_register_controller(ctlr);
 	if (ret) {
 		dev_err(&pdev->dev, "spi controller registration failed\n");
-		goto out_clk;
+		devm_free_irq(dev, irq, spisg);
+		goto out_pm;
 	}
 
 	pm_runtime_put(&spisg->pdev->dev);
 
 	return 0;
+out_pm:
+	pm_runtime_disable(dev);
+	pm_runtime_put_noidle(dev);
 out_clk:
-	if (spisg->core)
-		clk_disable_unprepare(spisg->core);
-	clk_disable_unprepare(spisg->pclk);
+	clk_disable_unprepare(spisg->sclk);
+	clk_disable_unprepare(spisg->core);
+	pm_runtime_set_suspended(dev);
 
 	return ret;
 }
@@ -882,11 +899,15 @@ static void aml_spisg_remove(struct platform_device *pdev)
 
 	spi_unregister_controller(spisg->controller);
 
-	if (!pm_runtime_suspended(&pdev->dev)) {
+	pm_runtime_disable(&pdev->dev);
+	devm_free_irq(&pdev->dev, spisg->irq, spisg);
+
+	if (!pm_runtime_status_suspended(&pdev->dev)) {
 		pinctrl_pm_select_sleep_state(&spisg->pdev->dev);
+		clk_disable_unprepare(spisg->sclk);
 		clk_disable_unprepare(spisg->core);
-		clk_disable_unprepare(spisg->pclk);
 	}
+	pm_runtime_set_suspended(&pdev->dev);
 }
 
 static int spisg_suspend_runtime(struct device *dev)

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH] spi: amlogic-spisg: Balance runtime PM and clock cleanup

[sashiko-bot]

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

Pre-existing issues:
- [High] The IRQ handler accesses hardware registers without verifying the device's runtime PM state.
- [High] Division by zero in aml_spisg_setup_transfer when bits_per_word is less than 8.
- [Low] The dev_pm_ops structure is unconditionally assigned to the driver structure, violating subsystem guidelines for conditional compilation of PM callbacks.
--

commit 9ad90d104df3bfd91e2fb30632c7e2e3d7ab281c
Author: Myeonghun Pak <mhun512@gmail.com>
Subject: spi: amlogic-spisg: Balance runtime PM and clock cleanup

This commit explicitly manages the core and sclk enable references for the
Amlogic SPISG driver. It checks pm_runtime_set_active() and holds a no-resume
reference before enabling runtime PM, balancing that reference during probe
failure and teardown.

> diff --git a/drivers/spi/spi-amlogic-spisg.c b/drivers/spi/spi-amlogic-spisg.c
> index 1b00f34e9e931..33c5cbf57cb87 100644
> --- a/drivers/spi/spi-amlogic-spisg.c
> +++ b/drivers/spi/spi-amlogic-spisg.c
[ ... ]
> @@ -832,9 +841,13 @@ static int aml_spisg_probe(struct platform_device *pdev)
[ ... ]
> +	/* Clock initialization has already powered up the controller. */
> +	pm_runtime_get_noresume(dev);
> +	pm_runtime_enable(dev);
>  
>  	ctlr->num_chipselect = 4;
>  	ctlr->mode_bits = SPI_CPHA | SPI_CPOL | SPI_LSB_FIRST |

[Severity: High]
This is a pre-existing issue, but does failing to initialize
ctlr->bits_per_word_mask here bypass core validation?

Because the mask is 0, __spi_validate_bits_per_word() in the SPI core allows
arbitrary sizes. If a user sets bits_per_word to a value like 4 via the
SPI_IOC_WR_BITS_PER_WORD ioctl, could this trigger a division by zero in
aml_spisg_setup_transfer() when shifting right by 3?

drivers/spi/spi-amlogic-spisg.c:aml_spisg_setup_transfer() {
	...
	block_size = xfer->bits_per_word >> 3;
	blocks = xfer->len / block_size;
	...
}

> @@ -856,22 +869,26 @@ static int aml_spisg_probe(struct platform_device *pdev)
>  	ret = devm_request_irq(&pdev->dev, irq, aml_spisg_irq, 0, NULL, spisg);

[Severity: High]
This is a pre-existing issue, but can spurious IRQs cause a crash here if the
device is suspended?

Looking at aml_spisg_irq(), the handler accesses hardware registers without
verifying the runtime PM state:

drivers/spi/spi-amlogic-spisg.c:aml_spisg_irq() {
	...
	spisg->status = 0;
	regmap_read(spisg->map, SPISG_REG_IRQ_STS, &sts);
	...
}

If the interrupt fires while the device is in RPM_SUSPENDED (where both sclk
and core clocks are disabled), could reading this MMIO register cause a
synchronous external abort or bus hang? Should we use
pm_runtime_get_if_active() before accessing hardware registers in the IRQ
handler?

[ ... ]
> @@ -882,11 +899,15 @@ static void aml_spisg_remove(struct platform_device *pdev)
[ ... ]
>  static int spisg_suspend_runtime(struct device *dev)

[Severity: Low]
This is a pre-existing issue, but is it safe to assign the pm operations
unconditionally?

Looking at the driver definition:

drivers/spi/spi-amlogic-spisg.c:amlogic_spisg_driver {
	...
	.driver = {
		.name = "amlogic-spisg",
		.pm = &amlogic_spisg_pm_ops,
		.of_match_table = aml_spisg_of_match,
	...
}

When CONFIG_PM is disabled, amlogic_spisg_pm_ops and its callbacks remain
linked into the kernel as dead code. Should this be wrapped with the pm_ptr()
macro to avoid wasting memory when power management is disabled?

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

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic