diff mbox series

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

Message ID 20230613161034.3496047-5-michal.wilczynski@intel.com (mailing list archive)
State Superseded, archived
Headers show
Series Prefer using _OSC method over deprecated _PDC | expand

Commit Message

Wilczynski, Michal June 13, 2023, 4:10 p.m. UTC 
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(-)

Comments

Rafael J. Wysocki June 29, 2023, 2:23 p.m. UTC | #1 
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
>
>  /* --------------------------------------------------------------------------
> --
Wilczynski, Michal June 30, 2023, 9:18 a.m. UTC | #2 
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
>>
>>  /* --------------------------------------------------------------------------
>> --
diff mbox series

Patch

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
 
 /* --------------------------------------------------------------------------