Re: [PATCH 2/2] clk: qcom: gcc-msm8660: register PLL4_VOTE for LPASS

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] DT Binding Schema mismatch due to missing input clock documentation.

Pre-existing issues:
- [High] NULL pointer dereference when enabling an orphan `pll4_vote` clock.
--

commit 7c30a11d991e96ae79ebcc182c62453a708e5ec5
Author: Herman van Hazendonk <github.com@herrie.org>

clk: qcom: gcc-msm8660: register PLL4_VOTE for LPASS

Add the CPU-side software vote register for LPASS PLL4. PLL4 itself
lives in the LCC (Low Power Audio Subsystem clock controller); GCC
holds the apps-processor vote in PLL_ENA_SC0 (0x34c0) BIT(4).

> diff --git a/drivers/clk/qcom/gcc-msm8660.c b/drivers/clk/qcom/gcc-msm8660.c
> index e81b8851a7861..cd392e140e95c 100644
> --- a/drivers/clk/qcom/gcc-msm8660.c
> +++ b/drivers/clk/qcom/gcc-msm8660.c
[ ... ]
> @@ -54,6 +54,20 @@ static struct clk_regmap pll8_vote = {
>  	},
>  };
>  
> +/* PLL4 is the LPASS PLL, defined in LCC. This is the voting clock. */
> +static struct clk_regmap pll4_vote = {
> +	.enable_reg = 0x34c0,
> +	.enable_mask = BIT(4),
> +	.hw.init = &(struct clk_init_data){
> +		.name = "pll4_vote",
> +		.parent_data = &(const struct clk_parent_data){
> +			.fw_name = "pll4", .name = "pll4",

[Severity: High]
This is a pre-existing issue, but will this cause a NULL pointer dereference
if the LCC driver is not loaded or the DT node is disabled, leaving pll4_vote
as an orphan?

The Common Clock Framework permits clk_enable() to be called on orphan clocks.
If this happens, clk_pll_vote_enable() fetches the parent via
clk_hw_get_parent(), which returns NULL.

It then unconditionally casts this NULL to struct clk_pll using to_clk_pll().
Because clkr is the last member of struct clk_pll, this yields a non-NULL
invalid pointer offset:

drivers/clk/qcom/clk-pll.c:clk_pll_vote_enable() {
    struct clk_pll *p = to_clk_pll(clk_hw_get_parent(hw));
    ...
    return wait_for_pll(p);
}

This pointer is passed to wait_for_pll(), which calls
clk_hw_get_name(&pll->clkr.hw). The &pll->clkr.hw calculation perfectly
reverses the offset, yielding exactly NULL:

drivers/clk/qcom/clk-pll.c:wait_for_pll() {
    const char *name = clk_hw_get_name(&pll->clkr.hw);
    ...
}

clk_hw_get_name() then dereferences NULL->core->name, causing a panic. This
patch exposes this pre-existing flaw by introducing the first clk_pll_vote_ops
clock with an external parent.

[Severity: Medium]
Will this fw_name direct the clock framework to look for a clock named pll4
in the clock-names property of the qcom,gcc-msm8660 device tree node?

The YAML schema for this binding
(Documentation/devicetree/bindings/clock/qcom,gcc-msm8660.yaml) strictly
enforces maxItems: 2 for the clocks property and only permits pxo and cxo
in clock-names.

Any device tree that correctly supplies pll4 to satisfy this lookup will fail
dt_binding_check validation. Should the DT binding schema be updated alongside
this patch to accommodate the new input clock?

> +		},
> +		.num_parents = 1,
> +		.ops = &clk_pll_vote_ops,
> +	},
> +};
> +
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260602042747.277270-1-github.com@herrie.org?part=2