Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1199381 > unrolled thread

[PATCH 0/8] watchdog: Add support for keepalives triggered by infrastructure

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/8] watchdog: Add support for keepalives triggered by infrastructure Guenter Roeck <linux@roeck-us.net> - 2015-08-04 04:20 +0200
    [PATCH 8/8] watchdog: at91sam9: Convert to use infrastructure triggered keepalives Guenter Roeck <linux@roeck-us.net> - 2015-08-04 04:20 +0200
    [PATCH 1/8] watchdog: watchdog_dev: Use single variable name for struct watchdog_device Guenter Roeck <linux@roeck-us.net> - 2015-08-04 04:20 +0200
      Re: [PATCH 1/8] watchdog: watchdog_dev: Use single variable name for  struct watchdog_device Uwe Kleine-König   <u.kleine-koenig@pengutronix.de> - 2015-08-04 13:30 +0200
    [PATCH 6/8] watchdog: retu: Convert to use infrastructure triggered keepalives Guenter Roeck <linux@roeck-us.net> - 2015-08-04 04:20 +0200
    Re: [PATCH 0/8] watchdog: Add support for keepalives triggered by  infrastructure Uwe Kleine-König   <u.kleine-koenig@pengutronix.de> - 2015-08-04 13:30 +0200
      Re: [PATCH 0/8] watchdog: Add support for keepalives triggered by  infrastructure Guenter Roeck <linux@roeck-us.net> - 2015-08-04 17:10 +0200
    Re: [PATCH 0/8] watchdog: Add support for keepalives triggered by  infrastructure Pádraig Brady <P@draigBrady.com> - 2015-08-05 01:50 +0200
      Re: [PATCH 0/8] watchdog: Add support for keepalives triggered by  infrastructure Guenter Roeck <linux@roeck-us.net> - 2015-08-05 02:50 +0200
      Re: [PATCH 0/8] watchdog: Add support for keepalives triggered by  infrastructure Uwe Kleine-König   <u.kleine-koenig@pengutronix.de> - 2015-08-05 09:40 +0200
        Re: [PATCH 0/8] watchdog: Add support for keepalives triggered by  infrastructure Guenter Roeck <linux@roeck-us.net> - 2015-08-05 10:00 +0200
          Re: [PATCH 0/8] watchdog: Add support for keepalives triggered by  infrastructure Uwe Kleine-König   <u.kleine-koenig@pengutronix.de> - 2015-08-05 10:30 +0200
    Re: [PATCH 0/8] watchdog: Add support for keepalives triggered by  infrastructure David Teigland <teigland@redhat.com> - 2015-08-05 19:20 +0200
      Re: [PATCH 0/8] watchdog: Add support for keepalives triggered by  infrastructure Guenter Roeck <linux@roeck-us.net> - 2015-08-05 19:50 +0200
        Re: [PATCH 0/8] watchdog: Add support for keepalives triggered by  infrastructure David Teigland <teigland@redhat.com> - 2015-08-05 20:00 +0200
          Re: [PATCH 0/8] watchdog: Add support for keepalives triggered by  infrastructure Guenter Roeck <linux@roeck-us.net> - 2015-08-05 21:10 +0200
            Re: [PATCH 0/8] watchdog: Add support for keepalives triggered by  infrastructure David Teigland <teigland@redhat.com> - 2015-08-05 22:00 +0200
              Re: [PATCH 0/8] watchdog: Add support for keepalives triggered by  infrastructure Guenter Roeck <linux@roeck-us.net> - 2015-08-05 22:30 +0200

#1199381 — [PATCH 0/8] watchdog: Add support for keepalives triggered by infrastructure

The watchdog infrastructure is currently purely passive, meaning
it only passes information from user space to drivers and vice versa.

Since watchdog hardware tends to have its own quirks, this can result
in quite complex watchdog drivers. A number of scanarios are especially common.

- A watchdog is always active and can not be disabled, or can not be disabled
  once enabled. To support such hardware, watchdog drivers have to implement
  their own timers and use those timers to trigger watchdog keepalives while
  the watchdog device is not or not yet opened.
- A variant of this is the desire to enable a watchdog as soon as its driver
  has been instantiated, to protect the system while it is still booting up,
  but the watchdog daemon is not yet running.
- Some watchdogs have a very short maximum timeout, in the range of just a few
  seconds. Such low timeouts are difficult if not impossible to support from
  user space. Drivers supporting such watchdog hardware need to implement
  a timer function to augment heartbeats from user space.

This patch set solves the above problems while keeping changes to the
watchdog core minimal.

- A new status flag, WDOG_RUNNING, informs the watchdog subsystem that a
  watchdog is running, and that the watchdog subsystem needs to generate
  heartbeat requests while the associated watchdog device is closed.
- A new parameter in the watchdog data structure, max_hw_timeout_ms, informs
  the watchdog subsystem about a maximum hardware timeout. The watchdog
  subsystem uses this information together with the configured timeout
  and the maximum permitted timeout to determine if it needs to generate
  additional heartbeat requests.

Patch #1 is a preparatory patch.

Patch #2 adds timer functionality to the watchdog core. It solves the problem
of short maximum hardware timeouts by augmenting heartbeats triggered from
user space with internally triggered heartbeats.

Patch #3 adds functionality to generate heartbeats while the watchdog device is
closed. It handles situation where where the watchdog is running after
the driver has been instantiated, but the device is not yet opened,
and post-close situations necessary if a watchdog can not be stopped.

Patch #4 makes the set_timeout function optional. This is now possible since
timeout changes can now be completely handled in the watchdog core, for
example if the hardware watchdog timeout is fixed.

Patch #5 to #8 are example conversions of some watchdog drivers.
Those patches will require testing.

This patch set does not solve all limitations of the watchdog subsystem.
Specifically, it does not add support for the following features.

- It is desirable to be able to specify a maximum early timeout,
  from booting the system to opening the watchdog device.
- Some watchdogs may require a minimum period of time between
  heartbeats. Examples are DA9062 and possibly AT91SAM9x.

This and other features will be adddessed with subsequent patches.

The patch set is inspired by an earlier patch set from Timo Kokonnen.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1199382 — [PATCH 8/8] watchdog: at91sam9: Convert to use infrastructure triggered keepalives

The watchdog infrastructure now supports handling watchdog keepalive
if the watchdog is running while the watchdog device is closed.
The infrastructure now also supports generating additional heartbeats
if the maximum hardware timeout is smaller than or close to the
configured timeout. Convert the driver to use this
infrastructure.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/at91sam9_wdt.c | 102 +++++-----------------------------------
 1 file changed, 11 insertions(+), 91 deletions(-)

diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
index e4698f7c5f93..0de39b52962c 100644
--- a/drivers/watchdog/at91sam9_wdt.c
+++ b/drivers/watchdog/at91sam9_wdt.c
@@ -29,7 +29,6 @@
 #include <linux/types.h>
 #include <linux/watchdog.h>
 #include <linux/jiffies.h>
-#include <linux/timer.h>
 #include <linux/bitops.h>
 #include <linux/uaccess.h>
 #include <linux/of.h>
@@ -48,8 +47,8 @@
  * use this to convert a watchdog
  * value from/to milliseconds.
  */
-#define ticks_to_hz_rounddown(t)	((((t) + 1) * HZ) >> 8)
-#define ticks_to_hz_roundup(t)		(((((t) + 1) * HZ) + 255) >> 8)
+#define ticks_to_ms_rounddown(t)	((((t) + 1) * 1000) >> 8)
+#define ticks_to_ms_roundup(t)		(((((t) + 1) * 1000) + 255) >> 8)
 #define ticks_to_secs(t)		(((t) + 1) >> 8)
 #define secs_to_ticks(s)		((s) ? (((s) << 8) - 1) : 0)
 
@@ -64,9 +63,6 @@
 /* Hardware timeout in seconds */
 #define WDT_HW_TIMEOUT 2
 
-/* Timer heartbeat (500ms) */
-#define WDT_TIMEOUT	(HZ/2)
-
 /* User land timeout */
 #define WDT_HEARTBEAT 15
 static int heartbeat;
@@ -83,11 +79,8 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
 struct at91wdt {
 	struct watchdog_device wdd;
 	void __iomem *base;
-	unsigned long next_heartbeat;	/* the next_heartbeat for the timer */
-	struct timer_list timer;	/* The timer that pings the watchdog */
 	u32 mr;
 	u32 mr_mask;
-	unsigned long heartbeat;	/* WDT heartbeat in jiffies */
 	bool nowayout;
 	unsigned int irq;
 };
@@ -107,47 +100,13 @@ static irqreturn_t wdt_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-/*
- * Reload the watchdog timer.  (ie, pat the watchdog)
- */
-static inline void at91_wdt_reset(struct at91wdt *wdt)
-{
-	wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
-}
-
-/*
- * Timer tick
- */
-static void at91_ping(unsigned long data)
-{
-	struct at91wdt *wdt = (struct at91wdt *)data;
-	if (time_before(jiffies, wdt->next_heartbeat) ||
-	    !watchdog_active(&wdt->wdd)) {
-		at91_wdt_reset(wdt);
-		mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
-	} else {
-		pr_crit("I will reset your machine !\n");
-	}
-}
-
 static int at91_wdt_start(struct watchdog_device *wdd)
 {
 	struct at91wdt *wdt = to_wdt(wdd);
-	/* calculate when the next userspace timeout will be */
-	wdt->next_heartbeat = jiffies + wdd->timeout * HZ;
-	return 0;
-}
 
-static int at91_wdt_stop(struct watchdog_device *wdd)
-{
-	/* The watchdog timer hardware can not be stopped... */
-	return 0;
-}
+	wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
 
-static int at91_wdt_set_timeout(struct watchdog_device *wdd, unsigned int new_timeout)
-{
-	wdd->timeout = new_timeout;
-	return at91_wdt_start(wdd);
+	return 0;
 }
 
 static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
@@ -157,8 +116,8 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
 	u32 value;
 	int err;
 	u32 mask = wdt->mr_mask;
-	unsigned long min_heartbeat = 1;
-	unsigned long max_heartbeat;
+	unsigned int min_timeout = jiffies_to_msecs(1);
+	unsigned int hw_timeout;
 	struct device *dev = &pdev->dev;
 
 	tmp = wdt_read(wdt, AT91_WDT_MR);
@@ -180,31 +139,15 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
 	delta = (tmp & AT91_WDT_WDD) >> 16;
 
 	if (delta < value)
-		min_heartbeat = ticks_to_hz_roundup(value - delta);
+		min_timeout = ticks_to_ms_roundup(value - delta);
 
-	max_heartbeat = ticks_to_hz_rounddown(value);
-	if (!max_heartbeat) {
+	hw_timeout = ticks_to_ms_rounddown(value);
+	if (hw_timeout < min_timeout * 2) {
 		dev_err(dev,
 			"heartbeat is too small for the system to handle it correctly\n");
 		return -EINVAL;
 	}
-
-	/*
-	 * Try to reset the watchdog counter 4 or 2 times more often than
-	 * actually requested, to avoid spurious watchdog reset.
-	 * If this is not possible because of the min_heartbeat value, reset
-	 * it at the min_heartbeat period.
-	 */
-	if ((max_heartbeat / 4) >= min_heartbeat)
-		wdt->heartbeat = max_heartbeat / 4;
-	else if ((max_heartbeat / 2) >= min_heartbeat)
-		wdt->heartbeat = max_heartbeat / 2;
-	else
-		wdt->heartbeat = min_heartbeat;
-
-	if (max_heartbeat < min_heartbeat + 4)
-		dev_warn(dev,
-			 "min heartbeat and max heartbeat might be too close for the system to handle it correctly\n");
+	wdt->wdd.max_hw_timeout_ms = hw_timeout;
 
 	if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
 		err = request_irq(wdt->irq, wdt_interrupt,
@@ -220,32 +163,12 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
 			 "watchdog already configured differently (mr = %x expecting %x)\n",
 			 tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
 
-	setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
-
-	/*
-	 * Use min_heartbeat the first time to avoid spurious watchdog reset:
-	 * we don't know for how long the watchdog counter is running, and
-	 *  - resetting it right now might trigger a watchdog fault reset
-	 *  - waiting for heartbeat time might lead to a watchdog timeout
-	 *    reset
-	 */
-	mod_timer(&wdt->timer, jiffies + min_heartbeat);
-
 	/* Try to set timeout from device tree first */
 	if (watchdog_init_timeout(&wdt->wdd, 0, dev))
 		watchdog_init_timeout(&wdt->wdd, heartbeat, dev);
 	watchdog_set_nowayout(&wdt->wdd, wdt->nowayout);
-	err = watchdog_register_device(&wdt->wdd);
-	if (err)
-		goto out_stop_timer;
-
-	wdt->next_heartbeat = jiffies + wdt->wdd.timeout * HZ;
-
-	return 0;
 
-out_stop_timer:
-	del_timer(&wdt->timer);
-	return err;
+	return watchdog_register_device(&wdt->wdd);
 }
 
 /* ......................................................................... */
@@ -259,8 +182,6 @@ static const struct watchdog_info at91_wdt_info = {
 static const struct watchdog_ops at91_wdt_ops = {
 	.owner =	THIS_MODULE,
 	.start =	at91_wdt_start,
-	.stop =		at91_wdt_stop,
-	.set_timeout =	at91_wdt_set_timeout,
 };
 
 #if defined(CONFIG_OF)
@@ -376,7 +297,6 @@ static int __exit at91wdt_remove(struct platform_device *pdev)
 	watchdog_unregister_device(&wdt->wdd);
 
 	pr_warn("I quit now, hardware will probably reboot!\n");
-	del_timer(&wdt->timer);
 
 	return 0;
 }
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1199383 — [PATCH 1/8] watchdog: watchdog_dev: Use single variable name for struct watchdog_device

The current code uses 'wdd', wddev', and 'watchdog' as variable names
for struct watchdog_device. This is confusing and makes it difficult
to enhance the code. Replace it all with 'wdd'.

Cc: Timo Kokkonen <timo.kokkonen@offcode.fi>
Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/watchdog_dev.c | 151 ++++++++++++++++++++--------------------
 1 file changed, 75 insertions(+), 76 deletions(-)

diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 6aaefbad303e..06171c73daf5 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -51,7 +51,7 @@ static struct watchdog_device *old_wdd;
 
 /*
  *	watchdog_ping: ping the watchdog.
- *	@wddev: the watchdog device to ping
+ *	@wdd: the watchdog device to ping
  *
  *	If the watchdog has no own ping operation then it needs to be
  *	restarted via the start operation. This wrapper function does
@@ -59,65 +59,65 @@ static struct watchdog_device *old_wdd;
  *	We only ping when the watchdog device is running.
  */
 
-static int watchdog_ping(struct watchdog_device *wddev)
+static int watchdog_ping(struct watchdog_device *wdd)
 {
 	int err = 0;
 
-	mutex_lock(&wddev->lock);
+	mutex_lock(&wdd->lock);
 
-	if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
+	if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
 		err = -ENODEV;
 		goto out_ping;
 	}
 
-	if (!watchdog_active(wddev))
+	if (!watchdog_active(wdd))
 		goto out_ping;
 
-	if (wddev->ops->ping)
-		err = wddev->ops->ping(wddev);  /* ping the watchdog */
+	if (wdd->ops->ping)
+		err = wdd->ops->ping(wdd);	/* ping the watchdog */
 	else
-		err = wddev->ops->start(wddev); /* restart watchdog */
+		err = wdd->ops->start(wdd);	/* restart watchdog */
 
 out_ping:
-	mutex_unlock(&wddev->lock);
+	mutex_unlock(&wdd->lock);
 	return err;
 }
 
 /*
  *	watchdog_start: wrapper to start the watchdog.
- *	@wddev: the watchdog device to start
+ *	@wdd: the watchdog device to start
  *
  *	Start the watchdog if it is not active and mark it active.
  *	This function returns zero on success or a negative errno code for
  *	failure.
  */
 
-static int watchdog_start(struct watchdog_device *wddev)
+static int watchdog_start(struct watchdog_device *wdd)
 {
 	int err = 0;
 
-	mutex_lock(&wddev->lock);
+	mutex_lock(&wdd->lock);
 
-	if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
+	if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
 		err = -ENODEV;
 		goto out_start;
 	}
 
-	if (watchdog_active(wddev))
+	if (watchdog_active(wdd))
 		goto out_start;
 
-	err = wddev->ops->start(wddev);
+	err = wdd->ops->start(wdd);
 	if (err == 0)
-		set_bit(WDOG_ACTIVE, &wddev->status);
+		set_bit(WDOG_ACTIVE, &wdd->status);
 
 out_start:
-	mutex_unlock(&wddev->lock);
+	mutex_unlock(&wdd->lock);
 	return err;
 }
 
 /*
  *	watchdog_stop: wrapper to stop the watchdog.
- *	@wddev: the watchdog device to stop
+ *	@wdd: the watchdog device to stop
  *
  *	Stop the watchdog if it is still active and unmark it active.
  *	This function returns zero on success or a negative errno code for
@@ -125,155 +125,154 @@ out_start:
  *	If the 'nowayout' feature was set, the watchdog cannot be stopped.
  */
 
-static int watchdog_stop(struct watchdog_device *wddev)
+static int watchdog_stop(struct watchdog_device *wdd)
 {
 	int err = 0;
 
-	mutex_lock(&wddev->lock);
+	mutex_lock(&wdd->lock);
 
-	if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
+	if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
 		err = -ENODEV;
 		goto out_stop;
 	}
 
-	if (!watchdog_active(wddev))
+	if (!watchdog_active(wdd))
 		goto out_stop;
 
-	if (test_bit(WDOG_NO_WAY_OUT, &wddev->status)) {
-		dev_info(wddev->dev, "nowayout prevents watchdog being stopped!\n");
+	if (test_bit(WDOG_NO_WAY_OUT, &wdd->status)) {
+		dev_info(wdd->dev, "nowayout prevents watchdog being stopped!\n");
 		err = -EBUSY;
 		goto out_stop;
 	}
 
-	err = wddev->ops->stop(wddev);
+	err = wdd->ops->stop(wdd);
 	if (err == 0)
-		clear_bit(WDOG_ACTIVE, &wddev->status);
+		clear_bit(WDOG_ACTIVE, &wdd->status);
 
 out_stop:
-	mutex_unlock(&wddev->lock);
+	mutex_unlock(&wdd->lock);
 	return err;
 }
 
 /*
  *	watchdog_get_status: wrapper to get the watchdog status
- *	@wddev: the watchdog device to get the status from
+ *	@wdd: the watchdog device to get the status from
  *	@status: the status of the watchdog device
  *
  *	Get the watchdog's status flags.
  */
 
-static int watchdog_get_status(struct watchdog_device *wddev,
+static int watchdog_get_status(struct watchdog_device *wdd,
 							unsigned int *status)
 {
 	int err = 0;
 
 	*status = 0;
-	if (!wddev->ops->status)
+	if (!wdd->ops->status)
 		return -EOPNOTSUPP;
 
-	mutex_lock(&wddev->lock);
+	mutex_lock(&wdd->lock);
 
-	if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
+	if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
 		err = -ENODEV;
 		goto out_status;
 	}
 
-	*status = wddev->ops->status(wddev);
+	*status = wdd->ops->status(wdd);
 
 out_status:
-	mutex_unlock(&wddev->lock);
+	mutex_unlock(&wdd->lock);
 	return err;
 }
 
 /*
  *	watchdog_set_timeout: set the watchdog timer timeout
- *	@wddev: the watchdog device to set the timeout for
+ *	@wdd: the watchdog device to set the timeout for
  *	@timeout: timeout to set in seconds
  */
 
-static int watchdog_set_timeout(struct watchdog_device *wddev,
+static int watchdog_set_timeout(struct watchdog_device *wdd,
 							unsigned int timeout)
 {
 	int err;
 
-	if ((wddev->ops->set_timeout == NULL) ||
-	    !(wddev->info->options & WDIOF_SETTIMEOUT))
+	if (!wdd->ops->set_timeout || !(wdd->info->options & WDIOF_SETTIMEOUT))
 		return -EOPNOTSUPP;
 
-	if (watchdog_timeout_invalid(wddev, timeout))
+	if (watchdog_timeout_invalid(wdd, timeout))
 		return -EINVAL;
 
-	mutex_lock(&wddev->lock);
+	mutex_lock(&wdd->lock);
 
-	if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
+	if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
 		err = -ENODEV;
 		goto out_timeout;
 	}
 
-	err = wddev->ops->set_timeout(wddev, timeout);
+	err = wdd->ops->set_timeout(wdd, timeout);
 
 out_timeout:
-	mutex_unlock(&wddev->lock);
+	mutex_unlock(&wdd->lock);
 	return err;
 }
 
 /*
  *	watchdog_get_timeleft: wrapper to get the time left before a reboot
- *	@wddev: the watchdog device to get the remaining time from
+ *	@wdd: the watchdog device to get the remaining time from
  *	@timeleft: the time that's left
  *
  *	Get the time before a watchdog will reboot (if not pinged).
  */
 
-static int watchdog_get_timeleft(struct watchdog_device *wddev,
+static int watchdog_get_timeleft(struct watchdog_device *wdd,
 							unsigned int *timeleft)
 {
 	int err = 0;
 
 	*timeleft = 0;
-	if (!wddev->ops->get_timeleft)
+	if (!wdd->ops->get_timeleft)
 		return -EOPNOTSUPP;
 
-	mutex_lock(&wddev->lock);
+	mutex_lock(&wdd->lock);
 
-	if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
+	if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
 		err = -ENODEV;
 		goto out_timeleft;
 	}
 
-	*timeleft = wddev->ops->get_timeleft(wddev);
+	*timeleft = wdd->ops->get_timeleft(wdd);
 
 out_timeleft:
-	mutex_unlock(&wddev->lock);
+	mutex_unlock(&wdd->lock);
 	return err;
 }
 
 /*
  *	watchdog_ioctl_op: call the watchdog drivers ioctl op if defined
- *	@wddev: the watchdog device to do the ioctl on
+ *	@wdd: the watchdog device to do the ioctl on
  *	@cmd: watchdog command
  *	@arg: argument pointer
  */
 
-static int watchdog_ioctl_op(struct watchdog_device *wddev, unsigned int cmd,
+static int watchdog_ioctl_op(struct watchdog_device *wdd, unsigned int cmd,
 							unsigned long arg)
 {
 	int err;
 
-	if (!wddev->ops->ioctl)
+	if (!wdd->ops->ioctl)
 		return -ENOIOCTLCMD;
 
-	mutex_lock(&wddev->lock);
+	mutex_lock(&wdd->lock);
 
-	if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
+	if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
 		err = -ENODEV;
 		goto out_ioctl;
 	}
 
-	err = wddev->ops->ioctl(wddev, cmd, arg);
+	err = wdd->ops->ioctl(wdd, cmd, arg);
 
 out_ioctl:
-	mutex_unlock(&wddev->lock);
+	mutex_unlock(&wdd->lock);
 	return err;
 }
 
@@ -513,43 +512,43 @@ static struct miscdevice watchdog_miscdev = {
 
 /*
  *	watchdog_dev_register: register a watchdog device
- *	@watchdog: watchdog device
+ *	@wdd: watchdog device
  *
  *	Register a watchdog device including handling the legacy
  *	/dev/watchdog node. /dev/watchdog is actually a miscdevice and
  *	thus we set it up like that.
  */
 
-int watchdog_dev_register(struct watchdog_device *watchdog)
+int watchdog_dev_register(struct watchdog_device *wdd)
 {
 	int err, devno;
 
-	if (watchdog->id == 0) {
-		old_wdd = watchdog;
-		watchdog_miscdev.parent = watchdog->parent;
+	if (wdd->id == 0) {
+		old_wdd = wdd;
+		watchdog_miscdev.parent = wdd->parent;
 		err = misc_register(&watchdog_miscdev);
 		if (err != 0) {
 			pr_err("%s: cannot register miscdev on minor=%d (err=%d).\n",
-				watchdog->info->identity, WATCHDOG_MINOR, err);
+				wdd->info->identity, WATCHDOG_MINOR, err);
 			if (err == -EBUSY)
 				pr_err("%s: a legacy watchdog module is probably present.\n",
-					watchdog->info->identity);
+					wdd->info->identity);
 			old_wdd = NULL;
 			return err;
 		}
 	}
 
 	/* Fill in the data structures */
-	devno = MKDEV(MAJOR(watchdog_devt), watchdog->id);
-	cdev_init(&watchdog->cdev, &watchdog_fops);
-	watchdog->cdev.owner = watchdog->ops->owner;
+	devno = MKDEV(MAJOR(watchdog_devt), wdd->id);
+	cdev_init(&wdd->cdev, &watchdog_fops);
+	wdd->cdev.owner = wdd->ops->owner;
 
 	/* Add the device */
-	err  = cdev_add(&watchdog->cdev, devno, 1);
+	err  = cdev_add(&wdd->cdev, devno, 1);
 	if (err) {
 		pr_err("watchdog%d unable to add device %d:%d\n",
-			watchdog->id,  MAJOR(watchdog_devt), watchdog->id);
-		if (watchdog->id == 0) {
+			wdd->id,  MAJOR(watchdog_devt), wdd->id);
+		if (wdd->id == 0) {
 			misc_deregister(&watchdog_miscdev);
 			old_wdd = NULL;
 		}
@@ -564,14 +563,14 @@ int watchdog_dev_register(struct watchdog_device *watchdog)
  *	Unregister the watchdog and if needed the legacy /dev/watchdog device.
  */
 
-int watchdog_dev_unregister(struct watchdog_device *watchdog)
+int watchdog_dev_unregister(struct watchdog_device *wdd)
 {
-	mutex_lock(&watchdog->lock);
-	set_bit(WDOG_UNREGISTERED, &watchdog->status);
-	mutex_unlock(&watchdog->lock);
+	mutex_lock(&wdd->lock);
+	set_bit(WDOG_UNREGISTERED, &wdd->status);
+	mutex_unlock(&wdd->lock);
 
-	cdev_del(&watchdog->cdev);
-	if (watchdog->id == 0) {
+	cdev_del(&wdd->cdev);
+	if (wdd->id == 0) {
 		misc_deregister(&watchdog_miscdev);
 		old_wdd = NULL;
 	}
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1199688 — Re: [PATCH 1/8] watchdog: watchdog_dev: Use single variable name for struct watchdog_device

Hello Guenter,

On Mon, Aug 03, 2015 at 07:13:27PM -0700, Guenter Roeck wrote:
> The current code uses 'wdd', wddev', and 'watchdog' as variable names
> for struct watchdog_device. This is confusing and makes it difficult
> to enhance the code. Replace it all with 'wdd'.
Seems sensible.

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1199384 — [PATCH 6/8] watchdog: retu: Convert to use infrastructure triggered keepalives

The watchdog infrastructure now supports handling watchdog keepalive
if the watchdog is running while the watchdog device is closed.
Convert the driver to use this infrastructure.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/retu_wdt.c | 78 ++++-----------------------------------------
 1 file changed, 7 insertions(+), 71 deletions(-)

diff --git a/drivers/watchdog/retu_wdt.c b/drivers/watchdog/retu_wdt.c
index b7c68e275aeb..ce2982a7670c 100644
--- a/drivers/watchdog/retu_wdt.c
+++ b/drivers/watchdog/retu_wdt.c
@@ -28,69 +28,22 @@
 /* Watchdog timer values in seconds */
 #define RETU_WDT_MAX_TIMER	63
 
-struct retu_wdt_dev {
-	struct retu_dev		*rdev;
-	struct device		*dev;
-	struct delayed_work	ping_work;
-};
-
-/*
- * Since Retu watchdog cannot be disabled in hardware, we must kick it
- * with a timer until userspace watchdog software takes over. If
- * CONFIG_WATCHDOG_NOWAYOUT is set, we never start the feeding.
- */
-static void retu_wdt_ping_enable(struct retu_wdt_dev *wdev)
-{
-	retu_write(wdev->rdev, RETU_REG_WATCHDOG, RETU_WDT_MAX_TIMER);
-	schedule_delayed_work(&wdev->ping_work,
-			round_jiffies_relative(RETU_WDT_MAX_TIMER * HZ / 2));
-}
-
-static void retu_wdt_ping_disable(struct retu_wdt_dev *wdev)
-{
-	retu_write(wdev->rdev, RETU_REG_WATCHDOG, RETU_WDT_MAX_TIMER);
-	cancel_delayed_work_sync(&wdev->ping_work);
-}
-
-static void retu_wdt_ping_work(struct work_struct *work)
-{
-	struct retu_wdt_dev *wdev = container_of(to_delayed_work(work),
-						struct retu_wdt_dev, ping_work);
-	retu_wdt_ping_enable(wdev);
-}
-
 static int retu_wdt_start(struct watchdog_device *wdog)
 {
-	struct retu_wdt_dev *wdev = watchdog_get_drvdata(wdog);
+	struct retu_dev *rdev = watchdog_get_drvdata(wdog);
 
-	retu_wdt_ping_disable(wdev);
+	set_bit(WDOG_RUNNING, &wdog->status);
 
-	return retu_write(wdev->rdev, RETU_REG_WATCHDOG, wdog->timeout);
-}
-
-static int retu_wdt_stop(struct watchdog_device *wdog)
-{
-	struct retu_wdt_dev *wdev = watchdog_get_drvdata(wdog);
-
-	retu_wdt_ping_enable(wdev);
-
-	return 0;
-}
-
-static int retu_wdt_ping(struct watchdog_device *wdog)
-{
-	struct retu_wdt_dev *wdev = watchdog_get_drvdata(wdog);
-
-	return retu_write(wdev->rdev, RETU_REG_WATCHDOG, wdog->timeout);
+	return retu_write(rdev, RETU_REG_WATCHDOG, wdog->timeout);
 }
 
 static int retu_wdt_set_timeout(struct watchdog_device *wdog,
 				unsigned int timeout)
 {
-	struct retu_wdt_dev *wdev = watchdog_get_drvdata(wdog);
+	struct retu_dev *rdev = watchdog_get_drvdata(wdog);
 
 	wdog->timeout = timeout;
-	return retu_write(wdev->rdev, RETU_REG_WATCHDOG, wdog->timeout);
+	return retu_write(rdev, RETU_REG_WATCHDOG, wdog->timeout);
 }
 
 static const struct watchdog_info retu_wdt_info = {
@@ -101,8 +54,6 @@ static const struct watchdog_info retu_wdt_info = {
 static const struct watchdog_ops retu_wdt_ops = {
 	.owner		= THIS_MODULE,
 	.start		= retu_wdt_start,
-	.stop		= retu_wdt_stop,
-	.ping		= retu_wdt_ping,
 	.set_timeout	= retu_wdt_set_timeout,
 };
 
@@ -111,39 +62,26 @@ static int retu_wdt_probe(struct platform_device *pdev)
 	struct retu_dev *rdev = dev_get_drvdata(pdev->dev.parent);
 	bool nowayout = WATCHDOG_NOWAYOUT;
 	struct watchdog_device *retu_wdt;
-	struct retu_wdt_dev *wdev;
 	int ret;
 
 	retu_wdt = devm_kzalloc(&pdev->dev, sizeof(*retu_wdt), GFP_KERNEL);
 	if (!retu_wdt)
 		return -ENOMEM;
 
-	wdev = devm_kzalloc(&pdev->dev, sizeof(*wdev), GFP_KERNEL);
-	if (!wdev)
-		return -ENOMEM;
-
 	retu_wdt->info		= &retu_wdt_info;
 	retu_wdt->ops		= &retu_wdt_ops;
 	retu_wdt->timeout	= RETU_WDT_MAX_TIMER;
 	retu_wdt->min_timeout	= 0;
 	retu_wdt->max_timeout	= RETU_WDT_MAX_TIMER;
 
-	watchdog_set_drvdata(retu_wdt, wdev);
+	watchdog_set_drvdata(retu_wdt, rdev);
 	watchdog_set_nowayout(retu_wdt, nowayout);
 
-	wdev->rdev		= rdev;
-	wdev->dev		= &pdev->dev;
-
-	INIT_DELAYED_WORK(&wdev->ping_work, retu_wdt_ping_work);
-
 	ret = watchdog_register_device(retu_wdt);
 	if (ret < 0)
 		return ret;
 
-	if (nowayout)
-		retu_wdt_ping(retu_wdt);
-	else
-		retu_wdt_ping_enable(wdev);
+	retu_wdt_start(retu_wdt);
 
 	platform_set_drvdata(pdev, retu_wdt);
 
@@ -153,10 +91,8 @@ static int retu_wdt_probe(struct platform_device *pdev)
 static int retu_wdt_remove(struct platform_device *pdev)
 {
 	struct watchdog_device *wdog = platform_get_drvdata(pdev);
-	struct retu_wdt_dev *wdev = watchdog_get_drvdata(wdog);
 
 	watchdog_unregister_device(wdog);
-	cancel_delayed_work_sync(&wdev->ping_work);
 
 	return 0;
 }
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1199689 — Re: [PATCH 0/8] watchdog: Add support for keepalives triggered by infrastructure

Hello Guenter,

On Mon, Aug 03, 2015 at 07:13:26PM -0700, Guenter Roeck wrote:
> This patch set does not solve all limitations of the watchdog subsystem.
> Specifically, it does not add support for the following features.
> 
> - It is desirable to be able to specify a maximum early timeout,
>   from booting the system to opening the watchdog device.
> - Some watchdogs may require a minimum period of time between
>   heartbeats. Examples are DA9062 and possibly AT91SAM9x.

Other things that come to my mind:
 - move handling of nowayout into the core
 - some drivers use a reboot notifier. Not sure what they are intended
   to do there. For i.MX2x it triggers a reboot; for gpio-wdt it tries
   to stop the watchdog.

Of course this doesn't mean this should addressed here. I like the
series in general.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1200017 — Re: [PATCH 0/8] watchdog: Add support for keepalives triggered by infrastructure

Hi Uwe,

On 08/04/2015 04:24 AM, Uwe Kleine-König wrote:
> Hello Guenter,
>
> On Mon, Aug 03, 2015 at 07:13:26PM -0700, Guenter Roeck wrote:
>> This patch set does not solve all limitations of the watchdog subsystem.
>> Specifically, it does not add support for the following features.
>>
>> - It is desirable to be able to specify a maximum early timeout,
>>    from booting the system to opening the watchdog device.
>> - Some watchdogs may require a minimum period of time between
>>    heartbeats. Examples are DA9062 and possibly AT91SAM9x.
>
> Other things that come to my mind:
>   - move handling of nowayout into the core
>   - some drivers use a reboot notifier. Not sure what they are intended
>     to do there. For i.MX2x it triggers a reboot; for gpio-wdt it tries
>     to stop the watchdog.
>
Be careful - those are different functions. The imx2 driver installs
a _restart_ handler, which indeed is supposed to restart the system.
The gpio wdt driver installs a _reboot_ notifier, which is supposed to
stop the watchdog. Other drivers install a shutdown callback in the
platform driver structure to do the same; not really sure what is better.

> Of course this doesn't mean this should addressed here. I like the
> series in general.
>

The list wasn't supposed to be exhaustive. Otherwise adding "support devm"
should have been there as well. I even have a patch hanging around somewhere
to do that. Guess I should have said something like "does not solve all
timeout related limitations ...".

Thanks,
Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1200318 — Re: [PATCH 0/8] watchdog: Add support for keepalives triggered by infrastructure

On 04/08/15 03:13, Guenter Roeck wrote:
> The watchdog infrastructure is currently purely passive, meaning
> it only passes information from user space to drivers and vice versa.
> 
> Since watchdog hardware tends to have its own quirks, this can result
> in quite complex watchdog drivers. A number of scanarios are especially common.
> 
> - A watchdog is always active and can not be disabled, or can not be disabled
>   once enabled. To support such hardware, watchdog drivers have to implement
>   their own timers and use those timers to trigger watchdog keepalives while
>   the watchdog device is not or not yet opened.
> - A variant of this is the desire to enable a watchdog as soon as its driver
>   has been instantiated, to protect the system while it is still booting up,
>   but the watchdog daemon is not yet running.

Just mentioning that patting the watchdog in the boot loader
(by patching grub etc.) can be a more general solution here as it
avoids hangs if the kernel crashes before it runs the watchdog driver,
which is especially true if PXE loaded across the net for example.
Also this tends to be better spaced between boot start and user space loading.

> - Some watchdogs have a very short maximum timeout, in the range of just a few
>   seconds. Such low timeouts are difficult if not impossible to support from
>   user space. Drivers supporting such watchdog hardware need to implement
>   a timer function to augment heartbeats from user space.

Fair enough.

thanks,
Pádraig.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1200330 — Re: [PATCH 0/8] watchdog: Add support for keepalives triggered by infrastructure

On 08/04/2015 04:43 PM, Pádraig Brady wrote:
> On 04/08/15 03:13, Guenter Roeck wrote:
>> The watchdog infrastructure is currently purely passive, meaning
>> it only passes information from user space to drivers and vice versa.
>>
>> Since watchdog hardware tends to have its own quirks, this can result
>> in quite complex watchdog drivers. A number of scanarios are especially common.
>>
>> - A watchdog is always active and can not be disabled, or can not be disabled
>>    once enabled. To support such hardware, watchdog drivers have to implement
>>    their own timers and use those timers to trigger watchdog keepalives while
>>    the watchdog device is not or not yet opened.
>> - A variant of this is the desire to enable a watchdog as soon as its driver
>>    has been instantiated, to protect the system while it is still booting up,
>>    but the watchdog daemon is not yet running.
>
> Just mentioning that patting the watchdog in the boot loader
> (by patching grub etc.) can be a more general solution here as it
> avoids hangs if the kernel crashes before it runs the watchdog driver,
> which is especially true if PXE loaded across the net for example.
> Also this tends to be better spaced between boot start and user space loading.
>

I understand. However, that is not always sufficient, and it may not work
well since grub would need to know about the actual watchdog hardware.
Also, there are systems where the time between "watchdog driver instantiated"
and "watchdog daemon started" is just too large for the maximum watchdog
hardware timeout.

The point here is that there _are_ many drivers which implement this
functionality in the driver code. Not counting the drivers I converted as an
exercise, "git grep mod_timer | cut -f1 -d: | sort -u | wc" in drivers/watchdog
suggests that there are 20 more drivers implementing their own heartbeat
management. With that, it just makes sense to move the functionality to
the infrastructure.

Thanks,
Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1200481 — Re: [PATCH 0/8] watchdog: Add support for keepalives triggered by infrastructure

Hello Pádraig,

On Wed, Aug 05, 2015 at 12:43:39AM +0100, Pádraig Brady wrote:
> On 04/08/15 03:13, Guenter Roeck wrote:
> > The watchdog infrastructure is currently purely passive, meaning
> > it only passes information from user space to drivers and vice versa.
> > 
> > Since watchdog hardware tends to have its own quirks, this can result
> > in quite complex watchdog drivers. A number of scanarios are especially common.
> > 
> > - A watchdog is always active and can not be disabled, or can not be disabled
> >   once enabled. To support such hardware, watchdog drivers have to implement
> >   their own timers and use those timers to trigger watchdog keepalives while
> >   the watchdog device is not or not yet opened.
> > - A variant of this is the desire to enable a watchdog as soon as its driver
> >   has been instantiated, to protect the system while it is still booting up,
> >   but the watchdog daemon is not yet running.
> 
> Just mentioning that patting the watchdog in the boot loader
> (by patching grub etc.) can be a more general solution here as it
> avoids hangs if the kernel crashes before it runs the watchdog driver,
> which is especially true if PXE loaded across the net for example.
> Also this tends to be better spaced between boot start and user space loading.

the watchdog I'm currently working with on a powerpc platform has a
unchangable timeout of ~1 s. To make the machine boot I patched the
bootloader and need some automatic pinging in the kernel before
userspace takes over.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1200494 — Re: [PATCH 0/8] watchdog: Add support for keepalives triggered by infrastructure

Hi Uwe,

On 08/05/2015 12:36 AM, Uwe Kleine-König wrote:
> Hello Pádraig,
>
> On Wed, Aug 05, 2015 at 12:43:39AM +0100, Pádraig Brady wrote:
>> On 04/08/15 03:13, Guenter Roeck wrote:
>>> The watchdog infrastructure is currently purely passive, meaning
>>> it only passes information from user space to drivers and vice versa.
>>>
>>> Since watchdog hardware tends to have its own quirks, this can result
>>> in quite complex watchdog drivers. A number of scanarios are especially common.
>>>
>>> - A watchdog is always active and can not be disabled, or can not be disabled
>>>    once enabled. To support such hardware, watchdog drivers have to implement
>>>    their own timers and use those timers to trigger watchdog keepalives while
>>>    the watchdog device is not or not yet opened.
>>> - A variant of this is the desire to enable a watchdog as soon as its driver
>>>    has been instantiated, to protect the system while it is still booting up,
>>>    but the watchdog daemon is not yet running.
>>
>> Just mentioning that patting the watchdog in the boot loader
>> (by patching grub etc.) can be a more general solution here as it
>> avoids hangs if the kernel crashes before it runs the watchdog driver,
>> which is especially true if PXE loaded across the net for example.
>> Also this tends to be better spaced between boot start and user space loading.
>
> the watchdog I'm currently working with on a powerpc platform has a
> unchangable timeout of ~1 s. To make the machine boot I patched the
> bootloader and need some automatic pinging in the kernel before
> userspace takes over.
>

Does using arch_initcall in the watchdog driver help here, or is that
still too slow and you really need to hack the kernel ?

Thanks,
Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1200524 — Re: [PATCH 0/8] watchdog: Add support for keepalives triggered by infrastructure

Hello Guenter,

On Wed, Aug 05, 2015 at 12:50:25AM -0700, Guenter Roeck wrote:
> On 08/05/2015 12:36 AM, Uwe Kleine-König wrote:
> >the watchdog I'm currently working with on a powerpc platform has a
> >unchangable timeout of ~1 s. To make the machine boot I patched the
> >bootloader and need some automatic pinging in the kernel before
> >userspace takes over.
> >
> 
> Does using arch_initcall in the watchdog driver help here, or is that
> still too slow and you really need to hack the kernel ?

I didn't do any measurements yet, but having the bootloader extracting
the kernel, and then send a ping before jumping into it is good enough
even without CONFIG_GPIO_WATCHDOG_ARCH_INITCALL to get the machine up.
(Userspace doesn't do any watchdog handling yet.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1201004 — Re: [PATCH 0/8] watchdog: Add support for keepalives triggered by infrastructure

On Mon, Aug 03, 2015 at 07:13:26PM -0700, Guenter Roeck wrote:
> - Some watchdogs have a very short maximum timeout, in the range of just a few
>   seconds. Such low timeouts are difficult if not impossible to support from
>   user space. Drivers supporting such watchdog hardware need to implement
>   a timer function to augment heartbeats from user space.

> - A new status flag, WDOG_RUNNING, informs the watchdog subsystem that a
>   watchdog is running, and that the watchdog subsystem needs to generate
>   heartbeat requests while the associated watchdog device is closed.

> Patch #2 adds timer functionality to the watchdog core. It solves the problem
> of short maximum hardware timeouts by augmenting heartbeats triggered from
> user space with internally triggered heartbeats.
> 
> Patch #3 adds functionality to generate heartbeats while the watchdog device is
> closed. It handles situation where where the watchdog is running after
> the driver has been instantiated, but the device is not yet opened,
> and post-close situations necessary if a watchdog can not be stopped.

These sound concerning because it seems that heartbeats could be generated
outside of the direct control of userspace.  I have a program that depends
on having direct control over whether heartbeats are generated (or more
specifically, *not* generated.)  If these new features introduce a new way
for heartbeats to be generated, is there a way I can detect or disable
that behavior from userspace?  Unwanted heartbeats could break my program
and may lead to data corruption.

A related issue from some years ago is the unfortunate fact that closing
the watchdog device also generates a heartbeat.  I'd like to disable that
also, and submitted a patch for it here:
http://www.spinics.net/lists/linux-watchdog/msg01477.html

(Without the patch, I have to work around it by closing the device
prematurely as a way to generate the potentially final heartbeat, and then
reopen it again if I want to continue the heartbeats.)

Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1201024 — Re: [PATCH 0/8] watchdog: Add support for keepalives triggered by infrastructure

Hi David,

On 08/05/2015 10:13 AM, David Teigland wrote:
> On Mon, Aug 03, 2015 at 07:13:26PM -0700, Guenter Roeck wrote:
>> - Some watchdogs have a very short maximum timeout, in the range of just a few
>>    seconds. Such low timeouts are difficult if not impossible to support from
>>    user space. Drivers supporting such watchdog hardware need to implement
>>    a timer function to augment heartbeats from user space.
>
>> - A new status flag, WDOG_RUNNING, informs the watchdog subsystem that a
>>    watchdog is running, and that the watchdog subsystem needs to generate
>>    heartbeat requests while the associated watchdog device is closed.
>
>> Patch #2 adds timer functionality to the watchdog core. It solves the problem
>> of short maximum hardware timeouts by augmenting heartbeats triggered from
>> user space with internally triggered heartbeats.
>>
>> Patch #3 adds functionality to generate heartbeats while the watchdog device is
>> closed. It handles situation where where the watchdog is running after
>> the driver has been instantiated, but the device is not yet opened,
>> and post-close situations necessary if a watchdog can not be stopped.
>
> These sound concerning because it seems that heartbeats could be generated
> outside of the direct control of userspace.  I have a program that depends
> on having direct control over whether heartbeats are generated (or more
> specifically, *not* generated.)  If these new features introduce a new way
> for heartbeats to be generated, is there a way I can detect or disable
> that behavior from userspace?  Unwanted heartbeats could break my program
> and may lead to data corruption.
>

Not really. The heartbeats will be generated such that the watchdog expires
no later that <last heartbeat from userspace + configured timeout>. I discussed
this already with Uwe; he had the same concern. This isn't in the current
version of the patch set, but it will be in the next version. That means
that nothing will change from user space perspective.

> A related issue from some years ago is the unfortunate fact that closing
> the watchdog device also generates a heartbeat.  I'd like to disable that
> also, and submitted a patch for it here:
> http://www.spinics.net/lists/linux-watchdog/msg01477.html
>

That is a different issue, though, and unrelated to this patch set.
Wim had a good point there: Presumably the problem you are trying to solve
applies to the entire system, not to a specific watchdog. What you are looking
for looks more like a system parameter, not like something to set with an ioctl
message. The reason here is that you'd still want to be able to use standard
applications such as systemd or watchdogd to trigger heartbeats, and not depend
on your own.

Thanks,
Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1201028 — Re: [PATCH 0/8] watchdog: Add support for keepalives triggered by infrastructure

On Wed, Aug 05, 2015 at 10:41:51AM -0700, Guenter Roeck wrote:
> Not really. The heartbeats will be generated such that the watchdog expires
> no later that <last heartbeat from userspace + configured timeout>. I discussed
> this already with Uwe; he had the same concern. This isn't in the current
> version of the patch set, but it will be in the next version. That means
> that nothing will change from user space perspective.

Sounds good, thanks.

> >A related issue from some years ago is the unfortunate fact that closing
> >the watchdog device also generates a heartbeat.  I'd like to disable that
> >also, and submitted a patch for it here:
> >http://www.spinics.net/lists/linux-watchdog/msg01477.html
> >
> 
> That is a different issue, though, and unrelated to this patch set.
> Wim had a good point there: Presumably the problem you are trying to solve
> applies to the entire system, not to a specific watchdog. What you are looking
> for looks more like a system parameter, not like something to set with an ioctl
> message. The reason here is that you'd still want to be able to use standard
> applications such as systemd or watchdogd to trigger heartbeats, and not depend
> on your own.

I'd need this behavior when the system is running my program (sanlock with
wdmd), which uses /dev/watchdog.  No other programs (systemd or watchdogd)
could be using /dev/watchdog at the same time.

Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1201067 — Re: [PATCH 0/8] watchdog: Add support for keepalives triggered by infrastructure

Hi David,

On 08/05/2015 10:51 AM, David Teigland wrote:
> On Wed, Aug 05, 2015 at 10:41:51AM -0700, Guenter Roeck wrote:
>> Not really. The heartbeats will be generated such that the watchdog expires
>> no later that <last heartbeat from userspace + configured timeout>. I discussed
>> this already with Uwe; he had the same concern. This isn't in the current
>> version of the patch set, but it will be in the next version. That means
>> that nothing will change from user space perspective.
>
> Sounds good, thanks.
>
>>> A related issue from some years ago is the unfortunate fact that closing
>>> the watchdog device also generates a heartbeat.  I'd like to disable that
>>> also, and submitted a patch for it here:
>>> http://www.spinics.net/lists/linux-watchdog/msg01477.html
>>>
>>
>> That is a different issue, though, and unrelated to this patch set.
>> Wim had a good point there: Presumably the problem you are trying to solve
>> applies to the entire system, not to a specific watchdog. What you are looking
>> for looks more like a system parameter, not like something to set with an ioctl
>> message. The reason here is that you'd still want to be able to use standard
>> applications such as systemd or watchdogd to trigger heartbeats, and not depend
>> on your own.
>
> I'd need this behavior when the system is running my program (sanlock with
> wdmd), which uses /dev/watchdog.  No other programs (systemd or watchdogd)
> could be using /dev/watchdog at the same time.
>

I think I can understand why Wim was reluctant to accept your patch;
I must admit I don't understand your use case either.

I wonder if you are actually mis-using the watchdog subsystem to generate
hard resets. After all, you could avoid the unexpected close situation with
an exit handler in your application. That handler could catch anything but
SIGKILL, but anyone using SIGKILL doesn't really deserve better. If the intent
is to reset the system after the application closes, executing "/sbin/restart -f"
might be a safer approach than just killing the watchdog.

In addition to that, I don't think it is a good idea to rely on the assumption
that the watchdog will expire exactly after the configured timeout.
Many watchdog drivers implement a soft timeout on top of the hardware timeout,
and thus already implement the internal heartbeat. Most of those drivers
will stop sending internal heartbeats if user space did not send a heartbeat
within the configured timeout period. The actual reset will then occur later,
after the actual hardware watchdog timed out. This can be as much as the
hardware timeout period, which may be substantial.

Thanks,
Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1201086 — Re: [PATCH 0/8] watchdog: Add support for keepalives triggered by infrastructure

On Wed, Aug 05, 2015 at 12:01:38PM -0700, Guenter Roeck wrote:
> I think I can understand why Wim was reluctant to accept your patch;
> I must admit I don't understand your use case either.

Very breifly, sanlock is a shared storage based lease manager, and the
expiration of a lease is tied to the expiration of the watchdog.  I have
to ensure that the watchdog expires at or before the time that the lease
expires.  This means that I cannot allow a watchdog heartbeat apart from a
corresponding lease renewal on the shared storage.  Otherwise, the
calculation by other hosts of the time of the hard reset will be wrong,
and the data on shared storage could be corrupted.

> I wonder if you are actually mis-using the watchdog subsystem to generate
> hard resets.

I am indeed using it to generate hard resets.

> After all, you could avoid the unexpected close situation with
> an exit handler in your application. That handler could catch anything but
> SIGKILL, but anyone using SIGKILL doesn't really deserve better.

I avoid the unexpected close situation by prematurely closing the device
to generate the heartbeat from close, and then reopening if needed.  That
covers the SIGKILL case.  So, I have a work around, but the patch would
still be nice.

> If the intent is to reset the system after the application closes,
> executing "/sbin/restart -f" might be a safer approach than just killing
> the watchdog.

I need to reset the system if the application crashes, or if the
application is running but can't renew its lease.  In the former case,
executing something doesn't work.  In the later case, I have done similar
(with /proc/sysrq-trigger), but it doesn't always apply and I'd still want
the hardware reset as redundancy.

> In addition to that, I don't think it is a good idea to rely on the assumption
> that the watchdog will expire exactly after the configured timeout.
> Many watchdog drivers implement a soft timeout on top of the hardware timeout,
> and thus already implement the internal heartbeat. Most of those drivers
> will stop sending internal heartbeats if user space did not send a heartbeat
> within the configured timeout period. The actual reset will then occur later,
> after the actual hardware watchdog timed out. This can be as much as the
> hardware timeout period, which may be substantial.

OK, thanks, I'll look into this in more detail.  Is there a way I can
identify which cases these are, or do you know an example I can look at?
In the worst case I'd have to extend the lease expiration time by a full
timeout period when the dubious drivers are used.

Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1201125 — Re: [PATCH 0/8] watchdog: Add support for keepalives triggered by infrastructure

Hi David,

On 08/05/2015 12:51 PM, David Teigland wrote:
> On Wed, Aug 05, 2015 at 12:01:38PM -0700, Guenter Roeck wrote:
>> I think I can understand why Wim was reluctant to accept your patch;
>> I must admit I don't understand your use case either.
>
> Very breifly, sanlock is a shared storage based lease manager, and the
> expiration of a lease is tied to the expiration of the watchdog.  I have
> to ensure that the watchdog expires at or before the time that the lease
> expires.  This means that I cannot allow a watchdog heartbeat apart from a
> corresponding lease renewal on the shared storage.  Otherwise, the
> calculation by other hosts of the time of the hard reset will be wrong,
> and the data on shared storage could be corrupted.
>
>> I wonder if you are actually mis-using the watchdog subsystem to generate
>> hard resets.
>
> I am indeed using it to generate hard resets.
>
So there is no concern that the hard reset may corrupt some data ? Interesting.
Hope you don't use any SSDs - some of those don't like that.

>> After all, you could avoid the unexpected close situation with
>> an exit handler in your application. That handler could catch anything but
>> SIGKILL, but anyone using SIGKILL doesn't really deserve better.
>
> I avoid the unexpected close situation by prematurely closing the device
> to generate the heartbeat from close, and then reopening if needed.  That
> covers the SIGKILL case.  So, I have a work around, but the patch would
> still be nice.
>
Sounds messy....

>> If the intent is to reset the system after the application closes,
>> executing "/sbin/restart -f" might be a safer approach than just killing
>> the watchdog.
>
> I need to reset the system if the application crashes, or if the
> application is running but can't renew its lease.  In the former case,
> executing something doesn't work.  In the later case, I have done similar

Maybe you could have the system monitor (systemd or whatever it is) the
application and run /sbin/restart if it crashes. Essentially monitor
the application from the outside.

> (with /proc/sysrq-trigger), but it doesn't always apply and I'd still want
> the hardware reset as redundancy.
>
>> In addition to that, I don't think it is a good idea to rely on the assumption
>> that the watchdog will expire exactly after the configured timeout.
>> Many watchdog drivers implement a soft timeout on top of the hardware timeout,
>> and thus already implement the internal heartbeat. Most of those drivers
>> will stop sending internal heartbeats if user space did not send a heartbeat
>> within the configured timeout period. The actual reset will then occur later,
>> after the actual hardware watchdog timed out. This can be as much as the
>> hardware timeout period, which may be substantial.
>
> OK, thanks, I'll look into this in more detail.  Is there a way I can
> identify which cases these are, or do you know an example I can look at?
> In the worst case I'd have to extend the lease expiration time by a full
> timeout period when the dubious drivers are used.
>

git grep mod_timer drivers/watchdog | cut -f1 -d: | sort -u

gives you the following list:

alim7101_wdt.c
at91sam9_wdt.c
bcm47xx_wdt.c
bcm63xx_wdt.c
cpu5wdt.c
dw_wdt.c
ep93xx_wdt.c
gpio_wdt.c
imx2_wdt.c
machzwd.c
mixcomwd.c
mpc8xxx_wdt.c
mtx-1_wdt.c
nuc900_wdt.c
pcwd.c
pika_wdt.c
rdc321x_wdt.c
sbc60xxwdt.c
sc520_wdt.c
shwdt.c
softdog.c
via_wdt.c
w83877f_wdt.c

Those would be the immediate candidates to look out for. Note that this situation
will actually improve with my patch set, since it tries to tie the actual expiry
to the configured timeout. This will only work if the driver(s) are converted
to use the new infrastructure, of course.

Still, the ABI guarantees that "the hardware watchdog will reset the system
(causing a reboot) after the timeout occurs", but that doesn't mean that it
will reset the system immediately. I think the only safe guarantee is that
it won't reset the system as long as the timeout did _not_ occur. Extending
the lease expiration in your application by a timeout period will help,
but there is still no _guarantee_ that the reset will occur within
"expiration time + timeout".

I am not even sure if all watchdog drivers which don't implement a soft timer
will always timeout exactly after "timeout" seconds. It will be "at least
timeout" seconds, but I would not bet that it is always the exact time.

Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web