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

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 <[email protected]>

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

MAINTAINERS +1 -0

index 1640b9faa75e..589b32405bf4 100644
@@ -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 <[email protected]>

drivers/hwmon/hwmon.c +14 -0

index ac1cdf88840f..975c95169884 100644
@@ -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;
 

drivers/hwmon/hwmon.c +5 -7

+				     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) ||

Quoted 10 lines

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

Quoted 4 lines

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?

Quoted 8 lines

drivers/hwmon/hwmon.c +1 -0

@@ -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;
 

drivers/hwmon/hwmon.c static ssize_t hwmon_attr_show_string(struct device *dev, · line 218

Quoted 5 lines

drivers/hwmon/hwmon.c +1 -0

@@ -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?

Quoted 10 lines · Steven Rostedt

Quoted 10 lines

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

Quoted 8 lines

Quoted 4 lines

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.

Quoted 13 lines

Quoted 13 lines

drivers/hwmon/hwmon.c +1 -0

@@ -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;
 

Quoted 4 lines

drivers/hwmon/hwmon.c static ssize_t hwmon_attr_show_string(struct device *dev, · line 218

Quoted 1 line

drivers/hwmon/hwmon.c +1 -0

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

Quoted 25 lines

Quoted 10 lines

Quoted 10 lines

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

Quoted 8 lines

Quoted 4 lines

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.

Quoted 2 lines

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.

Quoted 17 lines

Quoted 16 lines

drivers/hwmon/hwmon.c static ssize_t hwmon_attr_show_string(struct device *dev, · line 218

Quoted 13 lines

drivers/hwmon/hwmon.c +2 -0

@@ -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?

Quoted 2 lines

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 ;-)

Quoted 25 lines · Steven Rostedt

Quoted 25 lines

Quoted 10 lines

Quoted 10 lines

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

Quoted 8 lines

Quoted 4 lines

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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 17 lines

Quoted 17 lines

Quoted 16 lines

drivers/hwmon/hwmon.c static ssize_t hwmon_attr_show_string(struct device *dev, · line 218

Quoted 13 lines

drivers/hwmon/hwmon.c +2 -0

@@ -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?

Quoted 4 lines

Quoted 2 lines

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 :)

Quoted 3 lines

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.

Quoted 5 lines · On Tue, 9 Oct 2018 13:39:37 -0700 Guenter Roeck <[email protected]> wrote:

Quoted 3 lines

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.

Quoted 3 lines

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.