Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230613161034.3496047-1-michal.wilczynski@intel.com> <20230613161034.3496047-5-michal.wilczynski@intel.com>
In-Reply-To: <20230613161034.3496047-5-michal.wilczynski@intel.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Thu, 29 Jun 2023 16:23:20 +0200
Message-ID: <CAJZ5v0ig9=5wgQbH__q6BJU=p2uryReS_Lmq7s7HdWoDX6eXsQ@mail.gmail.com>
Subject: Re: [PATCH v3 4/5] acpi: Use _OSC method to convey processor OSPM capabilities
To:     Michal Wilczynski <michal.wilczynski@intel.com>
Cc:     linux-acpi@vger.kernel.org, rafael@kernel.org,
        andriy.shevchenko@intel.com, artem.bityutskiy@linux.intel.com,
        mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com,
        hpa@zytor.com, lenb@kernel.org, jgross@suse.com,
        linux-kernel@vger.kernel.org, x86@kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

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