| Message ID | 20221004091950.3419662-1-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] watchdog: Add tracing events for the most usual watchdog events | expand |
On Tue, 4 Oct 2022 11:19:49 +0200 Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > +DEFINE_EVENT(watchdog_template, watchdog_start, > + TP_PROTO(struct watchdog_device *wdd, int err), > + TP_ARGS(wdd, err)); > + > +TRACE_EVENT(watchdog_set_timeout, > + > + TP_PROTO(struct watchdog_device *wdd, unsigned int timeout, int err), > + > + TP_ARGS(wdd, timeout, err), > + > + TP_STRUCT__entry( > + __field(int, id) > + __field(unsigned int, timeout) > + __field(int, err) > + ), > + > + TP_fast_assign( > + __entry->id = wdd->id; > + __entry->timeout = timeout; > + __entry->err = err; > + ), > + > + TP_printk("watchdog%d timeout=%u err=%d", __entry->id, __entry->timeout, __entry->err) > +); Nit, but I would probably put the above TRACE_EVENT() below the two DEFINE_EVENT()s below. That way we have all the DEFINE_EVENT()s for a specific DECLARE_EVENT_CLASS() together. Otherwise people may get confused. -- Steve > + > +DEFINE_EVENT(watchdog_template, watchdog_ping, > + TP_PROTO(struct watchdog_device *wdd, int err), > + TP_ARGS(wdd, err)); > + > +DEFINE_EVENT(watchdog_template, watchdog_stop, > + TP_PROTO(struct watchdog_device *wdd, int err), > + TP_ARGS(wdd, err)); > + > +#endif /* !defined(_TRACE_WATCHDOG_H) || defined(TRACE_HEADER_MULTI_READ) */ > +
On Tue, Oct 04, 2022 at 06:51:46PM -0400, Steven Rostedt wrote: > On Tue, 4 Oct 2022 11:19:49 +0200 > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > > +DEFINE_EVENT(watchdog_template, watchdog_start, > > + TP_PROTO(struct watchdog_device *wdd, int err), > > + TP_ARGS(wdd, err)); > > + > > +TRACE_EVENT(watchdog_set_timeout, > > + > > + TP_PROTO(struct watchdog_device *wdd, unsigned int timeout, int err), > > + > > + TP_ARGS(wdd, timeout, err), > > + > > + TP_STRUCT__entry( > > + __field(int, id) > > + __field(unsigned int, timeout) > > + __field(int, err) > > + ), > > + > > + TP_fast_assign( > > + __entry->id = wdd->id; > > + __entry->timeout = timeout; > > + __entry->err = err; > > + ), > > + > > + TP_printk("watchdog%d timeout=%u err=%d", __entry->id, __entry->timeout, __entry->err) > > +); > > Nit, but I would probably put the above TRACE_EVENT() below the two > DEFINE_EVENT()s below. That way we have all the DEFINE_EVENT()s for a > specific DECLARE_EVENT_CLASS() together. Otherwise people may get confused. I thought about that, too. The argument for the order I chose is that having start at the start and stop at the end is also intuitive. But I don't care much and would let the watchdog guys decide what they prefer. @Wim+Guenter: Feel free to reorder at application time or ask for a v3 if this v2 doesn't fit your preference. Best regards Uwe
On Wed, Oct 05, 2022 at 09:19:46AM +0200, Uwe Kleine-König wrote: > On Tue, Oct 04, 2022 at 06:51:46PM -0400, Steven Rostedt wrote: > > On Tue, 4 Oct 2022 11:19:49 +0200 > > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > > > > +DEFINE_EVENT(watchdog_template, watchdog_start, > > > + TP_PROTO(struct watchdog_device *wdd, int err), > > > + TP_ARGS(wdd, err)); > > > + > > > +TRACE_EVENT(watchdog_set_timeout, > > > + > > > + TP_PROTO(struct watchdog_device *wdd, unsigned int timeout, int err), > > > + > > > + TP_ARGS(wdd, timeout, err), > > > + > > > + TP_STRUCT__entry( > > > + __field(int, id) > > > + __field(unsigned int, timeout) > > > + __field(int, err) > > > + ), > > > + > > > + TP_fast_assign( > > > + __entry->id = wdd->id; > > > + __entry->timeout = timeout; > > > + __entry->err = err; > > > + ), > > > + > > > + TP_printk("watchdog%d timeout=%u err=%d", __entry->id, __entry->timeout, __entry->err) > > > +); > > > > Nit, but I would probably put the above TRACE_EVENT() below the two > > DEFINE_EVENT()s below. That way we have all the DEFINE_EVENT()s for a > > specific DECLARE_EVENT_CLASS() together. Otherwise people may get confused. > > I thought about that, too. The argument for the order I chose is that > having start at the start and stop at the end is also intuitive. > > But I don't care much and would let the watchdog guys decide what they > prefer. > > @Wim+Guenter: Feel free to reorder at application time or ask for a v3 > if this v2 doesn't fit your preference. For my part I would prefer a version with Steven's Reviewed-by: tag, whatever it is. Guenter
On Wed, 5 Oct 2022 12:39:24 -0700 Guenter Roeck <linux@roeck-us.net> wrote: > > > Nit, but I would probably put the above TRACE_EVENT() below the two > > > DEFINE_EVENT()s below. That way we have all the DEFINE_EVENT()s for a > > > specific DECLARE_EVENT_CLASS() together. Otherwise people may get confused. > > > > I thought about that, too. The argument for the order I chose is that > > having start at the start and stop at the end is also intuitive. > > > > But I don't care much and would let the watchdog guys decide what they > > prefer. > > > > @Wim+Guenter: Feel free to reorder at application time or ask for a v3 > > if this v2 doesn't fit your preference. > > For my part I would prefer a version with Steven's Reviewed-by: tag, > whatever it is. I much rather have the DEFINE_EVENTS followed by the DECLARE_EVENT_CLASS() as that's what most people look for. For start and stop being together, I believe that will not trip many people up, where as the DEFINE_EVENTS() scattering will. I'm OK if maintainers of the code are fine with the scattering, but it will break precedence. -- Steve
On Wed, Oct 05, 2022 at 05:11:36PM -0400, Steven Rostedt wrote: > On Wed, 5 Oct 2022 12:39:24 -0700 > Guenter Roeck <linux@roeck-us.net> wrote: > > > > > Nit, but I would probably put the above TRACE_EVENT() below the two > > > > DEFINE_EVENT()s below. That way we have all the DEFINE_EVENT()s for a > > > > specific DECLARE_EVENT_CLASS() together. Otherwise people may get confused. > > > > > > I thought about that, too. The argument for the order I chose is that > > > having start at the start and stop at the end is also intuitive. > > > > > > But I don't care much and would let the watchdog guys decide what they > > > prefer. > > > > > > @Wim+Guenter: Feel free to reorder at application time or ask for a v3 > > > if this v2 doesn't fit your preference. > > > > For my part I would prefer a version with Steven's Reviewed-by: tag, > > whatever it is. > > I much rather have the DEFINE_EVENTS followed by the DECLARE_EVENT_CLASS() > as that's what most people look for. > > For start and stop being together, I believe that will not trip many people > up, where as the DEFINE_EVENTS() scattering will. > I agree. Uwe, please send v3 with those changes. Thanks, Guenter
diff --git a/MAINTAINERS b/MAINTAINERS index 72b9654f764c..507de599116b 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -21890,6 +21890,7 @@ F: Documentation/watchdog/ F: drivers/watchdog/ F: include/linux/watchdog.h F: include/uapi/linux/watchdog.h +F: include/trace/events/watchdog.h WHISKEYCOVE PMIC GPIO DRIVER M: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c index 3fe8a7edc252..c777a612d932 100644 --- a/drivers/watchdog/watchdog_core.c +++ b/drivers/watchdog/watchdog_core.c @@ -38,6 +38,9 @@ #include "watchdog_core.h" /* For watchdog_dev_register/... */ +#define CREATE_TRACE_POINTS +#include <trace/events/watchdog.h> + static DEFINE_IDA(watchdog_ida); static int stop_on_reboot = -1; @@ -163,6 +166,7 @@ static int watchdog_reboot_notifier(struct notifier_block *nb, int ret; ret = wdd->ops->stop(wdd); + trace_watchdog_stop(wdd, ret); if (ret) return NOTIFY_BAD; } diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c index 54903f3c851e..3323309c913c 100644 --- a/drivers/watchdog/watchdog_dev.c +++ b/drivers/watchdog/watchdog_dev.c @@ -47,6 +47,8 @@ #include "watchdog_core.h" #include "watchdog_pretimeout.h" +#include <trace/events/watchdog.h> + /* the dev_t structure to store the dynamically allocated watchdog devices */ static dev_t watchdog_devt; /* Reference to watchdog device behind /dev/watchdog */ @@ -157,10 +159,13 @@ static int __watchdog_ping(struct watchdog_device *wdd) wd_data->last_hw_keepalive = now; - if (wdd->ops->ping) + if (wdd->ops->ping) { err = wdd->ops->ping(wdd); /* ping the watchdog */ - else + trace_watchdog_ping(wdd, err); + } else { err = wdd->ops->start(wdd); /* restart watchdog */ + trace_watchdog_start(wdd, err); + } if (err == 0) watchdog_hrtimer_pretimeout_start(wdd); @@ -259,6 +264,7 @@ static int watchdog_start(struct watchdog_device *wdd) } } else { err = wdd->ops->start(wdd); + trace_watchdog_start(wdd, err); if (err == 0) { set_bit(WDOG_ACTIVE, &wdd->status); wd_data->last_keepalive = started_at; @@ -297,6 +303,7 @@ static int watchdog_stop(struct watchdog_device *wdd) if (wdd->ops->stop) { clear_bit(WDOG_HW_RUNNING, &wdd->status); err = wdd->ops->stop(wdd); + trace_watchdog_stop(wdd, err); } else { set_bit(WDOG_HW_RUNNING, &wdd->status); } @@ -369,6 +376,7 @@ static int watchdog_set_timeout(struct watchdog_device *wdd, if (wdd->ops->set_timeout) { err = wdd->ops->set_timeout(wdd, timeout); + trace_watchdog_set_timeout(wdd, timeout, err); } else { wdd->timeout = timeout; /* Disable pretimeout if it doesn't fit the new timeout */ diff --git a/include/trace/events/watchdog.h b/include/trace/events/watchdog.h new file mode 100644 index 000000000000..77802d0d8c09 --- /dev/null +++ b/include/trace/events/watchdog.h @@ -0,0 +1,66 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +#undef TRACE_SYSTEM +#define TRACE_SYSTEM watchdog + +#if !defined(_TRACE_WATCHDOG_H) || defined(TRACE_HEADER_MULTI_READ) +#define _TRACE_WATCHDOG_H + +#include <linux/watchdog.h> +#include <linux/tracepoint.h> + +DECLARE_EVENT_CLASS(watchdog_template, + + TP_PROTO(struct watchdog_device *wdd, int err), + + TP_ARGS(wdd, err), + + TP_STRUCT__entry( + __field(int, id) + __field(int, err) + ), + + TP_fast_assign( + __entry->id = wdd->id; + __entry->err = err; + ), + + TP_printk("watchdog%d err=%d", __entry->id, __entry->err) +); + +DEFINE_EVENT(watchdog_template, watchdog_start, + TP_PROTO(struct watchdog_device *wdd, int err), + TP_ARGS(wdd, err)); + +TRACE_EVENT(watchdog_set_timeout, + + TP_PROTO(struct watchdog_device *wdd, unsigned int timeout, int err), + + TP_ARGS(wdd, timeout, err), + + TP_STRUCT__entry( + __field(int, id) + __field(unsigned int, timeout) + __field(int, err) + ), + + TP_fast_assign( + __entry->id = wdd->id; + __entry->timeout = timeout; + __entry->err = err; + ), + + TP_printk("watchdog%d timeout=%u err=%d", __entry->id, __entry->timeout, __entry->err) +); + +DEFINE_EVENT(watchdog_template, watchdog_ping, + TP_PROTO(struct watchdog_device *wdd, int err), + TP_ARGS(wdd, err)); + +DEFINE_EVENT(watchdog_template, watchdog_stop, + TP_PROTO(struct watchdog_device *wdd, int err), + TP_ARGS(wdd, err)); + +#endif /* !defined(_TRACE_WATCHDOG_H) || defined(TRACE_HEADER_MULTI_READ) */ + +/* This part must be outside protection */ +#include <trace/define_trace.h>
To simplify debugging which process touches a watchdog and when, add tracing events for .start(), .set_timeout(), .ping() and .stop(). Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- Changes since (implict) v1 sent with Message-Id: 20220930144935.3373366-1-u.kleine-koenig@pengutronix.de: - Make use of DECLARE_EVENT_CLASS as {requ,sugg}ested by Steven. Best regards Uwe MAINTAINERS | 1 + drivers/watchdog/watchdog_core.c | 4 ++ drivers/watchdog/watchdog_dev.c | 12 +++++- include/trace/events/watchdog.h | 66 ++++++++++++++++++++++++++++++++ 4 files changed, 81 insertions(+), 2 deletions(-) create mode 100644 include/trace/events/watchdog.h base-commit: 4fe89d07dcc2804c8b562f6c7896a45643d34b2f