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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250128042445.24920-1-keyz@google.com>
Date: Tue, 28 Jan 2025 12:24:44 +0800
From: Keita Morisaki <keyz@...gle.com>
To: christian.loehle@....com
Cc: aarontian@...gle.com, daniel.lezcano@...aro.org, keyz@...gle.com, 
	linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org, 
	linux-trace-kernel@...r.kernel.org, lpieralisi@...nel.org, 
	mathieu.desnoyers@...icios.com, mhiramat@...nel.org, rafael@...nel.org, 
	rostedt@...dmis.org, sudeep.holla@....com, yimingtseng@...gle.com
Subject: Re: [PATCH v2] cpuidle: psci: Add trace for PSCI domain idle

> > 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@...gle.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.