Re: [PATCH treewide 5/5] spi: amlogic-spisg: Make sure clk_init_data is fully initialized

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

Pre-existing issues:
- [High] Severe imbalance in clock prepare/enable counts for `pclk` and `core` clocks, leading to negative enable counts and warnings during driver probe failure or removal.
- [High] Missing cleanup for runtime PM and `sclk` clock on the probe error path and module removal.
--

--- Patch [5]: [PATCH treewide 5/5] spi: amlogic-spisg: Make sure clk_init_data is fully initialized ---
commit aa4fc6e665f6f8293335c8edd96584e8cdda40c6
Author: Geert Uytterhoeven <[email protected]>
Subject: spi: amlogic-spisg: Make sure clk_init_data is fully initialized

The clk_init_data structure contains several mutually-exclusive members
for different methods to specify the possible parents of a clock,
prompting drivers to initialize only the members they need. However,
not initializing all members may cause subtle issues.

Fixes: cef9991e04aed330 ("spi: Add Amlogic SPISG driver")

> diff --git a/drivers/spi/spi-amlogic-spisg.c b/drivers/spi/spi-amlogic-spisg.c
> index afc8af04638d3..9049a87e9d0f2 100644
> --- a/drivers/spi/spi-amlogic-spisg.c
> +++ b/drivers/spi/spi-amlogic-spisg.c
> @@ -636,7 +636,7 @@ static int aml_spisg_target_abort(struct spi_controller *ctlr)
>  static int aml_spisg_clk_init(struct spisg_device *spisg, void __iomem *base)
>  {
>  	struct device *dev = &spisg->pdev->dev;
> -	struct clk_init_data init;
> +	struct clk_init_data init = {};
>  	struct clk_divider *div;
>  	struct clk_div_table *tbl;
>  	char name[32];

[Severity: High]
This is a pre-existing issue, but I noticed a severe imbalance in clock
prepare/enable counts for the pclk and core clocks.

In aml_spisg_clk_init(), pclk is acquired with devm_clk_get_enabled():

	spisg->pclk = devm_clk_get_enabled(dev, "pclk");

But later in the same function, it is manually disabled:

	clk_disable_unprepare(spisg->pclk);

Since pclk is managed by devres, won't this manual disable unbalance the
clock enable count right from the start? 

This pattern is also repeated in the probe error path and remove functions:

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

	return ret;
}

Does this cause double-disables and negative enable counts when the
devres cleanup runs?

[Severity: High]
This is also a pre-existing issue, but I noticed missing cleanup for
runtime PM and the sclk clock.

In aml_spisg_clk_init(), sclk is manually enabled:

	clk_prepare_enable(spisg->sclk);

And in aml_spisg_probe(), runtime PM is enabled:

	pm_runtime_enable(&spisg->pdev->dev);

However, if devm_request_irq() or spi_register_controller() fails, the
out_clk error path simply returns:

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

	return ret;
}

Doesn't this leak the sclk enable count and leave runtime PM enabled?

Additionally, aml_spisg_remove() also lacks a pm_runtime_disable() call
and skips disabling sclk if the device is not runtime suspended. Should
there be explicit teardowns for these in the error and removal paths?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic