The Runtime Verification (RV) interface

Sample reactor that printks the reaction message.

Note: do not use this reactor with rq_lock taken, it will lock the system until printk can handle that.

Cc Jonathan Corbet <[email protected]>

Cc Steven Rostedt <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Will Deacon <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Marco Elver <[email protected]>

Cc Dmitry Vyukov <[email protected]>

Cc "Paul E. McKenney" <[email protected]>

Cc Shuah Khan <[email protected]>

Cc Gabriele Paoloni <[email protected]>

Cc Juri Lelli <[email protected]>

Cc Clark Williams <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Signed-off-by Daniel Bristot de Oliveira <[email protected]>

---
  kernel/trace/rv/Kconfig          |  8 ++++++
  kernel/trace/rv/Makefile         |  3 ++-
  kernel/trace/rv/reactor_printk.c | 43 ++++++++++++++++++++++++++++++++
  3 files changed, 53 insertions(+), 1 deletion(-)

create mode 100644 kernel/trace/rv/reactor_printk.c

kernel/trace/rv/Kconfig +8 -0

index f2d0bed05a55..490f1417def9 100644
@@ -37,4 +37,12 @@ config RV_REACTORS
  	  tracing reactions, printing the monitor output via tracepoints,
  	  but other reactions can be added (on-demand) via this interface.
 
+config RV_REACT_PRINTK
+	tristate "Printk reactor"
+	depends on RV_REACTORS
+	default y if RV_REACTORS
+	help
+	  Enables the printk reactor. The printk reactor emmits a printk()
+	  message if an exception is found.
+
  endif # RV

kernel/trace/rv/Makefile +2 -1

index edad01bb2b5d..d86ceb103ef2 100644
@@ -1,6 +1,7 @@
  # SPDX-License-Identifier: GPL-2.0
 
  obj-$(CONFIG_RV) += rv.o
-obj-$(CONFIG_RV_REACTORS) += rv_reactors.o
  obj-$(CONFIG_RV_MON_WIP) += monitor_wip/wip.o
  obj-$(CONFIG_RV_MON_WWNR) += monitor_wwnr/wwnr.o
+obj-$(CONFIG_RV_REACTORS) += rv_reactors.o
+obj-$(CONFIG_RV_REACT_PRINTK) += reactor_printk.o

kernel/trace/rv/reactor_printk.c new file +43 -0

new file mode 100644
index 000000000000..9f9c9bb2d304
@@ -0,0 +1,43 @@
+/*
+ * Printk RV reactor:
+ *   Prints the exception msg to the kernel message log.
+ *
+ * Copyright (C) 2019-2022 Daniel Bristot de Oliveira <[email protected]>
+ *
+ * SPDX-License-Identifier: GPL-2.0
+ */
+#include <linux/ftrace.h>
+#include <linux/tracepoint.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/rv.h>
+
+static void rv_printk_reaction(char *msg)
+{
+	printk(msg);
+}
+
+struct rv_reactor rv_printk = {
+	.name = "printk",
+	.description = "prints the exception msg to the kernel message log",
+	.react = rv_printk_reaction
+};
+
+int register_react_printk(void)
+{
+	rv_register_reactor(&rv_printk);
+	return 0;
+}
+
+void unregister_react_printk(void)
+{
+	rv_unregister_reactor(&rv_printk);
+}
+
+module_init(register_react_printk);
+module_exit(unregister_react_printk);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Daniel Bristot de Oliveira");
+MODULE_DESCRIPTION("printk rv reactor: printk if an exception is hit");

Quoted 2 lines · Daniel Bristot de Oliveira

Sample reactor that printks the reaction message.

Who is supposed to take action on this message and how do they get notified?

Quoted 3 lines

Note: do not use this reactor with rq_lock taken, it will lock the system until printk can handle that.

Please give more details on "lock the system" and how locking would impact users and system operation

Quoted 6 lines · Shuah Khan

Quoted 2 lines · Daniel Bristot de Oliveira

Note: do not use this reactor with rq_lock taken, it will lock the system until printk can handle that.

Please give more details on "lock the system" and how locking would impact users and system operation

It is a deadlock on the rq_lock. Hopefully the new printk implementation handles that better.

Quoted 8 lines · Daniel Bristot de Oliveira

Quoted 6 lines · Shuah Khan

Quoted 2 lines · Daniel Bristot de Oliveira

Note: do not use this reactor with rq_lock taken, it will lock the system until printk can handle that.

Please give more details on "lock the system" and how locking would impact users and system operation

Quoted 2 lines

It is a deadlock on the rq_lock. Hopefully the new printk implementation handles that better.

I suggest using printk_deferred() for this reactor for now. With printk_deferred() the message and timestamp are immediately and locklessly stored in the buffer, and the printing is performed in a separate context. printk_deferred() is safe for all contexts.

John

Quoted 4 lines

I suggest using printk_deferred() for this reactor for now. With printk_deferred() the message and timestamp are immediately and locklessly stored in the buffer, and the printing is performed in a separate context. printk_deferred() is safe for all contexts.

Question: Does it always postpone or only postpone when in a particular contexts, like, with irqs disabled?

Note: do not use this reactor with rq_lock taken, it will lock the system until printk can handle that.

Quoted 6 lines · Daniel Bristot de Oliveira

Quoted 4 lines

I suggest using printk_deferred() for this reactor for now. With printk_deferred() the message and timestamp are immediately and locklessly stored in the buffer, and the printing is performed in a separate context. printk_deferred() is safe for all contexts.

Question: Does it always postpone or only postpone when in a particular contexts, like, with irqs disabled?

printk_deferred() always postpones printing.

Quoted 2 lines

Note: do not use this reactor with rq_lock taken, it will lock the system until printk can handle that.

Perhaps you could explain thi comment in your commit message? printk() should never lock the system.

I am aware of printk_deferred(), and every once and while I am Cc'ed on patches suggesting changing printk() to printk_deferred(), but they are not, let's say, welcome [1]... that is why I am not using it.

I saw deadlocks in the past, and while testing the WIP monitor some time ago, it seems it depends on the console type. If such restriction does not exist anymore, I can remove that comment, it would be even better!

[1] https://lore.kernel.org/lkml/e68888438cec9a1da53aaa1647720ade638d6ad4.1600705105.git.bristot@redhat.com/

Quoted 4 lines

I saw deadlocks in the past, and while testing the WIP monitor some time ago, it seems it depends on the console type. If such restriction does not exist anymore, I can remove that comment, it would be even better!

If you say it depended on the console type, then it was probably the framebuffer console that was at fault. The fbcon is a landmine for deadlocks, which is why PREEMPT_RT avoids fbcon printing from non-preemptible context. For mainline, the series is currently in review.

Perhaps avoiding fbcon would be good enough for you to avoid deadlocks with this reactor using printk().

So, I will keep printk(), add a depends on *!* fbcon, and remove the comment. What do you think?

Quoted 2 lines · Daniel Bristot de Oliveira

So, I will keep printk(), add a depends on *!* fbcon, and remove the comment. What do you think?

printk() and depends on !VT_CONSOLE. OK for me. Hopefully soon this restriction can be removed.

John

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

Cc Guenter Roeck <[email protected]>

Cc Jonathan Corbet <[email protected]>

Cc Steven Rostedt <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Will Deacon <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Marco Elver <[email protected]>

Cc Dmitry Vyukov <[email protected]>

Cc "Paul E. McKenney" <[email protected]>

Cc Shuah Khan <[email protected]>

Cc Gabriele Paoloni <[email protected]>

Cc Juri Lelli <[email protected]>

Cc Clark Williams <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Signed-off-by Daniel Bristot de Oliveira <[email protected]>

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

drivers/watchdog/watchdog_dev.c +6 -1

index 3a3d8b5c7ad5..0beeac5d4541 100644
@@ -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);
  	}

drivers/watchdog/watchdog_dev.c +8 -0

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

drivers/watchdog/watchdog_dev.c +21 -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);

drivers/watchdog/watchdog_dev.c +0 -0

  	return 0;
  }
 

include/linux/watchdog.h +1 -6

index 99660197a36c..11d93407e492 100644
@@ -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)

include/trace/events/watchdog.h new file +103 -0

new file mode 100644
index 000000000000..5d5617ab611a
@@ -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>

Quoted 66 lines · Daniel Bristot de Oliveira

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

Cc Guenter Roeck <[email protected]>

Cc Jonathan Corbet <[email protected]>

Cc Steven Rostedt <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Will Deacon <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Marco Elver <[email protected]>

Cc Dmitry Vyukov <[email protected]>

Cc "Paul E. McKenney" <[email protected]>

Cc Shuah Khan <[email protected]>

Cc Gabriele Paoloni <[email protected]>

Cc Juri Lelli <[email protected]>

Cc Clark Williams <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Signed-off-by Daniel Bristot de Oliveira <[email protected]>

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

drivers/watchdog/watchdog_dev.c +0 -0

index 3a3d8b5c7ad5..0beeac5d4541 100644
@@ -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

drivers/watchdog/watchdog_dev.c +2 -0

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

drivers/watchdog/watchdog_dev.c static int __watchdog_ping(struct watchdog_device *wdd) · line 156

Quoted 6 lines

drivers/watchdog/watchdog_dev.c +1 -2

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

Quoted 38 lines

drivers/watchdog/watchdog_dev.c +2 -0

+        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

drivers/watchdog/watchdog_dev.c +2 -0

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

Quoted 6 lines

drivers/watchdog/watchdog_dev.c +0 -0

       } else {

@@ -432,6 +444,23 @@ static int watchdog_get_timeleft(struct watchdog_device
 *wdd,
       return 0;
   }

  +/*

/** ?

Quoted 55 lines

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

drivers/watchdog/watchdog_dev.c +9 -0

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

drivers/watchdog/watchdog_dev.c +1 -0

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

  }

drivers/watchdog/watchdog_dev.c +1 -0

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

drivers/watchdog/watchdog_dev.c +0 -0

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

drivers/watchdog/watchdog_dev.c +0 -0

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

drivers/watchdog/watchdog_dev.c +0 -0

@@ -952,6 +986,7 @@ static int watchdog_release(struct inode *inode, struct
 file *file)

drivers/watchdog/watchdog_dev.c static int watchdog_release(struct inode *inode, struct file *file) · line 989

Quoted 4 lines

drivers/watchdog/watchdog_dev.c +1 -0

           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

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

Cc Guenter Roeck <[email protected]>

Cc Jonathan Corbet <[email protected]>

Cc Steven Rostedt <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Will Deacon <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Marco Elver <[email protected]>

Cc Dmitry Vyukov <[email protected]>

Cc "Paul E. McKenney" <[email protected]>

Cc Shuah Khan <[email protected]>

Cc Gabriele Paoloni <[email protected]>

Cc Juri Lelli <[email protected]>

Cc Clark Williams <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Signed-off-by Daniel Bristot de Oliveira <[email protected]>

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

drivers/watchdog/watchdog_dev.c +38 -3

index 3a3d8b5c7ad5..0beeac5d4541 100644
@@ -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;
 }
 

include/linux/watchdog.h +1 -6

index 99660197a36c..11d93407e492 100644
@@ -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)

include/trace/events/watchdog.h new file +103 -0

new file mode 100644
index 000000000000..5d5617ab611a
@@ -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>

Quoted 13 lines · Daniel Bristot de Oliveira

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.

Hi Peter

Quoted 15 lines · On 2/16/22 17:01, [email protected] wrote:

Quoted 13 lines · Daniel Bristot de Oliveira

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

Quoted 2 lines

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

I understand. The problems is that I do not see the bark abstraction in the watchdog_dev layer.

In the case I am investigating (the safety_monitor/safety_app) the bark is outside of the "OS" view, it is an hardware action - like. shutdown.

Am I missing something? Thoughts?

-- Daniel

Quoted 22 lines · Daniel Bristot de Oliveira

Hi Peter

Quoted 18 lines · On 2/16/22 17:01, [email protected] wrote:

Quoted 13 lines · Daniel Bristot de Oliveira

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.

Quoted 4 lines

I understand. The problems is that I do not see the bark abstraction in the
watchdog_dev layer.

I don't even know what "bark functionality" means. A new term for pretimeout ?
Something else ?

Guenter

Quoted 7 lines

In the case I am investigating (the safety_monitor/safety_app) the bark is
outside of the "OS" view, it is an hardware action - like. shutdown.

Am I missing something? Thoughts?

-- Daniel

Quoted 25 lines · Guenter Roeck

Quoted 24 lines · Daniel Bristot de Oliveira

Hi Peter

Quoted 14 lines · On 2/16/22 17:01, [email protected] wrote:

Quoted 13 lines · Daniel Bristot de Oliveira

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

Quoted 3 lines

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

I understand. The problems is that I do not see the bark abstraction in the
watchdog_dev layer.

Quoted 3 lines

I don't even know what "bark functionality" means. A new term for pretimeout ?
Something else ?

From my understanding the bark timeout is actually the pretimeout whereas the bite timeout is the actual timeout.

Quoted 28 lines · Guenter Roeck

Quoted 26 lines · Daniel Bristot de Oliveira

Hi Peter

Quoted 15 lines · On 2/16/22 17:01, [email protected] wrote:

Quoted 13 lines · Daniel Bristot de Oliveira

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

Quoted 3 lines

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

I understand. The problems is that I do not see the bark abstraction in the
watchdog_dev layer.

Quoted 5 lines

I don't even know what "bark functionality" means. A new term for pretimeout ?
Something else ?

From my understanding the bark timeout is actually the pretimeout

whereas the bite timeout is the actual timeout.
I think in the Kernel ftwdt010_wdt and qcom-wdt are bark/bite WTDs

Quoted 35 lines · Gabriele Paoloni

Quoted 31 lines · Guenter Roeck

Quoted 18 lines · Daniel Bristot de Oliveira

Hi Peter

Quoted 15 lines · On 2/16/22 17:01, [email protected] wrote:

Quoted 13 lines · Daniel Bristot de Oliveira

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

Quoted 8 lines

Quoted 3 lines

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

I understand. The problems is that I do not see the bark abstraction in the
watchdog_dev layer.

I don't even know what "bark functionality" means. A new term for pretimeout ?
Something else ?