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

[Guenter Roeck]

On 2/14/22 02: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
>
> 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);

I am quite sure this line creates a checkpatch warning.

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

You may disagree with current empty lines or other cosmetics, but that is not an
acceptable reason for such changes. Please drop this change and the similar change
further up.

Guenter

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