[Public]
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: Tuesday, April 1, 2025 2:38 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 01.04.2025 07:44, Penny, Zheng wrote:
> > [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;
> > + }
>
> 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
