On Fri, Sep 27, 2019 at 04:42:39AM -0400, Steven Rostedt wrote: > On Thu, 26 Sep 2019 15:04:38 -0700 > Matthias Kaehlcke <[email protected]> wrote: > > > Define genpd_power_on/off and genpd_set_performance_state > > tracepoints and use them. > > I agree with Greg about adding a "why" you need this. But, in case > there's a good reason to have this, I have comments about the code > below.
Thanks Greg and Steven for your comments. How about this instead: Add tracepoints for genpd_power_on, genpd_power_off and genpd_set_performance_state. The tracepoints can help with understanding power domain behavior of a given device, which may be particularly interesting for battery powered devices and suspend/resume. > > Signed-off-by: Matthias Kaehlcke <[email protected]> > > --- > > > > drivers/base/power/domain.c | 27 +++++++++++++++++--- > > include/trace/events/power.h | 49 ++++++++++++++++++++++++++++++++++++ > > 2 files changed, 72 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > > index cc85e87eaf05..aee988c112e5 100644 > > --- a/drivers/base/power/domain.c > > +++ b/drivers/base/power/domain.c > > @@ -21,6 +21,7 @@ > > #include <linux/suspend.h> > > #include <linux/export.h> > > #include <linux/cpu.h> > > +#include <trace/events/power.h> > > > > #include "power.h" > > > > @@ -329,6 +330,9 @@ static int _genpd_set_performance_state(struct > > generic_pm_domain *genpd, > > goto err; > > > > genpd->performance_state = state; > > + > > + trace_genpd_set_performance_state(genpd); > > + > > return 0; > > > > err: > > @@ -418,14 +422,21 @@ static int _genpd_power_on(struct generic_pm_domain > > *genpd, bool timed) > > if (!genpd->power_on) > > return 0; > > > > - if (!timed) > > - return genpd->power_on(genpd); > > + if (!timed) { > > + ret = genpd->power_on(genpd); > > + if (!ret) > > + trace_genpd_power_on(genpd); > > + > > + return ret; > > + } > > > > time_start = ktime_get(); > > ret = genpd->power_on(genpd); > > if (ret) > > return ret; > > > > + trace_genpd_power_on(genpd); > > + > > elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start)); > > if (elapsed_ns <= genpd->states[state_idx].power_on_latency_ns) > > return ret; > > @@ -448,14 +459,22 @@ static int _genpd_power_off(struct generic_pm_domain > > *genpd, bool timed) > > if (!genpd->power_off) > > return 0; > > > > - if (!timed) > > - return genpd->power_off(genpd); > > + if (!timed) { > > + ret = genpd->power_off(genpd); > > + if (!ret) > > Here > > > + trace_genpd_power_off(genpd); > > + > > + return ret; > > + } > > > > time_start = ktime_get(); > > ret = genpd->power_off(genpd); > > if (ret) > > return ret; > > > > + if (!ret) > > And here add a conditional branch for only a tracepoint. To eliminate > the branch when tracepoints are not enabled, please do it this way > instead: > > if (trace_genpd_power_off_enabled() && !ret) > > The above is a static branch (nop when disabled, and jmp when enabled), > and the above should move the conditional branch on !ret into the > section that is only called when the tracepoint is enabled. ok, will do in future versions.
