[PATCH] cpuidle: psci: Add trace for PSCI domain idle

[PATCH] cpuidle: psci: Add trace for PSCI domain idle

[Keita Morisaki]

The trace event cpu_idle provides insufficient information for debugging
PSCI requests due to lacking access to determined PSCI domain idle
states. The cpu_idle usually only shows -1, 0, or 1 regardless how many
CPUidle states the power domain has.

Add a new trace named psci_domain_idle with a determined idle state info
before the state entry, and after the state exit.

This new trace event will help developers debug CPUidle issues on ARM
systems using PSCI by providing more detailed information about the
requested idle states.

Signed-off-by: Keita Morisaki <keyz@google.com>
---
 drivers/cpuidle/cpuidle-psci.c |  3 +++
 include/trace/events/power.h   | 25 +++++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
index 2562dc001fc1..5888d4511787 100644
--- a/drivers/cpuidle/cpuidle-psci.c
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -25,6 +25,7 @@
 #include <linux/syscore_ops.h>
 
 #include <asm/cpuidle.h>
+#include <trace/events/power.h>
 
 #include "cpuidle-psci.h"
 #include "dt_idle_states.h"
@@ -74,7 +75,9 @@ static __cpuidle int __psci_enter_domain_idle_state(struct cpuidle_device *dev,
 	if (!state)
 		state = states[idx];
 
+	trace_psci_domain_idle(dev->cpu, state, true, s2idle);
 	ret = psci_cpu_suspend_enter(state) ? -1 : idx;
+	trace_psci_domain_idle(dev->cpu, state, false, s2idle);
 
 	if (s2idle)
 		dev_pm_genpd_resume(pd_dev);
diff --git a/include/trace/events/power.h b/include/trace/events/power.h
index d2349b6b531a..82ad8bb1c477 100644
--- a/include/trace/events/power.h
+++ b/include/trace/events/power.h
@@ -62,6 +62,31 @@ TRACE_EVENT(cpu_idle_miss,
 		(unsigned long)__entry->state, (__entry->below)?"below":"above")
 );
 
+TRACE_EVENT(psci_domain_idle,
+
+	TP_PROTO(unsigned int cpu_id, unsigned int state, bool enter, bool s2idle),
+
+	TP_ARGS(cpu_id, state, enter, s2idle),
+
+	TP_STRUCT__entry(
+		__field(u32,		cpu_id)
+		__field(u32,		state)
+		__field(bool,		enter)
+		__field(bool,		s2idle)
+	),
+
+	TP_fast_assign(
+		__entry->cpu_id = cpu_id;
+		__entry->state = state;
+		__entry->enter = enter;
+		__entry->s2idle = s2idle;
+	),
+
+	TP_printk("cpu_id=%lu state=0x%lx type=%s, is_s2idle=%s",
+		  (unsigned long)__entry->cpu_id, (unsigned long)__entry->state,
+		  (__entry->enter)?"enter":"exit", (__entry->s2idle)?"yes":"no")
+);
+
 TRACE_EVENT(powernv_throttle,
 
 	TP_PROTO(int chip_id, const char *reason, int pmax),

base-commit: 9bffa1ad25b8b3b95d8f463e5c24dabe3c87d54d
-- 
2.48.0.rc2.279.g1de40edade-goog

Re: [PATCH] cpuidle: psci: Add trace for PSCI domain idle

[Steven Rostedt]

On Fri, 17 Jan 2025 12:01:32 +0800
Keita Morisaki <keyz@google.com> wrote:

> @@ -74,7 +75,9 @@ static __cpuidle int __psci_enter_domain_idle_state(struct cpuidle_device *dev,
>  	if (!state)
>  		state = states[idx];
>  
> +	trace_psci_domain_idle(dev->cpu, state, true, s2idle);
>  	ret = psci_cpu_suspend_enter(state) ? -1 : idx;
> +	trace_psci_domain_idle(dev->cpu, state, false, s2idle);

Why not make that into two different events:

+	trace_psci_domain_idle_enter(dev->cpu, state, s2idle);
 	ret = psci_cpu_suspend_enter(state) ? -1 : idx;
+	trace_psci_domain_idle_exit(dev->cpu, state, s2idle);

>  
>  	if (s2idle)
>  		dev_pm_genpd_resume(pd_dev);
> diff --git a/include/trace/events/power.h b/include/trace/events/power.h
> index d2349b6b531a..82ad8bb1c477 100644
> --- a/include/trace/events/power.h
> +++ b/include/trace/events/power.h
> @@ -62,6 +62,31 @@ TRACE_EVENT(cpu_idle_miss,
>  		(unsigned long)__entry->state, (__entry->below)?"below":"above")
>  );
>  
> +TRACE_EVENT(psci_domain_idle,
> +
> +	TP_PROTO(unsigned int cpu_id, unsigned int state, bool enter, bool s2idle),
> +
> +	TP_ARGS(cpu_id, state, enter, s2idle),
> +
> +	TP_STRUCT__entry(
> +		__field(u32,		cpu_id)
> +		__field(u32,		state)
> +		__field(bool,		enter)
> +		__field(bool,		s2idle)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->cpu_id = cpu_id;
> +		__entry->state = state;
> +		__entry->enter = enter;
> +		__entry->s2idle = s2idle;
> +	),
> +
> +	TP_printk("cpu_id=%lu state=0x%lx type=%s, is_s2idle=%s",
> +		  (unsigned long)__entry->cpu_id, (unsigned long)__entry->state,
> +		  (__entry->enter)?"enter":"exit", (__entry->s2idle)?"yes":"no")
> +);
> +

Then make the above into a DECLARE_EVENT_CLASS:

DECLARE_EVENT_CLASS(psci_domain_idle_template,

	TP_PROTO(unsigned int cpu_id, unsigned int state, bool s2idle),

	TP_ARGS(cpu_id, state, s2idle),

	TP_STRUCT__entry(
		__field(u32,		cpu_id)
		__field(u32,		state)
		__field(bool,		s2idle)
	),

	TP_fast_assign(
		__entry->cpu_id = cpu_id;
		__entry->state = state;
		__entry->s2idle = s2idle;
	),

	TP_printk("cpu_id=%lu state=0x%lx type=%s, is_s2idle=%s",
		  (unsigned long)__entry->cpu_id, (unsigned long)__entry->state,
		  (__entry->s2idle)?"yes":"no")
);


DEFINE_EVENT(psci_domain_idle_template, psci_domain_idle_enter,

	TP_PROTO(unsigned int cpu_id, unsigned int state, bool s2idle),

	TP_ARGS(cpu_id, state, s2idle),
);

DEFINE_EVENT(psci_domain_idle_template, psci_domain_idle_exit,

	TP_PROTO(unsigned int cpu_id, unsigned int state, bool s2idle),

	TP_ARGS(cpu_id, state, s2idle),
);

And then you could easily attach synthetic events to them to get the
timings and such.

-- Steve

Re: [PATCH] cpuidle: psci: Add trace for PSCI domain idle

[Keita Morisaki]

Thank you for the review!

> Why not make that into two different events:

> +	trace_psci_domain_idle_enter(dev->cpu, state, s2idle);
> 	ret = psci_cpu_suspend_enter(state) ? -1 : idx;
> +	trace_psci_domain_idle_exit(dev->cpu, state, s2idle);

> Then make the above into a DECLARE_EVENT_CLASS:

> DECLARE_EVENT_CLASS(psci_domain_idle_template,

> 	TP_PROTO(unsigned int cpu_id, unsigned int state, bool s2idle),

> 	TP_ARGS(cpu_id, state, s2idle),

> 	TP_STRUCT__entry(
> 		__field(u32,		cpu_id)
> 		__field(u32,		state)
> 		__field(bool,		s2idle)
> 	),

> 	TP_fast_assign(
> 		__entry->cpu_id = cpu_id;
> 		__entry->state = state;
> 		__entry->s2idle = s2idle;
> 	),

> 	TP_printk("cpu_id=%lu state=0x%lx type=%s, is_s2idle=%s",
> 		  (unsigned long)__entry->cpu_id, (unsigned long)__entry->state,
> 		  (__entry->s2idle)?"yes":"no")
> );


> DEFINE_EVENT(psci_domain_idle_template, psci_domain_idle_enter,

> 	TP_PROTO(unsigned int cpu_id, unsigned int state, bool s2idle),

> 	TP_ARGS(cpu_id, state, s2idle),
> );

> DEFINE_EVENT(psci_domain_idle_template, psci_domain_idle_exit,

> 	TP_PROTO(unsigned int cpu_id, unsigned int state, bool s2idle),

> 	TP_ARGS(cpu_id, state, s2idle),
> );

Looks good. Let me apply this change in v2.

Thanks,
Keita

[PATCH v2] cpuidle: psci: Add trace for PSCI domain idle

[Keita Morisaki]

The trace event cpu_idle provides insufficient information for debugging
PSCI requests due to lacking access to determined PSCI domain idle
states. The cpu_idle usually only shows -1, 0, or 1 regardless how many
idle states the power domain has.

Add new trace events namely psci_domain_idle_enter and
psci_domain_idle_exit to trace enter and exit events with a determined
idle state.

These new trace events will help developers debug CPUidle issues on ARM
systems using PSCI by providing more detailed information about the
requested idle states.

Signed-off-by: Keita Morisaki <keyz@google.com>
---
v1->v2: Split the ftrace event into two (psci_domain_idle_(enter|exit))
		and rephrase the commit message accordingly. Rebased onto the latest.

 drivers/cpuidle/cpuidle-psci.c |  3 +++
 include/trace/events/power.h   | 37 ++++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
index 2562dc001fc1..dd8d776d6e39 100644
--- a/drivers/cpuidle/cpuidle-psci.c
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -25,6 +25,7 @@
 #include <linux/syscore_ops.h>

 #include <asm/cpuidle.h>
+#include <trace/events/power.h>

 #include "cpuidle-psci.h"
 #include "dt_idle_states.h"
@@ -74,7 +75,9 @@ static __cpuidle int __psci_enter_domain_idle_state(struct cpuidle_device *dev,
 	if (!state)
 		state = states[idx];

+	trace_psci_domain_idle_enter(dev->cpu, state, s2idle);
 	ret = psci_cpu_suspend_enter(state) ? -1 : idx;
+	trace_psci_domain_idle_exit(dev->cpu, state, s2idle);

 	if (s2idle)
 		dev_pm_genpd_resume(pd_dev);
diff --git a/include/trace/events/power.h b/include/trace/events/power.h
index d2349b6b531a..9253e83b9bb4 100644
--- a/include/trace/events/power.h
+++ b/include/trace/events/power.h
@@ -62,6 +62,43 @@ TRACE_EVENT(cpu_idle_miss,
 		(unsigned long)__entry->state, (__entry->below)?"below":"above")
 );

+DECLARE_EVENT_CLASS(psci_domain_idle,
+
+	TP_PROTO(unsigned int cpu_id, unsigned int state, bool s2idle),
+
+	TP_ARGS(cpu_id, state, s2idle),
+
+	TP_STRUCT__entry(
+		__field(u32,		cpu_id)
+		__field(u32,		state)
+		__field(bool,		s2idle)
+	),
+
+	TP_fast_assign(
+		__entry->cpu_id = cpu_id;
+		__entry->state = state;
+		__entry->s2idle = s2idle;
+	),
+
+	TP_printk("cpu_id=%lu state=0x%lx is_s2idle=%s",
+		  (unsigned long)__entry->cpu_id, (unsigned long)__entry->state,
+		  (__entry->s2idle)?"yes":"no")
+);
+
+DEFINE_EVENT(psci_domain_idle, psci_domain_idle_enter,
+
+	TP_PROTO(unsigned int cpu_id, unsigned int state, bool s2idle),
+
+	TP_ARGS(cpu_id, state, s2idle)
+);
+
+DEFINE_EVENT(psci_domain_idle, psci_domain_idle_exit,
+
+	TP_PROTO(unsigned int cpu_id, unsigned int state, bool s2idle),
+
+	TP_ARGS(cpu_id, state, s2idle)
+);
+
 TRACE_EVENT(powernv_throttle,

 	TP_PROTO(int chip_id, const char *reason, int pmax),

base-commit: ffd294d346d185b70e28b1a28abe367bbfe53c04
--
2.48.0.rc2.279.g1de40edade-goog

Re: [PATCH v2] cpuidle: psci: Add trace for PSCI domain idle

[Steven Rostedt]

On Mon, 20 Jan 2025 09:36:16 +0800
Keita Morisaki <keyz@google.com> wrote:

> The trace event cpu_idle provides insufficient information for debugging
> PSCI requests due to lacking access to determined PSCI domain idle
> states. The cpu_idle usually only shows -1, 0, or 1 regardless how many
> idle states the power domain has.
> 
> Add new trace events namely psci_domain_idle_enter and
> psci_domain_idle_exit to trace enter and exit events with a determined
> idle state.
> 
> These new trace events will help developers debug CPUidle issues on ARM
> systems using PSCI by providing more detailed information about the
> requested idle states.
> 
> Signed-off-by: Keita Morisaki <keyz@google.com>
> ---
> v1->v2: Split the ftrace event into two (psci_domain_idle_(enter|exit))
> 		and rephrase the commit message accordingly. Rebased onto the latest.
> 
>  drivers/cpuidle/cpuidle-psci.c |  3 +++
>  include/trace/events/power.h   | 37 ++++++++++++++++++++++++++++++++++
>  2 files changed, 40 insertions(+)

From the tracing point of view, there's nothing that sticks out that is incorrect.

Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>

As for if it should be applied, that's for the maintainers of the subsystem
that it exists in.

-- Steve

Re: [PATCH v2] cpuidle: psci: Add trace for PSCI domain idle

[Keita Morisaki]

> Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>

> As for if it should be applied, that's for the maintainers of the subsystem
> that it exists in.

Thank you so much!

[PATCH v3] cpuidle: psci: Add trace for PSCI domain idle

[Keita Morisaki]

The trace event cpu_idle provides insufficient information for debugging
PSCI requests due to lacking access to determined PSCI domain idle
states. The cpu_idle usually only shows -1, 0, or 1 regardless how many
idle states the power domain has.

Add new trace events namely psci_domain_idle_enter and
psci_domain_idle_exit to trace enter and exit events with a determined
idle state.

These new trace events will help developers debug CPUidle issues on ARM
systems using PSCI by providing more detailed information about the
requested idle states.

Signed-off-by: Keita Morisaki <keyz@google.com>
Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
v1->v2: Split the ftrace event into two (psci_domain_idle_(enter|exit))
        and rephrase the commit message accordingly. Rebased onto the latest.
v2->v3: Add the Reviewed-by label

 drivers/cpuidle/cpuidle-psci.c |  3 +++
 include/trace/events/power.h   | 37 ++++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
index 2562dc001fc1..dd8d776d6e39 100644
--- a/drivers/cpuidle/cpuidle-psci.c
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -25,6 +25,7 @@
 #include <linux/syscore_ops.h>

 #include <asm/cpuidle.h>
+#include <trace/events/power.h>

 #include "cpuidle-psci.h"
 #include "dt_idle_states.h"
@@ -74,7 +75,9 @@ static __cpuidle int __psci_enter_domain_idle_state(struct cpuidle_device *dev,
 	if (!state)
 		state = states[idx];

+	trace_psci_domain_idle_enter(dev->cpu, state, s2idle);
 	ret = psci_cpu_suspend_enter(state) ? -1 : idx;
+	trace_psci_domain_idle_exit(dev->cpu, state, s2idle);

 	if (s2idle)
 		dev_pm_genpd_resume(pd_dev);
diff --git a/include/trace/events/power.h b/include/trace/events/power.h
index d2349b6b531a..9253e83b9bb4 100644
--- a/include/trace/events/power.h
+++ b/include/trace/events/power.h
@@ -62,6 +62,43 @@ TRACE_EVENT(cpu_idle_miss,
 		(unsigned long)__entry->state, (__entry->below)?"below":"above")
 );

+DECLARE_EVENT_CLASS(psci_domain_idle,
+
+	TP_PROTO(unsigned int cpu_id, unsigned int state, bool s2idle),
+
+	TP_ARGS(cpu_id, state, s2idle),
+
+	TP_STRUCT__entry(
+		__field(u32,		cpu_id)
+		__field(u32,		state)
+		__field(bool,		s2idle)
+	),
+
+	TP_fast_assign(
+		__entry->cpu_id = cpu_id;
+		__entry->state = state;
+		__entry->s2idle = s2idle;
+	),
+
+	TP_printk("cpu_id=%lu state=0x%lx is_s2idle=%s",
+		  (unsigned long)__entry->cpu_id, (unsigned long)__entry->state,
+		  (__entry->s2idle)?"yes":"no")
+);
+
+DEFINE_EVENT(psci_domain_idle, psci_domain_idle_enter,
+
+	TP_PROTO(unsigned int cpu_id, unsigned int state, bool s2idle),
+
+	TP_ARGS(cpu_id, state, s2idle)
+);
+
+DEFINE_EVENT(psci_domain_idle, psci_domain_idle_exit,
+
+	TP_PROTO(unsigned int cpu_id, unsigned int state, bool s2idle),
+
+	TP_ARGS(cpu_id, state, s2idle)
+);
+
 TRACE_EVENT(powernv_throttle,

 	TP_PROTO(int chip_id, const char *reason, int pmax),

base-commit: ffd294d346d185b70e28b1a28abe367bbfe53c04
--
2.48.1.262.g85cc9f2d1e-goog

Re: [PATCH v2] cpuidle: psci: Add trace for PSCI domain idle

[Christian Loehle]

On 1/20/25 01:36, Keita Morisaki wrote:
> The trace event cpu_idle provides insufficient information for debugging
> PSCI requests due to lacking access to determined PSCI domain idle
> states. The cpu_idle usually only shows -1, 0, or 1 regardless how many
> idle states the power domain has.
> 
> Add new trace events namely psci_domain_idle_enter and
> psci_domain_idle_exit to trace enter and exit events with a determined
> idle state.
> 
> These new trace events will help developers debug CPUidle issues on ARM
> systems using PSCI by providing more detailed information about the
> requested idle states.
> 
> Signed-off-by: Keita Morisaki <keyz@google.com>
> ---
> v1->v2: Split the ftrace event into two (psci_domain_idle_(enter|exit))
> 		and rephrase the commit message accordingly. Rebased onto the latest.


Which makes it different to cpu_idle event FWIW.

> 
>  drivers/cpuidle/cpuidle-psci.c |  3 +++
>  include/trace/events/power.h   | 37 ++++++++++++++++++++++++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> index 2562dc001fc1..dd8d776d6e39 100644
> --- a/drivers/cpuidle/cpuidle-psci.c
> +++ b/drivers/cpuidle/cpuidle-psci.c
> @@ -25,6 +25,7 @@
>  #include <linux/syscore_ops.h>
> 
>  #include <asm/cpuidle.h>
> +#include <trace/events/power.h>
> 
>  #include "cpuidle-psci.h"
>  #include "dt_idle_states.h"
> @@ -74,7 +75,9 @@ static __cpuidle int __psci_enter_domain_idle_state(struct cpuidle_device *dev,
>  	if (!state)
>  		state = states[idx];
> 
> +	trace_psci_domain_idle_enter(dev->cpu, state, s2idle);
>  	ret = psci_cpu_suspend_enter(state) ? -1 : idx;
> +	trace_psci_domain_idle_exit(dev->cpu, state, s2idle);

Not tracking ret seems odd, is that fine?

Re: [PATCH v2] cpuidle: psci: Add trace for PSCI domain idle

[Keita Morisaki]

> > The trace event cpu_idle provides insufficient information for debugging
> > PSCI requests due to lacking access to determined PSCI domain idle
> > states. The cpu_idle usually only shows -1, 0, or 1 regardless how many
> > idle states the power domain has.
> >
> > Add new trace events namely psci_domain_idle_enter and
> > psci_domain_idle_exit to trace enter and exit events with a determined
> > idle state.
> >
> > These new trace events will help developers debug CPUidle issues on ARM
> > systems using PSCI by providing more detailed information about the
> > requested idle states.
> >
> > Signed-off-by: Keita Morisaki <keyz@google.com>
> > ---
> > v1->v2: Split the ftrace event into two (psci_domain_idle_(enter|exit))
> > 		and rephrase the commit message accordingly. Rebased onto the latest.
> Which makes it different to cpu_idle event FWIW.

Yes, psci_domain_idle_(enter|exit) are not meant to replace cpu_idle nor a
variant of it. It's new and different events that provide finer=grained info.

> >  drivers/cpuidle/cpuidle-psci.c |  3 +++
> >  include/trace/events/power.h   | 37 ++++++++++++++++++++++++++++++++++
> >  2 files changed, 40 insertions(+)
> >
> > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> > index 2562dc001fc1..dd8d776d6e39 100644
> > --- a/drivers/cpuidle/cpuidle-psci.c
> > +++ b/drivers/cpuidle/cpuidle-psci.c
> > @@ -25,6 +25,7 @@
> >  #include <linux/syscore_ops.h>
> >
> >  #include <asm/cpuidle.h>
> > +#include <trace/events/power.h>
> >
> >  #include "cpuidle-psci.h"
> >  #include "dt_idle_states.h"
> > @@ -74,7 +75,9 @@ static __cpuidle int __psci_enter_domain_idle_state(struct cpuidle_device *dev,
> >  	if (!state)
> >  		state = states[idx];
> >
> > +	trace_psci_domain_idle_enter(dev->cpu, state, s2idle);
> >  	ret = psci_cpu_suspend_enter(state) ? -1 : idx;
> > +	trace_psci_domain_idle_exit(dev->cpu, state, s2idle);
> Not tracking ret seems odd, is that fine?

I think it's fine not to track ret here.
__psci_enter_domain_idle_state does not seems to care the return value of
psci_cpu_suspend_enter to me because it just proceeds with executing subsequent
functions regardless of ret, and returns ret to the higher function. If the
value should be traced, it should probably be done in a lower layer or a higher
layer.

Another small small reason I'm not interested in adding ret to the
trace_psci_domain_idle_exit's arguments is that
trace_psci_domain_idle_(enter|exit) currently share the same trace event
(i.e. same set of arguments) and it makes the trace events simple.

Re: [PATCH v2] cpuidle: psci: Add trace for PSCI domain idle

[Christian Loehle]

On 1/28/25 04:24, Keita Morisaki wrote:
>>> The trace event cpu_idle provides insufficient information for debugging
>>> PSCI requests due to lacking access to determined PSCI domain idle
>>> states. The cpu_idle usually only shows -1, 0, or 1 regardless how many
>>> idle states the power domain has.
>>>
>>> Add new trace events namely psci_domain_idle_enter and
>>> psci_domain_idle_exit to trace enter and exit events with a determined
>>> idle state.
>>>
>>> These new trace events will help developers debug CPUidle issues on ARM
>>> systems using PSCI by providing more detailed information about the
>>> requested idle states.
>>>
>>> Signed-off-by: Keita Morisaki <keyz@google.com>
>>> ---
>>> v1->v2: Split the ftrace event into two (psci_domain_idle_(enter|exit))
>>> 		and rephrase the commit message accordingly. Rebased onto the latest.
>> Which makes it different to cpu_idle event FWIW.
> 
> Yes, psci_domain_idle_(enter|exit) are not meant to replace cpu_idle nor a
> variant of it. It's new and different events that provide finer=grained info.
> 

I mentioned it because it means it doesn't benefit from cpu_idle tooling
directly, which is slightly odd, but fine with me.

Re: [PATCH v2] cpuidle: psci: Add trace for PSCI domain idle

[Keita Morisaki]

> > Yes, psci_domain_idle_(enter|exit) are not meant to replace cpu_idle nor a
> > variant of it. It's new and different events that provide finer=grained info.

> I mentioned it because it means it doesn't benefit from cpu_idle tooling
> directly, which is slightly odd, but fine with me.

I might not fully understand your comments.
Do you mean that even mentioning cpu_idle in the commit message does not feel
right to you, or utilizing cpu_idle by exposing the determined state instead of
adding new trace events is the right direction?

Re: [PATCH v3] cpuidle: psci: Add trace for PSCI domain idle

[Kevin Hilman]

Keita Morisaki <keyz@google.com> writes:

> The trace event cpu_idle provides insufficient information for debugging
> PSCI requests due to lacking access to determined PSCI domain idle
> states. The cpu_idle usually only shows -1, 0, or 1 regardless how many
> idle states the power domain has.
>
> Add new trace events namely psci_domain_idle_enter and
> psci_domain_idle_exit to trace enter and exit events with a determined
> idle state.
>
> These new trace events will help developers debug CPUidle issues on ARM
> systems using PSCI by providing more detailed information about the
> requested idle states.
>
> Signed-off-by: Keita Morisaki <keyz@google.com>
> Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>

Tested-by: Kevin Hilman <khilman@baylibre.com>

I've been using some local trace_printk() to do exactly this, so I fully
support having some official tracepoints here. 

For my local hacks, I was trackin the state index as well as the state
value since for quick debug, I find the index to more human readable
than the state value, which I have to compare with the
arm,psci-suspend-pararm from the DT.

Anyways, thanks for submitting this!

Kevin

Re: [PATCH v3] cpuidle: psci: Add trace for PSCI domain idle

[Keita Morisaki]

> Tested-by: Kevin Hilman <khilman@baylibre.com>
>
> I've been using some local trace_printk() to do exactly this, so I fully
> support having some official tracepoints here.
>
> For my local hacks, I was trackin the state index as well as the state
> value since for quick debug, I find the index to more human readable
> than the state value, which I have to compare with the
> arm,psci-suspend-pararm from the DT.
>
> Anyways, thanks for submitting this!
>
> Kevin

Thank you so much for the endorsement.
Showing the index sounds like a good idea, although it should be done in a
different layer (probably in the GenPD governor or cpuidle-psci driver?) because
idx here should be the different index from the one we're looking for.
I will keep my patch as it is just for now. Thank you for the idea!

[PATCH v4] cpuidle: psci: Add trace for PSCI domain idle

[Keita Morisaki]

The trace event cpu_idle provides insufficient information for debugging
PSCI requests due to lacking access to determined PSCI domain idle
states. The cpu_idle usually only shows -1, 0, or 1 regardless how many
idle states the power domain has.

Add new trace events namely psci_domain_idle_enter and
psci_domain_idle_exit to trace enter and exit events with a determined
idle state.

These new trace events will help developers debug CPUidle issues on ARM
systems using PSCI by providing more detailed information about the
requested idle states.

Signed-off-by: Keita Morisaki <keyz@google.com>
Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>
Tested-by: Kevin Hilman <khilman@baylibre.com>
---
v1->v2: Split the ftrace event into two (psci_domain_idle_(enter|exit))
        and rephrase the commit message accordingly. Rebased onto the latest.
v2->v3: Add the Reviewed-by label
v3->v4: Add the Tested-by label

 drivers/cpuidle/cpuidle-psci.c |  3 +++
 include/trace/events/power.h   | 37 ++++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
index 2562dc001fc1..dd8d776d6e39 100644
--- a/drivers/cpuidle/cpuidle-psci.c
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -25,6 +25,7 @@
 #include <linux/syscore_ops.h>

 #include <asm/cpuidle.h>
+#include <trace/events/power.h>

 #include "cpuidle-psci.h"
 #include "dt_idle_states.h"
@@ -74,7 +75,9 @@ static __cpuidle int __psci_enter_domain_idle_state(struct cpuidle_device *dev,
 	if (!state)
 		state = states[idx];

+	trace_psci_domain_idle_enter(dev->cpu, state, s2idle);
 	ret = psci_cpu_suspend_enter(state) ? -1 : idx;
+	trace_psci_domain_idle_exit(dev->cpu, state, s2idle);

 	if (s2idle)
 		dev_pm_genpd_resume(pd_dev);
diff --git a/include/trace/events/power.h b/include/trace/events/power.h
index d2349b6b531a..9253e83b9bb4 100644
--- a/include/trace/events/power.h
+++ b/include/trace/events/power.h
@@ -62,6 +62,43 @@ TRACE_EVENT(cpu_idle_miss,
 		(unsigned long)__entry->state, (__entry->below)?"below":"above")
 );

+DECLARE_EVENT_CLASS(psci_domain_idle,
+
+	TP_PROTO(unsigned int cpu_id, unsigned int state, bool s2idle),
+
+	TP_ARGS(cpu_id, state, s2idle),
+
+	TP_STRUCT__entry(
+		__field(u32,		cpu_id)
+		__field(u32,		state)
+		__field(bool,		s2idle)
+	),
+
+	TP_fast_assign(
+		__entry->cpu_id = cpu_id;
+		__entry->state = state;
+		__entry->s2idle = s2idle;
+	),
+
+	TP_printk("cpu_id=%lu state=0x%lx is_s2idle=%s",
+		  (unsigned long)__entry->cpu_id, (unsigned long)__entry->state,
+		  (__entry->s2idle)?"yes":"no")
+);
+
+DEFINE_EVENT(psci_domain_idle, psci_domain_idle_enter,
+
+	TP_PROTO(unsigned int cpu_id, unsigned int state, bool s2idle),
+
+	TP_ARGS(cpu_id, state, s2idle)
+);
+
+DEFINE_EVENT(psci_domain_idle, psci_domain_idle_exit,
+
+	TP_PROTO(unsigned int cpu_id, unsigned int state, bool s2idle),
+
+	TP_ARGS(cpu_id, state, s2idle)
+);
+
 TRACE_EVENT(powernv_throttle,

 	TP_PROTO(int chip_id, const char *reason, int pmax),

base-commit: ffd294d346d185b70e28b1a28abe367bbfe53c04
--
2.48.1.362.g079036d154-goog

Re: [PATCH v4] cpuidle: psci: Add trace for PSCI domain idle

[Dhruva Gole]

Hi Keita,

On Feb 02, 2025 at 18:46:08 +0800, Keita Morisaki wrote:
> The trace event cpu_idle provides insufficient information for debugging
> PSCI requests due to lacking access to determined PSCI domain idle
> states. The cpu_idle usually only shows -1, 0, or 1 regardless how many
> idle states the power domain has.
> 
> Add new trace events namely psci_domain_idle_enter and
> psci_domain_idle_exit to trace enter and exit events with a determined
> idle state.

Thanks for this, will really ease those psci idle debugs for everyone!

> 
> These new trace events will help developers debug CPUidle issues on ARM
> systems using PSCI by providing more detailed information about the
> requested idle states.
> 
> Signed-off-by: Keita Morisaki <keyz@google.com>
> Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> Tested-by: Kevin Hilman <khilman@baylibre.com>

Reviewed-by: Dhruva Gole <d-gole@ti.com>


-- 
Best regards,
Dhruva Gole
Texas Instruments Incorporated