Re: [PATCH v3 3/5] acpi: Introduce new function callback for _OSC

From: Wilczynski, Michal
Date: Fri Jun 30 2023 - 05:02:39 EST



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


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


>
>>> + acpi_handle_info(handle,
>>> + "_OSC native thermal LVT Acked\n");
>>> + acpi_hwp_native_thermal_lvt_set = true;
>>> + }
>>> + }
>>> + kfree(osc_context.ret.pointer);
>>> +
>>> + return AE_OK;
>>> +}
>>> +
>>> static acpi_status __init acpi_hwp_native_thermal_lvt_osc(acpi_handle handle,
>>> u32 lvl,
>>> void *context,
>>> void **rv)
>>> {
>>> - u8 sb_uuid_str[] = "4077A616-290C-47BE-9EBD-D87058713953";
>>> u32 capbuf[2];
>>> struct acpi_osc_context osc_context = {
>>> .uuid_str = sb_uuid_str,
>>> diff --git a/include/acpi/pdc_intel.h b/include/acpi/pdc_intel.h
>>> index 967c552d1cd3..9427f639287f 100644
>>> --- a/include/acpi/pdc_intel.h
>>> +++ b/include/acpi/pdc_intel.h
>>> @@ -16,6 +16,7 @@
>>> #define ACPI_PDC_C_C1_FFH (0x0100)
>>> #define ACPI_PDC_C_C2C3_FFH (0x0200)
>>> #define ACPI_PDC_SMP_P_HWCOORD (0x0800)
>>> +#define ACPI_PDC_COLLAB_PROC_PERF (0x1000)
>> I would call this ACPI_OSC_COLLAB_PROC_PERF to avoid confusion.
>>
>> It may also be a good idea to introduce ACPI_OSC_ symbols to replace
>> the existing ACPI_PDC_ ones (with the same values, respectively) and
>> get rid of the latter later.
>>
>>> #define ACPI_PDC_EST_CAPABILITY_SMP (ACPI_PDC_SMP_C1PT | \
>>> ACPI_PDC_C_C1_HALT | \
>>> --