Re: [PATCH v8 8/8] xen/cpufreq: Adapt SET/GET_CPUFREQ_CPPC xen_sysctl_pm_op for amd-cppc driver

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.09.2025 08:46, Penny, Zheng wrote:
> [Public]
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Wednesday, September 3, 2025 2:22 PM
>> To: Penny, Zheng <penny.zheng@xxxxxxx>
>> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Anthony PERARD
>> <anthony.perard@xxxxxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>;
>> Roger Pau Monné <roger.pau@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
>> Andryuk, Jason <Jason.Andryuk@xxxxxxx>
>> Subject: Re: [PATCH v8 8/8] xen/cpufreq: Adapt SET/GET_CPUFREQ_CPPC
>> xen_sysctl_pm_op for amd-cppc driver
>>
>> On 03.09.2025 05:14, Penny, Zheng wrote:
>>> [Public]
>>>
>>>> -----Original Message-----
>>>> From: Jan Beulich <jbeulich@xxxxxxxx>
>>>> Sent: Thursday, August 28, 2025 7:07 PM
>>>> To: Penny, Zheng <penny.zheng@xxxxxxx>
>>>> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Anthony PERARD
>>>> <anthony.perard@xxxxxxxxxx>; Andrew Cooper
>>>> <andrew.cooper3@xxxxxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>;
>>>> xen-devel@xxxxxxxxxxxxxxxxxxxx
>>>> Subject: Re: [PATCH v8 8/8] xen/cpufreq: Adapt SET/GET_CPUFREQ_CPPC
>>>> xen_sysctl_pm_op for amd-cppc driver
>>>>
>>>> On 28.08.2025 12:06, 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] )
>>>>> +        strlcpy(op->u.get_para.u.s.scaling_governor,
>>>>> +                policy->governor->name, CPUFREQ_NAME_LEN);
>>>>> +    else
>>>>> +        strlcpy(op->u.get_para.u.s.scaling_governor, "Unknown",
>>>>> +                CPUFREQ_NAME_LEN);
>>>>
>>>> Isn't pulling this ...
>>>>
>>>>>      if ( !cpufreq_is_governorless(op->cpuid) )
>>>>>      {
>>>>>          if ( !(scaling_available_governors =
>>>>
>>>> ... out of this if()'s body going to affect HWP? It's not clear to me
>>>> whether that would be entirely benign.
>>>
>>> HWP has its own unique "hwp" governor. So, imo, it may not affect.
>>
>> How does it matter what (unique or not) governor it uses? The relevant 
>> aspect (to
>> me) is the !cpufreq_is_governorless() check that previously guarded the 
>> copying
>> of the name. (It would be another thing if this was benign to HWP, but such 
>> would
>> need clarifying in the description. Cc-ing Jason just in case.)
> 
> Sorry, What I mean is that HWP do have a governor, so such copying of the 
> name shall be benign to the HWP. I'll clarify it in the description

FTAOD - "shall" isn't enough, it needs to be (provably) "is".

Jan