| Message ID | 20230613161034.3496047-4-michal.wilczynski@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Prefer using _OSC method over deprecated _PDC | expand |
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@intel.com> 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@intel.com> > Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > 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(). > + 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 | \ > --
On Thu, Jun 29, 2023 at 1:04 PM Rafael J. Wysocki <rafael@kernel.org> 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@intel.com> 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@intel.com> > > Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > --- > > 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 | \ > > --
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@intel.com> 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@intel.com> >> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> >> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> --- >> 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 | \ >> --
On 6/29/2023 3:15 PM, Rafael J. Wysocki wrote: > On Thu, Jun 29, 2023 at 1:04 PM Rafael J. Wysocki <rafael@kernel.org> 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@intel.com> 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@intel.com> >>> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> >>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >>> --- >>> 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 | \ >>> --
On Fri, Jun 30, 2023 at 11:02 AM Wilczynski, Michal <michal.wilczynski@intel.com> 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@kernel.org> 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@intel.com> 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@intel.com> > >>> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> > >>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > >>> --- > >>> 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. > - 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. > > > > 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.
On 6/30/2023 11:10 AM, Rafael J. Wysocki wrote: > On Fri, Jun 30, 2023 at 11:02 AM Wilczynski, Michal > <michal.wilczynski@intel.com> 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@kernel.org> 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@intel.com> 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@intel.com> >>>>> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> >>>>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >>>>> --- >>>>> 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
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@intel.com> 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@intel.com> >>> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> >>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >>> --- >>> 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 ? 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, Michał > >>> #define ACPI_PDC_EST_CAPABILITY_SMP (ACPI_PDC_SMP_C1PT | \ >>> ACPI_PDC_C_C1_HALT | \ >>> --
On 6/30/2023 11:23 AM, Wilczynski, Michal wrote: > > On 6/30/2023 11:10 AM, Rafael J. Wysocki wrote: >> On Fri, Jun 30, 2023 at 11:02 AM Wilczynski, Michal >> <michal.wilczynski@intel.com> 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@kernel.org> 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@intel.com> 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@intel.com> >>>>>> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> >>>>>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >>>>>> --- >>>>>> 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 I looked into that, and I'm still not sure whether setting those constants in arch specific code is a good idea. Basically OSC and PDC are supported on two architectures ia64 and x86, so that would introduce unnecessary code duplication, as this mechanic is present regardless of an architecture, and in this particular case boot_option_idle_override is set by acpi_processor.c function set_no_mwait(). One could argue theoretically that system defined in processor_dmi_table[] is an x86 so there is no need to add any logic to ia64, but to me this is confusing. If we have a workaround in the acpi_processor, maybe entire workaround should stay there instead of dragging innocent arch code into it. Please let me know your thoughts, Michał > >
On Mon, Jul 3, 2023 at 10:54 AM Wilczynski, Michal <michal.wilczynski@intel.com> 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@intel.com> 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@intel.com> > >>> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> > >>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > >>> --- > >>> 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.
On Mon, Jul 3, 2023 at 11:52 AM Wilczynski, Michal <michal.wilczynski@intel.com> wrote: > > > > On 6/30/2023 11:23 AM, Wilczynski, Michal wrote: > > > > On 6/30/2023 11:10 AM, Rafael J. Wysocki wrote: > >> On Fri, Jun 30, 2023 at 11:02 AM Wilczynski, Michal > >> <michal.wilczynski@intel.com> 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@kernel.org> 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@intel.com> 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@intel.com> > >>>>>> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> > >>>>>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > >>>>>> --- > >>>>>> 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 > > I looked into that, and I'm still not sure whether setting those constants in arch > specific code is a good idea. Basically OSC and PDC are supported on two architectures > ia64 and x86, so that would introduce unnecessary code duplication, as this mechanic > is present regardless of an architecture, and in this particular case boot_option_idle_override > is set by acpi_processor.c function set_no_mwait(). Which is x86-specific AFAICS. > One could argue theoretically that system defined in processor_dmi_table[] is an x86 so there > is no need to add any logic to ia64, Good observation! > but to me this is confusing. Why is it so? > If we have a workaround in the acpi_processor, maybe entire workaround should stay there > instead of dragging innocent arch code into it. And maybe it would be better to move it to arch code as a whole.
On 7/3/2023 5:22 PM, Rafael J. Wysocki wrote: > On Mon, Jul 3, 2023 at 11:52 AM Wilczynski, Michal > <michal.wilczynski@intel.com> wrote: >> >> >> On 6/30/2023 11:23 AM, Wilczynski, Michal wrote: >>> On 6/30/2023 11:10 AM, Rafael J. Wysocki wrote: >>>> On Fri, Jun 30, 2023 at 11:02 AM Wilczynski, Michal >>>> <michal.wilczynski@intel.com> 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@kernel.org> 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@intel.com> 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@intel.com> >>>>>>>> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> >>>>>>>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >>>>>>>> --- >>>>>>>> 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 >> I looked into that, and I'm still not sure whether setting those constants in arch >> specific code is a good idea. Basically OSC and PDC are supported on two architectures >> ia64 and x86, so that would introduce unnecessary code duplication, as this mechanic >> is present regardless of an architecture, and in this particular case boot_option_idle_override >> is set by acpi_processor.c function set_no_mwait(). > Which is x86-specific AFAICS. > >> One could argue theoretically that system defined in processor_dmi_table[] is an x86 so there >> is no need to add any logic to ia64, > Good observation! > >> but to me this is confusing. > Why is it so? > >> If we have a workaround in the acpi_processor, maybe entire workaround should stay there >> instead of dragging innocent arch code into it. > And maybe it would be better to move it to arch code as a whole. OK, so I thought this through... There are actually three things here: - first introduced by this commit a21211672c9a ("ACPI / processor: Request native thermal interrupt handling via _OSC"). It enabled bit 12 in capabilities buffer meaning enabling Collaborative Processor Performance Control interrupts to be handled by the OSPM instead of the firmware. I guess it's not really a workaround per se, but it was introduced in the context of the workaround. Frankly I can't see a reason why it should stay in it's current state i.e executed only once and acknowledged, while all other proc capabilites aren't really acknowledged. It should be treated exactly as other capabilities without any special treatment. So to fix that I would propose to remove this altogether: if (!acpi_hwp_native_thermal_lvt_set && capbuf_ret[1] & ACPI_PDC_COLLAB_PROC_PERF) { Plus remove acpi_hwp_native_thermal_lvt altogether and the print acknowledging sucessful set. And leave the setting of this bit in arch_acpi_set_proc_cap_bits(). This makes most sense to me. I could imagine that the author of this commit wanted to see whether workaround worked or not and have some trace in logs but I don't think it's necessary anymore. - second commit da5e09a1b3e5 ("ACPI : Create "idle=nomwait" bootparam"). It introduced this line in acpi_processor_set_pdc(): buffer[2] &= ~(ACPI_PDC_C_C2C3_FFH | ACPI_PDC_C_C1_FFH);. I think it was incorrectly put in the wrong file from the start. It should be set in arch code, preferably in arch_acpi_set_proc_cap_bits(). This achieves better coherency, as it's an actual arch specific thing and we have setting of all capabilities bits in one place. - third commit 2a2a64714d9c ("ACPI: Disable MWAIT via DMI on broken Compal board"), it is an actual workaround. I think like you suggested offline it should belong in drivers/acpi/x86 directory. We can use utils.c file in that directory as it already contain some other workarounds. Please let me know whether you object to any of this, Michał
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) + 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) { + 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) #define ACPI_PDC_EST_CAPABILITY_SMP (ACPI_PDC_SMP_C1PT | \ ACPI_PDC_C_C1_HALT | \
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@intel.com> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- 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(-)