[Public]
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: Thursday, July 17, 2025 12:00 AM
> 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-
> de...@lists.xenproject.org
> Subject: Re: [PATCH v6 11/19] xen/x86: introduce "cpufreq=amd-cppc" xen
> cmdline
> and amd-cppc driver
>
> On 11.07.2025 05:50, Penny Zheng wrote:
> > --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
> > +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
> > @@ -128,12 +128,14 @@ static int __init cf_check
> > cpufreq_driver_init(void)
> >
> > if ( cpufreq_controller == FREQCTL_xen )
> > {
> > + unsigned int i = 0;
>
> Pointless initializer; both for() loops set i to 0. But also see further down.
>
> > @@ -157,9 +164,70 @@ static int __init cf_check
> > cpufreq_driver_init(void)
> >
> > case X86_VENDOR_AMD:
> > case X86_VENDOR_HYGON:
> > - ret = IS_ENABLED(CONFIG_AMD) ? powernow_register_driver() : -
> ENODEV;
> > + if ( !IS_ENABLED(CONFIG_AMD) )
> > + {
> > + ret = -ENODEV;
> > + break;
> > + }
> > + ret = -ENOENT;
>
> The code structure is sufficiently different from the Intel counterpart for
> this to
> perhaps better move ...
>
> > + for ( i = 0; i < cpufreq_xen_cnt; i++ )
> > + {
> > + switch ( cpufreq_xen_opts[i] )
> > + {
> > + case CPUFREQ_xen:
> > + ret = powernow_register_driver();
> > + break;
> > +
> > + case CPUFREQ_amd_cppc:
> > + ret = amd_cppc_register_driver();
> > + break;
> > +
> > + case CPUFREQ_none:
> > + ret = 0;
> > + break;
> > +
> > + default:
> > + printk(XENLOG_WARNING
> > + "Unsupported cpufreq driver for vendor AMD or
> > Hygon\n");
> > + break;
>
> ... here.
>
Are we suggesting moving
"
if ( !IS_ENABLED(CONFIG_AMD) )
{
ret = -ENODEV;
break;
}
" here? In which case, When CONFIG_AMD=n and users doesn't provide
"cpufreq=xxx", we will have cpufreq_xen_cnt initialized as 1 and
cpufreq_xen_opts[0] = CPUFREQ_xen. powernow_register_driver() hence gets
invoked. The thing is that we don't have stub for it and it is compiled under
CONFIG_AMD
I suggest to change to use #ifdef CONFIG_AMD code wrapping
> > + }
> > +
> > + if ( !ret || ret == -EBUSY )
> > + break;
> > + }
> > +
> > break;
> > }
> > +
> > + /*
> > + * After successful cpufreq driver registeration,
> XEN_PROCESSOR_PM_CPPC
> > + * and XEN_PROCESSOR_PM_PX shall become exclusive flags.
> > + */
> > + if ( !ret )
> > + {
> > + ASSERT(i < cpufreq_xen_cnt);
> > + switch ( cpufreq_xen_opts[i] )
>
> Hmm, this is using the the initializer of i that I commented on. I think
> there's
> another default: case missing, where you simply "return 0" (to retain prior
> behavior).
> But again see also yet further down.
>
>
> > + /*
> > + * No cpufreq driver gets registered, clear both
> > + * XEN_PROCESSOR_PM_CPPC and XEN_PROCESSOR_PM_PX
> > + */
> > + xen_processor_pmbits &= ~(XEN_PROCESSOR_PM_CPPC |
> > + XEN_PROCESSOR_PM_PX);
>
> Yet more hmm - this path you want to get through for the case mentioned above.
> But only this code; specifically not the "switch ( cpufreq_xen_opts[i] )",
> which really
> is "switch ( cpufreq_xen_opts[0] )" in that case, and that's pretty clearly
> wrong to
> evaluate in then.
>
Correct me if I understand you wrongly:
The above "case missing" , are we talking about is entering "case CPUFREQ_none"
?
IMO, it may never be entered. If users doesn't provide "cpufreq=xxx", we will
have cpufreq_xen_cnt initialized as 1 and cpufreq_xen_opts[0] = CPUFREQ_xen.
That is, we will have px states as default driver. Even if we have failed
px-driver initialization, with cpufreq_xen_cnt limited to 1, we will not enter
CPUFREQ_none.
CPUFREQ_none only could be set when users explicitly set
"cpufreq=disabled/none/0", but in which case, cpufreq_controller will be set
with FREQCTL_none. And the whole cpufreq_driver_init() is under "
cpufreq_controller == FREQCTL_xen " condition
Or "case missing" is referring entering default case? In which case, we will
have -ENOENT errno. As we have ret=-ENOENT in the very beginning
> Jan
