| Message ID | 3cc5b83b-f81c-4bd7-b7ff-4d02db4e25d8@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | cpufreq: Fix initialization with disabled boost | expand |
On 2025/6/17 3:10, Robin Murphy wrote: > On 2025-06-16 6:25 pm, Christian Loehle wrote: >> The boost_enabled early return in policy_set_boost() caused >> the boost disabled at initialization to not actually set the >> initial policy->max, therefore effectively enabling boost while >> it should have been enabled. >> >> Fixes: 27241c8b63bd ("cpufreq: Introduce policy_set_boost()") > > I think it's a bit older than that - I noticed this with 6.15 stable, prior to that refactoring, and from a poke through the history the underlying logic appears to date back to dd016f379ebc ("cpufreq: Introduce a more generic way to set default per-policy boost flag"). Hopefully someone can figure out the appropriate stable backport. > > I can at least confirm that equivalently hacking out the "&& policy->boost_enabled != cpufreq_boost_enabled()" condition previously here does have the desired effect for me of initialising scaling_max_freq correctly at boot, but I'm not sure that's entirely correct on its own... > > Thanks, > Robin. > >> Reported-by: Robin Murphy <robin.murphy@arm.com> >> Signed-off-by: Christian Loehle <christian.loehle@arm.com> >> --- >> drivers/cpufreq/cpufreq.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >> index d7426e1d8bdd..e85139bd0436 100644 >> --- a/drivers/cpufreq/cpufreq.c >> +++ b/drivers/cpufreq/cpufreq.c >> @@ -1630,7 +1630,7 @@ static int cpufreq_online(unsigned int cpu) >> */ >> if (cpufreq_driver->set_boost && policy->boost_supported && >> (new_policy || !cpufreq_boost_enabled())) { >> - ret = policy_set_boost(policy, cpufreq_boost_enabled()); >> + ret = cpufreq_driver->set_boost(policy, cpufreq_boost_enabled()); >> if (ret) { >> /* If the set_boost fails, the online operation is not affected */ >> pr_info("%s: CPU%d: Cannot %s BOOST\n", __func__, policy->cpu, > > I don't quite understand what problem you've met. It semms like you guys propose that set_boost() should be called no matter what policy->boost_enabled is. Having more details would help to clarify things, such as which driver you use and what you expect but not be achieved.
On 6/17/25 03:14, zhenglifeng (A) wrote: > On 2025/6/17 3:10, Robin Murphy wrote: >> On 2025-06-16 6:25 pm, Christian Loehle wrote: >>> The boost_enabled early return in policy_set_boost() caused >>> the boost disabled at initialization to not actually set the >>> initial policy->max, therefore effectively enabling boost while >>> it should have been enabled. >>> >>> Fixes: 27241c8b63bd ("cpufreq: Introduce policy_set_boost()") >> >> I think it's a bit older than that - I noticed this with 6.15 stable, prior to that refactoring, and from a poke through the history the underlying logic appears to date back to dd016f379ebc ("cpufreq: Introduce a more generic way to set default per-policy boost flag"). Hopefully someone can figure out the appropriate stable backport. >> >> I can at least confirm that equivalently hacking out the "&& policy->boost_enabled != cpufreq_boost_enabled()" condition previously here does have the desired effect for me of initialising scaling_max_freq correctly at boot, but I'm not sure that's entirely correct on its own... >> >> Thanks, >> Robin. >> >>> Reported-by: Robin Murphy <robin.murphy@arm.com> >>> Signed-off-by: Christian Loehle <christian.loehle@arm.com> >>> --- >>> drivers/cpufreq/cpufreq.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >>> index d7426e1d8bdd..e85139bd0436 100644 >>> --- a/drivers/cpufreq/cpufreq.c >>> +++ b/drivers/cpufreq/cpufreq.c >>> @@ -1630,7 +1630,7 @@ static int cpufreq_online(unsigned int cpu) >>> */ >>> if (cpufreq_driver->set_boost && policy->boost_supported && >>> (new_policy || !cpufreq_boost_enabled())) { >>> - ret = policy_set_boost(policy, cpufreq_boost_enabled()); >>> + ret = cpufreq_driver->set_boost(policy, cpufreq_boost_enabled()); >>> if (ret) { >>> /* If the set_boost fails, the online operation is not affected */ >>> pr_info("%s: CPU%d: Cannot %s BOOST\n", __func__, policy->cpu, >> >> > > I don't quite understand what problem you've met. It semms like you guys > propose that set_boost() should be called no matter what > policy->boost_enabled is. Having more details would help to clarify things, > such as which driver you use and what you expect but not be achieved. > so calling policy_set_boost(policy, enable) is a noop here if policy->boost_enabled == cpufreq_boost_enabled(): if (policy->boost_enabled == enable) return 0; We have policy->boost_enabled == false on boot, thus never actually setting policy->max up ever, which leads to the following: # cat /sys/devices/system/cpu/cpufreq/policy4/scaling_boost_frequencies 2016000 # cat /sys/devices/system/cpu/cpufreq/policy4/scaling_max_freq 2016000 # cat /sys/devices/system/cpu/cpufreq/boost 0 # echo 1 > /sys/devices/system/cpu/cpufreq/boost # echo 0 > /sys/devices/system/cpu/cpufreq/boost # cat /sys/devices/system/cpu/cpufreq/policy4/scaling_boost_frequencies 1800000 Anyway I'll bisect some more to find the actual first bad commit and resend.
On 17/06/2025 9:20 am, Christian Loehle wrote: > On 6/17/25 03:14, zhenglifeng (A) wrote: >> On 2025/6/17 3:10, Robin Murphy wrote: >>> On 2025-06-16 6:25 pm, Christian Loehle wrote: >>>> The boost_enabled early return in policy_set_boost() caused >>>> the boost disabled at initialization to not actually set the >>>> initial policy->max, therefore effectively enabling boost while >>>> it should have been enabled. >>>> >>>> Fixes: 27241c8b63bd ("cpufreq: Introduce policy_set_boost()") >>> >>> I think it's a bit older than that - I noticed this with 6.15 stable, prior to that refactoring, and from a poke through the history the underlying logic appears to date back to dd016f379ebc ("cpufreq: Introduce a more generic way to set default per-policy boost flag"). Hopefully someone can figure out the appropriate stable backport. >>> >>> I can at least confirm that equivalently hacking out the "&& policy->boost_enabled != cpufreq_boost_enabled()" condition previously here does have the desired effect for me of initialising scaling_max_freq correctly at boot, but I'm not sure that's entirely correct on its own... >>> >>> Thanks, >>> Robin. >>> >>>> Reported-by: Robin Murphy <robin.murphy@arm.com> >>>> Signed-off-by: Christian Loehle <christian.loehle@arm.com> >>>> --- >>>> drivers/cpufreq/cpufreq.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >>>> index d7426e1d8bdd..e85139bd0436 100644 >>>> --- a/drivers/cpufreq/cpufreq.c >>>> +++ b/drivers/cpufreq/cpufreq.c >>>> @@ -1630,7 +1630,7 @@ static int cpufreq_online(unsigned int cpu) >>>> */ >>>> if (cpufreq_driver->set_boost && policy->boost_supported && >>>> (new_policy || !cpufreq_boost_enabled())) { >>>> - ret = policy_set_boost(policy, cpufreq_boost_enabled()); >>>> + ret = cpufreq_driver->set_boost(policy, cpufreq_boost_enabled()); >>>> if (ret) { >>>> /* If the set_boost fails, the online operation is not affected */ >>>> pr_info("%s: CPU%d: Cannot %s BOOST\n", __func__, policy->cpu, >>> >>> >> >> I don't quite understand what problem you've met. It semms like you guys >> propose that set_boost() should be called no matter what >> policy->boost_enabled is. Having more details would help to clarify things, >> such as which driver you use and what you expect but not be achieved. >> > > so calling policy_set_boost(policy, enable) is a noop here if > policy->boost_enabled == cpufreq_boost_enabled(): > > if (policy->boost_enabled == enable) > return 0; > > We have policy->boost_enabled == false on boot, thus never actually > setting policy->max up ever, which leads to the following: And for clarity, this is with the cpufreq_dt driver (at least in my case). Thanks, Robin. > # cat /sys/devices/system/cpu/cpufreq/policy4/scaling_boost_frequencies > 2016000 > # cat /sys/devices/system/cpu/cpufreq/policy4/scaling_max_freq > 2016000 > # cat /sys/devices/system/cpu/cpufreq/boost > 0 > # echo 1 > /sys/devices/system/cpu/cpufreq/boost > # echo 0 > /sys/devices/system/cpu/cpufreq/boost > # cat /sys/devices/system/cpu/cpufreq/policy4/scaling_boost_frequencies > 1800000 > > Anyway I'll bisect some more to find the actual first bad commit and > resend.
On 6/18/25 15:32, Rafael J. Wysocki wrote: > On Mon, Jun 16, 2025 at 7:25 PM Christian Loehle > <christian.loehle@arm.com> wrote: >> >> The boost_enabled early return in policy_set_boost() caused >> the boost disabled at initialization to not actually set the >> initial policy->max, therefore effectively enabling boost while >> it should have been enabled. > > Did you mean "disabled"? Yup, the latter 'enabled' should be disabled. > > It would be good to mention the failure scenario here too. > Absolutely, let me respin this in a series that provides some context, too. >> Fixes: 27241c8b63bd ("cpufreq: Introduce policy_set_boost()") >> Reported-by: Robin Murphy <robin.murphy@arm.com> >> Signed-off-by: Christian Loehle <christian.loehle@arm.com> >> --- >> drivers/cpufreq/cpufreq.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >> index d7426e1d8bdd..e85139bd0436 100644 >> --- a/drivers/cpufreq/cpufreq.c >> +++ b/drivers/cpufreq/cpufreq.c >> @@ -1630,7 +1630,7 @@ static int cpufreq_online(unsigned int cpu) >> */ >> if (cpufreq_driver->set_boost && policy->boost_supported && >> (new_policy || !cpufreq_boost_enabled())) { >> - ret = policy_set_boost(policy, cpufreq_boost_enabled()); >> + ret = cpufreq_driver->set_boost(policy, cpufreq_boost_enabled()); >> if (ret) { >> /* If the set_boost fails, the online operation is not affected */ >> pr_info("%s: CPU%d: Cannot %s BOOST\n", __func__, policy->cpu, >> -- >> 2.34.1
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index d7426e1d8bdd..e85139bd0436 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1630,7 +1630,7 @@ static int cpufreq_online(unsigned int cpu) */ if (cpufreq_driver->set_boost && policy->boost_supported && (new_policy || !cpufreq_boost_enabled())) { - ret = policy_set_boost(policy, cpufreq_boost_enabled()); + ret = cpufreq_driver->set_boost(policy, cpufreq_boost_enabled()); if (ret) { /* If the set_boost fails, the online operation is not affected */ pr_info("%s: CPU%d: Cannot %s BOOST\n", __func__, policy->cpu,
The boost_enabled early return in policy_set_boost() caused the boost disabled at initialization to not actually set the initial policy->max, therefore effectively enabling boost while it should have been enabled. Fixes: 27241c8b63bd ("cpufreq: Introduce policy_set_boost()") Reported-by: Robin Murphy <robin.murphy@arm.com> Signed-off-by: Christian Loehle <christian.loehle@arm.com> --- drivers/cpufreq/cpufreq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)