[PATCH v3 0/5] Prefer using _OSC method over deprecated _PDC

[PATCH v3 0/5] Prefer using _OSC method over deprecated _PDC

[Michal Wilczynski]

ACPI 3.0 introduced a new Operating System Capabilities _OSC control
method. This method is similar to _PDC, which was marked as deprecated
in ACPI 3.0.

Prefer using _OSC method over deprecated _PDC in the acpi_bus_init(). In
case of the failure of the _OSC, try using _PDC as a fallback.

Testing done:
Tested on physical server with BIOS implementing _OSC methods. In this
case acpi_processor_osc() was executed for each CPU core. acpi_run_osc()
returned success indicating that _OSC method succeeded.

Tested on qemu VM to check whether the code would work on a SeaBios (the
default for qemu, doesn't support _OSC methods, or _PDC). This way I was
able to see how code behaves in case BIOS doesn't implement _OSC. In
that case the function
acpi_run_osc() returned failure, which propagated all the way up to
acpi_early_processor_osc(). The logic responsible for triggering _PDC
execution was triggered correctly.

Tested this using debug messages with printk.

v3:
 - split into more commits to make review easier
 - changed "_UID" to METHOD_NAME_UID
 - changed hard-coded constant to ACPI_PDC_COLLAB_PROC_PERF
 - added Suggested-by tags
 - fixed style issues
 - fixed whitespaces
 - refactored code
v2:
 - fixed compilation issues on ia64 and arm

Michal Wilczynski (5):
  acpi: Move logic responsible for conveying processor OSPM capabilities
  acpi: Refactor arch_acpi_set_pdc_bits()
  acpi: Introduce new function callback for _OSC
  acpi: Use _OSC method to convey processor OSPM capabilities
  acpi: Remove acpi_hwp_native_thermal_lvt_osc()

 arch/ia64/include/asm/acpi.h  |   4 +-
 arch/x86/include/asm/acpi.h   |  13 +--
 drivers/acpi/acpi_processor.c | 151 +++++++++++++++++++++++++++-------
 drivers/acpi/bus.c            |  13 ++-
 drivers/acpi/internal.h       |  10 +--
 drivers/acpi/processor_pdc.c  |  82 +-----------------
 include/acpi/pdc_intel.h      |   1 +
 include/acpi/processor.h      |   2 +-
 8 files changed, 148 insertions(+), 128 deletions(-)

-- 
2.41.0

[PATCH v3 1/5] acpi: Move logic responsible for conveying processor OSPM capabilities

[Michal Wilczynski]

Since _PDC method is deprecated and a preferred method of communicating
OSPM processor power management capabilities is _OSC, there is a need to
move functions checking whether processor is present and workarounds for
specific hardware to acpi_processor.c as this logic is not _PDC specific.
It also applies to the _OSC objects.

Move processor_dmi_check(), processor_idle_dmi_table, set_no_mwait() and
processor_physically_present() to acpi_processor.c.
Introduce IDLE_NOMWAIT workaround and processor_dmi_table workarounds to
work with _OSC objects.
Mark acpi_early_processor_set_pdc() and acpi_processor_set_pdc() as
deprecated.

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>
---
 drivers/acpi/acpi_processor.c | 84 ++++++++++++++++++++++++++++++++++-
 drivers/acpi/internal.h       |  3 ++
 drivers/acpi/processor_pdc.c  | 80 +--------------------------------
 include/acpi/processor.h      |  2 +-
 4 files changed, 89 insertions(+), 80 deletions(-)

diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index f9aa02cac6d1..8c5d0295a042 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -12,6 +12,7 @@
 
 #include <linux/acpi.h>
 #include <linux/device.h>
+#include <linux/dmi.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/pci.h>
@@ -21,6 +22,8 @@
 
 #include <asm/cpu.h>
 
+#include <xen/xen.h>
+
 #include "internal.h"
 
 DEFINE_PER_CPU(struct acpi_processor *, processors);
@@ -508,7 +511,86 @@ static void acpi_processor_remove(struct acpi_device *device)
 }
 #endif /* CONFIG_ACPI_HOTPLUG_CPU */
 
-#ifdef CONFIG_X86
+#ifdef CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC
+bool __init processor_physically_present(acpi_handle handle)
+{
+	int cpuid, type;
+	u32 acpi_id;
+	acpi_status status;
+	acpi_object_type acpi_type;
+	unsigned long long tmp;
+	union acpi_object object = {};
+	struct acpi_buffer buffer = { sizeof(union acpi_object), &object };
+
+	status = acpi_get_type(handle, &acpi_type);
+	if (ACPI_FAILURE(status))
+		return false;
+
+	switch (acpi_type) {
+	case ACPI_TYPE_PROCESSOR:
+		status = acpi_evaluate_object(handle, NULL, NULL, &buffer);
+		if (ACPI_FAILURE(status))
+			return false;
+		acpi_id = object.processor.proc_id;
+		break;
+	case ACPI_TYPE_DEVICE:
+		status = acpi_evaluate_integer(handle, METHOD_NAME__UID,
+					       NULL, &tmp);
+		if (ACPI_FAILURE(status))
+			return false;
+		acpi_id = tmp;
+		break;
+	default:
+		return false;
+	}
+
+	if (xen_initial_domain())
+		/*
+		 * When running as a Xen dom0 the number of processors Linux
+		 * sees can be different from the real number of processors on
+		 * the system, and we still need to execute _PDC or _OSC for
+		 * all of them.
+		 */
+		return xen_processor_present(acpi_id);
+
+	type = (acpi_type == ACPI_TYPE_DEVICE) ? 1 : 0;
+	cpuid = acpi_get_cpuid(handle, type, acpi_id);
+
+	return !invalid_logical_cpuid(cpuid);
+}
+
+static int __init set_no_mwait(const struct dmi_system_id *id)
+{
+	pr_notice("%s detected - disabling mwait for CPU C-states\n",
+		  id->ident);
+	boot_option_idle_override = IDLE_NOMWAIT;
+	return 0;
+}
+
+static const struct dmi_system_id processor_idle_dmi_table[] __initconst = {
+	{
+		.callback = set_no_mwait,
+		.ident = "Extensa 5220",
+		.matches =  {
+			DMI_MATCH(DMI_BIOS_VENDOR, "Phoenix Technologies LTD"),
+			DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
+			DMI_MATCH(DMI_PRODUCT_VERSION, "0100"),
+			DMI_MATCH(DMI_BOARD_NAME, "Columbia"),
+		},
+		.driver_data = NULL,
+	},
+	{}
+};
+
+void __init processor_dmi_check(void)
+{
+	/*
+	 * Check whether the system is DMI table. If yes, OSPM
+	 * should not use mwait for CPU-states.
+	 */
+	dmi_check_system(processor_idle_dmi_table);
+}
+
 static bool acpi_hwp_native_thermal_lvt_set;
 static acpi_status __init acpi_hwp_native_thermal_lvt_osc(acpi_handle handle,
 							  u32 lvl,
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 06ad497067ac..f979a2f7077c 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -151,6 +151,9 @@ int acpi_wakeup_device_init(void);
    -------------------------------------------------------------------------- */
 #ifdef CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC
 void acpi_early_processor_set_pdc(void);
+
+void processor_dmi_check(void);
+bool processor_physically_present(acpi_handle handle);
 #else
 static inline void acpi_early_processor_set_pdc(void) {}
 #endif
diff --git a/drivers/acpi/processor_pdc.c b/drivers/acpi/processor_pdc.c
index 18fb04523f93..5596862e6fea 100644
--- a/drivers/acpi/processor_pdc.c
+++ b/drivers/acpi/processor_pdc.c
@@ -9,61 +9,12 @@
 
 #define pr_fmt(fmt) "ACPI: " fmt
 
-#include <linux/dmi.h>
 #include <linux/slab.h>
 #include <linux/acpi.h>
 #include <acpi/processor.h>
 
-#include <xen/xen.h>
-
 #include "internal.h"
 
-static bool __init processor_physically_present(acpi_handle handle)
-{
-	int cpuid, type;
-	u32 acpi_id;
-	acpi_status status;
-	acpi_object_type acpi_type;
-	unsigned long long tmp;
-	union acpi_object object = { 0 };
-	struct acpi_buffer buffer = { sizeof(union acpi_object), &object };
-
-	status = acpi_get_type(handle, &acpi_type);
-	if (ACPI_FAILURE(status))
-		return false;
-
-	switch (acpi_type) {
-	case ACPI_TYPE_PROCESSOR:
-		status = acpi_evaluate_object(handle, NULL, NULL, &buffer);
-		if (ACPI_FAILURE(status))
-			return false;
-		acpi_id = object.processor.proc_id;
-		break;
-	case ACPI_TYPE_DEVICE:
-		status = acpi_evaluate_integer(handle, "_UID", NULL, &tmp);
-		if (ACPI_FAILURE(status))
-			return false;
-		acpi_id = tmp;
-		break;
-	default:
-		return false;
-	}
-
-	if (xen_initial_domain())
-		/*
-		 * When running as a Xen dom0 the number of processors Linux
-		 * sees can be different from the real number of processors on
-		 * the system, and we still need to execute _PDC for all of
-		 * them.
-		 */
-		return xen_processor_present(acpi_id);
-
-	type = (acpi_type == ACPI_TYPE_DEVICE) ? 1 : 0;
-	cpuid = acpi_get_cpuid(handle, type, acpi_id);
-
-	return !invalid_logical_cpuid(cpuid);
-}
-
 static void acpi_set_pdc_bits(u32 *buf)
 {
 	buf[0] = ACPI_PDC_REVISION_ID;
@@ -146,7 +97,7 @@ acpi_processor_eval_pdc(acpi_handle handle, struct acpi_object_list *pdc_in)
 	return status;
 }
 
-void acpi_processor_set_pdc(acpi_handle handle)
+void __deprecated acpi_processor_set_pdc(acpi_handle handle)
 {
 	struct acpi_object_list *obj_list;
 
@@ -174,34 +125,7 @@ early_init_pdc(acpi_handle handle, u32 lvl, void *context, void **rv)
 	return AE_OK;
 }
 
-static int __init set_no_mwait(const struct dmi_system_id *id)
-{
-	pr_notice("%s detected - disabling mwait for CPU C-states\n",
-		  id->ident);
-	boot_option_idle_override = IDLE_NOMWAIT;
-	return 0;
-}
-
-static const struct dmi_system_id processor_idle_dmi_table[] __initconst = {
-	{
-	set_no_mwait, "Extensa 5220", {
-	DMI_MATCH(DMI_BIOS_VENDOR, "Phoenix Technologies LTD"),
-	DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
-	DMI_MATCH(DMI_PRODUCT_VERSION, "0100"),
-	DMI_MATCH(DMI_BOARD_NAME, "Columbia") }, NULL},
-	{},
-};
-
-static void __init processor_dmi_check(void)
-{
-	/*
-	 * Check whether the system is DMI table. If yes, OSPM
-	 * should not use mwait for CPU-states.
-	 */
-	dmi_check_system(processor_idle_dmi_table);
-}
-
-void __init acpi_early_processor_set_pdc(void)
+void __init __deprecated acpi_early_processor_set_pdc(void)
 {
 	processor_dmi_check();
 
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index 94181fe9780a..83ed42254567 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -372,7 +372,7 @@ static inline void acpi_cppc_processor_exit(struct acpi_processor *pr)
 #endif	/* CONFIG_ACPI_CPPC_LIB */
 
 /* in processor_pdc.c */
-void acpi_processor_set_pdc(acpi_handle handle);
+void __deprecated acpi_processor_set_pdc(acpi_handle handle);
 
 /* in processor_throttling.c */
 #ifdef CONFIG_ACPI_CPU_FREQ_PSS
-- 
2.41.0

[PATCH v3 2/5] acpi: Refactor arch_acpi_set_pdc_bits()

[Michal Wilczynski]

Capabilities buffer modified by the arch_acpi_set_pdc_bits() is not
_PDC specific, as it is used by _OSC method as well. Change function
name to better reflect it's independence from PDC. Change function
expected argument to pass capability buffer directly without any
offset, as the offset differ among _OSC and _PDC methods.

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/ia64/include/asm/acpi.h |  4 ++--
 arch/x86/include/asm/acpi.h  | 10 +++++-----
 drivers/acpi/processor_pdc.c |  2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/ia64/include/asm/acpi.h b/arch/ia64/include/asm/acpi.h
index 87927eb824cc..43797cb44383 100644
--- a/arch/ia64/include/asm/acpi.h
+++ b/arch/ia64/include/asm/acpi.h
@@ -69,9 +69,9 @@ extern int __initdata nid_to_pxm_map[MAX_NUMNODES];
 #endif
 
 static inline bool arch_has_acpi_pdc(void) { return true; }
-static inline void arch_acpi_set_pdc_bits(u32 *buf)
+static inline void arch_acpi_set_proc_cap_bits(u32 *cap)
 {
-	buf[2] |= ACPI_PDC_EST_CAPABILITY_SMP;
+	*cap |= ACPI_PDC_EST_CAPABILITY_SMP;
 }
 
 #ifdef CONFIG_ACPI_NUMA
diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
index 8eb74cf386db..6a498d1781e7 100644
--- a/arch/x86/include/asm/acpi.h
+++ b/arch/x86/include/asm/acpi.h
@@ -100,23 +100,23 @@ static inline bool arch_has_acpi_pdc(void)
 		c->x86_vendor == X86_VENDOR_CENTAUR);
 }
 
-static inline void arch_acpi_set_pdc_bits(u32 *buf)
+static inline void arch_acpi_set_proc_cap_bits(u32 *cap)
 {
 	struct cpuinfo_x86 *c = &cpu_data(0);
 
-	buf[2] |= ACPI_PDC_C_CAPABILITY_SMP;
+	*cap |= ACPI_PDC_C_CAPABILITY_SMP;
 
 	if (cpu_has(c, X86_FEATURE_EST))
-		buf[2] |= ACPI_PDC_EST_CAPABILITY_SWSMP;
+		*cap |= ACPI_PDC_EST_CAPABILITY_SWSMP;
 
 	if (cpu_has(c, X86_FEATURE_ACPI))
-		buf[2] |= ACPI_PDC_T_FFH;
+		*cap |= ACPI_PDC_T_FFH;
 
 	/*
 	 * If mwait/monitor is unsupported, C2/C3_FFH will be disabled
 	 */
 	if (!cpu_has(c, X86_FEATURE_MWAIT))
-		buf[2] &= ~(ACPI_PDC_C_C2C3_FFH);
+		*cap &= ~(ACPI_PDC_C_C2C3_FFH);
 }
 
 static inline bool acpi_has_cpu_in_madt(void)
diff --git a/drivers/acpi/processor_pdc.c b/drivers/acpi/processor_pdc.c
index 5596862e6fea..ce3acd86dd12 100644
--- a/drivers/acpi/processor_pdc.c
+++ b/drivers/acpi/processor_pdc.c
@@ -24,7 +24,7 @@ static void acpi_set_pdc_bits(u32 *buf)
 	buf[2] = ACPI_PDC_SMP_T_SWCOORD;
 
 	/* Twiddle arch-specific bits needed for _PDC */
-	arch_acpi_set_pdc_bits(buf);
+	arch_acpi_set_proc_cap_bits(&buf[2]);
 }
 
 static struct acpi_object_list *acpi_processor_alloc_pdc(void)
-- 
2.41.0

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

[Michal Wilczynski]

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

[PATCH v3 4/5] acpi: Use _OSC method to convey processor OSPM capabilities

[Michal Wilczynski]

Change acpi_early_processor_osc() to return value in case of the failure.
Make it more generic - previously it served only to execute workaround
for buggy BIOS in Skylake systems. Now it will walk through ACPI
namespace looking for processor objects and will convey OSPM processor
capabilities using _OSC method.

Prefer using _OSC method over deprecated _PDC in the acpi_bus_init(). In
case of the failure of the _OSC, try using  _PDC as a fallback.

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>
---
 drivers/acpi/acpi_processor.c | 23 +++++++++++++----------
 drivers/acpi/bus.c            | 13 +++++++++----
 drivers/acpi/internal.h       |  9 +--------
 3 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index 0de0b05b6f53..8965e01406e0 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -669,17 +669,20 @@ static acpi_status __init acpi_hwp_native_thermal_lvt_osc(acpi_handle handle,
 	return AE_OK;
 }
 
-void __init acpi_early_processor_osc(void)
+acpi_status __init acpi_early_processor_osc(void)
 {
-	if (boot_cpu_has(X86_FEATURE_HWP)) {
-		acpi_walk_namespace(ACPI_TYPE_PROCESSOR, ACPI_ROOT_OBJECT,
-				    ACPI_UINT32_MAX,
-				    acpi_hwp_native_thermal_lvt_osc,
-				    NULL, NULL, NULL);
-		acpi_get_devices(ACPI_PROCESSOR_DEVICE_HID,
-				 acpi_hwp_native_thermal_lvt_osc,
-				 NULL, NULL);
-	}
+	acpi_status status;
+
+	processor_dmi_check();
+
+	status = acpi_walk_namespace(ACPI_TYPE_PROCESSOR, ACPI_ROOT_OBJECT,
+				     ACPI_UINT32_MAX, acpi_processor_osc, NULL,
+				     NULL, NULL);
+	if (ACPI_FAILURE(status))
+		return status;
+
+	return acpi_get_devices(ACPI_PROCESSOR_DEVICE_HID, acpi_processor_osc,
+				NULL, NULL);
 }
 #endif
 
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index d161ff707de4..e8d1f645224f 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -1317,9 +1317,6 @@ static int __init acpi_bus_init(void)
 		goto error1;
 	}
 
-	/* Set capability bits for _OSC under processor scope */
-	acpi_early_processor_osc();
-
 	/*
 	 * _OSC method may exist in module level code,
 	 * so it must be run after ACPI_FULL_INITIALIZATION
@@ -1335,7 +1332,15 @@ static int __init acpi_bus_init(void)
 
 	acpi_sysfs_init();
 
-	acpi_early_processor_set_pdc();
+#ifdef CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC
+	status = acpi_early_processor_osc();
+	if (ACPI_FAILURE(status)) {
+		pr_err("_OSC methods failed, trying _PDC\n");
+		acpi_early_processor_set_pdc();
+	} else {
+		pr_info("_OSC methods ran successfully\n");
+	}
+#endif
 
 	/*
 	 * Maybe EC region is required at bus_scan/acpi_get_devices. So it
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index f979a2f7077c..e7cc41313997 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -151,17 +151,10 @@ int acpi_wakeup_device_init(void);
    -------------------------------------------------------------------------- */
 #ifdef CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC
 void acpi_early_processor_set_pdc(void);
+acpi_status acpi_early_processor_osc(void);
 
 void processor_dmi_check(void);
 bool processor_physically_present(acpi_handle handle);
-#else
-static inline void acpi_early_processor_set_pdc(void) {}
-#endif
-
-#ifdef CONFIG_X86
-void acpi_early_processor_osc(void);
-#else
-static inline void acpi_early_processor_osc(void) {}
 #endif
 
 /* --------------------------------------------------------------------------
-- 
2.41.0

[PATCH v3 5/5] acpi: Remove acpi_hwp_native_thermal_lvt_osc()

[Michal Wilczynski]

Workaround for buggy skylake BIOS is implemented in acpi_processor_osc()
and acpi_hwp_native_thermal_lvt_osc() function is not called anywhere.
Remove it.

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>
---
 drivers/acpi/acpi_processor.c | 35 -----------------------------------
 1 file changed, 35 deletions(-)

diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index 8965e01406e0..1e305ad9540e 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -634,41 +634,6 @@ static acpi_status __init acpi_processor_osc(acpi_handle handle, u32 lvl,
 	return AE_OK;
 }
 
-static acpi_status __init acpi_hwp_native_thermal_lvt_osc(acpi_handle handle,
-							  u32 lvl,
-							  void *context,
-							  void **rv)
-{
-	u32 capbuf[2];
-	struct acpi_osc_context osc_context = {
-		.uuid_str = sb_uuid_str,
-		.rev = 1,
-		.cap.length = 8,
-		.cap.pointer = capbuf,
-	};
-
-	if (acpi_hwp_native_thermal_lvt_set)
-		return AE_CTRL_TERMINATE;
-
-	capbuf[0] = 0x0000;
-	capbuf[1] = 0x1000; /* set bit 12 */
-
-	if (ACPI_SUCCESS(acpi_run_osc(handle, &osc_context))) {
-		if (osc_context.ret.pointer && osc_context.ret.length > 1) {
-			u32 *capbuf_ret = osc_context.ret.pointer;
-
-			if (capbuf_ret[1] & 0x1000) {
-				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;
-}
-
 acpi_status __init acpi_early_processor_osc(void)
 {
 	acpi_status status;
-- 
2.41.0

Re: [PATCH v3 2/5] acpi: Refactor arch_acpi_set_pdc_bits()

[Rafael J. Wysocki]

On Tue, Jun 13, 2023 at 6:12 PM Michal Wilczynski
<michal.wilczynski@intel.com> wrote:
>
> Capabilities buffer modified by the arch_acpi_set_pdc_bits() is not
> _PDC specific, as it is used by _OSC method as well. Change function
> name to better reflect it's independence from PDC. Change function
> expected argument to pass capability buffer directly without any
> offset, as the offset differ among _OSC and _PDC methods.
>
> 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>

Any objections to this from the x86 arch side?  if not, I will route
it via ACPI along with the rest of the series.

> ---
>  arch/ia64/include/asm/acpi.h |  4 ++--
>  arch/x86/include/asm/acpi.h  | 10 +++++-----
>  drivers/acpi/processor_pdc.c |  2 +-
>  3 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/arch/ia64/include/asm/acpi.h b/arch/ia64/include/asm/acpi.h
> index 87927eb824cc..43797cb44383 100644
> --- a/arch/ia64/include/asm/acpi.h
> +++ b/arch/ia64/include/asm/acpi.h
> @@ -69,9 +69,9 @@ extern int __initdata nid_to_pxm_map[MAX_NUMNODES];
>  #endif
>
>  static inline bool arch_has_acpi_pdc(void) { return true; }
> -static inline void arch_acpi_set_pdc_bits(u32 *buf)
> +static inline void arch_acpi_set_proc_cap_bits(u32 *cap)
>  {
> -       buf[2] |= ACPI_PDC_EST_CAPABILITY_SMP;
> +       *cap |= ACPI_PDC_EST_CAPABILITY_SMP;
>  }
>
>  #ifdef CONFIG_ACPI_NUMA
> diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
> index 8eb74cf386db..6a498d1781e7 100644
> --- a/arch/x86/include/asm/acpi.h
> +++ b/arch/x86/include/asm/acpi.h
> @@ -100,23 +100,23 @@ static inline bool arch_has_acpi_pdc(void)
>                 c->x86_vendor == X86_VENDOR_CENTAUR);
>  }
>
> -static inline void arch_acpi_set_pdc_bits(u32 *buf)
> +static inline void arch_acpi_set_proc_cap_bits(u32 *cap)
>  {
>         struct cpuinfo_x86 *c = &cpu_data(0);
>
> -       buf[2] |= ACPI_PDC_C_CAPABILITY_SMP;
> +       *cap |= ACPI_PDC_C_CAPABILITY_SMP;
>
>         if (cpu_has(c, X86_FEATURE_EST))
> -               buf[2] |= ACPI_PDC_EST_CAPABILITY_SWSMP;
> +               *cap |= ACPI_PDC_EST_CAPABILITY_SWSMP;
>
>         if (cpu_has(c, X86_FEATURE_ACPI))
> -               buf[2] |= ACPI_PDC_T_FFH;
> +               *cap |= ACPI_PDC_T_FFH;
>
>         /*
>          * If mwait/monitor is unsupported, C2/C3_FFH will be disabled
>          */
>         if (!cpu_has(c, X86_FEATURE_MWAIT))
> -               buf[2] &= ~(ACPI_PDC_C_C2C3_FFH);
> +               *cap &= ~(ACPI_PDC_C_C2C3_FFH);
>  }
>
>  static inline bool acpi_has_cpu_in_madt(void)
> diff --git a/drivers/acpi/processor_pdc.c b/drivers/acpi/processor_pdc.c
> index 5596862e6fea..ce3acd86dd12 100644
> --- a/drivers/acpi/processor_pdc.c
> +++ b/drivers/acpi/processor_pdc.c
> @@ -24,7 +24,7 @@ static void acpi_set_pdc_bits(u32 *buf)
>         buf[2] = ACPI_PDC_SMP_T_SWCOORD;
>
>         /* Twiddle arch-specific bits needed for _PDC */
> -       arch_acpi_set_pdc_bits(buf);
> +       arch_acpi_set_proc_cap_bits(&buf[2]);
>  }
>
>  static struct acpi_object_list *acpi_processor_alloc_pdc(void)
> --
> 2.41.0
>

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

[Rafael J. Wysocki]

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

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

[Rafael J. Wysocki]

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

Re: [PATCH v3 4/5] acpi: Use _OSC method to convey processor OSPM capabilities

[Rafael J. Wysocki]

On Tue, Jun 13, 2023 at 6:12 PM Michal Wilczynski
<michal.wilczynski@intel.com> wrote:
>
> Change acpi_early_processor_osc() to return value in case of the failure.
> Make it more generic - previously it served only to execute workaround
> for buggy BIOS in Skylake systems. Now it will walk through ACPI
> namespace looking for processor objects and will convey OSPM processor
> capabilities using _OSC method.
>
> Prefer using _OSC method over deprecated _PDC in the acpi_bus_init(). In
> case of the failure of the _OSC, try using  _PDC as a fallback.
>
> 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>
> ---
>  drivers/acpi/acpi_processor.c | 23 +++++++++++++----------
>  drivers/acpi/bus.c            | 13 +++++++++----
>  drivers/acpi/internal.h       |  9 +--------
>  3 files changed, 23 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> index 0de0b05b6f53..8965e01406e0 100644
> --- a/drivers/acpi/acpi_processor.c
> +++ b/drivers/acpi/acpi_processor.c
> @@ -669,17 +669,20 @@ static acpi_status __init acpi_hwp_native_thermal_lvt_osc(acpi_handle handle,
>         return AE_OK;
>  }
>
> -void __init acpi_early_processor_osc(void)

I would rename this to something like
acpi_early_processor_control_setup() and would make it attempt to call
_PDC if _OSC doesn't work.

Then it could remain void and it could be put under a
CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC #ifdef.

> +acpi_status __init acpi_early_processor_osc(void)
>  {
> -       if (boot_cpu_has(X86_FEATURE_HWP)) {
> -               acpi_walk_namespace(ACPI_TYPE_PROCESSOR, ACPI_ROOT_OBJECT,
> -                                   ACPI_UINT32_MAX,
> -                                   acpi_hwp_native_thermal_lvt_osc,
> -                                   NULL, NULL, NULL);
> -               acpi_get_devices(ACPI_PROCESSOR_DEVICE_HID,
> -                                acpi_hwp_native_thermal_lvt_osc,
> -                                NULL, NULL);
> -       }
> +       acpi_status status;
> +
> +       processor_dmi_check();
> +
> +       status = acpi_walk_namespace(ACPI_TYPE_PROCESSOR, ACPI_ROOT_OBJECT,
> +                                    ACPI_UINT32_MAX, acpi_processor_osc, NULL,
> +                                    NULL, NULL);
> +       if (ACPI_FAILURE(status))
> +               return status;
> +
> +       return acpi_get_devices(ACPI_PROCESSOR_DEVICE_HID, acpi_processor_osc,
> +                               NULL, NULL);
>  }
>  #endif
>
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index d161ff707de4..e8d1f645224f 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -1317,9 +1317,6 @@ static int __init acpi_bus_init(void)
>                 goto error1;
>         }
>
> -       /* Set capability bits for _OSC under processor scope */
> -       acpi_early_processor_osc();
> -
>         /*
>          * _OSC method may exist in module level code,
>          * so it must be run after ACPI_FULL_INITIALIZATION
> @@ -1335,7 +1332,15 @@ static int __init acpi_bus_init(void)
>
>         acpi_sysfs_init();
>
> -       acpi_early_processor_set_pdc();
> +#ifdef CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC
> +       status = acpi_early_processor_osc();
> +       if (ACPI_FAILURE(status)) {
> +               pr_err("_OSC methods failed, trying _PDC\n");
> +               acpi_early_processor_set_pdc();
> +       } else {
> +               pr_info("_OSC methods ran successfully\n");
> +       }
> +#endif
>
>         /*
>          * Maybe EC region is required at bus_scan/acpi_get_devices. So it
> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index f979a2f7077c..e7cc41313997 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -151,17 +151,10 @@ int acpi_wakeup_device_init(void);
>     -------------------------------------------------------------------------- */
>  #ifdef CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC
>  void acpi_early_processor_set_pdc(void);
> +acpi_status acpi_early_processor_osc(void);
>
>  void processor_dmi_check(void);
>  bool processor_physically_present(acpi_handle handle);
> -#else
> -static inline void acpi_early_processor_set_pdc(void) {}
> -#endif
> -
> -#ifdef CONFIG_X86
> -void acpi_early_processor_osc(void);
> -#else
> -static inline void acpi_early_processor_osc(void) {}
>  #endif
>
>  /* --------------------------------------------------------------------------
> --

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

[Wilczynski, Michal]

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

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

[Wilczynski, Michal]

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

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

[Rafael J. Wysocki]

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.

Re: [PATCH v3 4/5] acpi: Use _OSC method to convey processor OSPM capabilities

[Wilczynski, Michal]

On 6/29/2023 4:23 PM, Rafael J. Wysocki wrote:
> On Tue, Jun 13, 2023 at 6:12 PM Michal Wilczynski
> <michal.wilczynski@intel.com> wrote:
>> Change acpi_early_processor_osc() to return value in case of the failure.
>> Make it more generic - previously it served only to execute workaround
>> for buggy BIOS in Skylake systems. Now it will walk through ACPI
>> namespace looking for processor objects and will convey OSPM processor
>> capabilities using _OSC method.
>>
>> Prefer using _OSC method over deprecated _PDC in the acpi_bus_init(). In
>> case of the failure of the _OSC, try using  _PDC as a fallback.
>>
>> 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>
>> ---
>>  drivers/acpi/acpi_processor.c | 23 +++++++++++++----------
>>  drivers/acpi/bus.c            | 13 +++++++++----
>>  drivers/acpi/internal.h       |  9 +--------
>>  3 files changed, 23 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
>> index 0de0b05b6f53..8965e01406e0 100644
>> --- a/drivers/acpi/acpi_processor.c
>> +++ b/drivers/acpi/acpi_processor.c
>> @@ -669,17 +669,20 @@ static acpi_status __init acpi_hwp_native_thermal_lvt_osc(acpi_handle handle,
>>         return AE_OK;
>>  }
>>
>> -void __init acpi_early_processor_osc(void)
> I would rename this to something like
> acpi_early_processor_control_setup() and would make it attempt to call
> _PDC if _OSC doesn't work.
>
> Then it could remain void and it could be put under a
> CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC #ifdef.

Sure, makes sense to me

>
>> +acpi_status __init acpi_early_processor_osc(void)
>>  {
>> -       if (boot_cpu_has(X86_FEATURE_HWP)) {
>> -               acpi_walk_namespace(ACPI_TYPE_PROCESSOR, ACPI_ROOT_OBJECT,
>> -                                   ACPI_UINT32_MAX,
>> -                                   acpi_hwp_native_thermal_lvt_osc,
>> -                                   NULL, NULL, NULL);
>> -               acpi_get_devices(ACPI_PROCESSOR_DEVICE_HID,
>> -                                acpi_hwp_native_thermal_lvt_osc,
>> -                                NULL, NULL);
>> -       }
>> +       acpi_status status;
>> +
>> +       processor_dmi_check();
>> +
>> +       status = acpi_walk_namespace(ACPI_TYPE_PROCESSOR, ACPI_ROOT_OBJECT,
>> +                                    ACPI_UINT32_MAX, acpi_processor_osc, NULL,
>> +                                    NULL, NULL);
>> +       if (ACPI_FAILURE(status))
>> +               return status;
>> +
>> +       return acpi_get_devices(ACPI_PROCESSOR_DEVICE_HID, acpi_processor_osc,
>> +                               NULL, NULL);
>>  }
>>  #endif
>>
>> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
>> index d161ff707de4..e8d1f645224f 100644
>> --- a/drivers/acpi/bus.c
>> +++ b/drivers/acpi/bus.c
>> @@ -1317,9 +1317,6 @@ static int __init acpi_bus_init(void)
>>                 goto error1;
>>         }
>>
>> -       /* Set capability bits for _OSC under processor scope */
>> -       acpi_early_processor_osc();
>> -
>>         /*
>>          * _OSC method may exist in module level code,
>>          * so it must be run after ACPI_FULL_INITIALIZATION
>> @@ -1335,7 +1332,15 @@ static int __init acpi_bus_init(void)
>>
>>         acpi_sysfs_init();
>>
>> -       acpi_early_processor_set_pdc();
>> +#ifdef CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC
>> +       status = acpi_early_processor_osc();
>> +       if (ACPI_FAILURE(status)) {
>> +               pr_err("_OSC methods failed, trying _PDC\n");
>> +               acpi_early_processor_set_pdc();
>> +       } else {
>> +               pr_info("_OSC methods ran successfully\n");
>> +       }
>> +#endif
>>
>>         /*
>>          * Maybe EC region is required at bus_scan/acpi_get_devices. So it
>> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
>> index f979a2f7077c..e7cc41313997 100644
>> --- a/drivers/acpi/internal.h
>> +++ b/drivers/acpi/internal.h
>> @@ -151,17 +151,10 @@ int acpi_wakeup_device_init(void);
>>     -------------------------------------------------------------------------- */
>>  #ifdef CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC
>>  void acpi_early_processor_set_pdc(void);
>> +acpi_status acpi_early_processor_osc(void);
>>
>>  void processor_dmi_check(void);
>>  bool processor_physically_present(acpi_handle handle);
>> -#else
>> -static inline void acpi_early_processor_set_pdc(void) {}
>> -#endif
>> -
>> -#ifdef CONFIG_X86
>> -void acpi_early_processor_osc(void);
>> -#else
>> -static inline void acpi_early_processor_osc(void) {}
>>  #endif
>>
>>  /* --------------------------------------------------------------------------
>> --

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

[Wilczynski, Michal]

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

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

[Wilczynski, Michal]

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

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

[Wilczynski, Michal]

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ł

>
>

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

[Rafael J. Wysocki]

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.

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

[Rafael J. Wysocki]

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.

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

[Wilczynski, Michal]

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ł