Re: [PATCH v5 03/18] xen/cpufreq: extract _PSD info from "struct xen_processor_performance"

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.06.2025 06:12, Penny, Zheng wrote:
> [Public]
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Monday, June 16, 2025 5:51 PM
>> To: Penny, Zheng <penny.zheng@xxxxxxx>
>> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
>> Subject: Re: [PATCH v5 03/18] xen/cpufreq: extract _PSD info from "struct
>> xen_processor_performance"
>>
>> On 16.06.2025 11:43, Penny, Zheng wrote:
>>> [Public]
>>>
>>>> -----Original Message-----
>>>> From: Jan Beulich <jbeulich@xxxxxxxx>
>>>> Sent: Wednesday, June 11, 2025 11:34 PM
>>>> To: Penny, Zheng <penny.zheng@xxxxxxx>
>>>> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
>>>> Subject: Re: [PATCH v5 03/18] xen/cpufreq: extract _PSD info from
>>>> "struct xen_processor_performance"
>>>>
>>>> On 27.05.2025 10:48, Penny Zheng wrote:
>>>>> @@ -545,14 +597,9 @@ int set_px_pminfo(uint32_t acpi_id, struct
>>>>> xen_processor_performance *perf)
>>>>>
>>>>>      if ( perf->flags & XEN_PX_PSD )
>>>>>      {
>>>>> -        /* check domain coordination */
>>>>> -        if ( perf->shared_type != CPUFREQ_SHARED_TYPE_ALL &&
>>>>> -             perf->shared_type != CPUFREQ_SHARED_TYPE_ANY &&
>>>>> -             perf->shared_type != CPUFREQ_SHARED_TYPE_HW )
>>>>> -        {
>>>>> -            ret = -EINVAL;
>>>>> +        ret = check_psd_pminfo(perf->shared_type);
>>>>> +        if ( ret )
>>>>>              goto out;
>>>>> -        }
>>>>>
>>>>>          pxpt->shared_type = perf->shared_type;
>>>>>          memcpy(&pxpt->domain_info, &perf->domain_info,
>>>>
>>>> ... the need for this change. And even if there is a need, a
>>>> follow-on question would be how this relates to the subject of this patch.
>>>
>>> I extracted this snippet out for sharing the same checking logic both
>>> in Px and later CPPC. They both need _PSD info
>>
>> Right, and that (iirc) becomes visible later in the series. But it needs 
>> saying here. As
>> it stands the description talks of only get_psd_info() right now. And the 
>> change
>> above is also unrelated to the "extract" mentioned in the title.
>>
>>> I could change title to "xen/cpufreq: make _PSD info common" and also
>>> add description in commit message for introducing check_psd_pminfo()
>>
>> The title was probably fine; it's the description which was lacking. In fact 
>> I'd deem
>> "make ... common" misleading when there's no 2nd user (yet).
>>
> 
> How about:
> "
> Title: xen/cpufreq: export _PSD info and checking

As said, the original title was probably fine. In the new title (and also in
the text suggested below), I wonder what "export" means.

> _PSD info, consisted of "shared_type" and "struct xen_psd_package", will not
> only be provided from px-specific "struct xen_processor_performance", but also
> in CPPC data.
> 
> In cpufreq_add/del_cpu(), a new helper get_psd_info() is introduced to
> export _PSD info. While in set_px_pminfo(), check_psd_pminfo() is also 
> introduced to
> export _PSD value checking.

How about "Two new helper functions are introduced to deal with _PSD. They
will later be re-used for handling the same data for CPPC."

Jan

> in the meantime, the following style corrections get applied at the same 
> time: ........
> ```
> 
>> Jan