[Public]
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: Wednesday, July 16, 2025 11:01 PM
> To: Penny, Zheng <penny.zh...@amd.com>
> Cc: Huang, Ray <ray.hu...@amd.com>; xen-devel@lists.xenproject.org
> 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
