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

["Penny, Zheng" <penny.zheng@xxxxxxx>]

[Public]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Wednesday, July 16, 2025 11:01 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v6 05/19] xen/cpufreq: refactor cmdline "cpufreq=xxx"
>
> 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)?
>

The cpufreq_xen_cnt initialized as 1 is to have default  CPUFREQ_xen value when 
there is no "cpufreq=xxx" cmdline option
I suppose you are pointing out that the macro NR_CPUFREQ_OPTS is pointless, as 
we could use ARRAY_SIZE().

> >  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 ...
>

Understood, will use ARRAY_SIZE(), and will use if() to error out

> > +    cpufreq_xen_opts[cpufreq_xen_cnt++] = option;
>
> ... you then still overrun this array if something's wrong in this regard.
>
> Jan