On Thu, Mar 16, 2023 at 4:10 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.
>
> In the worst case, without the recent fix in commit 0254127ab977e
> ("module: Don't wait for GOING modules"), these module loads occupied
> all udev workers and had their initialization attempts ran sequentially.
> Resolving all these loads then on some larger machines took too long,
> prevented other hardware from getting its drivers initialized and
> resulted in a failed boot. Discussion over these duplicate module
> requests ended up with a conclusion that only one load attempt should be
> ideally made.
>
> Both acpi-cpufreq and pcc-cpufreq drivers use platform firmware controls
> which are defined by ACPI. It is possible to treat 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.
>
> Signed-off-by: Petr Pavlu <petr.pavlu@xxxxxxxx>
> ---
>
> Changes since v3 [1]:
> - Mark acpi_pcc_cpufreq_init() as static.
>
> Changes since v2 [2]:
> - Remove mention about all CPUs needing same clocking parameters from the commit
> message.
> - Integrate the new logic to detect presence of PPC and PCC with the current
> ACPI processor code which avoids an additional scan of the ACPI namespace.
>
> Changes since v1 [3]:
> - Describe the worst case scenario without the recent fix 0254127ab977e
> ("module: Don't wait for GOING modules") and refer to its discussion
> in the commit message.
> - Consider ACPI processor device objects when looking for _PCT, in
> addition to processor objects.
> - Add a few more comments explaining the code.
>
> [1] https://lore.kernel.org/lkml/20230301085717.10411-1-petr.pavlu@xxxxxxxx/
> [2] https://lore.kernel.org/lkml/20230220143143.3492-1-petr.pavlu@xxxxxxxx/
> [3] https://lore.kernel.org/lkml/20230131130041.629-1-petr.pavlu@xxxxxxxx/
>
> drivers/acpi/acpi_processor.c | 42 ++++++++++++++++++++++++++++++++--
> drivers/cpufreq/acpi-cpufreq.c | 39 +++++++++++++++++--------------
> drivers/cpufreq/pcc-cpufreq.c | 34 +++++++++++++++++++--------
> 3 files changed, 86 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> index 6737b1cbf6d6..9dbd8d855009 100644
> --- a/drivers/acpi/acpi_processor.c
> +++ b/drivers/acpi/acpi_processor.c
> @@ -15,6 +15,7 @@
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/pci.h>
> +#include <linux/platform_device.h>
>
> #include <acpi/processor.h>
>
> @@ -148,6 +149,34 @@ static int acpi_processor_errata(void)
> return result;
> }
>
> +/* Create a platform device to represent a CPU frequency control mechanism. */
> +static void 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));
> +}
> +
> +#ifdef CONFIG_X86
> +/* Check presence of Processor Clocking Control by searching for \_SB.PCCH. */
> +static void __init acpi_pcc_cpufreq_init(void)
> +{
> + acpi_status status;
> + acpi_handle handle;
> +
> + status = acpi_get_handle(NULL, "\\_SB", &handle);
> + if (ACPI_FAILURE(status))
> + return;
> + if (acpi_has_method(handle, "PCCH"))
> + cpufreq_add_device("pcc-cpufreq");
> +}
> +#else
> +static void __init acpi_pcc_cpufreq_init(void) {}
> +#endif /* CONFIG_X86 */
> +
> /* Initialization */
> #ifdef CONFIG_ACPI_HOTPLUG_CPU
> int __weak acpi_map_cpu(acpi_handle handle,
> @@ -280,14 +309,22 @@ static int acpi_processor_get_info(struct acpi_device *device)
> dev_dbg(&device->dev, "Failed to get CPU physical ID.\n");
>
> pr->id = acpi_map_cpuid(pr->phys_id, pr->acpi_id);
> - if (!cpu0_initialized && !acpi_has_cpu_in_madt()) {
> + if (!cpu0_initialized) {
> cpu0_initialized = 1;
> /*
> * Handle UP system running SMP kernel, with no CPU
> * entry in MADT
> */
> - if (invalid_logical_cpuid(pr->id) && (num_online_cpus() == 1))
> + if (!acpi_has_cpu_in_madt() && invalid_logical_cpuid(pr->id) &&
> + (num_online_cpus() == 1))
> pr->id = 0;
> + /*
> + * Check availability of Processor Performance Control by
> + * looking at the presence of the _PCT object under the first
> + * processor definition.
> + */
> + if (acpi_has_method(pr->handle, "_PCT"))
> + cpufreq_add_device("acpi-cpufreq");
> }
>
> /*
> @@ -686,6 +723,7 @@ void __init acpi_processor_init(void)
> acpi_processor_check_duplicates();
> acpi_scan_add_handler_with_hotplug(&processor_handler, "processor");
> acpi_scan_add_handler(&processor_container_handler);
> + acpi_pcc_cpufreq_init();
> }
>
> #ifdef CONFIG_ACPI_PROCESSOR_CSTATE
> 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);
> --
Applied as 6.4 material under a slightly edited subject and with some
adjustments to the white space and error message log level.
Thanks!
