[PATCH] clk: imx: imx8qxp-lpcg: fix runtime PM cleanup on error path

[PATCH] clk: imx: imx8qxp-lpcg: fix runtime PM cleanup on error path

[Guangshuo Li]

imx_lpcg_parse_clks_from_dt() takes a runtime PM usage reference with
pm_runtime_get_noresume() and enables autosuspend with
pm_runtime_use_autosuspend(). The success path drops the usage
reference with pm_runtime_put_autosuspend(), but the error path only
disables runtime PM.

As a result, the reference taken by pm_runtime_get_noresume() remains
unbalanced on registration failure. The autosuspend setting is also
left enabled instead of being undone with
pm_runtime_dont_use_autosuspend().

Add pm_runtime_dont_use_autosuspend() to undo the autosuspend setting
and pm_runtime_put_noidle() to drop the usage reference on the error
path.

This issue was found by manual code inspection.

Fixes: 18cdbad40c6c ("clk: imx: clk-imx8qxp-lpcg: add runtime pm support")
Cc: stable@vger.kernel.org
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
 drivers/clk/imx/clk-imx8qxp-lpcg.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c b/drivers/clk/imx/clk-imx8qxp-lpcg.c
index 1dae3410ee99..262645bef62d 100644
--- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
+++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
@@ -277,8 +277,9 @@ static int imx_lpcg_parse_clks_from_dt(struct platform_device *pdev,
 		if (clk_hws[idx])
 			imx_clk_lpcg_scu_unregister(clk_hws[idx]);
 	}
-
+	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
+	pm_runtime_put_noidle(&pdev->dev);
 
 	return ret;
 }
-- 
2.43.0

Re: [PATCH] clk: imx: imx8qxp-lpcg: fix runtime PM cleanup on error path

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Missing bounds check on `count` leads to a severe stack buffer overflow when reading device tree properties.
- [High] System suspend causes a NULL pointer dereference or Use-After-Free when the driver uses the legacy device tree binding or fails parsing.
- [High] Duplicate indices in the `clock-indices` device tree property lead to a double-free and potential invalid pointer dereference on the error path.
--

--- Patch [1]: [PATCH] clk: imx: imx8qxp-lpcg: fix runtime PM cleanup on error path ---
commit 662987daf90805747f064d71045351ad4f076ebd
Author: Guangshuo Li <lgs201920130244@gmail.com>

clk: imx: imx8qxp-lpcg: fix runtime PM cleanup on error path

This commit fixes the error path of imx_lpcg_parse_clks_from_dt to properly
undo the runtime PM configurations. It ensures autosuspend is disabled and the
usage counter is correctly dropped on registration failure.

> diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> index f0cf6cf918992..fa79d52cbb36e 100644
> --- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
> +++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> @@ -277,8 +277,9 @@ static int imx_lpcg_parse_clks_from_dt(struct platform_device *pdev,
>  		if (clk_hws[idx])
>  			imx_clk_lpcg_scu_unregister(clk_hws[idx]);
>  	}

[Severity: High]
This is a pre-existing issue, but does this teardown loop properly handle
duplicate indices in the clock-indices device tree property?

If the device tree contains duplicate values in clock-indices, the second
clock unconditionally overwrites the first in clk_hws[idx] during registration:

	clk_hws[idx] = imx_clk_lpcg_scu_dev(&pdev->dev, output_names[i],
					    parent_names[i], 0, base,
					    bit_offset[i], false);

If an error occurs on a subsequent clock registration, the teardown loop
iterates backward and calls imx_clk_lpcg_scu_unregister() on clk_hws[idx].
Because clk_hws[idx] is never nulled out after unregistration, when the loop
encounters the duplicate index again, could it call kfree() on the
already-freed pointer, leading to a double-free?

Furthermore, if the failed registration itself returned an ERR_PTR that
overwrote a duplicate index, would the loop pass the ERR_PTR to
imx_clk_lpcg_scu_unregister() and crash the system?

> -
> +	pm_runtime_dont_use_autosuspend(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
> +	pm_runtime_put_noidle(&pdev->dev);
>  
>  	return ret;
>  }

While reviewing this code, I noticed a few other pre-existing issues in
the surrounding code.

[Severity: Critical]
This is a pre-existing issue, but does this function properly validate the
count retrieved from the device tree before using it to populate stack arrays?

imx_lpcg_parse_clks_from_dt() retrieves the number of elements into count:

	count = of_property_count_u32_elems(np, "clock-indices");

It then uses count to populate statically sized stack arrays (bit_offset,
parent_names, and output_names) of size IMX_LPCG_MAX_CLKS:

	ret = of_property_read_u32_array(np, "clock-indices", bit_offset,
					 count);
	...
	ret = of_clk_parent_fill(np, parent_names, count);
	...
	ret = of_property_read_string_array(np, "clock-output-names",
					    output_names, count);

If a device tree provides more than IMX_LPCG_MAX_CLKS elements, will this
overflow these stack allocations?

[Severity: High]
This is a pre-existing issue, but does system suspend cause a NULL pointer
dereference or Use-After-Free when the driver uses the legacy device tree
binding or fails parsing?

If the driver falls back to legacy device tree parsing, clocks are
registered using imx_clk_lpcg_scu() which passes dev = NULL. This leaves the
device drvdata unset:

drivers/clk/imx/clk-lpcg-scu.c:__imx_clk_lpcg_scu() {
	...
	if (dev)
		dev_set_drvdata(dev, clk);
	...
}

Alternatively, if imx_lpcg_parse_clks_from_dt() successfully registers one
clock but fails on a subsequent one, the error path unregisters and frees the
clock without clearing drvdata, leaving a dangling pointer.

When the system suspends, imx_clk_lpcg_scu_suspend() retrieves this NULL or
dangling pointer via dev_get_drvdata(dev) and dereferences it:

drivers/clk/imx/clk-lpcg-scu.c:imx_clk_lpcg_scu_suspend() {
	...
	if (!strncmp("hdmi_lpcg", clk_hw_get_name(&clk->hw), strlen("hdmi_lpcg")))
	...
}

Can this lead to a panic during system suspend on systems using the legacy
binding or when encountering a malformed DT?

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

RE: [PATCH] clk: imx: imx8qxp-lpcg: fix runtime PM cleanup on error path

[Peng Fan (OSS)]

> Subject: [PATCH] clk: imx: imx8qxp-lpcg: fix runtime PM cleanup on
> error path
> 
> imx_lpcg_parse_clks_from_dt() takes a runtime PM usage reference
> with
> pm_runtime_get_noresume() and enables autosuspend with
> pm_runtime_use_autosuspend(). The success path drops the usage
> reference with pm_runtime_put_autosuspend(), but the error path only
> disables runtime PM.
> 
> As a result, the reference taken by pm_runtime_get_noresume()
> remains unbalanced on registration failure. The autosuspend setting is
> also left enabled instead of being undone with
> pm_runtime_dont_use_autosuspend().
> 
> Add pm_runtime_dont_use_autosuspend() to undo the autosuspend
> setting and pm_runtime_put_noidle() to drop the usage reference on
> the error path.
> 
> This issue was found by manual code inspection.
> 
> Fixes: 18cdbad40c6c ("clk: imx: clk-imx8qxp-lpcg: add runtime pm
> support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>

Reviewed-by: Peng Fan <peng.fan@nxp.com>

Re: [PATCH] clk: imx: imx8qxp-lpcg: fix runtime PM cleanup on error path

[Brian Masney]

On Sat, Aug 08, 2026 at 04:02:15PM +0800, Guangshuo Li wrote:
> imx_lpcg_parse_clks_from_dt() takes a runtime PM usage reference with
> pm_runtime_get_noresume() and enables autosuspend with
> pm_runtime_use_autosuspend(). The success path drops the usage
> reference with pm_runtime_put_autosuspend(), but the error path only
> disables runtime PM.
> 
> As a result, the reference taken by pm_runtime_get_noresume() remains
> unbalanced on registration failure. The autosuspend setting is also
> left enabled instead of being undone with
> pm_runtime_dont_use_autosuspend().
> 
> Add pm_runtime_dont_use_autosuspend() to undo the autosuspend setting
> and pm_runtime_put_noidle() to drop the usage reference on the error
> path.
> 
> This issue was found by manual code inspection.
> 
> Fixes: 18cdbad40c6c ("clk: imx: clk-imx8qxp-lpcg: add runtime pm support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> ---
>  drivers/clk/imx/clk-imx8qxp-lpcg.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> index 1dae3410ee99..262645bef62d 100644
> --- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
> +++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> @@ -277,8 +277,9 @@ static int imx_lpcg_parse_clks_from_dt(struct platform_device *pdev,
>  		if (clk_hws[idx])
>  			imx_clk_lpcg_scu_unregister(clk_hws[idx]);
>  	}
> -

Don't remove the newline. With that fixed:

Reviewed-by: Brian Masney <bmasney@redhat.com>


> +	pm_runtime_dont_use_autosuspend(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
> +	pm_runtime_put_noidle(&pdev->dev);
>  
>  	return ret;
>  }
> -- 
> 2.43.0
> 

Re: [PATCH] clk: imx: imx8qxp-lpcg: fix runtime PM cleanup on error path

[Frank Li]

On Sat, Aug 08, 2026 at 04:02:15PM +0800, Guangshuo Li wrote:
> [You don't often get email from lgs201920130244@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> imx_lpcg_parse_clks_from_dt() takes a runtime PM usage reference with
> pm_runtime_get_noresume() and enables autosuspend with
> pm_runtime_use_autosuspend(). The success path drops the usage
> reference with pm_runtime_put_autosuspend(), but the error path only
> disables runtime PM.
>
> As a result, the reference taken by pm_runtime_get_noresume() remains
> unbalanced on registration failure. The autosuspend setting is also
> left enabled instead of being undone with
> pm_runtime_dont_use_autosuspend().
>
> Add pm_runtime_dont_use_autosuspend() to undo the autosuspend setting
> and pm_runtime_put_noidle() to drop the usage reference on the error
> path.
>
> This issue was found by manual code inspection.
>
> Fixes: 18cdbad40c6c ("clk: imx: clk-imx8qxp-lpcg: add runtime pm support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> ---
>  drivers/clk/imx/clk-imx8qxp-lpcg.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> index 1dae3410ee99..262645bef62d 100644
> --- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
> +++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> @@ -277,8 +277,9 @@ static int imx_lpcg_parse_clks_from_dt(struct platform_device *pdev,
>                 if (clk_hws[idx])
>                         imx_clk_lpcg_scu_unregister(clk_hws[idx]);
>         }
> -
> +       pm_runtime_dont_use_autosuspend(&pdev->dev);
>         pm_runtime_disable(&pdev->dev);
> +       pm_runtime_put_noidle(&pdev->dev);

Can you use devm_pm_runtime_set_active_enabled() to fix this problem.

Frank
>
>         return ret;
>  }
> --
> 2.43.0
>
>