RE: [PATCH v3 03/15] xen/cpufreq: refactor cmdline "cpufreq=xxx"

["Penny, Zheng" <penny.zheng@xxxxxxx>]

[Public]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Tuesday, April 1, 2025 2:38 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Anthony PERARD <anthony.perard@xxxxxxxxxx>;
> Orzel, Michal <Michal.Orzel@xxxxxxx>; Julien Grall <julien@xxxxxxx>; Roger
> Pau Monné <roger.pau@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>;
> xen-devel@xxxxxxxxxxxxxxxxxxxx
> 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 <jbeulich@xxxxxxxx>
> >> Sent: Wednesday, March 26, 2025 6:43 PM
> >> To: Penny, Zheng <penny.zheng@xxxxxxx>
> >> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Andrew Cooper
> >> <andrew.cooper3@xxxxxxxxxx>; Anthony PERARD
> >> <anthony.perard@xxxxxxxxxx>; Orzel, Michal <Michal.Orzel@xxxxxxx>;
> >> Julien Grall <julien@xxxxxxx>; Roger Pau Monné
> >> <roger.pau@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>;
> >> xen-devel@xxxxxxxxxxxxxxxxxxxx
> >> 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 <jbeulich@xxxxxxxx>
> >>>> 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