Re: [PATCH v6 05/19] xen/cpufreq: refactor cmdline "cpufreq=xxx"

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.07.2025 05:50, Penny Zheng wrote:
> --- a/xen/drivers/cpufreq/cpufreq.c
> +++ b/xen/drivers/cpufreq/cpufreq.c
> @@ -64,12 +64,53 @@ LIST_HEAD_READ_MOSTLY(cpufreq_governor_list);
>  /* set xen as default cpufreq */
>  enum cpufreq_controller cpufreq_controller = FREQCTL_xen;
>  
> -enum cpufreq_xen_opt __initdata cpufreq_xen_opts[2] = { CPUFREQ_xen,
> -                                                        CPUFREQ_none };
> +enum cpufreq_xen_opt __initdata cpufreq_xen_opts[NR_CPUFREQ_OPTS] = {
> +    CPUFREQ_xen,
> +    CPUFREQ_none
> +};
>  unsigned int __initdata cpufreq_xen_cnt = 1;

Given this, isn't the array index 1 initializer quite pointless above? Or
else, if you really mean to explicitly fill all slots with CPUFREQ_none
(despite that deliberately having numeric value 0), why not
"[1 ... NR_CPUFREQ_OPTS - 1] = CPUFREQ_none" (or using ARRAY_SIZE(), as
per below)?

>  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) )
> +        return 0;
> +
> +    cpufreq_controller = FREQCTL_xen;
> +    ASSERT(cpufreq_xen_cnt < NR_CPUFREQ_OPTS);

This would better use ARRAY_SIZE(), at which point NR_CPUFREQ_OPTS can go
away again. What's worse, though, is that on release builds ...

> +    cpufreq_xen_opts[cpufreq_xen_cnt++] = option;

... you then still overrun this array if something's wrong in this regard.

Jan