[PATCH] firmware: arm_scmi: perf: ignore an implausible sustained frequency

[PATCH] firmware: arm_scmi: perf: ignore an implausible sustained frequency

[Charlie Garner]

On a Dell Inspiron 14 Plus 7441 (Snapdragon X Plus X1P64100, soc_id 615)
the SCMI firmware reports a sustained frequency below the lowest available
OPP for performance domains NCC1 and NCC2. It reports zero for both
sustained_freq_khz and sustained_perf_level there, while NCC0 reports
3417600 kHz at level 12. All three domains use level indexing mode, so
mult_factor is fixed at 1000 and the OPP frequencies come from
indicative_freq; this is not a units or mult_factor problem.

scmi_dvfs_device_opps_add() then flags every OPP in NCC1 and NCC2 as turbo:

	data.turbo = freq > dom->sustained_freq_khz * 1000UL;

cpufreq_frequency_table_cpuinfo() skips boost-flagged entries and fails
when none are left:

	if ((!cpufreq_boost_enabled() || !policy->boost_enabled)
	    && (pos->flags & CPUFREQ_BOOST_FREQ))
		continue;
	...
	if (min_freq == ~0)
		return -EINVAL;

cpufreq_policy_online() drops the policy on that error without logging
anything. Only one of the three performance domains ends up with a policy.
The part has 10 cores (the x1e80100 DT describes 12, CPUs 7 and 11 fail to
boot): 4 of them can scale, the other 6 get no policy, no governor, and no
cpufreq cooling device.

This cannot be worked around by enabling boost. policy->boost_enabled is
still 0 during that validation, and both places that set it -
boost_supported in cpufreq_table_validate_and_sort(), boost_enabled in
cpufreq_online() - run after the call that already returned -EINVAL. A
domain whose OPPs are all flagged turbo can therefore never get a policy.

Confirmed by probing dev_pm_opp_add_dynamic() on the affected machine: all
13 OPPs are added with turbo=0 for domain NCC0 and turbo=1 for every CPU in
NCC1 and NCC2, across an identical 710400-3417600 kHz table. The raw domain
attributes above were read the same way, with a kprobe on
scmi_dvfs_device_opps_add() fetching the perf_dom_info fields.

A sustained frequency below the lowest OPP carries no information - it
cannot separate sustained levels from boost levels. Treat it as "this
domain has no turbo levels" instead of letting it disable the domain
entirely, and say so once with a FW_BUG warning, since the failure is
otherwise silent. A sustained frequency equal to the lowest OPP is left
alone, it already leaves that OPP non-turbo.

Fixes: a897575e79d7 ("firmware: arm_scmi: Add support for marking certain frequencies as turbo")
Cc: stable@vger.kernel.org
Signed-off-by: Charlie Garner <charlie@akao.au>
---

Notes:
    The SCMI quirks framework (quirks.c) would also work here, and this
    platform already has two quirks enabled. I went with a generic check
    because a sustained frequency below every OPP is meaningless on any
    platform, and the check is a no-op for firmware that reports a sane value.
    Happy to turn it into a quirk if you'd prefer that.

 drivers/firmware/arm_scmi/perf.c | 34 ++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index 4583d02bee1c..94f644996f9d 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -861,11 +861,20 @@ static void scmi_perf_domain_init_fc(const struct scmi_protocol_handle *ph,
 	dom->fc_info = fc;
 }
 
+static unsigned long scmi_perf_opp_freq(const struct perf_dom_info *dom,
+					int idx)
+{
+	if (!dom->level_indexing_mode)
+		return dom->opp[idx].perf * dom->mult_factor;
+
+	return dom->opp[idx].indicative_freq * dom->mult_factor;
+}
+
 static int scmi_dvfs_device_opps_add(const struct scmi_protocol_handle *ph,
 				     struct device *dev, u32 domain)
 {
 	int idx, ret;
-	unsigned long freq;
+	unsigned long freq, sustained_hz, lowest_hz = ULONG_MAX;
 	struct dev_pm_opp_data data = {};
 	struct perf_dom_info *dom;
 
@@ -873,14 +882,27 @@ static int scmi_dvfs_device_opps_add(const struct scmi_protocol_handle *ph,
 	if (IS_ERR(dom))
 		return PTR_ERR(dom);
 
+	for (idx = 0; idx < dom->opp_count; idx++)
+		lowest_hz = min(lowest_hz, scmi_perf_opp_freq(dom, idx));
+
+	/*
+	 * A sustained frequency below every OPP would mark all of them as
+	 * turbo. Such a value cannot separate sustained levels from boost
+	 * levels, so ignore it and treat the domain as having no turbo OPPs.
+	 */
+	sustained_hz = dom->sustained_freq_khz * 1000UL;
+	if (dom->opp_count && sustained_hz < lowest_hz) {
+		dev_warn_once(dev, FW_BUG
+			      "[%d][%s]: sustained freq %lu Hz below lowest OPP %lu Hz, ignored\n",
+			      domain, dom->info.name, sustained_hz, lowest_hz);
+		sustained_hz = ULONG_MAX;
+	}
+
 	for (idx = 0; idx < dom->opp_count; idx++) {
-		if (!dom->level_indexing_mode)
-			freq = dom->opp[idx].perf * dom->mult_factor;
-		else
-			freq = dom->opp[idx].indicative_freq * dom->mult_factor;
+		freq = scmi_perf_opp_freq(dom, idx);
 
 		/* All OPPs above the sustained frequency are treated as turbo */
-		data.turbo = freq > dom->sustained_freq_khz * 1000UL;
+		data.turbo = freq > sustained_hz;
 
 		data.level = dom->opp[idx].perf;
 		data.freq = freq;

base-commit: 60a89ec8d8f56dcd99611cb054fbf7d0e864cf4e
-- 
2.55.0

Re: [PATCH] firmware: arm_scmi: perf: ignore an implausible sustained frequency

[Sudeep Holla]

On Thu, Sep 24, 2026 at 12:52:45AM +0930, Charlie Garner wrote:
> On a Dell Inspiron 14 Plus 7441 (Snapdragon X Plus X1P64100, soc_id 615)
> the SCMI firmware reports a sustained frequency below the lowest available
> OPP for performance domains NCC1 and NCC2. It reports zero for both
> sustained_freq_khz and sustained_perf_level there, while NCC0 reports
> 3417600 kHz at level 12. All three domains use level indexing mode, so
> mult_factor is fixed at 1000 and the OPP frequencies come from
> indicative_freq; this is not a units or mult_factor problem.
> 
> scmi_dvfs_device_opps_add() then flags every OPP in NCC1 and NCC2 as turbo:
> 
> 	data.turbo = freq > dom->sustained_freq_khz * 1000UL;
> 
> cpufreq_frequency_table_cpuinfo() skips boost-flagged entries and fails
> when none are left:
> 
> 	if ((!cpufreq_boost_enabled() || !policy->boost_enabled)
> 	    && (pos->flags & CPUFREQ_BOOST_FREQ))
> 		continue;
> 	...
> 	if (min_freq == ~0)
> 		return -EINVAL;
> 
> cpufreq_policy_online() drops the policy on that error without logging
> anything. Only one of the three performance domains ends up with a policy.
> The part has 10 cores (the x1e80100 DT describes 12, CPUs 7 and 11 fail to
> boot): 4 of them can scale, the other 6 get no policy, no governor, and no
> cpufreq cooling device.
> 
> This cannot be worked around by enabling boost. policy->boost_enabled is
> still 0 during that validation, and both places that set it -
> boost_supported in cpufreq_table_validate_and_sort(), boost_enabled in
> cpufreq_online() - run after the call that already returned -EINVAL. A
> domain whose OPPs are all flagged turbo can therefore never get a policy.
> 
> Confirmed by probing dev_pm_opp_add_dynamic() on the affected machine: all
> 13 OPPs are added with turbo=0 for domain NCC0 and turbo=1 for every CPU in
> NCC1 and NCC2, across an identical 710400-3417600 kHz table. The raw domain
> attributes above were read the same way, with a kprobe on
> scmi_dvfs_device_opps_add() fetching the perf_dom_info fields.
> 
> A sustained frequency below the lowest OPP carries no information - it
> cannot separate sustained levels from boost levels. Treat it as "this
> domain has no turbo levels" instead of letting it disable the domain
> entirely, and say so once with a FW_BUG warning, since the failure is
> otherwise silent. A sustained frequency equal to the lowest OPP is left
> alone, it already leaves that OPP non-turbo.
> 
> Fixes: a897575e79d7 ("firmware: arm_scmi: Add support for marking certain frequencies as turbo")
> Cc: stable@vger.kernel.org
> Signed-off-by: Charlie Garner <charlie@akao.au>
> ---
> 
> Notes:
>     The SCMI quirks framework (quirks.c) would also work here, and this
>     platform already has two quirks enabled. I went with a generic check
>     because a sustained frequency below every OPP is meaningless on any
>     platform, and the check is a no-op for firmware that reports a sane value.
>     Happy to turn it into a quirk if you'd prefer that.
> 

I am unable to decide which is better approach. I am more inclined to quirks
so that other systems are not penalised by unnecessary checks but at the same
time it seems like a sensible generic check.

Cristian,

Any strong opinion/inclination ?

>  drivers/firmware/arm_scmi/perf.c | 34 ++++++++++++++++++++++++++------
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
> index 4583d02bee1c..94f644996f9d 100644
> --- a/drivers/firmware/arm_scmi/perf.c
> +++ b/drivers/firmware/arm_scmi/perf.c
> @@ -861,11 +861,20 @@ static void scmi_perf_domain_init_fc(const struct scmi_protocol_handle *ph,
>  	dom->fc_info = fc;
>  }
>  
> +static unsigned long scmi_perf_opp_freq(const struct perf_dom_info *dom,
> +					int idx)
> +{
> +	if (!dom->level_indexing_mode)
> +		return dom->opp[idx].perf * dom->mult_factor;
> +
> +	return dom->opp[idx].indicative_freq * dom->mult_factor;
> +}
> +
>  static int scmi_dvfs_device_opps_add(const struct scmi_protocol_handle *ph,
>  				     struct device *dev, u32 domain)
>  {
>  	int idx, ret;
> -	unsigned long freq;
> +	unsigned long freq, sustained_hz, lowest_hz = ULONG_MAX;
>  	struct dev_pm_opp_data data = {};
>  	struct perf_dom_info *dom;
>  
> @@ -873,14 +882,27 @@ static int scmi_dvfs_device_opps_add(const struct scmi_protocol_handle *ph,
>  	if (IS_ERR(dom))
>  		return PTR_ERR(dom);
>  
> +	for (idx = 0; idx < dom->opp_count; idx++)
> +		lowest_hz = min(lowest_hz, scmi_perf_opp_freq(dom, idx));
> +

Just wondering how badly the perf values are also screwed up on this system ?
If we are adding it as generic, I am thinking of just using the sorted
opp list and lowest_hz = scmi_perf_opp_freq(dom, 0), no ? It avoids
unnecessary loop above.

-- 
Regards,
Sudeep