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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.05.2025 08:27, Penny, Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Monday, May 12, 2025 11:58 PM
>> On 07.05.2025 05:18, Penny, Zheng wrote:
>>>> -----Original Message-----
>>>> From: Jan Beulich <jbeulich@xxxxxxxx>
>>>> Sent: Tuesday, April 29, 2025 7:47 PM
>>>> 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",
>         };

Provided the "none" entry doesn't cause any anomalies, this way.

Jan