[Public] > -----Original Message----- > From: Jan Beulich <[email protected]> > Sent: Thursday, September 25, 2025 2:12 PM > To: Andryuk, Jason <[email protected]>; Penny, Zheng > <[email protected]> > Cc: Huang, Ray <[email protected]>; Anthony PERARD > <[email protected]>; Andrew Cooper <[email protected]>; > Roger Pau Monné <[email protected]>; [email protected] > Subject: Re: [PATCH v10 7/8] xen/cpufreq: Adapt SET/GET_CPUFREQ_CPPC > xen_sysctl_pm_op for amd-cppc driver > > On 23.09.2025 18:47, Jason Andryuk wrote: > > On 2025-09-23 11:38, Jan Beulich wrote: > >> On 23.09.2025 06:38, Penny Zheng wrote: > >>> @@ -154,6 +156,17 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op > *op) > >>> else > >>> strlcpy(op->u.get_para.scaling_driver, "Unknown", > >>> CPUFREQ_NAME_LEN); > >>> > >>> + /* > >>> + * In CPPC active mode, we are borrowing governor field to indicate > >>> + * policy info. > >>> + */ > >>> + if ( policy->governor->name[0] ) > >> > >> amd_cppc_prepare_policy() may leave ->governor set to NULL afaics, so > >> I think you need to add a NULL check here alongside with pulling this > >> out of ... > >> > >>> + strlcpy(op->u.get_para.s.scaling_governor, > >>> + policy->governor->name, CPUFREQ_NAME_LEN); > >>> + else > >>> + strlcpy(op->u.get_para.s.scaling_governor, "Unknown", > >>> + CPUFREQ_NAME_LEN); > >>> + > >>> if ( !cpufreq_is_governorless(op->cpuid) ) > >>> { > >> > >> ... this conditional. > >> > >> The description also continues to not mention the effect for HWP. I'm > >> actually somewhat confused, I suppose (Jason, question mainly to you): > >> HWP falls in the governor-less category, iirc. Yet it doesn't supply > >> a .setpolicy hook, hence __cpufreq_set_policy() goes through the > >> normal governor setting logic. What's the deal here? The answer may > >> affect whether I'd deem the pulling out of the conditional correct > >> (or at least > >> benign) here as to HWP. > > > > Hi, > > > > When I wrote HWP, I didn't realize using .setpolicy would bypass the > > governor code. Instead, I implemented the no-op HWP governor, since I > > thought I needed something as a governor. > > > > set_hwp_para() actually changes the configuration. HWP only > > implements the equivalent of amd-cppc-epp autonomous (active) mode. > > > > So I think HWP could switch to .setpolicy and drop its governor. > > > > But looking at this hunk: > > > > > @@ -321,10 +327,12 @@ static int set_cpufreq_cppc(struct > > > xen_sysctl_pm_op *op) > > > if ( !policy || !policy->governor ) > > > > Doesn't this !policy->governor prevent amd-cppc-epp from setting > > parameters? > > Only if amd_cppc_prepare_policy() took the default case path of its switch(), > aiui. > Penny? >
Hmmm, I shall admit that I omitted the default case. Actually, in current codes, no matter it is amd-cppc or amd-cppc-epp, or any other cpufreq driver(except hwp), in its .init(), we have: ``` policy->governor = cpufreq_opt_governor ? : CPUFREQ_DEFAULT_GOVERNOR; ``` So we have CPUFREQ_DEFAULT_GOVERNOR when no specified governor set through cmfline, and it is ondemand... As long as .init() is executed successfully, we will never have NULL governor field right now. > Jan > > > > return -ENOENT; > > > > > > - if ( !hwp_active() ) > > > - return -EOPNOTSUPP; > > > + if ( hwp_active() ) > > > + return set_hwp_para(policy, &op->u.set_cppc); > > > + if ( processor_pminfo[op->cpuid]->init & XEN_CPPC_INIT ) > > > + return amd_cppc_set_para(policy, &op->u.set_cppc); > > > > > > - return set_hwp_para(policy, &op->u.set_cppc); > > > + return -EOPNOTSUPP; > > > } > > > > So there may be other checks that would need dropping or adjusting to > > support HWP without a governor. > > > > Thanks, > > Jason
