[PATCH 1/2] acpi: cppc: add cpufreq device

[PATCH 1/2] acpi: cppc: add cpufreq device

[Mian Yousaf Kaukab]

From: Mian Yousaf Kaukab <ykaukab@suse.de>

Since commit 28f06f770454 ("cppc_cpufreq: replace per-cpu structures with
lists"), cppc-cpufreq driver doesn't check availability of PSD data before
registering with cpufreq core. As a result on a ThunderX2 platform when
CPPC is disabled from BIOS, kernel log is spammed with following messages:

[  180.974166] CPPC Cpufreq: Error in acquiring _CPC/_PSD data for CPUxx

acpi_cppc_processor_probe() never succeed in this case because
acpi_evaluate_object_typed("_CPC") always returns AE_NOT_FOUND. When
cpufreq core calls cppc_cpufreq_cpu_init(), driver fails to obtain PSD data
and print error messages.

Convert cppc-cpufreq driver to a platform driver (done in a separate patch)
and add cppc-cpufreq device when acpi_cppc_processor_probe() succeeds.

Fixes: 28f06f770454 ("cppc_cpufreq: replace per-cpu structures with lists")
Reported-by: Petr Cervinka <pcervinka@suse.com>
Signed-off-by: Mian Yousaf Kaukab <ykaukab@suse.de>
---
 drivers/acpi/cppc_acpi.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 4e478f751ff7..adf9ca3be9fe 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -39,6 +39,7 @@
 #include <linux/ktime.h>
 #include <linux/rwsem.h>
 #include <linux/wait.h>
+#include <linux/platform_device.h>
 
 #include <acpi/cppc_acpi.h>
 
@@ -606,6 +607,23 @@ static bool is_cppc_supported(int revision, int num_ent)
 	return true;
 }
 
+static void add_cppc_cpufreq_dev(void)
+{
+	static bool already_added;
+
+	if (!already_added) {
+		struct platform_device *pdev;
+
+		pdev = platform_device_register_simple("cppc-cpufreq", -1,
+				NULL, 0);
+		if (IS_ERR(pdev))
+			pr_err("Couldn't register cppc-cpufreq err=%ld\n",
+					PTR_ERR(pdev));
+		else
+			already_added = true;
+	}
+}
+
 /*
  * An example CPC table looks like the following.
  *
@@ -815,6 +833,9 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
 	}
 
 	kfree(output.pointer);
+
+	add_cppc_cpufreq_dev();
+
 	return 0;
 
 out_free:
-- 
2.26.2

[PATCH 2/2] cpufreq: cppc: convert to a platform driver

[Mian Yousaf Kaukab]

From: Mian Yousaf Kaukab <ykaukab@suse.de>

Instead of piggybacking on ACPI_PROCESSOR_DEVICE_HID, convert the driver
to a proper platform driver with its own ID. Driver will only be probed
when cppc-cpufreq device is available.

Fixes: 28f06f770454 ("cppc_cpufreq: replace per-cpu structures with lists")
Signed-off-by: Mian Yousaf Kaukab <ykaukab@suse.de>
---
 drivers/cpufreq/cppc_cpufreq.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index bb4c068601db..defac1dd9a2a 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -11,13 +11,13 @@
 #define pr_fmt(fmt)	"CPPC Cpufreq:"	fmt
 
 #include <linux/kernel.h>
-#include <linux/module.h>
 #include <linux/delay.h>
 #include <linux/cpu.h>
 #include <linux/cpufreq.h>
 #include <linux/dmi.h>
 #include <linux/time.h>
 #include <linux/vmalloc.h>
+#include <linux/platform_device.h>
 
 #include <asm/unaligned.h>
 
@@ -512,7 +512,7 @@ static void cppc_check_hisi_workaround(void)
 	acpi_put_table(tbl);
 }
 
-static int __init cppc_cpufreq_init(void)
+static int cppc_cpufreq_probe(struct platform_device *pdev)
 {
 	if (acpi_disabled)
 		return -ENODEV;
@@ -533,26 +533,27 @@ static inline void free_cpu_data(void)
 		list_del(&iter->node);
 		kfree(iter);
 	}
-
 }
 
-static void __exit cppc_cpufreq_exit(void)
+static int cppc_cpufreq_remove(struct platform_device *pdev)
 {
 	cpufreq_unregister_driver(&cppc_cpufreq_driver);
 
 	free_cpu_data();
+
+	return 0;
 }
 
-module_exit(cppc_cpufreq_exit);
+static struct platform_driver cppc_cpufreq_platform_driver = {
+	.driver = {
+		.name = "cppc-cpufreq",
+	},
+	.probe = cppc_cpufreq_probe,
+	.remove = cppc_cpufreq_remove,
+};
+module_platform_driver(cppc_cpufreq_platform_driver);
+
+MODULE_ALIAS("platform:cppc-cpufreq");
 MODULE_AUTHOR("Ashwin Chaugule");
 MODULE_DESCRIPTION("CPUFreq driver based on the ACPI CPPC v5.0+ spec");
 MODULE_LICENSE("GPL");
-
-late_initcall(cppc_cpufreq_init);
-
-static const struct acpi_device_id cppc_acpi_ids[] __used = {
-	{ACPI_PROCESSOR_DEVICE_HID, },
-	{}
-};
-
-MODULE_DEVICE_TABLE(acpi, cppc_acpi_ids);
-- 
2.26.2

Re: [PATCH 1/2] acpi: cppc: add cpufreq device

[Rafael J. Wysocki]

On Thu, Dec 10, 2020 at 3:23 PM Mian Yousaf Kaukab
<yousaf.kaukab@suse.com> wrote:
>
> From: Mian Yousaf Kaukab <ykaukab@suse.de>
>
> Since commit 28f06f770454 ("cppc_cpufreq: replace per-cpu structures with
> lists"), cppc-cpufreq driver doesn't check availability of PSD data before
> registering with cpufreq core. As a result on a ThunderX2 platform when
> CPPC is disabled from BIOS, kernel log is spammed with following messages:
>
> [  180.974166] CPPC Cpufreq: Error in acquiring _CPC/_PSD data for CPUxx
>
> acpi_cppc_processor_probe() never succeed in this case because
> acpi_evaluate_object_typed("_CPC") always returns AE_NOT_FOUND. When
> cpufreq core calls cppc_cpufreq_cpu_init(), driver fails to obtain PSD data
> and print error messages.
>
> Convert cppc-cpufreq driver to a platform driver (done in a separate patch)
> and add cppc-cpufreq device when acpi_cppc_processor_probe() succeeds.

Honestly, I prefer to drop 28f06f770454 (along with its follower)
instead of making this change.

> Fixes: 28f06f770454 ("cppc_cpufreq: replace per-cpu structures with lists")

Thanks!

Re: [PATCH 1/2] acpi: cppc: add cpufreq device

[Ionela Voinescu]

Hi guys,

On Thursday 10 Dec 2020 at 15:32:09 (+0100), Rafael J. Wysocki wrote:
> On Thu, Dec 10, 2020 at 3:23 PM Mian Yousaf Kaukab
> <yousaf.kaukab@suse.com> wrote:
> >
> > From: Mian Yousaf Kaukab <ykaukab@suse.de>
> >
> > Since commit 28f06f770454 ("cppc_cpufreq: replace per-cpu structures with
> > lists"), cppc-cpufreq driver doesn't check availability of PSD data before
> > registering with cpufreq core. As a result on a ThunderX2 platform when
> > CPPC is disabled from BIOS, kernel log is spammed with following messages:
> >
> > [  180.974166] CPPC Cpufreq: Error in acquiring _CPC/_PSD data for CPUxx
> >
> > acpi_cppc_processor_probe() never succeed in this case because
> > acpi_evaluate_object_typed("_CPC") always returns AE_NOT_FOUND. When
> > cpufreq core calls cppc_cpufreq_cpu_init(), driver fails to obtain PSD data
> > and print error messages.
> >
> > Convert cppc-cpufreq driver to a platform driver (done in a separate patch)
> > and add cppc-cpufreq device when acpi_cppc_processor_probe() succeeds.
> 
> Honestly, I prefer to drop 28f06f770454 (along with its follower)
> instead of making this change.
> 
> > Fixes: 28f06f770454 ("cppc_cpufreq: replace per-cpu structures with lists")
> 

Sorry for introducing this, I though it was enough to bail out of cpu
init if _CPC entries are not present.

I'll defer to Rafael to decide whether to drop the patches or accept
alternative fixes, but I believe the rework of the data structures and
initialisation is useful.

As alternative fix, would it be okay to introduce a check function to
verify for !cpc_ptr, to be called in cppc_cpufreq_init()? In that case
the driver would not be registered if this check fails.

Thanks,
Ionela.

> Thanks!

Re: [PATCH 1/2] acpi: cppc: add cpufreq device

[Mian Yousaf Kaukab]

On Thu, Dec 10, 2020 at 03:32:09PM +0100, Rafael J. Wysocki wrote:
> On Thu, Dec 10, 2020 at 3:23 PM Mian Yousaf Kaukab
> <yousaf.kaukab@suse.com> wrote:
> >
> > Convert cppc-cpufreq driver to a platform driver (done in a separate patch)
> > and add cppc-cpufreq device when acpi_cppc_processor_probe() succeeds.
> 
> Honestly, I prefer to drop 28f06f770454 (along with its follower)
> instead of making this change.
> 
Even if we revert 28f06f770454 there is still one more small issue that these
patches fix. Currently, ACPI_PROCESSOR_DEVICE_HID is used to load cppc-cpufreq
module. In case when CPPC is disabled, some cycles will be wasted in loading
cppc-cpufreq module. The module will return error from the init call though
so no memory is wasted.

After converting to platform-driver, cppc-cpufreq module will only be loaded
when the platform-device is available.

BR,
Yousaf

Re: [PATCH 1/2] acpi: cppc: add cpufreq device

[Rafael J. Wysocki]

On Thursday, December 10, 2020 4:04:40 PM CET Mian Yousaf Kaukab wrote:
> On Thu, Dec 10, 2020 at 03:32:09PM +0100, Rafael J. Wysocki wrote:
> > On Thu, Dec 10, 2020 at 3:23 PM Mian Yousaf Kaukab
> > <yousaf.kaukab@suse.com> wrote:
> > >
> > > Convert cppc-cpufreq driver to a platform driver (done in a separate patch)
> > > and add cppc-cpufreq device when acpi_cppc_processor_probe() succeeds.
> > 
> > Honestly, I prefer to drop 28f06f770454 (along with its follower)
> > instead of making this change.
> > 
> Even if we revert 28f06f770454 there is still one more small issue that these
> patches fix. Currently, ACPI_PROCESSOR_DEVICE_HID is used to load cppc-cpufreq
> module. In case when CPPC is disabled, some cycles will be wasted in loading
> cppc-cpufreq module. The module will return error from the init call though
> so no memory is wasted.
> 
> After converting to platform-driver, cppc-cpufreq module will only be loaded
> when the platform-device is available.

Even so, that issue is low-impact AFAICS and may be addressed later and I'd
rather not let known breakage go into the mainline.

I'm going to do drop the problematic commit now and please work with Ionela
to produce a clean series of patches in the right order to avoid introducing
issues between them.

If that is done timely enough, it may still be possible to push those patches
for 5.11-rc1.

Thanks!

Re: [PATCH 1/2] acpi: cppc: add cpufreq device

[Ionela Voinescu]

Hi Rafael,

On Thursday 10 Dec 2020 at 17:55:56 (+0100), Rafael J. Wysocki wrote:
> On Thursday, December 10, 2020 4:04:40 PM CET Mian Yousaf Kaukab wrote:
> > On Thu, Dec 10, 2020 at 03:32:09PM +0100, Rafael J. Wysocki wrote:
> > > On Thu, Dec 10, 2020 at 3:23 PM Mian Yousaf Kaukab
> > > <yousaf.kaukab@suse.com> wrote:
> > > >
> > > > Convert cppc-cpufreq driver to a platform driver (done in a separate patch)
> > > > and add cppc-cpufreq device when acpi_cppc_processor_probe() succeeds.
> > > 
> > > Honestly, I prefer to drop 28f06f770454 (along with its follower)
> > > instead of making this change.
> > > 
> > Even if we revert 28f06f770454 there is still one more small issue that these
> > patches fix. Currently, ACPI_PROCESSOR_DEVICE_HID is used to load cppc-cpufreq
> > module. In case when CPPC is disabled, some cycles will be wasted in loading
> > cppc-cpufreq module. The module will return error from the init call though
> > so no memory is wasted.
> > 
> > After converting to platform-driver, cppc-cpufreq module will only be loaded
> > when the platform-device is available.
> 
> Even so, that issue is low-impact AFAICS and may be addressed later and I'd
> rather not let known breakage go into the mainline.
> 
> I'm going to do drop the problematic commit now and please work with Ionela
> to produce a clean series of patches in the right order to avoid introducing
> issues between them.
> 

The following commit will be easy to drop:
a37afa60de38  cppc_cpufreq: optimise memory allocation for HW and NONE coordination (2 weeks ago)

28f06f770454  will be more difficult to drop as it's embedded in the
series, and removing that one will produce conflicts in the patches
that follow it:

f9f5baa8b2a8  ACPI: processor: fix NONE coordination for domain mapping failure (3 weeks ago)
cdb4ae5de6f7  cppc_cpufreq: expose information on frequency domains (3 weeks ago)
c783a4d94848  cppc_cpufreq: clarify support for coordination types (3 weeks ago)
3bd412fb2c7f  cppc_cpufreq: use policy->cpu as driver of frequency setting (3 weeks ago)
28f06f770454  cppc_cpufreq: replace per-cpu structures with lists (3 weeks ago)
bb025fb6c276  cppc_cpufreq: simplify use of performance capabilities (3 weeks ago)
48ad8dc94032  cppc_cpufreq: clean up cpu, cpu_num and cpunum variable use (3 weeks ago)
63087265c288  cppc_cpufreq: fix misspelling, code style and readability issues (3 weeks ago)

Let me know how you want to proceed and I can either send a replacement
series or reverts with conflicts fixed.

Also, if you want you can drop all 5 (all before and including 28f06f770454),
and I can add them again in the series that fixes the problem that Yousaf
reported.

Thanks,
Ionela.

> If that is done timely enough, it may still be possible to push those patches
> for 5.11-rc1.
> 
> Thanks!
> 
> 
> 

Re: [PATCH 1/2] acpi: cppc: add cpufreq device

[Rafael J. Wysocki]

On Thu, Dec 10, 2020 at 6:23 PM Ionela Voinescu <ionela.voinescu@arm.com> wrote:
>
> Hi Rafael,
>
> On Thursday 10 Dec 2020 at 17:55:56 (+0100), Rafael J. Wysocki wrote:
> > On Thursday, December 10, 2020 4:04:40 PM CET Mian Yousaf Kaukab wrote:
> > > On Thu, Dec 10, 2020 at 03:32:09PM +0100, Rafael J. Wysocki wrote:
> > > > On Thu, Dec 10, 2020 at 3:23 PM Mian Yousaf Kaukab
> > > > <yousaf.kaukab@suse.com> wrote:
> > > > >
> > > > > Convert cppc-cpufreq driver to a platform driver (done in a separate patch)
> > > > > and add cppc-cpufreq device when acpi_cppc_processor_probe() succeeds.
> > > >
> > > > Honestly, I prefer to drop 28f06f770454 (along with its follower)
> > > > instead of making this change.
> > > >
> > > Even if we revert 28f06f770454 there is still one more small issue that these
> > > patches fix. Currently, ACPI_PROCESSOR_DEVICE_HID is used to load cppc-cpufreq
> > > module. In case when CPPC is disabled, some cycles will be wasted in loading
> > > cppc-cpufreq module. The module will return error from the init call though
> > > so no memory is wasted.
> > >
> > > After converting to platform-driver, cppc-cpufreq module will only be loaded
> > > when the platform-device is available.
> >
> > Even so, that issue is low-impact AFAICS and may be addressed later and I'd
> > rather not let known breakage go into the mainline.
> >
> > I'm going to do drop the problematic commit now and please work with Ionela
> > to produce a clean series of patches in the right order to avoid introducing
> > issues between them.
> >
>
> The following commit will be easy to drop:
> a37afa60de38  cppc_cpufreq: optimise memory allocation for HW and NONE coordination (2 weeks ago)
>
> 28f06f770454  will be more difficult to drop as it's embedded in the
> series, and removing that one will produce conflicts in the patches
> that follow it:
>
> f9f5baa8b2a8  ACPI: processor: fix NONE coordination for domain mapping failure (3 weeks ago)
> cdb4ae5de6f7  cppc_cpufreq: expose information on frequency domains (3 weeks ago)
> c783a4d94848  cppc_cpufreq: clarify support for coordination types (3 weeks ago)
> 3bd412fb2c7f  cppc_cpufreq: use policy->cpu as driver of frequency setting (3 weeks ago)
> 28f06f770454  cppc_cpufreq: replace per-cpu structures with lists (3 weeks ago)

I dropped the commits above along with a37afa60de38 (and regenerated
my pm-cpufreq branch).

> bb025fb6c276  cppc_cpufreq: simplify use of performance capabilities (3 weeks ago)
> 48ad8dc94032  cppc_cpufreq: clean up cpu, cpu_num and cpunum variable use (3 weeks ago)
> 63087265c288  cppc_cpufreq: fix misspelling, code style and readability issues (3 weeks ago)
>
> Let me know how you want to proceed and I can either send a replacement
> series or reverts with conflicts fixed.

Please feel free to resubmit with the issue at hand addressed.

Thanks!