A helper function handle_cpufreq_cmdline() is introduced to tidy different
handling pathes.
We also add a new helper cpufreq_opts_contain() to ignore redundant setting,
like "cpufreq=hwp;hwp;xen"
Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
v2 -> v3:
- new commit
---
v3 -> v4:
- add one single helper to do the tidy work
- ignore and warn user redundant setting
---
v4 -> v5:
- make "cpufreq_opts_str" static and the string literals end up in
.init.rodata.
- use "CPUFREQ_xxx" as array slot index
- blank line between non-fall-through case blocks
---
v5 -> v6:
- change to "while ( count-- )"
- remove unnecessary warning
- add an assertion to ensure not overruning the array
- add ASSERT_UNREACHABLE()
- check ret of handle_cpufreq_cmdline() and error out
---
xen/drivers/cpufreq/cpufreq.c | 59 ++++++++++++++++++++++++------
xen/include/acpi/cpufreq/cpufreq.h | 3 +-
2 files changed, 49 insertions(+), 13 deletions(-)
diff --git a/xen/drivers/cpufreq/cpufreq.c b/xen/drivers/cpufreq/cpufreq.c
index 564f926341..887bc5953d 100644
--- 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;
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);
+ cpufreq_xen_opts[cpufreq_xen_cnt++] = option;
+ switch ( option )
+ {
+ case CPUFREQ_hwp:
+ case CPUFREQ_xen:
+ xen_processor_pmbits |= XEN_PROCESSOR_PM_PX;
+ break;
+
+ default:
+ ASSERT_UNREACHABLE();
+ ret = -EINVAL;
+ break;
+ }
+
+ return ret;
+}
+
static int __init cf_check setup_cpufreq_option(const char *str)
{
const char *arg = strpbrk(str, ",:;");
@@ -113,21 +154,15 @@ static int __init cf_check setup_cpufreq_option(const char *str)
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 = handle_cpufreq_cmdline(CPUFREQ_xen);
+ if ( !ret && arg[0] && arg[1] )
ret = cpufreq_cmdline_parse(arg + 1, 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 = handle_cpufreq_cmdline(CPUFREQ_hwp);
+ if ( !ret && arg[0] && arg[1] )
ret = hwp_cmdline_parse(arg + 1, end);
}
else
diff --git a/xen/include/acpi/cpufreq/cpufreq.h b/xen/include/acpi/cpufreq/cpufreq.h
index 0742aa9f44..948530218a 100644
--- a/xen/include/acpi/cpufreq/cpufreq.h
+++ b/xen/include/acpi/cpufreq/cpufreq.h
@@ -27,7 +27,8 @@ enum cpufreq_xen_opt {
CPUFREQ_xen,
CPUFREQ_hwp,
};
-extern enum cpufreq_xen_opt cpufreq_xen_opts[2];
+#define NR_CPUFREQ_OPTS 2
+extern enum cpufreq_xen_opt cpufreq_xen_opts[NR_CPUFREQ_OPTS];
extern unsigned int cpufreq_xen_cnt;
struct cpufreq_governor;
--
2.34.1On 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 ...
> + cpufreq_xen_opts[cpufreq_xen_cnt++] = option;
... you then still overrun this array if something's wrong in this regard.
Jan
[Public]
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Wednesday, July 16, 2025 11:01 PM
> To: Penny, Zheng <penny.zheng@amd.com>
> Cc: Huang, Ray <Ray.Huang@amd.com>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v6 05/19] xen/cpufreq: refactor cmdline "cpufreq=xxx"
>
> 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)?
>
The cpufreq_xen_cnt initialized as 1 is to have default CPUFREQ_xen value when there is no "cpufreq=xxx" cmdline option
I suppose you are pointing out that the macro NR_CPUFREQ_OPTS is pointless, as we could use ARRAY_SIZE().
> > 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 ...
>
Understood, will use ARRAY_SIZE(), and will use if() to error out
> > + cpufreq_xen_opts[cpufreq_xen_cnt++] = option;
>
> ... you then still overrun this array if something's wrong in this regard.
>
> Jan
On 04.08.2025 07:47, Penny, Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> 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)?
>>
>
> The cpufreq_xen_cnt initialized as 1 is to have default CPUFREQ_xen value when there is no "cpufreq=xxx" cmdline option
> I suppose you are pointing out that the macro NR_CPUFREQ_OPTS is pointless, as we could use ARRAY_SIZE().
That I'm suggesting further down, yes. But here I'm questioning the array
initializer: As said, I think only slot 0 needs explicit initializing. Or
else the initializer would need touching again when the array size is
grown. Which would be nice to avoid, providing doing so is correct.
Jan
[Public]
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Wednesday, July 16, 2025 11:01 PM
> To: Penny, Zheng <penny.zheng@amd.com>
> Cc: Huang, Ray <Ray.Huang@amd.com>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v6 05/19] xen/cpufreq: refactor cmdline "cpufreq=xxx"
>
> 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
> > + cpufreq_xen_opts[cpufreq_xen_cnt++] = option;
>
> ... you then still overrun this array if something's wrong in this regard.
>
> Jan
On 04.08.2025 08:04, Penny, Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> 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
© 2016 - 2025 Red Hat, Inc.