Re: [PATCH v4 04/15] xen/cpufreq: refactor cmdline "cpufreq=xxx"

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.04.2025 09:40, Penny Zheng wrote:
> --- a/xen/drivers/cpufreq/cpufreq.c
> +++ b/xen/drivers/cpufreq/cpufreq.c
> @@ -71,6 +71,49 @@ 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 handle_cpufreq_cmdline(enum cpufreq_xen_opt option)
> +{
> +    int ret = 0;
> +
> +    if ( cpufreq_opts_contain(option) )
> +    {
> +        const char *cpufreq_opts_str[] = { "CPUFREQ_xen", "CPUFREQ_hwp" };

        const char *const __initconstrel cpufreq_opts_str[] = { "CPUFREQ_xen", 
"CPUFREQ_hwp" };

(line wrapped suitably, of course) Or maybe even better

        const char __initconst cpufreq_opts_str[][12] = { "CPUFREQ_xen", 
"CPUFREQ_hwp" };

for the string literals to also end up in .init.rodata.

However, the CPUFREQ_ prefixes want dropping, as they're an internal of the
implementation, and ...

> +
> +        printk(XENLOG_WARNING
> +               "Duplicate cpufreq driver option: %s",
> +               cpufreq_opts_str[option - 1]);

... aren't necessarily meaningful when presented this way to the user; the user
specified "xen" or "hwp", after all.

> +        return 0;
> +    }
> +
> +    cpufreq_controller = FREQCTL_xen;
> +    cpufreq_xen_opts[cpufreq_xen_cnt++] = option;
> +    switch ( option )
> +    {
> +    case CPUFREQ_hwp:
> +    case CPUFREQ_xen:
> +        xen_processor_pmbits |= XEN_PROCESSOR_PM_PX;
> +        break;
> +    default:

Blank line please between non-fall-through case blocks.

With all of the adjustments:
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan