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

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

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

> Jan