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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4de5c2da-5bdd-68fe-fa80-e96f778f7fd2@intel.com>
Date:   Fri, 30 Jun 2023 11:18:37 +0200
From:   "Wilczynski, Michal" <michal.wilczynski@...el.com>
To:     "Rafael J. Wysocki" <rafael@...nel.org>
CC:     <linux-acpi@...r.kernel.org>, <andriy.shevchenko@...el.com>,
        <artem.bityutskiy@...ux.intel.com>, <mingo@...hat.com>,
        <bp@...en8.de>, <dave.hansen@...ux.intel.com>, <hpa@...or.com>,
        <lenb@...nel.org>, <jgross@...e.com>,
        <linux-kernel@...r.kernel.org>, <x86@...nel.org>
Subject: Re: [PATCH v3 4/5] acpi: Use _OSC method to convey processor OSPM
 capabilities



On 6/29/2023 4:23 PM, Rafael J. Wysocki wrote:
> On Tue, Jun 13, 2023 at 6:12 PM Michal Wilczynski
> <michal.wilczynski@...el.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@...el.com>
>> Signed-off-by: Michal Wilczynski <michal.wilczynski@...el.com>
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@...ux.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
>>
>>  /* --------------------------------------------------------------------------
>> --