On Mon, Jul 3, 2023 at 10:54 AM Wilczynski, Michal <michal.wilczynski@xxxxxxxxx> wrote: > > > > On 6/30/2023 10:46 AM, Wilczynski, Michal wrote: > > 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, > > Actually on a second thought, maybe instead of creating _OSC specifc constants it would > be better to just generalize constant names ? Yes, that would work too. > As they're the same for both methods, they > are not really method specific and could be called as follows: > > ACPI_PROC_CAP_C_C1_FFH > ACPI_PROC_CAP_C_C2C3_FFH > > So instead of using OSC, or PDC, we just use PROC_CAP, which better explain what those bits > mean at the end, and removes the hassle of removing those PDC specifc constants in some far > away future. > > Please let me know your thoughts, Yes, you can do that as far as I am concerned.