[PATCH v6 00/12] handling all DM watchdogs in watchdog_reset()

[PATCH v6 00/12] handling all DM watchdogs in watchdog_reset()

[Rasmus Villemoes]

This series is an attempt at expanding the wdt-uclass provided
watchdog_reset() to handle all DM watchdogs, not just the first
one. Some of the ad hoc work done for the first DM watchdog in
initr_watchdog() is now moved to a .pre_probe hook so it is
automatically done for all devices.

It also includes a patch adding a driver for a gpio-petted watchdog
device (and a sandbox test of that) - it is included here because that
also gives a relatively easy way to have more than one (kind of)
watchdog device in the sandbox, which is then used at the end to test
that watchdog_reset() behaves as expected.

v2: https://lore.kernel.org/u-boot/20210527220017.1266765-1-rasmus.villemoes@prevas.dk/

v3: https://lore.kernel.org/u-boot/20210702124510.124401-1-rasmus.villemoes@prevas.dk/

v4: https://lore.kernel.org/u-boot/20210802150016.588750-1-rasmus.villemoes@prevas.dk/

v5: https://lore.kernel.org/u-boot/20210811124800.2593226-1-rasmus.villemoes@prevas.dk/

Changes in v6: Make wdt_stop_all() return the first error encountered
(if any), yet still visit all watchdog devices.

Rasmus Villemoes (12):
  watchdog: wdt-uclass.c: use wdt_start() in wdt_expire_now()
  watchdog: wdt-uclass.c: introduce struct wdt_priv
  watchdog: wdt-uclass.c: neaten UCLASS_DRIVER definition
  watchdog: wdt-uclass.c: refactor initr_watchdog()
  watchdog: wdt-uclass.c: keep track of each device's running state
  sandbox: disable CONFIG_WATCHDOG_AUTOSTART
  watchdog: wdt-uclass.c: add wdt_stop_all() helper
  board: x530: switch to wdt_stop_all()
  watchdog: wdt-uclass.c: handle all DM watchdogs in watchdog_reset()
  watchdog: add gpio watchdog driver
  sandbox: add test of wdt_gpio driver
  sandbox: add test of wdt-uclass' watchdog_reset()

 arch/sandbox/dts/test.dts                     |   8 +
 board/alliedtelesis/x530/x530.c               |   5 +-
 configs/sandbox64_defconfig                   |   2 +
 configs/sandbox_defconfig                     |   2 +
 .../watchdog/gpio-wdt.txt                     |  19 ++
 drivers/watchdog/Kconfig                      |   9 +
 drivers/watchdog/Makefile                     |   1 +
 drivers/watchdog/gpio_wdt.c                   |  68 +++++++
 drivers/watchdog/wdt-uclass.c                 | 192 +++++++++++++-----
 include/asm-generic/global_data.h             |   6 -
 include/wdt.h                                 |   8 +
 test/dm/wdt.c                                 |  90 +++++++-
 12 files changed, 349 insertions(+), 61 deletions(-)
 create mode 100644 doc/device-tree-bindings/watchdog/gpio-wdt.txt
 create mode 100644 drivers/watchdog/gpio_wdt.c

-- 
2.31.1

[PATCH v6 01/12] watchdog: wdt-uclass.c: use wdt_start() in wdt_expire_now()

[Rasmus Villemoes]

wdt_start() does the "no ->start? return -ENOSYS" check, don't
open-code that in wdt_expire_now().

Also, wdt_start() maintains some global (and later some per-device)
state, which would get out of sync with this direct method call - not
that it matters much here since the board is supposed to reset very
soon.

Reviewed-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Stefan Roese <sr@denx.de>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/watchdog/wdt-uclass.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
index 17334dbda6..df8164da2a 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -120,10 +120,8 @@ int wdt_expire_now(struct udevice *dev, ulong flags)
 	if (ops->expire_now) {
 		return ops->expire_now(dev, flags);
 	} else {
-		if (!ops->start)
-			return -ENOSYS;
+		ret = wdt_start(dev, 1, flags);
 
-		ret = ops->start(dev, 1, flags);
 		if (ret < 0)
 			return ret;
 
-- 
2.31.1

[PATCH v6 02/12] watchdog: wdt-uclass.c: introduce struct wdt_priv

[Rasmus Villemoes]

As preparation for having the wdt-uclass provided watchdog_reset()
function handle all DM watchdog devices, and not just the first such,
introduce a uclass-owned struct to hold the reset_period and
next_reset, so these become per-device instead of being static
variables.

No functional change intended.

Reviewed-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Stefan Roese <sr@denx.de>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/watchdog/wdt-uclass.c | 74 +++++++++++++++++++++++++----------
 1 file changed, 54 insertions(+), 20 deletions(-)

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
index df8164da2a..b29d214724 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -20,15 +20,24 @@ DECLARE_GLOBAL_DATA_PTR;
 
 #define WATCHDOG_TIMEOUT_SECS	(CONFIG_WATCHDOG_TIMEOUT_MSECS / 1000)
 
-/*
- * Reset every 1000ms, or however often is required as indicated by a
- * hw_margin_ms property.
- */
-static ulong reset_period = 1000;
+struct wdt_priv {
+	/* Timeout, in seconds, to configure this device to. */
+	u32 timeout;
+	/*
+	 * Time, in milliseconds, between calling the device's ->reset()
+	 * method from watchdog_reset().
+	 */
+	ulong reset_period;
+	/*
+	 * Next time (as returned by get_timer(0)) to call
+	 * ->reset().
+	 */
+	ulong next_reset;
+};
 
 int initr_watchdog(void)
 {
-	u32 timeout = WATCHDOG_TIMEOUT_SECS;
+	struct wdt_priv *priv;
 	int ret;
 
 	/*
@@ -44,28 +53,21 @@ int initr_watchdog(void)
 			return 0;
 		}
 	}
-
-	if (CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA)) {
-		timeout = dev_read_u32_default(gd->watchdog_dev, "timeout-sec",
-					       WATCHDOG_TIMEOUT_SECS);
-		reset_period = dev_read_u32_default(gd->watchdog_dev,
-						    "hw_margin_ms",
-						    4 * reset_period) / 4;
-	}
+	priv = dev_get_uclass_priv(gd->watchdog_dev);
 
 	if (!IS_ENABLED(CONFIG_WATCHDOG_AUTOSTART)) {
 		printf("WDT:   Not starting\n");
 		return 0;
 	}
 
-	ret = wdt_start(gd->watchdog_dev, timeout * 1000, 0);
+	ret = wdt_start(gd->watchdog_dev, priv->timeout * 1000, 0);
 	if (ret != 0) {
 		printf("WDT:   Failed to start\n");
 		return 0;
 	}
 
 	printf("WDT:   Started with%s servicing (%ds timeout)\n",
-	       IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", timeout);
+	       IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", priv->timeout);
 
 	return 0;
 }
@@ -139,18 +141,21 @@ int wdt_expire_now(struct udevice *dev, ulong flags)
  */
 void watchdog_reset(void)
 {
-	static ulong next_reset;
+	struct wdt_priv *priv;
+	struct udevice *dev;
 	ulong now;
 
 	/* Exit if GD is not ready or watchdog is not initialized yet */
 	if (!gd || !(gd->flags & GD_FLG_WDT_READY))
 		return;
 
+	dev = gd->watchdog_dev;
+	priv = dev_get_uclass_priv(dev);
 	/* Do not reset the watchdog too often */
 	now = get_timer(0);
-	if (time_after_eq(now, next_reset)) {
-		next_reset = now + reset_period;
-		wdt_reset(gd->watchdog_dev);
+	if (time_after_eq(now, priv->next_reset)) {
+		priv->next_reset = now + priv->reset_period;
+		wdt_reset(dev);
 	}
 }
 #endif
@@ -177,9 +182,38 @@ static int wdt_post_bind(struct udevice *dev)
 	return 0;
 }
 
+static int wdt_pre_probe(struct udevice *dev)
+{
+	u32 timeout = WATCHDOG_TIMEOUT_SECS;
+	/*
+	 * Reset every 1000ms, or however often is required as
+	 * indicated by a hw_margin_ms property.
+	 */
+	ulong reset_period = 1000;
+	struct wdt_priv *priv;
+
+	if (CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA)) {
+		timeout = dev_read_u32_default(dev, "timeout-sec", timeout);
+		reset_period = dev_read_u32_default(dev, "hw_margin_ms",
+						    4 * reset_period) / 4;
+	}
+	priv = dev_get_uclass_priv(dev);
+	priv->timeout = timeout;
+	priv->reset_period = reset_period;
+	/*
+	 * Pretend this device was last reset "long" ago so the first
+	 * watchdog_reset will actually call its ->reset method.
+	 */
+	priv->next_reset = get_timer(0);
+
+	return 0;
+}
+
 UCLASS_DRIVER(wdt) = {
 	.id		= UCLASS_WDT,
 	.name		= "watchdog",
 	.flags		= DM_UC_FLAG_SEQ_ALIAS,
 	.post_bind	= wdt_post_bind,
+	.pre_probe		= wdt_pre_probe,
+	.per_device_auto	= sizeof(struct wdt_priv),
 };
-- 
2.31.1

[PATCH v6 03/12] watchdog: wdt-uclass.c: neaten UCLASS_DRIVER definition

[Rasmus Villemoes]

The addition of .pre_probe and .per_device_auto made this look
bad. Fix it.

Reviewed-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Stefan Roese <sr@denx.de>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/watchdog/wdt-uclass.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
index b29d214724..81287c759a 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -210,10 +210,10 @@ static int wdt_pre_probe(struct udevice *dev)
 }
 
 UCLASS_DRIVER(wdt) = {
-	.id		= UCLASS_WDT,
-	.name		= "watchdog",
-	.flags		= DM_UC_FLAG_SEQ_ALIAS,
-	.post_bind	= wdt_post_bind,
+	.id			= UCLASS_WDT,
+	.name			= "watchdog",
+	.flags			= DM_UC_FLAG_SEQ_ALIAS,
+	.post_bind		= wdt_post_bind,
 	.pre_probe		= wdt_pre_probe,
 	.per_device_auto	= sizeof(struct wdt_priv),
 };
-- 
2.31.1

[PATCH v6 04/12] watchdog: wdt-uclass.c: refactor initr_watchdog()

[Rasmus Villemoes]

In preparation for handling all DM watchdogs in watchdog_reset(), pull
out the code which handles starting (or not) the gd->watchdog_dev
device.

Include the device name in various printfs.

Reviewed-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Stefan Roese <sr@denx.de>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/watchdog/wdt-uclass.c | 37 ++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
index 81287c759a..0a1f43771c 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -35,11 +35,30 @@ struct wdt_priv {
 	ulong next_reset;
 };
 
-int initr_watchdog(void)
+static void init_watchdog_dev(struct udevice *dev)
 {
 	struct wdt_priv *priv;
 	int ret;
 
+	priv = dev_get_uclass_priv(dev);
+
+	if (!IS_ENABLED(CONFIG_WATCHDOG_AUTOSTART)) {
+		printf("WDT:   Not starting %s\n", dev->name);
+		return;
+	}
+
+	ret = wdt_start(dev, priv->timeout * 1000, 0);
+	if (ret != 0) {
+		printf("WDT:   Failed to start %s\n", dev->name);
+		return;
+	}
+
+	printf("WDT:   Started %s with%s servicing (%ds timeout)\n", dev->name,
+	       IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", priv->timeout);
+}
+
+int initr_watchdog(void)
+{
 	/*
 	 * Init watchdog: This will call the probe function of the
 	 * watchdog driver, enabling the use of the device
@@ -53,21 +72,7 @@ int initr_watchdog(void)
 			return 0;
 		}
 	}
-	priv = dev_get_uclass_priv(gd->watchdog_dev);
-
-	if (!IS_ENABLED(CONFIG_WATCHDOG_AUTOSTART)) {
-		printf("WDT:   Not starting\n");
-		return 0;
-	}
-
-	ret = wdt_start(gd->watchdog_dev, priv->timeout * 1000, 0);
-	if (ret != 0) {
-		printf("WDT:   Failed to start\n");
-		return 0;
-	}
-
-	printf("WDT:   Started with%s servicing (%ds timeout)\n",
-	       IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", priv->timeout);
+	init_watchdog_dev(gd->watchdog_dev);
 
 	return 0;
 }
-- 
2.31.1

[PATCH v6 05/12] watchdog: wdt-uclass.c: keep track of each device's running state

[Rasmus Villemoes]

As a step towards handling all DM watchdogs in watchdog_reset(), use a
per-device flag to keep track of whether the device has been started
instead of a bit in gd->flags.

We will still need that bit to know whether we are past
initr_watchdog() and hence have populated gd->watchdog_dev -
incidentally, that is how it was used prior to commit 9c44ff1c5f3c.

Reviewed-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Stefan Roese <sr@denx.de>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/watchdog/wdt-uclass.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
index 0a1f43771c..358fc68e27 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -33,6 +33,8 @@ struct wdt_priv {
 	 * ->reset().
 	 */
 	ulong next_reset;
+	/* Whether watchdog_start() has been called on the device. */
+	bool running;
 };
 
 static void init_watchdog_dev(struct udevice *dev)
@@ -74,6 +76,7 @@ int initr_watchdog(void)
 	}
 	init_watchdog_dev(gd->watchdog_dev);
 
+	gd->flags |= GD_FLG_WDT_READY;
 	return 0;
 }
 
@@ -86,8 +89,11 @@ int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags)
 		return -ENOSYS;
 
 	ret = ops->start(dev, timeout_ms, flags);
-	if (ret == 0)
-		gd->flags |= GD_FLG_WDT_READY;
+	if (ret == 0) {
+		struct wdt_priv *priv = dev_get_uclass_priv(dev);
+
+		priv->running = true;
+	}
 
 	return ret;
 }
@@ -101,8 +107,11 @@ int wdt_stop(struct udevice *dev)
 		return -ENOSYS;
 
 	ret = ops->stop(dev);
-	if (ret == 0)
-		gd->flags &= ~GD_FLG_WDT_READY;
+	if (ret == 0) {
+		struct wdt_priv *priv = dev_get_uclass_priv(dev);
+
+		priv->running = false;
+	}
 
 	return ret;
 }
@@ -156,6 +165,9 @@ void watchdog_reset(void)
 
 	dev = gd->watchdog_dev;
 	priv = dev_get_uclass_priv(dev);
+	if (!priv->running)
+		return;
+
 	/* Do not reset the watchdog too often */
 	now = get_timer(0);
 	if (time_after_eq(now, priv->next_reset)) {
-- 
2.31.1

Re: [PATCH v6 05/12] watchdog: wdt-uclass.c: keep track of each device's running state

[Wolfgang Denk]

Dear Rasmus,

please check your patches for proper error handling.

In message <20210819095706.3585923-6-rasmus.villemoes@prevas.dk> you wrote:
>
...
> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
> index 0a1f43771c..358fc68e27 100644
> --- a/drivers/watchdog/wdt-uclass.c
> +++ b/drivers/watchdog/wdt-uclass.c
...
> @@ -86,8 +89,11 @@ int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags)
>  		return -ENOSYS;
>  
>  	ret = ops->start(dev, timeout_ms, flags);
> -	if (ret == 0)
> -		gd->flags |= GD_FLG_WDT_READY;
> +	if (ret == 0) {
> +		struct wdt_priv *priv = dev_get_uclass_priv(dev);
> +
> +		priv->running = true;

dev_get_uclass_priv() can return NULL, in which case you would be
dereferencing a NULL pointer...

> @@ -101,8 +107,11 @@ int wdt_stop(struct udevice *dev)
>  		return -ENOSYS;
>  
>  	ret = ops->stop(dev);
> -	if (ret == 0)
> -		gd->flags &= ~GD_FLG_WDT_READY;
> +	if (ret == 0) {
> +		struct wdt_priv *priv = dev_get_uclass_priv(dev);
> +
> +		priv->running = false;

Same here.

> @@ -156,6 +165,9 @@ void watchdog_reset(void)
>  
>  	dev = gd->watchdog_dev;
>  	priv = dev_get_uclass_priv(dev);
> +	if (!priv->running)
> +		return;
> +

And here again.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
I used to be indecisive, now I'm not sure.

[PATCH v6 06/12] sandbox: disable CONFIG_WATCHDOG_AUTOSTART

[Rasmus Villemoes]

For the unit tests, it is more convenient if the tests are in charge
of when the watchdog devices are started and stopped, so prevent
wdt-uclass from doing it automatically.

Reviewed-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Stefan Roese <sr@denx.de>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 configs/sandbox64_defconfig | 1 +
 configs/sandbox_defconfig   | 1 +
 2 files changed, 2 insertions(+)

diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
index f7098b4969..3627a066b4 100644
--- a/configs/sandbox64_defconfig
+++ b/configs/sandbox64_defconfig
@@ -223,6 +223,7 @@ CONFIG_OSD=y
 CONFIG_SANDBOX_OSD=y
 CONFIG_SPLASH_SCREEN_ALIGN=y
 CONFIG_VIDEO_BMP_RLE8=y
+# CONFIG_WATCHDOG_AUTOSTART is not set
 CONFIG_WDT=y
 CONFIG_WDT_SANDBOX=y
 CONFIG_FS_CBFS=y
diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index bcd82f76ff..34b749b47b 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -282,6 +282,7 @@ CONFIG_W1=y
 CONFIG_W1_GPIO=y
 CONFIG_W1_EEPROM=y
 CONFIG_W1_EEPROM_SANDBOX=y
+# CONFIG_WATCHDOG_AUTOSTART is not set
 CONFIG_WDT=y
 CONFIG_WDT_SANDBOX=y
 CONFIG_FS_CBFS=y
-- 
2.31.1

[PATCH v6 07/12] watchdog: wdt-uclass.c: add wdt_stop_all() helper

[Rasmus Villemoes]

Since the watchdog_dev member of struct global_data is going away in
favor of the wdt-uclass handling all watchdog devices, prepare for
that by adding a helper to call wdt_stop() on all known devices.

If an error is encountered, still do wdt_stop() on remaining devices,
but remember and return the first error seen.

Initially, this will only be used in one single
place (board/alliedtelesis/x530/x530.c).

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/watchdog/wdt-uclass.c | 25 +++++++++++++++++++++++++
 include/wdt.h                 |  8 ++++++++
 2 files changed, 33 insertions(+)

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
index 358fc68e27..5b1c0df5d6 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -116,6 +116,31 @@ int wdt_stop(struct udevice *dev)
 	return ret;
 }
 
+int wdt_stop_all(void)
+{
+	struct wdt_priv *priv;
+	struct udevice *dev;
+	struct uclass *uc;
+	int ret, err;
+
+	ret = uclass_get(UCLASS_WDT, &uc);
+	if (ret)
+		return ret;
+
+	uclass_foreach_dev(dev, uc) {
+		if (!device_active(dev))
+			continue;
+		priv = dev_get_uclass_priv(dev);
+		if (!priv->running)
+			continue;
+		err = wdt_stop(dev);
+		if (!ret)
+			ret = err;
+	}
+
+	return ret;
+}
+
 int wdt_reset(struct udevice *dev)
 {
 	const struct wdt_ops *ops = device_get_ops(dev);
diff --git a/include/wdt.h b/include/wdt.h
index bc242c2eb2..baaa9db08a 100644
--- a/include/wdt.h
+++ b/include/wdt.h
@@ -37,6 +37,14 @@ int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags);
  */
 int wdt_stop(struct udevice *dev);
 
+/*
+ * Stop all registered watchdog devices.
+ *
+ * @return: 0 if ok, first error encountered otherwise (but wdt_stop()
+ * is still called on following devices)
+ */
+int wdt_stop_all(void);
+
 /*
  * Reset the timer, typically restoring the counter to
  * the value configured by start()
-- 
2.31.1

Re: [PATCH v6 07/12] watchdog: wdt-uclass.c: add wdt_stop_all() helper

[Wolfgang Denk]

Dear Rasmus,

again: error handling.

In message <20210819095706.3585923-8-rasmus.villemoes@prevas.dk> you wrote:
>
> --- a/drivers/watchdog/wdt-uclass.c
> +++ b/drivers/watchdog/wdt-uclass.c
> @@ -116,6 +116,31 @@ int wdt_stop(struct udevice *dev)
>  	return ret;
>  }
>  
> +int wdt_stop_all(void)
> +{
> +	struct wdt_priv *priv;
> +	struct udevice *dev;
> +	struct uclass *uc;
> +	int ret, err;
> +
> +	ret = uclass_get(UCLASS_WDT, &uc);
> +	if (ret)
> +		return ret;
> +
> +	uclass_foreach_dev(dev, uc) {
> +		if (!device_active(dev))
> +			continue;
> +		priv = dev_get_uclass_priv(dev);
> +		if (!priv->running)
> +			continue;

Potential NULL pointer dereferencing.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
I paid too much for it, but its worth it.

[PATCH v6 08/12] board: x530: switch to wdt_stop_all()

[Rasmus Villemoes]

Since the gd->watchdog_dev member is going away, switch to using the
new wdt_stop_all() helper.

While here, clean up the preprocessor conditional: The ->watchdog_dev
member is actually guarded by CONFIG_WDT [disabling that in
x530_defconfig while keeping CONFIG_WATCHDOG breaks the build], and in
the new world order so is the existence of the wdt_stop_all()
function.

Actually, existence of wdt_stop_all() depends on CONFIG_${SPL_}WDT, so
really spell the condition using CONFIG_IS_ENABLED, and make it a C
rather than cpp if.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 board/alliedtelesis/x530/x530.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/board/alliedtelesis/x530/x530.c b/board/alliedtelesis/x530/x530.c
index 7bcfa828d7..8b31045a07 100644
--- a/board/alliedtelesis/x530/x530.c
+++ b/board/alliedtelesis/x530/x530.c
@@ -121,9 +121,8 @@ int board_init(void)
 
 void arch_preboot_os(void)
 {
-#ifdef CONFIG_WATCHDOG
-	wdt_stop(gd->watchdog_dev);
-#endif
+	if (CONFIG_IS_ENABLED(WDT))
+		wdt_stop_all();
 }
 
 static int led_7seg_init(unsigned int segments)
-- 
2.31.1

[PATCH v6 09/12] watchdog: wdt-uclass.c: handle all DM watchdogs in watchdog_reset()

[Rasmus Villemoes]

A board can have and make use of more than one watchdog device, say
one built into the SOC and an external gpio-petted one. Having
wdt-uclass only handle the first is both a little arbitrary and
unexpected.

So change initr_watchdog() so we visit (probe) all DM watchdog
devices, and call the init_watchdog_dev helper for each.

Similarly let watchdog_reset() loop over the whole uclass - each
having their own ratelimiting metadata, and a separate "is this device
running" flag.

This gets rid of the watchdog_dev member of struct global_data.  We
do, however, still need the GD_FLG_WDT_READY set in
initr_watchdog(). This is because watchdog_reset() can get called
before DM is ready, and I don't think we can call uclass_get() that
early.

The current code just returns 0 if "getting" the first device fails -
that can of course happen because there are no devices, but it could
also happen if its ->probe call failed. In keeping with that, continue
with the handling of the remaining devices even if one fails to
probe. This is also why we cannot use uclass_probe_all().

If desired, it's possible to later add a per-device "u-boot,autostart"
boolean property, so that one can do CONFIG_WATCHDOG_AUTOSTART
per-device.

Reviewed-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Stefan Roese <sr@denx.de>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/watchdog/wdt-uclass.c     | 56 ++++++++++++++++++++-----------
 include/asm-generic/global_data.h |  6 ----
 2 files changed, 36 insertions(+), 26 deletions(-)

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
index 5b1c0df5d6..7570710c4d 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -61,20 +61,24 @@ static void init_watchdog_dev(struct udevice *dev)
 
 int initr_watchdog(void)
 {
-	/*
-	 * Init watchdog: This will call the probe function of the
-	 * watchdog driver, enabling the use of the device
-	 */
-	if (uclass_get_device_by_seq(UCLASS_WDT, 0,
-				     (struct udevice **)&gd->watchdog_dev)) {
-		debug("WDT:   Not found by seq!\n");
-		if (uclass_get_device(UCLASS_WDT, 0,
-				      (struct udevice **)&gd->watchdog_dev)) {
-			printf("WDT:   Not found!\n");
-			return 0;
+	struct udevice *dev;
+	struct uclass *uc;
+	int ret;
+
+	ret = uclass_get(UCLASS_WDT, &uc);
+	if (ret) {
+		log_debug("Error getting UCLASS_WDT: %d\n", ret);
+		return 0;
+	}
+
+	uclass_foreach_dev(dev, uc) {
+		ret = device_probe(dev);
+		if (ret) {
+			log_debug("Error probing %s: %d\n", dev->name, ret);
+			continue;
 		}
+		init_watchdog_dev(dev);
 	}
-	init_watchdog_dev(gd->watchdog_dev);
 
 	gd->flags |= GD_FLG_WDT_READY;
 	return 0;
@@ -182,22 +186,34 @@ void watchdog_reset(void)
 {
 	struct wdt_priv *priv;
 	struct udevice *dev;
+	struct uclass *uc;
 	ulong now;
 
 	/* Exit if GD is not ready or watchdog is not initialized yet */
 	if (!gd || !(gd->flags & GD_FLG_WDT_READY))
 		return;
 
-	dev = gd->watchdog_dev;
-	priv = dev_get_uclass_priv(dev);
-	if (!priv->running)
+	if (uclass_get(UCLASS_WDT, &uc))
 		return;
 
-	/* Do not reset the watchdog too often */
-	now = get_timer(0);
-	if (time_after_eq(now, priv->next_reset)) {
-		priv->next_reset = now + priv->reset_period;
-		wdt_reset(dev);
+	/*
+	 * All devices bound to the wdt uclass should have been probed
+	 * in initr_watchdog(). But just in case something went wrong,
+	 * check device_active() before accessing the uclass private
+	 * data.
+	 */
+	uclass_foreach_dev(dev, uc) {
+		if (!device_active(dev))
+			continue;
+		priv = dev_get_uclass_priv(dev);
+		if (!priv->running)
+			continue;
+		/* Do not reset the watchdog too often */
+		now = get_timer(0);
+		if (time_after_eq(now, priv->next_reset)) {
+			priv->next_reset = now + priv->reset_period;
+			wdt_reset(dev);
+		}
 	}
 }
 #endif
diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
index e55070303f..28d749538c 100644
--- a/include/asm-generic/global_data.h
+++ b/include/asm-generic/global_data.h
@@ -447,12 +447,6 @@ struct global_data {
 	 */
 	fdt_addr_t translation_offset;
 #endif
-#if CONFIG_IS_ENABLED(WDT)
-	/**
-	 * @watchdog_dev: watchdog device
-	 */
-	struct udevice *watchdog_dev;
-#endif
 #ifdef CONFIG_GENERATE_ACPI_TABLE
 	/**
 	 * @acpi_ctx: ACPI context pointer
-- 
2.31.1

Re: [PATCH v6 09/12] watchdog: wdt-uclass.c: handle all DM watchdogs in watchdog_reset()

[Wolfgang Denk]

Dear Rasmus,

In message <20210819095706.3585923-10-rasmus.villemoes@prevas.dk> you wrote:
>
> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
> index 5b1c0df5d6..7570710c4d 100644
> --- a/drivers/watchdog/wdt-uclass.c
> +++ b/drivers/watchdog/wdt-uclass.c
> @@ -61,20 +61,24 @@ static void init_watchdog_dev(struct udevice *dev)
...
> +	ret = uclass_get(UCLASS_WDT, &uc);
> +	if (ret) {
> +		log_debug("Error getting UCLASS_WDT: %d\n", ret);
> +		return 0;
> +	}
> +
> +	uclass_foreach_dev(dev, uc) {
> +		ret = device_probe(dev);
> +		if (ret) {
> +			log_debug("Error probing %s: %d\n", dev->name, ret);
> +			continue;
>  		}

As discussed - errors need to be shown to the user, and not only in
images with debugging enabled.

> @@ -182,22 +186,34 @@ void watchdog_reset(void)
>  {
>  	struct wdt_priv *priv;
>  	struct udevice *dev;
> +	struct uclass *uc;
>  	ulong now;
>  
>  	/* Exit if GD is not ready or watchdog is not initialized yet */
>  	if (!gd || !(gd->flags & GD_FLG_WDT_READY))
>  		return;
>  
> -	dev = gd->watchdog_dev;
> -	priv = dev_get_uclass_priv(dev);
> -	if (!priv->running)
> +	if (uclass_get(UCLASS_WDT, &uc))
>  		return;


Do I see this crrectly that you remove here the code which you just
added in patch 02 of this series?

Why not doing it right from the beginning?

> +	uclass_foreach_dev(dev, uc) {
> +		if (!device_active(dev))
> +			continue;
> +		priv = dev_get_uclass_priv(dev);
> +		if (!priv->running)
> +			continue;

Potential NULL pointer dereference.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
An Ada exception is when a routine gets in trouble and says
'Beam me up, Scotty'.

[PATCH v6 10/12] watchdog: add gpio watchdog driver

[Rasmus Villemoes]

A rather common kind of external watchdog circuit is one that is kept
alive by toggling a gpio. Add a driver for handling such a watchdog.

The corresponding linux driver apparently has support for some
watchdog circuits which can be disabled by tri-stating the gpio, but I
have never actually encountered such a chip in the wild; the whole
point of adding an external watchdog is usually that it is not in any
way under software control. For forward-compatibility, and to make DT
describe the hardware, the current driver only supports devices that
have the always-running property. I went a little back and forth on
whether I should fail ->probe or only ->start, and ended up deciding
->start was the right place.

The compatible string is probably a little odd as it has nothing to do
with linux per se - however, I chose that to make .dts snippets
reusable between device trees used with U-Boot and linux, and this is
the (only) compatible string that linux' corresponding driver and DT
binding accepts. I have asked whether one should/could add "wdt-gpio"
to that binding, but the answer was no:

  https://lore.kernel.org/lkml/CAL_JsqKEGaFpiFV_oAtE+S_bnHkg4qry+bhx2EDs=NSbVf_giA@mail.gmail.com/

If someone feels strongly about this, I can certainly remove the
"linux," part from the string - it probably wouldn't the only place where
one can't reuse a DT snippet as-is between linux and U-Boot.

Reviewed-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Stefan Roese <sr@denx.de>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 .../watchdog/gpio-wdt.txt                     | 19 ++++++
 drivers/watchdog/Kconfig                      |  9 +++
 drivers/watchdog/Makefile                     |  1 +
 drivers/watchdog/gpio_wdt.c                   | 68 +++++++++++++++++++
 4 files changed, 97 insertions(+)
 create mode 100644 doc/device-tree-bindings/watchdog/gpio-wdt.txt
 create mode 100644 drivers/watchdog/gpio_wdt.c

diff --git a/doc/device-tree-bindings/watchdog/gpio-wdt.txt b/doc/device-tree-bindings/watchdog/gpio-wdt.txt
new file mode 100644
index 0000000000..c9a8559a3e
--- /dev/null
+++ b/doc/device-tree-bindings/watchdog/gpio-wdt.txt
@@ -0,0 +1,19 @@
+GPIO watchdog timer
+
+Describes a simple watchdog timer which is reset by toggling a gpio.
+
+Required properties:
+
+- compatible: Must be "linux,wdt-gpio".
+- gpios: gpio to toggle when wdt driver reset method is called.
+- always-running: Boolean property indicating that the watchdog cannot
+  be disabled. At present, U-Boot only supports this kind of GPIO
+  watchdog.
+
+Example:
+
+	gpio-wdt {
+		gpios = <&gpio0 1 0>;
+		compatible = "linux,wdt-gpio";
+		always-running;
+	};
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index f0ff2612a6..6fbb5c1b6d 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -147,6 +147,15 @@ config WDT_CORTINA
 	  This driver support all CPU ISAs supported by Cortina
 	  Access CAxxxx SoCs.
 
+config WDT_GPIO
+	bool "External gpio watchdog support"
+	depends on WDT
+	depends on DM_GPIO
+	help
+	  Support for external watchdog fed by toggling a gpio. See
+	  doc/device-tree-bindings/watchdog/gpio-wdt.txt for
+	  information on how to describe the watchdog in device tree.
+
 config WDT_MPC8xx
 	bool "MPC8xx watchdog timer support"
 	depends on WDT && MPC8xx
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 5c7ef593fe..f14415bb8e 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -25,6 +25,7 @@ obj-$(CONFIG_WDT_BOOKE) += booke_wdt.o
 obj-$(CONFIG_WDT_CORTINA) += cortina_wdt.o
 obj-$(CONFIG_WDT_ORION) += orion_wdt.o
 obj-$(CONFIG_WDT_CDNS) += cdns_wdt.o
+obj-$(CONFIG_WDT_GPIO) += gpio_wdt.o
 obj-$(CONFIG_WDT_MPC8xx) += mpc8xx_wdt.o
 obj-$(CONFIG_WDT_MT7620) += mt7620_wdt.o
 obj-$(CONFIG_WDT_MT7621) += mt7621_wdt.o
diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
new file mode 100644
index 0000000000..982a66b3f9
--- /dev/null
+++ b/drivers/watchdog/gpio_wdt.c
@@ -0,0 +1,68 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <dm.h>
+#include <dm/device_compat.h>
+#include <wdt.h>
+#include <asm/gpio.h>
+
+struct gpio_wdt_priv {
+	struct gpio_desc gpio;
+	bool always_running;
+	int state;
+};
+
+static int gpio_wdt_reset(struct udevice *dev)
+{
+	struct gpio_wdt_priv *priv = dev_get_priv(dev);
+
+	priv->state = !priv->state;
+
+	return dm_gpio_set_value(&priv->gpio, priv->state);
+}
+
+static int gpio_wdt_start(struct udevice *dev, u64 timeout, ulong flags)
+{
+	struct gpio_wdt_priv *priv = dev_get_priv(dev);
+
+	if (priv->always_running)
+		return 0;
+
+	return -ENOSYS;
+}
+
+static int dm_probe(struct udevice *dev)
+{
+	struct gpio_wdt_priv *priv = dev_get_priv(dev);
+	int ret;
+
+	priv->always_running = dev_read_bool(dev, "always-running");
+	ret = gpio_request_by_name(dev, "gpios", 0, &priv->gpio, GPIOD_IS_OUT);
+	if (ret < 0) {
+		dev_err(dev, "Request for wdt gpio failed: %d\n", ret);
+		return ret;
+	}
+
+	if (priv->always_running)
+		ret = gpio_wdt_reset(dev);
+
+	return ret;
+}
+
+static const struct wdt_ops gpio_wdt_ops = {
+	.start = gpio_wdt_start,
+	.reset = gpio_wdt_reset,
+};
+
+static const struct udevice_id gpio_wdt_ids[] = {
+	{ .compatible = "linux,wdt-gpio" },
+	{}
+};
+
+U_BOOT_DRIVER(wdt_gpio) = {
+	.name = "wdt_gpio",
+	.id = UCLASS_WDT,
+	.of_match = gpio_wdt_ids,
+	.ops = &gpio_wdt_ops,
+	.probe	= dm_probe,
+	.priv_auto = sizeof(struct gpio_wdt_priv),
+};
-- 
2.31.1

Re: [PATCH v6 10/12] watchdog: add gpio watchdog driver

[Wolfgang Denk]

Dear Rasmus,

again: error handling.

In message <20210819095706.3585923-11-rasmus.villemoes@prevas.dk> you wrote:
>
> diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
> new file mode 100644
> index 0000000000..982a66b3f9
> --- /dev/null
> +++ b/drivers/watchdog/gpio_wdt.c
> @@ -0,0 +1,68 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include <dm.h>
> +#include <dm/device_compat.h>
> +#include <wdt.h>
> +#include <asm/gpio.h>
> +
> +struct gpio_wdt_priv {
> +	struct gpio_desc gpio;
> +	bool always_running;
> +	int state;
> +};
> +
> +static int gpio_wdt_reset(struct udevice *dev)
> +{
> +	struct gpio_wdt_priv *priv = dev_get_priv(dev);
> +
> +	priv->state = !priv->state;

Potential NULL pointer dereference.

> +static int gpio_wdt_start(struct udevice *dev, u64 timeout, ulong flags)
> +{
> +	struct gpio_wdt_priv *priv = dev_get_priv(dev);
> +
> +	if (priv->always_running)
> +		return 0;

Ditto.

> +static int dm_probe(struct udevice *dev)
> +{
> +	struct gpio_wdt_priv *priv = dev_get_priv(dev);
> +	int ret;
> +
> +	priv->always_running = dev_read_bool(dev, "always-running");

Ditto.

> +	ret = gpio_request_by_name(dev, "gpios", 0, &priv->gpio, GPIOD_IS_OUT);
> +	if (ret < 0) {
> +		dev_err(dev, "Request for wdt gpio failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (priv->always_running)
> +		ret = gpio_wdt_reset(dev);

Ditto.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Even if you aren't in doubt, consider the mental welfare of the  per-
son who has to maintain the code after you, and who will probably put
parens in the wrong place.          - Larry Wall in the perl man page

Re: [PATCH v6 10/12] watchdog: add gpio watchdog driver

[Rasmus Villemoes]

On 19/08/2021 13.46, Wolfgang Denk wrote:
> Dear Rasmus,
> 
> again: error handling.
> 
> In message <20210819095706.3585923-11-rasmus.villemoes@prevas.dk> you wrote:
>>
>> diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
>> new file mode 100644
>> index 0000000000..982a66b3f9
>> --- /dev/null
>> +++ b/drivers/watchdog/gpio_wdt.c
>> @@ -0,0 +1,68 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +
>> +#include <dm.h>
>> +#include <dm/device_compat.h>
>> +#include <wdt.h>
>> +#include <asm/gpio.h>
>> +
>> +struct gpio_wdt_priv {
>> +	struct gpio_desc gpio;
>> +	bool always_running;
>> +	int state;
>> +};
>> +
>> +static int gpio_wdt_reset(struct udevice *dev)
>> +{
>> +	struct gpio_wdt_priv *priv = dev_get_priv(dev);
>> +
>> +	priv->state = !priv->state;
> 
> Potential NULL pointer dereference.

No, no and no. If allocation of the (driver or uclass) private data
fails, the device probe would have failed, so this code can never get
called with such a struct udevice.

Perhaps try doing a

git grep -10 -E 'dev_get(_uclass)?_priv'

and see how many cases you can find where that is followed by a NULL check?

Rasmus

Re: [PATCH v6 10/12] watchdog: add gpio watchdog driver

[Wolfgang Denk]

Dear Rasmus,

In message <62540f7b-0e07-8759-8e12-125527c2edec@prevas.dk> you wrote:
>
> >> +static int gpio_wdt_reset(struct udevice *dev)
> >> +{
> >> +	struct gpio_wdt_priv *priv = dev_get_priv(dev);
> >> +
> >> +	priv->state = !priv->state;
> > 
> > Potential NULL pointer dereference.
>
> No, no and no. If allocation of the (driver or uclass) private data
> fails, the device probe would have failed, so this code can never get
> called with such a struct udevice.

Famous last words...

> Perhaps try doing a
>
> git grep -10 -E 'dev_get(_uclass)?_priv'
>
> and see how many cases you can find where that is followed by a NULL check?

The existence of bad code is not a justification to add more of it.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
No, I'm not going to explain it. If you  can't  figure  it  out,  you
didn't want to know anyway... :-)
                   - Larry Wall in <1991Aug7.180856.2854@netlabs.com>

Re: [PATCH v6 10/12] watchdog: add gpio watchdog driver

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1047 bytes --]

On Thu, Aug 19, 2021 at 02:32:01PM +0200, Wolfgang Denk wrote:
> Dear Rasmus,
> 
> In message <62540f7b-0e07-8759-8e12-125527c2edec@prevas.dk> you wrote:
> >
> > >> +static int gpio_wdt_reset(struct udevice *dev)
> > >> +{
> > >> +	struct gpio_wdt_priv *priv = dev_get_priv(dev);
> > >> +
> > >> +	priv->state = !priv->state;
> > > 
> > > Potential NULL pointer dereference.
> >
> > No, no and no. If allocation of the (driver or uclass) private data
> > fails, the device probe would have failed, so this code can never get
> > called with such a struct udevice.
> 
> Famous last words...
> 
> > Perhaps try doing a
> >
> > git grep -10 -E 'dev_get(_uclass)?_priv'
> >
> > and see how many cases you can find where that is followed by a NULL check?
> 
> The existence of bad code is not a justification to add more of it.

Since I literally just sent this in another email you couldn't have seen
yet, I'll repeat it here.  Feel free to follow up to this with a series
to further update things, Wolfgang.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v6 10/12] watchdog: add gpio watchdog driver

[Wolfgang Denk]

Dear Tom,

In message <20210819123540.GV858@bill-the-cat> you wrote:
> 
> Since I literally just sent this in another email you couldn't have seen
> yet, I'll repeat it here.  Feel free to follow up to this with a series
> to further update things, Wolfgang.

So we have now a policy to wave through code, and ask others to
clean it up later?  That's ... sad.

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
"Consistency requires you to be as ignorant today as you were a  year
ago."                                              - Bernard Berenson

Re: [PATCH v6 10/12] watchdog: add gpio watchdog driver

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 798 bytes --]

On Thu, Aug 19, 2021 at 03:03:46PM +0200, Wolfgang Denk wrote:
> Dear Tom,
> 
> In message <20210819123540.GV858@bill-the-cat> you wrote:
> > 
> > Since I literally just sent this in another email you couldn't have seen
> > yet, I'll repeat it here.  Feel free to follow up to this with a series
> > to further update things, Wolfgang.
> 
> So we have now a policy to wave through code, and ask others to
> clean it up later?  That's ... sad.

No, we continue to have the policy of expecting reviewers to follow the
whole discussion and relevant subsystems.  Changing _every_ caller of
dev_get_priv to check for NULL and then, what? is clearly not the right
answer.  If you think you see a problem here please go audit the DM code
itself more and propose some changes.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v6 10/12] watchdog: add gpio watchdog driver

[Wolfgang Denk]

Dear Tom,

In message <20210819130806.GW858@bill-the-cat> you wrote:
> 
> > So we have now a policy to wave through code, and ask others to
> > clean it up later?  That's ... sad.
>
> No, we continue to have the policy of expecting reviewers to follow the
> whole discussion and relevant subsystems.

Once upon a time there has also been a policy that if a function
might return error codes, these need to be checked and handled.

> Changing _every_ caller of dev_get_priv to check for NULL and
> then, what? is clearly not the right answer.

Then what is the right answer in your opinion?

I mean, look at the implementation of dev_get_priv():

 628 void *dev_get_priv(const struct udevice *dev)
 629 {
 630         if (!dev) {
 631                 dm_warn("%s: null device\n", __func__);
 632                 return NULL;
 633         }
 634
 635         return dm_priv_to_rw(dev->priv_);
 636 }

If there is guaranteed no way that dev_get_priv() can return a NULL
pointer, that means that it must be guaranteed that the "dev"
argument can never be a NULL pointer, either.  So why do we check it
at all?

The same applies for all functions in "drivers/core/device.c" - they
all check for valid input parameters, like any code should do.

> If you think you see a problem here please go audit the DM code
> itself more and propose some changes.

I can see that the DM code itself implements proper error checking
and reporting; it's the callers where negligent code prevails.


If you are consequent, you must decide what you want:

- Either we want robust and reliable code - then we have to handle
  the error codes which functions like dev_get_priv() etc. return.

- Or you don't care about software quality, then we can omit such
  handling, but then it would also be consequent to remove all the
  error checking from "drivers/core/device.c" etc. - hey, that would
  even save a few hundred bytes of code size.


Sugarcoating code which fails to handle error codes because "these
errors can never happen" does not seem to be a clever approach to
software engineering to me.


I stop here.  You know my opinion.  You are the maintainer.


Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
A witty saying proves nothing, but saying  something  pointless  gets
people's attention.

Re: [PATCH v6 10/12] watchdog: add gpio watchdog driver

[Simon Glass]

Hi,

On Thu, 19 Aug 2021 at 08:16, Wolfgang Denk <wd@denx.de> wrote:
>
> Dear Tom,
>
> In message <20210819130806.GW858@bill-the-cat> you wrote:
> >
> > > So we have now a policy to wave through code, and ask others to
> > > clean it up later?  That's ... sad.
> >
> > No, we continue to have the policy of expecting reviewers to follow the
> > whole discussion and relevant subsystems.
>
> Once upon a time there has also been a policy that if a function
> might return error codes, these need to be checked and handled.
>
> > Changing _every_ caller of dev_get_priv to check for NULL and
> > then, what? is clearly not the right answer.

Note that we should not check these calls, as the priv data is
allocated by driver model and a device cannot be probed until it gets
past this step.

I know that sometimes people add this check, but whenever I see it I
ask them to remove it. It is pointless and confusing, since it
suggests that driver model may not have allocated it yet. This sort of
confusion can really make things hard to understand for new people.

>
> Then what is the right answer in your opinion?

If a device is probed, you can rely on the priv data being set up. The
only way to access a probed device is via the device-internal.h or
uclass-internal.h APIs. Be careful with those if you must use them.

>
> I mean, look at the implementation of dev_get_priv():
>
>  628 void *dev_get_priv(const struct udevice *dev)
>  629 {
>  630         if (!dev) {
>  631                 dm_warn("%s: null device\n", __func__);
>  632                 return NULL;
>  633         }
>  634
>  635         return dm_priv_to_rw(dev->priv_);
>  636 }
>
> If there is guaranteed no way that dev_get_priv() can return a NULL
> pointer, that means that it must be guaranteed that the "dev"
> argument can never be a NULL pointer, either.  So why do we check it
> at all?
>
> The same applies for all functions in "drivers/core/device.c" - they
> all check for valid input parameters, like any code should do.

I think did a series on this many years ago - a way to turn on checks
for this and that the right struct is obtained when you call
dev_get_priv(), etc. We could certainly add this with a CONFIG option
for debugging purposes. The runtime cost is actually not terrible but
it does require collusion with malloc() to be efficient.

>
> > If you think you see a problem here please go audit the DM code
> > itself more and propose some changes.
>
> I can see that the DM code itself implements proper error checking
> and reporting; it's the callers where negligent code prevails.
>
>
> If you are consequent, you must decide what you want:
>
> - Either we want robust and reliable code - then we have to handle
>   the error codes which functions like dev_get_priv() etc. return.
>
> - Or you don't care about software quality, then we can omit such
>   handling, but then it would also be consequent to remove all the
>   error checking from "drivers/core/device.c" etc. - hey, that would
>   even save a few hundred bytes of code size.
>
>
> Sugarcoating code which fails to handle error codes because "these
> errors can never happen" does not seem to be a clever approach to
> software engineering to me.
>
>
> I stop here.  You know my opinion.  You are the maintainer.

There is a wider issue here about arrg checking. We sometimes use
assert but at present that only appears in the code if DEBUG is
enabled. Also it just halts.

OTOH if we add arg checking everywhere it cluttles the code and can
substantially increase the size (I know of a project where it doubled
the size). I like to distinguish between:

- programming errors
- security errors where user input is insufficiently checked

IMO the former should not be present if you have sufficient tests and
trying to catch them in the field at runtime is not very kind to your
users.

Regards,
Simon

Re: [PATCH v6 10/12] watchdog: add gpio watchdog driver

[Wolfgang Denk]

Dear Simon,

In message <CAPnjgZ0aVwo2mq9BNw-9xz+xPZVm6Negf9sQxnTAr9GHTTxPxQ@mail.gmail.com> you wrote:
>
> - programming errors
> - security errors where user input is insufficiently checked
>
> IMO the former should not be present if you have sufficient tests and
> trying to catch them in the field at runtime is not very kind to your
> users.

Wow.

I think I'll add this to my signature database:

| "Trying to catch [programming errors] in the field at runtime is not
| very kind to your users."
| 
| - Simon Glass in <CAPnjgZ0aVwo2mq9BNw-9xz+xPZVm6Negf9sQxnTAr9GHTTxPxQ@mail.gmail.com>

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
It is wrong always, everywhere and for everyone to  believe  anything
upon  insufficient  evidence.  - W. K. Clifford, British philosopher,
circa 1876

Re: [PATCH v6 10/12] watchdog: add gpio watchdog driver

[Simon Glass]

Hi Wolfgang,

On Thu, 19 Aug 2021 at 08:58, Wolfgang Denk <wd@denx.de> wrote:
>
> Dear Simon,
>
> In message <CAPnjgZ0aVwo2mq9BNw-9xz+xPZVm6Negf9sQxnTAr9GHTTxPxQ@mail.gmail.com> you wrote:
> >
> > - programming errors
> > - security errors where user input is insufficiently checked
> >
> > IMO the former should not be present if you have sufficient tests and
> > trying to catch them in the field at runtime is not very kind to your
> > users.
>
> Wow.
>
> I think I'll add this to my signature database:
>
> | "Trying to catch [programming errors] in the field at runtime is not
> | very kind to your users."
> |
> | - Simon Glass in <CAPnjgZ0aVwo2mq9BNw-9xz+xPZVm6Negf9sQxnTAr9GHTTxPxQ@mail.gmail.com>

Well you've taken it out of context :-) It would make more sense if
you mention the need for tests

Regards,
Simon

Re: [PATCH v6 10/12] watchdog: add gpio watchdog driver

[Wolfgang Denk]

Dear Simon,

In message <CAPnjgZ3-k4sGn0G-7tiGixsBxT77s9bDNXeTf-86Fj18oWSaaQ@mail.gmail.com> you wrote:
> >
> > Wow.
> >
> > I think I'll add this to my signature database:
> >
> > | "Trying to catch [programming errors] in the field at runtime is not
> > | very kind to your users."
> > |
> > | - Simon Glass in <CAPnjgZ0aVwo2mq9BNw-9xz+xPZVm6Negf9sQxnTAr9GHTTxPxQ@mail.gmail.com>
>
> Well you've taken it out of context :-) It would make more sense if
> you mention the need for tests

Testing can show the presense of bugs, but not their absence.
                                                   -- Edsger Dijkstr

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
The moral of the story is: "Don't stop to  tighten  your  shoe  laces
during the Olympics 100m finals".
                             - Kevin Jones in <DEJo68.K1t@bri.hp.com>

Re: [PATCH v6 10/12] watchdog: add gpio watchdog driver

[Simon Glass]

Hi Wolfgang,

On Mon, 23 Aug 2021 at 05:07, Wolfgang Denk <wd@denx.de> wrote:
>
> Dear Simon,
>
> In message <CAPnjgZ3-k4sGn0G-7tiGixsBxT77s9bDNXeTf-86Fj18oWSaaQ@mail.gmail.com> you wrote:
> > >
> > > Wow.
> > >
> > > I think I'll add this to my signature database:
> > >
> > > | "Trying to catch [programming errors] in the field at runtime is not
> > > | very kind to your users."
> > > |
> > > | - Simon Glass in <CAPnjgZ0aVwo2mq9BNw-9xz+xPZVm6Negf9sQxnTAr9GHTTxPxQ@mail.gmail.com>
> >
> > Well you've taken it out of context :-) It would make more sense if
> > you mention the need for tests
>
> Testing can show the presense of bugs, but not their absence.
>                                                    -- Edsger Dijkstr

Indeed, although that is really just a truism. If this were a
philosophy channel and I had actually studied it properly, I would go
on about evidence of absence at this point.

From a s/w POV, there is always one more bug.

Regards,
SImon

Re: [PATCH v6 10/12] watchdog: add gpio watchdog driver

[Rasmus Villemoes]

On 19/08/2021 14.32, Wolfgang Denk wrote:

> The existence of bad code is not a justification to add more of it.

Obviously true and I agree.

However, it is at the same time completely irrelevant in this context,
because the pattern of using the return value of dev_get_priv() without
a NULL check is neither bad or wrong, as has now been explained to you
several times.

If you really think checking the return value of dev_get_priv() must be
done religiously, perhaps you could tap Stefan (737c3de09984), Marek
(7e1f1e16fe75), or Heiko (6e31c62a175c) on the shoulder and tell them to
stop cranking out "bad" code.

On 19/08/2021 16.16, Wolfgang Denk wrote:

> I mean, look at the implementation of dev_get_priv():
>
>  628 void *dev_get_priv(const struct udevice *dev)
>  629 {
>  630         if (!dev) {
>  631                 dm_warn("%s: null device\n", __func__);
>  632                 return NULL;
>  633         }
>  634
>  635         return dm_priv_to_rw(dev->priv_);
>  636 }
>
> If there is guaranteed no way that dev_get_priv() can return a NULL
> pointer, that means that it must be guaranteed that the "dev"
> argument can never be a NULL pointer, either.

There's another logical fallacy right here. Sure, you've found an input
value for which dev_get_priv() would return NULL. But any caller who
knows they're not passing a NULL dev also know they won't follow that
code path.

A driver which doesn't populate the priv field by via a non-zero
.priv_auto field may need to check the return value of dev_get_priv().
I'm not claiming that checking that is always redundant. However,
neither is it anywhere near true that checking is always required.

Rasmus

Re: [PATCH v6 10/12] watchdog: add gpio watchdog driver

[Simon Glass]

Hi Rasmus,

On Fri, 20 Aug 2021 at 00:22, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> On 19/08/2021 14.32, Wolfgang Denk wrote:
>
> > The existence of bad code is not a justification to add more of it.
>
> Obviously true and I agree.
>
> However, it is at the same time completely irrelevant in this context,
> because the pattern of using the return value of dev_get_priv() without
> a NULL check is neither bad or wrong, as has now been explained to you
> several times.
>
> If you really think checking the return value of dev_get_priv() must be
> done religiously, perhaps you could tap Stefan (737c3de09984), Marek
> (7e1f1e16fe75), or Heiko (6e31c62a175c) on the shoulder and tell them to
> stop cranking out "bad" code.
>
> On 19/08/2021 16.16, Wolfgang Denk wrote:
>
> > I mean, look at the implementation of dev_get_priv():
> >
> >  628 void *dev_get_priv(const struct udevice *dev)
> >  629 {
> >  630         if (!dev) {
> >  631                 dm_warn("%s: null device\n", __func__);
> >  632                 return NULL;
> >  633         }
> >  634
> >  635         return dm_priv_to_rw(dev->priv_);
> >  636 }
> >
> > If there is guaranteed no way that dev_get_priv() can return a NULL
> > pointer, that means that it must be guaranteed that the "dev"
> > argument can never be a NULL pointer, either.
>
> There's another logical fallacy right here. Sure, you've found an input
> value for which dev_get_priv() would return NULL. But any caller who
> knows they're not passing a NULL dev also know they won't follow that
> code path.
>
> A driver which doesn't populate the priv field by via a non-zero
> .priv_auto field may need to check the return value of dev_get_priv().
> I'm not claiming that checking that is always redundant. However,
> neither is it anywhere near true that checking is always required.

Just on this point, drivers should use the priv pointer without
priv_auto. If we do introduce run-time checks, it would fail with
those.

Regards,
Simon

[PATCH v6 11/12] sandbox: add test of wdt_gpio driver

[Rasmus Villemoes]

It seems that no other test has claimed gpio_a:7 yet, so use that.

The only small wrinkle is modifying the existing wdt test to use
uclass_get_device_by_driver() since we now have two UCLASS_WDT
instances in play, so it's a little more robust to fetch the device by
driver and not merely uclass+index.

Reviewed-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Stefan Roese <sr@denx.de>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 arch/sandbox/dts/test.dts   |  6 ++++++
 configs/sandbox64_defconfig |  1 +
 configs/sandbox_defconfig   |  1 +
 test/dm/wdt.c               | 36 +++++++++++++++++++++++++++++++++++-
 4 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index d5976318d1..fe5ac6ecd9 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -779,6 +779,12 @@
 		};
 	};
 
+	gpio-wdt {
+		gpios = <&gpio_a 7 0>;
+		compatible = "linux,wdt-gpio";
+		always-running;
+	};
+
 	mbox: mbox {
 		compatible = "sandbox,mbox";
 		#mbox-cells = <1>;
diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
index 3627a066b4..6cad90b03e 100644
--- a/configs/sandbox64_defconfig
+++ b/configs/sandbox64_defconfig
@@ -225,6 +225,7 @@ CONFIG_SPLASH_SCREEN_ALIGN=y
 CONFIG_VIDEO_BMP_RLE8=y
 # CONFIG_WATCHDOG_AUTOSTART is not set
 CONFIG_WDT=y
+CONFIG_WDT_GPIO=y
 CONFIG_WDT_SANDBOX=y
 CONFIG_FS_CBFS=y
 CONFIG_FS_CRAMFS=y
diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index 34b749b47b..4a8b4f220d 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -284,6 +284,7 @@ CONFIG_W1_EEPROM=y
 CONFIG_W1_EEPROM_SANDBOX=y
 # CONFIG_WATCHDOG_AUTOSTART is not set
 CONFIG_WDT=y
+CONFIG_WDT_GPIO=y
 CONFIG_WDT_SANDBOX=y
 CONFIG_FS_CBFS=y
 CONFIG_FS_CRAMFS=y
diff --git a/test/dm/wdt.c b/test/dm/wdt.c
index 24b991dff6..abff853a02 100644
--- a/test/dm/wdt.c
+++ b/test/dm/wdt.c
@@ -6,6 +6,7 @@
 #include <common.h>
 #include <dm.h>
 #include <wdt.h>
+#include <asm/gpio.h>
 #include <asm/state.h>
 #include <asm/test.h>
 #include <dm/test.h>
@@ -19,7 +20,8 @@ static int dm_test_wdt_base(struct unit_test_state *uts)
 	struct udevice *dev;
 	const u64 timeout = 42;
 
-	ut_assertok(uclass_get_device(UCLASS_WDT, 0, &dev));
+	ut_assertok(uclass_get_device_by_driver(UCLASS_WDT,
+						DM_DRIVER_GET(wdt_sandbox), &dev));
 	ut_assertnonnull(dev);
 	ut_asserteq(0, state->wdt.counter);
 	ut_asserteq(false, state->wdt.running);
@@ -39,3 +41,35 @@ static int dm_test_wdt_base(struct unit_test_state *uts)
 	return 0;
 }
 DM_TEST(dm_test_wdt_base, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
+
+static int dm_test_wdt_gpio(struct unit_test_state *uts)
+{
+	/*
+	 * The sandbox wdt gpio is "connected" to gpio bank a, offset
+	 * 7. Use the sandbox back door to verify that the gpio-wdt
+	 * driver behaves as expected.
+	 */
+	struct udevice *wdt, *gpio;
+	const u64 timeout = 42;
+	const int offset = 7;
+	int val;
+
+	ut_assertok(uclass_get_device_by_driver(UCLASS_WDT,
+						DM_DRIVER_GET(wdt_gpio), &wdt));
+	ut_assertnonnull(wdt);
+
+	ut_assertok(uclass_get_device_by_name(UCLASS_GPIO, "base-gpios", &gpio));
+	ut_assertnonnull(gpio);
+	ut_assertok(wdt_start(wdt, timeout, 0));
+
+	val = sandbox_gpio_get_value(gpio, offset);
+	ut_assertok(wdt_reset(wdt));
+	ut_asserteq(!val, sandbox_gpio_get_value(gpio, offset));
+	ut_assertok(wdt_reset(wdt));
+	ut_asserteq(val, sandbox_gpio_get_value(gpio, offset));
+
+	ut_asserteq(-ENOSYS, wdt_stop(wdt));
+
+	return 0;
+}
+DM_TEST(dm_test_wdt_gpio, UT_TESTF_SCAN_FDT);
-- 
2.31.1

[PATCH v6 12/12] sandbox: add test of wdt-uclass' watchdog_reset()

[Rasmus Villemoes]

Check that the watchdog_reset() implementation in wdt-uclass behaves
as expected:

- resets all activated watchdog devices
- leaves unactivated/stopped devices alone
- that the rate-limiting works, with a per-device threshold

Reviewed-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Stefan Roese <sr@denx.de>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 arch/sandbox/dts/test.dts |  2 ++
 test/dm/wdt.c             | 54 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index fe5ac6ecd9..bafc2e9494 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -782,6 +782,7 @@
 	gpio-wdt {
 		gpios = <&gpio_a 7 0>;
 		compatible = "linux,wdt-gpio";
+		hw_margin_ms = <100>;
 		always-running;
 	};
 
@@ -1264,6 +1265,7 @@
 
 	wdt0: wdt@0 {
 		compatible = "sandbox,wdt";
+		hw_margin_ms = <200>;
 	};
 
 	axi: axi@0 {
diff --git a/test/dm/wdt.c b/test/dm/wdt.c
index abff853a02..ee615f0e14 100644
--- a/test/dm/wdt.c
+++ b/test/dm/wdt.c
@@ -12,6 +12,8 @@
 #include <dm/test.h>
 #include <test/test.h>
 #include <test/ut.h>
+#include <linux/delay.h>
+#include <watchdog.h>
 
 /* Test that watchdog driver functions are called */
 static int dm_test_wdt_base(struct unit_test_state *uts)
@@ -73,3 +75,55 @@ static int dm_test_wdt_gpio(struct unit_test_state *uts)
 	return 0;
 }
 DM_TEST(dm_test_wdt_gpio, UT_TESTF_SCAN_FDT);
+
+static int dm_test_wdt_watchdog_reset(struct unit_test_state *uts)
+{
+	struct sandbox_state *state = state_get_current();
+	struct udevice *gpio_wdt, *sandbox_wdt;
+	struct udevice *gpio;
+	const u64 timeout = 42;
+	const int offset = 7;
+	uint reset_count;
+	int val;
+
+	ut_assertok(uclass_get_device_by_driver(UCLASS_WDT,
+						DM_DRIVER_GET(wdt_gpio), &gpio_wdt));
+	ut_assertnonnull(gpio_wdt);
+	ut_assertok(uclass_get_device_by_driver(UCLASS_WDT,
+						DM_DRIVER_GET(wdt_sandbox), &sandbox_wdt));
+	ut_assertnonnull(sandbox_wdt);
+	ut_assertok(uclass_get_device_by_name(UCLASS_GPIO, "base-gpios", &gpio));
+	ut_assertnonnull(gpio);
+
+	/* Neither device should be "started", so watchdog_reset() should be a no-op. */
+	reset_count = state->wdt.reset_count;
+	val = sandbox_gpio_get_value(gpio, offset);
+	watchdog_reset();
+	ut_asserteq(reset_count, state->wdt.reset_count);
+	ut_asserteq(val, sandbox_gpio_get_value(gpio, offset));
+
+	/* Start both devices. */
+	ut_assertok(wdt_start(gpio_wdt, timeout, 0));
+	ut_assertok(wdt_start(sandbox_wdt, timeout, 0));
+
+	/* Make sure both devices have just been pinged. */
+	timer_test_add_offset(100);
+	watchdog_reset();
+	reset_count = state->wdt.reset_count;
+	val = sandbox_gpio_get_value(gpio, offset);
+
+	/* The gpio watchdog should be pinged, the sandbox one not. */
+	timer_test_add_offset(30);
+	watchdog_reset();
+	ut_asserteq(reset_count, state->wdt.reset_count);
+	ut_asserteq(!val, sandbox_gpio_get_value(gpio, offset));
+
+	/* After another ~30ms, both devices should get pinged. */
+	timer_test_add_offset(30);
+	watchdog_reset();
+	ut_asserteq(reset_count + 1, state->wdt.reset_count);
+	ut_asserteq(val, sandbox_gpio_get_value(gpio, offset));
+
+	return 0;
+}
+DM_TEST(dm_test_wdt_watchdog_reset, UT_TESTF_SCAN_FDT);
-- 
2.31.1

Re: [PATCH v6 12/12] sandbox: add test of wdt-uclass' watchdog_reset()

[Stefan Roese]

Hi Rasmus,

I've pulled this patchset now into next [1] and have run it through
CI via Azure. Here an error occurs:

https://dev.azure.com/sr0718/u-boot/_build/results?buildId=109&view=logs&j=50449d1b-398e-53ae-48fa-6bf338edeb51&t=97605dd2-f5a5-5dd7-2118-315ffdc8bcd6&l=533

Could you please take a look at this?

Thanks,
Stefan

[1] https://source.denx.de/u-boot/custodians/u-boot-marvell/-/commits/next

On 19.08.21 11:57, Rasmus Villemoes wrote:
> Check that the watchdog_reset() implementation in wdt-uclass behaves
> as expected:
> 
> - resets all activated watchdog devices
> - leaves unactivated/stopped devices alone
> - that the rate-limiting works, with a per-device threshold
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Reviewed-by: Stefan Roese <sr@denx.de>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>   arch/sandbox/dts/test.dts |  2 ++
>   test/dm/wdt.c             | 54 +++++++++++++++++++++++++++++++++++++++
>   2 files changed, 56 insertions(+)
> 
> diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
> index fe5ac6ecd9..bafc2e9494 100644
> --- a/arch/sandbox/dts/test.dts
> +++ b/arch/sandbox/dts/test.dts
> @@ -782,6 +782,7 @@
>   	gpio-wdt {
>   		gpios = <&gpio_a 7 0>;
>   		compatible = "linux,wdt-gpio";
> +		hw_margin_ms = <100>;
>   		always-running;
>   	};
>   
> @@ -1264,6 +1265,7 @@
>   
>   	wdt0: wdt@0 {
>   		compatible = "sandbox,wdt";
> +		hw_margin_ms = <200>;
>   	};
>   
>   	axi: axi@0 {
> diff --git a/test/dm/wdt.c b/test/dm/wdt.c
> index abff853a02..ee615f0e14 100644
> --- a/test/dm/wdt.c
> +++ b/test/dm/wdt.c
> @@ -12,6 +12,8 @@
>   #include <dm/test.h>
>   #include <test/test.h>
>   #include <test/ut.h>
> +#include <linux/delay.h>
> +#include <watchdog.h>
>   
>   /* Test that watchdog driver functions are called */
>   static int dm_test_wdt_base(struct unit_test_state *uts)
> @@ -73,3 +75,55 @@ static int dm_test_wdt_gpio(struct unit_test_state *uts)
>   	return 0;
>   }
>   DM_TEST(dm_test_wdt_gpio, UT_TESTF_SCAN_FDT);
> +
> +static int dm_test_wdt_watchdog_reset(struct unit_test_state *uts)
> +{
> +	struct sandbox_state *state = state_get_current();
> +	struct udevice *gpio_wdt, *sandbox_wdt;
> +	struct udevice *gpio;
> +	const u64 timeout = 42;
> +	const int offset = 7;
> +	uint reset_count;
> +	int val;
> +
> +	ut_assertok(uclass_get_device_by_driver(UCLASS_WDT,
> +						DM_DRIVER_GET(wdt_gpio), &gpio_wdt));
> +	ut_assertnonnull(gpio_wdt);
> +	ut_assertok(uclass_get_device_by_driver(UCLASS_WDT,
> +						DM_DRIVER_GET(wdt_sandbox), &sandbox_wdt));
> +	ut_assertnonnull(sandbox_wdt);
> +	ut_assertok(uclass_get_device_by_name(UCLASS_GPIO, "base-gpios", &gpio));
> +	ut_assertnonnull(gpio);
> +
> +	/* Neither device should be "started", so watchdog_reset() should be a no-op. */
> +	reset_count = state->wdt.reset_count;
> +	val = sandbox_gpio_get_value(gpio, offset);
> +	watchdog_reset();
> +	ut_asserteq(reset_count, state->wdt.reset_count);
> +	ut_asserteq(val, sandbox_gpio_get_value(gpio, offset));
> +
> +	/* Start both devices. */
> +	ut_assertok(wdt_start(gpio_wdt, timeout, 0));
> +	ut_assertok(wdt_start(sandbox_wdt, timeout, 0));
> +
> +	/* Make sure both devices have just been pinged. */
> +	timer_test_add_offset(100);
> +	watchdog_reset();
> +	reset_count = state->wdt.reset_count;
> +	val = sandbox_gpio_get_value(gpio, offset);
> +
> +	/* The gpio watchdog should be pinged, the sandbox one not. */
> +	timer_test_add_offset(30);
> +	watchdog_reset();
> +	ut_asserteq(reset_count, state->wdt.reset_count);
> +	ut_asserteq(!val, sandbox_gpio_get_value(gpio, offset));
> +
> +	/* After another ~30ms, both devices should get pinged. */
> +	timer_test_add_offset(30);
> +	watchdog_reset();
> +	ut_asserteq(reset_count + 1, state->wdt.reset_count);
> +	ut_asserteq(val, sandbox_gpio_get_value(gpio, offset));
> +
> +	return 0;
> +}
> +DM_TEST(dm_test_wdt_watchdog_reset, UT_TESTF_SCAN_FDT);
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH v6 12/12] sandbox: add test of wdt-uclass' watchdog_reset()

[Rasmus Villemoes]

On 31/08/2021 10.17, Stefan Roese wrote:
> Hi Rasmus,
> 
> I've pulled this patchset now into next [1] and have run it through
> CI via Azure. Here an error occurs:
> 
> https://dev.azure.com/sr0718/u-boot/_build/results?buildId=109&view=logs&j=50449d1b-398e-53ae-48fa-6bf338edeb51&t=97605dd2-f5a5-5dd7-2118-315ffdc8bcd6&l=533
> 
> 
> Could you please take a look at this?

I'm pretty confident that has nothing to do with my patches, but is a
broken (or, to put it more gently, fragile) test.

It does

// fetch the emulated device's current base_time value, setting it to 0
        old_base_time = sandbox_i2c_rtc_get_set_base_time(emul, 0);

// fetch the emuluated device's current base_time value, don't (-1) set
// a new value, check that we got 0
        ut_asserteq(0, sandbox_i2c_rtc_get_set_base_time(emul, -1));

// call the device's ->reset method
        /* Resetting the RTC should put he base time back to normal */
        ut_assertok(dm_rtc_reset(dev));
// fetch the new base_time value, without altering it
        base_time = sandbox_i2c_rtc_get_set_base_time(emul, -1);
// and check that the base time was put back to "normal"
        ut_asserteq(old_base_time, base_time);

The thing is, the ->reset method does

static void reset_time(struct udevice *dev)
{
        struct sandbox_i2c_rtc_plat_data *plat = dev_get_plat(dev);
        struct rtc_time now;

        os_localtime(&now);
        plat->base_time = rtc_mktime(&now);

It's inevitable that this will cause occasional spurious failures. I can
trivially reproduce it with

=> while ut dm rtc_reset ; do echo . ; done

it fails after a few screenfuls of successes.

Rasmus

Re: [PATCH v6 12/12] sandbox: add test of wdt-uclass' watchdog_reset()

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2081 bytes --]

On Tue, Aug 31, 2021 at 11:29:51AM +0200, Rasmus Villemoes wrote:
> On 31/08/2021 10.17, Stefan Roese wrote:
> > Hi Rasmus,
> > 
> > I've pulled this patchset now into next [1] and have run it through
> > CI via Azure. Here an error occurs:
> > 
> > https://dev.azure.com/sr0718/u-boot/_build/results?buildId=109&view=logs&j=50449d1b-398e-53ae-48fa-6bf338edeb51&t=97605dd2-f5a5-5dd7-2118-315ffdc8bcd6&l=533
> > 
> > 
> > Could you please take a look at this?
> 
> I'm pretty confident that has nothing to do with my patches, but is a
> broken (or, to put it more gently, fragile) test.
> 
> It does
> 
> // fetch the emulated device's current base_time value, setting it to 0
>         old_base_time = sandbox_i2c_rtc_get_set_base_time(emul, 0);
> 
> // fetch the emuluated device's current base_time value, don't (-1) set
> // a new value, check that we got 0
>         ut_asserteq(0, sandbox_i2c_rtc_get_set_base_time(emul, -1));
> 
> // call the device's ->reset method
>         /* Resetting the RTC should put he base time back to normal */
>         ut_assertok(dm_rtc_reset(dev));
> // fetch the new base_time value, without altering it
>         base_time = sandbox_i2c_rtc_get_set_base_time(emul, -1);
> // and check that the base time was put back to "normal"
>         ut_asserteq(old_base_time, base_time);
> 
> The thing is, the ->reset method does
> 
> static void reset_time(struct udevice *dev)
> {
>         struct sandbox_i2c_rtc_plat_data *plat = dev_get_plat(dev);
>         struct rtc_time now;
> 
>         os_localtime(&now);
>         plat->base_time = rtc_mktime(&now);
> 
> It's inevitable that this will cause occasional spurious failures. I can
> trivially reproduce it with
> 
> => while ut dm rtc_reset ; do echo . ; done
> 
> it fails after a few screenfuls of successes.

Yes, this test fails sometimes and just needs to be re-run.  Currently
we even have code in the test framework to allow for a little bit of
wiggle room in the value, but perhaps it needs to be bumped up slightly.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v6 12/12] sandbox: add test of wdt-uclass' watchdog_reset()

[Stefan Roese]

On 31.08.21 11:29, Rasmus Villemoes wrote:
> On 31/08/2021 10.17, Stefan Roese wrote:
>> Hi Rasmus,
>>
>> I've pulled this patchset now into next [1] and have run it through
>> CI via Azure. Here an error occurs:
>>
>> https://dev.azure.com/sr0718/u-boot/_build/results?buildId=109&view=logs&j=50449d1b-398e-53ae-48fa-6bf338edeb51&t=97605dd2-f5a5-5dd7-2118-315ffdc8bcd6&l=533
>>
>>
>> Could you please take a look at this?
> 
> I'm pretty confident that has nothing to do with my patches, but is a
> broken (or, to put it more gently, fragile) test.
> 
> It does
> 
> // fetch the emulated device's current base_time value, setting it to 0
>          old_base_time = sandbox_i2c_rtc_get_set_base_time(emul, 0);
> 
> // fetch the emuluated device's current base_time value, don't (-1) set
> // a new value, check that we got 0
>          ut_asserteq(0, sandbox_i2c_rtc_get_set_base_time(emul, -1));
> 
> // call the device's ->reset method
>          /* Resetting the RTC should put he base time back to normal */
>          ut_assertok(dm_rtc_reset(dev));
> // fetch the new base_time value, without altering it
>          base_time = sandbox_i2c_rtc_get_set_base_time(emul, -1);
> // and check that the base time was put back to "normal"
>          ut_asserteq(old_base_time, base_time);
> 
> The thing is, the ->reset method does
> 
> static void reset_time(struct udevice *dev)
> {
>          struct sandbox_i2c_rtc_plat_data *plat = dev_get_plat(dev);
>          struct rtc_time now;
> 
>          os_localtime(&now);
>          plat->base_time = rtc_mktime(&now);
> 
> It's inevitable that this will cause occasional spurious failures. I can
> trivially reproduce it with
> 
> => while ut dm rtc_reset ; do echo . ; done
> 
> it fails after a few screenfuls of successes.

Thanks for looking into this.

Thanks,
Stefan