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@xxxxxxxxxx>
> ---
>  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