Re: [PATCH v3 03/15] xen/cpufreq: refactor cmdline "cpufreq=xxx"

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.04.2025 07:44, Penny, Zheng wrote:
> [Public]
> 
> Hi
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Wednesday, March 26, 2025 6:43 PM
>> To: Penny, Zheng <penny.zheng@xxxxxxx>
>> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Andrew Cooper
>> <andrew.cooper3@xxxxxxxxxx>; Anthony PERARD <anthony.perard@xxxxxxxxxx>;
>> Orzel, Michal <Michal.Orzel@xxxxxxx>; Julien Grall <julien@xxxxxxx>; Roger
>> Pau Monné <roger.pau@xxxxxxxxxx>; Stefano Stabellini 
>> <sstabellini@xxxxxxxxxx>;
>> xen-devel@xxxxxxxxxxxxxxxxxxxx
>> Subject: Re: [PATCH v3 03/15] xen/cpufreq: refactor cmdline "cpufreq=xxx"
>>
>> On 26.03.2025 08:20, Penny, Zheng wrote:
>>>> -----Original Message-----
>>>> From: Jan Beulich <jbeulich@xxxxxxxx>
>>>> Sent: Monday, March 24, 2025 11:01 PM
>>>>
>>>> On 06.03.2025 09:39, Penny Zheng wrote:
>>> Maybe I mis-understood the previous comment you said ```
>>>         >          else if ( IS_ENABLED(CONFIG_INTEL) && choice < 0 &&
>>>         > ```
>>>
>>>         For the rest of this, I guess I'd prefer to see this in context. 
>>> Also with
>>>         regard to the helper function's name.
>>> ```
>>> I thought you suggested to introduce helper function to wrap the conditional
>> codes...
>>> Or may you were suggesting something like:
>>> ```
>>> #ifdef CONFIG_INTEL
>>> else if ( choice < 0 && !cmdline_strcmp(str, "hwp") ) {
>>>     xen_processor_pmbits |= XEN_PROCES
>>>     ...
>>> }
>>> #endif
>>> ```
>>
>> Was this reply of yours misplaced? It doesn't fit with the part of my reply 
>> in context
>> above. Or maybe I'm not understanding what you mean to say.
>>
>>>> In the end I'm also not entirely convinced that we need these two
>>>> almost identical helpers (with a 3rd likely appearing in a later patch).
>>
>> Instead it feels as if this response of yours was to this part of my comment.
>> Indeed iirc I was suggesting to introduce a helper function. Note, however, 
>> the
>> singular here as well as in your response above.
>>
> 
> Correct if I understood wrongly, you are suggesting that we shall use one 
> single helper
> function here to cover all scenarios, maybe as follows:
> ```
> +static int __init handle_cpufreq_cmdline(const char *arg, const char *end,
> +                                         enum cpufreq_xen_opt option)
> +{
> +    int ret;
> +
> +    if ( cpufreq_opts_contain(option) )
> +    {
> +        const char *cpufreq_opts_str[] = { "CPUFREQ_xen", "CPUFREQ_hwp" };
> +
> +        printk(XENLOG_WARNING
> +               "Duplicate cpufreq driver option: %s",
> +               cpufreq_opts_str[option - 1]);
> +        return 0;
> +    }
> +
> +    xen_processor_pmbits |= XEN_PROCESSOR_PM_PX;
> +    cpufreq_controller = FREQCTL_xen;
> +    cpufreq_xen_opts[cpufreq_xen_cnt++] = option;
> +    switch ( option )
> +    {
> +    case CPUFREQ_hwp:
> +        if ( arg[0] && arg[1] )
> +            ret = hwp_cmdline_parse(arg + 1, end);
> +    case CPUFREQ_xen:
> +        if ( arg[0] && arg[1] )
> +            ret = cpufreq_cmdline_parse(arg + 1, end);
> +    default:
> +        ret = -EINVAL;
> +    }

Apart from the switch() missing all break statements, the helper I was thinking
of would end right before the switch(). The <xyz>_cmdline_parse() calls would
remain at the call sites of the helper.

Jan