Re: [PATCH v6 1/5] ASoC: dt-bindings: qcom: add LPASS LPR vote clock ID

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Adding clock ID LPASS_HW_LPR_VOTE (104) without incrementing Q6DSP_MAX_CLK_ID in the clock driver prevents its use and introduces a latent out-of-bounds array access.
- [Medium] The new clock ID `LPASS_HW_LPR_VOTE` is defined in the dt-bindings header but is completely unimplemented in the actual clock provider driver, making it unusable.
--

--- Patch [1]: [PATCH v6 1/5] ASoC: dt-bindings: qcom: add LPASS LPR vote clock ID ---
commit f9999fdb82c99c4b2b674da0fc47d8921c55373d
Author: Prasad Kumpatla <prasad.kumpatla@oss.qualcomm.com>

ASoC: dt-bindings: qcom: add LPASS LPR vote clock ID

This patch adds a new clock ID, LPASS_HW_LPR_VOTE (104), to the dt-bindings
for the LPASS low-power resource (LPR) vote.

> Add a new clock ID, LPASS_HW_LPR_VOTE, to represent the LPASS low-power
> resource (LPR) vote through the PRM interface.
> 
> The LPASS PRM supports a resource voting mechanism to control low-power
> states via PARAM_ID_RSC_CPU_LPR. Exposing this as a q6prm clock ID allows
> clients to request the LPR vote using the existing qcom,q6prm clock
> provider interface.

[Severity: Medium]
Is the clock provider implementation missing from this patch?

The existing qcom,q6prm clock provider interface doesn't appear to have this
new ID wired up in the q6prm_clks array in sound/soc/qcom/qdsp6/q6prm-clocks.c.
Without adding it there, clients won't actually be able to request the clock
as described in the commit message.

[ ... ]

> diff --git a/include/dt-bindings/sound/qcom,q6dsp-lpass-ports.h b/include/dt-bindings/sound/qcom,q6dsp-lpass-ports.h
> index 7b553a73bc928..8e04106d48be5 100644
> --- a/include/dt-bindings/sound/qcom,q6dsp-lpass-ports.h
> +++ b/include/dt-bindings/sound/qcom,q6dsp-lpass-ports.h
> @@ -236,6 +236,7 @@
>  #define LPASS_HW_AVTIMER_VOTE		101
>  #define LPASS_HW_MACRO_VOTE		102
>  #define LPASS_HW_DCODEC_VOTE		103
> +#define LPASS_HW_LPR_VOTE		104

[Severity: High]
Will this new ID get rejected by the driver's bounds checking?

In sound/soc/qcom/qdsp6/q6dsp-lpass-clocks.c, Q6DSP_MAX_CLK_ID is currently
defined as 104. When a client requests this new clock, the validation check
in q6dsp_of_clk_hw_get() will fail:

    if (idx >= Q6DSP_MAX_CLK_ID || attr > LPASS_CLK_ATTRIBUTE_COUPLE_DIVISOR) {
        dev_err(cc->dev, "Invalid clk specifier (%d, %d)\n", idx, attr);
        return ERR_PTR(-EINVAL);
    }

Since 104 >= 104 is true, it will return -EINVAL.

Additionally, if the new clock is wired up later in the provider array without
increasing Q6DSP_MAX_CLK_ID, q6dsp_clock_dev_probe() will perform an
out-of-bounds write because the array is sized based on the max ID:

    cc->clks[id] = clk;

Does Q6DSP_MAX_CLK_ID need to be incremented to accommodate this new ID?

>  
>  #define LPASS_CLK_ATTRIBUTE_INVALID		0x0
>  #define LPASS_CLK_ATTRIBUTE_COUPLE_NO		0x1

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724141708.2212057-1-prasad.kumpatla@oss.qualcomm.com?part=1