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

[Jim Mattson]

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?