[Public]
> -----Original Message-----
> From: Penny, Zheng <penny.zh...@amd.com>
> Sent: Monday, April 14, 2025 3:41 PM
> To: xen-devel@lists.xenproject.org
> Cc: Huang, Ray <ray.hu...@amd.com>; Penny, Zheng
> <penny.zh...@amd.com>; Jan Beulich <jbeul...@suse.com>
> Subject: [PATCH v4 04/15] xen/cpufreq: refactor cmdline "cpufreq=xxx"
>
> 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.zh...@amd.com>
> ---
> v2 -> v3:
> - new commit
> ---
> v3 -> v4:
> - add one single helper to do the tidy work
> - ignore and warn user redundant setting
> ---
> xen/drivers/cpufreq/cpufreq.c | 53 +++++++++++++++++++++++++++++------
> 1 file changed, 45 insertions(+), 8 deletions(-)
>
> diff --git a/xen/drivers/cpufreq/cpufreq.c b/xen/drivers/cpufreq/cpufreq.c
> index
> e01acc0c2d..79c6444116 100644
> --- a/xen/drivers/cpufreq/cpufreq.c
> +++ b/xen/drivers/cpufreq/cpufreq.c
> @@ -71,6 +71,49 @@ 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) )
> + {
> + const char *cpufreq_opts_str[] = { "CPUFREQ_xen", "CPUFREQ_hwp"
> + };
> +
> + printk(XENLOG_WARNING
> + "Duplicate cpufreq driver option: %s",
> + cpufreq_opts_str[option - 1]);
> + 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;
I'm thinking since handle_cpufreq_cmdline() is totally internal and has very
few caller, maybe ASSERT_UNREACHABLE() is more suitable. Then the function
itself could become void return.
> + break;
> + }
> +
> + return ret;
> +}
> +
> static int __init cf_check setup_cpufreq_option(const char *str) {
> const char *arg = strpbrk(str, ",:;"); @@ -114,20 +157,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
