A helper function handle_cpufreq_cmdline() is introduced to tidy different
handling pathes.
We also add a new helper cpufreq_opts_contain() to ignore and warn user
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
---
 xen/drivers/cpufreq/cpufreq.c | 57 ++++++++++++++++++++++++++++++-----
 1 file changed, 49 insertions(+), 8 deletions(-)

diff --git a/xen/drivers/cpufreq/cpufreq.c b/xen/drivers/cpufreq/cpufreq.c
index d6b6c844d8..d1b51c8dd0 100644
--- a/xen/drivers/cpufreq/cpufreq.c
+++ b/xen/drivers/cpufreq/cpufreq.c
@@ -69,8 +69,55 @@ enum cpufreq_xen_opt __initdata cpufreq_xen_opts[2] = { CPUFREQ_xen,
                                                         CPUFREQ_none };
 unsigned int __initdata cpufreq_xen_cnt = 1;
 
+static const char __initconst cpufreq_opts_str[][5] = {
+    [CPUFREQ_none] = "none",
+    [CPUFREQ_xen] = "xen",
+    [CPUFREQ_hwp] = "hwp",
+};
+
 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) )
+    {
+        printk(XENLOG_WARNING "Duplicate cpufreq driver option: %s\n",
+               cpufreq_opts_str[option]);
+        return 0;
+    }
+
+    cpufreq_controller = FREQCTL_xen;
+    cpufreq_xen_opts[cpufreq_xen_cnt++] = option;
+    switch ( option )
+    {
+    case CPUFREQ_hwp:
+    case CPUFREQ_xen:
+        xen_processor_pmbits |= XEN_PROCESSOR_PM_PX;
+        break;
+
+    default:
+        ret = -EINVAL;
+        break;
+    }
+
+    return ret;
+}
+
 static int __init cf_check setup_cpufreq_option(const char *str)
 {
     const char *arg = strpbrk(str, ",:;");
@@ -114,20 +161,14 @@ 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;
+            ret = handle_cpufreq_cmdline(CPUFREQ_xen);
             if ( 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;
+            ret = handle_cpufreq_cmdline(CPUFREQ_hwp);
             if ( arg[0] && arg[1] )
                 ret = hwp_cmdline_parse(arg + 1, end);
         }
-- 
2.34.1

On 27.05.2025 10:48, Penny Zheng wrote:
> --- a/xen/drivers/cpufreq/cpufreq.c
> +++ b/xen/drivers/cpufreq/cpufreq.c
> @@ -69,8 +69,55 @@ enum cpufreq_xen_opt __initdata cpufreq_xen_opts[2] = { CPUFREQ_xen,
>                                                          CPUFREQ_none };
>  unsigned int __initdata cpufreq_xen_cnt = 1;
>  
> +static const char __initconst cpufreq_opts_str[][5] = {
> +    [CPUFREQ_none] = "none",
> +    [CPUFREQ_xen] = "xen",
> +    [CPUFREQ_hwp] = "hwp",
> +};
> +
>  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 )

Instead of this, "while ( count-- )" would likely be slightly better;
less risk of an edit to the loop body (however unlikely that may seem
right now) then bypassing the decrement.

> +            return true;
> +    }
> +
> +    return false;
> +}
> +
> +static int __init handle_cpufreq_cmdline(enum cpufreq_xen_opt option)
> +{
> +    int ret = 0;
> +
> +    if ( cpufreq_opts_contain(option) )
> +    {
> +        printk(XENLOG_WARNING "Duplicate cpufreq driver option: %s\n",
> +               cpufreq_opts_str[option]);

Do we really need such a warning?

> +        return 0;
> +    }
> +
> +    cpufreq_controller = FREQCTL_xen;
> +    cpufreq_xen_opts[cpufreq_xen_cnt++] = option;

This could do with (at least) an assertion that we indeed don't overrun the
array.

Jan

Hi Penny,

On 2025-05-27 04:48, Penny Zheng wrote:
> A helper function handle_cpufreq_cmdline() is introduced to tidy different
> handling pathes.
> We also add a new helper cpufreq_opts_contain() to ignore and warn user
> redundant setting, like "cpufreq=hwp;hwp;xen"
> 
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>

> @@ -114,20 +161,14 @@ 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;
> +            ret = handle_cpufreq_cmdline(CPUFREQ_xen);

Do we need to check ret and error out?  (and below)

Thanks,
Jason

>               if ( 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;
> +            ret = handle_cpufreq_cmdline(CPUFREQ_hwp);
>               if ( arg[0] && arg[1] )
>                   ret = hwp_cmdline_parse(arg + 1, end);
>           }

[Public]

> -----Original Message-----
> From: Jason Andryuk <jason.andryuk@amd.com>
> Sent: Wednesday, June 11, 2025 4:03 PM
> To: Penny, Zheng <penny.zheng@amd.com>; xen-devel@lists.xenproject.org
> Cc: Huang, Ray <Ray.Huang@amd.com>; Jan Beulich <jbeulich@suse.com>
> Subject: Re: [PATCH v5 05/18] xen/cpufreq: refactor cmdline "cpufreq=xxx"
>
> Hi Penny,
>
> On 2025-05-27 04:48, Penny Zheng wrote:
> > A helper function handle_cpufreq_cmdline() is introduced to tidy
> > different handling pathes.
> > We also add a new helper cpufreq_opts_contain() to ignore and warn
> > user redundant setting, like "cpufreq=hwp;hwp;xen"
> >
> > Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
>
> > @@ -114,20 +161,14 @@ 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;
> > +            ret = handle_cpufreq_cmdline(CPUFREQ_xen);
>
> Do we need to check ret and error out?  (and below)
>

Right, we need, thanks, will fix

> Thanks,
> Jason
>
> >               if ( 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;
> > +            ret = handle_cpufreq_cmdline(CPUFREQ_hwp);
> >               if ( arg[0] && arg[1] )
> >                   ret = hwp_cmdline_parse(arg + 1, end);
> >           }