LinuxLists.cc - [PATCH v6 3/7] powercap/drivers/dtpm: Simplify the dtpm table

[permalink] [raw]

Subject: Re: [PATCH v6 3/7] powercap/drivers/dtpm: Simplify the dtpm table

Hi Doug,

On Fri, Nov 26, 2021 at 6:08 PM Doug Smythies <[email protected]> wrote:
>
> Hi Daniel,
>
> This patch introduces a regression, at least on my test system.
> I can no longer change CPU frequency scaling drivers, for example
> from intel_cpufreq (A.K.A intel_pstate in passive mode) to intel_pstate
> (A.K.A. active mode). The task just hangs forever.
>
> I bisected the kernel and got this commit as the result.
> As a double check, I reverted this commit:
> 7a89d7eacf8e84f2afb94db5ae9d9f9faa93f01c
> on kernel 5.16-rc2 and the issue was resolved.
>
> While your email is fairly old, I observe that it was only included as of
> kernel 5.16-rc1.
>
> Command Example that never completes:
>
> $ echo passive | sudo tee /sys/devices/system/cpu/intel_pstate/status
>
> syslog excerpt attached.

This looks like it may be problematic:

diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
index f6076de39540..98841524a782 100644
--- a/drivers/powercap/dtpm_cpu.c
+++ b/drivers/powercap/dtpm_cpu.c
@@ -204,7 +204,7 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
return ret;
}

-int dtpm_register_cpu(struct dtpm *parent)
+static int __init dtpm_cpu_init(void)
{
int ret;

so please try to remove the __init annotation from dtpm_cpu_init() and
see if that helps.

2021-11-26 17:31:42

[permalink] [raw]

Subject: Re: [PATCH v6 3/7] powercap/drivers/dtpm: Simplify the dtpm table

Hi Daniel,

This patch introduces a regression, at least on my test system.
I can no longer change CPU frequency scaling drivers, for example
from intel_cpufreq (A.K.A intel_pstate in passive mode) to intel_pstate
(A.K.A. active mode). The task just hangs forever.

I bisected the kernel and got this commit as the result.
As a double check, I reverted this commit:
7a89d7eacf8e84f2afb94db5ae9d9f9faa93f01c
on kernel 5.16-rc2 and the issue was resolved.

While your email is fairly old, I observe that it was only included as of
kernel 5.16-rc1.

Command Example that never completes:

$ echo passive | sudo tee /sys/devices/system/cpu/intel_pstate/status

syslog excerpt attached.

... Doug

On Thu, Apr 1, 2021 at 12:24 PM Daniel Lezcano
<[email protected]> wrote:
>
> The dtpm table is an array of pointers, that forces the user of the
> table to define initdata along with the declaration of the table
> entry. It is more efficient to create an array of dtpm structure, so
> the declaration of the table entry can be done by initializing the
> different fields.
>
> Signed-off-by: Daniel Lezcano <[email protected]>
> Reviewed-by: Lukasz Luba <[email protected]>
> ---
> drivers/powercap/dtpm.c | 4 ++--
> drivers/powercap/dtpm_cpu.c | 4 +++-
> include/linux/dtpm.h | 20 +++++++++-----------
> 3 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c
> index a707cc2965a1..d9aac107c927 100644
> --- a/drivers/powercap/dtpm.c
> +++ b/drivers/powercap/dtpm.c
> @@ -583,7 +583,7 @@ int dtpm_register(const char *name, struct dtpm *dtpm, struct dtpm *parent)
>
> static int __init dtpm_init(void)
> {
> - struct dtpm_descr **dtpm_descr;
> + struct dtpm_descr *dtpm_descr;
>
> pct = powercap_register_control_type(NULL, "dtpm", NULL);
> if (IS_ERR(pct)) {
> @@ -592,7 +592,7 @@ static int __init dtpm_init(void)
> }
>
> for_each_dtpm_table(dtpm_descr)
> - (*dtpm_descr)->init(*dtpm_descr);
> + dtpm_descr->init();
>
> return 0;
> }
> diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
> index 9deafd16a197..74b39a1082e5 100644
> --- a/drivers/powercap/dtpm_cpu.c
> +++ b/drivers/powercap/dtpm_cpu.c
> @@ -204,7 +204,7 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
> return ret;
> }
>
> -int dtpm_register_cpu(struct dtpm *parent)
> +static int __init dtpm_cpu_init(void)
> {
> int ret;
>
> @@ -241,3 +241,5 @@ int dtpm_register_cpu(struct dtpm *parent)
>
> return 0;
> }
> +
> +DTPM_DECLARE(dtpm_cpu, dtpm_cpu_init);
> diff --git a/include/linux/dtpm.h b/include/linux/dtpm.h
> index 577c71d4e098..2ec2821111d1 100644
> --- a/include/linux/dtpm.h
> +++ b/include/linux/dtpm.h
> @@ -33,25 +33,23 @@ struct dtpm_ops {
> void (*release)(struct dtpm *);
> };
>
> -struct dtpm_descr;
> -
> -typedef int (*dtpm_init_t)(struct dtpm_descr *);
> +typedef int (*dtpm_init_t)(void);
>
> struct dtpm_descr {
> - struct dtpm *parent;
> - const char *name;
> dtpm_init_t init;
> };
>
> /* Init section thermal table */
> -extern struct dtpm_descr *__dtpm_table[];
> -extern struct dtpm_descr *__dtpm_table_end[];
> +extern struct dtpm_descr __dtpm_table[];
> +extern struct dtpm_descr __dtpm_table_end[];
>
> -#define DTPM_TABLE_ENTRY(name) \
> - static typeof(name) *__dtpm_table_entry_##name \
> - __used __section("__dtpm_table") = &name
> +#define DTPM_TABLE_ENTRY(name, __init) \
> + static struct dtpm_descr __dtpm_table_entry_##name \
> + __used __section("__dtpm_table") = { \
> + .init = __init, \
> + }
>
> -#define DTPM_DECLARE(name) DTPM_TABLE_ENTRY(name)
> +#define DTPM_DECLARE(name, init) DTPM_TABLE_ENTRY(name, init)
>
> #define for_each_dtpm_table(__dtpm) \
> for (__dtpm = __dtpm_table; \
> --
> 2.17.1
>

Attachments:

syslog_example.txt (3.88 kB)

2021-11-26 18:01:44

[permalink] [raw]

Subject: Re: [PATCH v6 3/7] powercap/drivers/dtpm: Simplify the dtpm table

On 26/11/2021 18:08, Doug Smythies wrote:
> Hi Daniel,
>
> This patch introduces a regression, at least on my test system.
> I can no longer change CPU frequency scaling drivers, for example
> from intel_cpufreq (A.K.A intel_pstate in passive mode) to intel_pstate
> (A.K.A. active mode). The task just hangs forever.
>
> I bisected the kernel and got this commit as the result.
> As a double check, I reverted this commit:
> 7a89d7eacf8e84f2afb94db5ae9d9f9faa93f01c
> on kernel 5.16-rc2 and the issue was resolved.
>
> While your email is fairly old, I observe that it was only included as of
> kernel 5.16-rc1.

Could it be related to and fixed by:

https://lore.kernel.org/all/[email protected]/

?

> Command Example that never completes:
>
> $ echo passive | sudo tee /sys/devices/system/cpu/intel_pstate/status
>
> syslog excerpt attached.
>
> ... Doug

--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

2021-11-26 18:08:35

[permalink] [raw]

Subject: Re: [PATCH v6 3/7] powercap/drivers/dtpm: Simplify the dtpm table

On 26/11/2021 18:21, Rafael J. Wysocki wrote:
> Hi Doug,
>
> On Fri, Nov 26, 2021 at 6:08 PM Doug Smythies <[email protected]> wrote:
>>
>> Hi Daniel,
>>
>> This patch introduces a regression, at least on my test system.
>> I can no longer change CPU frequency scaling drivers, for example
>> from intel_cpufreq (A.K.A intel_pstate in passive mode) to intel_pstate
>> (A.K.A. active mode). The task just hangs forever.
>>
>> I bisected the kernel and got this commit as the result.
>> As a double check, I reverted this commit:
>> 7a89d7eacf8e84f2afb94db5ae9d9f9faa93f01c
>> on kernel 5.16-rc2 and the issue was resolved.
>>
>> While your email is fairly old, I observe that it was only included as of
>> kernel 5.16-rc1.
>>
>> Command Example that never completes:
>>
>> $ echo passive | sudo tee /sys/devices/system/cpu/intel_pstate/status
>>
>> syslog excerpt attached.
>
> This looks like it may be problematic:
>
> diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
> index f6076de39540..98841524a782 100644
> --- a/drivers/powercap/dtpm_cpu.c
> +++ b/drivers/powercap/dtpm_cpu.c
> @@ -204,7 +204,7 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
> return ret;
> }
>
> -int dtpm_register_cpu(struct dtpm *parent)
> +static int __init dtpm_cpu_init(void)
> {
> int ret;
>
> so please try to remove the __init annotation from dtpm_cpu_init() and
> see if that helps.

Yes, actually that should be called only if it is configured properly.
The dtpm_cpu just initializes itself unconditionally, I did not figured
out there is the usually allyesconfig used by default by the distros.

That should be fixed with a proper DT configuration [1]

[1]
https://lore.kernel.org/all/[email protected]/

--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

2021-11-26 18:20:16

[permalink] [raw]

Subject: Re: [PATCH v6 3/7] powercap/drivers/dtpm: Simplify the dtpm table

On Friday, November 26, 2021 6:43:24 PM CET Daniel Lezcano wrote:
> On 26/11/2021 18:21, Rafael J. Wysocki wrote:
> > Hi Doug,
> >
> > On Fri, Nov 26, 2021 at 6:08 PM Doug Smythies <[email protected]> wrote:
> >>
> >> Hi Daniel,
> >>
> >> This patch introduces a regression, at least on my test system.
> >> I can no longer change CPU frequency scaling drivers, for example
> >> from intel_cpufreq (A.K.A intel_pstate in passive mode) to intel_pstate
> >> (A.K.A. active mode). The task just hangs forever.
> >>
> >> I bisected the kernel and got this commit as the result.
> >> As a double check, I reverted this commit:
> >> 7a89d7eacf8e84f2afb94db5ae9d9f9faa93f01c
> >> on kernel 5.16-rc2 and the issue was resolved.
> >>
> >> While your email is fairly old, I observe that it was only included as of
> >> kernel 5.16-rc1.
> >>
> >> Command Example that never completes:
> >>
> >> $ echo passive | sudo tee /sys/devices/system/cpu/intel_pstate/status
> >>
> >> syslog excerpt attached.
> >
> > This looks like it may be problematic:
> >
> > diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
> > index f6076de39540..98841524a782 100644
> > --- a/drivers/powercap/dtpm_cpu.c
> > +++ b/drivers/powercap/dtpm_cpu.c
> > @@ -204,7 +204,7 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
> > return ret;
> > }
> >
> > -int dtpm_register_cpu(struct dtpm *parent)
> > +static int __init dtpm_cpu_init(void)
> > {
> > int ret;
> >
> > so please try to remove the __init annotation from dtpm_cpu_init() and
> > see if that helps.
>
> Yes, actually that should be called only if it is configured properly.

What do you mean?

> The dtpm_cpu just initializes itself unconditionally, I did not figured
> out there is the usually allyesconfig used by default by the distros.

Well, it is.

> That should be fixed with a proper DT configuration [1]
>
> [1]
> https://lore.kernel.org/all/[email protected]/

No, we are talking about systems without any DT at all here.

2021-11-26 18:25:39

[permalink] [raw]

Subject: Re: [PATCH v6 3/7] powercap/drivers/dtpm: Simplify the dtpm table

On Fri, Nov 26, 2021 at 6:40 PM Daniel Lezcano
<[email protected]> wrote:
>
> On 26/11/2021 18:08, Doug Smythies wrote:
> > Hi Daniel,
> >
> > This patch introduces a regression, at least on my test system.
> > I can no longer change CPU frequency scaling drivers, for example
> > from intel_cpufreq (A.K.A intel_pstate in passive mode) to intel_pstate
> > (A.K.A. active mode). The task just hangs forever.
> >
> > I bisected the kernel and got this commit as the result.
> > As a double check, I reverted this commit:
> > 7a89d7eacf8e84f2afb94db5ae9d9f9faa93f01c
> > on kernel 5.16-rc2 and the issue was resolved.
> >
> > While your email is fairly old, I observe that it was only included as of
> > kernel 5.16-rc1.
>
> Could it be related to and fixed by:
>
> https://lore.kernel.org/all/[email protected]/
>
> ?

It seems so, but that patch is present in 5.16-rc2.

2021-11-26 19:32:12

[permalink] [raw]

Subject: Re: [PATCH v6 3/7] powercap/drivers/dtpm: Simplify the dtpm table

On Fri, Nov 26, 2021 at 8:10 PM Doug Smythies <[email protected]> wrote:
>
> On Fri, Nov 26, 2021 at 9:22 AM Rafael J. Wysocki <[email protected]> wrote:
> >
> > Hi Doug,
> >
> > On Fri, Nov 26, 2021 at 6:08 PM Doug Smythies <[email protected]> wrote:
> > >
> > > Hi Daniel,
> > >
> > > This patch introduces a regression, at least on my test system.
> > > I can no longer change CPU frequency scaling drivers, for example
> > > from intel_cpufreq (A.K.A intel_pstate in passive mode) to intel_pstate
> > > (A.K.A. active mode). The task just hangs forever.
> > >
> > > I bisected the kernel and got this commit as the result.
> > > As a double check, I reverted this commit:
> > > 7a89d7eacf8e84f2afb94db5ae9d9f9faa93f01c
> > > on kernel 5.16-rc2 and the issue was resolved.
> > >
> > > While your email is fairly old, I observe that it was only included as of
> > > kernel 5.16-rc1.
> > >
> > > Command Example that never completes:
> > >
> > > $ echo passive | sudo tee /sys/devices/system/cpu/intel_pstate/status
> > >
> > > syslog excerpt attached.
> >
> > This looks like it may be problematic:
> >
> > diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
> > index f6076de39540..98841524a782 100644
> > --- a/drivers/powercap/dtpm_cpu.c
> > +++ b/drivers/powercap/dtpm_cpu.c
> > @@ -204,7 +204,7 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
> > return ret;
> > }
> >
> > -int dtpm_register_cpu(struct dtpm *parent)
> > +static int __init dtpm_cpu_init(void)
> > {
> > int ret;
> >
> > so please try to remove the __init annotation from dtpm_cpu_init() and
> > see if that helps.
>
> Hi Rafael,
>
> That did not fix the issue.
> Just to be clear this is what I did, on top of 5.16-rc2:
>
> $ git diff
> diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
> index b740866b228d..26d1a87bdec6 100644
> --- a/drivers/powercap/dtpm_cpu.c
> +++ b/drivers/powercap/dtpm_cpu.c
> @@ -231,7 +231,7 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
> return ret;
> }
>
> -static int __init dtpm_cpu_init(void)
> +static int dtpm_cpu_init(void)
> {
> int ret;

OK

This needs to be fixed or we'll need to revert commit
7a89d7eacf8e84f2afb94db5ae9d9f9faa93f01c for 5.16.

2021-11-26 19:38:47

[permalink] [raw]

Subject: Re: [PATCH v6 3/7] powercap/drivers/dtpm: Simplify the dtpm table

On Fri, Nov 26, 2021 at 9:22 AM Rafael J. Wysocki <[email protected]> wrote:
>
> Hi Doug,
>
> On Fri, Nov 26, 2021 at 6:08 PM Doug Smythies <[email protected]> wrote:
> >
> > Hi Daniel,
> >
> > This patch introduces a regression, at least on my test system.
> > I can no longer change CPU frequency scaling drivers, for example
> > from intel_cpufreq (A.K.A intel_pstate in passive mode) to intel_pstate
> > (A.K.A. active mode). The task just hangs forever.
> >
> > I bisected the kernel and got this commit as the result.
> > As a double check, I reverted this commit:
> > 7a89d7eacf8e84f2afb94db5ae9d9f9faa93f01c
> > on kernel 5.16-rc2 and the issue was resolved.
> >
> > While your email is fairly old, I observe that it was only included as of
> > kernel 5.16-rc1.
> >
> > Command Example that never completes:
> >
> > $ echo passive | sudo tee /sys/devices/system/cpu/intel_pstate/status
> >
> > syslog excerpt attached.
>
> This looks like it may be problematic:
>
> diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
> index f6076de39540..98841524a782 100644
> --- a/drivers/powercap/dtpm_cpu.c
> +++ b/drivers/powercap/dtpm_cpu.c
> @@ -204,7 +204,7 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
> return ret;
> }
>
> -int dtpm_register_cpu(struct dtpm *parent)
> +static int __init dtpm_cpu_init(void)
> {
> int ret;
>
> so please try to remove the __init annotation from dtpm_cpu_init() and
> see if that helps.

Hi Rafael,

That did not fix the issue.
Just to be clear this is what I did, on top of 5.16-rc2:

$ git diff
diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
index b740866b228d..26d1a87bdec6 100644
--- a/drivers/powercap/dtpm_cpu.c
+++ b/drivers/powercap/dtpm_cpu.c
@@ -231,7 +231,7 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
return ret;
}

-static int __init dtpm_cpu_init(void)
+static int dtpm_cpu_init(void)
{
int ret;

2021-11-26 21:58:50

[permalink] [raw]

Subject: Re: [PATCH v6 3/7] powercap/drivers/dtpm: Simplify the dtpm table

On Fri, Nov 26, 2021 at 9:43 AM Daniel Lezcano
<[email protected]> wrote:
>
> On 26/11/2021 18:21, Rafael J. Wysocki wrote:
> > Hi Doug,
> >
> > On Fri, Nov 26, 2021 at 6:08 PM Doug Smythies <[email protected]> wrote:
> >>
> >> Hi Daniel,
> >>
> >> This patch introduces a regression, at least on my test system.
> >> I can no longer change CPU frequency scaling drivers, for example
> >> from intel_cpufreq (A.K.A intel_pstate in passive mode) to intel_pstate
> >> (A.K.A. active mode). The task just hangs forever.
> >>
> >> I bisected the kernel and got this commit as the result.
> >> As a double check, I reverted this commit:
> >> 7a89d7eacf8e84f2afb94db5ae9d9f9faa93f01c
> >> on kernel 5.16-rc2 and the issue was resolved.
> >>
> >> While your email is fairly old, I observe that it was only included as of
> >> kernel 5.16-rc1.
> >>
> >> Command Example that never completes:
> >>
> >> $ echo passive | sudo tee /sys/devices/system/cpu/intel_pstate/status
> >>
> >> syslog excerpt attached.
> >
> > This looks like it may be problematic:
> >
> > diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
> > index f6076de39540..98841524a782 100644
> > --- a/drivers/powercap/dtpm_cpu.c
> > +++ b/drivers/powercap/dtpm_cpu.c
> > @@ -204,7 +204,7 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
> > return ret;
> > }
> >
> > -int dtpm_register_cpu(struct dtpm *parent)
> > +static int __init dtpm_cpu_init(void)
> > {
> > int ret;
> >
> > so please try to remove the __init annotation from dtpm_cpu_init() and
> > see if that helps.
>
> Yes, actually that should be called only if it is configured properly.
> The dtpm_cpu just initializes itself unconditionally, I did not figured
> out there is the usually allyesconfig used by default by the distros.
>
> That should be fixed with a proper DT configuration [1]

I added your 5 patch set on top of 5.16-rc2 and confirm it fixes
the issue. I tested both ways, with CONFIG_OF not set, forcing the
CONFIG_DTPM stuff off, and with CONFIG_OF=y.

Oh, I used V2 of the patch set from earlier today.

... Doug

>
> [1]
> https://lore.kernel.org/all/[email protected]/
>
> --
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog

2021-11-26 23:08:01

[permalink] [raw]

Subject: Re: [PATCH v6 3/7] powercap/drivers/dtpm: Simplify the dtpm table

On 26/11/2021 22:56, Doug Smythies wrote:
> On Fri, Nov 26, 2021 at 9:43 AM Daniel Lezcano
> <[email protected]> wrote:
>>
>> On 26/11/2021 18:21, Rafael J. Wysocki wrote:
>>> Hi Doug,
>>>
>>> On Fri, Nov 26, 2021 at 6:08 PM Doug Smythies <[email protected]> wrote:
>>>>
>>>> Hi Daniel,
>>>>
>>>> This patch introduces a regression, at least on my test system.
>>>> I can no longer change CPU frequency scaling drivers, for example
>>>> from intel_cpufreq (A.K.A intel_pstate in passive mode) to intel_pstate
>>>> (A.K.A. active mode). The task just hangs forever.
>>>>
>>>> I bisected the kernel and got this commit as the result.
>>>> As a double check, I reverted this commit:
>>>> 7a89d7eacf8e84f2afb94db5ae9d9f9faa93f01c
>>>> on kernel 5.16-rc2 and the issue was resolved.
>>>>
>>>> While your email is fairly old, I observe that it was only included as of
>>>> kernel 5.16-rc1.
>>>>
>>>> Command Example that never completes:
>>>>
>>>> $ echo passive | sudo tee /sys/devices/system/cpu/intel_pstate/status
>>>>
>>>> syslog excerpt attached.
>>>
>>> This looks like it may be problematic:
>>>
>>> diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
>>> index f6076de39540..98841524a782 100644
>>> --- a/drivers/powercap/dtpm_cpu.c
>>> +++ b/drivers/powercap/dtpm_cpu.c
>>> @@ -204,7 +204,7 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
>>> return ret;
>>> }
>>>
>>> -int dtpm_register_cpu(struct dtpm *parent)
>>> +static int __init dtpm_cpu_init(void)
>>> {
>>> int ret;
>>>
>>> so please try to remove the __init annotation from dtpm_cpu_init() and
>>> see if that helps.
>>
>> Yes, actually that should be called only if it is configured properly.
>> The dtpm_cpu just initializes itself unconditionally, I did not figured
>> out there is the usually allyesconfig used by default by the distros.
>>
>> That should be fixed with a proper DT configuration [1]
>
> I added your 5 patch set on top of 5.16-rc2 and confirm it fixes
> the issue. I tested both ways, with CONFIG_OF not set, forcing the
> CONFIG_DTPM stuff off, and with CONFIG_OF=y.
>
> Oh, I used V2 of the patch set from earlier today.

Thanks Doug for testing.

For a -rc I should prevent the cpu to setup at boot time with a simple fix.

--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

2021-11-26 23:11:12

[permalink] [raw]

Subject: [PATCH] powercap/drivers/dtpm: Disable dtpm at boot time

The DTPM framework misses a mechanism to set it up. That is currently
under review but will come after the next cycle.

As the distro are enabling all the kernel options, the DTPM framework
is enabled on platforms where the energy model is not implemented,
thus making the framework inconsistent and disrupting the CPU
frequency scaling service.

Remove the initialization at boot time as a hot fix.

Fixes: 7a89d7eacf8e ("powercap/drivers/dtpm: Simplify the dtpm table")
Signed-off-by: Daniel Lezcano <[email protected]>
---
drivers/powercap/dtpm.c | 3 ---
1 file changed, 3 deletions(-)

diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c
index b9fac786246a..fb35c5828bfb 100644
--- a/drivers/powercap/dtpm.c
+++ b/drivers/powercap/dtpm.c
@@ -471,9 +471,6 @@ static int __init init_dtpm(void)
return PTR_ERR(pct);
}

- for_each_dtpm_table(dtpm_descr)
- dtpm_descr->init();
-
return 0;
}
late_initcall(init_dtpm);
--
2.25.1

2021-11-26 23:12:57

[permalink] [raw]

Subject: Re: [PATCH] powercap/drivers/dtpm: Disable dtpm at boot time

Hi Doug,

I was unable to reproduce the issue because I don't have an x86 platform.

Is it possible to check if this fix is ok?

-- Daniel

On 27/11/2021 00:08, Daniel Lezcano wrote:
> The DTPM framework misses a mechanism to set it up. That is currently
> under review but will come after the next cycle.
>
> As the distro are enabling all the kernel options, the DTPM framework
> is enabled on platforms where the energy model is not implemented,
> thus making the framework inconsistent and disrupting the CPU
> frequency scaling service.
>
> Remove the initialization at boot time as a hot fix.
>
> Fixes: 7a89d7eacf8e ("powercap/drivers/dtpm: Simplify the dtpm table")
> Signed-off-by: Daniel Lezcano <[email protected]>
> ---
> drivers/powercap/dtpm.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c
> index b9fac786246a..fb35c5828bfb 100644
> --- a/drivers/powercap/dtpm.c
> +++ b/drivers/powercap/dtpm.c
> @@ -471,9 +471,6 @@ static int __init init_dtpm(void)
> return PTR_ERR(pct);
> }
>
> - for_each_dtpm_table(dtpm_descr)
> - dtpm_descr->init();
> -
> return 0;
> }
> late_initcall(init_dtpm);
>

--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

2021-11-27 01:15:26

[permalink] [raw]

Subject: Re: [PATCH] powercap/drivers/dtpm: Disable dtpm at boot time

On Fri, Nov 26, 2021 at 3:10 PM Daniel Lezcano
<[email protected]> wrote:
>
>
> Hi Doug,
>
> I was unable to reproduce the issue because I don't have an x86 platform.
>
> Is it possible to check if this fix is ok?

Hi Daniel,

Yes, confirmed.

Tested-By: Doug Smythies <[email protected]>

>
> -- Daniel
>
> On 27/11/2021 00:08, Daniel Lezcano wrote:
> > The DTPM framework misses a mechanism to set it up. That is currently
> > under review but will come after the next cycle.
> >
> > As the distro are enabling all the kernel options, the DTPM framework
> > is enabled on platforms where the energy model is not implemented,
> > thus making the framework inconsistent and disrupting the CPU
> > frequency scaling service.
> >
> > Remove the initialization at boot time as a hot fix.
> >
> > Fixes: 7a89d7eacf8e ("powercap/drivers/dtpm: Simplify the dtpm table")
> > Signed-off-by: Daniel Lezcano <[email protected]>
> > ---
> > drivers/powercap/dtpm.c | 3 ---
> > 1 file changed, 3 deletions(-)
> >
> > diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c
> > index b9fac786246a..fb35c5828bfb 100644
> > --- a/drivers/powercap/dtpm.c
> > +++ b/drivers/powercap/dtpm.c
> > @@ -471,9 +471,6 @@ static int __init init_dtpm(void)
> > return PTR_ERR(pct);
> > }
> >
> > - for_each_dtpm_table(dtpm_descr)
> > - dtpm_descr->init();
> > -
> > return 0;
> > }
> > late_initcall(init_dtpm);
> >
>
>
> --
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog

2021-11-30 16:47:44

[permalink] [raw]

Subject: Re: [PATCH v6 3/7] powercap/drivers/dtpm: Simplify the dtpm table

On 26/11/2021 20:29, Rafael J. Wysocki wrote:
> On Fri, Nov 26, 2021 at 8:10 PM Doug Smythies <[email protected]> wrote:
>>
>> On Fri, Nov 26, 2021 at 9:22 AM Rafael J. Wysocki <[email protected]> wrote:
>>>
>>> Hi Doug,
>>>
>>> On Fri, Nov 26, 2021 at 6:08 PM Doug Smythies <[email protected]> wrote:
>>>>
>>>> Hi Daniel,
>>>>
>>>> This patch introduces a regression, at least on my test system.
>>>> I can no longer change CPU frequency scaling drivers, for example
>>>> from intel_cpufreq (A.K.A intel_pstate in passive mode) to intel_pstate
>>>> (A.K.A. active mode). The task just hangs forever.
>>>>
>>>> I bisected the kernel and got this commit as the result.
>>>> As a double check, I reverted this commit:
>>>> 7a89d7eacf8e84f2afb94db5ae9d9f9faa93f01c
>>>> on kernel 5.16-rc2 and the issue was resolved.

[ ... ]

>> -static int __init dtpm_cpu_init(void)
>> +static int dtpm_cpu_init(void)
>> {
>> int ret;
>
> OK
>
> This needs to be fixed or we'll need to revert commit
> 7a89d7eacf8e84f2afb94db5ae9d9f9faa93f01c for 5.16.

I've send a fix for that and Doug tested it (thanks) [1]

-- Daniel

[1]
https://lore.kernel.org/all/CAAYoRsWEXoe_LjuHuQUL3Tdov0JVW887T4ciUTVOC410mZjgvA@mail.gmail.com/

--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

2021-12-01 18:57:42