From: Mian Yousaf Kaukab <[email protected]>
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 <[email protected]>
Signed-off-by: Mian Yousaf Kaukab <[email protected]>
---
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
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
> <[email protected]> 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
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
> > <[email protected]> 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!
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
> > > <[email protected]> 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!
>
>
>
On Thu, Dec 10, 2020 at 3:23 PM Mian Yousaf Kaukab
<[email protected]> wrote:
>
> From: Mian Yousaf Kaukab <[email protected]>
>
> 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!
On Thu, Dec 10, 2020 at 6:23 PM Ionela Voinescu <[email protected]> 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
> > > > <[email protected]> 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!
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
> <[email protected]> wrote:
> >
> > From: Mian Yousaf Kaukab <[email protected]>
> >
> > 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!