[v6,3/7] powercap/drivers/dtpm: Simplify the dtpm table

Commit Message

Daniel Lezcano April 1, 2021, 6:36 p.m. UTC

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 <[e-mail]>
Reviewed-by: Lukasz Luba <[e-mail]>
---
 drivers/powercap/dtpm.c     |  4 ++--
 drivers/powercap/dtpm_cpu.c |  4 +++-
 include/linux/dtpm.h        | 20 +++++++++-----------
 3 files changed, 14 insertions(+), 14 deletions(-)

Comments

Doug Smythies Nov. 26, 2021, 5:08 p.m. UTC | #1

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
<[e-mail]> 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 <[e-mail]>
> Reviewed-by: Lukasz Luba <[e-mail]>
> ---
>  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
>
Nov 26 07:31:13 s19 systemd[1]: Finished Update UTMP about System Runlevel Changes.
Nov 26 07:31:13 s19 systemd[1]: Startup finished in 10.788s (firmware) + 12.351s (loader) + 1.808s (kernel) + 1min 38.047s (userspace) = 2min 2.995s.
Nov 26 07:31:58 s19 kernel: [  144.568026] INFO: task tee:1270 blocked for more than 20 seconds.
Nov 26 07:31:58 s19 kernel: [  144.568069]       Not tainted 5.15.0-rc1-n13 #977   <<<< 13th kernel of bisection
Nov 26 07:31:58 s19 kernel: [  144.568096] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
Nov 26 07:31:58 s19 kernel: [  144.568135] task:tee             state:D stack:    0 pid: 1270 ppid:  1269 flags:0x00004000
Nov 26 07:31:58 s19 kernel: [  144.568141] Call Trace:
Nov 26 07:31:58 s19 kernel: [  144.568145]  __schedule+0xce9/0x13f0
Nov 26 07:31:58 s19 kernel: [  144.568155]  ? __slab_free+0x217/0x400
Nov 26 07:31:58 s19 kernel: [  144.568162]  ? __slab_free+0x217/0x400
Nov 26 07:31:58 s19 kernel: [  144.568167]  ? radix_tree_lookup+0xd/0x10
Nov 26 07:31:58 s19 kernel: [  144.568172]  ? idr_find+0xf/0x20
Nov 26 07:31:58 s19 kernel: [  144.568177]  ? get_work_pool+0x2d/0x40
Nov 26 07:31:58 s19 kernel: [  144.568182]  schedule+0x67/0xe0
Nov 26 07:31:58 s19 kernel: [  144.568187]  schedule_timeout+0x200/0x290
Nov 26 07:31:58 s19 kernel: [  144.568191]  ? freq_qos_apply+0x2d/0x50
Nov 26 07:31:58 s19 kernel: [  144.568198]  wait_for_completion+0x8b/0xf0
Nov 26 07:31:58 s19 kernel: [  144.568202]  cpufreq_policy_put_kobj+0x4d/0x90
Nov 26 07:31:58 s19 kernel: [  144.568207]  cpufreq_policy_free+0x12f/0x160
Nov 26 07:31:58 s19 kernel: [  144.568211]  cpufreq_remove_dev+0x91/0xb0
Nov 26 07:31:58 s19 kernel: [  144.568216]  subsys_interface_unregister+0x9d/0x100
Nov 26 07:31:58 s19 kernel: [  144.568223]  cpufreq_unregister_driver+0x35/0xd0
Nov 26 07:31:58 s19 kernel: [  144.568227]  store_status+0x102/0x1c0
Nov 26 07:31:58 s19 kernel: [  144.568232]  kobj_attr_store+0xf/0x20
Nov 26 07:31:58 s19 kernel: [  144.568237]  sysfs_kf_write+0x3b/0x50
Nov 26 07:31:58 s19 kernel: [  144.568242]  kernfs_fop_write_iter+0x135/0x1c0
Nov 26 07:31:58 s19 kernel: [  144.568246]  new_sync_write+0x11d/0x1b0
Nov 26 07:31:58 s19 kernel: [  144.568253]  vfs_write+0x1a7/0x290
Nov 26 07:31:58 s19 kernel: [  144.568256]  ksys_write+0x67/0xe0
Nov 26 07:31:58 s19 kernel: [  144.568259]  __x64_sys_write+0x1a/0x20
Nov 26 07:31:58 s19 kernel: [  144.568261]  do_syscall_64+0x59/0xc0
Nov 26 07:31:58 s19 kernel: [  144.568268]  ? vfs_write+0x1a7/0x290
Nov 26 07:31:58 s19 kernel: [  144.568271]  ? exit_to_user_mode_prepare+0x3d/0x1c0
Nov 26 07:31:58 s19 kernel: [  144.568277]  ? ksys_write+0x67/0xe0
Nov 26 07:31:58 s19 kernel: [  144.568280]  ? syscall_exit_to_user_mode+0x27/0x50
Nov 26 07:31:58 s19 kernel: [  144.568283]  ? __x64_sys_write+0x1a/0x20
Nov 26 07:31:58 s19 kernel: [  144.568286]  ? do_syscall_64+0x69/0xc0
Nov 26 07:31:58 s19 kernel: [  144.568291]  entry_SYSCALL_64_after_hwframe+0x44/0xae
Nov 26 07:31:58 s19 kernel: [  144.568296] RIP: 0033:0x7fca551ca1e7
Nov 26 07:31:58 s19 kernel: [  144.568300] RSP: 002b:00007fffad47a6b8 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
Nov 26 07:31:58 s19 kernel: [  144.568304] RAX: ffffffffffffffda RBX: 0000000000000007 RCX: 00007fca551ca1e7
Nov 26 07:31:58 s19 kernel: [  144.568307] RDX: 0000000000000007 RSI: 00007fffad47a7b0 RDI: 0000000000000003
Nov 26 07:31:58 s19 kernel: [  144.568309] RBP: 00007fffad47a7b0 R08: 0000000000000007 R09: 0000000000000001
Nov 26 07:31:58 s19 kernel: [  144.568311] R10: 00000000000001b6 R11: 0000000000000246 R12: 0000000000000007
Nov 26 07:31:58 s19 kernel: [  144.568313] R13: 000055eb03a6f4a0 R14: 0000000000000007 R15: 00007fca552a58a0
Nov 26 07:32:13 s19 systemd[1]: Stopping Session 1 of user doug.
Nov 26 07:32:13 s19 systemd[1]: Stopping Session 3 of user doug.
Nov 26 07:32:13 s19 systemd[1]: Stopping Session 4 of user doug.
Nov 26 07:32:13 s19 systemd[1]: Removed slice system-modprobe.slice.
...

Rafael J. Wysocki Nov. 26, 2021, 5:21 p.m. UTC | #2

Hi Doug,

On Fri, Nov 26, 2021 at 6:08 PM Doug Smythies <[e-mail]> 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.

Daniel Lezcano Nov. 26, 2021, 5:40 p.m. UTC | #3

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/[e-mail]/

?

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

Daniel Lezcano Nov. 26, 2021, 5:43 p.m. UTC | #4

On 26/11/2021 18:21, Rafael J. Wysocki wrote:
> Hi Doug,
> 
> On Fri, Nov 26, 2021 at 6:08 PM Doug Smythies <[e-mail]> 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/[e-mail]/

Rafael J. Wysocki Nov. 26, 2021, 6:18 p.m. UTC | #5

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 <[e-mail]> 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/[e-mail]/

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

Rafael J. Wysocki Nov. 26, 2021, 6:23 p.m. UTC | #6

On Fri, Nov 26, 2021 at 6:40 PM Daniel Lezcano
<[e-mail]> 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/[e-mail]/
>
> ?

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

Doug Smythies Nov. 26, 2021, 7:10 p.m. UTC | #7

On Fri, Nov 26, 2021 at 9:22 AM Rafael J. Wysocki <[e-mail]> wrote:
>
> Hi Doug,
>
> On Fri, Nov 26, 2021 at 6:08 PM Doug Smythies <[e-mail]> 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;

Rafael J. Wysocki Nov. 26, 2021, 7:29 p.m. UTC | #8

On Fri, Nov 26, 2021 at 8:10 PM Doug Smythies <[e-mail]> wrote:
>
> On Fri, Nov 26, 2021 at 9:22 AM Rafael J. Wysocki <[e-mail]> wrote:
> >
> > Hi Doug,
> >
> > On Fri, Nov 26, 2021 at 6:08 PM Doug Smythies <[e-mail]> 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.

Doug Smythies Nov. 26, 2021, 9:56 p.m. UTC | #9

On Fri, Nov 26, 2021 at 9:43 AM Daniel Lezcano
<[e-mail]> wrote:
>
> On 26/11/2021 18:21, Rafael J. Wysocki wrote:
> > Hi Doug,
> >
> > On Fri, Nov 26, 2021 at 6:08 PM Doug Smythies <[e-mail]> 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/[e-mail]/
>
> --
> <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

Daniel Lezcano Nov. 26, 2021, 11:05 p.m. UTC | #10

On 26/11/2021 22:56, Doug Smythies wrote:
> On Fri, Nov 26, 2021 at 9:43 AM Daniel Lezcano
> <[e-mail]> wrote:
>>
>> On 26/11/2021 18:21, Rafael J. Wysocki wrote:
>>> Hi Doug,
>>>
>>> On Fri, Nov 26, 2021 at 6:08 PM Doug Smythies <[e-mail]> 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.

Daniel Lezcano Nov. 30, 2021, 4:46 p.m. UTC | #11

On 26/11/2021 20:29, Rafael J. Wysocki wrote:
> On Fri, Nov 26, 2021 at 8:10 PM Doug Smythies <[e-mail]> wrote:
>>
>> On Fri, Nov 26, 2021 at 9:22 AM Rafael J. Wysocki <[e-mail]> wrote:
>>>
>>> Hi Doug,
>>>
>>> On Fri, Nov 26, 2021 at 6:08 PM Doug Smythies <[e-mail]> 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/[e-mail]/

Patch

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;	\