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

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

[Public]

Hi,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Tuesday, April 29, 2025 7:47 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v4 04/15] xen/cpufreq: refactor cmdline "cpufreq=xxx"
>
> On 29.04.2025 12:36, Jan Beulich wrote:
> > 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.
>
> Actually, it didn't even occur to me that there might be a "static" missing 
> here, too.

Sorry, I may need more explanation, what is the "static" missing you are 
referring?

> Plus I'm missing any arrangement for the array slots to remain in sync with 
> the
> corresponding enumerators. With that ...
>

Thanks for the detailed instructions, learned and I'll change it to
        const char __initconst cpufreq_opts_str[][4] = { "xen", "hwp" };
And for in sync with the corresponding enumerators, maybe we shall add comment 
here,
        /* The order of cpufreq string literal must be in sync with  the order 
in "enum cpufreq_xen_opt" */

> > With all of the adjustments:
> > Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> I'm sorry, but I need to take this back. There are just too many issues.
>
> Jan