Re: [PATCH] ACPI: cpufreq: use a platform device to load ACPI PPC and PCC drivers

[Rafael J. Wysocki]

On Tue, Jan 31, 2023 at 2:01 PM Petr Pavlu <petr.pavlu@xxxxxxxx> wrote:
>
> The acpi-cpufreq and pcc-cpufreq drivers are loaded through per-CPU
> module aliases. This can result in many unnecessary load requests during
> boot if another frequency module, such as intel_pstate, is already
> active. For instance, on a typical Intel system, one can observe that
> udev makes 2x#CPUs attempts to insert acpi_cpufreq and 1x#CPUs attempts
> for pcc_cpufreq. All these tries then fail if another frequency module
> is already registered.

Right, which is unnecessary overhead.

> Both acpi-cpufreq and pcc-cpufreq drivers have their platform firmware
> interface defined by ACPI. Allowed performance states and parameters
> must be same for each CPU.

This is an assumption made by the code in those drivers, the
specification doesn't actually require this IIRC.

> This makes it possible to model these
> interfaces as platform devices.
>
> The patch extends the ACPI parsing logic to check the ACPI namespace if
> the PPC or PCC interface is present and creates a virtual platform
> device for each if it is available. The acpi-cpufreq and pcc-cpufreq
> drivers are then updated to map to these devices.
>
> This allows to try loading acpi-cpufreq and pcc-cpufreq only once during
> boot and only if a given interface is available in the firmware.

This is clever, but will it always work?

> Signed-off-by: Petr Pavlu <petr.pavlu@xxxxxxxx>
> ---
>  drivers/acpi/Makefile          |  1 +
>  drivers/acpi/acpi_cpufreq.c    | 49 ++++++++++++++++++++++++++++++++++
>  drivers/acpi/bus.c             |  1 +
>  drivers/acpi/internal.h        |  2 ++
>  drivers/cpufreq/acpi-cpufreq.c | 39 +++++++++++++++------------
>  drivers/cpufreq/pcc-cpufreq.c  | 34 ++++++++++++++++-------
>  6 files changed, 99 insertions(+), 27 deletions(-)
>  create mode 100644 drivers/acpi/acpi_cpufreq.c
>
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index feb36c0b9446..880db1082c3e 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -57,6 +57,7 @@ acpi-y                                += evged.o
>  acpi-y                         += sysfs.o
>  acpi-y                         += property.o
>  acpi-$(CONFIG_X86)             += acpi_cmos_rtc.o
> +acpi-$(CONFIG_X86)             += acpi_cpufreq.o
>  acpi-$(CONFIG_X86)             += x86/apple.o
>  acpi-$(CONFIG_X86)             += x86/utils.o
>  acpi-$(CONFIG_X86)             += x86/s2idle.o
> diff --git a/drivers/acpi/acpi_cpufreq.c b/drivers/acpi/acpi_cpufreq.c
> new file mode 100644
> index 000000000000..7cf243c67475
> --- /dev/null
> +++ b/drivers/acpi/acpi_cpufreq.c
> @@ -0,0 +1,49 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Registration of platform devices for ACPI Processor Performance Control and
> + * Processor Clocking Control.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/platform_device.h>
> +
> +#include "internal.h"
> +
> +static void __init cpufreq_add_device(const char *name)
> +{
> +       struct platform_device *pdev;
> +
> +       pdev = platform_device_register_simple(name, PLATFORM_DEVID_NONE, NULL,
> +                                              0);
> +       if (IS_ERR(pdev))
> +               pr_err("%s device creation failed: %ld\n", name, PTR_ERR(pdev));
> +}
> +
> +static acpi_status __init acpi_pct_match(acpi_handle handle, u32 level,
> +                                        void *context, void **return_value)
> +{
> +       bool *pct = context;
> +
> +       /* Check if the first CPU has _PCT. The data must be same for all. */
> +       *pct = acpi_has_method(handle, "_PCT");
> +       return AE_CTRL_TERMINATE;
> +}
> +
> +void __init acpi_cpufreq_init(void)
> +{
> +       acpi_status status;
> +       acpi_handle handle;
> +       bool pct = false;
> +
> +       status = acpi_get_handle(NULL, "\\_SB", &handle);
> +       if (ACPI_FAILURE(status))
> +               return;
> +
> +       acpi_walk_namespace(ACPI_TYPE_PROCESSOR, ACPI_ROOT_OBJECT,
> +                           ACPI_UINT32_MAX, acpi_pct_match, NULL, &pct, NULL);

Well, one more full ACPI Namespace walk at init time.

Also, this only covers processor objects and what about processor
device objects?

> +       if (pct)
> +               cpufreq_add_device("acpi-cpufreq");
> +
> +       if (acpi_has_method(handle, "PCCH"))
> +               cpufreq_add_device("pcc-cpufreq");
> +}
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 0c05ccde1f7a..f1559e26d5ff 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -1428,6 +1428,7 @@ static int __init acpi_init(void)
>         acpi_viot_init();
>         acpi_agdi_init();
>         acpi_apmt_init();
> +       acpi_cpufreq_init();
>         return 0;
>  }
>
> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index ec584442fb29..c9b1a5f689fa 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -157,8 +157,10 @@ static inline void acpi_early_processor_set_pdc(void) {}
>
>  #ifdef CONFIG_X86
>  void acpi_early_processor_osc(void);
> +void acpi_cpufreq_init(void);
>  #else
>  static inline void acpi_early_processor_osc(void) {}
> +static inline void acpi_cpufreq_init(void) {}
>  #endif
>
>  /* --------------------------------------------------------------------------
> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> index 78adfb2ffff6..e1a5384cf21c 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -965,7 +965,7 @@ static void __init acpi_cpufreq_boost_init(void)
>         acpi_cpufreq_driver.boost_enabled = boost_state(0);
>  }
>
> -static int __init acpi_cpufreq_init(void)
> +static int __init acpi_cpufreq_probe(struct platform_device *pdev)
>  {
>         int ret;
>
> @@ -1010,13 +1010,32 @@ static int __init acpi_cpufreq_init(void)
>         return ret;
>  }
>
> -static void __exit acpi_cpufreq_exit(void)
> +static int acpi_cpufreq_remove(struct platform_device *pdev)
>  {
>         pr_debug("%s\n", __func__);
>
>         cpufreq_unregister_driver(&acpi_cpufreq_driver);
>
>         free_acpi_perf_data();
> +
> +       return 0;
> +}
> +
> +static struct platform_driver acpi_cpufreq_platdrv = {
> +       .driver = {
> +               .name   = "acpi-cpufreq",
> +       },
> +       .remove         = acpi_cpufreq_remove,
> +};
> +
> +static int __init acpi_cpufreq_init(void)
> +{
> +       return platform_driver_probe(&acpi_cpufreq_platdrv, acpi_cpufreq_probe);
> +}
> +
> +static void __exit acpi_cpufreq_exit(void)
> +{
> +       platform_driver_unregister(&acpi_cpufreq_platdrv);
>  }
>
>  module_param(acpi_pstate_strict, uint, 0644);
> @@ -1027,18 +1046,4 @@ MODULE_PARM_DESC(acpi_pstate_strict,
>  late_initcall(acpi_cpufreq_init);
>  module_exit(acpi_cpufreq_exit);
>
> -static const struct x86_cpu_id __maybe_unused acpi_cpufreq_ids[] = {
> -       X86_MATCH_FEATURE(X86_FEATURE_ACPI, NULL),
> -       X86_MATCH_FEATURE(X86_FEATURE_HW_PSTATE, NULL),
> -       {}
> -};
> -MODULE_DEVICE_TABLE(x86cpu, acpi_cpufreq_ids);
> -
> -static const struct acpi_device_id __maybe_unused processor_device_ids[] = {
> -       {ACPI_PROCESSOR_OBJECT_HID, },
> -       {ACPI_PROCESSOR_DEVICE_HID, },
> -       {},
> -};
> -MODULE_DEVICE_TABLE(acpi, processor_device_ids);
> -
> -MODULE_ALIAS("acpi");
> +MODULE_ALIAS("platform:acpi-cpufreq");
> diff --git a/drivers/cpufreq/pcc-cpufreq.c b/drivers/cpufreq/pcc-cpufreq.c
> index 9f3fc7a073d0..0c362932ca60 100644
> --- a/drivers/cpufreq/pcc-cpufreq.c
> +++ b/drivers/cpufreq/pcc-cpufreq.c
> @@ -384,7 +384,7 @@ static int __init pcc_cpufreq_do_osc(acpi_handle *handle)
>         return ret;
>  }
>
> -static int __init pcc_cpufreq_probe(void)
> +static int __init pcc_cpufreq_evaluate(void)
>  {
>         acpi_status status;
>         struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
> @@ -576,7 +576,7 @@ static struct cpufreq_driver pcc_cpufreq_driver = {
>         .name = "pcc-cpufreq",
>  };
>
> -static int __init pcc_cpufreq_init(void)
> +static int __init pcc_cpufreq_probe(struct platform_device *pdev)
>  {
>         int ret;
>
> @@ -587,9 +587,9 @@ static int __init pcc_cpufreq_init(void)
>         if (acpi_disabled)
>                 return -ENODEV;
>
> -       ret = pcc_cpufreq_probe();
> +       ret = pcc_cpufreq_evaluate();
>         if (ret) {
> -               pr_debug("pcc_cpufreq_init: PCCH evaluation failed\n");
> +               pr_debug("pcc_cpufreq_probe: PCCH evaluation failed\n");
>                 return ret;
>         }
>
> @@ -607,21 +607,35 @@ static int __init pcc_cpufreq_init(void)
>         return ret;
>  }
>
> -static void __exit pcc_cpufreq_exit(void)
> +static int pcc_cpufreq_remove(struct platform_device *pdev)
>  {
>         cpufreq_unregister_driver(&pcc_cpufreq_driver);
>
>         pcc_clear_mapping();
>
>         free_percpu(pcc_cpu_info);
> +
> +       return 0;
>  }
>
> -static const struct acpi_device_id __maybe_unused processor_device_ids[] = {
> -       {ACPI_PROCESSOR_OBJECT_HID, },
> -       {ACPI_PROCESSOR_DEVICE_HID, },
> -       {},
> +static struct platform_driver pcc_cpufreq_platdrv = {
> +       .driver = {
> +               .name   = "pcc-cpufreq",
> +       },
> +       .remove         = pcc_cpufreq_remove,
>  };
> -MODULE_DEVICE_TABLE(acpi, processor_device_ids);
> +
> +static int __init pcc_cpufreq_init(void)
> +{
> +       return platform_driver_probe(&pcc_cpufreq_platdrv, pcc_cpufreq_probe);
> +}
> +
> +static void __exit pcc_cpufreq_exit(void)
> +{
> +       platform_driver_unregister(&pcc_cpufreq_platdrv);
> +}
> +
> +MODULE_ALIAS("platform:pcc-cpufreq");
>
>  MODULE_AUTHOR("Matthew Garrett, Naga Chumbalkar");
>  MODULE_VERSION(PCC_VERSION);
> --