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 <[email protected]>
---
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/[email protected]/
[2] https://lore.kernel.org/lkml/[email protected]/
[3] https://lore.kernel.org/lkml/[email protected]/
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);
--
2.35.3
On Thu, Mar 16, 2023 at 4:10 PM Petr Pavlu <[email protected]> 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 <[email protected]>
> ---
>
> 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/[email protected]/
> [2] https://lore.kernel.org/lkml/[email protected]/
> [3] https://lore.kernel.org/lkml/[email protected]/
>
> 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!
On Thu, 2023-03-16 at 16:10 +0100, Petr Pavlu wrote:
> 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.
As a result of this patch (691a637123470bfe63bccf5836ead40fac4c7fab)
my ThinkPad T430 with an i5-3320M CPU configured with
CONFIG_X86_INTEL_PSTATE=y and CONFIG_X86_ACPI_CPUFREQ=m (Debian's
amd64 kernel config) now logs
kernel: acpi-cpufreq: probe of acpi-cpufreq failed with error -17
during boot. Presumably this occurs because loading acpi-cpufreq
returns -EEXIST when intel-pstate is already loaded (or built-in, as
in this case). I'm unsure why the message was not printed before;
perhaps a difference between driver probing for platform and cpu bus
types? Although the error message is not wrong, it may lead to
unnecessary investigation by sysadmins, as it did for me. I thought
it was worth reporting so you can consider whether the change is
desirable.
Cheers,
Kevin
On 5/10/23 02:00, Kevin Locke wrote:
> On Thu, 2023-03-16 at 16:10 +0100, Petr Pavlu wrote:
>> 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.
>
> As a result of this patch (691a637123470bfe63bccf5836ead40fac4c7fab)
> my ThinkPad T430 with an i5-3320M CPU configured with
> CONFIG_X86_INTEL_PSTATE=y and CONFIG_X86_ACPI_CPUFREQ=m (Debian's
> amd64 kernel config) now logs
>
> kernel: acpi-cpufreq: probe of acpi-cpufreq failed with error -17
>
> during boot. Presumably this occurs because loading acpi-cpufreq
> returns -EEXIST when intel-pstate is already loaded (or built-in, as
> in this case). I'm unsure why the message was not printed before;
> perhaps a difference between driver probing for platform and cpu bus
> types? Although the error message is not wrong, it may lead to
> unnecessary investigation by sysadmins, as it did for me. I thought
> it was worth reporting so you can consider whether the change is
> desirable.
Thanks for reporting this issue. The patch moved the setup of
acpi-cpufreq from being done directly in its module init function to
going through the probe logic. The reported warning newly comes from
call_driver_probe() when the probe fails.
One immediate option that I can see to silence this warning would be to
change the return code for this case in acpi_cpufreq_probe() from
-EEXIST to -ENODEV/ENXIO. Function call_driver_probe() then prints only
a debug message about the probe rejecting the device.
-- Petr
On Wed, May 10, 2023 at 10:48 AM Petr Pavlu <[email protected]> wrote:
>
> On 5/10/23 02:00, Kevin Locke wrote:
> > On Thu, 2023-03-16 at 16:10 +0100, Petr Pavlu wrote:
> >> 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.
> >
> > As a result of this patch (691a637123470bfe63bccf5836ead40fac4c7fab)
> > my ThinkPad T430 with an i5-3320M CPU configured with
> > CONFIG_X86_INTEL_PSTATE=y and CONFIG_X86_ACPI_CPUFREQ=m (Debian's
> > amd64 kernel config) now logs
> >
> > kernel: acpi-cpufreq: probe of acpi-cpufreq failed with error -17
> >
> > during boot. Presumably this occurs because loading acpi-cpufreq
> > returns -EEXIST when intel-pstate is already loaded (or built-in, as
> > in this case). I'm unsure why the message was not printed before;
> > perhaps a difference between driver probing for platform and cpu bus
> > types? Although the error message is not wrong, it may lead to
> > unnecessary investigation by sysadmins, as it did for me. I thought
> > it was worth reporting so you can consider whether the change is
> > desirable.
>
> Thanks for reporting this issue. The patch moved the setup of
> acpi-cpufreq from being done directly in its module init function to
> going through the probe logic. The reported warning newly comes from
> call_driver_probe() when the probe fails.
>
> One immediate option that I can see to silence this warning would be to
> change the return code for this case in acpi_cpufreq_probe() from
> -EEXIST to -ENODEV/ENXIO. Function call_driver_probe() then prints only
> a debug message about the probe rejecting the device.
-ENODEV, pretty please. Or I can send a patch, whichever you prefer.
It would be a better error code for this condition anyway IMV.