Re: [RFC V2 17/21] watchdog/dev: Add tracepoints

[Peter.Enderborg@xxxxxxxx]

On 2/14/22 11:45, Daniel Bristot de Oliveira wrote:
> Add a set of tracepoints, enabling the observability of the watchdog
> device interactions with user-space.
>
> The events are:
> 	watchdog:watchdog_open
> 	watchdog:watchdog_close
> 	watchdog:watchdog_start
> 	watchdog:watchdog_stop
> 	watchdog:watchdog_set_timeout
> 	watchdog:watchdog_ping
> 	watchdog:watchdog_nowayout
> 	watchdog:watchdog_set_keep_alive
> 	watchdog:watchdog_keep_alive


Some watchdogs have a bark functionality, I think it should be event for that too.

> Cc: Wim Van Sebroeck <wim@xxxxxxxxxxxxxxxxxx>
> Cc: Guenter Roeck <linux@xxxxxxxxxxxx>
> Cc: Jonathan Corbet <corbet@xxxxxxx>
> Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Will Deacon <will@xxxxxxxxxx>
> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> Cc: Marco Elver <elver@xxxxxxxxxx>
> Cc: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
> Cc: "Paul E. McKenney" <paulmck@xxxxxxxxxx>
> Cc: Shuah Khan <skhan@xxxxxxxxxxxxxxxxxxx>
> Cc: Gabriele Paoloni <gpaoloni@xxxxxxxxxx>
> Cc: Juri Lelli <juri.lelli@xxxxxxxxxx>
> Cc: Clark Williams <williams@xxxxxxxxxx>
> Cc: linux-doc@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Cc: linux-trace-devel@xxxxxxxxxxxxxxx
> Signed-off-by: Daniel Bristot de Oliveira <bristot@xxxxxxxxxx>
> ---
>  drivers/watchdog/watchdog_dev.c |  41 ++++++++++++-
>  include/linux/watchdog.h        |   7 +--
>  include/trace/events/watchdog.h | 103 ++++++++++++++++++++++++++++++++
>  3 files changed, 142 insertions(+), 9 deletions(-)
>  create mode 100644 include/trace/events/watchdog.h
>
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 3a3d8b5c7ad5..0beeac5d4541 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -44,6 +44,9 @@
>  #include <linux/watchdog.h>	/* For watchdog specific items */
>  #include <linux/uaccess.h>	/* For copy_to_user/put_user/... */
>  
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/watchdog.h>
> +
>  #include "watchdog_core.h"
>  #include "watchdog_pretimeout.h"
>  
> @@ -130,9 +133,11 @@ static inline void watchdog_update_worker(struct watchdog_device *wdd)
>  	if (watchdog_need_worker(wdd)) {
>  		ktime_t t = watchdog_next_keepalive(wdd);
>  
> -		if (t > 0)
> +		if (t > 0) {
>  			hrtimer_start(&wd_data->timer, t,
>  				      HRTIMER_MODE_REL_HARD);
> +			trace_watchdog_set_keep_alive(wdd, ktime_to_ms(t));
> +		}
>  	} else {
>  		hrtimer_cancel(&wd_data->timer);
>  	}
> @@ -149,14 +154,16 @@ static int __watchdog_ping(struct watchdog_device *wdd)
>  	now = ktime_get();
>  
>  	if (ktime_after(earliest_keepalive, now)) {
> -		hrtimer_start(&wd_data->timer,
> -			      ktime_sub(earliest_keepalive, now),
> +		ktime_t t = ktime_sub(earliest_keepalive, now);
> +		hrtimer_start(&wd_data->timer, t,
>  			      HRTIMER_MODE_REL_HARD);
> +		trace_watchdog_set_keep_alive(wdd, ktime_to_ms(t));
>  		return 0;
>  	}
>  
>  	wd_data->last_hw_keepalive = now;
>  
> +	trace_watchdog_ping(wdd);
>  	if (wdd->ops->ping)
>  		err = wdd->ops->ping(wdd);  /* ping the watchdog */
>  	else
> @@ -215,6 +222,7 @@ static void watchdog_ping_work(struct kthread_work *work)
>  	wd_data = container_of(work, struct watchdog_core_data, work);
>  
>  	mutex_lock(&wd_data->lock);
> +	trace_watchdog_keep_alive(wd_data->wdd);
>  	if (watchdog_worker_should_ping(wd_data))
>  		__watchdog_ping(wd_data->wdd);
>  	mutex_unlock(&wd_data->lock);
> @@ -252,6 +260,8 @@ static int watchdog_start(struct watchdog_device *wdd)
>  
>  	set_bit(_WDOG_KEEPALIVE, &wd_data->status);
>  
> +	trace_watchdog_start(wdd);
> +
>  	started_at = ktime_get();
>  	if (watchdog_hw_running(wdd) && wdd->ops->ping) {
>  		err = __watchdog_ping(wdd);
> @@ -298,6 +308,7 @@ static int watchdog_stop(struct watchdog_device *wdd)
>  		return -EBUSY;
>  	}
>  
> +	trace_watchdog_stop(wdd);
>  	if (wdd->ops->stop) {
>  		clear_bit(WDOG_HW_RUNNING, &wdd->status);
>  		err = wdd->ops->stop(wdd);
> @@ -370,6 +381,7 @@ static int watchdog_set_timeout(struct watchdog_device *wdd,
>  	if (watchdog_timeout_invalid(wdd, timeout))
>  		return -EINVAL;
>  
> +	trace_watchdog_set_timeout(wdd, timeout);
>  	if (wdd->ops->set_timeout) {
>  		err = wdd->ops->set_timeout(wdd, timeout);
>  	} else {
> @@ -432,6 +444,23 @@ static int watchdog_get_timeleft(struct watchdog_device *wdd,
>  	return 0;
>  }
>  
> +/*
> + * watchdog_set_nowayout - set nowaout bit
> + * @wdd:	The watchdog device to set nowayoutbit
> + * @nowayout	A boolean on/off switcher
> + *
> + * If nowayout boolean is true, the nowayout option is set. No action is
> + * taken if nowayout is false.
> + */
> +void watchdog_set_nowayout(struct watchdog_device *wdd, bool nowayout)
> +{
> +	if (nowayout) {
> +		set_bit(WDOG_NO_WAY_OUT, &wdd->status);
> +		trace_watchdog_nowayout(wdd);
> +	}
> +}
> +EXPORT_SYMBOL(watchdog_set_nowayout);
> +
>  #ifdef CONFIG_WATCHDOG_SYSFS
>  static ssize_t nowayout_show(struct device *dev, struct device_attribute *attr,
>  				char *buf)
> @@ -457,6 +486,7 @@ static ssize_t nowayout_store(struct device *dev, struct device_attribute *attr,
>  	/* nowayout cannot be disabled once set */
>  	if (test_bit(WDOG_NO_WAY_OUT, &wdd->status) && !value)
>  		return -EPERM;
> +
>  	watchdog_set_nowayout(wdd, value);
>  	return len;
>  }
> @@ -858,6 +888,8 @@ static int watchdog_open(struct inode *inode, struct file *file)
>  		goto out_clear;
>  	}
>  
> +	trace_watchdog_open(wdd);
> +
>  	err = watchdog_start(wdd);
>  	if (err < 0)
>  		goto out_mod;
> @@ -880,6 +912,7 @@ static int watchdog_open(struct inode *inode, struct file *file)
>  	return stream_open(inode, file);
>  
>  out_mod:
> +	trace_watchdog_close(wdd);
>  	module_put(wd_data->wdd->ops->owner);
>  out_clear:
>  	clear_bit(_WDOG_DEV_OPEN, &wd_data->status);
> @@ -940,6 +973,7 @@ static int watchdog_release(struct inode *inode, struct file *file)
>  	/* make sure that /dev/watchdog can be re-opened */
>  	clear_bit(_WDOG_DEV_OPEN, &wd_data->status);
>  
> +	trace_watchdog_close(wdd);
>  done:
>  	running = wdd && watchdog_hw_running(wdd);
>  	mutex_unlock(&wd_data->lock);
> @@ -952,6 +986,7 @@ static int watchdog_release(struct inode *inode, struct file *file)
>  		module_put(wd_data->cdev.owner);
>  		put_device(&wd_data->dev);
>  	}
> +
>  	return 0;
>  }
>  
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index 99660197a36c..11d93407e492 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -139,12 +139,7 @@ static inline bool watchdog_hw_running(struct watchdog_device *wdd)
>  	return test_bit(WDOG_HW_RUNNING, &wdd->status);
>  }
>  
> -/* Use the following function to set the nowayout feature */
> -static inline void watchdog_set_nowayout(struct watchdog_device *wdd, bool nowayout)
> -{
> -	if (nowayout)
> -		set_bit(WDOG_NO_WAY_OUT, &wdd->status);
> -}
> +void watchdog_set_nowayout(struct watchdog_device *wdd, bool nowayout);
>  
>  /* Use the following function to stop the watchdog on reboot */
>  static inline void watchdog_stop_on_reboot(struct watchdog_device *wdd)
> diff --git a/include/trace/events/watchdog.h b/include/trace/events/watchdog.h
> new file mode 100644
> index 000000000000..5d5617ab611a
> --- /dev/null
> +++ b/include/trace/events/watchdog.h
> @@ -0,0 +1,103 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM watchdog
> +
> +#if !defined(_TRACE_WATCHDOG_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_WATCHDOG_H
> +
> +#include <linux/tracepoint.h>
> +
> +DECLARE_EVENT_CLASS(dev_operations_template,
> +
> +	TP_PROTO(struct watchdog_device *wdd),
> +
> +	TP_ARGS(wdd),
> +
> +	TP_STRUCT__entry(
> +		__field(__u32, id)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->id = wdd->id;
> +	),
> +
> +	TP_printk("id=%d",
> +		  __entry->id)
> +);
> +
> +/*
> + * Add a comment
> + */
> +DEFINE_EVENT(dev_operations_template, watchdog_open,
> +	     TP_PROTO(struct watchdog_device *wdd),
> +	     TP_ARGS(wdd));
> +
> +DEFINE_EVENT(dev_operations_template, watchdog_close,
> +	     TP_PROTO(struct watchdog_device *wdd),
> +	     TP_ARGS(wdd));
> +
> +DEFINE_EVENT(dev_operations_template, watchdog_start,
> +	     TP_PROTO(struct watchdog_device *wdd),
> +	     TP_ARGS(wdd));
> +
> +DEFINE_EVENT(dev_operations_template, watchdog_stop,
> +	     TP_PROTO(struct watchdog_device *wdd),
> +	     TP_ARGS(wdd));
> +
> +DEFINE_EVENT(dev_operations_template, watchdog_ping,
> +	     TP_PROTO(struct watchdog_device *wdd),
> +	     TP_ARGS(wdd));
> +
> +DEFINE_EVENT(dev_operations_template, watchdog_keep_alive,
> +	     TP_PROTO(struct watchdog_device *wdd),
> +	     TP_ARGS(wdd));
> +
> +DEFINE_EVENT(dev_operations_template, watchdog_nowayout,
> +	     TP_PROTO(struct watchdog_device *wdd),
> +	     TP_ARGS(wdd));
> +
> +
> +TRACE_EVENT(watchdog_set_timeout,
> +
> +	TP_PROTO(struct watchdog_device *wdd, u64 timeout),
> +
> +	TP_ARGS(wdd, timeout),
> +
> +	TP_STRUCT__entry(
> +		__field(__u32, id)
> +		__field(__u64, timeout)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->id		= wdd->id;
> +		__entry->timeout	= timeout;
> +	),
> +
> +	TP_printk("id=%d timeout=%llus",
> +		  __entry->id, __entry->timeout)
> +);
> +
> +TRACE_EVENT(watchdog_set_keep_alive,
> +
> +	TP_PROTO(struct watchdog_device *wdd, u64 timeout),
> +
> +	TP_ARGS(wdd, timeout),
> +
> +	TP_STRUCT__entry(
> +		__field(__u32, id)
> +		__field(__u64, timeout)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->id		= wdd->id;
> +		__entry->timeout	= timeout;
> +	),
> +
> +	TP_printk("id=%d keep_alive=%llums",
> +		  __entry->id, __entry->timeout)
> +);
> +
> +#endif /* _TRACE_WATCHDOG_H */
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>