On 26.03.2025 09:35, Penny, Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeul...@suse.com>
>> Sent: Monday, March 24, 2025 11:26 PM
>>
>> On 06.03.2025 09:39, Penny Zheng wrote:
>>> @@ -514,5 +515,14 @@ acpi_cpufreq_driver = {
>>>
>>> int __init acpi_cpufreq_register(void) {
>>> - return cpufreq_register_driver(&acpi_cpufreq_driver);
>>> + int ret;
>>> +
>>> + ret = cpufreq_register_driver(&acpi_cpufreq_driver);
>>> + if ( ret )
>>> + return ret;
>>> +
>>> + if ( IS_ENABLED(CONFIG_AMD) )
>>> + xen_processor_pmbits &= ~XEN_PROCESSOR_PM_CPPC;
>>
>> What's the purpose of the if() here?
>
> After cpufreq driver properly registered, I'd like XEN_PROCESSOR_PM_PX and
> XEN_PROCESSOR_PM_CPPC
> being exclusive value to represent the actual underlying registered driver.
> As users could define something like "cpufreq=amd-cppc,xen", which implies
> both XEN_PROCESSOR_PM_PX and XEN_PROCESSOR_PM_CPPC
> got set in parsing logic. With amd-cppc failing to register, we are falling
> back to legacy ones. Then XEN_PROCESSOR_PM_CPPC needs to clear.
Looks like you try to explain the &= when my question was about the if().
I understand the purpose of the &=. What I don't understand is why it needs
to be conditional.
>>> --- a/xen/include/acpi/cpufreq/cpufreq.h
>>> +++ b/xen/include/acpi/cpufreq/cpufreq.h
>>> @@ -28,6 +28,7 @@ enum cpufreq_xen_opt {
>>> CPUFREQ_none,
>>> CPUFREQ_xen,
>>> CPUFREQ_hwp,
>>> + CPUFREQ_amd_cppc,
>>> };
>>> extern enum cpufreq_xen_opt cpufreq_xen_opts[2];
>>
>> I'm pretty sure I pointed out before that this array needs to grow, now that
>> you add a
>> 3rd kind of handling.
>>
>
> Hmmm, but the CPUFREQ_hwp and CPUFREQ_amd_cppc are incompatible options.
> I thought cpufreq_xen_opts[] shall reflect available choices on their
> hardware.
> Even if users define "cpufreq=hwp;amd-cppc;xen", in Intel platform,
> cpufreq_xen_opts[] shall
> contain CPUFREQ_hwp and CPUFREQ_xen, while in amd platform,
> cpufreq_xen_opts[] shall
> contain CPUFREQ_amd_cppc and CPUFREQ_xen
Maybe I misread the code, but the impression I got was that
"cpufreq=hwp;amd-cppc;xen"
would populate 3 slots of the array (with one of "hwp" and "amd-cppc"
necessarily not
working, leading to the next one to be tried).
Jan
