[Public]
Hi,
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: Monday, May 12, 2025 11:58 PM
> To: Penny, Zheng <penny.zh...@amd.com>
> Cc: Huang, Ray <ray.hu...@amd.com>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v4 04/15] xen/cpufreq: refactor cmdline "cpufreq=xxx"
>
> On 07.05.2025 05:18, Penny, Zheng wrote:
> > [Public]
> >
> > Hi,
> >
> >> -----Original Message-----
> >> From: Jan Beulich <jbeul...@suse.com>
> >> Sent: Tuesday, April 29, 2025 7:47 PM
> >> To: Penny, Zheng <penny.zh...@amd.com>
> >> Cc: Huang, Ray <ray.hu...@amd.com>; xen-devel@lists.xenproject.org
> >> 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?
>
> In your code cpufreq_opts_str[] is an automatic variable, which the compiler
> needs
> to emit code for in order to instantiate it on the stack. This can be avoided
> if you
> make the array a static variable, as then all construction occurs at build
> time.
>
> >> 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" */
>
> Instead of a comment I has rather hoping for some use of dedicated array slot
> initializers.
Understood. I'll use "CPUFREQ_xxx" as array slot index.
static const char __initconst cpufreq_opts_str[][5] = {
[CPUFREQ_none] = "none",
[CPUFREQ_xen] = "xen",
[CPUFREQ_hwp] = "hwp",
};
>
> Jan
