On 6/30/2023 11:10 AM, Rafael J. Wysocki wrote:
> On Fri, Jun 30, 2023 at 11:02 AM Wilczynski, Michal
> <michal.wilczynski@xxxxxxxxx> wrote:
>>
>>
>> On 6/29/2023 3:15 PM, Rafael J. Wysocki wrote:
>>> On Thu, Jun 29, 2023 at 1:04 PM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>>>> I would just say "Introduce acpi_processor_osc()" in the subject and
>>>> then explain its role in the changelog.
>>>>
>>>> On Tue, Jun 13, 2023 at 6:12 PM Michal Wilczynski
>>>> <michal.wilczynski@xxxxxxxxx> wrote:
>>>>> Currently in ACPI code _OSC method is already used for workaround
>>>>> introduced in commit a21211672c9a ("ACPI / processor: Request native
>>>>> thermal interrupt handling via _OSC"). Create new function, similar to
>>>>> already existing acpi_hwp_native_thermal_lvt_osc(). Call new function
>>>>> acpi_processor_osc(). Make this function fulfill the purpose previously
>>>>> fulfilled by the workaround plus convey OSPM processor capabilities
>>>>> with it by setting correct processor capability bits.
>>>>>
>>>>> Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>>>>> Signed-off-by: Michal Wilczynski <michal.wilczynski@xxxxxxxxx>
>>>>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
>>>>> ---
>>>>> arch/x86/include/asm/acpi.h | 3 +++
>>>>> drivers/acpi/acpi_processor.c | 43 ++++++++++++++++++++++++++++++++++-
>>>>> include/acpi/pdc_intel.h | 1 +
>>>>> 3 files changed, 46 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
>>>>> index 6a498d1781e7..6c25ce2dad18 100644
>>>>> --- a/arch/x86/include/asm/acpi.h
>>>>> +++ b/arch/x86/include/asm/acpi.h
>>>>> @@ -112,6 +112,9 @@ static inline void arch_acpi_set_proc_cap_bits(u32 *cap)
>>>>> if (cpu_has(c, X86_FEATURE_ACPI))
>>>>> *cap |= ACPI_PDC_T_FFH;
>>>>>
>>>>> + if (cpu_has(c, X86_FEATURE_HWP))
>>>>> + *cap |= ACPI_PDC_COLLAB_PROC_PERF;
>>>>> +
>>>>> /*
>>>>> * If mwait/monitor is unsupported, C2/C3_FFH will be disabled
>>>>> */
>>>>> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
>>>>> index 8c5d0295a042..0de0b05b6f53 100644
>>>>> --- a/drivers/acpi/acpi_processor.c
>>>>> +++ b/drivers/acpi/acpi_processor.c
>>>>> @@ -591,13 +591,54 @@ void __init processor_dmi_check(void)
>>>>> dmi_check_system(processor_idle_dmi_table);
>>>>> }
>>>>>
>>>>> +/* vendor specific UUID indicating an Intel platform */
>>>>> +static u8 sb_uuid_str[] = "4077A616-290C-47BE-9EBD-D87058713953";
>>>>> static bool acpi_hwp_native_thermal_lvt_set;
>>>>> +static acpi_status __init acpi_processor_osc(acpi_handle handle, u32 lvl,
>>>>> + void *context, void **rv)
>>>>> +{
>>>>> + u32 capbuf[2] = {};
>>>>> + acpi_status status;
>>>>> + struct acpi_osc_context osc_context = {
>>>>> + .uuid_str = sb_uuid_str,
>>>>> + .rev = 1,
>>>>> + .cap.length = 8,
>>>>> + .cap.pointer = capbuf,
>>>>> + };
>>>>> +
>>>>> + if (processor_physically_present(handle) == false)
>>>> if (!processor_physically_present(handle))
>>>>
>>>>> + return AE_OK;
>>>>> +
>>>>> + arch_acpi_set_proc_cap_bits(&capbuf[OSC_SUPPORT_DWORD]);
>>>>> +
>>>>> + if (boot_option_idle_override == IDLE_NOMWAIT)
>>>>> + capbuf[OSC_SUPPORT_DWORD] &=
>>>>> + ~(ACPI_PDC_C_C2C3_FFH | ACPI_PDC_C_C1_FFH);
>>>>> +
>>>>> + status = acpi_run_osc(handle, &osc_context);
>>>>> + if (ACPI_FAILURE(status))
>>>>> + return status;
>>>>> +
>>>>> + if (osc_context.ret.pointer && osc_context.ret.length > 1) {
>>>>> + u32 *capbuf_ret = osc_context.ret.pointer;
>>>>> +
>>>>> + if (!acpi_hwp_native_thermal_lvt_set &&
>>>>> + capbuf_ret[1] & ACPI_PDC_COLLAB_PROC_PERF) {
>>>> Checking it in capbuf_ret[] if it was not set in capbuf[] is sort of
>>>> questionable.
>>>>
>>>> Note that acpi_hwp_native_thermal_lvt_osc() sets it in capbuf[] before
>>>> calling acpi_run_osc().
>>> So you moved setting it to arch_acpi_set_proc_cap_bits(), but then it
>>> should also be checked by the arch code. That is, add an arch
>>> function to check if a given bit is set in the returned capabilities
>>> buffer (passed as an argument).
>> Hmm, maybe that's true, but the only reason we check that is so we can print
>> a debug message
> No, it is not the only reason. The more important part is to set
> acpi_hwp_native_thermal_lvt_set if it is still unset at that point.
Yeah, that too. Okay I'll modify the code
>
>> - that's pretty much a leftover after a workaround. Introducing
>> more arch code to accommodate this seemed wasteful, since in my understanding
>> all workarounds are meant to be removed at some point, even if it takes a long time
>> to do so.
> Not really, until the systems needing them are in use.
Yeah I suspect it might take a very long time, and I guess it's very hard to definitely
say that some piece of hardware is not used by **anyone**
>
>>> Also it can be argued that ACPI_PDC_C_C2C3_FFH and ACPI_PDC_C_C1_FFH
>>> should be set by the arch code too.
>> That makes sense, but technically is also a workaround, since we're basically
>> checking for some specific DMI's and then we disable mwait for them.
> But boot_option_idle_override is set by the arch code, isn't it?
>
> So the arch code may as well do the quirk in accordance with it.
I think so, I'll modify the code to move setting those bits to the arch part
