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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.08.2025 08:04, Penny, Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Wednesday, July 16, 2025 11:01 PM
>>
>> 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)?
>>
>>>  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 ...
> 
> I found that we already have array index check in setup_cpufreq_option(), 
> before calling handle_cpufreq_cmdline()
> Then maybe there is no need to do it again here

Well, you will still need to deal with the release build aspect, as per your
earlier reply. At which point you can easily place an ASSERT_UNREACHABLE()
there as well.

Jan