Re: [PATCH v3 1/4] cpufreq: CPPC: Keep the policy across CPU hotplug

On 27/07/26 18:57, Christian Loehle wrote:
> External email: Use caution opening links or attachments
>
>
> On 7/24/26 22:59, Sumit Gupta wrote:
>> Without online()/offline() callbacks, the cpufreq core fully tears
>> down a policy during exit() when its last online CPU is offlined, and
>> rebuilds it during init() when it comes back.
>>
>> Add lightweight online()/offline() callbacks so the core instead keeps
>> the policy live and reuses the driver's cpu_data across CPU hotplug.
>> This avoids re-reading the CPPC capabilities on every offline/online,
>> making CPU hotplug faster.
>>
>>  From online(), re-enable CPPC, as the platform may have disabled it
>> while the CPU was offline. Since the policy is no longer rebuilt via
>> init(), online() also reprograms the CPPC performance controls
>> (desired/min/max).
>>
>> Signed-off-by: Sumit Gupta <[email protected]>
>> ---
>>   drivers/cpufreq/cppc_cpufreq.c | 44 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 44 insertions(+)
>>
>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>> index f6cea0c54dd9..6dc59f99d880 100644
>> --- a/drivers/cpufreq/cppc_cpufreq.c
>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>> @@ -722,6 +722,48 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>>        return ret;
>>   }
>>
>> +/*
>> + * With offline() defined, the cpufreq core keeps the policy alive when
>> + * a CPU is hotplugged out.
>> + */
>> +static int cppc_cpufreq_cpu_offline(struct cpufreq_policy *policy)
>> +{
>> +     return 0;
>> +}
>> +
>> +/*
>> + * Re-enable CPPC when the policy's CPU comes back online, since the platform
>> + * may have disabled it while the CPU was offline.
>> + */
>> +static int cppc_cpufreq_cpu_online(struct cpufreq_policy *policy)
>> +{
>> +     struct cppc_cpudata *cpu_data = policy->driver_data;
>> +     unsigned int cpu = policy->cpu;
>> +     int ret;
>> +
>> +     ret = cppc_set_enable(cpu, true);
>> +     if (ret && ret != -EOPNOTSUPP)
>> +             pr_warn("Failed to re-enable CPPC for CPU%d (%d)\n", cpu, ret);
> What's the intention of continuing restoring CPPC register values here?

Yes, there is no benefit in continuing the restore when
cppc_set_enable() fails with anything other than -EOPNOTSUPP.
I will stop the restore at that point and return 0 with a warning,
since propagating the error would cause the core to tear down
the policy.

>> +
>> +     /*
>> +      * The platform may reset the controls while the CPU is offline, so
>> +      * recompute min/max, clamp desired_perf into range, and reprogram them.
>> +      */
>> +     cppc_cpufreq_update_perf_limits(cpu_data, policy);
>> +
>> +     cpu_data->perf_ctrls.desired_perf =
>> +             clamp_t(u32, cpu_data->perf_ctrls.desired_perf,
>> +                     cpu_data->perf_ctrls.min_perf,
>> +                     cpu_data->perf_ctrls.max_perf);
>> +
>> +     ret = cppc_set_perf(cpu, &cpu_data->perf_ctrls);
>> +     if (ret)
>> +             pr_debug("Failed to reapply perf request on CPU%d (%d)\n",
>> +                      cpu, ret);
> I think cppc_set_perf() needs some prep first before using it on reset values.
> We assume that reset value may be Autonomous Mode on, right? So we must never
> write MIN>MAX and vice versa. I think we may just have to read and write
> the 'otherwise-offending' value first on reset.

You are right. cppc_set_perf() writes MIN before MAX, so restoring
a window whose MIN is above the reset MAX can briefly produce MIN > MAX
on registers not accessed through PCC. I will add a preparatory write
that raises MAX in that case, followed by the full restore:
----
   /* min/max recomputed and desired_perf clamped, as posted */

   cppc_get_perf(cpu, &cur);

   if (cpu_data->perf_ctrls.min_perf > cur.max_perf) {
     prep = cur;              /* Rewrites DESIRED with its current value. */
     prep.min_perf = 0;  /* Zero leaves MIN unchanged. */
     prep.max_perf = cpu_data->perf_ctrls.max_perf;

     /* Raise MAX first. */
     cppc_set_perf(cpu, &prep);
   }

   /* Then restore the full desired/min/max request. */
   cppc_set_perf(cpu, &cpu_data->perf_ctrls);
----

The reverse transition does not need this extra write because the
existing MIN-before-MAX order writes the lower MIN first, so the window
only widens before MAX comes down.

Please let me know if you see any issue with this.

Thanks,
Sumit
....