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

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

[Praveen Talari]

_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>
---
Changes in v3:
- Added OPP framework maintainers
- Link to v2: https://patch.msgid.link/20260725-fix_ptr_check_on_clk-v2-1-7f269f7d33f3@oss.qualcomm.com

Changes in v2:
- Switched from guarding clk_round_rate() against error pointers to
  fixing the root cause in OPP core: use clk_get_optional() instead
  of clk_get() in _update_opp_table_clk(), so opp_table->clk is left
  as NULL (not ERR_PTR(-ENOENT)) when a device has no Linux clock,
  per Sebastian Reichel's review suggestion.
- Dropped the drivers/clk/clk.c change entirely.
- Link to v1: https://patch.msgid.link/20260723-fix_ptr_check_on_clk-v1-1-568a7ed87746@oss.qualcomm.com

To: konrad.dybcio@oss.qualcomm.com
To: Viresh Kumar <vireshk@kernel.org>
To: Nishanth Menon <nm@ti.com>
To: Stephen Boyd <sboyd@kernel.org>
Cc: chandana.chiluveru@oss.qualcomm.com
Cc: linux-pm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/opp/core.c | 51 ++++++++++++++++++++++++---------------------------
 1 file changed, 24 insertions(+), 27 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index b6966e509f7d..80da241d40f5 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1591,39 +1591,36 @@ static struct opp_table *_update_opp_table_clk(struct device *dev,
 	    opp_table->clks)
 		return opp_table;
 
-	/* Find clk for the device */
-	opp_table->clk = clk_get(dev, NULL);
+	/*
+	 * There are few platforms which don't want the OPP core to manage
+	 * device's clock settings. In such cases neither the platform
+	 * provides the clks explicitly to us, nor the DT contains a valid
+	 * clk entry. The OPP nodes in DT may still contain "opp-hz" property
+	 * though, which we need to parse and allow the platform to find an
+	 * OPP based on freq later on.
+	 *
+	 * This is a simple solution to take care of such corner cases, i.e.
+	 * make the clk_count 1, which lets us allocate space for frequency
+	 * in opp->rates and also parse the entries in DT. Use
+	 * clk_get_optional() instead of clk_get() so opp_table->clk stays
+	 * NULL for such devices, instead of holding an ERR_PTR(-ENOENT) that
+	 * consumers must remember to special-case.
+	 */
+	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) {
+		dev_pm_opp_put_opp_table(opp_table);
+		dev_err_probe(dev, ret, "Couldn't find clock\n");
+		return ERR_PTR(ret);
 	}
 
-	if (ret == -ENOENT) {
-		/*
-		 * There are few platforms which don't want the OPP core to
-		 * manage device's clock settings. In such cases neither the
-		 * platform provides the clks explicitly to us, nor the DT
-		 * contains a valid clk entry. The OPP nodes in DT may still
-		 * contain "opp-hz" property though, which we need to parse and
-		 * allow the platform to find an OPP based on freq later on.
-		 *
-		 * This is a simple solution to take care of such corner cases,
-		 * i.e. make the clk_count 1, which lets us allocate space for
-		 * frequency in opp->rates and also parse the entries in DT.
-		 */
-		opp_table->clk_count = 1;
-
-		dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__, ret);
-		return opp_table;
-	}
+	if (opp_table->clk)
+		opp_table->config_clks = _opp_config_clk_single;
 
-	dev_pm_opp_put_opp_table(opp_table);
-	dev_err_probe(dev, ret, "Couldn't find clock\n");
+	opp_table->clk_count = 1;
 
-	return ERR_PTR(ret);
+	return opp_table;
 }
 
 /*

---
base-commit: b4515cf4156356e8f4fe6e0fdc17f59adab9772f
change-id: 20260723-fix_ptr_check_on_clk-603605c3e350

Best regards,
--  
Praveen Talari <praveen.talari@oss.qualcomm.com>

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

[Viresh Kumar]

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

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))

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

-- 
viresh

[1] https://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git/log/?h=cpufreq/arm/linux-next

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

[Praveen Talari]

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);
>>   	}

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

[Viresh Kumar]

On 27-07-26, 15:11, Praveen Talari wrote:
> 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

Looks like you are trying to rebase Linus's master branch over my
branch ? This commit is already merged in the kernel and you shouldn't
be rebasing it over.

Please apply the $subject patch only over my branch, maybe with git
cherry-pick, and then send me the same.

> How to proceed further?

-- 
viresh