Re: [PATCH 01/22] cpufreq/acpi-cpufreq: Convert to hotplug state machine

[Sebastian Andrzej Siewior]

On 2016-11-28 10:45:20 [+0530], Viresh Kumar wrote:
> > diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> > index 297e9128fe9f..2c29cbaca7b5 100644
> > --- a/drivers/cpufreq/acpi-cpufreq.c
> > +++ b/drivers/cpufreq/acpi-cpufreq.c
> > @@ -922,37 +909,47 @@ static struct cpufreq_driver acpi_cpufreq_driver = {
> >  	.attr		= acpi_cpufreq_attr,
> >  };
> >  
> > +static enum cpuhp_state acpi_cpufreq_online;
> > +
> >  static void __init acpi_cpufreq_boost_init(void)
> >  {
> > -	if (boot_cpu_has(X86_FEATURE_CPB) || boot_cpu_has(X86_FEATURE_IDA)) {
> > -		msrs = msrs_alloc();
> > +	int ret;
> >  
> > -		if (!msrs)
> > -			return;
> > +	if (!(boot_cpu_has(X86_FEATURE_CPB) || boot_cpu_has(X86_FEATURE_IDA)))
> > +		return;
> >  
> > -		acpi_cpufreq_driver.set_boost = set_boost;
> > -		acpi_cpufreq_driver.boost_enabled = boost_state(0);
> > +	msrs = msrs_alloc();
> >  
> > -		cpu_notifier_register_begin();
> > +	if (!msrs)
> > +		return;
> >  
> > -		/* Force all MSRs to the same value */
> > -		boost_set_msrs(acpi_cpufreq_driver.boost_enabled,
> > -			       cpu_online_mask);
> 
> Why isn't this required anymore ?

Later there is cpuhp_setup_state() which invokes the online callback
(cpufreq_boost_online) which is doing this.

> > +	acpi_cpufreq_driver.set_boost = set_boost;
> > +	acpi_cpufreq_driver.boost_enabled = boost_state(0);
> >  
> > -		__register_cpu_notifier(&boost_nb);
> > -
> > -		cpu_notifier_register_done();
> > +	/*
> > +	 * This calls the online callback on all online cpu and forces all
> > +	 * MSRs to the same value.
> > +	 */
> > +	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "cpufreq/acpi:online",
> > +				cpufreq_boost_online, cpufreq_boost_down_prep);
> > +	if (ret < 0) {
> > +		pr_err("acpi_cpufreq: failed to register hotplug callbacks\n");
> > +		msrs_free(msrs);
> 
> If 'msrs = NULL' is required to be done in the _exit() routine, then it should
> be done here as well. Right?

It is not required in the exit path, it is just needed here. It is gone
in the following patch anyway but I will post a fixed version.

> > +		return;
> >  	}
> > +	acpi_cpufreq_online = ret;
> >  }
> >  
> >  static void acpi_cpufreq_boost_exit(void)
> >  {
> > -	if (msrs) {
> > -		unregister_cpu_notifier(&boost_nb);
> > +	if (!msrs)
> > +		return;
> >  
> > -		msrs_free(msrs);
> > -		msrs = NULL;
> > -	}
> > +	if (acpi_cpufreq_online >= 0)
> > +		cpuhp_remove_state_nocalls(acpi_cpufreq_online);
> > +
> > +	msrs_free(msrs);
> > +	msrs = NULL;
> >  }
> >  
> >  static int __init acpi_cpufreq_init(void)
> > -- 
> > 2.10.2

Sebastian