[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;
+ }
+
+ return ret;
+}
```
> Jan
