Re: [PATCH 2/2] cpufreq: Specify default governor on command line

[Quentin Perret <qperret@google.com>]

Hey Viresh,

On Tuesday 16 Jun 2020 at 10:01:43 (+0530), Viresh Kumar wrote:
> On 15-06-20, 17:55, Quentin Perret wrote:
> > +static void cpufreq_get_default_governor(void)
> > +{
> > +	default_governor = cpufreq_parse_governor(cpufreq_param_governor);
> > +	if (!default_governor) {
> > +		if (*cpufreq_param_governor)
> > +			pr_warn("Failed to find %s\n", cpufreq_param_governor);
> > +		default_governor = cpufreq_default_governor();
> 
> A module_get() never happened for this case and so maybe a
> module_put() should never get called.

Correct, however cpufreq_default_governor() being a weak function, we're
basically guaranteed the governor we get from there is builtin, so
gov->owner is NULL. That is, module_put() is not actively useful, but it
doesn't harm. So I figured that should be fine. That could definitely
use a comment, though :)

> > +	}
> > +}
> > +
> > +static void cpufreq_put_default_governor(void)
> > +{
> > +	if (!default_governor)
> > +		return;
> > +	module_put(default_governor->owner);
> > +	default_governor = NULL;
> > +}
> > +
> >  static int cpufreq_init_governor(struct cpufreq_policy *policy)
> >  {
> >  	int ret;
> > @@ -2701,6 +2721,8 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
> >  
> >  	if (driver_data->setpolicy)
> >  		driver_data->flags |= CPUFREQ_CONST_LOOPS;
> > +	else
> > +		cpufreq_get_default_governor();
> >  
> >  	if (cpufreq_boost_supported()) {
> >  		ret = create_boost_sysfs_file();
> > @@ -2769,6 +2791,7 @@ int cpufreq_unregister_driver(struct cpufreq_driver *driver)
> >  	subsys_interface_unregister(&cpufreq_interface);
> >  	remove_boost_sysfs_file();
> >  	cpuhp_remove_state_nocalls_cpuslocked(hp_online);
> > +	cpufreq_put_default_governor();
> >  
> >  	write_lock_irqsave(&cpufreq_driver_lock, flags);
> >  
> > @@ -2792,4 +2815,5 @@ static int __init cpufreq_core_init(void)
> >  	return 0;
> >  }
> 
> And since this is a per boot thing, there is perhaps no need of doing
> these at driver register/unregister, I would rather do it at:
> cpufreq_core_init() time itself and so we will never need to run
> cpufreq_put_default_governor() and so can be removed.

Right, so the reason I avoided cpufreq_core_init() was because it is
called at core_initcall() time, which means I can't really assume the
governors have been loaded by that time. By waiting for the driver to
probe before detecting the default gov, we get that nice ordering. But
yes, it feels odd to have it here :/

Thinking about it more, the natural fit for this would rather be the
register/unregister path for governors directly. If that sounds good to
you (?) I'll try to move it there in v2.

> And another thing I am not able to understand (despite you commenting
> about that in the commit log) is what happens if the default governor
> chosen is built as a module ?

So the answer is 'it depends'. If the driver is built as a module too,
then you should load the governor module first, and then the driver
module, and everything will work just fine.

But in the case where the governor is loaded _after_ the driver (either
because we got the module ordering wrong, or because the driver is
builtin), then the policies will be initialized with the builtin
default, and nothing special will happen when the governor module is
loaded.

That behaviour very much is open for discussion, though. A possible
alternative would be to automatically switch all policies to the default
governor upon loading. That would have the nice benefit or removing the
ordering dependency, but that is more involved and I didn't have a
use-case for it, so I went for the simpler option ('the-default
governor-needs-to-be-registered-before-the-policies-are-created').

Thoughts?

Thanks,
Quentin