Re: [PATCH v6 2/5] ASoC: qcom: qdsp6: Increase Q6DSP_MAX_CLK_ID for LPASS LPR vote clock

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The LPASS LPR vote clock is supported by the array bound increase, but the clock itself is never actually instantiated in the provider tables.

Pre-existing issues:
- [High] Clock operations can be called before device drvdata is initialized, leading to a NULL pointer dereference.
- [Medium] Concurrent or multiple OF clock lookups mutate shared clock attributes locklessly.
--

--- Patch [2]: [PATCH v6 2/5] ASoC: qcom: qdsp6: Increase Q6DSP_MAX_CLK_ID for LPASS LPR vote clock ---
commit c2ccafc9793d5fdece974aa409ae112fa2b07ae4
Author: Prasad Kumpatla <prasad.kumpatla@oss.qualcomm.com>

ASoC: qcom: qdsp6: Increase Q6DSP_MAX_CLK_ID for LPASS LPR vote clock
    
Q6DSP_MAX_CLK_ID defines the upper bound of supported clock
identifiers in the qdsp6 LPASS clock driver.
    
Increase the maximum clock ID value to accommodate the LPASS
LPR vote clock identifier.

> diff --git a/sound/soc/qcom/qdsp6/q6dsp-lpass-clocks.c b/sound/soc/qcom/qdsp6/q6dsp-lpass-clocks.c
> index 03838582aeade..ab7d20580638b 100644
> --- a/sound/soc/qcom/qdsp6/q6dsp-lpass-clocks.c
> +++ b/sound/soc/qcom/qdsp6/q6dsp-lpass-clocks.c
> @@ -12,7 +12,7 @@
>  #include <dt-bindings/sound/qcom,q6dsp-lpass-ports.h>
>  #include "q6dsp-lpass-clocks.h"
>  
> -#define Q6DSP_MAX_CLK_ID			104
> +#define Q6DSP_MAX_CLK_ID			105

[Severity: Medium]
Will consumers actually be able to get this clock?  While the bounds limit has
been increased, it does not appear that the clock is ever instantiated in the
provider tables (like q6afe_clks[] or q6prm_clks[]).

If a consumer requests LPASS_HW_LPR_VOTE (104), cc->clks[104] will remain NULL
during lookup, resulting in a safe but incorrect fallback:

sound/soc/qcom/qdsp6/q6dsp-lpass-clocks.c:q6dsp_of_clk_hw_get() {
    ...
	if (cc->clks[idx]) {
		cc->clks[idx]->attributes = attr;
		return &cc->clks[idx]->hw;
	}

	return ERR_PTR(-ENOENT);
}

Is there a missing clock provider array update in this patch?

>  #define Q6DSP_LPASS_CLK_ROOT_DEFAULT		0

[Severity: High]
This is a pre-existing issue, but can this lead to a NULL pointer dereference
during early clock operations?

The driver registers the hardware clock provider before setting the device
drvdata:

sound/soc/qcom/qdsp6/q6dsp-lpass-clocks.c:q6dsp_clock_dev_probe() {
    ...
	ret = devm_of_clk_add_hw_provider(dev, q6dsp_of_clk_hw_get, cc);
	if (ret)
		return ret;

	dev_set_drvdata(dev, cc);

	return 0;
}

If an asynchronous or deferred probe consumer requests the clock and calls
the prepare operations, the callback fetches drvdata which will be NULL:

sound/soc/qcom/qdsp6/q6dsp-lpass-clocks.c:clk_q6dsp_prepare() {
	struct q6dsp_clk *clk = to_q6dsp_clk(hw);
	struct q6dsp_cc *cc = dev_get_drvdata(clk->dev);

	return cc->desc->lpass_set_clk(clk->dev, clk->q6dsp_clk_id, clk->attributes,
                                 ...
}

Should dev_set_drvdata() be moved before devm_of_clk_add_hw_provider()?

[Severity: Medium]
This is also a pre-existing issue, but does this clock lookup introduce a data
race if multiple consumers share the same clock?

When a consumer requests a clock via of_clk_get(), the shared clock
attributes are mutated locklessly based on the caller's arguments:

sound/soc/qcom/qdsp6/q6dsp-lpass-clocks.c:q6dsp_of_clk_hw_get() {
    ...
	if (cc->clks[idx]) {
		cc->clks[idx]->attributes = attr;
		return &cc->clks[idx]->hw;
	}
    ...
}

If multiple consumers request the same clock concurrently, this could race.
Additionally, the last consumer's attributes will silently overwrite those of
earlier consumers, potentially causing subsequent prepare operations for all
consumers to execute with incorrect attributes.

Could this shared state mutation be avoided or protected?

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