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

On 7/24/2026 7:59 PM, [email protected] wrote:
> 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 <[email protected]>
>
> 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.
Thanks for review.

The corresponding clock provider support is added in patch 2 of the
series, where LPASS_HW_LPR_VOTE is registered in q6prm_clks[] and
the required PRM handling is implemented.

As a result, clients can request this clock through the existing
qcom,q6prm clock provider interface when the full series is applied.

>
> [ ... ]
>
>> 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?

This is addressed in patch 2 of the series. Along with adding support 
for |LPASS_HW_LPR_VOTE|,

patch 2 increases |Q6DSP_MAX_CLK_ID| to accommodate the new clock ID and 
updates the clock
provider accordingly.

As a result, clock ID 104 is accepted by the bounds checking and the 
clock array remains correctly sized.

Thanks,
Prasad

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