Re: [PATCH v3] opp: Use clk_get_optional() to avoid leaving opp_table->clk as an error pointer
From: Praveen Talari
Date: Mon Jul 27 2026 - 05:48:10 EST
Hi viresh
On 27-07-2026 10:44, Viresh Kumar wrote:
On 27-07-26, 09:46, Praveen Talari wrote:I have followed below steps but faced conflict while rebasing it.
_update_opp_table_clk() uses clk_get(dev, NULL) to acquire theI am getting this with git am -3, please rebase over [1].
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@xxxxxxxxxx>
Reviewed-by: Sebastian Reichel <sre@xxxxxxxxxx>
Signed-off-by: Praveen Talari <praveen.talari@xxxxxxxxxxxxxxxx>
---
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.cSince `ret` isn't used later anymore, you can instead do this and drop
+ 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) {
`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);
}