Re: [PATCH v3 03/15] xen/cpufreq: refactor cmdline "cpufreq=xxx"

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.03.2025 09:39, Penny Zheng wrote:
> This commit includes the following modification:
> - Introduce helper function cpufreq_cmdline_parse_xen and
> cpufreq_cmdline_parse_hwp to tidy the different parsing path
> - Add helper cpufreq_opts_contain to ignore user redundant setting,
> like "cpufreq=hwp;hwp;xen"
> - Doc refinement

See my earlier comment as to wording to avoid. In descriptions and comments
it would also be nice if function names could be followed by () (and array
names then be followed by []) to clearly identify the nature of such
identifiers.

> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -535,7 +535,8 @@ choice of `dom0-kernel` is deprecated and not supported 
> by all Dom0 kernels.
>    processor to autonomously force physical package components into idle 
> state.
>    The default is enabled, but the option only applies when `hwp` is enabled.
>  
> -There is also support for `;`-separated fallback options:
> +User could use `;`-separated options to support universal options which they
> +would like to try on any agnostic platform, *but* under priority order, like
>  `cpufreq=hwp;xen,verbose`.  This first tries `hwp` and falls back to `xen` if
>  unavailable.  Note: The `verbose` suboption is handled globally.  Setting it
>  for either the primary or fallback option applies to both irrespective of 
> where

What does "support" here mean? I fear I can't even suggest what else to use,
as I don't follow what additional information you mean to add here. Is a
change here really needed?

> --- a/xen/drivers/cpufreq/cpufreq.c
> +++ b/xen/drivers/cpufreq/cpufreq.c
> @@ -71,6 +71,46 @@ unsigned int __initdata cpufreq_xen_cnt = 1;
>  
>  static int __init cpufreq_cmdline_parse(const char *s, const char *e);
>  
> +static bool __init cpufreq_opts_contain(enum cpufreq_xen_opt option)
> +{
> +    unsigned int count = cpufreq_xen_cnt;
> +
> +    while ( count )
> +    {
> +        if ( cpufreq_xen_opts[--count] == option )
> +            return true;
> +    }
> +
> +    return false;
> +}
> +
> +static int __init cpufreq_cmdline_parse_xen(const char *arg, const char *end)
> +{
> +    int ret = 0;
> +
> +    xen_processor_pmbits |= XEN_PROCESSOR_PM_PX;
> +    cpufreq_controller = FREQCTL_xen;
> +    cpufreq_xen_opts[cpufreq_xen_cnt++] = CPUFREQ_xen;
> +    ret = 0;

ret was already set to 0 by the initializer.

> +    if ( arg[0] && arg[1] )
> +        ret = cpufreq_cmdline_parse(arg + 1, end);
> +
> +    return ret;
> +}
> +
> +static int __init cpufreq_cmdline_parse_hwp(const char *arg, const char *end)
> +{
> +    int ret = 0;
> +
> +    xen_processor_pmbits |= XEN_PROCESSOR_PM_PX;
> +    cpufreq_controller = FREQCTL_xen;
> +    cpufreq_xen_opts[cpufreq_xen_cnt++] = CPUFREQ_hwp;
> +    if ( arg[0] && arg[1] )
> +        ret = hwp_cmdline_parse(arg + 1, end);
> +
> +    return ret;
> +}

For both of the helpers may I suggest s/parse/process/ or some such
("handle" might be another possible term to use), as themselves they
don't do any parsing?

In the end I'm also not entirely convinced that we need these two almost
identical helpers (with a 3rd likely appearing in a later patch).

> @@ -112,25 +152,13 @@ static int __init cf_check setup_cpufreq_option(const 
> char *str)
>          if ( cpufreq_xen_cnt == ARRAY_SIZE(cpufreq_xen_opts) )
>              return -E2BIG;
>  
> -        if ( choice > 0 || !cmdline_strcmp(str, "xen") )
> -        {
> -            xen_processor_pmbits |= XEN_PROCESSOR_PM_PX;
> -            cpufreq_controller = FREQCTL_xen;
> -            cpufreq_xen_opts[cpufreq_xen_cnt++] = CPUFREQ_xen;
> -            ret = 0;
> -            if ( arg[0] && arg[1] )
> -                ret = cpufreq_cmdline_parse(arg + 1, end);
> -        }
> +        if ( (choice > 0 || !cmdline_strcmp(str, "xen")) &&
> +             !cpufreq_opts_contain(CPUFREQ_xen) )
> +            ret = cpufreq_cmdline_parse_xen(arg, end);
>          else if ( IS_ENABLED(CONFIG_INTEL) && choice < 0 &&
> -                  !cmdline_strcmp(str, "hwp") )
> -        {
> -            xen_processor_pmbits |= XEN_PROCESSOR_PM_PX;
> -            cpufreq_controller = FREQCTL_xen;
> -            cpufreq_xen_opts[cpufreq_xen_cnt++] = CPUFREQ_hwp;
> -            ret = 0;
> -            if ( arg[0] && arg[1] )
> -                ret = hwp_cmdline_parse(arg + 1, end);
> -        }
> +                  !cmdline_strcmp(str, "hwp") &&
> +                  !cpufreq_opts_contain(CPUFREQ_hwp) )
> +            ret = cpufreq_cmdline_parse_hwp(arg, end);
>          else
>              ret = -EINVAL;

Hmm, if I'm not mistaken the example "cpufreq=hwp;hwp;xen" would lead us
to this -EINVAL then. That's not quite "ignore" as the description says.

Jan