This commit includes the following modification:
- Introduce helper function cpufreq_cmdline_parse_xen and
cpufreq_cmdline_parse_hwp to tidy the different parsing path
- Add helper cpufreq_opts_contain to ignore user redundant setting,
like "cpufreq=hwp;hwp;xen"
- Doc refinement
Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
v2 -> v3:
- new commit
---
docs/misc/xen-command-line.pandoc | 3 +-
xen/drivers/cpufreq/cpufreq.c | 64 ++++++++++++++++++++++---------
2 files changed, 48 insertions(+), 19 deletions(-)
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 0c6225391d..a440042471 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -535,7 +535,8 @@ choice of `dom0-kernel` is deprecated and not supported by all Dom0 kernels.
processor to autonomously force physical package components into idle state.
The default is enabled, but the option only applies when `hwp` is enabled.
-There is also support for `;`-separated fallback options:
+User could use `;`-separated options to support universal options which they
+would like to try on any agnostic platform, *but* under priority order, like
`cpufreq=hwp;xen,verbose`. This first tries `hwp` and falls back to `xen` if
unavailable. Note: The `verbose` suboption is handled globally. Setting it
for either the primary or fallback option applies to both irrespective of where
diff --git a/xen/drivers/cpufreq/cpufreq.c b/xen/drivers/cpufreq/cpufreq.c
index 894bafebaa..cfae16c15f 100644
--- a/xen/drivers/cpufreq/cpufreq.c
+++ b/xen/drivers/cpufreq/cpufreq.c
@@ -71,6 +71,46 @@ 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 cpufreq_cmdline_parse_xen(const char *arg, const char *end)
+{
+ int ret = 0;
+
+ xen_processor_pmbits |= XEN_PROCESSOR_PM_PX;
+ cpufreq_controller = FREQCTL_xen;
+ cpufreq_xen_opts[cpufreq_xen_cnt++] = CPUFREQ_xen;
+ ret = 0;
+ if ( arg[0] && arg[1] )
+ ret = cpufreq_cmdline_parse(arg + 1, end);
+
+ return ret;
+}
+
+static int __init cpufreq_cmdline_parse_hwp(const char *arg, const char *end)
+{
+ int ret = 0;
+
+ xen_processor_pmbits |= XEN_PROCESSOR_PM_PX;
+ cpufreq_controller = FREQCTL_xen;
+ cpufreq_xen_opts[cpufreq_xen_cnt++] = CPUFREQ_hwp;
+ if ( arg[0] && arg[1] )
+ ret = hwp_cmdline_parse(arg + 1, end);
+
+ return ret;
+}
+
static int __init cf_check setup_cpufreq_option(const char *str)
{
const char *arg = strpbrk(str, ",:;");
@@ -112,25 +152,13 @@ static int __init cf_check setup_cpufreq_option(const char *str)
if ( cpufreq_xen_cnt == ARRAY_SIZE(cpufreq_xen_opts) )
return -E2BIG;
- if ( choice > 0 || !cmdline_strcmp(str, "xen") )
- {
- xen_processor_pmbits |= XEN_PROCESSOR_PM_PX;
- cpufreq_controller = FREQCTL_xen;
- cpufreq_xen_opts[cpufreq_xen_cnt++] = CPUFREQ_xen;
- ret = 0;
- if ( arg[0] && arg[1] )
- ret = cpufreq_cmdline_parse(arg + 1, end);
- }
+ if ( (choice > 0 || !cmdline_strcmp(str, "xen")) &&
+ !cpufreq_opts_contain(CPUFREQ_xen) )
+ ret = cpufreq_cmdline_parse_xen(arg, end);
else if ( IS_ENABLED(CONFIG_INTEL) && choice < 0 &&
- !cmdline_strcmp(str, "hwp") )
- {
- xen_processor_pmbits |= XEN_PROCESSOR_PM_PX;
- cpufreq_controller = FREQCTL_xen;
- cpufreq_xen_opts[cpufreq_xen_cnt++] = CPUFREQ_hwp;
- ret = 0;
- if ( arg[0] && arg[1] )
- ret = hwp_cmdline_parse(arg + 1, end);
- }
+ !cmdline_strcmp(str, "hwp") &&
+ !cpufreq_opts_contain(CPUFREQ_hwp) )
+ ret = cpufreq_cmdline_parse_hwp(arg, end);
else
ret = -EINVAL;
--
2.34.1On 06.03.2025 09:39, Penny Zheng wrote:
> This commit includes the following modification:
> - Introduce helper function cpufreq_cmdline_parse_xen and
> cpufreq_cmdline_parse_hwp to tidy the different parsing path
> - Add helper cpufreq_opts_contain to ignore user redundant setting,
> like "cpufreq=hwp;hwp;xen"
> - Doc refinement
See my earlier comment as to wording to avoid. In descriptions and comments
it would also be nice if function names could be followed by () (and array
names then be followed by []) to clearly identify the nature of such
identifiers.
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -535,7 +535,8 @@ choice of `dom0-kernel` is deprecated and not supported by all Dom0 kernels.
> processor to autonomously force physical package components into idle state.
> The default is enabled, but the option only applies when `hwp` is enabled.
>
> -There is also support for `;`-separated fallback options:
> +User could use `;`-separated options to support universal options which they
> +would like to try on any agnostic platform, *but* under priority order, like
> `cpufreq=hwp;xen,verbose`. This first tries `hwp` and falls back to `xen` if
> unavailable. Note: The `verbose` suboption is handled globally. Setting it
> for either the primary or fallback option applies to both irrespective of where
What does "support" here mean? I fear I can't even suggest what else to use,
as I don't follow what additional information you mean to add here. Is a
change here really needed?
> --- a/xen/drivers/cpufreq/cpufreq.c
> +++ b/xen/drivers/cpufreq/cpufreq.c
> @@ -71,6 +71,46 @@ 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 cpufreq_cmdline_parse_xen(const char *arg, const char *end)
> +{
> + int ret = 0;
> +
> + xen_processor_pmbits |= XEN_PROCESSOR_PM_PX;
> + cpufreq_controller = FREQCTL_xen;
> + cpufreq_xen_opts[cpufreq_xen_cnt++] = CPUFREQ_xen;
> + ret = 0;
ret was already set to 0 by the initializer.
> + if ( arg[0] && arg[1] )
> + ret = cpufreq_cmdline_parse(arg + 1, end);
> +
> + return ret;
> +}
> +
> +static int __init cpufreq_cmdline_parse_hwp(const char *arg, const char *end)
> +{
> + int ret = 0;
> +
> + xen_processor_pmbits |= XEN_PROCESSOR_PM_PX;
> + cpufreq_controller = FREQCTL_xen;
> + cpufreq_xen_opts[cpufreq_xen_cnt++] = CPUFREQ_hwp;
> + if ( arg[0] && arg[1] )
> + ret = hwp_cmdline_parse(arg + 1, end);
> +
> + return ret;
> +}
For both of the helpers may I suggest s/parse/process/ or some such
("handle" might be another possible term to use), as themselves they
don't do any parsing?
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).
> @@ -112,25 +152,13 @@ static int __init cf_check setup_cpufreq_option(const char *str)
> if ( cpufreq_xen_cnt == ARRAY_SIZE(cpufreq_xen_opts) )
> return -E2BIG;
>
> - if ( choice > 0 || !cmdline_strcmp(str, "xen") )
> - {
> - xen_processor_pmbits |= XEN_PROCESSOR_PM_PX;
> - cpufreq_controller = FREQCTL_xen;
> - cpufreq_xen_opts[cpufreq_xen_cnt++] = CPUFREQ_xen;
> - ret = 0;
> - if ( arg[0] && arg[1] )
> - ret = cpufreq_cmdline_parse(arg + 1, end);
> - }
> + if ( (choice > 0 || !cmdline_strcmp(str, "xen")) &&
> + !cpufreq_opts_contain(CPUFREQ_xen) )
> + ret = cpufreq_cmdline_parse_xen(arg, end);
> else if ( IS_ENABLED(CONFIG_INTEL) && choice < 0 &&
> - !cmdline_strcmp(str, "hwp") )
> - {
> - xen_processor_pmbits |= XEN_PROCESSOR_PM_PX;
> - cpufreq_controller = FREQCTL_xen;
> - cpufreq_xen_opts[cpufreq_xen_cnt++] = CPUFREQ_hwp;
> - ret = 0;
> - if ( arg[0] && arg[1] )
> - ret = hwp_cmdline_parse(arg + 1, end);
> - }
> + !cmdline_strcmp(str, "hwp") &&
> + !cpufreq_opts_contain(CPUFREQ_hwp) )
> + ret = cpufreq_cmdline_parse_hwp(arg, end);
> else
> ret = -EINVAL;
Hmm, if I'm not mistaken the example "cpufreq=hwp;hwp;xen" would lead us
to this -EINVAL then. That's not quite "ignore" as the description says.
Jan
[Public]
Hi,
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Monday, March 24, 2025 11:01 PM
> To: Penny, Zheng <penny.zheng@amd.com>
> Cc: Huang, Ray <Ray.Huang@amd.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Anthony PERARD <anthony.perard@vates.tech>;
> Orzel, Michal <Michal.Orzel@amd.com>; Julien Grall <julien@xen.org>; Roger Pau
> Monné <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
> devel@lists.xenproject.org
> Subject: Re: [PATCH v3 03/15] xen/cpufreq: refactor cmdline "cpufreq=xxx"
>
> On 06.03.2025 09:39, Penny Zheng wrote:
> > --- a/docs/misc/xen-command-line.pandoc
> > +++ b/docs/misc/xen-command-line.pandoc
> > @@ -535,7 +535,8 @@ choice of `dom0-kernel` is deprecated and not supported
> by all Dom0 kernels.
> > processor to autonomously force physical package components into idle state.
> > The default is enabled, but the option only applies when `hwp` is enabled.
> >
> > -There is also support for `;`-separated fallback options:
> > +User could use `;`-separated options to support universal options
> > +which they would like to try on any agnostic platform, *but* under
> > +priority order, like
> > `cpufreq=hwp;xen,verbose`. This first tries `hwp` and falls back to
> > `xen` if unavailable. Note: The `verbose` suboption is handled
> > globally. Setting it for either the primary or fallback option
> > applies to both irrespective of where
>
> What does "support" here mean? I fear I can't even suggest what else to use, as I
> don't follow what additional information you mean to add here. Is a change here
> really needed?
>
There are two changes I'd like to address:
1) ";" is not designed for fallback options anymore, like we discussed before, we would
like to support something like "cpufreq=hwp;amd-cppc;xen" for users to define all universal options
they would like to try.
2) Must under *priority* order. As in cpufreq_driver_init(), we are using loop to decide which driver to
try firstly. If user defines "cpufreq=xen;amd-cppc", which leads legacy P-state set before amd-cppc in cpufreq_xen_opts[],
then in the loop, we will try to register legacy P-state firstly, once it gets registered successfully, we will not try to register amd-cppc at all.
> > --- a/xen/drivers/cpufreq/cpufreq.c
> > +++ b/xen/drivers/cpufreq/cpufreq.c
> > + if ( arg[0] && arg[1] )
> > + ret = cpufreq_cmdline_parse(arg + 1, end);
> > +
> > + return ret;
> > +}
> > +
> > +static int __init cpufreq_cmdline_parse_hwp(const char *arg, const
> > +char *end) {
> > + int ret = 0;
> > +
> > + xen_processor_pmbits |= XEN_PROCESSOR_PM_PX;
> > + cpufreq_controller = FREQCTL_xen;
> > + cpufreq_xen_opts[cpufreq_xen_cnt++] = CPUFREQ_hwp;
> > + if ( arg[0] && arg[1] )
> > + ret = hwp_cmdline_parse(arg + 1, end);
> > +
> > + return ret;
> > +}
>
> For both of the helpers may I suggest s/parse/process/ or some such ("handle"
> might be another possible term to use), as themselves they don't do any parsing?
>
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
```
> 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).
>
> Jan
On 26.03.2025 08:20, Penny, Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Monday, March 24, 2025 11:01 PM
>>
>> On 06.03.2025 09:39, Penny Zheng wrote:
>>> --- a/docs/misc/xen-command-line.pandoc
>>> +++ b/docs/misc/xen-command-line.pandoc
>>> @@ -535,7 +535,8 @@ choice of `dom0-kernel` is deprecated and not supported
>> by all Dom0 kernels.
>>> processor to autonomously force physical package components into idle state.
>>> The default is enabled, but the option only applies when `hwp` is enabled.
>>>
>>> -There is also support for `;`-separated fallback options:
>>> +User could use `;`-separated options to support universal options
>>> +which they would like to try on any agnostic platform, *but* under
>>> +priority order, like
>>> `cpufreq=hwp;xen,verbose`. This first tries `hwp` and falls back to
>>> `xen` if unavailable. Note: The `verbose` suboption is handled
>>> globally. Setting it for either the primary or fallback option
>>> applies to both irrespective of where
>>
>> What does "support" here mean? I fear I can't even suggest what else to use, as I
>> don't follow what additional information you mean to add here. Is a change here
>> really needed?
>
> There are two changes I'd like to address:
> 1) ";" is not designed for fallback options anymore, like we discussed before, we would
> like to support something like "cpufreq=hwp;amd-cppc;xen" for users to define all universal options
> they would like to try.
Why would the meaning of ; change? There's no difference between having a single
fallback option from hwp, or two of them from amd-cppc.
> 2) Must under *priority* order. As in cpufreq_driver_init(), we are using loop to decide which driver to
> try firstly. If user defines "cpufreq=xen;amd-cppc", which leads legacy P-state set before amd-cppc in cpufreq_xen_opts[],
> then in the loop, we will try to register legacy P-state firstly, once it gets registered successfully, we will not try to register amd-cppc at all.
This in-order aspect also doesn't change.
Overall I fear I don't feel my question was answered.
>>> --- a/xen/drivers/cpufreq/cpufreq.c
>>> +++ b/xen/drivers/cpufreq/cpufreq.c
>>> + if ( arg[0] && arg[1] )
>>> + ret = cpufreq_cmdline_parse(arg + 1, end);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int __init cpufreq_cmdline_parse_hwp(const char *arg, const
>>> +char *end) {
>>> + int ret = 0;
>>> +
>>> + xen_processor_pmbits |= XEN_PROCESSOR_PM_PX;
>>> + cpufreq_controller = FREQCTL_xen;
>>> + cpufreq_xen_opts[cpufreq_xen_cnt++] = CPUFREQ_hwp;
>>> + if ( arg[0] && arg[1] )
>>> + ret = hwp_cmdline_parse(arg + 1, end);
>>> +
>>> + return ret;
>>> +}
>>
>> For both of the helpers may I suggest s/parse/process/ or some such ("handle"
>> might be another possible term to use), as themselves they don't do any parsing?
>>
>
> 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.
Jan
[Public]
Hi
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Wednesday, March 26, 2025 6:43 PM
> To: Penny, Zheng <penny.zheng@amd.com>
> Cc: Huang, Ray <Ray.Huang@amd.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Anthony PERARD <anthony.perard@vates.tech>;
> Orzel, Michal <Michal.Orzel@amd.com>; Julien Grall <julien@xen.org>; Roger
> Pau Monné <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@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 <jbeulich@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;
+ }
+
+ return ret;
+}
```
> Jan
On 01.04.2025 07:44, Penny, Zheng wrote:
> [Public]
>
> Hi
>
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Wednesday, March 26, 2025 6:43 PM
>> To: Penny, Zheng <penny.zheng@amd.com>
>> Cc: Huang, Ray <Ray.Huang@amd.com>; Andrew Cooper
>> <andrew.cooper3@citrix.com>; Anthony PERARD <anthony.perard@vates.tech>;
>> Orzel, Michal <Michal.Orzel@amd.com>; Julien Grall <julien@xen.org>; Roger
>> Pau Monné <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@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 <jbeulich@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
[Public]
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Tuesday, April 1, 2025 2:38 PM
> To: Penny, Zheng <penny.zheng@amd.com>
> Cc: Huang, Ray <Ray.Huang@amd.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Anthony PERARD <anthony.perard@vates.tech>;
> Orzel, Michal <Michal.Orzel@amd.com>; Julien Grall <julien@xen.org>; Roger
> Pau Monné <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>;
> xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v3 03/15] xen/cpufreq: refactor cmdline "cpufreq=xxx"
>
> On 01.04.2025 07:44, Penny, Zheng wrote:
> > [Public]
> >
> > Hi
> >
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: Wednesday, March 26, 2025 6:43 PM
> >> To: Penny, Zheng <penny.zheng@amd.com>
> >> Cc: Huang, Ray <Ray.Huang@amd.com>; Andrew Cooper
> >> <andrew.cooper3@citrix.com>; Anthony PERARD
> >> <anthony.perard@vates.tech>; Orzel, Michal <Michal.Orzel@amd.com>;
> >> Julien Grall <julien@xen.org>; Roger Pau Monné
> >> <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@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 <jbeulich@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.
>
Understood!
> Jan
© 2016 - 2026 Red Hat, Inc.