Re: [PATCH v3] opp: Use clk_get_optional() to avoid leaving opp_table->clk as an error pointer

[Praveen Talari <praveen.talari@oss.qualcomm.com>]

Hi viresh

On 27-07-2026 10:44, Viresh Kumar wrote:
> On 27-07-26, 09:46, Praveen Talari wrote:
>> _update_opp_table_clk() uses clk_get(dev, NULL) to acquire the
>> device's clock. On platforms where the perf domain device has no
>> Linux clock and is instead managed entirely by firmware via
>> devm_pm_opp_of_add_table() (through
>> of_genpd_add_provider_simple()/onecell()), clk_get() returns
>> -ENOENT. That case is treated as valid (the OPP table can still
>> have entries sourced from firmware), but opp_table->clk is left
>> holding ERR_PTR(-ENOENT) rather than being reset to NULL:
>>
>> 	opp_table->clk = clk_get(dev, NULL);
>> 	ret = PTR_ERR_OR_ZERO(opp_table->clk);
>> 	...
>> 	if (ret == -ENOENT) {
>> 		opp_table->clk_count = 1;
>> 		return opp_table;   /* opp_table->clk is still ERR_PTR(-ENOENT) */
>> 	}
>>
>> Consumers that only check IS_ERR(opp_table->clk) treat this as a
>> valid clk and pass it straight into the clk consumer API. In
>> particular, dev_pm_opp_set_rate() calls
>> clk_round_rate(opp_table->clk, target_freq), and clk_round_rate()
>> only guards against a NULL clk, so it dereferences the error pointer
>> to read clk->exclusive_count and crashes:
>>
>>    Unable to handle kernel NULL pointer dereference at virtual
>>    address 000000000000002e
>>    ...
>>    pc : clk_round_rate+0x3c/0x188
>>    ...
>>    Call trace:
>>     clk_round_rate+0x3c/0x188 (P)
>>     dev_pm_opp_set_rate+0x114/0x33c
>>
>> Rather than teaching every clk consumer API to special-case
>> ERR_PTR(-ENOENT), fix it at the source: use clk_get_optional()
>> instead of clk_get() in _update_opp_table_clk(), which already
>> translates -ENOENT into a NULL clk. This documents that the clock is
>> genuinely optional for such devices, and keeps opp_table->clk holding
>> either a valid clk or NULL, never a lingering -ENOENT error pointer.
>> _opp_config_clk_single() is only wired up via opp_table->config_clks
>> when a clk was actually found, and every other opp_table->clk
>> consumer already tolerates NULL through the standard clk API (which
>> treats a NULL clk as a no-op), so no other call site needs to change.
>>
>> Suggested-by: Sebastian Reichel <sre@kernel.org>
>> Reviewed-by: Sebastian Reichel <sre@kernel.org>
>> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
>> ---
> I am getting this with git am -3, please rebase over [1].
I have followed below steps but faced conflict while rebasing it.

git clone https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
git remote add viresh 
https://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git
git fetch viresh
git rebase viresh/cpufreq/arm/linux-next

$git rebase viresh/cpufreq/arm/linux-next
Auto-merging drivers/gpio/gpio-shared-proxy.c
CONFLICT (content): Merge conflict in drivers/gpio/gpio-shared-proxy.c
error: could not apply efecde8a254d... gpio: shared-proxy: always 
serialize with a sleeping mutex
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git 
rebase --abort".

Could not apply efecde8a254d... gpio: shared-proxy: always serialize 
with a sleeping mutex

How to proceed further?

>
> Applying: opp: Use clk_get_optional() to avoid leaving opp_table->clk as an error pointer
> error: unrecognized input
> error: could not build fake ancestor
>
>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>> +	opp_table->clk = clk_get_optional(dev, NULL);
>>   
>>   	ret = PTR_ERR_OR_ZERO(opp_table->clk);
>> -	if (!ret) {
>> -		opp_table->config_clks = _opp_config_clk_single;
>> -		opp_table->clk_count = 1;
>> -		return opp_table;
>> +	if (ret) {
> Since `ret` isn't used later anymore, you can instead do this and drop
> `ret` ?
>          if (IS_ERR(opp_table->clk))

Sure, will fix in next patch.


Thanks,

Praveen Talari

>
>> +		dev_pm_opp_put_opp_table(opp_table);
>> +		dev_err_probe(dev, ret, "Couldn't find clock\n");
>> +		return ERR_PTR(ret);
>>   	}