[PATCH 1/2] watchdog: Hide WATCHDOG_RESET_DISABLE

[PATCH 1/2] watchdog: Hide WATCHDOG_RESET_DISABLE

[Michael Walle]

This option is only supported by the IMX watchdog and seems to be
similar to CONFIG_WATCHDOG.

Move it below the IMX watchdog and make it dependent on IMX_WATCHDOG.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/watchdog/Kconfig | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 210d9f8093..4532a40e45 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -21,12 +21,6 @@ config WATCHDOG_TIMEOUT_MSECS
 config HW_WATCHDOG
 	bool
 
-config WATCHDOG_RESET_DISABLE
-	bool "Disable reset watchdog"
-	help
-	  Disable reset watchdog, which can let WATCHDOG_RESET invalid, so
-	  that the watchdog will not be fed in u-boot.
-
 config IMX_WATCHDOG
 	bool "Enable Watchdog Timer support for IMX and LSCH2 of NXP"
 	select HW_WATCHDOG if !WDT
@@ -34,6 +28,13 @@ config IMX_WATCHDOG
 	  Select this to enable the IMX and LSCH2 of Layerscape watchdog
 	  driver.
 
+config WATCHDOG_RESET_DISABLE
+	bool "Disable reset watchdog"
+	depends on IMX_WATCHDOG
+	help
+	  Disable reset watchdog, which can let WATCHDOG_RESET invalid, so
+	  that the watchdog will not be fed in u-boot.
+
 config OMAP_WATCHDOG
 	bool "TI OMAP watchdog driver"
 	depends on ARCH_OMAP2PLUS
-- 
2.20.1

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Michael Walle]

Let the user choose between three different behaviours of the watchdog:
 (1) Keep the watchdog disabled
 (2) Supervise u-boot
 (3) Supervise u-boot and the operating systen (default)

Option (2) will disable the watchdog right before handing control to the
operating system. This is useful when the OS is not aware of the
watchdog. Option (3) doesn't disable the watchdog and assumes the OS
will continue servicing.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 cmd/boot.c                    |  7 +++++++
 cmd/bootefi.c                 |  7 +++++++
 cmd/efidebug.c                |  7 +++++++
 common/board_r.c              |  2 +-
 common/bootm_os.c             |  5 +++++
 common/spl/spl.c              |  6 +++---
 drivers/watchdog/Kconfig      | 24 ++++++++++++++++++++++++
 drivers/watchdog/wdt-uclass.c | 33 +++++++++++++++++++++++++--------
 include/wdt.h                 | 17 +++++++++++++++++
 9 files changed, 96 insertions(+), 12 deletions(-)

diff --git a/cmd/boot.c b/cmd/boot.c
index 36aba22b30..2412410371 100644
--- a/cmd/boot.c
+++ b/cmd/boot.c
@@ -10,6 +10,7 @@
 #include <common.h>
 #include <command.h>
 #include <net.h>
+#include <wdt.h>
 
 #ifdef CONFIG_CMD_GO
 
@@ -33,6 +34,9 @@ static int do_go(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 
 	printf ("## Starting application at 0x%08lX ...\n", addr);
 
+	if (IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT))
+		stop_watchdog();
+
 	/*
 	 * pass address parameter as argv[0] (aka command name),
 	 * and all remaining args
@@ -40,6 +44,9 @@ static int do_go(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	rc = do_go_exec ((void *)addr, argc - 1, argv + 1);
 	if (rc != 0) rcode = 1;
 
+	if (IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT))
+		start_watchdog();
+
 	printf ("## Application terminated, rc = 0x%lX\n", rc);
 	return rcode;
 }
diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 40d5ef2b3a..21f6650174 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -24,6 +24,7 @@
 #include <memalign.h>
 #include <asm-generic/sections.h>
 #include <linux/linkage.h>
+#include <wdt.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -320,6 +321,9 @@ static efi_status_t do_bootefi_exec(efi_handle_t handle, void *load_options)
 	efi_uintn_t exit_data_size = 0;
 	u16 *exit_data = NULL;
 
+	if (IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT))
+		stop_watchdog();
+
 	/* Call our payload! */
 	ret = EFI_CALL(efi_start_image(handle, &exit_data_size, &exit_data));
 	if (ret != EFI_SUCCESS) {
@@ -333,6 +337,9 @@ static efi_status_t do_bootefi_exec(efi_handle_t handle, void *load_options)
 
 	efi_restore_gd();
 
+	if (IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT))
+		start_watchdog();
+
 	free(load_options);
 
 	return ret;
diff --git a/cmd/efidebug.c b/cmd/efidebug.c
index 9874838b00..5fa0cd1df7 100644
--- a/cmd/efidebug.c
+++ b/cmd/efidebug.c
@@ -16,6 +16,7 @@
 #include <mapmem.h>
 #include <search.h>
 #include <linux/ctype.h>
+#include <wdt.h>
 
 #define BS systab.boottime
 
@@ -1131,6 +1132,9 @@ static int do_efi_test_bootmgr(struct cmd_tbl *cmdtp, int flag,
 	ret = efi_bootmgr_load(&image, &load_options);
 	printf("efi_bootmgr_load() returned: %ld\n", ret & ~EFI_ERROR_MASK);
 
+	if (IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT))
+		stop_watchdog();
+
 	/* We call efi_start_image() even if error for test purpose. */
 	ret = EFI_CALL(efi_start_image(image, &exit_data_size, &exit_data));
 	printf("efi_start_image() returned: %ld\n", ret & ~EFI_ERROR_MASK);
@@ -1139,6 +1143,9 @@ static int do_efi_test_bootmgr(struct cmd_tbl *cmdtp, int flag,
 
 	efi_restore_gd();
 
+	if (IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT))
+		start_watchdog();
+
 	free(load_options);
 	return CMD_RET_SUCCESS;
 }
diff --git a/common/board_r.c b/common/board_r.c
index 9b2fec701a..6734d3ad25 100644
--- a/common/board_r.c
+++ b/common/board_r.c
@@ -731,7 +731,7 @@ static init_fnc_t init_sequence_r[] = {
 	stdio_init_tables,
 	serial_initialize,
 	initr_announce,
-#if CONFIG_IS_ENABLED(WDT)
+#if !IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_NOTHING)
 	initr_watchdog,
 #endif
 	INIT_FUNC_WATCHDOG_RESET
diff --git a/common/bootm_os.c b/common/bootm_os.c
index e9aaddf3e6..2b57a4e02c 100644
--- a/common/bootm_os.c
+++ b/common/bootm_os.c
@@ -19,6 +19,7 @@
 #include <mapmem.h>
 #include <vxworks.h>
 #include <tee/optee.h>
+#include <wdt.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -612,6 +613,10 @@ int boot_selected_os(int argc, char *const argv[], int state,
 {
 	arch_preboot_os();
 	board_preboot_os();
+
+	if (IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT))
+		stop_watchdog();
+
 	boot_fn(state, argc, argv, images);
 
 	/* Stand-alone may return when 'autostart' is 'no' */
diff --git a/common/spl/spl.c b/common/spl/spl.c
index 4840d1d367..63c47c739c 100644
--- a/common/spl/spl.c
+++ b/common/spl/spl.c
@@ -639,9 +639,9 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
 	spl_board_init();
 #endif
 
-#if defined(CONFIG_SPL_WATCHDOG_SUPPORT) && CONFIG_IS_ENABLED(WDT)
-	initr_watchdog();
-#endif
+	if (IS_ENABLED(CONFIG_SPL_WATCHDOG_SUPPORT) &&
+	    !IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_NOTHING))
+		initr_watchdog();
 
 	if (IS_ENABLED(CONFIG_SPL_OS_BOOT) || CONFIG_IS_ENABLED(HANDOFF) ||
 	    IS_ENABLED(CONFIG_SPL_ATF))
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 4532a40e45..861a0e012d 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1,5 +1,29 @@
 menu "Watchdog Timer Support"
 
+choice
+  prompt "Watchdog behavior"
+  default WATCHDOG_SUPERVISE_OS
+  depends on WDT
+
+config WATCHDOG_SUPERVISE_NOTHING
+   bool "Supervise nothing"
+   help
+     No watchdog will be started.
+
+config WATCHDOG_SUPERVISE_U_BOOT
+   bool "Supervise U-Boot only"
+   help
+     Upon U-Boot startup the first watchdog will be started automatically
+     and stopped as soon as an operating system is booted.
+
+config WATCHDOG_SUPERVISE_OS
+   bool "Supervise U-boot and operating system"
+   help
+     Upon U-Boot startup the first watchdog will be started automatically
+     and kept running even after booting the operating system.
+     Be aware, that the operating system needs to service the watchdog!
+endchoice
+
 config WATCHDOG
 	bool "Enable U-Boot watchdog reset"
 	depends on !HW_WATCHDOG
diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
index e632f077f3..ba3f8dde11 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -22,11 +22,10 @@ DECLARE_GLOBAL_DATA_PTR;
  * hw_margin_ms property.
  */
 static ulong reset_period = 1000;
+static ulong wdt_timeout = WATCHDOG_TIMEOUT_SECS;
 
 int initr_watchdog(void)
 {
-	u32 timeout = WATCHDOG_TIMEOUT_SECS;
-
 	/*
 	 * Init watchdog: This will call the probe function of the
 	 * watchdog driver, enabling the use of the device
@@ -42,21 +41,39 @@ int initr_watchdog(void)
 	}
 
 	if (CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA)) {
-		timeout = dev_read_u32_default(gd->watchdog_dev, "timeout-sec",
-					       WATCHDOG_TIMEOUT_SECS);
+		wdt_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;
 	}
 
-	wdt_start(gd->watchdog_dev, timeout * 1000, 0);
-	gd->flags |= GD_FLG_WDT_READY;
-	printf("WDT:   Started with%s servicing (%ds timeout)\n",
-	       IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", timeout);
+	start_watchdog();
+	printf("WDT:   Started with%s servicing (supervising %s, %lus timeout)\n",
+	       IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out",
+	       IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT) ? "U-Boot" : "OS",
+	       wdt_timeout);
 
 	return 0;
 }
 
+void start_watchdog(void)
+{
+	if (gd->watchdog_dev) {
+		wdt_start(gd->watchdog_dev, wdt_timeout * 1000, 0);
+		gd->flags |= GD_FLG_WDT_READY;
+	}
+}
+
+void stop_watchdog(void)
+{
+	if (gd->watchdog_dev) {
+		wdt_stop(gd->watchdog_dev);
+		gd->flags &= ~GD_FLG_WDT_READY;
+	}
+}
+
 int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags)
 {
 	const struct wdt_ops *ops = device_get_ops(dev);
diff --git a/include/wdt.h b/include/wdt.h
index bc242c2eb2..d261cd0dd6 100644
--- a/include/wdt.h
+++ b/include/wdt.h
@@ -105,6 +105,23 @@ struct wdt_ops {
 	int (*expire_now)(struct udevice *dev, ulong flags);
 };
 
+/*
+ * Initialize and start the global watchdog timer.
+ *
+ * @return: 0 if OK, -ve on error
+ */
 int initr_watchdog(void);
 
+/*
+ * (Re)start the global watchdog timer. Used after returning from an EFI or
+ * application image.
+ */
+void start_watchdog(void);
+
+/*
+ * Stop the global watchdog timer. Used before handing control to an EFI image
+ * or operating system.
+ */
+void stop_watchdog(void);
+
 #endif  /* _WDT_H_ */
-- 
2.20.1

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Mark Kettenis]

> From: Michael Walle <michael@walle.cc>
> Date: Wed, 23 Sep 2020 18:45:27 +0200
> 
> Let the user choose between three different behaviours of the watchdog:
>  (1) Keep the watchdog disabled
>  (2) Supervise u-boot
>  (3) Supervise u-boot and the operating systen (default)
> 
> Option (2) will disable the watchdog right before handing control to the
> operating system. This is useful when the OS is not aware of the
> watchdog. Option (3) doesn't disable the watchdog and assumes the OS
> will continue servicing.

(3) can't be the default, at least for EFI

The UEFI standard explicitly says that upon calling
ExitBootServices(), the watchdog timer is disabled.

In general, you can't expect an OS to have support for a particular
watchdog timer.  So (3) only makes sense in cases where U-Boot is
bundled with an OS image.

> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  cmd/boot.c                    |  7 +++++++
>  cmd/bootefi.c                 |  7 +++++++
>  cmd/efidebug.c                |  7 +++++++
>  common/board_r.c              |  2 +-
>  common/bootm_os.c             |  5 +++++
>  common/spl/spl.c              |  6 +++---
>  drivers/watchdog/Kconfig      | 24 ++++++++++++++++++++++++
>  drivers/watchdog/wdt-uclass.c | 33 +++++++++++++++++++++++++--------
>  include/wdt.h                 | 17 +++++++++++++++++
>  9 files changed, 96 insertions(+), 12 deletions(-)
> 
> diff --git a/cmd/boot.c b/cmd/boot.c
> index 36aba22b30..2412410371 100644
> --- a/cmd/boot.c
> +++ b/cmd/boot.c
> @@ -10,6 +10,7 @@
>  #include <common.h>
>  #include <command.h>
>  #include <net.h>
> +#include <wdt.h>
>  
>  #ifdef CONFIG_CMD_GO
>  
> @@ -33,6 +34,9 @@ static int do_go(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>  
>  	printf ("## Starting application at 0x%08lX ...\n", addr);
>  
> +	if (IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT))
> +		stop_watchdog();
> +
>  	/*
>  	 * pass address parameter as argv[0] (aka command name),
>  	 * and all remaining args
> @@ -40,6 +44,9 @@ static int do_go(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>  	rc = do_go_exec ((void *)addr, argc - 1, argv + 1);
>  	if (rc != 0) rcode = 1;
>  
> +	if (IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT))
> +		start_watchdog();
> +
>  	printf ("## Application terminated, rc = 0x%lX\n", rc);
>  	return rcode;
>  }
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 40d5ef2b3a..21f6650174 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -24,6 +24,7 @@
>  #include <memalign.h>
>  #include <asm-generic/sections.h>
>  #include <linux/linkage.h>
> +#include <wdt.h>
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> @@ -320,6 +321,9 @@ static efi_status_t do_bootefi_exec(efi_handle_t handle, void *load_options)
>  	efi_uintn_t exit_data_size = 0;
>  	u16 *exit_data = NULL;
>  
> +	if (IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT))
> +		stop_watchdog();
> +
>  	/* Call our payload! */
>  	ret = EFI_CALL(efi_start_image(handle, &exit_data_size, &exit_data));
>  	if (ret != EFI_SUCCESS) {
> @@ -333,6 +337,9 @@ static efi_status_t do_bootefi_exec(efi_handle_t handle, void *load_options)
>  
>  	efi_restore_gd();
>  
> +	if (IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT))
> +		start_watchdog();
> +
>  	free(load_options);
>  
>  	return ret;
> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> index 9874838b00..5fa0cd1df7 100644
> --- a/cmd/efidebug.c
> +++ b/cmd/efidebug.c
> @@ -16,6 +16,7 @@
>  #include <mapmem.h>
>  #include <search.h>
>  #include <linux/ctype.h>
> +#include <wdt.h>
>  
>  #define BS systab.boottime
>  
> @@ -1131,6 +1132,9 @@ static int do_efi_test_bootmgr(struct cmd_tbl *cmdtp, int flag,
>  	ret = efi_bootmgr_load(&image, &load_options);
>  	printf("efi_bootmgr_load() returned: %ld\n", ret & ~EFI_ERROR_MASK);
>  
> +	if (IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT))
> +		stop_watchdog();
> +
>  	/* We call efi_start_image() even if error for test purpose. */
>  	ret = EFI_CALL(efi_start_image(image, &exit_data_size, &exit_data));
>  	printf("efi_start_image() returned: %ld\n", ret & ~EFI_ERROR_MASK);
> @@ -1139,6 +1143,9 @@ static int do_efi_test_bootmgr(struct cmd_tbl *cmdtp, int flag,
>  
>  	efi_restore_gd();
>  
> +	if (IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT))
> +		start_watchdog();
> +
>  	free(load_options);
>  	return CMD_RET_SUCCESS;
>  }
> diff --git a/common/board_r.c b/common/board_r.c
> index 9b2fec701a..6734d3ad25 100644
> --- a/common/board_r.c
> +++ b/common/board_r.c
> @@ -731,7 +731,7 @@ static init_fnc_t init_sequence_r[] = {
>  	stdio_init_tables,
>  	serial_initialize,
>  	initr_announce,
> -#if CONFIG_IS_ENABLED(WDT)
> +#if !IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_NOTHING)
>  	initr_watchdog,
>  #endif
>  	INIT_FUNC_WATCHDOG_RESET
> diff --git a/common/bootm_os.c b/common/bootm_os.c
> index e9aaddf3e6..2b57a4e02c 100644
> --- a/common/bootm_os.c
> +++ b/common/bootm_os.c
> @@ -19,6 +19,7 @@
>  #include <mapmem.h>
>  #include <vxworks.h>
>  #include <tee/optee.h>
> +#include <wdt.h>
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> @@ -612,6 +613,10 @@ int boot_selected_os(int argc, char *const argv[], int state,
>  {
>  	arch_preboot_os();
>  	board_preboot_os();
> +
> +	if (IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT))
> +		stop_watchdog();
> +
>  	boot_fn(state, argc, argv, images);
>  
>  	/* Stand-alone may return when 'autostart' is 'no' */
> diff --git a/common/spl/spl.c b/common/spl/spl.c
> index 4840d1d367..63c47c739c 100644
> --- a/common/spl/spl.c
> +++ b/common/spl/spl.c
> @@ -639,9 +639,9 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
>  	spl_board_init();
>  #endif
>  
> -#if defined(CONFIG_SPL_WATCHDOG_SUPPORT) && CONFIG_IS_ENABLED(WDT)
> -	initr_watchdog();
> -#endif
> +	if (IS_ENABLED(CONFIG_SPL_WATCHDOG_SUPPORT) &&
> +	    !IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_NOTHING))
> +		initr_watchdog();
>  
>  	if (IS_ENABLED(CONFIG_SPL_OS_BOOT) || CONFIG_IS_ENABLED(HANDOFF) ||
>  	    IS_ENABLED(CONFIG_SPL_ATF))
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 4532a40e45..861a0e012d 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1,5 +1,29 @@
>  menu "Watchdog Timer Support"
>  
> +choice
> +  prompt "Watchdog behavior"
> +  default WATCHDOG_SUPERVISE_OS
> +  depends on WDT
> +
> +config WATCHDOG_SUPERVISE_NOTHING
> +   bool "Supervise nothing"
> +   help
> +     No watchdog will be started.
> +
> +config WATCHDOG_SUPERVISE_U_BOOT
> +   bool "Supervise U-Boot only"
> +   help
> +     Upon U-Boot startup the first watchdog will be started automatically
> +     and stopped as soon as an operating system is booted.
> +
> +config WATCHDOG_SUPERVISE_OS
> +   bool "Supervise U-boot and operating system"
> +   help
> +     Upon U-Boot startup the first watchdog will be started automatically
> +     and kept running even after booting the operating system.
> +     Be aware, that the operating system needs to service the watchdog!
> +endchoice
> +
>  config WATCHDOG
>  	bool "Enable U-Boot watchdog reset"
>  	depends on !HW_WATCHDOG
> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
> index e632f077f3..ba3f8dde11 100644
> --- a/drivers/watchdog/wdt-uclass.c
> +++ b/drivers/watchdog/wdt-uclass.c
> @@ -22,11 +22,10 @@ DECLARE_GLOBAL_DATA_PTR;
>   * hw_margin_ms property.
>   */
>  static ulong reset_period = 1000;
> +static ulong wdt_timeout = WATCHDOG_TIMEOUT_SECS;
>  
>  int initr_watchdog(void)
>  {
> -	u32 timeout = WATCHDOG_TIMEOUT_SECS;
> -
>  	/*
>  	 * Init watchdog: This will call the probe function of the
>  	 * watchdog driver, enabling the use of the device
> @@ -42,21 +41,39 @@ int initr_watchdog(void)
>  	}
>  
>  	if (CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA)) {
> -		timeout = dev_read_u32_default(gd->watchdog_dev, "timeout-sec",
> -					       WATCHDOG_TIMEOUT_SECS);
> +		wdt_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;
>  	}
>  
> -	wdt_start(gd->watchdog_dev, timeout * 1000, 0);
> -	gd->flags |= GD_FLG_WDT_READY;
> -	printf("WDT:   Started with%s servicing (%ds timeout)\n",
> -	       IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", timeout);
> +	start_watchdog();
> +	printf("WDT:   Started with%s servicing (supervising %s, %lus timeout)\n",
> +	       IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out",
> +	       IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT) ? "U-Boot" : "OS",
> +	       wdt_timeout);
>  
>  	return 0;
>  }
>  
> +void start_watchdog(void)
> +{
> +	if (gd->watchdog_dev) {
> +		wdt_start(gd->watchdog_dev, wdt_timeout * 1000, 0);
> +		gd->flags |= GD_FLG_WDT_READY;
> +	}
> +}
> +
> +void stop_watchdog(void)
> +{
> +	if (gd->watchdog_dev) {
> +		wdt_stop(gd->watchdog_dev);
> +		gd->flags &= ~GD_FLG_WDT_READY;
> +	}
> +}
> +
>  int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags)
>  {
>  	const struct wdt_ops *ops = device_get_ops(dev);
> diff --git a/include/wdt.h b/include/wdt.h
> index bc242c2eb2..d261cd0dd6 100644
> --- a/include/wdt.h
> +++ b/include/wdt.h
> @@ -105,6 +105,23 @@ struct wdt_ops {
>  	int (*expire_now)(struct udevice *dev, ulong flags);
>  };
>  
> +/*
> + * Initialize and start the global watchdog timer.
> + *
> + * @return: 0 if OK, -ve on error
> + */
>  int initr_watchdog(void);
>  
> +/*
> + * (Re)start the global watchdog timer. Used after returning from an EFI or
> + * application image.
> + */
> +void start_watchdog(void);
> +
> +/*
> + * Stop the global watchdog timer. Used before handing control to an EFI image
> + * or operating system.
> + */
> +void stop_watchdog(void);
> +
>  #endif  /* _WDT_H_ */
> -- 
> 2.20.1
> 
> 

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Tom Rini]

On Wed, Sep 23, 2020 at 07:01:54PM +0200, Mark Kettenis wrote:
> > From: Michael Walle <michael@walle.cc>
> > Date: Wed, 23 Sep 2020 18:45:27 +0200
> > 
> > Let the user choose between three different behaviours of the watchdog:
> >  (1) Keep the watchdog disabled
> >  (2) Supervise u-boot
> >  (3) Supervise u-boot and the operating systen (default)
> > 
> > Option (2) will disable the watchdog right before handing control to the
> > operating system. This is useful when the OS is not aware of the
> > watchdog. Option (3) doesn't disable the watchdog and assumes the OS
> > will continue servicing.
> 
> (3) can't be the default, at least for EFI
> 
> The UEFI standard explicitly says that upon calling
> ExitBootServices(), the watchdog timer is disabled.
> 
> In general, you can't expect an OS to have support for a particular
> watchdog timer.  So (3) only makes sense in cases where U-Boot is
> bundled with an OS image.

We need to be careful here then.  The current and historical / generally
expected behavior is if we've enabled the watchdog we supervise it and
leave it enabled for the OS.  Given what UEFI requires I'd like to see
that case handled with a print about disabling the watchdog so it's not
a surprise to the user.  I say this because it's a surprise to me and I
guess answers the question of "how does x86 handle this?" I had the
other day.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200923/34d47213/attachment.sig>

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Heinrich Schuchardt]

On 9/23/20 6:45 PM, Michael Walle wrote:
> Let the user choose between three different behaviours of the watchdog:
>  (1) Keep the watchdog disabled
>  (2) Supervise u-boot
>  (3) Supervise u-boot and the operating systen (default)
>
> Option (2) will disable the watchdog right before handing control to the
> operating system. This is useful when the OS is not aware of the
> watchdog. Option (3) doesn't disable the watchdog and assumes the OS
> will continue servicing.
>
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  cmd/boot.c                    |  7 +++++++
>  cmd/bootefi.c                 |  7 +++++++
>  cmd/efidebug.c                |  7 +++++++
>  common/board_r.c              |  2 +-
>  common/bootm_os.c             |  5 +++++
>  common/spl/spl.c              |  6 +++---
>  drivers/watchdog/Kconfig      | 24 ++++++++++++++++++++++++
>  drivers/watchdog/wdt-uclass.c | 33 +++++++++++++++++++++++++--------
>  include/wdt.h                 | 17 +++++++++++++++++
>  9 files changed, 96 insertions(+), 12 deletions(-)
>
> diff --git a/cmd/boot.c b/cmd/boot.c
> index 36aba22b30..2412410371 100644
> --- a/cmd/boot.c
> +++ b/cmd/boot.c
> @@ -10,6 +10,7 @@
>  #include <common.h>
>  #include <command.h>
>  #include <net.h>
> +#include <wdt.h>
>
>  #ifdef CONFIG_CMD_GO
>
> @@ -33,6 +34,9 @@ static int do_go(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>
>  	printf ("## Starting application at 0x%08lX ...\n", addr);
>
> +	if (IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT))
> +		stop_watchdog();
> +
>  	/*
>  	 * pass address parameter as argv[0] (aka command name),
>  	 * and all remaining args
> @@ -40,6 +44,9 @@ static int do_go(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>  	rc = do_go_exec ((void *)addr, argc - 1, argv + 1);
>  	if (rc != 0) rcode = 1;
>
> +	if (IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT))
> +		start_watchdog();
> +
>  	printf ("## Application terminated, rc = 0x%lX\n", rc);
>  	return rcode;
>  }
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 40d5ef2b3a..21f6650174 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -24,6 +24,7 @@
>  #include <memalign.h>
>  #include <asm-generic/sections.h>
>  #include <linux/linkage.h>
> +#include <wdt.h>
>
>  DECLARE_GLOBAL_DATA_PTR;
>
> @@ -320,6 +321,9 @@ static efi_status_t do_bootefi_exec(efi_handle_t handle, void *load_options)
>  	efi_uintn_t exit_data_size = 0;
>  	u16 *exit_data = NULL;
>
> +	if (IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT))
> +		stop_watchdog();
> +
>  	/* Call our payload! */
>  	ret = EFI_CALL(efi_start_image(handle, &exit_data_size, &exit_data));
>  	if (ret != EFI_SUCCESS) {
> @@ -333,6 +337,9 @@ static efi_status_t do_bootefi_exec(efi_handle_t handle, void *load_options)
>
>  	efi_restore_gd();
>
> +	if (IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT))
> +		start_watchdog();
> +
>  	free(load_options);
>
>  	return ret;
> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> index 9874838b00..5fa0cd1df7 100644
> --- a/cmd/efidebug.c
> +++ b/cmd/efidebug.c
> @@ -16,6 +16,7 @@
>  #include <mapmem.h>
>  #include <search.h>
>  #include <linux/ctype.h>
> +#include <wdt.h>
>
>  #define BS systab.boottime
>
> @@ -1131,6 +1132,9 @@ static int do_efi_test_bootmgr(struct cmd_tbl *cmdtp, int flag,
>  	ret = efi_bootmgr_load(&image, &load_options);
>  	printf("efi_bootmgr_load() returned: %ld\n", ret & ~EFI_ERROR_MASK);
>
> +	if (IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT))
> +		stop_watchdog();
> +
>  	/* We call efi_start_image() even if error for test purpose. */
>  	ret = EFI_CALL(efi_start_image(image, &exit_data_size, &exit_data));
>  	printf("efi_start_image() returned: %ld\n", ret & ~EFI_ERROR_MASK);
> @@ -1139,6 +1143,9 @@ static int do_efi_test_bootmgr(struct cmd_tbl *cmdtp, int flag,
>
>  	efi_restore_gd();
>
> +	if (IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT))
> +		start_watchdog();
> +
>  	free(load_options);
>  	return CMD_RET_SUCCESS;
>  }
> diff --git a/common/board_r.c b/common/board_r.c
> index 9b2fec701a..6734d3ad25 100644
> --- a/common/board_r.c
> +++ b/common/board_r.c
> @@ -731,7 +731,7 @@ static init_fnc_t init_sequence_r[] = {
>  	stdio_init_tables,
>  	serial_initialize,
>  	initr_announce,
> -#if CONFIG_IS_ENABLED(WDT)
> +#if !IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_NOTHING)
>  	initr_watchdog,

This does not compile on odroid_c2_defconfig.

aarch64-linux-gnu-ld.bfd:
common/built-in.o:(.data.init_sequence_r+0x88): undefined reference to
`initr_watchdog'
make: *** [Makefile:1753: u-boot] Error 1

Best regards

Heinrich

>  #endif
>  	INIT_FUNC_WATCHDOG_RESET
> diff --git a/common/bootm_os.c b/common/bootm_os.c
> index e9aaddf3e6..2b57a4e02c 100644
> --- a/common/bootm_os.c
> +++ b/common/bootm_os.c
> @@ -19,6 +19,7 @@
>  #include <mapmem.h>
>  #include <vxworks.h>
>  #include <tee/optee.h>
> +#include <wdt.h>
>
>  DECLARE_GLOBAL_DATA_PTR;
>
> @@ -612,6 +613,10 @@ int boot_selected_os(int argc, char *const argv[], int state,
>  {
>  	arch_preboot_os();
>  	board_preboot_os();
> +
> +	if (IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT))
> +		stop_watchdog();
> +
>  	boot_fn(state, argc, argv, images);
>
>  	/* Stand-alone may return when 'autostart' is 'no' */
> diff --git a/common/spl/spl.c b/common/spl/spl.c
> index 4840d1d367..63c47c739c 100644
> --- a/common/spl/spl.c
> +++ b/common/spl/spl.c
> @@ -639,9 +639,9 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
>  	spl_board_init();
>  #endif
>
> -#if defined(CONFIG_SPL_WATCHDOG_SUPPORT) && CONFIG_IS_ENABLED(WDT)
> -	initr_watchdog();
> -#endif
> +	if (IS_ENABLED(CONFIG_SPL_WATCHDOG_SUPPORT) &&
> +	    !IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_NOTHING))
> +		initr_watchdog();
>
>  	if (IS_ENABLED(CONFIG_SPL_OS_BOOT) || CONFIG_IS_ENABLED(HANDOFF) ||
>  	    IS_ENABLED(CONFIG_SPL_ATF))
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 4532a40e45..861a0e012d 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1,5 +1,29 @@
>  menu "Watchdog Timer Support"
>
> +choice
> +  prompt "Watchdog behavior"
> +  default WATCHDOG_SUPERVISE_OS
> +  depends on WDT
> +
> +config WATCHDOG_SUPERVISE_NOTHING
> +   bool "Supervise nothing"
> +   help
> +     No watchdog will be started.
> +
> +config WATCHDOG_SUPERVISE_U_BOOT
> +   bool "Supervise U-Boot only"
> +   help
> +     Upon U-Boot startup the first watchdog will be started automatically
> +     and stopped as soon as an operating system is booted.
> +
> +config WATCHDOG_SUPERVISE_OS
> +   bool "Supervise U-boot and operating system"
> +   help
> +     Upon U-Boot startup the first watchdog will be started automatically
> +     and kept running even after booting the operating system.
> +     Be aware, that the operating system needs to service the watchdog!
> +endchoice
> +
>  config WATCHDOG
>  	bool "Enable U-Boot watchdog reset"
>  	depends on !HW_WATCHDOG
> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
> index e632f077f3..ba3f8dde11 100644
> --- a/drivers/watchdog/wdt-uclass.c
> +++ b/drivers/watchdog/wdt-uclass.c
> @@ -22,11 +22,10 @@ DECLARE_GLOBAL_DATA_PTR;
>   * hw_margin_ms property.
>   */
>  static ulong reset_period = 1000;
> +static ulong wdt_timeout = WATCHDOG_TIMEOUT_SECS;
>
>  int initr_watchdog(void)
>  {
> -	u32 timeout = WATCHDOG_TIMEOUT_SECS;
> -
>  	/*
>  	 * Init watchdog: This will call the probe function of the
>  	 * watchdog driver, enabling the use of the device
> @@ -42,21 +41,39 @@ int initr_watchdog(void)
>  	}
>
>  	if (CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA)) {
> -		timeout = dev_read_u32_default(gd->watchdog_dev, "timeout-sec",
> -					       WATCHDOG_TIMEOUT_SECS);
> +		wdt_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;
>  	}
>
> -	wdt_start(gd->watchdog_dev, timeout * 1000, 0);
> -	gd->flags |= GD_FLG_WDT_READY;
> -	printf("WDT:   Started with%s servicing (%ds timeout)\n",
> -	       IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", timeout);
> +	start_watchdog();
> +	printf("WDT:   Started with%s servicing (supervising %s, %lus timeout)\n",
> +	       IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out",
> +	       IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT) ? "U-Boot" : "OS",
> +	       wdt_timeout);
>
>  	return 0;
>  }
>
> +void start_watchdog(void)
> +{
> +	if (gd->watchdog_dev) {
> +		wdt_start(gd->watchdog_dev, wdt_timeout * 1000, 0);
> +		gd->flags |= GD_FLG_WDT_READY;
> +	}
> +}
> +
> +void stop_watchdog(void)
> +{
> +	if (gd->watchdog_dev) {
> +		wdt_stop(gd->watchdog_dev);
> +		gd->flags &= ~GD_FLG_WDT_READY;
> +	}
> +}
> +
>  int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags)
>  {
>  	const struct wdt_ops *ops = device_get_ops(dev);
> diff --git a/include/wdt.h b/include/wdt.h
> index bc242c2eb2..d261cd0dd6 100644
> --- a/include/wdt.h
> +++ b/include/wdt.h
> @@ -105,6 +105,23 @@ struct wdt_ops {
>  	int (*expire_now)(struct udevice *dev, ulong flags);
>  };
>
> +/*
> + * Initialize and start the global watchdog timer.
> + *
> + * @return: 0 if OK, -ve on error
> + */
>  int initr_watchdog(void);
>
> +/*
> + * (Re)start the global watchdog timer. Used after returning from an EFI or
> + * application image.
> + */
> +void start_watchdog(void);
> +
> +/*
> + * Stop the global watchdog timer. Used before handing control to an EFI image
> + * or operating system.
> + */
> +void stop_watchdog(void);
> +
>  #endif  /* _WDT_H_ */
>

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Tom Rini]

On Wed, Sep 23, 2020 at 07:21:57PM +0200, Heinrich Schuchardt wrote:
> On 9/23/20 6:45 PM, Michael Walle wrote:
> > Let the user choose between three different behaviours of the watchdog:
> >  (1) Keep the watchdog disabled
> >  (2) Supervise u-boot
> >  (3) Supervise u-boot and the operating systen (default)
> >
> > Option (2) will disable the watchdog right before handing control to the
> > operating system. This is useful when the OS is not aware of the
> > watchdog. Option (3) doesn't disable the watchdog and assumes the OS
> > will continue servicing.
> >
> > Signed-off-by: Michael Walle <michael@walle.cc>
> > ---
> >  cmd/boot.c                    |  7 +++++++
> >  cmd/bootefi.c                 |  7 +++++++
> >  cmd/efidebug.c                |  7 +++++++
> >  common/board_r.c              |  2 +-
> >  common/bootm_os.c             |  5 +++++
> >  common/spl/spl.c              |  6 +++---
> >  drivers/watchdog/Kconfig      | 24 ++++++++++++++++++++++++
> >  drivers/watchdog/wdt-uclass.c | 33 +++++++++++++++++++++++++--------
> >  include/wdt.h                 | 17 +++++++++++++++++
> >  9 files changed, 96 insertions(+), 12 deletions(-)
> >
> > diff --git a/cmd/boot.c b/cmd/boot.c
> > index 36aba22b30..2412410371 100644
> > --- a/cmd/boot.c
> > +++ b/cmd/boot.c
> > @@ -10,6 +10,7 @@
> >  #include <common.h>
> >  #include <command.h>
> >  #include <net.h>
> > +#include <wdt.h>
> >
> >  #ifdef CONFIG_CMD_GO
> >
> > @@ -33,6 +34,9 @@ static int do_go(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> >
> >  	printf ("## Starting application at 0x%08lX ...\n", addr);
> >
> > +	if (IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT))
> > +		stop_watchdog();
> > +
> >  	/*
> >  	 * pass address parameter as argv[0] (aka command name),
> >  	 * and all remaining args
> > @@ -40,6 +44,9 @@ static int do_go(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> >  	rc = do_go_exec ((void *)addr, argc - 1, argv + 1);
> >  	if (rc != 0) rcode = 1;
> >
> > +	if (IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT))
> > +		start_watchdog();
> > +
> >  	printf ("## Application terminated, rc = 0x%lX\n", rc);
> >  	return rcode;
> >  }
> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > index 40d5ef2b3a..21f6650174 100644
> > --- a/cmd/bootefi.c
> > +++ b/cmd/bootefi.c
> > @@ -24,6 +24,7 @@
> >  #include <memalign.h>
> >  #include <asm-generic/sections.h>
> >  #include <linux/linkage.h>
> > +#include <wdt.h>
> >
> >  DECLARE_GLOBAL_DATA_PTR;
> >
> > @@ -320,6 +321,9 @@ static efi_status_t do_bootefi_exec(efi_handle_t handle, void *load_options)
> >  	efi_uintn_t exit_data_size = 0;
> >  	u16 *exit_data = NULL;
> >
> > +	if (IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT))
> > +		stop_watchdog();
> > +
> >  	/* Call our payload! */
> >  	ret = EFI_CALL(efi_start_image(handle, &exit_data_size, &exit_data));
> >  	if (ret != EFI_SUCCESS) {
> > @@ -333,6 +337,9 @@ static efi_status_t do_bootefi_exec(efi_handle_t handle, void *load_options)
> >
> >  	efi_restore_gd();
> >
> > +	if (IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT))
> > +		start_watchdog();
> > +
> >  	free(load_options);
> >
> >  	return ret;
> > diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> > index 9874838b00..5fa0cd1df7 100644
> > --- a/cmd/efidebug.c
> > +++ b/cmd/efidebug.c
> > @@ -16,6 +16,7 @@
> >  #include <mapmem.h>
> >  #include <search.h>
> >  #include <linux/ctype.h>
> > +#include <wdt.h>
> >
> >  #define BS systab.boottime
> >
> > @@ -1131,6 +1132,9 @@ static int do_efi_test_bootmgr(struct cmd_tbl *cmdtp, int flag,
> >  	ret = efi_bootmgr_load(&image, &load_options);
> >  	printf("efi_bootmgr_load() returned: %ld\n", ret & ~EFI_ERROR_MASK);
> >
> > +	if (IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT))
> > +		stop_watchdog();
> > +
> >  	/* We call efi_start_image() even if error for test purpose. */
> >  	ret = EFI_CALL(efi_start_image(image, &exit_data_size, &exit_data));
> >  	printf("efi_start_image() returned: %ld\n", ret & ~EFI_ERROR_MASK);
> > @@ -1139,6 +1143,9 @@ static int do_efi_test_bootmgr(struct cmd_tbl *cmdtp, int flag,
> >
> >  	efi_restore_gd();
> >
> > +	if (IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT))
> > +		start_watchdog();
> > +
> >  	free(load_options);
> >  	return CMD_RET_SUCCESS;
> >  }
> > diff --git a/common/board_r.c b/common/board_r.c
> > index 9b2fec701a..6734d3ad25 100644
> > --- a/common/board_r.c
> > +++ b/common/board_r.c
> > @@ -731,7 +731,7 @@ static init_fnc_t init_sequence_r[] = {
> >  	stdio_init_tables,
> >  	serial_initialize,
> >  	initr_announce,
> > -#if CONFIG_IS_ENABLED(WDT)
> > +#if !IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_NOTHING)
> >  	initr_watchdog,
> 
> This does not compile on odroid_c2_defconfig.
> 
> aarch64-linux-gnu-ld.bfd:
> common/built-in.o:(.data.init_sequence_r+0x88): undefined reference to
> `initr_watchdog'
> make: *** [Makefile:1753: u-boot] Error 1

I think a pre-req for this will be to have re-run the migration of
CONFIG_WATCHDOG / CONFIG_WDT from config headers and in to defconfigs.
I'm not 100% sure that's what's causing the breakage here (migration
isn't done yet) but I could see that being a problem.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200923/eca32699/attachment.sig>

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Heinrich Schuchardt]

On 9/23/20 7:14 PM, Tom Rini wrote:
> On Wed, Sep 23, 2020 at 07:01:54PM +0200, Mark Kettenis wrote:
>>> From: Michael Walle <michael@walle.cc>
>>> Date: Wed, 23 Sep 2020 18:45:27 +0200
>>>
>>> Let the user choose between three different behaviours of the watchdog:
>>>  (1) Keep the watchdog disabled
>>>  (2) Supervise u-boot
>>>  (3) Supervise u-boot and the operating systen (default)
>>>
>>> Option (2) will disable the watchdog right before handing control to the
>>> operating system. This is useful when the OS is not aware of the
>>> watchdog. Option (3) doesn't disable the watchdog and assumes the OS
>>> will continue servicing.
>>
>> (3) can't be the default, at least for EFI
>>
>> The UEFI standard explicitly says that upon calling
>> ExitBootServices(), the watchdog timer is disabled.
>>
>> In general, you can't expect an OS to have support for a particular
>> watchdog timer.  So (3) only makes sense in cases where U-Boot is
>> bundled with an OS image.
>
> We need to be careful here then.  The current and historical / generally
> expected behavior is if we've enabled the watchdog we supervise it and
> leave it enabled for the OS.  Given what UEFI requires I'd like to see
> that case handled with a print about disabling the watchdog so it's not

Not printf(), maybe log_info().

The disabling has to occur in ExitBootServices() (aka.
efi_exit_boot_services()). Here we are in the middle of an executing
UEFI application. Printing anything on the screen may mess up the output
of the UEFI application.

So, please, don't output anything.

Best regards

Heinrich

> a surprise to the user.  I say this because it's a surprise to me and I
> guess answers the question of "how does x86 handle this?" I had the
> other day.
>

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Tom Rini]

On Wed, Sep 23, 2020 at 07:31:00PM +0200, Heinrich Schuchardt wrote:
> On 9/23/20 7:14 PM, Tom Rini wrote:
> > On Wed, Sep 23, 2020 at 07:01:54PM +0200, Mark Kettenis wrote:
> >>> From: Michael Walle <michael@walle.cc>
> >>> Date: Wed, 23 Sep 2020 18:45:27 +0200
> >>>
> >>> Let the user choose between three different behaviours of the watchdog:
> >>>  (1) Keep the watchdog disabled
> >>>  (2) Supervise u-boot
> >>>  (3) Supervise u-boot and the operating systen (default)
> >>>
> >>> Option (2) will disable the watchdog right before handing control to the
> >>> operating system. This is useful when the OS is not aware of the
> >>> watchdog. Option (3) doesn't disable the watchdog and assumes the OS
> >>> will continue servicing.
> >>
> >> (3) can't be the default, at least for EFI
> >>
> >> The UEFI standard explicitly says that upon calling
> >> ExitBootServices(), the watchdog timer is disabled.
> >>
> >> In general, you can't expect an OS to have support for a particular
> >> watchdog timer.  So (3) only makes sense in cases where U-Boot is
> >> bundled with an OS image.
> >
> > We need to be careful here then.  The current and historical / generally
> > expected behavior is if we've enabled the watchdog we supervise it and
> > leave it enabled for the OS.  Given what UEFI requires I'd like to see
> > that case handled with a print about disabling the watchdog so it's not
> 
> Not printf(), maybe log_info().
> 
> The disabling has to occur in ExitBootServices() (aka.
> efi_exit_boot_services()). Here we are in the middle of an executing
> UEFI application. Printing anything on the screen may mess up the output
> of the UEFI application.
> 
> So, please, don't output anything.

We need to find a good way to inform the user we're disabling their
watchdog.  Maybe before we fully jump in to UEFI note that it will be
disabled before entering the OS?  Or something a bit more generally
understood than ExitBootServices() having been called.  I don't know
_where_ the best place is, but I think it's important to inform the
user.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200923/594807fc/attachment.sig>

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Mark Kettenis]

> Date: Wed, 23 Sep 2020 13:14:09 -0400
> From: Tom Rini <trini@konsulko.com>
> 
> On Wed, Sep 23, 2020 at 07:01:54PM +0200, Mark Kettenis wrote:
> > > From: Michael Walle <michael@walle.cc>
> > > Date: Wed, 23 Sep 2020 18:45:27 +0200
> > > 
> > > Let the user choose between three different behaviours of the watchdog:
> > >  (1) Keep the watchdog disabled
> > >  (2) Supervise u-boot
> > >  (3) Supervise u-boot and the operating systen (default)
> > > 
> > > Option (2) will disable the watchdog right before handing control to the
> > > operating system. This is useful when the OS is not aware of the
> > > watchdog. Option (3) doesn't disable the watchdog and assumes the OS
> > > will continue servicing.
> > 
> > (3) can't be the default, at least for EFI
> > 
> > The UEFI standard explicitly says that upon calling
> > ExitBootServices(), the watchdog timer is disabled.
> > 
> > In general, you can't expect an OS to have support for a particular
> > watchdog timer.  So (3) only makes sense in cases where U-Boot is
> > bundled with an OS image.
> 
> We need to be careful here then.  The current and historical / generally
> expected behavior is if we've enabled the watchdog we supervise it and
> leave it enabled for the OS.  Given what UEFI requires I'd like to see
> that case handled with a print about disabling the watchdog so it's not
> a surprise to the user.  I say this because it's a surprise to me and I
> guess answers the question of "how does x86 handle this?" I had the
> other day.

So the UEFI requirement actually is:

* Before starting an EFI payload a watchdog timer is started to reset
  the system after 5 minutes.

* This watchdog timer is cancelled as soon as the EFI payload calls
  ExitBootServices().

The OpenBSD kernel in general does not supervise the watchdog unless
explicitly requested to do so by the user.  What may happen is that
the driver for the hardware stops the watchdog timer when it attaches,
but (a) that is just a side-effect and (b) watchdog timer support
isn't implemented for all supported SoCs.

The OpenBSD EFI bootloader does explicitly disable the watchdog timer
though by calling SetWatchdogTime() as soon as it starts.  This is to
prevent an automatic reboot if you leave it sitting at the boot prompt
for more than 5 minutes.  This is done across all our architectures
that support EFI, including amd64.  So maybe that hides any
non-conforming behaviour.

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Heinrich Schuchardt]

On 9/23/20 7:53 PM, Mark Kettenis wrote:
>> Date: Wed, 23 Sep 2020 13:14:09 -0400
>> From: Tom Rini <trini@konsulko.com>
>>
>> On Wed, Sep 23, 2020 at 07:01:54PM +0200, Mark Kettenis wrote:
>>>> From: Michael Walle <michael@walle.cc>
>>>> Date: Wed, 23 Sep 2020 18:45:27 +0200
>>>>
>>>> Let the user choose between three different behaviours of the watchdog:
>>>>  (1) Keep the watchdog disabled
>>>>  (2) Supervise u-boot
>>>>  (3) Supervise u-boot and the operating systen (default)
>>>>
>>>> Option (2) will disable the watchdog right before handing control to the
>>>> operating system. This is useful when the OS is not aware of the
>>>> watchdog. Option (3) doesn't disable the watchdog and assumes the OS
>>>> will continue servicing.
>>>
>>> (3) can't be the default, at least for EFI
>>>
>>> The UEFI standard explicitly says that upon calling
>>> ExitBootServices(), the watchdog timer is disabled.
>>>

So 3) must not be allowed for CONFIG_EFI_LOADER=y which we have enabled
on most boards.

>>> In general, you can't expect an OS to have support for a particular
>>> watchdog timer.  So (3) only makes sense in cases where U-Boot is
>>> bundled with an OS image.
>>
>> We need to be careful here then.  The current and historical / generally
>> expected behavior is if we've enabled the watchdog we supervise it and
>> leave it enabled for the OS.  Given what UEFI requires I'd like to see
>> that case handled with a print about disabling the watchdog so it's not
>> a surprise to the user.  I say this because it's a surprise to me and I
>> guess answers the question of "how does x86 handle this?" I had the
>> other day.
>
> So the UEFI requirement actually is:
>
> * Before starting an EFI payload a watchdog timer is started to reset
>   the system after 5 minutes.
>
> * This watchdog timer is cancelled as soon as the EFI payload calls
>   ExitBootServices().

This requirement is currently fulfilled by a software watchdog in
lib/efi_loader/efi_watchdog.c. We need an emulation because many boards
don't offer a hardware watchdog in U-Boot.

>
> The OpenBSD kernel in general does not supervise the watchdog unless
> explicitly requested to do so by the user.  What may happen is that
> the driver for the hardware stops the watchdog timer when it attaches,
> but (a) that is just a side-effect and (b) watchdog timer support
> isn't implemented for all supported SoCs.
>
> The OpenBSD EFI bootloader does explicitly disable the watchdog timer
> though by calling SetWatchdogTime() as soon as it starts.  This is to
> prevent an automatic reboot if you leave it sitting at the boot prompt
> for more than 5 minutes.  This is done across all our architectures
> that support EFI, including amd64.  So maybe that hides any
> non-conforming behaviour.
>

SetWatchdogTime() resets the software watchdog implemented in
lib/efi_loader/efi_watchdog.c.

When an UEFI payload reads a key via the simple text input protocol or
the extended simple text input protocol U-Boot calls efi_timer_check()
which invokes WATCHDOG_RESET(). This is why hardware watchdogs don't
kill an UEFI application waiting for keyboard input.

Best regards

Heinrich

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Tom Rini]

On Wed, Sep 23, 2020 at 08:51:53PM +0200, Heinrich Schuchardt wrote:
> On 9/23/20 7:53 PM, Mark Kettenis wrote:
> >> Date: Wed, 23 Sep 2020 13:14:09 -0400
> >> From: Tom Rini <trini@konsulko.com>
> >>
> >> On Wed, Sep 23, 2020 at 07:01:54PM +0200, Mark Kettenis wrote:
> >>>> From: Michael Walle <michael@walle.cc>
> >>>> Date: Wed, 23 Sep 2020 18:45:27 +0200
> >>>>
> >>>> Let the user choose between three different behaviours of the watchdog:
> >>>>  (1) Keep the watchdog disabled
> >>>>  (2) Supervise u-boot
> >>>>  (3) Supervise u-boot and the operating systen (default)
> >>>>
> >>>> Option (2) will disable the watchdog right before handing control to the
> >>>> operating system. This is useful when the OS is not aware of the
> >>>> watchdog. Option (3) doesn't disable the watchdog and assumes the OS
> >>>> will continue servicing.
> >>>
> >>> (3) can't be the default, at least for EFI
> >>>
> >>> The UEFI standard explicitly says that upon calling
> >>> ExitBootServices(), the watchdog timer is disabled.
> >>>
> 
> So 3) must not be allowed for CONFIG_EFI_LOADER=y which we have enabled
> on most boards.

No, EFI is going to need to figure out how to deal with the watchdog.
See below...

> >>> In general, you can't expect an OS to have support for a particular
> >>> watchdog timer.  So (3) only makes sense in cases where U-Boot is
> >>> bundled with an OS image.
> >>
> >> We need to be careful here then.  The current and historical / generally
> >> expected behavior is if we've enabled the watchdog we supervise it and
> >> leave it enabled for the OS.  Given what UEFI requires I'd like to see
> >> that case handled with a print about disabling the watchdog so it's not
> >> a surprise to the user.  I say this because it's a surprise to me and I
> >> guess answers the question of "how does x86 handle this?" I had the
> >> other day.
> >
> > So the UEFI requirement actually is:
> >
> > * Before starting an EFI payload a watchdog timer is started to reset
> >   the system after 5 minutes.
> >
> > * This watchdog timer is cancelled as soon as the EFI payload calls
> >   ExitBootServices().
> 
> This requirement is currently fulfilled by a software watchdog in
> lib/efi_loader/efi_watchdog.c. We need an emulation because many boards
> don't offer a hardware watchdog in U-Boot.

OK.

> > The OpenBSD kernel in general does not supervise the watchdog unless
> > explicitly requested to do so by the user.  What may happen is that
> > the driver for the hardware stops the watchdog timer when it attaches,
> > but (a) that is just a side-effect and (b) watchdog timer support
> > isn't implemented for all supported SoCs.
> >
> > The OpenBSD EFI bootloader does explicitly disable the watchdog timer
> > though by calling SetWatchdogTime() as soon as it starts.  This is to
> > prevent an automatic reboot if you leave it sitting at the boot prompt
> > for more than 5 minutes.  This is done across all our architectures
> > that support EFI, including amd64.  So maybe that hides any
> > non-conforming behaviour.
> >
> 
> SetWatchdogTime() resets the software watchdog implemented in
> lib/efi_loader/efi_watchdog.c.
> 
> When an UEFI payload reads a key via the simple text input protocol or
> the extended simple text input protocol U-Boot calls efi_timer_check()
> which invokes WATCHDOG_RESET(). This is why hardware watchdogs don't
> kill an UEFI application waiting for keyboard input.

I think we need to make things such that if there's a HW watchdog, our
EFI loader/services make use of it.  And maybe just the Kconfig help
text ends up spelling out what the UEFI spec says happens with
watchdogs, so it's documented and expected behavior.  My concern is that
it's not obvious to the average user/developer what the behavior is
going to be in this case and that they'll expect a watchdog to fire in a
case where we've turned it off, and that's an important detail that's
just within the spec.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200923/38ac22f5/attachment.sig>

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Heinrich Schuchardt]

On 9/23/20 9:02 PM, Tom Rini wrote:
> On Wed, Sep 23, 2020 at 08:51:53PM +0200, Heinrich Schuchardt wrote:
>> On 9/23/20 7:53 PM, Mark Kettenis wrote:
>>>> Date: Wed, 23 Sep 2020 13:14:09 -0400
>>>> From: Tom Rini <trini@konsulko.com>
>>>>
>>>> On Wed, Sep 23, 2020 at 07:01:54PM +0200, Mark Kettenis wrote:
>>>>>> From: Michael Walle <michael@walle.cc>
>>>>>> Date: Wed, 23 Sep 2020 18:45:27 +0200
>>>>>>
>>>>>> Let the user choose between three different behaviours of the watchdog:
>>>>>>  (1) Keep the watchdog disabled
>>>>>>  (2) Supervise u-boot
>>>>>>  (3) Supervise u-boot and the operating systen (default)
>>>>>>
>>>>>> Option (2) will disable the watchdog right before handing control to the
>>>>>> operating system. This is useful when the OS is not aware of the
>>>>>> watchdog. Option (3) doesn't disable the watchdog and assumes the OS
>>>>>> will continue servicing.
>>>>>
>>>>> (3) can't be the default, at least for EFI
>>>>>
>>>>> The UEFI standard explicitly says that upon calling
>>>>> ExitBootServices(), the watchdog timer is disabled.
>>>>>
>>
>> So 3) must not be allowed for CONFIG_EFI_LOADER=y which we have enabled
>> on most boards.
>
> No, EFI is going to need to figure out how to deal with the watchdog.
> See below...

Any active watchdog after ExitBootServices() is forbidden by the UEFI spec.

3) does not describe disabling at ExitBootServices().

>
>>>>> In general, you can't expect an OS to have support for a particular
>>>>> watchdog timer.  So (3) only makes sense in cases where U-Boot is
>>>>> bundled with an OS image.
>>>>
>>>> We need to be careful here then.  The current and historical / generally
>>>> expected behavior is if we've enabled the watchdog we supervise it and
>>>> leave it enabled for the OS.  Given what UEFI requires I'd like to see
>>>> that case handled with a print about disabling the watchdog so it's not
>>>> a surprise to the user.  I say this because it's a surprise to me and I
>>>> guess answers the question of "how does x86 handle this?" I had the
>>>> other day.
>>>
>>> So the UEFI requirement actually is:
>>>
>>> * Before starting an EFI payload a watchdog timer is started to reset
>>>   the system after 5 minutes.
>>>
>>> * This watchdog timer is cancelled as soon as the EFI payload calls
>>>   ExitBootServices().
>>
>> This requirement is currently fulfilled by a software watchdog in
>> lib/efi_loader/efi_watchdog.c. We need an emulation because many boards
>> don't offer a hardware watchdog in U-Boot.
>
> OK.
>
>>> The OpenBSD kernel in general does not supervise the watchdog unless
>>> explicitly requested to do so by the user.  What may happen is that
>>> the driver for the hardware stops the watchdog timer when it attaches,
>>> but (a) that is just a side-effect and (b) watchdog timer support
>>> isn't implemented for all supported SoCs.
>>>
>>> The OpenBSD EFI bootloader does explicitly disable the watchdog timer
>>> though by calling SetWatchdogTime() as soon as it starts.  This is to
>>> prevent an automatic reboot if you leave it sitting at the boot prompt
>>> for more than 5 minutes.  This is done across all our architectures
>>> that support EFI, including amd64.  So maybe that hides any
>>> non-conforming behaviour.
>>>
>>
>> SetWatchdogTime() resets the software watchdog implemented in
>> lib/efi_loader/efi_watchdog.c.
>>
>> When an UEFI payload reads a key via the simple text input protocol or
>> the extended simple text input protocol U-Boot calls efi_timer_check()
>> which invokes WATCHDOG_RESET(). This is why hardware watchdogs don't
>> kill an UEFI application waiting for keyboard input.
>
> I think we need to make things such that if there's a HW watchdog, our
> EFI loader/services make use of it.  And maybe just the Kconfig help
> text ends up spelling out what the UEFI spec says happens with
> watchdogs, so it's documented and expected behavior.

There is a symbol CONFIG_WATCHDOG where mentioning UEFI might make
sense. But I could not find any software watchdog implementation
matching this symbol. Does it exist at all?

> My concern is that
> it's not obvious to the average user/developer what the behavior is
> going to be in this case and that they'll expect a watchdog to fire in a
> case where we've turned it off, and that's an important detail that's
> just within the spec.
>

git grep -n hw_watchdog_reset
tells us that most boards don't have a hardware watchdog.

There is no maintainer for drivers/watchdog/wdt-uclass.c.

doc/README.watchdog provides no description on the usage.

We do not even have a routine to set a time out period in struct wdt_ops.

Hardware watchdogs are already used by some serial drivers. So we cannot
independently use them for UEFI.

This does not look enticing.

Best regards

Heinrich

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Tom Rini]

On Wed, Sep 23, 2020 at 10:01:29PM +0200, Heinrich Schuchardt wrote:
> On 9/23/20 9:02 PM, Tom Rini wrote:
> > On Wed, Sep 23, 2020 at 08:51:53PM +0200, Heinrich Schuchardt wrote:
> >> On 9/23/20 7:53 PM, Mark Kettenis wrote:
> >>>> Date: Wed, 23 Sep 2020 13:14:09 -0400
> >>>> From: Tom Rini <trini@konsulko.com>
> >>>>
> >>>> On Wed, Sep 23, 2020 at 07:01:54PM +0200, Mark Kettenis wrote:
> >>>>>> From: Michael Walle <michael@walle.cc>
> >>>>>> Date: Wed, 23 Sep 2020 18:45:27 +0200
> >>>>>>
> >>>>>> Let the user choose between three different behaviours of the watchdog:
> >>>>>>  (1) Keep the watchdog disabled
> >>>>>>  (2) Supervise u-boot
> >>>>>>  (3) Supervise u-boot and the operating systen (default)
> >>>>>>
> >>>>>> Option (2) will disable the watchdog right before handing control to the
> >>>>>> operating system. This is useful when the OS is not aware of the
> >>>>>> watchdog. Option (3) doesn't disable the watchdog and assumes the OS
> >>>>>> will continue servicing.
> >>>>>
> >>>>> (3) can't be the default, at least for EFI
> >>>>>
> >>>>> The UEFI standard explicitly says that upon calling
> >>>>> ExitBootServices(), the watchdog timer is disabled.
> >>>>>
> >>
> >> So 3) must not be allowed for CONFIG_EFI_LOADER=y which we have enabled
> >> on most boards.
> >
> > No, EFI is going to need to figure out how to deal with the watchdog.
> > See below...
> 
> Any active watchdog after ExitBootServices() is forbidden by the UEFI spec.
> 
> 3) does not describe disabling at ExitBootServices().

Well, that's why I go back and forth on having the EFI loader say it's
changing the current watchdog behavior.

But just because EFI loader is enabled by default doesn't mean it gets
to set the overall default behavior for watchdogs.  It's an option in a
lot of cases where it's not actually used and a traditional method is
instead used.

> >>>>> In general, you can't expect an OS to have support for a particular
> >>>>> watchdog timer.  So (3) only makes sense in cases where U-Boot is
> >>>>> bundled with an OS image.
> >>>>
> >>>> We need to be careful here then.  The current and historical / generally
> >>>> expected behavior is if we've enabled the watchdog we supervise it and
> >>>> leave it enabled for the OS.  Given what UEFI requires I'd like to see
> >>>> that case handled with a print about disabling the watchdog so it's not
> >>>> a surprise to the user.  I say this because it's a surprise to me and I
> >>>> guess answers the question of "how does x86 handle this?" I had the
> >>>> other day.
> >>>
> >>> So the UEFI requirement actually is:
> >>>
> >>> * Before starting an EFI payload a watchdog timer is started to reset
> >>>   the system after 5 minutes.
> >>>
> >>> * This watchdog timer is cancelled as soon as the EFI payload calls
> >>>   ExitBootServices().
> >>
> >> This requirement is currently fulfilled by a software watchdog in
> >> lib/efi_loader/efi_watchdog.c. We need an emulation because many boards
> >> don't offer a hardware watchdog in U-Boot.
> >
> > OK.
> >
> >>> The OpenBSD kernel in general does not supervise the watchdog unless
> >>> explicitly requested to do so by the user.  What may happen is that
> >>> the driver for the hardware stops the watchdog timer when it attaches,
> >>> but (a) that is just a side-effect and (b) watchdog timer support
> >>> isn't implemented for all supported SoCs.
> >>>
> >>> The OpenBSD EFI bootloader does explicitly disable the watchdog timer
> >>> though by calling SetWatchdogTime() as soon as it starts.  This is to
> >>> prevent an automatic reboot if you leave it sitting at the boot prompt
> >>> for more than 5 minutes.  This is done across all our architectures
> >>> that support EFI, including amd64.  So maybe that hides any
> >>> non-conforming behaviour.
> >>>
> >>
> >> SetWatchdogTime() resets the software watchdog implemented in
> >> lib/efi_loader/efi_watchdog.c.
> >>
> >> When an UEFI payload reads a key via the simple text input protocol or
> >> the extended simple text input protocol U-Boot calls efi_timer_check()
> >> which invokes WATCHDOG_RESET(). This is why hardware watchdogs don't
> >> kill an UEFI application waiting for keyboard input.
> >
> > I think we need to make things such that if there's a HW watchdog, our
> > EFI loader/services make use of it.  And maybe just the Kconfig help
> > text ends up spelling out what the UEFI spec says happens with
> > watchdogs, so it's documented and expected behavior.
> 
> There is a symbol CONFIG_WATCHDOG where mentioning UEFI might make
> sense. But I could not find any software watchdog implementation
> matching this symbol. Does it exist at all?

Do we have a pure software watchdog?  I'm not 100% sure off-hand.

> > My concern is that
> > it's not obvious to the average user/developer what the behavior is
> > going to be in this case and that they'll expect a watchdog to fire in a
> > case where we've turned it off, and that's an important detail that's
> > just within the spec.
> >
> 
> git grep -n hw_watchdog_reset
> tells us that most boards don't have a hardware watchdog.

That's the non-DM function.  And looking at where we have drivers for
SoC watchdogs under drivers/watchdog/ I disagree with "most".  We keep
adding more, even.

> There is no maintainer for drivers/watchdog/wdt-uclass.c.

Yes, just general "Is DM, check with Simon".

> doc/README.watchdog provides no description on the usage.

Given the amount of docs you've been writing of late (thanks!) yes,
neither of us are surprised docs aren't in sync with implementation
right now.

> We do not even have a routine to set a time out period in struct wdt_ops.

Changing the timeout at run time?

> Hardware watchdogs are already used by some serial drivers. So we cannot
> independently use them for UEFI.

Yes, that's where we typically service the watchdog.

> This does not look enticing.

OK.  So long as you don't break the non-EFI case when EFI is enabled,
whatever needs doing to meet the spec, until someone has time to poke
this area a bit harder still.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200923/d9f53a1a/attachment.sig>

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Michael Walle]

Hi,

Am 2020-09-23 20:51, schrieb Heinrich Schuchardt:
> On 9/23/20 7:53 PM, Mark Kettenis wrote:
>>> Date: Wed, 23 Sep 2020 13:14:09 -0400
>>> From: Tom Rini <trini@konsulko.com>
>>> 
>>> On Wed, Sep 23, 2020 at 07:01:54PM +0200, Mark Kettenis wrote:
>>>>> From: Michael Walle <michael@walle.cc>
>>>>> Date: Wed, 23 Sep 2020 18:45:27 +0200
>>>>> 
>>>>> Let the user choose between three different behaviours of the 
>>>>> watchdog:
>>>>>  (1) Keep the watchdog disabled
>>>>>  (2) Supervise u-boot
>>>>>  (3) Supervise u-boot and the operating systen (default)
>>>>> 
>>>>> Option (2) will disable the watchdog right before handing control 
>>>>> to the
>>>>> operating system. This is useful when the OS is not aware of the
>>>>> watchdog. Option (3) doesn't disable the watchdog and assumes the 
>>>>> OS
>>>>> will continue servicing.
>>>> 
>>>> (3) can't be the default, at least for EFI
>>>> 
>>>> The UEFI standard explicitly says that upon calling
>>>> ExitBootServices(), the watchdog timer is disabled.
>>>> 
> 
> So 3) must not be allowed for CONFIG_EFI_LOADER=y which we have enabled
> on most boards.

"most not be allowed" seems a bit strong to me. First it is the users
choice (well at least on compile-time) and there may still be OSes out
there which are booted by EFI but still support the watchdog. And I 
agree
with Tom, that it is bad to disable it just to re-enable it again.

That being said, I still think having the watchdog enabled by default
is the exceptional case and not the common one. I know U-Boot might
be highly targeted to an embedded use case, but with efi_loader now
available it is shifting to a more "generic" (in the lack of a
better word) bootloader.

The reason (3) is the default is that this was the current behaviour
and I didn't want to change that.

>>>> In general, you can't expect an OS to have support for a particular
>>>> watchdog timer.  So (3) only makes sense in cases where U-Boot is
>>>> bundled with an OS image.
>>> 
>>> We need to be careful here then.  The current and historical / 
>>> generally
>>> expected behavior is if we've enabled the watchdog we supervise it 
>>> and
>>> leave it enabled for the OS.  Given what UEFI requires I'd like to 
>>> see
>>> that case handled with a print about disabling the watchdog so it's 
>>> not
>>> a surprise to the user.  I say this because it's a surprise to me and 
>>> I
>>> guess answers the question of "how does x86 handle this?" I had the
>>> other day.
>> 
>> So the UEFI requirement actually is:
>> 
>> * Before starting an EFI payload a watchdog timer is started to reset
>>   the system after 5 minutes.
>> 
>> * This watchdog timer is cancelled as soon as the EFI payload calls
>>   ExitBootServices().
> 
> This requirement is currently fulfilled by a software watchdog in
> lib/efi_loader/efi_watchdog.c. We need an emulation because many boards
> don't offer a hardware watchdog in U-Boot.

If a board has a hardware watchdog then there will be two running,
correct? Could we detect (during runtime) if the hardware watchdog
is capable of doing the 5min timeout and then use that one instead
of a software timer?

Then we could have another behavior mode which can be EFI-compliant,
i.e. disable the watchdog before jumping to the OS when using bootm/go
but set it to 5mins and keep it enabled until ExitBootServices() is
called in the bootefi case.

>> The OpenBSD kernel in general does not supervise the watchdog unless
>> explicitly requested to do so by the user.  What may happen is that
>> the driver for the hardware stops the watchdog timer when it attaches,
>> but (a) that is just a side-effect and (b) watchdog timer support
>> isn't implemented for all supported SoCs.
>> 
>> The OpenBSD EFI bootloader does explicitly disable the watchdog timer
>> though by calling SetWatchdogTime() as soon as it starts.  This is to
>> prevent an automatic reboot if you leave it sitting at the boot prompt
>> for more than 5 minutes.  This is done across all our architectures
>> that support EFI, including amd64.  So maybe that hides any
>> non-conforming behaviour.
>> 
> 
> SetWatchdogTime() resets the software watchdog implemented in
> lib/efi_loader/efi_watchdog.c.
> 
> When an UEFI payload reads a key via the simple text input protocol or
> the extended simple text input protocol U-Boot calls efi_timer_check()
> which invokes WATCHDOG_RESET(). This is why hardware watchdogs don't
> kill an UEFI application waiting for keyboard input.

Ok I was already curious in which codepath the watchdog is kicked (its
also kicked in efi_check_event(), right?). But can we be sure that EFI
application will call that (periodically)?

-michael

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Heinrich Schuchardt]

On 9/23/20 10:23 PM, Tom Rini wrote:
> On Wed, Sep 23, 2020 at 10:01:29PM +0200, Heinrich Schuchardt wrote:
>> On 9/23/20 9:02 PM, Tom Rini wrote:
>>> On Wed, Sep 23, 2020 at 08:51:53PM +0200, Heinrich Schuchardt wrote:
>>>> On 9/23/20 7:53 PM, Mark Kettenis wrote:
>>>>>> Date: Wed, 23 Sep 2020 13:14:09 -0400
>>>>>> From: Tom Rini <trini@konsulko.com>
>>>>>>
>>>>>> On Wed, Sep 23, 2020 at 07:01:54PM +0200, Mark Kettenis wrote:
>>>>>>>> From: Michael Walle <michael@walle.cc>
>>>>>>>> Date: Wed, 23 Sep 2020 18:45:27 +0200
>>>>>>>>
>>>>>>>> Let the user choose between three different behaviours of the watchdog:
>>>>>>>>  (1) Keep the watchdog disabled
>>>>>>>>  (2) Supervise u-boot
>>>>>>>>  (3) Supervise u-boot and the operating systen (default)
>>>>>>>>
>>>>>>>> Option (2) will disable the watchdog right before handing control to the
>>>>>>>> operating system. This is useful when the OS is not aware of the
>>>>>>>> watchdog. Option (3) doesn't disable the watchdog and assumes the OS
>>>>>>>> will continue servicing.
>>>>>>>
>>>>>>> (3) can't be the default, at least for EFI
>>>>>>>
>>>>>>> The UEFI standard explicitly says that upon calling
>>>>>>> ExitBootServices(), the watchdog timer is disabled.
>>>>>>>
>>>>
>>>> So 3) must not be allowed for CONFIG_EFI_LOADER=y which we have enabled
>>>> on most boards.
>>>
>>> No, EFI is going to need to figure out how to deal with the watchdog.
>>> See below...
>>
>> Any active watchdog after ExitBootServices() is forbidden by the UEFI spec.
>>
>> 3) does not describe disabling at ExitBootServices().
>
> Well, that's why I go back and forth on having the EFI loader say it's
> changing the current watchdog behavior.
>
> But just because EFI loader is enabled by default doesn't mean it gets
> to set the overall default behavior for watchdogs.  It's an option in a
> lot of cases where it's not actually used and a traditional method is
> instead used.
>
>>>>>>> In general, you can't expect an OS to have support for a particular
>>>>>>> watchdog timer.  So (3) only makes sense in cases where U-Boot is
>>>>>>> bundled with an OS image.
>>>>>>
>>>>>> We need to be careful here then.  The current and historical / generally
>>>>>> expected behavior is if we've enabled the watchdog we supervise it and
>>>>>> leave it enabled for the OS.  Given what UEFI requires I'd like to see
>>>>>> that case handled with a print about disabling the watchdog so it's not
>>>>>> a surprise to the user.  I say this because it's a surprise to me and I
>>>>>> guess answers the question of "how does x86 handle this?" I had the
>>>>>> other day.
>>>>>
>>>>> So the UEFI requirement actually is:
>>>>>
>>>>> * Before starting an EFI payload a watchdog timer is started to reset
>>>>>   the system after 5 minutes.
>>>>>
>>>>> * This watchdog timer is cancelled as soon as the EFI payload calls
>>>>>   ExitBootServices().
>>>>
>>>> This requirement is currently fulfilled by a software watchdog in
>>>> lib/efi_loader/efi_watchdog.c. We need an emulation because many boards
>>>> don't offer a hardware watchdog in U-Boot.
>>>
>>> OK.
>>>
>>>>> The OpenBSD kernel in general does not supervise the watchdog unless
>>>>> explicitly requested to do so by the user.  What may happen is that
>>>>> the driver for the hardware stops the watchdog timer when it attaches,
>>>>> but (a) that is just a side-effect and (b) watchdog timer support
>>>>> isn't implemented for all supported SoCs.
>>>>>
>>>>> The OpenBSD EFI bootloader does explicitly disable the watchdog timer
>>>>> though by calling SetWatchdogTime() as soon as it starts.  This is to
>>>>> prevent an automatic reboot if you leave it sitting at the boot prompt
>>>>> for more than 5 minutes.  This is done across all our architectures
>>>>> that support EFI, including amd64.  So maybe that hides any
>>>>> non-conforming behaviour.
>>>>>
>>>>
>>>> SetWatchdogTime() resets the software watchdog implemented in
>>>> lib/efi_loader/efi_watchdog.c.
>>>>
>>>> When an UEFI payload reads a key via the simple text input protocol or
>>>> the extended simple text input protocol U-Boot calls efi_timer_check()
>>>> which invokes WATCHDOG_RESET(). This is why hardware watchdogs don't
>>>> kill an UEFI application waiting for keyboard input.
>>>
>>> I think we need to make things such that if there's a HW watchdog, our
>>> EFI loader/services make use of it.  And maybe just the Kconfig help
>>> text ends up spelling out what the UEFI spec says happens with
>>> watchdogs, so it's documented and expected behavior.
>>
>> There is a symbol CONFIG_WATCHDOG where mentioning UEFI might make
>> sense. But I could not find any software watchdog implementation
>> matching this symbol. Does it exist at all?
>
> Do we have a pure software watchdog?  I'm not 100% sure off-hand.
>
>>> My concern is that
>>> it's not obvious to the average user/developer what the behavior is
>>> going to be in this case and that they'll expect a watchdog to fire in a
>>> case where we've turned it off, and that's an important detail that's
>>> just within the spec.
>>>
>>
>> git grep -n hw_watchdog_reset
>> tells us that most boards don't have a hardware watchdog.
>
> That's the non-DM function.  And looking at where we have drivers for
> SoC watchdogs under drivers/watchdog/ I disagree with "most".  We keep
> adding more, even.
>
>> There is no maintainer for drivers/watchdog/wdt-uclass.c.
>
> Yes, just general "Is DM, check with Simon".
>
>> doc/README.watchdog provides no description on the usage.
>
> Given the amount of docs you've been writing of late (thanks!) yes,
> neither of us are surprised docs aren't in sync with implementation
> right now.
>
>> We do not even have a routine to set a time out period in struct wdt_ops.
>
> Changing the timeout at run time?

That is what EFI_BOOT_SERVICES.SetWatchdogTimer() is about.

The UEFI application can set the watchdog timeout to a multiple of 1
second (without upper limit) or disable it completely.

But this must not interfere with our serial drivers which are using
hardware watchdogs. And now look at

include/configs/M5373EVB.h:27:
#define CONFIG_WATCHDOG_TIMEOUT   3360
/* timeout in ms, max is 3.36 sec */

The current use of hardware watchdog does not fit well to UEFI.

A possible design would be a 100 Hz watchdog, to which U-Boot drivers
can add listeners.

Best regards

Heinrich

>
>> Hardware watchdogs are already used by some serial drivers. So we cannot
>> independently use them for UEFI.
>
> Yes, that's where we typically service the watchdog.
>
>> This does not look enticing.
>
> OK.  So long as you don't break the non-EFI case when EFI is enabled,
> whatever needs doing to meet the spec, until someone has time to poke
> this area a bit harder still.
>

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Heinrich Schuchardt]

On 9/23/20 10:38 PM, Michael Walle wrote:
> Hi,
>
> Am 2020-09-23 20:51, schrieb Heinrich Schuchardt:
>> On 9/23/20 7:53 PM, Mark Kettenis wrote:
>>>> Date: Wed, 23 Sep 2020 13:14:09 -0400
>>>> From: Tom Rini <trini@konsulko.com>
>>>>
>>>> On Wed, Sep 23, 2020 at 07:01:54PM +0200, Mark Kettenis wrote:
>>>>>> From: Michael Walle <michael@walle.cc>
>>>>>> Date: Wed, 23 Sep 2020 18:45:27 +0200
>>>>>>
>>>>>> Let the user choose between three different behaviours of the
>>>>>> watchdog:
>>>>>> ?(1) Keep the watchdog disabled
>>>>>> ?(2) Supervise u-boot
>>>>>> ?(3) Supervise u-boot and the operating systen (default)
>>>>>>
>>>>>> Option (2) will disable the watchdog right before handing control
>>>>>> to the
>>>>>> operating system. This is useful when the OS is not aware of the
>>>>>> watchdog. Option (3) doesn't disable the watchdog and assumes the OS
>>>>>> will continue servicing.
>>>>>
>>>>> (3) can't be the default, at least for EFI
>>>>>
>>>>> The UEFI standard explicitly says that upon calling
>>>>> ExitBootServices(), the watchdog timer is disabled.
>>>>>
>>
>> So 3) must not be allowed for CONFIG_EFI_LOADER=y which we have enabled
>> on most boards.
>
> "most not be allowed" seems a bit strong to me. First it is the users
> choice (well at least on compile-time) and there may still be OSes out
> there which are booted by EFI but still support the watchdog. And I agree
> with Tom, that it is bad to disable it just to re-enable it again.
>
> That being said, I still think having the watchdog enabled by default
> is the exceptional case and not the common one. I know U-Boot might
> be highly targeted to an embedded use case, but with efi_loader now
> available it is shifting to a more "generic" (in the lack of a
> better word) bootloader.
>
> The reason (3) is the default is that this was the current behaviour
> and I didn't want to change that.
>
>>>>> In general, you can't expect an OS to have support for a particular
>>>>> watchdog timer.? So (3) only makes sense in cases where U-Boot is
>>>>> bundled with an OS image.
>>>>
>>>> We need to be careful here then.? The current and historical /
>>>> generally
>>>> expected behavior is if we've enabled the watchdog we supervise it and
>>>> leave it enabled for the OS.? Given what UEFI requires I'd like to see
>>>> that case handled with a print about disabling the watchdog so it's not
>>>> a surprise to the user.? I say this because it's a surprise to me and I
>>>> guess answers the question of "how does x86 handle this?" I had the
>>>> other day.
>>>
>>> So the UEFI requirement actually is:
>>>
>>> * Before starting an EFI payload a watchdog timer is started to reset
>>> ? the system after 5 minutes.
>>>
>>> * This watchdog timer is cancelled as soon as the EFI payload calls
>>> ? ExitBootServices().
>>
>> This requirement is currently fulfilled by a software watchdog in
>> lib/efi_loader/efi_watchdog.c. We need an emulation because many boards
>> don't offer a hardware watchdog in U-Boot.
>
> If a board has a hardware watchdog then there will be two running,
> correct? Could we detect (during runtime) if the hardware watchdog
> is capable of doing the 5min timeout and then use that one instead
> of a software timer?
>
> Then we could have another behavior mode which can be EFI-compliant,
> i.e. disable the watchdog before jumping to the OS when using bootm/go
> but set it to 5mins and keep it enabled until ExitBootServices() is
> called in the bootefi case.
>
>>> The OpenBSD kernel in general does not supervise the watchdog unless
>>> explicitly requested to do so by the user.? What may happen is that
>>> the driver for the hardware stops the watchdog timer when it attaches,
>>> but (a) that is just a side-effect and (b) watchdog timer support
>>> isn't implemented for all supported SoCs.
>>>
>>> The OpenBSD EFI bootloader does explicitly disable the watchdog timer
>>> though by calling SetWatchdogTime() as soon as it starts.? This is to
>>> prevent an automatic reboot if you leave it sitting at the boot prompt
>>> for more than 5 minutes.? This is done across all our architectures
>>> that support EFI, including amd64.? So maybe that hides any
>>> non-conforming behaviour.
>>>
>>
>> SetWatchdogTime() resets the software watchdog implemented in
>> lib/efi_loader/efi_watchdog.c.
>>
>> When an UEFI payload reads a key via the simple text input protocol or
>> the extended simple text input protocol U-Boot calls efi_timer_check()
>> which invokes WATCHDOG_RESET(). This is why hardware watchdogs don't
>> kill an UEFI application waiting for keyboard input.
>
> Ok I was already curious in which codepath the watchdog is kicked (its
> also kicked in efi_check_event(), right?). But can we be sure that EFI
> application will call that (periodically)?

No, if you have an endless loop in the UEFI payload without calling the
UEFI API the software watchdog will not kick in.

Best regards

Heinrich

>
> -michael

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Tom Rini]

On Wed, Sep 23, 2020 at 10:58:00PM +0200, Heinrich Schuchardt wrote:
> On 9/23/20 10:23 PM, Tom Rini wrote:
> > On Wed, Sep 23, 2020 at 10:01:29PM +0200, Heinrich Schuchardt wrote:
> >> On 9/23/20 9:02 PM, Tom Rini wrote:
> >>> On Wed, Sep 23, 2020 at 08:51:53PM +0200, Heinrich Schuchardt wrote:
> >>>> On 9/23/20 7:53 PM, Mark Kettenis wrote:
> >>>>>> Date: Wed, 23 Sep 2020 13:14:09 -0400
> >>>>>> From: Tom Rini <trini@konsulko.com>
> >>>>>>
> >>>>>> On Wed, Sep 23, 2020 at 07:01:54PM +0200, Mark Kettenis wrote:
> >>>>>>>> From: Michael Walle <michael@walle.cc>
> >>>>>>>> Date: Wed, 23 Sep 2020 18:45:27 +0200
> >>>>>>>>
> >>>>>>>> Let the user choose between three different behaviours of the watchdog:
> >>>>>>>>  (1) Keep the watchdog disabled
> >>>>>>>>  (2) Supervise u-boot
> >>>>>>>>  (3) Supervise u-boot and the operating systen (default)
> >>>>>>>>
> >>>>>>>> Option (2) will disable the watchdog right before handing control to the
> >>>>>>>> operating system. This is useful when the OS is not aware of the
> >>>>>>>> watchdog. Option (3) doesn't disable the watchdog and assumes the OS
> >>>>>>>> will continue servicing.
> >>>>>>>
> >>>>>>> (3) can't be the default, at least for EFI
> >>>>>>>
> >>>>>>> The UEFI standard explicitly says that upon calling
> >>>>>>> ExitBootServices(), the watchdog timer is disabled.
> >>>>>>>
> >>>>
> >>>> So 3) must not be allowed for CONFIG_EFI_LOADER=y which we have enabled
> >>>> on most boards.
> >>>
> >>> No, EFI is going to need to figure out how to deal with the watchdog.
> >>> See below...
> >>
> >> Any active watchdog after ExitBootServices() is forbidden by the UEFI spec.
> >>
> >> 3) does not describe disabling at ExitBootServices().
> >
> > Well, that's why I go back and forth on having the EFI loader say it's
> > changing the current watchdog behavior.
> >
> > But just because EFI loader is enabled by default doesn't mean it gets
> > to set the overall default behavior for watchdogs.  It's an option in a
> > lot of cases where it's not actually used and a traditional method is
> > instead used.
> >
> >>>>>>> In general, you can't expect an OS to have support for a particular
> >>>>>>> watchdog timer.  So (3) only makes sense in cases where U-Boot is
> >>>>>>> bundled with an OS image.
> >>>>>>
> >>>>>> We need to be careful here then.  The current and historical / generally
> >>>>>> expected behavior is if we've enabled the watchdog we supervise it and
> >>>>>> leave it enabled for the OS.  Given what UEFI requires I'd like to see
> >>>>>> that case handled with a print about disabling the watchdog so it's not
> >>>>>> a surprise to the user.  I say this because it's a surprise to me and I
> >>>>>> guess answers the question of "how does x86 handle this?" I had the
> >>>>>> other day.
> >>>>>
> >>>>> So the UEFI requirement actually is:
> >>>>>
> >>>>> * Before starting an EFI payload a watchdog timer is started to reset
> >>>>>   the system after 5 minutes.
> >>>>>
> >>>>> * This watchdog timer is cancelled as soon as the EFI payload calls
> >>>>>   ExitBootServices().
> >>>>
> >>>> This requirement is currently fulfilled by a software watchdog in
> >>>> lib/efi_loader/efi_watchdog.c. We need an emulation because many boards
> >>>> don't offer a hardware watchdog in U-Boot.
> >>>
> >>> OK.
> >>>
> >>>>> The OpenBSD kernel in general does not supervise the watchdog unless
> >>>>> explicitly requested to do so by the user.  What may happen is that
> >>>>> the driver for the hardware stops the watchdog timer when it attaches,
> >>>>> but (a) that is just a side-effect and (b) watchdog timer support
> >>>>> isn't implemented for all supported SoCs.
> >>>>>
> >>>>> The OpenBSD EFI bootloader does explicitly disable the watchdog timer
> >>>>> though by calling SetWatchdogTime() as soon as it starts.  This is to
> >>>>> prevent an automatic reboot if you leave it sitting at the boot prompt
> >>>>> for more than 5 minutes.  This is done across all our architectures
> >>>>> that support EFI, including amd64.  So maybe that hides any
> >>>>> non-conforming behaviour.
> >>>>>
> >>>>
> >>>> SetWatchdogTime() resets the software watchdog implemented in
> >>>> lib/efi_loader/efi_watchdog.c.
> >>>>
> >>>> When an UEFI payload reads a key via the simple text input protocol or
> >>>> the extended simple text input protocol U-Boot calls efi_timer_check()
> >>>> which invokes WATCHDOG_RESET(). This is why hardware watchdogs don't
> >>>> kill an UEFI application waiting for keyboard input.
> >>>
> >>> I think we need to make things such that if there's a HW watchdog, our
> >>> EFI loader/services make use of it.  And maybe just the Kconfig help
> >>> text ends up spelling out what the UEFI spec says happens with
> >>> watchdogs, so it's documented and expected behavior.
> >>
> >> There is a symbol CONFIG_WATCHDOG where mentioning UEFI might make
> >> sense. But I could not find any software watchdog implementation
> >> matching this symbol. Does it exist at all?
> >
> > Do we have a pure software watchdog?  I'm not 100% sure off-hand.
> >
> >>> My concern is that
> >>> it's not obvious to the average user/developer what the behavior is
> >>> going to be in this case and that they'll expect a watchdog to fire in a
> >>> case where we've turned it off, and that's an important detail that's
> >>> just within the spec.
> >>>
> >>
> >> git grep -n hw_watchdog_reset
> >> tells us that most boards don't have a hardware watchdog.
> >
> > That's the non-DM function.  And looking at where we have drivers for
> > SoC watchdogs under drivers/watchdog/ I disagree with "most".  We keep
> > adding more, even.
> >
> >> There is no maintainer for drivers/watchdog/wdt-uclass.c.
> >
> > Yes, just general "Is DM, check with Simon".
> >
> >> doc/README.watchdog provides no description on the usage.
> >
> > Given the amount of docs you've been writing of late (thanks!) yes,
> > neither of us are surprised docs aren't in sync with implementation
> > right now.
> >
> >> We do not even have a routine to set a time out period in struct wdt_ops.
> >
> > Changing the timeout at run time?
> 
> That is what EFI_BOOT_SERVICES.SetWatchdogTimer() is about.
> 
> The UEFI application can set the watchdog timeout to a multiple of 1
> second (without upper limit) or disable it completely.
> 
> But this must not interfere with our serial drivers which are using
> hardware watchdogs. And now look at
> 
> include/configs/M5373EVB.h:27:
> #define CONFIG_WATCHDOG_TIMEOUT   3360
> /* timeout in ms, max is 3.36 sec */

Yes, but that's also not a UEFI-possible platform.  But even the i.MX
watchdog seems to have a 128 second timeout as the default/max.

> The current use of hardware watchdog does not fit well to UEFI.
> 
> A possible design would be a 100 Hz watchdog, to which U-Boot drivers
> can add listeners.

Maybe we need to step back and ask what UEFI says about how to handle
hardware watchdogs.  They can't be as rare as it seems right now in the
x86 server world.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200923/e6594820/attachment.sig>

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Michael Walle]

Am 2020-09-23 19:28, schrieb Tom Rini:
> On Wed, Sep 23, 2020 at 07:21:57PM +0200, Heinrich Schuchardt wrote:
>> On 9/23/20 6:45 PM, Michael Walle wrote:
>> > Let the user choose between three different behaviours of the watchdog:
>> >  (1) Keep the watchdog disabled
>> >  (2) Supervise u-boot
>> >  (3) Supervise u-boot and the operating systen (default)
>> >
>> > Option (2) will disable the watchdog right before handing control to the
>> > operating system. This is useful when the OS is not aware of the
>> > watchdog. Option (3) doesn't disable the watchdog and assumes the OS
>> > will continue servicing.
>> >
>> > Signed-off-by: Michael Walle <michael@walle.cc>
>> > ---
>> >  cmd/boot.c                    |  7 +++++++
>> >  cmd/bootefi.c                 |  7 +++++++
>> >  cmd/efidebug.c                |  7 +++++++
>> >  common/board_r.c              |  2 +-
>> >  common/bootm_os.c             |  5 +++++
>> >  common/spl/spl.c              |  6 +++---
>> >  drivers/watchdog/Kconfig      | 24 ++++++++++++++++++++++++
>> >  drivers/watchdog/wdt-uclass.c | 33 +++++++++++++++++++++++++--------
>> >  include/wdt.h                 | 17 +++++++++++++++++
>> >  9 files changed, 96 insertions(+), 12 deletions(-)
>> >
>> > diff --git a/cmd/boot.c b/cmd/boot.c
>> > index 36aba22b30..2412410371 100644
>> > --- a/cmd/boot.c
>> > +++ b/cmd/boot.c
>> > @@ -10,6 +10,7 @@
>> >  #include <common.h>
>> >  #include <command.h>
>> >  #include <net.h>
>> > +#include <wdt.h>
>> >
>> >  #ifdef CONFIG_CMD_GO
>> >
>> > @@ -33,6 +34,9 @@ static int do_go(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>> >
>> >  	printf ("## Starting application at 0x%08lX ...\n", addr);
>> >
>> > +	if (IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT))
>> > +		stop_watchdog();
>> > +
>> >  	/*
>> >  	 * pass address parameter as argv[0] (aka command name),
>> >  	 * and all remaining args
>> > @@ -40,6 +44,9 @@ static int do_go(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>> >  	rc = do_go_exec ((void *)addr, argc - 1, argv + 1);
>> >  	if (rc != 0) rcode = 1;
>> >
>> > +	if (IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT))
>> > +		start_watchdog();
>> > +
>> >  	printf ("## Application terminated, rc = 0x%lX\n", rc);
>> >  	return rcode;
>> >  }
>> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>> > index 40d5ef2b3a..21f6650174 100644
>> > --- a/cmd/bootefi.c
>> > +++ b/cmd/bootefi.c
>> > @@ -24,6 +24,7 @@
>> >  #include <memalign.h>
>> >  #include <asm-generic/sections.h>
>> >  #include <linux/linkage.h>
>> > +#include <wdt.h>
>> >
>> >  DECLARE_GLOBAL_DATA_PTR;
>> >
>> > @@ -320,6 +321,9 @@ static efi_status_t do_bootefi_exec(efi_handle_t handle, void *load_options)
>> >  	efi_uintn_t exit_data_size = 0;
>> >  	u16 *exit_data = NULL;
>> >
>> > +	if (IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT))
>> > +		stop_watchdog();
>> > +
>> >  	/* Call our payload! */
>> >  	ret = EFI_CALL(efi_start_image(handle, &exit_data_size, &exit_data));
>> >  	if (ret != EFI_SUCCESS) {
>> > @@ -333,6 +337,9 @@ static efi_status_t do_bootefi_exec(efi_handle_t handle, void *load_options)
>> >
>> >  	efi_restore_gd();
>> >
>> > +	if (IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT))
>> > +		start_watchdog();
>> > +
>> >  	free(load_options);
>> >
>> >  	return ret;
>> > diff --git a/cmd/efidebug.c b/cmd/efidebug.c
>> > index 9874838b00..5fa0cd1df7 100644
>> > --- a/cmd/efidebug.c
>> > +++ b/cmd/efidebug.c
>> > @@ -16,6 +16,7 @@
>> >  #include <mapmem.h>
>> >  #include <search.h>
>> >  #include <linux/ctype.h>
>> > +#include <wdt.h>
>> >
>> >  #define BS systab.boottime
>> >
>> > @@ -1131,6 +1132,9 @@ static int do_efi_test_bootmgr(struct cmd_tbl *cmdtp, int flag,
>> >  	ret = efi_bootmgr_load(&image, &load_options);
>> >  	printf("efi_bootmgr_load() returned: %ld\n", ret & ~EFI_ERROR_MASK);
>> >
>> > +	if (IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT))
>> > +		stop_watchdog();
>> > +
>> >  	/* We call efi_start_image() even if error for test purpose. */
>> >  	ret = EFI_CALL(efi_start_image(image, &exit_data_size, &exit_data));
>> >  	printf("efi_start_image() returned: %ld\n", ret & ~EFI_ERROR_MASK);
>> > @@ -1139,6 +1143,9 @@ static int do_efi_test_bootmgr(struct cmd_tbl *cmdtp, int flag,
>> >
>> >  	efi_restore_gd();
>> >
>> > +	if (IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT))
>> > +		start_watchdog();
>> > +
>> >  	free(load_options);
>> >  	return CMD_RET_SUCCESS;
>> >  }
>> > diff --git a/common/board_r.c b/common/board_r.c
>> > index 9b2fec701a..6734d3ad25 100644
>> > --- a/common/board_r.c
>> > +++ b/common/board_r.c
>> > @@ -731,7 +731,7 @@ static init_fnc_t init_sequence_r[] = {
>> >  	stdio_init_tables,
>> >  	serial_initialize,
>> >  	initr_announce,
>> > -#if CONFIG_IS_ENABLED(WDT)
>> > +#if !IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_NOTHING)
>> >  	initr_watchdog,
>> 
>> This does not compile on odroid_c2_defconfig.
>> 
>> aarch64-linux-gnu-ld.bfd:
>> common/built-in.o:(.data.init_sequence_r+0x88): undefined reference to
>> `initr_watchdog'
>> make: *** [Makefile:1753: u-boot] Error 1
> 
> I think a pre-req for this will be to have re-run the migration of
> CONFIG_WATCHDOG / CONFIG_WDT from config headers and in to defconfigs.
> I'm not 100% sure that's what's causing the breakage here (migration
> isn't done yet) but I could see that being a problem.

No, I actually messed up my patch in the last minute (I did a kernel-ci
run in parallel, though).

#if !IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_NOTHING)
	initr_watchdog,
#endif

must be

#if IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_U_BOOT) || 
IS_ENABLED(CONFIG_WATCHDOG_SUPERVISE_OS)
	initr_watchdog,
#endif

-michael

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Michael Walle]

Am 2020-09-23 19:35, schrieb Tom Rini:
> On Wed, Sep 23, 2020 at 07:31:00PM +0200, Heinrich Schuchardt wrote:
>> On 9/23/20 7:14 PM, Tom Rini wrote:
>> > On Wed, Sep 23, 2020 at 07:01:54PM +0200, Mark Kettenis wrote:
>> >>> From: Michael Walle <michael@walle.cc>
>> >>> Date: Wed, 23 Sep 2020 18:45:27 +0200
>> >>>
>> >>> Let the user choose between three different behaviours of the watchdog:
>> >>>  (1) Keep the watchdog disabled
>> >>>  (2) Supervise u-boot
>> >>>  (3) Supervise u-boot and the operating systen (default)
>> >>>
>> >>> Option (2) will disable the watchdog right before handing control to the
>> >>> operating system. This is useful when the OS is not aware of the
>> >>> watchdog. Option (3) doesn't disable the watchdog and assumes the OS
>> >>> will continue servicing.
>> >>
>> >> (3) can't be the default, at least for EFI
>> >>
>> >> The UEFI standard explicitly says that upon calling
>> >> ExitBootServices(), the watchdog timer is disabled.
>> >>
>> >> In general, you can't expect an OS to have support for a particular
>> >> watchdog timer.  So (3) only makes sense in cases where U-Boot is
>> >> bundled with an OS image.
>> >
>> > We need to be careful here then.  The current and historical / generally
>> > expected behavior is if we've enabled the watchdog we supervise it and
>> > leave it enabled for the OS.  Given what UEFI requires I'd like to see
>> > that case handled with a print about disabling the watchdog so it's not

I agree with "current and historical behavior" but not with "expected
behavior".

I was thinking about something like

+choice
+       prompt "Watchdog behavior"
+       default WATCHDOG_SUPERVISE_U_BOOT if EFI_LOADER
+       default WATCHDOG_SUPERVISE_OS if !EFI_LOADER
+       depends on WDT

Unfortunately, EFI_LOADER is default y for any architecture != ARM.
Therefore, it is likely we are changing the behavior of some boards
and I agree this isn't what we want.

>> Not printf(), maybe log_info().
>> 
>> The disabling has to occur in ExitBootServices() (aka.
>> efi_exit_boot_services()). Here we are in the middle of an executing
>> UEFI application. Printing anything on the screen may mess up the 
>> output
>> of the UEFI application.
>> 
>> So, please, don't output anything.
> 
> We need to find a good way to inform the user we're disabling their
> watchdog.  Maybe before we fully jump in to UEFI note that it will be
> disabled before entering the OS?  Or something a bit more generally
> understood than ExitBootServices() having been called.  I don't know
> _where_ the best place is, but I think it's important to inform the
> user.

The watchdog is only disabled in the "supervise u-boot" mode, why
would we need to inform the user? It was the users choice to have
the timer only enabled in u-boot.

Or do you mean if for example the vendor chooses that option and
in this case the user doesn't know anything about it? The mode
is indicated in the "WDT:" output.

-michael

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Mark Kettenis]

> Date: Thu, 24 Sep 2020 09:33:50 +0200
> From: Michael Walle <michael@walle.cc>
> 
> Am 2020-09-23 19:35, schrieb Tom Rini:
> > On Wed, Sep 23, 2020 at 07:31:00PM +0200, Heinrich Schuchardt wrote:
> >> On 9/23/20 7:14 PM, Tom Rini wrote:
> >> > On Wed, Sep 23, 2020 at 07:01:54PM +0200, Mark Kettenis wrote:
> >> >>> From: Michael Walle <michael@walle.cc>
> >> >>> Date: Wed, 23 Sep 2020 18:45:27 +0200
> >> >>>
> >> >>> Let the user choose between three different behaviours of the watchdog:
> >> >>>  (1) Keep the watchdog disabled
> >> >>>  (2) Supervise u-boot
> >> >>>  (3) Supervise u-boot and the operating systen (default)
> >> >>>
> >> >>> Option (2) will disable the watchdog right before handing control to the
> >> >>> operating system. This is useful when the OS is not aware of the
> >> >>> watchdog. Option (3) doesn't disable the watchdog and assumes the OS
> >> >>> will continue servicing.
> >> >>
> >> >> (3) can't be the default, at least for EFI
> >> >>
> >> >> The UEFI standard explicitly says that upon calling
> >> >> ExitBootServices(), the watchdog timer is disabled.
> >> >>
> >> >> In general, you can't expect an OS to have support for a particular
> >> >> watchdog timer.  So (3) only makes sense in cases where U-Boot is
> >> >> bundled with an OS image.
> >> >
> >> > We need to be careful here then.  The current and historical / generally
> >> > expected behavior is if we've enabled the watchdog we supervise it and
> >> > leave it enabled for the OS.  Given what UEFI requires I'd like to see
> >> > that case handled with a print about disabling the watchdog so it's not
> 
> I agree with "current and historical behavior" but not with "expected
> behavior".
> 
> I was thinking about something like
> 
> +choice
> +       prompt "Watchdog behavior"
> +       default WATCHDOG_SUPERVISE_U_BOOT if EFI_LOADER
> +       default WATCHDOG_SUPERVISE_OS if !EFI_LOADER
> +       depends on WDT
> 
> Unfortunately, EFI_LOADER is default y for any architecture != ARM.
> Therefore, it is likely we are changing the behavior of some boards
> and I agree this isn't what we want.

I think you are misreading that.  The following stanza:

        depends on OF_LIBFDT && ( \
                ARM && (SYS_CPU = arm1136 || \
                        SYS_CPU = arm1176 || \
                        SYS_CPU = armv7   || \
                        SYS_CPU = armv8)  || \
                X86 || RISCV || SANDBOX)

means that EFI_LOADER is onlu ever defined on (newish) ARM, X86, RISCV
and SANDBOX.  Which makes sense since there is no EFI calling
convention defined for other architectures like MIPS and PPC.

> >> Not printf(), maybe log_info().
> >> 
> >> The disabling has to occur in ExitBootServices() (aka.
> >> efi_exit_boot_services()). Here we are in the middle of an executing
> >> UEFI application. Printing anything on the screen may mess up the 
> >> output
> >> of the UEFI application.
> >> 
> >> So, please, don't output anything.
> > 
> > We need to find a good way to inform the user we're disabling their
> > watchdog.  Maybe before we fully jump in to UEFI note that it will be
> > disabled before entering the OS?  Or something a bit more generally
> > understood than ExitBootServices() having been called.  I don't know
> > _where_ the best place is, but I think it's important to inform the
> > user.
> 
> The watchdog is only disabled in the "supervise u-boot" mode, why
> would we need to inform the user? It was the users choice to have
> the timer only enabled in u-boot.
> 
> Or do you mean if for example the vendor chooses that option and
> in this case the user doesn't know anything about it? The mode
> is indicated in the "WDT:" output.
> 
> -michael
> 

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Michael Walle]

Am 2020-09-24 10:10, schrieb Mark Kettenis:
>> Date: Thu, 24 Sep 2020 09:33:50 +0200
>> From: Michael Walle <michael@walle.cc>
>> 
>> Am 2020-09-23 19:35, schrieb Tom Rini:
>> > On Wed, Sep 23, 2020 at 07:31:00PM +0200, Heinrich Schuchardt wrote:
>> >> On 9/23/20 7:14 PM, Tom Rini wrote:
>> >> > On Wed, Sep 23, 2020 at 07:01:54PM +0200, Mark Kettenis wrote:
>> >> >>> From: Michael Walle <michael@walle.cc>
>> >> >>> Date: Wed, 23 Sep 2020 18:45:27 +0200
>> >> >>>
>> >> >>> Let the user choose between three different behaviours of the watchdog:
>> >> >>>  (1) Keep the watchdog disabled
>> >> >>>  (2) Supervise u-boot
>> >> >>>  (3) Supervise u-boot and the operating systen (default)
>> >> >>>
>> >> >>> Option (2) will disable the watchdog right before handing control to the
>> >> >>> operating system. This is useful when the OS is not aware of the
>> >> >>> watchdog. Option (3) doesn't disable the watchdog and assumes the OS
>> >> >>> will continue servicing.
>> >> >>
>> >> >> (3) can't be the default, at least for EFI
>> >> >>
>> >> >> The UEFI standard explicitly says that upon calling
>> >> >> ExitBootServices(), the watchdog timer is disabled.
>> >> >>
>> >> >> In general, you can't expect an OS to have support for a particular
>> >> >> watchdog timer.  So (3) only makes sense in cases where U-Boot is
>> >> >> bundled with an OS image.
>> >> >
>> >> > We need to be careful here then.  The current and historical / generally
>> >> > expected behavior is if we've enabled the watchdog we supervise it and
>> >> > leave it enabled for the OS.  Given what UEFI requires I'd like to see
>> >> > that case handled with a print about disabling the watchdog so it's not
>> 
>> I agree with "current and historical behavior" but not with "expected
>> behavior".
>> 
>> I was thinking about something like
>> 
>> +choice
>> +       prompt "Watchdog behavior"
>> +       default WATCHDOG_SUPERVISE_U_BOOT if EFI_LOADER
>> +       default WATCHDOG_SUPERVISE_OS if !EFI_LOADER
>> +       depends on WDT
>> 
>> Unfortunately, EFI_LOADER is default y for any architecture != ARM.
>> Therefore, it is likely we are changing the behavior of some boards
>> and I agree this isn't what we want.
> 
> I think you are misreading that.  The following stanza:
> 
>         depends on OF_LIBFDT && ( \
>                 ARM && (SYS_CPU = arm1136 || \
>                         SYS_CPU = arm1176 || \
>                         SYS_CPU = armv7   || \
>                         SYS_CPU = armv8)  || \
>                 X86 || RISCV || SANDBOX)
> 
> means that EFI_LOADER is onlu ever defined on (newish) ARM, X86, RISCV
> and SANDBOX.  Which makes sense since there is no EFI calling
> convention defined for other architectures like MIPS and PPC.

I've missed that, but that will just limit the search space.
Like how can we figure out what board has both EFI_LOADER=y and
WDT=y set? If there is no such board, then we can use the
defaults above. If there are such boards we will have to put
CONFIG_SUPERVISE_OS=y in their defconfig.

If EFI_LOADER would have had no default y, then a simple
grep for EFI_LOADER=y (and WDT=y) would have been sufficient.

Are there any handy methods/tools?

-michael

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Mark Kettenis]

> Content-Type: text/plain; charset=US-ASCII;
>  format=flowed
> Date: Thu, 24 Sep 2020 10:20:30 +0200
> From: Michael Walle <michael@walle.cc>
> Cc: trini at konsulko.com, xypron.glpk at gmx.de, u-boot at lists.denx.de,
>         agraf at csgraf.de, marex at denx.de, judge.packham at gmail.com, sr at denx.de,
>         rayagonda.kokatanur at broadcom.com
> 
> Am 2020-09-24 10:10, schrieb Mark Kettenis:
> >> Date: Thu, 24 Sep 2020 09:33:50 +0200
> >> From: Michael Walle <michael@walle.cc>
> >> 
> >> Am 2020-09-23 19:35, schrieb Tom Rini:
> >> > On Wed, Sep 23, 2020 at 07:31:00PM +0200, Heinrich Schuchardt wrote:
> >> >> On 9/23/20 7:14 PM, Tom Rini wrote:
> >> >> > On Wed, Sep 23, 2020 at 07:01:54PM +0200, Mark Kettenis wrote:
> >> >> >>> From: Michael Walle <michael@walle.cc>
> >> >> >>> Date: Wed, 23 Sep 2020 18:45:27 +0200
> >> >> >>>
> >> >> >>> Let the user choose between three different behaviours of the watchdog:
> >> >> >>>  (1) Keep the watchdog disabled
> >> >> >>>  (2) Supervise u-boot
> >> >> >>>  (3) Supervise u-boot and the operating systen (default)
> >> >> >>>
> >> >> >>> Option (2) will disable the watchdog right before handing control to the
> >> >> >>> operating system. This is useful when the OS is not aware of the
> >> >> >>> watchdog. Option (3) doesn't disable the watchdog and assumes the OS
> >> >> >>> will continue servicing.
> >> >> >>
> >> >> >> (3) can't be the default, at least for EFI
> >> >> >>
> >> >> >> The UEFI standard explicitly says that upon calling
> >> >> >> ExitBootServices(), the watchdog timer is disabled.
> >> >> >>
> >> >> >> In general, you can't expect an OS to have support for a particular
> >> >> >> watchdog timer.  So (3) only makes sense in cases where U-Boot is
> >> >> >> bundled with an OS image.
> >> >> >
> >> >> > We need to be careful here then.  The current and historical / generally
> >> >> > expected behavior is if we've enabled the watchdog we supervise it and
> >> >> > leave it enabled for the OS.  Given what UEFI requires I'd like to see
> >> >> > that case handled with a print about disabling the watchdog so it's not
> >> 
> >> I agree with "current and historical behavior" but not with "expected
> >> behavior".
> >> 
> >> I was thinking about something like
> >> 
> >> +choice
> >> +       prompt "Watchdog behavior"
> >> +       default WATCHDOG_SUPERVISE_U_BOOT if EFI_LOADER
> >> +       default WATCHDOG_SUPERVISE_OS if !EFI_LOADER
> >> +       depends on WDT
> >> 
> >> Unfortunately, EFI_LOADER is default y for any architecture != ARM.
> >> Therefore, it is likely we are changing the behavior of some boards
> >> and I agree this isn't what we want.
> > 
> > I think you are misreading that.  The following stanza:
> > 
> >         depends on OF_LIBFDT && ( \
> >                 ARM && (SYS_CPU = arm1136 || \
> >                         SYS_CPU = arm1176 || \
> >                         SYS_CPU = armv7   || \
> >                         SYS_CPU = armv8)  || \
> >                 X86 || RISCV || SANDBOX)
> > 
> > means that EFI_LOADER is onlu ever defined on (newish) ARM, X86, RISCV
> > and SANDBOX.  Which makes sense since there is no EFI calling
> > convention defined for other architectures like MIPS and PPC.
> 
> I've missed that, but that will just limit the search space.
> Like how can we figure out what board has both EFI_LOADER=y and
> WDT=y set? If there is no such board, then we can use the
> defaults above. If there are such boards we will have to put
> CONFIG_SUPERVISE_OS=y in their defconfig.
> 
> If EFI_LOADER would have had no default y, then a simple
> grep for EFI_LOADER=y (and WDT=y) would have been sufficient.

And this is where the conflict of interest surfaces.

From a "running a generic OS" standpoint of view we really want to
have EFI_LOADER enabled on as many ARM/X86/RISCV boards as possible
and want to discourage the sometimes heavy customization that some of
the board vendors have done historically.  Such customizations often
break booting a generic OS or at least create inconsistencies tat are
confusing to users of such a generic OS. And WDT=y pretty is such a
customization.

At the same time there obviously is the desire from some vendors to
integrate U-Boot with an OS (which in practice probably always is a
customized Linux kernel).  This scenario typically uses "bootm"
instead of the EFI loader and I believe that only in this context the
historic watchdog behaviour makes sense.

Therefore I think it makes sense to always disable any running
hardware watchdog timer when starting an EFI payload, and reenable it
if that payload returns to the bootloader.  I don't think printing a
message when doing so makes sense as users of the EFI loader expect
any watchdog timer to be disabled, but printing a message that a
hardware watchdog is being disabled before starting the EFI payload
may be acceptable.

Please note that very few ARM board configurations actually set WDT=y,
so generic OS users may simply not have noticed issues with the
current "policy" of leaving a hardware watchdog running when booting
a generic OS.

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Tom Rini]

On Thu, Sep 24, 2020 at 09:33:50AM +0200, Michael Walle wrote:
> Am 2020-09-23 19:35, schrieb Tom Rini:
> > On Wed, Sep 23, 2020 at 07:31:00PM +0200, Heinrich Schuchardt wrote:
> > > On 9/23/20 7:14 PM, Tom Rini wrote:
> > > > On Wed, Sep 23, 2020 at 07:01:54PM +0200, Mark Kettenis wrote:
> > > >>> From: Michael Walle <michael@walle.cc>
> > > >>> Date: Wed, 23 Sep 2020 18:45:27 +0200
> > > >>>
> > > >>> Let the user choose between three different behaviours of the watchdog:
> > > >>>  (1) Keep the watchdog disabled
> > > >>>  (2) Supervise u-boot
> > > >>>  (3) Supervise u-boot and the operating systen (default)
> > > >>>
> > > >>> Option (2) will disable the watchdog right before handing control to the
> > > >>> operating system. This is useful when the OS is not aware of the
> > > >>> watchdog. Option (3) doesn't disable the watchdog and assumes the OS
> > > >>> will continue servicing.
> > > >>
> > > >> (3) can't be the default, at least for EFI
> > > >>
> > > >> The UEFI standard explicitly says that upon calling
> > > >> ExitBootServices(), the watchdog timer is disabled.
> > > >>
> > > >> In general, you can't expect an OS to have support for a particular
> > > >> watchdog timer.  So (3) only makes sense in cases where U-Boot is
> > > >> bundled with an OS image.
> > > >
> > > > We need to be careful here then.  The current and historical / generally
> > > > expected behavior is if we've enabled the watchdog we supervise it and
> > > > leave it enabled for the OS.  Given what UEFI requires I'd like to see
> > > > that case handled with a print about disabling the watchdog so it's not
> 
> I agree with "current and historical behavior" but not with "expected
> behavior".
> 
> I was thinking about something like
> 
> +choice
> +       prompt "Watchdog behavior"
> +       default WATCHDOG_SUPERVISE_U_BOOT if EFI_LOADER
> +       default WATCHDOG_SUPERVISE_OS if !EFI_LOADER
> +       depends on WDT
> 
> Unfortunately, EFI_LOADER is default y for any architecture != ARM.
> Therefore, it is likely we are changing the behavior of some boards
> and I agree this isn't what we want.
> 
> > > Not printf(), maybe log_info().
> > > 
> > > The disabling has to occur in ExitBootServices() (aka.
> > > efi_exit_boot_services()). Here we are in the middle of an executing
> > > UEFI application. Printing anything on the screen may mess up the
> > > output
> > > of the UEFI application.
> > > 
> > > So, please, don't output anything.
> > 
> > We need to find a good way to inform the user we're disabling their
> > watchdog.  Maybe before we fully jump in to UEFI note that it will be
> > disabled before entering the OS?  Or something a bit more generally
> > understood than ExitBootServices() having been called.  I don't know
> > _where_ the best place is, but I think it's important to inform the
> > user.
> 
> The watchdog is only disabled in the "supervise u-boot" mode, why
> would we need to inform the user? It was the users choice to have
> the timer only enabled in u-boot.
> 
> Or do you mean if for example the vendor chooses that option and
> in this case the user doesn't know anything about it? The mode
> is indicated in the "WDT:" output.

I'm talking about the case where we say we've enabled the WDT to
supervise OS, but then bootefi something and have disabled the watchdog
(to meet UEFI requirements) but didn't tell the user we've turned off
the WDT that we had told them is on.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200924/bc21fb77/attachment.sig>

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Michael Walle]

Am 2020-09-24 15:19, schrieb Tom Rini:
> On Thu, Sep 24, 2020 at 09:33:50AM +0200, Michael Walle wrote:
>> Am 2020-09-23 19:35, schrieb Tom Rini:
[..]

>> > > Not printf(), maybe log_info().
>> > >
>> > > The disabling has to occur in ExitBootServices() (aka.
>> > > efi_exit_boot_services()). Here we are in the middle of an executing
>> > > UEFI application. Printing anything on the screen may mess up the
>> > > output
>> > > of the UEFI application.
>> > >
>> > > So, please, don't output anything.
>> >
>> > We need to find a good way to inform the user we're disabling their
>> > watchdog.  Maybe before we fully jump in to UEFI note that it will be
>> > disabled before entering the OS?  Or something a bit more generally
>> > understood than ExitBootServices() having been called.  I don't know
>> > _where_ the best place is, but I think it's important to inform the
>> > user.
>> 
>> The watchdog is only disabled in the "supervise u-boot" mode, why
>> would we need to inform the user? It was the users choice to have
>> the timer only enabled in u-boot.
>> 
>> Or do you mean if for example the vendor chooses that option and
>> in this case the user doesn't know anything about it? The mode
>> is indicated in the "WDT:" output.
> 
> I'm talking about the case where we say we've enabled the WDT to
> supervise OS, but then bootefi something and have disabled the watchdog
> (to meet UEFI requirements) but didn't tell the user we've turned off
> the WDT that we had told them is on.

Ah, do you really want to have a different behavior between bootm and
bootefi? Thats even more suprising IMHO.

I had the following in mind:

+config WATCHDOG_SUPERVISE_OS
+       bool "Supervise U-boot and operating system"
+       help
+         Upon U-Boot startup the first watchdog will be started 
automatically
+         and kept running even after booting the operating system.
+         Be aware, that the operating system needs to service the 
watchdog!
+
+         Additionally, this is not UEFI compliant because:
+          - the timeout won't be set to 5 minutes before starting the 
OS and
+          - the watchdog timer isn't stopped after the OS calls
+            ExitBootServices().

-michael

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Mark Kettenis]

> Content-Type: text/plain; charset=US-ASCII;
>  format=flowed
> Date: Thu, 24 Sep 2020 22:30:24 +0200
> From: Michael Walle <michael@walle.cc>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>,
>         Mark Kettenis
>  <mark.kettenis@xs4all.nl>, u-boot at lists.denx.de,
>         agraf at csgraf.de, marex at denx.de, judge.packham at gmail.com, sr at denx.de,
>         rayagonda.kokatanur at broadcom.com
> 
> Am 2020-09-24 15:19, schrieb Tom Rini:
> > On Thu, Sep 24, 2020 at 09:33:50AM +0200, Michael Walle wrote:
> >> Am 2020-09-23 19:35, schrieb Tom Rini:
> [..]
> 
> >> > > Not printf(), maybe log_info().
> >> > >
> >> > > The disabling has to occur in ExitBootServices() (aka.
> >> > > efi_exit_boot_services()). Here we are in the middle of an executing
> >> > > UEFI application. Printing anything on the screen may mess up the
> >> > > output
> >> > > of the UEFI application.
> >> > >
> >> > > So, please, don't output anything.
> >> >
> >> > We need to find a good way to inform the user we're disabling their
> >> > watchdog.  Maybe before we fully jump in to UEFI note that it will be
> >> > disabled before entering the OS?  Or something a bit more generally
> >> > understood than ExitBootServices() having been called.  I don't know
> >> > _where_ the best place is, but I think it's important to inform the
> >> > user.
> >> 
> >> The watchdog is only disabled in the "supervise u-boot" mode, why
> >> would we need to inform the user? It was the users choice to have
> >> the timer only enabled in u-boot.
> >> 
> >> Or do you mean if for example the vendor chooses that option and
> >> in this case the user doesn't know anything about it? The mode
> >> is indicated in the "WDT:" output.
> > 
> > I'm talking about the case where we say we've enabled the WDT to
> > supervise OS, but then bootefi something and have disabled the watchdog
> > (to meet UEFI requirements) but didn't tell the user we've turned off
> > the WDT that we had told them is on.
> 
> Ah, do you really want to have a different behavior between bootm and
> bootefi? Thats even more suprising IMHO.

I fear it is the only way to support both user communities.

> I had the following in mind:
> 
> +config WATCHDOG_SUPERVISE_OS
> +       bool "Supervise U-boot and operating system"
> +       help
> +         Upon U-Boot startup the first watchdog will be started 
> automatically
> +         and kept running even after booting the operating system.
> +         Be aware, that the operating system needs to service the 
> watchdog!
> +
> +         Additionally, this is not UEFI compliant because:
> +          - the timeout won't be set to 5 minutes before starting the 
> OS and
> +          - the watchdog timer isn't stopped after the OS calls
> +            ExitBootServices().

That would be highly problematic:

* It is unfeasable for an EFI OS bootloader to include drivers for the
  hardware watchdog.  This means that the time available for the user
  to interact with the bootloader is limited by the timeout of the
  hardware watchdog.  Consider for example the case of full disk
  encryption where the user is supposed to enter a password before the
  OS can be loaded.  The hardware watchdog would limit the amount of
  time available to enter that password.  If the timeout is seconds
  rather than minutes the system becomes unusable.

* This requires the OS to have support for the hardware watchdog.
  Consider the case where a U-Boot with a default configuration has
  been programmed in SPI flash.  Without the hardware watchdog running
  you can install an arbitrary OS using an EFI bootable image.  With
  the hardware watchdog running an OS lacking support for the hardware
  watchdog would probably reboot in the middle of an install.  Even
  when the kernel that will be ultimately installed supports the
  hardware watchdog, the kernel used by the OS installer may not.  For
  example a Linux kernel where the watchdog timer driver is built as a
  module and that module isn't present on the install image.

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Michael Walle]

Am 2020-09-24 12:22, schrieb Mark Kettenis:
>> Am 2020-09-24 10:10, schrieb Mark Kettenis:
>> >> Date: Thu, 24 Sep 2020 09:33:50 +0200
>> >> From: Michael Walle <michael@walle.cc>
>> >>
>> >> Am 2020-09-23 19:35, schrieb Tom Rini:
>> >> > On Wed, Sep 23, 2020 at 07:31:00PM +0200, Heinrich Schuchardt wrote:
>> >> >> On 9/23/20 7:14 PM, Tom Rini wrote:
>> >> >> > On Wed, Sep 23, 2020 at 07:01:54PM +0200, Mark Kettenis wrote:
>> >> >> >>> From: Michael Walle <michael@walle.cc>
>> >> >> >>> Date: Wed, 23 Sep 2020 18:45:27 +0200
>> >> >> >>>
>> >> >> >>> Let the user choose between three different behaviours of the watchdog:
>> >> >> >>>  (1) Keep the watchdog disabled
>> >> >> >>>  (2) Supervise u-boot
>> >> >> >>>  (3) Supervise u-boot and the operating systen (default)
>> >> >> >>>
>> >> >> >>> Option (2) will disable the watchdog right before handing control to the
>> >> >> >>> operating system. This is useful when the OS is not aware of the
>> >> >> >>> watchdog. Option (3) doesn't disable the watchdog and assumes the OS
>> >> >> >>> will continue servicing.
>> >> >> >>
>> >> >> >> (3) can't be the default, at least for EFI
>> >> >> >>
>> >> >> >> The UEFI standard explicitly says that upon calling
>> >> >> >> ExitBootServices(), the watchdog timer is disabled.
>> >> >> >>
>> >> >> >> In general, you can't expect an OS to have support for a particular
>> >> >> >> watchdog timer.  So (3) only makes sense in cases where U-Boot is
>> >> >> >> bundled with an OS image.
>> >> >> >
>> >> >> > We need to be careful here then.  The current and historical / generally
>> >> >> > expected behavior is if we've enabled the watchdog we supervise it and
>> >> >> > leave it enabled for the OS.  Given what UEFI requires I'd like to see
>> >> >> > that case handled with a print about disabling the watchdog so it's not
>> >>
>> >> I agree with "current and historical behavior" but not with "expected
>> >> behavior".
>> >>
>> >> I was thinking about something like
>> >>
>> >> +choice
>> >> +       prompt "Watchdog behavior"
>> >> +       default WATCHDOG_SUPERVISE_U_BOOT if EFI_LOADER
>> >> +       default WATCHDOG_SUPERVISE_OS if !EFI_LOADER
>> >> +       depends on WDT
>> >>
>> >> Unfortunately, EFI_LOADER is default y for any architecture != ARM.
>> >> Therefore, it is likely we are changing the behavior of some boards
>> >> and I agree this isn't what we want.
>> >
>> > I think you are misreading that.  The following stanza:
>> >
>> >         depends on OF_LIBFDT && ( \
>> >                 ARM && (SYS_CPU = arm1136 || \
>> >                         SYS_CPU = arm1176 || \
>> >                         SYS_CPU = armv7   || \
>> >                         SYS_CPU = armv8)  || \
>> >                 X86 || RISCV || SANDBOX)
>> >
>> > means that EFI_LOADER is onlu ever defined on (newish) ARM, X86, RISCV
>> > and SANDBOX.  Which makes sense since there is no EFI calling
>> > convention defined for other architectures like MIPS and PPC.
>> 
>> I've missed that, but that will just limit the search space.
>> Like how can we figure out what board has both EFI_LOADER=y and
>> WDT=y set? If there is no such board, then we can use the
>> defaults above. If there are such boards we will have to put
>> CONFIG_SUPERVISE_OS=y in their defconfig.
>> 
>> If EFI_LOADER would have had no default y, then a simple
>> grep for EFI_LOADER=y (and WDT=y) would have been sufficient.
> 
> And this is where the conflict of interest surfaces.
> 
> From a "running a generic OS" standpoint of view we really want to
> have EFI_LOADER enabled on as many ARM/X86/RISCV boards as possible
> and want to discourage the sometimes heavy customization that some of
> the board vendors have done historically.  Such customizations often
> break booting a generic OS or at least create inconsistencies tat are
> confusing to users of such a generic OS. And WDT=y pretty is such a
> customization.
> 
> At the same time there obviously is the desire from some vendors to
> integrate U-Boot with an OS (which in practice probably always is a
> customized Linux kernel).  This scenario typically uses "bootm"
> instead of the EFI loader and I believe that only in this context the
> historic watchdog behaviour makes sense.
> 
> Therefore I think it makes sense to always disable any running
> hardware watchdog timer when starting an EFI payload, and reenable it
> if that payload returns to the bootloader.  I don't think printing a
> message when doing so makes sense as users of the EFI loader expect
> any watchdog timer to be disabled, but printing a message that a
> hardware watchdog is being disabled before starting the EFI payload
> may be acceptable.
> 
> Please note that very few ARM board configurations actually set WDT=y,
> so generic OS users may simply not have noticed issues with the
> current "policy" of leaving a hardware watchdog running when booting
> a generic OS.

Mh, so I did the following:
  - applied Tom's "Convert CONFIG_WATCHDOG et al to Kconfig"
  - for c in configs/*_defconfig; do
      BOARD=$(echo $c | sed -e 's#configs/\(.*\)_defconfig#\1#')
      make ${BOARD}_defconfig
      mv .config ${BOARD}.config
    done
    grep -l CONFIG_WDT=y $(grep -l CONFIG_EFI_LOADER=y *.config)

This will list 50 boards, which has both CONFIG_WDT and
CONFIG_EFI_LOADER set.

-michael

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Michael Walle]

Am 2020-09-24 22:58, schrieb Mark Kettenis:
>> Am 2020-09-24 15:19, schrieb Tom Rini:
>> > On Thu, Sep 24, 2020 at 09:33:50AM +0200, Michael Walle wrote:
>> >> Am 2020-09-23 19:35, schrieb Tom Rini:
>> [..]
>> 
>> >> > > Not printf(), maybe log_info().
>> >> > >
>> >> > > The disabling has to occur in ExitBootServices() (aka.
>> >> > > efi_exit_boot_services()). Here we are in the middle of an executing
>> >> > > UEFI application. Printing anything on the screen may mess up the
>> >> > > output
>> >> > > of the UEFI application.
>> >> > >
>> >> > > So, please, don't output anything.
>> >> >
>> >> > We need to find a good way to inform the user we're disabling their
>> >> > watchdog.  Maybe before we fully jump in to UEFI note that it will be
>> >> > disabled before entering the OS?  Or something a bit more generally
>> >> > understood than ExitBootServices() having been called.  I don't know
>> >> > _where_ the best place is, but I think it's important to inform the
>> >> > user.
>> >>
>> >> The watchdog is only disabled in the "supervise u-boot" mode, why
>> >> would we need to inform the user? It was the users choice to have
>> >> the timer only enabled in u-boot.
>> >>
>> >> Or do you mean if for example the vendor chooses that option and
>> >> in this case the user doesn't know anything about it? The mode
>> >> is indicated in the "WDT:" output.
>> >
>> > I'm talking about the case where we say we've enabled the WDT to
>> > supervise OS, but then bootefi something and have disabled the watchdog
>> > (to meet UEFI requirements) but didn't tell the user we've turned off
>> > the WDT that we had told them is on.
>> 
>> Ah, do you really want to have a different behavior between bootm and
>> bootefi? Thats even more suprising IMHO.
> 
> I fear it is the only way to support both user communities.
> 
>> I had the following in mind:
>> 
>> +config WATCHDOG_SUPERVISE_OS
>> +       bool "Supervise U-boot and operating system"
>> +       help
>> +         Upon U-Boot startup the first watchdog will be started
>> automatically
>> +         and kept running even after booting the operating system.
>> +         Be aware, that the operating system needs to service the
>> watchdog!
>> +
>> +         Additionally, this is not UEFI compliant because:
>> +          - the timeout won't be set to 5 minutes before starting the
>> OS and
>> +          - the watchdog timer isn't stopped after the OS calls
>> +            ExitBootServices().
> 
> That would be highly problematic:
> 
> * It is unfeasable for an EFI OS bootloader to include drivers for the
>   hardware watchdog.  This means that the time available for the user
>   to interact with the bootloader is limited by the timeout of the
>   hardware watchdog.  Consider for example the case of full disk
>   encryption where the user is supposed to enter a password before the
>   OS can be loaded.  The hardware watchdog would limit the amount of
>   time available to enter that password.  If the timeout is seconds
>   rather than minutes the system becomes unusable.
> 
> * This requires the OS to have support for the hardware watchdog.
>   Consider the case where a U-Boot with a default configuration has
>   been programmed in SPI flash.  Without the hardware watchdog running
>   you can install an arbitrary OS using an EFI bootable image.  With
>   the hardware watchdog running an OS lacking support for the hardware
>   watchdog would probably reboot in the middle of an install.  Even
>   when the kernel that will be ultimately installed supports the
>   hardware watchdog, the kernel used by the OS installer may not.  For
>   example a Linux kernel where the watchdog timer driver is built as a
>   module and that module isn't present on the install image.

You don't have to convince me, that having the HW watchdog enabled is
bad in the EFI case. Having a debian installer without watchdog support
was the reason I've started the old discussion thread and this patch.

Are there any objections to disable the HW watchdog unconditionally
and printing a notice before we start an EFI image and possibly
changing behavior for existing boards (if someone is actually using
bootefi)? Tom? Heinrich?

-michael

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Tom Rini]

On Thu, Sep 24, 2020 at 11:14:36PM +0200, Michael Walle wrote:
> Am 2020-09-24 22:58, schrieb Mark Kettenis:
> > > Am 2020-09-24 15:19, schrieb Tom Rini:
> > > > On Thu, Sep 24, 2020 at 09:33:50AM +0200, Michael Walle wrote:
> > > >> Am 2020-09-23 19:35, schrieb Tom Rini:
> > > [..]
> > > 
> > > >> > > Not printf(), maybe log_info().
> > > >> > >
> > > >> > > The disabling has to occur in ExitBootServices() (aka.
> > > >> > > efi_exit_boot_services()). Here we are in the middle of an executing
> > > >> > > UEFI application. Printing anything on the screen may mess up the
> > > >> > > output
> > > >> > > of the UEFI application.
> > > >> > >
> > > >> > > So, please, don't output anything.
> > > >> >
> > > >> > We need to find a good way to inform the user we're disabling their
> > > >> > watchdog.  Maybe before we fully jump in to UEFI note that it will be
> > > >> > disabled before entering the OS?  Or something a bit more generally
> > > >> > understood than ExitBootServices() having been called.  I don't know
> > > >> > _where_ the best place is, but I think it's important to inform the
> > > >> > user.
> > > >>
> > > >> The watchdog is only disabled in the "supervise u-boot" mode, why
> > > >> would we need to inform the user? It was the users choice to have
> > > >> the timer only enabled in u-boot.
> > > >>
> > > >> Or do you mean if for example the vendor chooses that option and
> > > >> in this case the user doesn't know anything about it? The mode
> > > >> is indicated in the "WDT:" output.
> > > >
> > > > I'm talking about the case where we say we've enabled the WDT to
> > > > supervise OS, but then bootefi something and have disabled the watchdog
> > > > (to meet UEFI requirements) but didn't tell the user we've turned off
> > > > the WDT that we had told them is on.
> > > 
> > > Ah, do you really want to have a different behavior between bootm and
> > > bootefi? Thats even more suprising IMHO.
> > 
> > I fear it is the only way to support both user communities.
> > 
> > > I had the following in mind:
> > > 
> > > +config WATCHDOG_SUPERVISE_OS
> > > +       bool "Supervise U-boot and operating system"
> > > +       help
> > > +         Upon U-Boot startup the first watchdog will be started
> > > automatically
> > > +         and kept running even after booting the operating system.
> > > +         Be aware, that the operating system needs to service the
> > > watchdog!
> > > +
> > > +         Additionally, this is not UEFI compliant because:
> > > +          - the timeout won't be set to 5 minutes before starting the
> > > OS and
> > > +          - the watchdog timer isn't stopped after the OS calls
> > > +            ExitBootServices().
> > 
> > That would be highly problematic:
> > 
> > * It is unfeasable for an EFI OS bootloader to include drivers for the
> >   hardware watchdog.  This means that the time available for the user
> >   to interact with the bootloader is limited by the timeout of the
> >   hardware watchdog.  Consider for example the case of full disk
> >   encryption where the user is supposed to enter a password before the
> >   OS can be loaded.  The hardware watchdog would limit the amount of
> >   time available to enter that password.  If the timeout is seconds
> >   rather than minutes the system becomes unusable.
> > 
> > * This requires the OS to have support for the hardware watchdog.
> >   Consider the case where a U-Boot with a default configuration has
> >   been programmed in SPI flash.  Without the hardware watchdog running
> >   you can install an arbitrary OS using an EFI bootable image.  With
> >   the hardware watchdog running an OS lacking support for the hardware
> >   watchdog would probably reboot in the middle of an install.  Even
> >   when the kernel that will be ultimately installed supports the
> >   hardware watchdog, the kernel used by the OS installer may not.  For
> >   example a Linux kernel where the watchdog timer driver is built as a
> >   module and that module isn't present on the install image.
> 
> You don't have to convince me, that having the HW watchdog enabled is
> bad in the EFI case. Having a debian installer without watchdog support
> was the reason I've started the old discussion thread and this patch.
> 
> Are there any objections to disable the HW watchdog unconditionally
> and printing a notice before we start an EFI image and possibly
> changing behavior for existing boards (if someone is actually using
> bootefi)? Tom? Heinrich?

The biggest concern I guess I have at this point is we have a lot of
boards where we don't have CONFIG_WDT set, but we do have a watchdog
driver available and in use and being frequently serviced.  See for
example mx6cuboxi.

To somewhat get back to the original problem, it feels like on the newer
NXP parts, watchdog support was implemented to the current frameworks,
DM, etc, and so this problem is more visibly exposed.  But on the 32bit
i.MX parts it's always there.  And a quick look at current'ish Linux
kernel defconfig, there's always the "IMX2_WDT" driver built in.  So I
don't know what's going on there off-hand.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200924/53f29783/attachment.sig>

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Wolfgang Denk]

Dear Tom,

In message <20200924131931.GF14816@bill-the-cat> you wrote:
> 
> I'm talking about the case where we say we've enabled the WDT to
> supervise OS, but then bootefi something and have disabled the watchdog
> (to meet UEFI requirements) but didn't tell the user we've turned off
> the WDT that we had told them is on.

Any so-called "watchdog" that can be disabled / switched off by
software is not really woth this name.  As such, the concept of
disabling a watchdog in software, is misleading at best and should
never ibe implemented.

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 at denx.de
Few people do business well who do nothing else.
                                       -- Philip Earl of Chesterfield

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Heinrich Schuchardt]

On 25.09.20 10:36, Wolfgang Denk wrote:
> Dear Tom,
>
> In message <20200924131931.GF14816@bill-the-cat> you wrote:
>>
>> I'm talking about the case where we say we've enabled the WDT to
>> supervise OS, but then bootefi something and have disabled the watchdog
>> (to meet UEFI requirements) but didn't tell the user we've turned off
>> the WDT that we had told them is on.
>
> Any so-called "watchdog" that can be disabled / switched off by
> software is not really woth this name.  As such, the concept of
> disabling a watchdog in software, is misleading at best and should
> never ibe implemented.

If we want to boot UEFI payloads, we will have to follow the UEFI
specification even if we think it is not perfect.

Best regards

Heinrich

>
> Best regards,
>
> Wolfgang Denk
>

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Tom Rini]

On Fri, Sep 25, 2020 at 01:29:18PM +0200, Heinrich Schuchardt wrote:
> On 25.09.20 10:36, Wolfgang Denk wrote:
> > Dear Tom,
> >
> > In message <20200924131931.GF14816@bill-the-cat> you wrote:
> >>
> >> I'm talking about the case where we say we've enabled the WDT to
> >> supervise OS, but then bootefi something and have disabled the watchdog
> >> (to meet UEFI requirements) but didn't tell the user we've turned off
> >> the WDT that we had told them is on.
> >
> > Any so-called "watchdog" that can be disabled / switched off by
> > software is not really woth this name.  As such, the concept of
> > disabling a watchdog in software, is misleading at best and should
> > never ibe implemented.
> 
> If we want to boot UEFI payloads, we will have to follow the UEFI
> specification even if we think it is not perfect.

I really really want to know what the UEFI specification says about
hardware watchdogs.  Especially given the push to use a subset of UEFI
for embedded.  Most modern SoCs include a watchdog IP block and it can
be used for a traditional watchdog and it's also used for reset.  Or
simply must be serviced periodically.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200925/31854b6f/attachment.sig>

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Wolfgang Denk]

Dear Heinrich Schuchardt,

In message <ceeb82b7-eef0-7435-0d0c-958c0bf07e04@gmx.de> you wrote:
>
> > Any so-called "watchdog" that can be disabled / switched off by
> > software is not really woth this name.  As such, the concept of
> > disabling a watchdog in software, is misleading at best and should
> > never ibe implemented.
>
> If we want to boot UEFI payloads, we will have to follow the UEFI
> specification even if we think it is not perfect.

That's perfectly OK.  But if the OS expects that the watchdog is
disabled, then we should not enable it in U-Boot in the first place.

Keep in ind that any real watchdog (which is worth the money and the
name) _cannot_ be disabled by software once it was started.

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 at denx.de
Q:  Do you know what the death rate around here is?
A:  One per person.

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Heinrich Schuchardt]

On 25.09.20 15:00, Tom Rini wrote:
> On Fri, Sep 25, 2020 at 01:29:18PM +0200, Heinrich Schuchardt wrote:
>> On 25.09.20 10:36, Wolfgang Denk wrote:
>>> Dear Tom,
>>>
>>> In message <20200924131931.GF14816@bill-the-cat> you wrote:
>>>>
>>>> I'm talking about the case where we say we've enabled the WDT to
>>>> supervise OS, but then bootefi something and have disabled the watchdog
>>>> (to meet UEFI requirements) but didn't tell the user we've turned off
>>>> the WDT that we had told them is on.
>>>
>>> Any so-called "watchdog" that can be disabled / switched off by
>>> software is not really woth this name.  As such, the concept of
>>> disabling a watchdog in software, is misleading at best and should
>>> never ibe implemented.
>>
>> If we want to boot UEFI payloads, we will have to follow the UEFI
>> specification even if we think it is not perfect.
>
> I really really want to know what the UEFI specification says about
> hardware watchdogs.  Especially given the push to use a subset of UEFI
> for embedded.  Most modern SoCs include a watchdog IP block and it can
> be used for a traditional watchdog and it's also used for reset.  Or
> simply must be serviced periodically.
>


The current specification is:

UEFI Specification Version 2.8 (Errata B) (released June 2020)
https://uefi.org/sites/default/files/resources/UEFI%20Spec%202.8B%20May%202020.pdf

Chapter 2.3.7 RISC-V Platforms

p. 40
"The causes of reset could be power-on reset, external hard reset,
brownout detected, watchdog timer elapse, sleep-mode wakeup, etc., which
machine-mode UEFI system firmware has to distinguish."

Chapter 3.1.2 Load Option Processing

p. 70
"If LoadImage() succeeds, the boot manager must enable the watchdog
timer for 5 minutes by using the EFI_BOOT_SERVICES.SetWatchdogTimer()
boot service prior to calling EFI_BOOT_SERVICES.StartImage(). If a boot
option returns control to the boot manager, the boot manager must
disable the watchdog timer with an additional call to the
SetWatchdogTimer() boot service."

Chapter 7.4 EFI_BOOT_SERVICES.ExitBootServices()

p. 222
"On success ... the boot services watchdog timer is disabled."

Chapter 7.5 EFI_BOOT_SERVICES.SetWatchdogTimer()

This chapter describes management of the watchdog timer. You can set any
duration with 1 second resolution. A value of 0 will disable the watchdog.

p. 223
"If the watchdog timer expires, the event is logged by the firmware. The
system may then either reset with the Runtime Service ResetSystem(), or
perform a platform specific action that must eventually cause the
platform to be reset. The watchdog timer is armed before the firmware's
boot manager invokes an EFI boot option. The watchdog must be set to a
period of 5 minutes. The EFI Image may reset or disable the watchdog
timer as needed. If control is returned to the firmware's boot manager,
the watchdog timer must be disabled.The watchdog timer is only used
during boot services. On successful completion of
EFI_BOOT_SERVICES.ExitBootServices() the watchdog timer is disabled."

Appendix R - Glossary

p. 2444
Watchdog Time
An alarm timer that may be set to go off. This can be used to regain
control in cases where a code path in the boot services environment
fails to or is unable to return control by the expected path.

Best regards

Heinrich

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Wolfgang Denk]

Dear Heinrich,

In message <81e467f1-aff3-a54d-5f7e-3b8f5390f410@gmx.de> you wrote:
>
> Chapter 2.3.7 RISC-V Platforms
>
> p. 40
> "The causes of reset could be power-on reset, external hard reset,
> brownout detected, watchdog timer elapse, sleep-mode wakeup, etc., which
> machine-mode UEFI system firmware has to distinguish."

This is possible only if the hardware actually supports such
distinction.  In many cases of actual hardware I've seen a watchdog
reset is just the same as an external hard reset, and brownout
detection does not exist.

 p. 70
> "If LoadImage() succeeds, the boot manager must enable the watchdog
> timer for 5 minutes by using the EFI_BOOT_SERVICES.SetWatchdogTimer()
> boot service prior to calling EFI_BOOT_SERVICES.StartImage(). If a boot
> option returns control to the boot manager, the boot manager must
> disable the watchdog timer with an additional call to the
> SetWatchdogTimer() boot service."
>
> Chapter 7.4 EFI_BOOT_SERVICES.ExitBootServices()
>
> p. 222
> "On success ... the boot services watchdog timer is disabled."
>
> Chapter 7.5 EFI_BOOT_SERVICES.SetWatchdogTimer()
>
> This chapter describes management of the watchdog timer. You can set any
> duration with 1 second resolution. A value of 0 will disable the watchdog.

To me this makes clear that what they are talking about is not a
hardware watchdog, which may not offer such flexibility, but a
software layer (driver) that provides such an API to the underlying
hardware.  Again depending on the capabilities of the actual
hardware.

> p. 223
> "If the watchdog timer expires, the event is logged by the firmware. The

And what if the hardware cannot detect this, for example because the
watchdog is an external chip that just pulls the external reset
line?

> ... The watchdog must be set to a
> period of 5 minutes. ...

Again: here "watchdog" can only mean some software component. Many
hardware watchdogs do not allow for such long timeouts at all.

> Appendix R - Glossary
>
> p. 2444
> Watchdog Time
> An alarm timer that may be set to go off. This can be used to regain
> control in cases where a code path in the boot services environment
> fails to or is unable to return control by the expected path.

Again this does not sound as if they were talking about a specific
hardware watchdog.  All this is about software services only, it
seem.

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 at denx.de
Egotist: A person of low taste, more interested in  himself  than  in
me.                                                  - Ambrose Bierce

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Mark Kettenis]

> From: Wolfgang Denk <wd@denx.de>
> Date: Sat, 26 Sep 2020 10:45:31 +0200

Hi Wolfgang,

> Dear Heinrich,
> 
> In message <81e467f1-aff3-a54d-5f7e-3b8f5390f410@gmx.de> you wrote:
> >
> > Chapter 2.3.7 RISC-V Platforms
> >
> > p. 40
> > "The causes of reset could be power-on reset, external hard reset,
> > brownout detected, watchdog timer elapse, sleep-mode wakeup, etc., which
> > machine-mode UEFI system firmware has to distinguish."
> 
> This is possible only if the hardware actually supports such
> distinction.  In many cases of actual hardware I've seen a watchdog
> reset is just the same as an external hard reset, and brownout
> detection does not exist.

This is RISC-V specific, and RISC-V apparently has an architected
register that communicates the reset cause.  Even then the text
immediately before the text quoted by Heinrich allows for hardware
that can't distinguish to simply report 0 in that register implying
"the most complete reset (e.g., hard reset)".

>  p. 70
> > "If LoadImage() succeeds, the boot manager must enable the watchdog
> > timer for 5 minutes by using the EFI_BOOT_SERVICES.SetWatchdogTimer()
> > boot service prior to calling EFI_BOOT_SERVICES.StartImage(). If a boot
> > option returns control to the boot manager, the boot manager must
> > disable the watchdog timer with an additional call to the
> > SetWatchdogTimer() boot service."
> >
> > Chapter 7.4 EFI_BOOT_SERVICES.ExitBootServices()
> >
> > p. 222
> > "On success ... the boot services watchdog timer is disabled."
> >
> > Chapter 7.5 EFI_BOOT_SERVICES.SetWatchdogTimer()
> >
> > This chapter describes management of the watchdog timer. You can set any
> > duration with 1 second resolution. A value of 0 will disable the watchdog.
> 
> To me this makes clear that what they are talking about is not a
> hardware watchdog, which may not offer such flexibility, but a
> software layer (driver) that provides such an API to the underlying
> hardware.  Again depending on the capabilities of the actual
> hardware.
> 
> > p. 223
> > "If the watchdog timer expires, the event is logged by the firmware. The
> 
> And what if the hardware cannot detect this, for example because the
> watchdog is an external chip that just pulls the external reset
> line?

Probably wishful thinking on behalf of the authors of the UEFI spec.
The existence of a firmware event log isn't mandated by UEFI and on
x86 typically only implemented on server systems.  On such systems the
watchdog may very well be implemented by software running on a BMC.

> > ... The watchdog must be set to a
> > period of 5 minutes. ...
> 
> Again: here "watchdog" can only mean some software component. Many
> hardware watchdogs do not allow for such long timeouts at all.
> 
> > Appendix R - Glossary
> >
> > p. 2444
> > Watchdog Time
> > An alarm timer that may be set to go off. This can be used to regain
> > control in cases where a code path in the boot services environment
> > fails to or is unable to return control by the expected path.
> 
> Again this does not sound as if they were talking about a specific
> hardware watchdog.  All this is about software services only, it
> seem.

Probably.

That doesn't change the fact that enabling a hardware watchdog timer
that resets the system is problematic for the EFI boot path in U-Boot.
The typical EFI boot path is:

  UEFI Firmware ->       EFI OS loader       ->    OS kernel
    (U-Boot)       (GRUB, OpenBSD's EFIBOOT)    (Linux, OpenBSD)

Here the EFI OS loader is hardware agnostic and only supposed to use
EFI interfaces to do its job.  As such it cannot reset or disable the
watchdog hardware.  Maybe it can do its job of loading and starting
the OS kernel before a hardware watchdog timer expires.  But if there
is any user interaction required the timer will at some point expire
and reset the system in the middle of the user interaction.

Even if the EFI OS loader does its job before the hardware watchdog
timer expires, there is no guarantee that the OS kernel has the driver
necesary to reset/disable the hardware watchdog.  Or even if it does
have such a driver, loading/attaching it may take too much time.

In my opinion enabling a hardware watchdo timer only makes sense if
U-Boot and the loaded OS kernel are tightly coupled.  Which is a use
case where one would probably not use the EFI boot path in the first
place and use a more traditional U-Boot bootpath such as "bootm"
instead.

At this point UEFI isn't really targeted at "deeply embedded" systems
that require a hardware watchdog to implement guaranteed recovery from
software failures.  If there is a desire to use the EFI boot path in
this scenario someone should probably lobby the UEFI folks to add a
EFI runtime service to reset the hardware watchdog that can be called
by the EFI OS Loader and the OS to prevent the timer from expiring.

Cheers,

Mark

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Heinrich Schuchardt]

On 9/26/20 12:54 PM, Mark Kettenis wrote:
>> From: Wolfgang Denk <wd@denx.de>
>> Date: Sat, 26 Sep 2020 10:45:31 +0200
>
> Hi Wolfgang,
>
>> Dear Heinrich,
>>
>> In message <81e467f1-aff3-a54d-5f7e-3b8f5390f410@gmx.de> you wrote:
>>>
>>> Chapter 2.3.7 RISC-V Platforms
>>>
>>> p. 40
>>> "The causes of reset could be power-on reset, external hard reset,
>>> brownout detected, watchdog timer elapse, sleep-mode wakeup, etc., which
>>> machine-mode UEFI system firmware has to distinguish."
>>
>> This is possible only if the hardware actually supports such
>> distinction.  In many cases of actual hardware I've seen a watchdog
>> reset is just the same as an external hard reset, and brownout
>> detection does not exist.
>
> This is RISC-V specific, and RISC-V apparently has an architected
> register that communicates the reset cause.  Even then the text
> immediately before the text quoted by Heinrich allows for hardware
> that can't distinguish to simply report 0 in that register implying
> "the most complete reset (e.g., hard reset)".
>
>>  p. 70
>>> "If LoadImage() succeeds, the boot manager must enable the watchdog
>>> timer for 5 minutes by using the EFI_BOOT_SERVICES.SetWatchdogTimer()
>>> boot service prior to calling EFI_BOOT_SERVICES.StartImage(). If a boot
>>> option returns control to the boot manager, the boot manager must
>>> disable the watchdog timer with an additional call to the
>>> SetWatchdogTimer() boot service."
>>>
>>> Chapter 7.4 EFI_BOOT_SERVICES.ExitBootServices()
>>>
>>> p. 222
>>> "On success ... the boot services watchdog timer is disabled."
>>>
>>> Chapter 7.5 EFI_BOOT_SERVICES.SetWatchdogTimer()
>>>
>>> This chapter describes management of the watchdog timer. You can set any
>>> duration with 1 second resolution. A value of 0 will disable the watchdog.
>>
>> To me this makes clear that what they are talking about is not a
>> hardware watchdog, which may not offer such flexibility, but a
>> software layer (driver) that provides such an API to the underlying
>> hardware.  Again depending on the capabilities of the actual
>> hardware.
>>
>>> p. 223
>>> "If the watchdog timer expires, the event is logged by the firmware. The
>>
>> And what if the hardware cannot detect this, for example because the
>> watchdog is an external chip that just pulls the external reset
>> line?
>
> Probably wishful thinking on behalf of the authors of the UEFI spec.
> The existence of a firmware event log isn't mandated by UEFI and on
> x86 typically only implemented on server systems.  On such systems the
> watchdog may very well be implemented by software running on a BMC.
>
>>> ... The watchdog must be set to a
>>> period of 5 minutes. ...
>>
>> Again: here "watchdog" can only mean some software component. Many
>> hardware watchdogs do not allow for such long timeouts at all.
>>
>>> Appendix R - Glossary
>>>
>>> p. 2444
>>> Watchdog Time
>>> An alarm timer that may be set to go off. This can be used to regain
>>> control in cases where a code path in the boot services environment
>>> fails to or is unable to return control by the expected path.
>>
>> Again this does not sound as if they were talking about a specific
>> hardware watchdog.  All this is about software services only, it
>> seem.
>
> Probably.
>
> That doesn't change the fact that enabling a hardware watchdog timer
> that resets the system is problematic for the EFI boot path in U-Boot.
> The typical EFI boot path is:
>
>   UEFI Firmware ->       EFI OS loader       ->    OS kernel
>     (U-Boot)       (GRUB, OpenBSD's EFIBOOT)    (Linux, OpenBSD)
>
> Here the EFI OS loader is hardware agnostic and only supposed to use
> EFI interfaces to do its job.  As such it cannot reset or disable the
> watchdog hardware.  Maybe it can do its job of loading and starting
> the OS kernel before a hardware watchdog timer expires.  But if there
> is any user interaction required the timer will at some point expire
> and reset the system in the middle of the user interaction.

This is why UEFI API has the EFI_BOOT_SERVICES.SetWatchdogTimer()
service which allows to set the timeout or disable the watchdog completely.

>
> Even if the EFI OS loader does its job before the hardware watchdog
> timer expires, there is no guarantee that the OS kernel has the driver
> necesary to reset/disable the hardware watchdog.  Or even if it does
> have such a driver, loading/attaching it may take too much time.

The Linux EFI stub calls EFI_BOOT_SERVICES.ExitBootServices. According
to the UEFI spec this is the moment when the watchdog must be disabled.

So in a UEFI environment you can only monitor the time in U-Boot, in
GRUB and part of the Linux EFI stub. You cannot monitor if Linux reaches
the command prompt using the watchdog provided by the UEFI firmware.
Linux could set up its own watchdog that takes over.

>
> In my opinion enabling a hardware watchdo timer only makes sense if
> U-Boot and the loaded OS kernel are tightly coupled.  Which is a use
> case where one would probably not use the EFI boot path in the first
> place and use a more traditional U-Boot bootpath such as "bootm"
> instead.
>
> At this point UEFI isn't really targeted at "deeply embedded" systems
> that require a hardware watchdog to implement guaranteed recovery from
> software failures.  If there is a desire to use the EFI boot path in
> this scenario someone should probably lobby the UEFI folks to add a
> EFI runtime service to reset the hardware watchdog that can be called
> by the EFI OS Loader and the OS to prevent the timer from expiring.

The API call is already exists as EFI_BOOT_SERVICES.SetWatchdogTimer()
as mentioned above.

If we want to have a hardware based watchdog in the UEFI context, we
need a U-Boot internal API by which we can enable and disable a hardware
watchdog with an arbitrary duration (>> 5 min) with 1 second resolution.
Then our implementation of SetWatchdogTimer() could call into this API.

Best regards

Heinrich

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Tom Rini]

On Sat, Sep 26, 2020 at 02:39:39PM +0200, Heinrich Schuchardt wrote:
> On 9/26/20 12:54 PM, Mark Kettenis wrote:
> >> From: Wolfgang Denk <wd@denx.de>
> >> Date: Sat, 26 Sep 2020 10:45:31 +0200
> >
> > Hi Wolfgang,
> >
> >> Dear Heinrich,
> >>
> >> In message <81e467f1-aff3-a54d-5f7e-3b8f5390f410@gmx.de> you wrote:
> >>>
> >>> Chapter 2.3.7 RISC-V Platforms
> >>>
> >>> p. 40
> >>> "The causes of reset could be power-on reset, external hard reset,
> >>> brownout detected, watchdog timer elapse, sleep-mode wakeup, etc., which
> >>> machine-mode UEFI system firmware has to distinguish."
> >>
> >> This is possible only if the hardware actually supports such
> >> distinction.  In many cases of actual hardware I've seen a watchdog
> >> reset is just the same as an external hard reset, and brownout
> >> detection does not exist.
> >
> > This is RISC-V specific, and RISC-V apparently has an architected
> > register that communicates the reset cause.  Even then the text
> > immediately before the text quoted by Heinrich allows for hardware
> > that can't distinguish to simply report 0 in that register implying
> > "the most complete reset (e.g., hard reset)".
> >
> >>  p. 70
> >>> "If LoadImage() succeeds, the boot manager must enable the watchdog
> >>> timer for 5 minutes by using the EFI_BOOT_SERVICES.SetWatchdogTimer()
> >>> boot service prior to calling EFI_BOOT_SERVICES.StartImage(). If a boot
> >>> option returns control to the boot manager, the boot manager must
> >>> disable the watchdog timer with an additional call to the
> >>> SetWatchdogTimer() boot service."
> >>>
> >>> Chapter 7.4 EFI_BOOT_SERVICES.ExitBootServices()
> >>>
> >>> p. 222
> >>> "On success ... the boot services watchdog timer is disabled."
> >>>
> >>> Chapter 7.5 EFI_BOOT_SERVICES.SetWatchdogTimer()
> >>>
> >>> This chapter describes management of the watchdog timer. You can set any
> >>> duration with 1 second resolution. A value of 0 will disable the watchdog.
> >>
> >> To me this makes clear that what they are talking about is not a
> >> hardware watchdog, which may not offer such flexibility, but a
> >> software layer (driver) that provides such an API to the underlying
> >> hardware.  Again depending on the capabilities of the actual
> >> hardware.
> >>
> >>> p. 223
> >>> "If the watchdog timer expires, the event is logged by the firmware. The
> >>
> >> And what if the hardware cannot detect this, for example because the
> >> watchdog is an external chip that just pulls the external reset
> >> line?
> >
> > Probably wishful thinking on behalf of the authors of the UEFI spec.
> > The existence of a firmware event log isn't mandated by UEFI and on
> > x86 typically only implemented on server systems.  On such systems the
> > watchdog may very well be implemented by software running on a BMC.
> >
> >>> ... The watchdog must be set to a
> >>> period of 5 minutes. ...
> >>
> >> Again: here "watchdog" can only mean some software component. Many
> >> hardware watchdogs do not allow for such long timeouts at all.
> >>
> >>> Appendix R - Glossary
> >>>
> >>> p. 2444
> >>> Watchdog Time
> >>> An alarm timer that may be set to go off. This can be used to regain
> >>> control in cases where a code path in the boot services environment
> >>> fails to or is unable to return control by the expected path.
> >>
> >> Again this does not sound as if they were talking about a specific
> >> hardware watchdog.  All this is about software services only, it
> >> seem.
> >
> > Probably.
> >
> > That doesn't change the fact that enabling a hardware watchdog timer
> > that resets the system is problematic for the EFI boot path in U-Boot.
> > The typical EFI boot path is:
> >
> >   UEFI Firmware ->       EFI OS loader       ->    OS kernel
> >     (U-Boot)       (GRUB, OpenBSD's EFIBOOT)    (Linux, OpenBSD)
> >
> > Here the EFI OS loader is hardware agnostic and only supposed to use
> > EFI interfaces to do its job.  As such it cannot reset or disable the
> > watchdog hardware.  Maybe it can do its job of loading and starting
> > the OS kernel before a hardware watchdog timer expires.  But if there
> > is any user interaction required the timer will at some point expire
> > and reset the system in the middle of the user interaction.
> 
> This is why UEFI API has the EFI_BOOT_SERVICES.SetWatchdogTimer()
> service which allows to set the timeout or disable the watchdog completely.
> 
> >
> > Even if the EFI OS loader does its job before the hardware watchdog
> > timer expires, there is no guarantee that the OS kernel has the driver
> > necesary to reset/disable the hardware watchdog.  Or even if it does
> > have such a driver, loading/attaching it may take too much time.
> 
> The Linux EFI stub calls EFI_BOOT_SERVICES.ExitBootServices. According
> to the UEFI spec this is the moment when the watchdog must be disabled.
> 
> So in a UEFI environment you can only monitor the time in U-Boot, in
> GRUB and part of the Linux EFI stub. You cannot monitor if Linux reaches
> the command prompt using the watchdog provided by the UEFI firmware.
> Linux could set up its own watchdog that takes over.
> 
> >
> > In my opinion enabling a hardware watchdo timer only makes sense if
> > U-Boot and the loaded OS kernel are tightly coupled.  Which is a use
> > case where one would probably not use the EFI boot path in the first
> > place and use a more traditional U-Boot bootpath such as "bootm"
> > instead.
> >
> > At this point UEFI isn't really targeted at "deeply embedded" systems
> > that require a hardware watchdog to implement guaranteed recovery from
> > software failures.  If there is a desire to use the EFI boot path in
> > this scenario someone should probably lobby the UEFI folks to add a
> > EFI runtime service to reset the hardware watchdog that can be called
> > by the EFI OS Loader and the OS to prevent the timer from expiring.
> 
> The API call is already exists as EFI_BOOT_SERVICES.SetWatchdogTimer()
> as mentioned above.
> 
> If we want to have a hardware based watchdog in the UEFI context, we
> need a U-Boot internal API by which we can enable and disable a hardware
> watchdog with an arbitrary duration (>> 5 min) with 1 second resolution.
> Then our implementation of SetWatchdogTimer() could call into this API.

Thanks for excerpting from the UEFI spec where it talks about a
"watchdog" at all.  It's clear from these excerpts that the spec simply
does not talk about a hardware watchdog at all and is using the term in
one of its other meanings.  Which means that we need to figure out and
document something nominally sane so at least it's not a surprise to the
user.

Michael, what driver under drivers/watchdog/ is your platform using
exactly?  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200926/b0c712ef/attachment.sig>

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Wolfgang Denk]

Dear Mark,

In message <8ba616a732f62f9c@bloch.sibelius.xs4all.nl> you wrote:
> > > p. 40
> > > "The causes of reset could be power-on reset, external hard reset,
> > > brownout detected, watchdog timer elapse, sleep-mode wakeup, etc., which
> > > machine-mode UEFI system firmware has to distinguish."
> > 
> > This is possible only if the hardware actually supports such
> > distinction.  In many cases of actual hardware I've seen a watchdog
> > reset is just the same as an external hard reset, and brownout
> > detection does not exist.
>
> This is RISC-V specific, and RISC-V apparently has an architected
> register that communicates the reset cause.  Even then the text
> immediately before the text quoted by Heinrich allows for hardware
> that can't distinguish to simply report 0 in that register implying
> "the most complete reset (e.g., hard reset)".

Sorry, I disagree here.  This is in no way RISC-V specific, it is a
generic hardware thing.  If your board design has an external
watchdog chip which simply pulls the main reset line, you CANNOT
distinguish between the watchdog triggering or the user pressing the
reset button.

The reset reason register can only show information it has access
to, i. e. when a SoC-internal WD triggers.

> That doesn't change the fact that enabling a hardware watchdog timer
> that resets the system is problematic for the EFI boot path in U-Boot.
> The typical EFI boot path is:
>
>   UEFI Firmware ->       EFI OS loader       ->    OS kernel
>     (U-Boot)       (GRUB, OpenBSD's EFIBOOT)    (Linux, OpenBSD)
>
> Here the EFI OS loader is hardware agnostic and only supposed to use
> EFI interfaces to do its job.  As such it cannot reset or disable the
> watchdog hardware.  Maybe it can do its job of loading and starting
> the OS kernel before a hardware watchdog timer expires.  But if there
> is any user interaction required the timer will at some point expire
> and reset the system in the middle of the user interaction.
>
> Even if the EFI OS loader does its job before the hardware watchdog
> timer expires, there is no guarantee that the OS kernel has the driver
> necesary to reset/disable the hardware watchdog.  Or even if it does
> have such a driver, loading/attaching it may take too much time.

I guess this is just another deficiency in the (U)EFI design...

> In my opinion enabling a hardware watchdo timer only makes sense if
> U-Boot and the loaded OS kernel are tightly coupled.  Which is a use
> case where one would probably not use the EFI boot path in the first
> place and use a more traditional U-Boot bootpath such as "bootm"
> instead.

Agreed.  I would never run these software layers on a system that
requires to be secure/reliable/robust/reliable/robust.

> At this point UEFI isn't really targeted at "deeply embedded" systems
> that require a hardware watchdog to implement guaranteed recovery from
> software failures.  If there is a desire to use the EFI boot path in
> this scenario someone should probably lobby the UEFI folks to add a
> EFI runtime service to reset the hardware watchdog that can be called
> by the EFI OS Loader and the OS to prevent the timer from expiring.

Or, as I mentioned before: if you cannot make sure to trigger the
watchdog in a sensible way, leave it off until you have reached a
software state where this can be done. Only then enable it (keeping
in mind that this may be a write-once register somehwere, so you
cannot assume to disable in other ways than a hardware reset).

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 at denx.de
Being schizophrenic is better than living alone.

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Wolfgang Denk]

Dear Heinrich,

In message <ef24629c-2c57-c681-eae5-2d034abfbd9b@gmx.de> you wrote:
>
> This is why UEFI API has the EFI_BOOT_SERVICES.SetWatchdogTimer()
> service which allows to set the timeout or disable the watchdog completely. 

UEFI may have any APIs it wants, this will not change the underlying
hardware in any way.  A proper watchdog implementation has
write-once register to prime the watchdog.  Once it is armed, there
will be no other way to disarm it except for a hardware reset.

> The Linux EFI stub calls EFI_BOOT_SERVICES.ExitBootServices. According
> to the UEFI spec this is the moment when the watchdog must be disabled.
>
> So in a UEFI environment you can only monitor the time in U-Boot, in
> GRUB and part of the Linux EFI stub. You cannot monitor if Linux reaches
> the command prompt using the watchdog provided by the UEFI firmware.
> Linux could set up its own watchdog that takes over.

The logical approach is to leave the watchdog uninitialized in
U-Boot, and activate it in Linux only.

Or, if you need to be able to detect and reset a system hanging in
the boot process, then simply do not use UEFI at all.  Apparently it
is unsuitable for any reliable systems.

I mean - when there are mandatory phases in EFI boot where the WD
_must_ be disabled, then what happens when booting hangs exactly
then?  then you are exactly in the same position as if not enabling
the WD at all in U-Boot.


> If we want to have a hardware based watchdog in the UEFI context, we
> need a U-Boot internal API by which we can enable and disable a hardware
> watchdog with an arbitrary duration (>> 5 min) with 1 second resolution.
> Then our implementation of SetWatchdogTimer() could call into this API.

Sorry, but please stop ignoring hardware properties!

Yes, eventually we could implement a software layer that triggers
the watchdog _while_U-Boot_is_running.  But as soon as U-Boot loats
and starts the next payload, this code is gone.

For example, assume a hardware watchdog with a fixed time-out
interval of 1.6 seconds that cannot be disabled at all in software -
it starts running with power on. [Note that this is not a theoretical
example, but real life.]

We can deal with this in U-Boot just fine.  We can also deal with
this booting Linux - 1.6 seconds is usually enough to get the needed
driver initialized in Linux, too.  But as soon as U-Boot starts the
first payload, you have only those 1.6 seconds - so if your payload
does not trigger the WD, you lose.

You can implement software services only within the capabilities of
the given hardware.

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 at denx.de
If a packet hits a pocket on a socket on a port,
And the bus is interrupted as a very last resort,
And the address of the memory makes your floppy disk abort,
Then the socket packet pocket has an error to report! - Ken Burchill?

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Michael Walle]

Am 2020-09-26 14:44, schrieb Tom Rini:
[..]
>> If we want to have a hardware based watchdog in the UEFI context, we
>> need a U-Boot internal API by which we can enable and disable a 
>> hardware
>> watchdog with an arbitrary duration (>> 5 min) with 1 second 
>> resolution.
>> Then our implementation of SetWatchdogTimer() could call into this 
>> API.
> 
> Thanks for excerpting from the UEFI spec where it talks about a
> "watchdog" at all.  It's clear from these excerpts that the spec simply
> does not talk about a hardware watchdog at all and is using the term in
> one of its other meanings.  Which means that we need to figure out and
> document something nominally sane so at least it's not a surprise to 
> the
> user.
> 
> Michael, what driver under drivers/watchdog/ is your platform using
> exactly?  Thanks!

In its current form (i.e. basic BSP support) it uses the sp805_wdt.c;
which is a nice to have, but the real watchdog is an external device.
There is no support in U-Boot for it yet, but I'm planning to add it
and thus I need the CONFIG_WDT ;)

Currently this external watchdog is disabled (there are non-volatile
configuration bits on the board and one bit is for disabling this
watchdog at startup). Otherwise it would always activate the board's
failsafe mode because it isn't triggered by U-Boot.

And yes, this watchdog also have a lock bit, once set, you cannot
deactivate it anymore (although you can change the timeout period
in a given range).

Because at the moment there is no driver for this watchdog yet (it
is a custom one), my newest version of my "basic board support for
Kontron sl28" has the CONFIG_WDT deactivated. I prefer having a
usable bootefi to have support for the SoC internal watchdog.

-michael

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Tom Rini]

On Sun, Sep 27, 2020 at 06:06:38PM +0200, Michael Walle wrote:
> Am 2020-09-26 14:44, schrieb Tom Rini:
> [..]
> > > If we want to have a hardware based watchdog in the UEFI context, we
> > > need a U-Boot internal API by which we can enable and disable a
> > > hardware
> > > watchdog with an arbitrary duration (>> 5 min) with 1 second
> > > resolution.
> > > Then our implementation of SetWatchdogTimer() could call into this
> > > API.
> > 
> > Thanks for excerpting from the UEFI spec where it talks about a
> > "watchdog" at all.  It's clear from these excerpts that the spec simply
> > does not talk about a hardware watchdog at all and is using the term in
> > one of its other meanings.  Which means that we need to figure out and
> > document something nominally sane so at least it's not a surprise to the
> > user.
> > 
> > Michael, what driver under drivers/watchdog/ is your platform using
> > exactly?  Thanks!
> 
> In its current form (i.e. basic BSP support) it uses the sp805_wdt.c;
> which is a nice to have, but the real watchdog is an external device.
> There is no support in U-Boot for it yet, but I'm planning to add it
> and thus I need the CONFIG_WDT ;)
> 
> Currently this external watchdog is disabled (there are non-volatile
> configuration bits on the board and one bit is for disabling this
> watchdog at startup). Otherwise it would always activate the board's
> failsafe mode because it isn't triggered by U-Boot.
> 
> And yes, this watchdog also have a lock bit, once set, you cannot
> deactivate it anymore (although you can change the timeout period
> in a given range).
> 
> Because at the moment there is no driver for this watchdog yet (it
> is a custom one), my newest version of my "basic board support for
> Kontron sl28" has the CONFIG_WDT deactivated. I prefer having a
> usable bootefi to have support for the SoC internal watchdog.

Thanks for explaining.  I was asking in part because I was wondering if
that SoC was falling into the case of drivers/watchdog/imx_watchdog.c

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200928/56382dfe/attachment.sig>

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Michael Walle]

Hi all,

Am 2020-09-25 15:04, schrieb Wolfgang Denk:
> Dear Heinrich Schuchardt,
> 
> In message <ceeb82b7-eef0-7435-0d0c-958c0bf07e04@gmx.de> you wrote:
>> 
>> > Any so-called "watchdog" that can be disabled / switched off by
>> > software is not really woth this name.  As such, the concept of
>> > disabling a watchdog in software, is misleading at best and should
>> > never ibe implemented.
>> 
>> If we want to boot UEFI payloads, we will have to follow the UEFI
>> specification even if we think it is not perfect.
> 
> That's perfectly OK.  But if the OS expects that the watchdog is
> disabled, then we should not enable it in U-Boot in the first place.
> 
> Keep in ind that any real watchdog (which is worth the money and the
> name) _cannot_ be disabled by software once it was started.

How do we proceed here? There seems to be no final conclusion what to
do.

I guess one thing we agree on is that after a system is booted with
bootefi the watchdog should be disabled. So Wolfgang argues, that the
watchdog shouldn't be enabled in the first place then. With this patch
this would actually be possible, the user could just choose to not
enable it at all; if he has a watchdog which can be disabled again,
he can choose that behavior too.

OTOH Mark argues, that in the bootefi case, the watchdog should be
disabled in _any_ case. But that would mean to change the behavior
of current boards. So the question is: is this acceptable?

So if this is not acceptable, I don't think there will be any
changes to this patch (otherwise than having some additional help
text).

If it is acceptable, I'd stop the watchdog before bootefi
unconditionally (and printing a message if SUPERVISE_OS is set), but
keep the current logic for bootm. Personally I'd also prefer this,
because in the end you'll have a bootloader which can boot an OS via
EFI in any case.

Tom, you've mentioned the IMX_WATCHDOG with !CONFIG_WDT. One key
difference is that without CONFIG_WDT the watchdog is only started
if CONFIG_WATCHDOG is enabled (or I missed something here). And I
don't think this patch will apply for that case, right? I think
in that case the watchdog can't even be stopped; there seems to be
only functions to initialize and kick it. Therefore, it also won't
be compatible the EFI spec. There might be warning during the build
if HW_WATCHDOG && EFI_LOADER is set. With a note which suggest the
move to CONFIG_WDT.

-michael

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Heinrich Schuchardt]

Am 4. Oktober 2020 16:55:32 MESZ schrieb Michael Walle <michael@walle.cc>:
>Hi all,
>
>Am 2020-09-25 15:04, schrieb Wolfgang Denk:
>> Dear Heinrich Schuchardt,
>> 
>> In message <ceeb82b7-eef0-7435-0d0c-958c0bf07e04@gmx.de> you wrote:
>>> 
>>> > Any so-called "watchdog" that can be disabled / switched off by
>>> > software is not really woth this name.  As such, the concept of
>>> > disabling a watchdog in software, is misleading at best and should
>>> > never ibe implemented.
>>> 
>>> If we want to boot UEFI payloads, we will have to follow the UEFI
>>> specification even if we think it is not perfect.
>> 
>> That's perfectly OK.  But if the OS expects that the watchdog is
>> disabled, then we should not enable it in U-Boot in the first place.
>> 
>> Keep in ind that any real watchdog (which is worth the money and the
>> name) _cannot_ be disabled by software once it was started.
>
>How do we proceed here? There seems to be no final conclusion what to
>do.
>
>I guess one thing we agree on is that after a system is booted with
>bootefi the watchdog should be disabled. So Wolfgang argues, that the
>watchdog shouldn't be enabled in the first place then. With this patch
>this would actually be possible, the user could just choose to not
>enable it at all; if he has a watchdog which can be disabled again,
>he can choose that behavior too.
>
>OTOH Mark argues, that in the bootefi case, the watchdog should be
>disabled in _any_ case. But that would mean to change the behavior
>of current boards. So the question is: is this acceptable?
>
>So if this is not acceptable, I don't think there will be any
>changes to this patch (otherwise than having some additional help
>text).
>
>If it is acceptable, I'd stop the watchdog before bootefi
>unconditionally (and printing a message if SUPERVISE_OS is set), but
>keep the current logic for bootm. Personally I'd also prefer this,
>because in the end you'll have a bootloader which can boot an OS via
>EFI in any case.
>
>Tom, you've mentioned the IMX_WATCHDOG with !CONFIG_WDT. One key
>difference is that without CONFIG_WDT the watchdog is only started
>if CONFIG_WATCHDOG is enabled (or I missed something here). And I
>don't think this patch will apply for that case, right? I think
>in that case the watchdog can't even be stopped; there seems to be
>only functions to initialize and kick it. Therefore, it also won't
>be compatible the EFI spec. There might be warning during the build
>if HW_WATCHDOG && EFI_LOADER is set. With a note which suggest the
>move to CONFIG_WDT.
>
>-michael

For the UEFI side the constraint is: do not arm any watchdog that is not disarmed in efi_exitboot_services() or has an initial timeout  < 300s or is not adjusted or disarmed by efi_set_watchdog().

Best regards

Heinrich

[PATCH 2/2] watchdog: add watchdog behavior configuration

[Tom Rini]

On Sun, Oct 04, 2020 at 05:10:32PM +0200, Heinrich Schuchardt wrote:
> Am 4. Oktober 2020 16:55:32 MESZ schrieb Michael Walle <michael@walle.cc>:
> >Hi all,
> >
> >Am 2020-09-25 15:04, schrieb Wolfgang Denk:
> >> Dear Heinrich Schuchardt,
> >> 
> >> In message <ceeb82b7-eef0-7435-0d0c-958c0bf07e04@gmx.de> you wrote:
> >>> 
> >>> > Any so-called "watchdog" that can be disabled / switched off by
> >>> > software is not really woth this name.  As such, the concept of
> >>> > disabling a watchdog in software, is misleading at best and should
> >>> > never ibe implemented.
> >>> 
> >>> If we want to boot UEFI payloads, we will have to follow the UEFI
> >>> specification even if we think it is not perfect.
> >> 
> >> That's perfectly OK.  But if the OS expects that the watchdog is
> >> disabled, then we should not enable it in U-Boot in the first place.
> >> 
> >> Keep in ind that any real watchdog (which is worth the money and the
> >> name) _cannot_ be disabled by software once it was started.
> >
> >How do we proceed here? There seems to be no final conclusion what to
> >do.
> >
> >I guess one thing we agree on is that after a system is booted with
> >bootefi the watchdog should be disabled. So Wolfgang argues, that the
> >watchdog shouldn't be enabled in the first place then. With this patch
> >this would actually be possible, the user could just choose to not
> >enable it at all; if he has a watchdog which can be disabled again,
> >he can choose that behavior too.
> >
> >OTOH Mark argues, that in the bootefi case, the watchdog should be
> >disabled in _any_ case. But that would mean to change the behavior
> >of current boards. So the question is: is this acceptable?
> >
> >So if this is not acceptable, I don't think there will be any
> >changes to this patch (otherwise than having some additional help
> >text).
> >
> >If it is acceptable, I'd stop the watchdog before bootefi
> >unconditionally (and printing a message if SUPERVISE_OS is set), but
> >keep the current logic for bootm. Personally I'd also prefer this,
> >because in the end you'll have a bootloader which can boot an OS via
> >EFI in any case.
> >
> >Tom, you've mentioned the IMX_WATCHDOG with !CONFIG_WDT. One key
> >difference is that without CONFIG_WDT the watchdog is only started
> >if CONFIG_WATCHDOG is enabled (or I missed something here). And I
> >don't think this patch will apply for that case, right? I think
> >in that case the watchdog can't even be stopped; there seems to be
> >only functions to initialize and kick it. Therefore, it also won't
> >be compatible the EFI spec. There might be warning during the build
> >if HW_WATCHDOG && EFI_LOADER is set. With a note which suggest the
> >move to CONFIG_WDT.
> >
> >-michael
> 
> For the UEFI side the constraint is: do not arm any watchdog that is
> not disarmed in efi_exitboot_services() or has an initial timeout  <
> 300s or is not adjusted or disarmed by efi_set_watchdog().

Which in turn means that UEFI fails to consider most modern SoCs.  I
think we'll need to state somewhere that for some cases we simply aren't
compliant.  That's not to say we shouldn't have a way to be compliant
when possible.  With that IMX_WATCHDOG case, I too need to dig in a bit
to see how it works really in that case as I thought I saw that we were
servicing the watchdog, always.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201004/2dd09bf0/attachment.sig>