On 01.04.2025 07:44, Penny, Zheng wrote:
> [Public]
>
> Hi
>
>> -----Original Message-----
>> From: Jan Beulich <jbeul...@suse.com>
>> Sent: Wednesday, March 26, 2025 6:43 PM
>> To: Penny, Zheng <penny.zh...@amd.com>
>> Cc: Huang, Ray <ray.hu...@amd.com>; Andrew Cooper
>> <andrew.coop...@citrix.com>; Anthony PERARD <anthony.per...@vates.tech>;
>> Orzel, Michal <michal.or...@amd.com>; Julien Grall <jul...@xen.org>; Roger
>> Pau Monné <roger....@citrix.com>; Stefano Stabellini
>> <sstabell...@kernel.org>;
>> xen-devel@lists.xenproject.org
>> 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 <jbeul...@suse.com>
>>>> 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
