Re: [PATCH] spi: amlogic-spisg: check clk_prepare_enable() return value

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Devm-managed clocks (core and pclk) are manually disabled in error, suspend, and remove paths, causing clock enable count underflows when devres performs automatic cleanup.
- [High] Missing `clk_disable_unprepare()` for manually managed `sclk` on the probe error path and during driver removal.
- [High] Missing `pm_runtime_disable()` and unbalanced usage count in driver removal and probe error paths.
- [High] DMA descriptors and data buffers are unmapped and freed on transfer timeout while the hardware SPI controller may still be actively accessing them.
--

commit a395b715bade0abeb95d54b7ee542ec559727fde
Author: Li Youhong <liyouhong@kylinos.cn>

spi: amlogic-spisg: check clk_prepare_enable() return value

The driver ignored clk_prepare_enable() failures for sclk during probe
and for core/sclk during runtime resume. Propagate the errors and correctly
disable core if enabling sclk fails during resume.

> diff --git a/drivers/spi/spi-amlogic-spisg.c b/drivers/spi/spi-amlogic-spisg.c
> index 9049a87e9d0f2..6030c8158be17 100644
> --- a/drivers/spi/spi-amlogic-spisg.c
> +++ b/drivers/spi/spi-amlogic-spisg.c
> @@ -704,7 +704,9 @@ static int aml_spisg_clk_init(struct spisg_device *spisg, void __iomem *base)
>  		return PTR_ERR(spisg->sclk);
>  	}
>  
> -	clk_prepare_enable(spisg->sclk);
> +	ret = clk_prepare_enable(spisg->sclk);
> +	if (ret)
> +		return ret;
>  
>  	return 0;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does aml_spisg_clk_init()
manually disable the devm-managed pclk?

drivers/spi/spi-amlogic-spisg.c:aml_spisg_clk_init() {
    ...
	spisg->pclk = devm_clk_get_enabled(dev, "pclk");
	if (IS_ERR(spisg->pclk)) {
    ...
	clk_disable_unprepare(spisg->pclk);
    ...
}

Because pclk is managed by devres via devm_clk_get_enabled(), wouldn't manually 
disabling it cause a clock enable count underflow when devres performs 
automatic cleanup during teardown?

[Severity: High]
This is a pre-existing issue, but is the manually enabled sclk missing from
the probe error path and driver removal?

Since sclk is manually enabled here in aml_spisg_clk_init(), wouldn't skipping
clk_disable_unprepare(spisg->sclk) in the aml_spisg_probe() error path and in
aml_spisg_remove() permanently leak the clock enable count?

> @@ -841,9 +843,18 @@ static int spisg_suspend_runtime(struct device *dev)
>  static int spisg_resume_runtime(struct device *dev)
>  {
>  	struct spisg_device *spisg = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = clk_prepare_enable(spisg->core);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_prepare_enable(spisg->sclk);
> +	if (ret) {
> +		clk_disable_unprepare(spisg->core);
> +		return ret;
> +	}
>  
> -	clk_prepare_enable(spisg->core);
> -	clk_prepare_enable(spisg->sclk);
>  	pinctrl_pm_select_default_state(&spisg->pdev->dev);
>  
>  	return 0;

[Severity: High]
This is a pre-existing issue, but does runtime suspend manually disable
the devm-managed core clock?

drivers/spi/spi-amlogic-spisg.c:spisg_suspend_runtime() {
    ...
	clk_disable_unprepare(spisg->sclk);
	clk_disable_unprepare(spisg->core);
    ...
}

Because core is initially obtained with devm_clk_get_enabled(), the devres
cleanup will automatically disable it.

Additionally, could the aml_spisg_probe() error path and aml_spisg_remove() 
functions also underflow the clock enable counts for core and pclk?

drivers/spi/spi-amlogic-spisg.c:aml_spisg_probe() {
    ...
out_clk:
	if (spisg->core)
		clk_disable_unprepare(spisg->core);
	clk_disable_unprepare(spisg->pclk);
    ...
}

drivers/spi/spi-amlogic-spisg.c:aml_spisg_remove() {
    ...
	if (!pm_runtime_suspended(&pdev->dev)) {
		pinctrl_pm_select_sleep_state(&spisg->pdev->dev);
		clk_disable_unprepare(spisg->core);
		clk_disable_unprepare(spisg->pclk);
	}
    ...
}

[Severity: High]
This is a pre-existing issue, but does aml_spisg_probe() fail to drop the
runtime PM usage count on the error path?

drivers/spi/spi-amlogic-spisg.c:aml_spisg_probe() {
    ...
	pm_runtime_set_active(&spisg->pdev->dev);
	pm_runtime_enable(&spisg->pdev->dev);
	pm_runtime_resume_and_get(&spisg->pdev->dev);
    ...
	ret = spi_register_controller(ctlr);
	if (ret) {
		dev_err(&pdev->dev, "spi controller registration failed\n");
		goto out_clk;
	}

	pm_runtime_put(&spisg->pdev->dev);
    ...
out_clk:
    ...
}

If spi_register_controller() fails, the code jumps to out_clk without rolling 
back pm_runtime_resume_and_get() or pm_runtime_enable(). 

Also, does aml_spisg_remove() omit pm_runtime_disable() completely during 
driver teardown? Leaving runtime PM active after the device structures are 
freed could lead to use-after-free crashes if PM callbacks are triggered later.

[Severity: High]
This is a pre-existing issue, but can this code cause a use-after-free or 
IOMMU fault upon timeout?

drivers/spi/spi-amlogic-spisg.c:aml_spisg_transfer_one_message() {
    ...
	if (wait_for_completion_timeout(&spisg->completion, ...))
		ret = spisg->status ? -EIO : 0;
	else
		ret = -ETIMEDOUT;

	dma_unmap_single(dev, descs_paddr, descs_len, DMA_TO_DEVICE);
end:
	desc = descs;
	exdesc = exdescs;
	list_for_each_entry(xfer, &msg->transfers, transfer_list)
		aml_spisg_cleanup_transfer(spisg, xfer, desc++, exdesc++);
	kfree(descs);
    ...
}

If wait_for_completion_timeout() expires, the driver falls through to unmap 
and free the SG lists and descriptors without stopping the controller or halting 
the hardware DMA.

Could the SPI controller still be actively writing to or reading from this 
memory after it is freed and unmapped?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831094553.2247003-1-dayou5941@163.com?part=1

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