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). 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. > > > + 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 | \ > > --