[PATCH] clk: qcom: clk-pll: reject vote enable on orphan parent

[PATCH] clk: qcom: clk-pll: reject vote enable on orphan parent

[Herman van Hazendonk]

clk_pll_vote_enable() unconditionally feeds the result of
clk_hw_get_parent(hw) through to_clk_pll() and on to wait_for_pll().
The common clock framework permits clk_enable() on an orphan clock
(supplier not bound yet), in which case clk_hw_get_parent() returns
NULL. to_clk_pll(NULL) then yields container_of(NULL, struct clk_pll,
clkr) -- a non-NULL bogus pointer pointing into the negative offset
of struct clk_pll.

wait_for_pll() reaches for the parent's name via
clk_hw_get_name(&pll->clkr.hw). Because clkr sits at a fixed offset
inside struct clk_pll, &pll->clkr.hw cancels the to_clk_pll offset
exactly back to NULL and clk_hw_get_name() then dereferences
core->name on a NULL clk_hw, panicking the kernel.

This is reachable today: gcc-msm8960.c and gcc-apq8064.c register a
pll4_vote whose parent (pll4) lives in lcc-msm8960.c, and the future
gcc-msm8660 pll4_vote does the same. If anything calls clk_enable()
on pll4_vote between gcc probe and the LCC clock controller binding,
the system panics. The exposure widens as more SoCs adopt the same
cross-controller voter pattern.

Resolve the parent with clk_hw_get_parent() once, return -ENODEV when
it is NULL, and only call into wait_for_pll() with a real
struct clk_pll. The enable-regmap write is also gated behind the
parent check so a failed enable cannot leave the vote bit asserted
against a clock the framework has not finished wiring up.

Signed-off-by: Herman van Hazendonk <github.com@herrie.org>
---
 drivers/clk/qcom/clk-pll.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/clk-pll.c b/drivers/clk/qcom/clk-pll.c
index 26ba709f43c8..7b26129565fe 100644
--- a/drivers/clk/qcom/clk-pll.c
+++ b/drivers/clk/qcom/clk-pll.c
@@ -199,14 +199,31 @@ static int wait_for_pll(struct clk_pll *pll)
 
 static int clk_pll_vote_enable(struct clk_hw *hw)
 {
+	struct clk_hw *parent;
 	int ret;
-	struct clk_pll *p = to_clk_pll(clk_hw_get_parent(hw));
+
+	/*
+	 * Vote clocks can be registered on one clock controller and have
+	 * the underlying PLL live on a different one (e.g. PLL4_VOTE in
+	 * GCC for the LPASS PLL4 owned by LCC on the MSM8x60 / MSM8960 /
+	 * APQ8064 families). The common clock framework permits enable
+	 * on an orphan, so clk_hw_get_parent() can legitimately return
+	 * NULL here while the supplier controller has not finished
+	 * probing yet. Reject the enable rather than handing a bogus
+	 * container_of(NULL, struct clk_pll, clkr) pointer to
+	 * wait_for_pll() - inside wait_for_pll(), clk_hw_get_name()
+	 * would reverse the offset back to NULL and dereference
+	 * core->name.
+	 */
+	parent = clk_hw_get_parent(hw);
+	if (!parent)
+		return -ENODEV;
 
 	ret = clk_enable_regmap(hw);
 	if (ret)
 		return ret;
 
-	return wait_for_pll(p);
+	return wait_for_pll(to_clk_pll(parent));
 }
 
 const struct clk_ops clk_pll_vote_ops = {

base-commit: 944125b4c454b58d2fe6e35f1087a932b2050dff
-- 
2.43.0

Re: [PATCH] clk: qcom: clk-pll: reject vote enable on orphan parent

[Dmitry Baryshkov]

On Tue, Jun 02, 2026 at 08:29:27AM +0200, Herman van Hazendonk wrote:
> clk_pll_vote_enable() unconditionally feeds the result of
> clk_hw_get_parent(hw) through to_clk_pll() and on to wait_for_pll().
> The common clock framework permits clk_enable() on an orphan clock
> (supplier not bound yet), in which case clk_hw_get_parent() returns
> NULL. to_clk_pll(NULL) then yields container_of(NULL, struct clk_pll,
> clkr) -- a non-NULL bogus pointer pointing into the negative offset
> of struct clk_pll.
> 
> wait_for_pll() reaches for the parent's name via
> clk_hw_get_name(&pll->clkr.hw). Because clkr sits at a fixed offset
> inside struct clk_pll, &pll->clkr.hw cancels the to_clk_pll offset
> exactly back to NULL and clk_hw_get_name() then dereferences
> core->name on a NULL clk_hw, panicking the kernel.
> 
> This is reachable today: gcc-msm8960.c and gcc-apq8064.c register a
> pll4_vote whose parent (pll4) lives in lcc-msm8960.c, and the future
> gcc-msm8660 pll4_vote does the same. If anything calls clk_enable()
> on pll4_vote between gcc probe and the LCC clock controller binding,
> the system panics. The exposure widens as more SoCs adopt the same
> cross-controller voter pattern.
> 
> Resolve the parent with clk_hw_get_parent() once, return -ENODEV when
> it is NULL, and only call into wait_for_pll() with a real
> struct clk_pll. The enable-regmap write is also gated behind the
> parent check so a failed enable cannot leave the vote bit asserted
> against a clock the framework has not finished wiring up.
> 
> Signed-off-by: Herman van Hazendonk <github.com@herrie.org>
> ---
>  drivers/clk/qcom/clk-pll.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/qcom/clk-pll.c b/drivers/clk/qcom/clk-pll.c
> index 26ba709f43c8..7b26129565fe 100644
> --- a/drivers/clk/qcom/clk-pll.c
> +++ b/drivers/clk/qcom/clk-pll.c
> @@ -199,14 +199,31 @@ static int wait_for_pll(struct clk_pll *pll)
>  
>  static int clk_pll_vote_enable(struct clk_hw *hw)
>  {
> +	struct clk_hw *parent;
>  	int ret;
> -	struct clk_pll *p = to_clk_pll(clk_hw_get_parent(hw));
> +
> +	/*
> +	 * Vote clocks can be registered on one clock controller and have
> +	 * the underlying PLL live on a different one (e.g. PLL4_VOTE in
> +	 * GCC for the LPASS PLL4 owned by LCC on the MSM8x60 / MSM8960 /
> +	 * APQ8064 families). The common clock framework permits enable
> +	 * on an orphan, so clk_hw_get_parent() can legitimately return
> +	 * NULL here while the supplier controller has not finished
> +	 * probing yet. Reject the enable rather than handing a bogus
> +	 * container_of(NULL, struct clk_pll, clkr) pointer to
> +	 * wait_for_pll() - inside wait_for_pll(), clk_hw_get_name()
> +	 * would reverse the offset back to NULL and dereference
> +	 * core->name.
> +	 */

This is a commit message, not a comment. SHorten it or drop completely.

> +	parent = clk_hw_get_parent(hw);
> +	if (!parent)
> +		return -ENODEV;
>  
>  	ret = clk_enable_regmap(hw);
>  	if (ret)
>  		return ret;
>  
> -	return wait_for_pll(p);
> +	return wait_for_pll(to_clk_pll(parent));
>  }
>  
>  const struct clk_ops clk_pll_vote_ops = {
> 
> base-commit: 944125b4c454b58d2fe6e35f1087a932b2050dff
> -- 
> 2.43.0
> 

-- 
With best wishes
Dmitry

Re: [PATCH] clk: qcom: clk-pll: reject vote enable on orphan parent

[Konrad Dybcio]

On 6/2/26 8:29 AM, Herman van Hazendonk wrote:
> clk_pll_vote_enable() unconditionally feeds the result of
> clk_hw_get_parent(hw) through to_clk_pll() and on to wait_for_pll().
> The common clock framework permits clk_enable() on an orphan clock
> (supplier not bound yet), in which case clk_hw_get_parent() returns
> NULL. to_clk_pll(NULL) then yields container_of(NULL, struct clk_pll,
> clkr) -- a non-NULL bogus pointer pointing into the negative offset
> of struct clk_pll.
> 
> wait_for_pll() reaches for the parent's name via
> clk_hw_get_name(&pll->clkr.hw). Because clkr sits at a fixed offset
> inside struct clk_pll, &pll->clkr.hw cancels the to_clk_pll offset
> exactly back to NULL and clk_hw_get_name() then dereferences
> core->name on a NULL clk_hw, panicking the kernel.
> 
> This is reachable today: gcc-msm8960.c and gcc-apq8064.c register a
> pll4_vote whose parent (pll4) lives in lcc-msm8960.c, and the future
> gcc-msm8660 pll4_vote does the same. If anything calls clk_enable()
> on pll4_vote between gcc probe and the LCC clock controller binding,
> the system panics. The exposure widens as more SoCs adopt the same
> cross-controller voter pattern.

Hm, I think this is a side-effect of the olden global clock by-name
lookup (i.e. parent_data.name). Nowadays this shouldn't be much of
an issue because of fw_devlink ensuring correct ordering.

There is already a reference in the dts, since about 2022:

80787e417f30 ("ARM: dts: qcom: msm8960: add clocks to the GCC device node")

Are there any other cross-dependencies between GCC and LCC?

Anyway, for this patch:

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad