Hi,
Thanks for the review !
On 6/29/2023 1:04 PM, Rafael J. Wysocki wrote:
> I would just say "Introduce acpi_processor_osc()" in the subject and
> then explain its role in the changelog.
Sure,
>
> 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))
Sure,
>
>> + 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().
We can add condition before checking capbuf_ret i.e
if (capbuf[OSC_SUPPORT_DWORD] & ACPI_PDC_COLLAB_PROC_PERF &&
osc_context.ret.pointer && osc_context.ret.length > 1)
>
>> + 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.
Sure I can do that, most likely in a separate commit preceeding this one, so
it's easier to explain and review,
>
>> #define ACPI_PDC_EST_CAPABILITY_SMP (ACPI_PDC_SMP_C1PT | \
>> ACPI_PDC_C_C1_HALT | \
>> --
