Re: [PATCH 01/13] cpufreq: Allow restricting to internal governors only

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.05.2021 21:27, Jason Andryuk wrote:
> For hwp, the standard governors are not usable, and only the internal
> one is applicable.

So you say "one" here but use plural in the subject. Which one is
it (going to be)?

>  Add the cpufreq_governor_internal boolean to
> indicate when an internal governor, like hwp-internal, will be used.
> This is set during presmp_initcall, so that it can suppress governor

DYM s/is/will be/? Afaict this is going to happen later in the series.
Which is a good indication that such "hanging in the air" changes
aren't necessarily the best way of splitting a set of changes, ...

> --- a/xen/drivers/cpufreq/cpufreq.c
> +++ b/xen/drivers/cpufreq/cpufreq.c
> @@ -57,6 +57,7 @@ struct cpufreq_dom {
>  };
>  static LIST_HEAD_READ_MOSTLY(cpufreq_dom_list_head);
>  
> +bool __read_mostly cpufreq_governor_internal;

... also supported by you introducing a non-static variable without
any consumer outside of this file (and without any producer at all).

> @@ -122,6 +123,9 @@ int __init cpufreq_register_governor(struct 
> cpufreq_governor *governor)
>      if (!governor)
>          return -EINVAL;
>  
> +    if (cpufreq_governor_internal && strstr(governor->name, "internal") == 
> NULL)

I wonder whether designating any governors ending in "-internal"
wouldn't be less prone for possible future ambiguities.

> --- a/xen/include/acpi/cpufreq/cpufreq.h
> +++ b/xen/include/acpi/cpufreq/cpufreq.h
> @@ -115,6 +115,7 @@ extern struct cpufreq_governor cpufreq_gov_dbs;
>  extern struct cpufreq_governor cpufreq_gov_userspace;
>  extern struct cpufreq_governor cpufreq_gov_performance;
>  extern struct cpufreq_governor cpufreq_gov_powersave;
> +extern bool cpufreq_governor_internal;

Please separate from the governor declarations by a blank line.

Sorry, all quite nit-like remarks, but still ...

Jan