[PATCH] hwmon: (core) Add trace events to _attr_show/store functions

[PATCH] hwmon: (core) Add trace events to _attr_show/store functions

[Nicolin Chen]

Trace events are useful for people who collect data from the
ftrace output. This patch adds initial trace events for the
hwmon core. To call hwmon_attr_base() for aligned attr index
numbers, this patch also moves the function upward.

Ftrace outputs:
 ...: hwmon_attr_show_string: index=2, attr_name=in2_label, val=VDD_5V
 ...: hwmon_attr_show: index=2, attr_name=in2_input, val=5112
 ...: hwmon_attr_show: index=2, attr_name=curr2_input, val=440

Note that the _attr_show and _attr_store functions are tied
to the _with_info API. So a hwmon driver requiring the trace
events feature should use _with_info API to register a hwmon
device.

Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---
 MAINTAINERS                  |  1 +
 drivers/hwmon/hwmon.c        | 27 ++++++++++----
 include/trace/events/hwmon.h | 71 ++++++++++++++++++++++++++++++++++++
 3 files changed, 92 insertions(+), 7 deletions(-)
 create mode 100644 include/trace/events/hwmon.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 1640b9faa75e..589b32405bf4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6461,6 +6461,7 @@ F:	Documentation/devicetree/bindings/hwmon/
 F:	Documentation/hwmon/
 F:	drivers/hwmon/
 F:	include/linux/hwmon*.h
+F:	include/trace/events/hwmon*.h
 
 HARDWARE RANDOM NUMBER GENERATOR CORE
 M:	Matt Mackall <mpm@selenic.com>
diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index ac1cdf88840f..975c95169884 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -24,6 +24,9 @@
 #include <linux/string.h>
 #include <linux/thermal.h>
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/hwmon.h>
+
 #define HWMON_ID_PREFIX "hwmon"
 #define HWMON_ID_FORMAT HWMON_ID_PREFIX "%d"
 
@@ -171,6 +174,13 @@ static int hwmon_thermal_add_sensor(struct device *dev,
 }
 #endif /* IS_REACHABLE(CONFIG_THERMAL) && ... */
 
+static int hwmon_attr_base(enum hwmon_sensor_types type)
+{
+	if (type == hwmon_in)
+		return 0;
+	return 1;
+}
+
 /* sysfs attribute management */
 
 static ssize_t hwmon_attr_show(struct device *dev,
@@ -185,6 +195,9 @@ static ssize_t hwmon_attr_show(struct device *dev,
 	if (ret < 0)
 		return ret;
 
+	trace_hwmon_attr_show(hattr->index + hwmon_attr_base(hattr->type),
+			      hattr->name, val);
+
 	return sprintf(buf, "%ld\n", val);
 }
 
@@ -193,6 +206,7 @@ static ssize_t hwmon_attr_show_string(struct device *dev,
 				      char *buf)
 {
 	struct hwmon_device_attribute *hattr = to_hwmon_attr(devattr);
+	enum hwmon_sensor_types type = hattr->type;
 	const char *s;
 	int ret;
 
@@ -201,6 +215,9 @@ static ssize_t hwmon_attr_show_string(struct device *dev,
 	if (ret < 0)
 		return ret;
 
+	trace_hwmon_attr_show_string(hattr->index + hwmon_attr_base(type),
+				     hattr->name, s);
+
 	return sprintf(buf, "%s\n", s);
 }
 
@@ -221,16 +238,12 @@ static ssize_t hwmon_attr_store(struct device *dev,
 	if (ret < 0)
 		return ret;
 
+	trace_hwmon_attr_store(hattr->index + hwmon_attr_base(hattr->type),
+			       hattr->name, val);
+
 	return count;
 }
 
-static int hwmon_attr_base(enum hwmon_sensor_types type)
-{
-	if (type == hwmon_in)
-		return 0;
-	return 1;
-}
-
 static bool is_string_attr(enum hwmon_sensor_types type, u32 attr)
 {
 	return (type == hwmon_temp && attr == hwmon_temp_label) ||
diff --git a/include/trace/events/hwmon.h b/include/trace/events/hwmon.h
new file mode 100644
index 000000000000..d7a1d0ffb679
--- /dev/null
+++ b/include/trace/events/hwmon.h
@@ -0,0 +1,71 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM hwmon
+
+#if !defined(_TRACE_HWMON_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_HWMON_H
+
+#include <linux/tracepoint.h>
+
+DECLARE_EVENT_CLASS(hwmon_attr_class,
+
+	TP_PROTO(int index, const char *attr_name, long val),
+
+	TP_ARGS(index, attr_name, val),
+
+	TP_STRUCT__entry(
+		__field(int, index)
+		__string(attr_name, attr_name)
+		__field(long, val)
+	),
+
+	TP_fast_assign(
+		__entry->index = index;
+		__assign_str(attr_name, attr_name);
+		__entry->val = val;
+	),
+
+	TP_printk("index=%d, attr_name=%s, val=%ld",
+		  __entry->index,  __get_str(attr_name), __entry->val)
+);
+
+DEFINE_EVENT(hwmon_attr_class, hwmon_attr_show,
+
+	TP_PROTO(int index, const char *attr_name, long val),
+
+	TP_ARGS(index, attr_name, val)
+);
+
+DEFINE_EVENT(hwmon_attr_class, hwmon_attr_store,
+
+	TP_PROTO(int index, const char *attr_name, long val),
+
+	TP_ARGS(index, attr_name, val)
+);
+
+TRACE_EVENT(hwmon_attr_show_string,
+
+	TP_PROTO(int index, const char *attr_name, const char *s),
+
+	TP_ARGS(index, attr_name, s),
+
+	TP_STRUCT__entry(
+		__field(int, index)
+		__string(attr_name, attr_name)
+		__string(label, s)
+	),
+
+	TP_fast_assign(
+		__entry->index = index;
+		__assign_str(attr_name, attr_name);
+		__assign_str(label, s);
+	),
+
+	TP_printk("index=%d, attr_name=%s, val=%s",
+		  __entry->index, __get_str(attr_name), __get_str(label))
+);
+
+#endif /* _TRACE_HWMON_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
-- 
2.17.1

Re: [PATCH] hwmon: (core) Add trace events to _attr_show/store functions

[Steven Rostedt]

On Tue,  9 Oct 2018 11:26:02 -0700
Nicolin Chen <nicoleotsuka@gmail.com> wrote:

> Trace events are useful for people who collect data from the
> ftrace output. This patch adds initial trace events for the
> hwmon core. To call hwmon_attr_base() for aligned attr index
> numbers, this patch also moves the function upward.
> 
> Ftrace outputs:
>  ...: hwmon_attr_show_string: index=2, attr_name=in2_label, val=VDD_5V
>  ...: hwmon_attr_show: index=2, attr_name=in2_input, val=5112
>  ...: hwmon_attr_show: index=2, attr_name=curr2_input, val=440
> 
> Note that the _attr_show and _attr_store functions are tied
> to the _with_info API. So a hwmon driver requiring the trace
> events feature should use _with_info API to register a hwmon
> device.

Hmm, this doesn't really explain why these trace events are needed.
They look to be attached to sysfs reads. What's the purpose of tracing
these?

> 
> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> ---
>  MAINTAINERS                  |  1 +
>  drivers/hwmon/hwmon.c        | 27 ++++++++++----
>  include/trace/events/hwmon.h | 71 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 92 insertions(+), 7 deletions(-)
>  create mode 100644 include/trace/events/hwmon.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1640b9faa75e..589b32405bf4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6461,6 +6461,7 @@ F:	Documentation/devicetree/bindings/hwmon/
>  F:	Documentation/hwmon/
>  F:	drivers/hwmon/
>  F:	include/linux/hwmon*.h
> +F:	include/trace/events/hwmon*.h
>  
>  HARDWARE RANDOM NUMBER GENERATOR CORE
>  M:	Matt Mackall <mpm@selenic.com>
> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> index ac1cdf88840f..975c95169884 100644
> --- a/drivers/hwmon/hwmon.c
> +++ b/drivers/hwmon/hwmon.c
> @@ -24,6 +24,9 @@
>  #include <linux/string.h>
>  #include <linux/thermal.h>
>  
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/hwmon.h>
> +
>  #define HWMON_ID_PREFIX "hwmon"
>  #define HWMON_ID_FORMAT HWMON_ID_PREFIX "%d"
>  
> @@ -171,6 +174,13 @@ static int hwmon_thermal_add_sensor(struct device *dev,
>  }
>  #endif /* IS_REACHABLE(CONFIG_THERMAL) && ... */
>  
> +static int hwmon_attr_base(enum hwmon_sensor_types type)
> +{
> +	if (type == hwmon_in)
> +		return 0;
> +	return 1;
> +}
> +
>  /* sysfs attribute management */
>  
>  static ssize_t hwmon_attr_show(struct device *dev,
> @@ -185,6 +195,9 @@ static ssize_t hwmon_attr_show(struct device *dev,
>  	if (ret < 0)
>  		return ret;
>  
> +	trace_hwmon_attr_show(hattr->index + hwmon_attr_base(hattr->type),
> +			      hattr->name, val);
> +
>  	return sprintf(buf, "%ld\n", val);
>  }
>  
> @@ -193,6 +206,7 @@ static ssize_t hwmon_attr_show_string(struct device *dev,
>  				      char *buf)
>  {
>  	struct hwmon_device_attribute *hattr = to_hwmon_attr(devattr);
> +	enum hwmon_sensor_types type = hattr->type;
>  	const char *s;
>  	int ret;
>  
> @@ -201,6 +215,9 @@ static ssize_t hwmon_attr_show_string(struct device *dev,
>  	if (ret < 0)
>  		return ret;
>  
> +	trace_hwmon_attr_show_string(hattr->index + hwmon_attr_base(type),

Also, the other to tracepoints use hattr->type, here you create a
separate variable. Is that just to keep within the 80 char limit?

-- Steve

> +				     hattr->name, s);
> +
>  	return sprintf(buf, "%s\n", s);
>  }
>  
> @@ -221,16 +238,12 @@ static ssize_t hwmon_attr_store(struct device *dev,
>  	if (ret < 0)
>  		return ret;
>  
> +	trace_hwmon_attr_store(hattr->index + hwmon_attr_base(hattr->type),
> +			       hattr->name, val);
> +
>  	return count;
>  }
>  
> -static int hwmon_attr_base(enum hwmon_sensor_types type)
> -{
> -	if (type == hwmon_in)
> -		return 0;
> -	return 1;
> -}
> -
>  static bool is_string_attr(enum hwmon_sensor_types type, u32 attr)
>  {
>  	return (type == hwmon_temp && attr == hwmon_temp_label) ||

Re: [PATCH] hwmon: (core) Add trace events to _attr_show/store functions

[Nicolin Chen]

On Tue, Oct 09, 2018 at 02:57:21PM -0400, Steven Rostedt wrote:

> > Trace events are useful for people who collect data from the
> > ftrace output. This patch adds initial trace events for the
> > hwmon core. To call hwmon_attr_base() for aligned attr index
> > numbers, this patch also moves the function upward.
> > 
> > Ftrace outputs:
> >  ...: hwmon_attr_show_string: index=2, attr_name=in2_label, val=VDD_5V
> >  ...: hwmon_attr_show: index=2, attr_name=in2_input, val=5112
> >  ...: hwmon_attr_show: index=2, attr_name=curr2_input, val=440
> > 
> > Note that the _attr_show and _attr_store functions are tied
> > to the _with_info API. So a hwmon driver requiring the trace
> > events feature should use _with_info API to register a hwmon
> > device.
> 
> Hmm, this doesn't really explain why these trace events are needed.
> They look to be attached to sysfs reads. What's the purpose of tracing
> these?

Our power folks analyse Ftrace outputs of cpufreq, thermal and
hwmon (power/voltage/current) so as to get the relationship of
them. The reason why we specifically need trace events is that
it's convenient and timestamped, and because both cpufreq and
thermal already have trace events.

I could add this to make the commit message more convincing if
you'd prefer that.

> > @@ -193,6 +206,7 @@ static ssize_t hwmon_attr_show_string(struct device *dev,
> >  				      char *buf)
> >  {
> >  	struct hwmon_device_attribute *hattr = to_hwmon_attr(devattr);
> > +	enum hwmon_sensor_types type = hattr->type;
> >  	const char *s;
> >  	int ret;
> >  
> > @@ -201,6 +215,9 @@ static ssize_t hwmon_attr_show_string(struct device *dev,
> >  	if (ret < 0)
> >  		return ret;
> >  
> > +	trace_hwmon_attr_show_string(hattr->index + hwmon_attr_base(type),
> 
> Also, the other to tracepoints use hattr->type, here you create a
> separate variable. Is that just to keep within the 80 char limit?

Yes. It looks clearer to me than wrapping the line here, since
complier usually optimize the local variable away.

Thanks
Nicolin

Re: [PATCH] hwmon: (core) Add trace events to _attr_show/store functions

[Steven Rostedt]

On Tue, 9 Oct 2018 12:11:11 -0700
Nicolin Chen <nicoleotsuka@gmail.com> wrote:

> On Tue, Oct 09, 2018 at 02:57:21PM -0400, Steven Rostedt wrote:
> 
> > > Trace events are useful for people who collect data from the
> > > ftrace output. This patch adds initial trace events for the
> > > hwmon core. To call hwmon_attr_base() for aligned attr index
> > > numbers, this patch also moves the function upward.
> > > 
> > > Ftrace outputs:
> > >  ...: hwmon_attr_show_string: index=2, attr_name=in2_label, val=VDD_5V
> > >  ...: hwmon_attr_show: index=2, attr_name=in2_input, val=5112
> > >  ...: hwmon_attr_show: index=2, attr_name=curr2_input, val=440
> > > 
> > > Note that the _attr_show and _attr_store functions are tied
> > > to the _with_info API. So a hwmon driver requiring the trace
> > > events feature should use _with_info API to register a hwmon
> > > device.  
> > 
> > Hmm, this doesn't really explain why these trace events are needed.
> > They look to be attached to sysfs reads. What's the purpose of tracing
> > these?  
> 
> Our power folks analyse Ftrace outputs of cpufreq, thermal and
> hwmon (power/voltage/current) so as to get the relationship of
> them. The reason why we specifically need trace events is that
> it's convenient and timestamped, and because both cpufreq and
> thermal already have trace events.
> 
> I could add this to make the commit message more convincing if
> you'd prefer that.

I'm not against the patch, but yes, having this in the change log is
helpful.


> 
> > > @@ -193,6 +206,7 @@ static ssize_t hwmon_attr_show_string(struct device *dev,
> > >  				      char *buf)
> > >  {
> > >  	struct hwmon_device_attribute *hattr = to_hwmon_attr(devattr);
> > > +	enum hwmon_sensor_types type = hattr->type;
> > >  	const char *s;
> > >  	int ret;
> > >  
> > > @@ -201,6 +215,9 @@ static ssize_t hwmon_attr_show_string(struct device *dev,
> > >  	if (ret < 0)
> > >  		return ret;
> > >  
> > > +	trace_hwmon_attr_show_string(hattr->index + hwmon_attr_base(type),  
> > 
> > Also, the other to tracepoints use hattr->type, here you create a
> > separate variable. Is that just to keep within the 80 char limit?  
> 
> Yes. It looks clearer to me than wrapping the line here, since
> complier usually optimize the local variable away.
> 

Or you can just break the 80 character limit ;-)

-- Steve

Re: [PATCH] hwmon: (core) Add trace events to _attr_show/store functions

[Nicolin Chen]

On Tue, Oct 09, 2018 at 03:49:45PM -0400, Steven Rostedt wrote:
> > > > Trace events are useful for people who collect data from the
> > > > ftrace output. This patch adds initial trace events for the
> > > > hwmon core. To call hwmon_attr_base() for aligned attr index
> > > > numbers, this patch also moves the function upward.
> > > > 
> > > > Ftrace outputs:
> > > >  ...: hwmon_attr_show_string: index=2, attr_name=in2_label, val=VDD_5V
> > > >  ...: hwmon_attr_show: index=2, attr_name=in2_input, val=5112
> > > >  ...: hwmon_attr_show: index=2, attr_name=curr2_input, val=440
> > > > 
> > > > Note that the _attr_show and _attr_store functions are tied
> > > > to the _with_info API. So a hwmon driver requiring the trace
> > > > events feature should use _with_info API to register a hwmon
> > > > device.  
> > > 
> > > Hmm, this doesn't really explain why these trace events are needed.
> > > They look to be attached to sysfs reads. What's the purpose of tracing
> > > these?  
> > 
> > Our power folks analyse Ftrace outputs of cpufreq, thermal and
> > hwmon (power/voltage/current) so as to get the relationship of
> > them. The reason why we specifically need trace events is that
> > it's convenient and timestamped, and because both cpufreq and
> > thermal already have trace events.
> > 
> > I could add this to make the commit message more convincing if
> > you'd prefer that.
> 
> I'm not against the patch, but yes, having this in the change log is
> helpful.

I will add it in v2.

> > > > @@ -193,6 +206,7 @@ static ssize_t hwmon_attr_show_string(struct device *dev,
> > > >  				      char *buf)
> > > >  {
> > > >  	struct hwmon_device_attribute *hattr = to_hwmon_attr(devattr);
> > > > +	enum hwmon_sensor_types type = hattr->type;
> > > >  	const char *s;
> > > >  	int ret;
> > > >  
> > > > @@ -201,6 +215,9 @@ static ssize_t hwmon_attr_show_string(struct device *dev,
> > > >  	if (ret < 0)
> > > >  		return ret;
> > > >  
> > > > +	trace_hwmon_attr_show_string(hattr->index + hwmon_attr_base(type),  
> > > 
> > > Also, the other to tracepoints use hattr->type, here you create a
> > > separate variable. Is that just to keep within the 80 char limit?  
> > 
> > Yes. It looks clearer to me than wrapping the line here, since
> > complier usually optimize the local variable away.
> 
> Or you can just break the 80 character limit ;-)

Personally a checkpatch warning bugs me more than having an extra
line :)

Thank you
Nicolin

Re: [PATCH] hwmon: (core) Add trace events to _attr_show/store functions

[Guenter Roeck]

On Tue, Oct 09, 2018 at 12:57:43PM -0700, Nicolin Chen wrote:
> On Tue, Oct 09, 2018 at 03:49:45PM -0400, Steven Rostedt wrote:
> > > > > Trace events are useful for people who collect data from the
> > > > > ftrace output. This patch adds initial trace events for the
> > > > > hwmon core. To call hwmon_attr_base() for aligned attr index
> > > > > numbers, this patch also moves the function upward.
> > > > > 
> > > > > Ftrace outputs:
> > > > >  ...: hwmon_attr_show_string: index=2, attr_name=in2_label, val=VDD_5V
> > > > >  ...: hwmon_attr_show: index=2, attr_name=in2_input, val=5112
> > > > >  ...: hwmon_attr_show: index=2, attr_name=curr2_input, val=440
> > > > > 
> > > > > Note that the _attr_show and _attr_store functions are tied
> > > > > to the _with_info API. So a hwmon driver requiring the trace
> > > > > events feature should use _with_info API to register a hwmon
> > > > > device.  
> > > > 
> > > > Hmm, this doesn't really explain why these trace events are needed.
> > > > They look to be attached to sysfs reads. What's the purpose of tracing
> > > > these?  
> > > 
> > > Our power folks analyse Ftrace outputs of cpufreq, thermal and
> > > hwmon (power/voltage/current) so as to get the relationship of
> > > them. The reason why we specifically need trace events is that
> > > it's convenient and timestamped, and because both cpufreq and
> > > thermal already have trace events.
> > > 
> > > I could add this to make the commit message more convincing if
> > > you'd prefer that.
> > 
> > I'm not against the patch, but yes, having this in the change log is
> > helpful.
> 
> I will add it in v2.
> 
> > > > > @@ -193,6 +206,7 @@ static ssize_t hwmon_attr_show_string(struct device *dev,
> > > > >  				      char *buf)
> > > > >  {
> > > > >  	struct hwmon_device_attribute *hattr = to_hwmon_attr(devattr);
> > > > > +	enum hwmon_sensor_types type = hattr->type;
> > > > >  	const char *s;
> > > > >  	int ret;
> > > > >  
> > > > > @@ -201,6 +215,9 @@ static ssize_t hwmon_attr_show_string(struct device *dev,
> > > > >  	if (ret < 0)
> > > > >  		return ret;
> > > > >  
> > > > > +	trace_hwmon_attr_show_string(hattr->index + hwmon_attr_base(type),  
> > > > 
> > > > Also, the other to tracepoints use hattr->type, here you create a
> > > > separate variable. Is that just to keep within the 80 char limit?  
> > > 
> > > Yes. It looks clearer to me than wrapping the line here, since
> > > complier usually optimize the local variable away.
> > 
> > Or you can just break the 80 character limit ;-)
> 
> Personally a checkpatch warning bugs me more than having an extra
> line :)
> 
Same here. If we no longer believe in the 80-column limit, we should remove it,
not use it to hide other problems in the noise.

Thanks,
Guenter

Re: [PATCH] hwmon: (core) Add trace events to _attr_show/store functions

[Steven Rostedt]

On Tue, 9 Oct 2018 13:39:37 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> > Personally a checkpatch warning bugs me more than having an extra
> > line :)
> >   
> Same here. If we no longer believe in the 80-column limit, we should remove it,
> not use it to hide other problems in the noise.

Yes, please, can we?

I personally hate the 80 character limit rule, because I like
descriptive variables and function names, which itself causes the 80
character limit to be broken. I find line breaks to avoid that limit
just makes the code look worse. Or at least up it to 100 chars.

-- Steve

Re: [PATCH] hwmon: (core) Add trace events to _attr_show/store functions

[Steven Rostedt]

On Tue, 9 Oct 2018 13:39:37 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> > Personally a checkpatch warning bugs me more than having an extra
> > line :)
> >   
> Same here. If we no longer believe in the 80-column limit, we should remove it,
> not use it to hide other problems in the noise.

Note, this is also the main reason why I don't even bother using
checkpatch.

-- Steve

Re: [PATCH] hwmon: (core) Add trace events to _attr_show/store functions

[Guenter Roeck]

On Tue, Oct 09, 2018 at 04:42:42PM -0400, Steven Rostedt wrote:
> On Tue, 9 Oct 2018 13:39:37 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
> > > Personally a checkpatch warning bugs me more than having an extra
> > > line :)
> > >   
> > Same here. If we no longer believe in the 80-column limit, we should remove it,
> > not use it to hide other problems in the noise.
> 
> Yes, please, can we?
> 
> I personally hate the 80 character limit rule, because I like
> descriptive variables and function names, which itself causes the 80
> character limit to be broken. I find line breaks to avoid that limit
> just makes the code look worse. Or at least up it to 100 chars.
> 

It is a two-edged sword. Downside is that a longer limit also invites
deeply nested loops and conditionals, which I am sure will be used as
argument against raising it.

Feel free to submit a patch to raise the limit to 100; I'll be more
than happy to give it an enthusiastic Reviewed-by:. My personal desire
for conflict isn't strong enough to submit it myself, though.

Guenter