Re: [PATCH v3 3/4] cpufreq: ACPI: Use IPI to update boost MSR in cpufreq_boost_down_prep()

[Zhongqiu Han <zhongqiu.han@oss.qualcomm.com>]

On 7/17/2026 3:11 AM, Jim Mattson wrote:
> On Thu, Jun 25, 2026 at 8:50 AM Zhongqiu Han
> <zhongqiu.han@oss.qualcomm.com> wrote:
> 
>> Hi Rafael,
>> Thanks for the clarification.
>>
>> I agree. Just for discussion, two possible approaches come to mind:
>>
>> Option 1 (structurally clean):
>> Introduce an optional per-CPU cpufreq_driver->set_boost_cpu(cpu, enable)
>> hook and call it from __cpufreq_offline() before cpumask_clear_cpu(),
>> but only on the unbind path (not on plain CPU hotplug-offline - clearing
>> it there maybe wrongly re-enable boost on a still-online sibling on
>> shared-scope HW). Roughly (rough sketch only):
>>
>> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
>> index 21639d9ac753..5f298fbb0d8c 100644
>> --- a/drivers/cpufreq/acpi-cpufreq.c
>> +++ b/drivers/cpufreq/acpi-cpufreq.c
>> @@ -525,13 +525,10 @@ static void free_acpi_perf_data(void)
>>           free_percpu(acpi_perf_data);
>>    }
>>
>> -static int cpufreq_boost_down_prep(unsigned int cpu)
>> +static void acpi_cpufreq_set_boost_cpu(unsigned int cpu, bool enable)
>>    {
>> -       /*
>> -        * Clear the boost-disable bit on the CPU_DOWN path so that
>> -        * this cpu cannot block the remaining ones from boosting.
>> -        */
>> -       return boost_set_msr(1);
>> +       smp_call_function_single(cpu, boost_set_msr_each,
>> +                                (void *)(long)enable, 1);
>>    }
>>
>>    /*
>> @@ -951,7 +948,6 @@ static void acpi_cpufreq_cpu_exit(struct
>> cpufreq_policy *policy)
>>
>>           pr_debug("%s\n", __func__);
>>
>> -       cpufreq_boost_down_prep(policy->cpu);
>>           policy->fast_switch_possible = false;
>>           policy->driver_data = NULL;
>>           acpi_processor_unregister_performance(data->acpi_perf_cpu);
>> @@ -986,6 +982,7 @@ static struct cpufreq_driver acpi_cpufreq_driver = {
>>           .bios_limit     = acpi_processor_get_bios_limit,
>>           .init           = acpi_cpufreq_cpu_init,
>>           .exit           = acpi_cpufreq_cpu_exit,
>> +       .set_boost_cpu  = acpi_cpufreq_set_boost_cpu,
>>           .resume         = acpi_cpufreq_resume,
>>           .name           = "acpi-cpufreq",
>>           .attr           = acpi_cpufreq_attr,
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 507224c9ecd3..c4952ca3e8a5 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -1669,13 +1669,18 @@ static int cpufreq_add_dev(struct device *dev,
>> struct subsys_interface *sif)
>>           return 0;
>>    }
>>
>> -static void __cpufreq_offline(unsigned int cpu, struct cpufreq_policy
>> *policy)
>> +static void __cpufreq_offline(unsigned int cpu, struct cpufreq_policy
>> *policy,
>> +                             bool unbinding)
>>    {
>>           int ret;
>>
>>           if (has_target())
>>                   cpufreq_stop_governor(policy);
>>
>> +       if (unbinding && cpufreq_driver->set_boost_cpu &&
>> +           policy->boost_supported)
>> +               cpufreq_driver->set_boost_cpu(cpu, true);
>> +
>>           cpumask_clear_cpu(cpu, policy->cpus);
>>
>>           if (!policy_is_inactive(policy)) {
>> @@ -1730,7 +1735,7 @@ static int cpufreq_offline(unsigned int cpu)
>>
>>           guard(cpufreq_policy_write)(policy);
>>
>> -       __cpufreq_offline(cpu, policy);
>> +       __cpufreq_offline(cpu, policy, false);
>>
>>           return 0;
>>    }
>> @@ -1750,7 +1755,7 @@ static void cpufreq_remove_dev(struct device *dev,
>> struct subsys_interface *sif)
>>
>>           scoped_guard(cpufreq_policy_write, policy) {
>>                   if (cpu_online(cpu))
>> -                       __cpufreq_offline(cpu, policy);
>> +                       __cpufreq_offline(cpu, policy, true);
>>
>>                   remove_cpu_dev_symlink(policy, cpu, dev);
>>
>> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
>> index ae9d1ce4f49c..c174b013dd76 100644
>> --- a/include/linux/cpufreq.h
>> +++ b/include/linux/cpufreq.h
>> @@ -413,6 +413,8 @@ struct cpufreq_driver {
>>           int             (*online)(struct cpufreq_policy *policy);
>>           int             (*offline)(struct cpufreq_policy *policy);
>>           void            (*exit)(struct cpufreq_policy *policy);
>> +       /* Optional */
>> +       void            (*set_boost_cpu)(unsigned int cpu, bool enable);
>>           int             (*suspend)(struct cpufreq_policy *policy);
>>           int             (*resume)(struct cpufreq_policy *policy);
>>
>>
>>
>> Option 2 (lighter, but it is at odds with the direction discussed
>> above): stay in acpi-cpufreq's ->exit() and restore over
>> policy->related_cpus instead.
>>
>>
>>
>>
>> --
>> Thx and BRs,
>> Zhongqiu Han
> 
> While you negotiate the cleanup, should I remove this change from the series?

Thanks a lot Jim for the checking. Maybe Rafael or Viresh can help
comment on this.


Besides, one possible improvement over Option 1 is to leave the
`cpufreq_offline()` API unchanged, and instead move the set boost-bit
callback into `cpufreq_remove_dev()`. To avoid redundant IPIs, place it
after

     if (!cpumask_empty(policy->real_cpus))
         return;

and use `related_cpus` instead of `policy->cpus` (because `policy->cpus`
is emptied after `__cpufreq_offline()` is called).

Pseudo-code:

     scoped_guard(cpufreq_policy_write, policy) {
         if (cpu_online(cpu))
             __cpufreq_offline(cpu, policy);

             remove_cpu_dev_symlink(policy, cpu, dev);

             if (!cpumask_empty(policy->real_cpus))
                 return;

+           if (cpufreq_driver->set_boost_cpu
                 && policy->boost_supported)
+               cpufreq_driver->set_boost_cpu(policy->related_cpus);
+
         /*
          * Unregister cpufreq cooling once all the CPUs of the policy
          * are removed.
          */


-- 
Thx and BRs,
Zhongqiu Han