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

[Zhongqiu Han]

On 7/17/2026 3:11 AM, Jim Mattson wrote:
> On Thu, Jun 25, 2026 at 8:50 AM Zhongqiu Han
> <zhongqiu.han@xxxxxxxxxxxxxxxx> 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