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?
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
