| Message ID | 20181009182602.3085-1-nicoleotsuka@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hwmon: (core) Add trace events to _attr_show/store functions | expand |
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) ||
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
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
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
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
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
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
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
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>
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