[PATCH v1] PM / Domains: Add tracepoints

[PATCH v1] PM / Domains: Add tracepoints

[Matthias Kaehlcke]

Define genpd_power_on/off and genpd_set_performance_state
tracepoints and use them.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---

 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)
+			trace_genpd_power_off(genpd);
+
+		return ret;
+	}
 
 	time_start = ktime_get();
 	ret = genpd->power_off(genpd);
 	if (ret)
 		return ret;
 
+	if (!ret)
+		trace_genpd_power_off(genpd);
+
 	elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
 	if (elapsed_ns <= genpd->states[state_idx].power_off_latency_ns)
 		return 0;
diff --git a/include/trace/events/power.h b/include/trace/events/power.h
index 7457e238e1b7..de56cd1e8d0d 100644
--- a/include/trace/events/power.h
+++ b/include/trace/events/power.h
@@ -7,6 +7,7 @@
 
 #include <linux/cpufreq.h>
 #include <linux/ktime.h>
+#include <linux/pm_domain.h>
 #include <linux/pm_qos.h>
 #include <linux/tracepoint.h>
 #include <linux/trace_events.h>
@@ -525,6 +526,54 @@ DEFINE_EVENT(dev_pm_qos_request, dev_pm_qos_remove_request,
 
 	TP_ARGS(name, type, new_value)
 );
+
+#ifdef CONFIG_PM_GENERIC_DOMAINS
+DECLARE_EVENT_CLASS(genpd_power_on_off,
+	TP_PROTO(struct generic_pm_domain *genpd),
+
+	TP_ARGS(genpd),
+
+	TP_STRUCT__entry(
+		__string(name, genpd->name)
+	),
+
+	TP_fast_assign(
+		__assign_str(name, genpd->name);
+	),
+
+	TP_printk("name=%s", __get_str(name))
+);
+
+DEFINE_EVENT(genpd_power_on_off, genpd_power_on,
+	TP_PROTO(struct generic_pm_domain *genpd),
+
+	TP_ARGS(genpd)
+);
+
+DEFINE_EVENT(genpd_power_on_off, genpd_power_off,
+	TP_PROTO(struct generic_pm_domain *genpd),
+
+	TP_ARGS(genpd)
+);
+
+TRACE_EVENT(genpd_set_performance_state,
+	TP_PROTO(struct generic_pm_domain *genpd),
+
+	TP_ARGS(genpd),
+
+	TP_STRUCT__entry(
+		__string(name, genpd->name)
+		__field(unsigned int, state)
+	),
+
+	TP_fast_assign(
+		__assign_str(name, genpd->name);
+		__entry->state = genpd->performance_state;
+	),
+
+	TP_printk("name=%s state=%u", __get_str(name), __entry->state)
+);
+#endif /* CONFIG_PM_GENERIC_DOMAINS */
 #endif /* _TRACE_POWER_H */
 
 /* This part must be outside protection */
-- 
2.23.0.444.g18eeb5a265-goog

Re: [PATCH v1] PM / Domains: Add tracepoints

[Greg Kroah-Hartman]

On Thu, Sep 26, 2019 at 03:04:38PM -0700, Matthias Kaehlcke wrote:
> Define genpd_power_on/off and genpd_set_performance_state
> tracepoints and use them.

This says _what_ you do, but not _why_ you want to do this.  Who is
going to use this?  What can be done with this information that is
needed by anyone/someone?

More details please.

thanks,

greg k-h

Re: [PATCH v1] PM / Domains: Add tracepoints

[Steven Rostedt]

On Thu, 26 Sep 2019 15:04:38 -0700
Matthias Kaehlcke <mka@chromium.org> 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.

> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> 
>  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.


> +		trace_genpd_power_off(genpd);
> +

-- Steve

>  	elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
>  	if (elapsed_ns <= genpd->states[state_idx].power_off_latency_ns)
>  		return 0;
> diff --git a/include/trace/events/power.h b/include/trace/events/power.h
> index 7457e238e1b7..de56cd1e8d0d 100644
> --- a/include/trace/events/power.h
> +++ b/include/trace/events/power.h
> @@ -7,6 +7,7 @@
>  
>  #include <linux/cpufreq.h>
>  #include <linux/ktime.h>
> +#include <linux/pm_domain.h>
>  #include <linux/pm_qos.h>
>  #include <linux/tracepoint.h>
>  #include <linux/trace_events.h>
> @@ -525,6 +526,54 @@ DEFINE_EVENT(dev_pm_qos_request, dev_pm_qos_remove_request,
>  
>  	TP_ARGS(name, type, new_value)
>  );
> +
> +#ifdef CONFIG_PM_GENERIC_DOMAINS
> +DECLARE_EVENT_CLASS(genpd_power_on_off,
> +	TP_PROTO(struct generic_pm_domain *genpd),
> +
> +	TP_ARGS(genpd),
> +
> +	TP_STRUCT__entry(
> +		__string(name, genpd->name)
> +	),
> +
> +	TP_fast_assign(
> +		__assign_str(name, genpd->name);
> +	),
> +
> +	TP_printk("name=%s", __get_str(name))
> +);
> +
> +DEFINE_EVENT(genpd_power_on_off, genpd_power_on,
> +	TP_PROTO(struct generic_pm_domain *genpd),
> +
> +	TP_ARGS(genpd)
> +);
> +
> +DEFINE_EVENT(genpd_power_on_off, genpd_power_off,
> +	TP_PROTO(struct generic_pm_domain *genpd),
> +
> +	TP_ARGS(genpd)
> +);
> +
> +TRACE_EVENT(genpd_set_performance_state,
> +	TP_PROTO(struct generic_pm_domain *genpd),
> +
> +	TP_ARGS(genpd),
> +
> +	TP_STRUCT__entry(
> +		__string(name, genpd->name)
> +		__field(unsigned int, state)
> +	),
> +
> +	TP_fast_assign(
> +		__assign_str(name, genpd->name);
> +		__entry->state = genpd->performance_state;
> +	),
> +
> +	TP_printk("name=%s state=%u", __get_str(name), __entry->state)
> +);
> +#endif /* CONFIG_PM_GENERIC_DOMAINS */
>  #endif /* _TRACE_POWER_H */
>  
>  /* This part must be outside protection */

Re: [PATCH v1] PM / Domains: Add tracepoints

[Matthias Kaehlcke]

On Fri, Sep 27, 2019 at 04:42:39AM -0400, Steven Rostedt wrote:
> On Thu, 26 Sep 2019 15:04:38 -0700
> Matthias Kaehlcke <mka@chromium.org> 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 <mka@chromium.org>
> > ---
> > 
> >  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.

Re: [PATCH v1] PM / Domains: Add tracepoints

[Steven Rostedt]

On Tue, 1 Oct 2019 09:35:42 -0700
Matthias Kaehlcke <mka@chromium.org> wrote:

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

Do you have a use case example to present?

Thanks!

-- Steve

Re: [PATCH v1] PM / Domains: Add tracepoints

[Matthias Kaehlcke]

On Tue, Oct 01, 2019 at 01:03:43PM -0400, Steven Rostedt wrote:
> On Tue, 1 Oct 2019 09:35:42 -0700
> Matthias Kaehlcke <mka@chromium.org> wrote:
> 
> > 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.
> 
> Do you have a use case example to present?

TBH I'm not looking into a specific use case right now. While
peeking around in /sys/kernel/debug/tracing/events to learn more
about existing tracepoints that might be relevant for my work
I noticed the absence of genpd ones and it seemed a good idea to
add them preemptively. Conceptually they seem similar to the
existing regulator_enable/disable and cpu_idle tracepoints.

As an abstract use case I could see power analysis on battery
powered devices during suspend. genpd_power_on/off allow to see
which power domains remain on during suspend, and might give
insights for possible power saving options. Examples could be that
a power domain stays unexpectedly on due to a misconfiguration, or
two power domains remain on when it could be only one if you just
moved that one pin/port over to the other domain in the next
hardware revision.

Re: [PATCH v1] PM / Domains: Add tracepoints

[Steven Rostedt]

On Tue, 1 Oct 2019 10:42:35 -0700
Matthias Kaehlcke <mka@chromium.org> wrote:

> On Tue, Oct 01, 2019 at 01:03:43PM -0400, Steven Rostedt wrote:
> > On Tue, 1 Oct 2019 09:35:42 -0700
> > Matthias Kaehlcke <mka@chromium.org> wrote:
> >   
> > > 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.  
> > 
> > Do you have a use case example to present?  
> 
> TBH I'm not looking into a specific use case right now. While
> peeking around in /sys/kernel/debug/tracing/events to learn more
> about existing tracepoints that might be relevant for my work
> I noticed the absence of genpd ones and it seemed a good idea to
> add them preemptively. Conceptually they seem similar to the
> existing regulator_enable/disable and cpu_idle tracepoints.
> 
> As an abstract use case I could see power analysis on battery
> powered devices during suspend. genpd_power_on/off allow to see
> which power domains remain on during suspend, and might give
> insights for possible power saving options. Examples could be that
> a power domain stays unexpectedly on due to a misconfiguration, or
> two power domains remain on when it could be only one if you just
> moved that one pin/port over to the other domain in the next
> hardware revision.

If the power management maintainers have no issues with adding these,
then neither do I ;-)  It would be them who would pull them in anyway.

-- Steve

Re: [PATCH v1] PM / Domains: Add tracepoints

[Matthias Kaehlcke]

On Tue, Oct 01, 2019 at 02:08:46PM -0400, Steven Rostedt wrote:
> On Tue, 1 Oct 2019 10:42:35 -0700
> Matthias Kaehlcke <mka@chromium.org> wrote:
> 
> > On Tue, Oct 01, 2019 at 01:03:43PM -0400, Steven Rostedt wrote:
> > > On Tue, 1 Oct 2019 09:35:42 -0700
> > > Matthias Kaehlcke <mka@chromium.org> wrote:
> > >   
> > > > 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.  
> > > 
> > > Do you have a use case example to present?  
> > 
> > TBH I'm not looking into a specific use case right now. While
> > peeking around in /sys/kernel/debug/tracing/events to learn more
> > about existing tracepoints that might be relevant for my work
> > I noticed the absence of genpd ones and it seemed a good idea to
> > add them preemptively. Conceptually they seem similar to the
> > existing regulator_enable/disable and cpu_idle tracepoints.
> > 
> > As an abstract use case I could see power analysis on battery
> > powered devices during suspend. genpd_power_on/off allow to see
> > which power domains remain on during suspend, and might give
> > insights for possible power saving options. Examples could be that
> > a power domain stays unexpectedly on due to a misconfiguration, or
> > two power domains remain on when it could be only one if you just
> > moved that one pin/port over to the other domain in the next
> > hardware revision.
> 
> If the power management maintainers have no issues with adding these,
> then neither do I ;-)  It would be them who would pull them in anyway.

Ok, I'll send a new version with the changes you suggested and some more
info in the commit message, unless PM maintainers raise concerns before
that.

Re: [PATCH v1] PM / Domains: Add tracepoints

[Rafael J. Wysocki]

On Tue, Oct 1, 2019 at 9:37 PM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> On Tue, Oct 01, 2019 at 02:08:46PM -0400, Steven Rostedt wrote:
> > On Tue, 1 Oct 2019 10:42:35 -0700
> > Matthias Kaehlcke <mka@chromium.org> wrote:
> >
> > > On Tue, Oct 01, 2019 at 01:03:43PM -0400, Steven Rostedt wrote:
> > > > On Tue, 1 Oct 2019 09:35:42 -0700
> > > > Matthias Kaehlcke <mka@chromium.org> wrote:
> > > >
> > > > > 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.
> > > >
> > > > Do you have a use case example to present?
> > >
> > > TBH I'm not looking into a specific use case right now. While
> > > peeking around in /sys/kernel/debug/tracing/events to learn more
> > > about existing tracepoints that might be relevant for my work
> > > I noticed the absence of genpd ones and it seemed a good idea to
> > > add them preemptively. Conceptually they seem similar to the
> > > existing regulator_enable/disable and cpu_idle tracepoints.
> > >
> > > As an abstract use case I could see power analysis on battery
> > > powered devices during suspend. genpd_power_on/off allow to see
> > > which power domains remain on during suspend, and might give
> > > insights for possible power saving options. Examples could be that
> > > a power domain stays unexpectedly on due to a misconfiguration, or
> > > two power domains remain on when it could be only one if you just
> > > moved that one pin/port over to the other domain in the next
> > > hardware revision.
> >
> > If the power management maintainers have no issues with adding these,
> > then neither do I ;-)  It would be them who would pull them in anyway.
>
> Ok, I'll send a new version with the changes you suggested and some more
> info in the commit message, unless PM maintainers raise concerns before
> that.

I have no problems with adding tracepoints (or trace events to be more
precise) in there, but Ulf has been taking care of that code for quite
some time, so his opinion matters more than mine in that respect.

Re: [PATCH v1] PM / Domains: Add tracepoints

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2403 bytes --]

On Tue 2019-10-01 12:37:01, Matthias Kaehlcke wrote:
> On Tue, Oct 01, 2019 at 02:08:46PM -0400, Steven Rostedt wrote:
> > On Tue, 1 Oct 2019 10:42:35 -0700
> > Matthias Kaehlcke <mka@chromium.org> wrote:
> > 
> > > On Tue, Oct 01, 2019 at 01:03:43PM -0400, Steven Rostedt wrote:
> > > > On Tue, 1 Oct 2019 09:35:42 -0700
> > > > Matthias Kaehlcke <mka@chromium.org> wrote:
> > > >   
> > > > > 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.  
> > > > 
> > > > Do you have a use case example to present?  
> > > 
> > > TBH I'm not looking into a specific use case right now. While
> > > peeking around in /sys/kernel/debug/tracing/events to learn more
> > > about existing tracepoints that might be relevant for my work
> > > I noticed the absence of genpd ones and it seemed a good idea to
> > > add them preemptively. Conceptually they seem similar to the
> > > existing regulator_enable/disable and cpu_idle tracepoints.
> > > 
> > > As an abstract use case I could see power analysis on battery
> > > powered devices during suspend. genpd_power_on/off allow to see
> > > which power domains remain on during suspend, and might give
> > > insights for possible power saving options. Examples could be that
> > > a power domain stays unexpectedly on due to a misconfiguration, or
> > > two power domains remain on when it could be only one if you just
> > > moved that one pin/port over to the other domain in the next
> > > hardware revision.
> > 
> > If the power management maintainers have no issues with adding these,
> > then neither do I ;-)  It would be them who would pull them in anyway.
> 
> Ok, I'll send a new version with the changes you suggested and some more
> info in the commit message, unless PM maintainers raise concerns before
> that.

Dunno. Adding tracepoints because someone might need them for, umm,
something... I'd wait until they are actually needed.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v1] PM / Domains: Add tracepoints

[Ulf Hansson]

On Tue, 1 Oct 2019 at 18:35, Matthias Kaehlcke <mka@chromium.org> wrote:
>
> On Fri, Sep 27, 2019 at 04:42:39AM -0400, Steven Rostedt wrote:
> > On Thu, 26 Sep 2019 15:04:38 -0700
> > Matthias Kaehlcke <mka@chromium.org> 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.

Apologize for the delay, no excuse!

I don't mind adding trace events, as long as it's for good reasons -
and to me, that seems a bit questionable here.

According to the above, I believe the information you need is already
available via genpd's debugfs interface, no?

[...]

Kind regards
Uffe

Re: [PATCH v1] PM / Domains: Add tracepoints

[Matthias Kaehlcke]

Hi Ulf,

On Tue, Oct 15, 2019 at 02:37:42PM +0200, Ulf Hansson wrote:
> On Tue, 1 Oct 2019 at 18:35, Matthias Kaehlcke <mka@chromium.org> wrote:
> >
> > On Fri, Sep 27, 2019 at 04:42:39AM -0400, Steven Rostedt wrote:
> > > On Thu, 26 Sep 2019 15:04:38 -0700
> > > Matthias Kaehlcke <mka@chromium.org> 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.
> 
> Apologize for the delay, no excuse!
> 
> I don't mind adding trace events, as long as it's for good reasons -
> and to me, that seems a bit questionable here.
> 
> According to the above, I believe the information you need is already
> available via genpd's debugfs interface, no?

Not in all cases, e.g. you can't peek at sysfs while the device is
suspended. Also sysfs doesn't help much with seeing that a PD is
toggling between on an off for some (possibly legitimate) reason.

At this point I don't need this information badly, just thought it
could be useful. No problem if it is decided to hold back on it for
now.

Re: [PATCH v1] PM / Domains: Add tracepoints

[Ulf Hansson]

On Tue, 15 Oct 2019 at 19:19, Matthias Kaehlcke <mka@chromium.org> wrote:
>
> Hi Ulf,
>
> On Tue, Oct 15, 2019 at 02:37:42PM +0200, Ulf Hansson wrote:
> > On Tue, 1 Oct 2019 at 18:35, Matthias Kaehlcke <mka@chromium.org> wrote:
> > >
> > > On Fri, Sep 27, 2019 at 04:42:39AM -0400, Steven Rostedt wrote:
> > > > On Thu, 26 Sep 2019 15:04:38 -0700
> > > > Matthias Kaehlcke <mka@chromium.org> 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.
> >
> > Apologize for the delay, no excuse!
> >
> > I don't mind adding trace events, as long as it's for good reasons -
> > and to me, that seems a bit questionable here.
> >
> > According to the above, I believe the information you need is already
> > available via genpd's debugfs interface, no?
>
> Not in all cases, e.g. you can't peek at sysfs while the device is
> suspended.

Not sure I get this right. If a device that is attached to a genpd
that is runtime suspended, for sure you can have a look at the genpd
debugfs to see its current status.

> Also sysfs doesn't help much with seeing that a PD is
> toggling between on an off for some (possibly legitimate) reason.

Well, you could look at the "active_time" and the "total_idle_time"
nodes for the genpd in question. Those should change accordingly.

>
> At this point I don't need this information badly, just thought it
> could be useful. No problem if it is decided to hold back on it for
> now.

Okay, thanks!

Kind regards
Uffe

Re: [PATCH v1] PM / Domains: Add tracepoints

[Matthias Kaehlcke]

On Wed, Oct 16, 2019 at 03:47:44PM +0200, Ulf Hansson wrote:
> On Tue, 15 Oct 2019 at 19:19, Matthias Kaehlcke <mka@chromium.org> wrote:
> >
> > Hi Ulf,
> >
> > On Tue, Oct 15, 2019 at 02:37:42PM +0200, Ulf Hansson wrote:
> > > On Tue, 1 Oct 2019 at 18:35, Matthias Kaehlcke <mka@chromium.org> wrote:
> > > >
> > > > On Fri, Sep 27, 2019 at 04:42:39AM -0400, Steven Rostedt wrote:
> > > > > On Thu, 26 Sep 2019 15:04:38 -0700
> > > > > Matthias Kaehlcke <mka@chromium.org> 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.
> > >
> > > Apologize for the delay, no excuse!
> > >
> > > I don't mind adding trace events, as long as it's for good reasons -
> > > and to me, that seems a bit questionable here.
> > >
> > > According to the above, I believe the information you need is already
> > > available via genpd's debugfs interface, no?
> >
> > Not in all cases, e.g. you can't peek at sysfs while the device is
> > suspended.
> 
> Not sure I get this right. If a device that is attached to a genpd
> that is runtime suspended, for sure you can have a look at the genpd
> debugfs to see its current status.

Sorry, I used an ambiguous terminology by talking about 'the device',
I was referring to system suspend.

> > Also sysfs doesn't help much with seeing that a PD is
> > toggling between on an off for some (possibly legitimate) reason.
> 
> Well, you could look at the "active_time" and the "total_idle_time"
> nodes for the genpd in question. Those should change accordingly.

Ok, thanks for the pointer!