[PATCH 1/6] Documentation: efi: emphasize watchdog deactivation on ExitBootServices

[PATCH 1/6] Documentation: efi: emphasize watchdog deactivation on ExitBootServices

[Ahmad Fatoum]

The UEFI specification paragraph quoted above notes:
> The watchdog timer is only used during boot services. On successful
> completion of ExitBootServices() the watchdog timer is disabled.

Thus disabling the watchdog is _the_ only proper behavior. Adjust the
wording accordingly.

Cc: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 Documentation/boards/efi.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/boards/efi.rst b/Documentation/boards/efi.rst
index 2178c9ab4293..f04b1d32378b 100644
--- a/Documentation/boards/efi.rst
+++ b/Documentation/boards/efi.rst
@@ -350,7 +350,7 @@ https://uefi.org/sites/default/files/resources/UEFI_Spec_2_1_D.pdf
 
 Current linux kernel (v5.0) will execute ExitBootServices() during the early
 boot stage and thus will automatically disable the (U)EFI watchdog. Since it is
-a proper behavior according to the (U)EFI specification, it is impossible to
+the proper behavior according to the (U)EFI specification, it is impossible to
 protect full boot chain by using this watchdog only. It is recommended to use
 an alternative hardware watchdog, preferably started before the bootloader. If (U)EFI
 firmware lacks this feature, the bootloader should be able to start an alternative
-- 
2.23.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/6] efi: efi-image: don't mask x86 interrupts on boot

[Ahmad Fatoum]

55da0cf1 ("efi: add support for initrd loading") introduced support for
the Linux v3.6+ handover protocol[1]. As part of this change a x86 cli
(Clear Interrupt Flag) instruction was introduced just prior to the jump
into the kernel's EFI handover protocol entry point.

While the normal Linux x86 boot protocols require that interrupts are
masked on entry, this doesn't apply to the EFI stub, because the EFI
stub itself is the one implementing the boot protocol and as such
masks the interrupts itself[2].

EFI watchdogs may, and often are, implemented using a timer interrupt.
Dropping the cli will allow monitoring the boot of the kernel up to it
calling ExitBootServices.

In absence of a hardware watchdog, this is the only watchdog available
to users with EFI 1.0+, so it seems prudent to not make it even more
useless.

[1]: https://www.kernel.org/doc/Documentation/x86/boot.txt
[2]: Linux v5.4-rc4, arch/x86/boot/compressed/eboot.c

Cc: Michael Olbrich <mol@pengutronix.de>
Fixes: 55da0cf1 ("efi: add support for initrd loading")
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/efi/efi-image.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/common/efi/efi-image.c b/common/efi/efi-image.c
index 939663a6e250..9c66c9f882c5 100644
--- a/common/efi/efi-image.c
+++ b/common/efi/efi-image.c
@@ -174,7 +174,6 @@ static inline void linux_efi_handover(efi_handle_t handle,
 {
 	handover_fn handover;
 
-	asm volatile ("cli");
 	handover = (handover_fn)((long)header->code32_start + 512 +
 				 header->handover_offset);
 	handover(handle, efi_sys_table, header);
-- 
2.23.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/6] watchdog: efi: bump down priority below default

[Ahmad Fatoum]

The barebox EFI documentation notes:

> Current linux kernel (v5.0) will execute ExitBootServices() during the
> early boot stage and thus will automatically disable the (U)EFI watchdog.
> Since it is the proper behavior according to the (U)EFI specification, it
> is impossible to protect full boot chain by using this watchdog only.
> It is recommended to use an alternative hardware watchdog

Heed the advice and bump down the EFI watchdog priority below the
watchdog priority default.
This ensures the EFI watchdog isn't inadvertently used if other
watchdogs are registered.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/watchdog/efi_wdt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/watchdog/efi_wdt.c b/drivers/watchdog/efi_wdt.c
index 8e3e51b7a923..ea1ede13817d 100644
--- a/drivers/watchdog/efi_wdt.c
+++ b/drivers/watchdog/efi_wdt.c
@@ -41,6 +41,7 @@ static int efi_wdt_probe(struct device_d *dev)
 	priv->wd.set_timeout = efi_wdt_set_timeout;
 	priv->wd.hwdev = dev;
 	priv->dev = dev;
+	priv->wd.priority = WATCHDOG_DEFAULT_PRIORITY - 50;
 
 	dev->priv = priv;
 
-- 
2.23.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 4/6] watchdog: export priority as device parameter

[Ahmad Fatoum]

8f4cf30903 ("watchdog: Allow multiple watchdogs") introduced the ability
to set a per-watchdog priority from within drivers, which is usually
populated with of_get_watchdog_priority.

For debugging, it can be useful to query and override this priority on
the fly.  Provide a device parameter to do so.
As watchdog_get_default only considers priorities > 0, it makes sense
to have a newly set priority of 0 disable the watchdog.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/watchdog/wd_core.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/watchdog/wd_core.c b/drivers/watchdog/wd_core.c
index e6e5ddecd2f8..ae29a76064aa 100644
--- a/drivers/watchdog/wd_core.c
+++ b/drivers/watchdog/wd_core.c
@@ -41,6 +41,16 @@ static int _watchdog_set_timeout(struct watchdog *wd, unsigned timeout)
 	return wd->set_timeout(wd, timeout);
 }
 
+static int watchdog_set_priority(struct param_d *param, void *priv)
+{
+	struct watchdog *wd = priv;
+
+	if (wd->priority == 0)
+		return _watchdog_set_timeout(wd, 0);
+
+	return 0;
+}
+
 static int watchdog_set_cur(struct param_d *param, void *priv)
 {
 	struct watchdog *wd = priv;
@@ -130,6 +140,12 @@ int watchdog_register(struct watchdog *wd)
 	if (!wd->priority)
 		wd->priority = WATCHDOG_DEFAULT_PRIORITY;
 
+	p = dev_add_param_uint32(&wd->dev, "priority",
+				 watchdog_set_priority, NULL,
+				 &wd->priority, "%u", wd);
+	if (IS_ERR(p))
+		return PTR_ERR(p);
+
 	/* set some default sane value */
 	if (!wd->timeout_max)
 		wd->timeout_max = 60 * 60 * 24;
-- 
2.23.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 5/6] watchdog: export API to configure watchdogs by name

[Ahmad Fatoum]

So far watchdog users could only configure the watchdog with the highest
priority. In preparation for having the wd command configure a watchdog
by name, extend watchdog_set_timeout with a struct watchdog *parameter
and export functions to query default watchdog and to find watchdog by
name.
No functional change.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 commands/wd.c              |  2 +-
 common/boot.c              |  3 ++-
 drivers/watchdog/wd_core.c | 40 +++++++++++++++++++++++---------------
 include/watchdog.h         | 18 +++++++++++++++--
 4 files changed, 43 insertions(+), 20 deletions(-)

diff --git a/commands/wd.c b/commands/wd.c
index 26823f869afb..f857291f0362 100644
--- a/commands/wd.c
+++ b/commands/wd.c
@@ -34,7 +34,7 @@ static int do_wd(int argc, char *argv[])
 		}
 	}
 
-	rc = watchdog_set_timeout(timeout);
+	rc = watchdog_set_timeout(watchdog_get_default(), timeout);
 	if (rc < 0) {
 		switch (rc) {
 		case -EINVAL:
diff --git a/common/boot.c b/common/boot.c
index 14d4fe9d6460..dcbe5cc2ec7d 100644
--- a/common/boot.c
+++ b/common/boot.c
@@ -146,7 +146,8 @@ int boot_entry(struct bootentry *be, int verbose, int dryrun)
 	printf("Booting entry '%s'\n", be->title);
 
 	if (IS_ENABLED(CONFIG_WATCHDOG) && boot_watchdog_timeout) {
-		ret = watchdog_set_timeout(boot_watchdog_timeout);
+		ret = watchdog_set_timeout(watchdog_get_default(),
+					   boot_watchdog_timeout);
 		if (ret)
 			pr_warn("Failed to enable watchdog: %s\n", strerror(-ret));
 	}
diff --git a/drivers/watchdog/wd_core.c b/drivers/watchdog/wd_core.c
index ae29a76064aa..80bde82f7cdd 100644
--- a/drivers/watchdog/wd_core.c
+++ b/drivers/watchdog/wd_core.c
@@ -31,8 +31,15 @@ static const char *watchdog_name(struct watchdog *wd)
 	return "unknown";
 }
 
-static int _watchdog_set_timeout(struct watchdog *wd, unsigned timeout)
+/*
+ * start, stop or retrigger the watchdog
+ * timeout in [seconds]. timeout of '0' will disable the watchdog (if possible)
+ */
+int watchdog_set_timeout(struct watchdog *wd, unsigned timeout)
 {
+	if (!wd)
+		return -ENODEV;
+
 	if (timeout > wd->timeout_max)
 		return -EINVAL;
 
@@ -40,13 +47,14 @@ static int _watchdog_set_timeout(struct watchdog *wd, unsigned timeout)
 
 	return wd->set_timeout(wd, timeout);
 }
+EXPORT_SYMBOL(watchdog_set_timeout);
 
 static int watchdog_set_priority(struct param_d *param, void *priv)
 {
 	struct watchdog *wd = priv;
 
 	if (wd->priority == 0)
-		return _watchdog_set_timeout(wd, 0);
+		return watchdog_set_timeout(wd, 0);
 
 	return 0;
 }
@@ -65,7 +73,7 @@ static void watchdog_poller_cb(void *priv);
 
 static void watchdog_poller_start(struct watchdog *wd)
 {
-	_watchdog_set_timeout(wd, wd->timeout_cur);
+	watchdog_set_timeout(wd, wd->timeout_cur);
 	poller_call_async(&wd->poller, 500 * MSECOND,
 			watchdog_poller_cb, wd);
 
@@ -192,7 +200,7 @@ int watchdog_deregister(struct watchdog *wd)
 }
 EXPORT_SYMBOL(watchdog_deregister);
 
-static struct watchdog *watchdog_get_default(void)
+struct watchdog *watchdog_get_default(void)
 {
 	struct watchdog *tmp, *wd = NULL;
 	int priority = 0;
@@ -206,23 +214,23 @@ static struct watchdog *watchdog_get_default(void)
 
 	return wd;
 }
+EXPORT_SYMBOL(watchdog_get_default);
 
-/*
- * start, stop or retrigger the watchdog
- * timeout in [seconds]. timeout of '0' will disable the watchdog (if possible)
- */
-int watchdog_set_timeout(unsigned timeout)
+struct watchdog *watchdog_get_by_name(const char *name)
 {
-	struct watchdog *wd;
+	struct watchdog *tmp;
+	struct device_d *dev = get_device_by_name(name);
+	if (!dev)
+		return NULL;
 
-	wd = watchdog_get_default();
-
-	if (!wd)
-		return -ENODEV;
+	list_for_each_entry(tmp, &watchdog_list, list) {
+		if (dev == tmp->hwdev || dev == &tmp->dev)
+			return tmp;
+	}
 
-	return _watchdog_set_timeout(wd, timeout);
+	return NULL;
 }
-EXPORT_SYMBOL(watchdog_set_timeout);
+EXPORT_SYMBOL(watchdog_get_by_name);
 
 /**
  * of_get_watchdog_priority() - get the desired watchdog priority from device tree
diff --git a/include/watchdog.h b/include/watchdog.h
index 0db4263a31ce..1455cc923c2d 100644
--- a/include/watchdog.h
+++ b/include/watchdog.h
@@ -14,6 +14,7 @@
 # define INCLUDE_WATCHDOG_H
 
 #include <poller.h>
+#include <driver.h>
 
 struct watchdog {
 	int (*set_timeout)(struct watchdog *, unsigned);
@@ -31,7 +32,9 @@ struct watchdog {
 #ifdef CONFIG_WATCHDOG
 int watchdog_register(struct watchdog *);
 int watchdog_deregister(struct watchdog *);
-int watchdog_set_timeout(unsigned);
+struct watchdog *watchdog_get_default(void);
+struct watchdog *watchdog_get_by_name(const char *name);
+int watchdog_set_timeout(struct watchdog*, unsigned);
 unsigned int of_get_watchdog_priority(struct device_node *node);
 #else
 static inline int watchdog_register(struct watchdog *w)
@@ -44,11 +47,22 @@ static inline int watchdog_deregister(struct watchdog *w)
 	return 0;
 }
 
-static inline int watchdog_set_timeout(unsigned t)
+static inline struct watchdog *watchdog_get_default(void)
+{
+	return NULL;
+}
+
+struct watchdog *watchdog_get_by_name(const char *name)
+{
+	return NULL;
+}
+
+static inline int watchdog_set_timeout(struct watchdog*w, unsigned t)
 {
 	return 0;
 }
 
+
 static inline unsigned int of_get_watchdog_priority(struct device_node *node)
 {
 	return 0;
-- 
2.23.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 6/6] commands: wd: support configuring watchdog by name

[Ahmad Fatoum]

So far, wd has always configured the highest-priority watchdog when
multiple are available. Add an optional -d parameter to support
configuring the other watchdogs as well. The name passed can be either
the watchdog device name (e.g. wdog0) or the hardware device name (e.g.
efi-wdt).

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 commands/wd.c | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/commands/wd.c b/commands/wd.c
index f857291f0362..8029bab1ce19 100644
--- a/commands/wd.c
+++ b/commands/wd.c
@@ -16,6 +16,8 @@
 #include <command.h>
 #include <errno.h>
 #include <linux/ctype.h>
+#include <getopt.h>
+#include <complete.h>
 #include <watchdog.h>
 
 /* default timeout in [sec] */
@@ -23,18 +25,30 @@ static unsigned timeout = CONFIG_CMD_WD_DEFAULT_TIMOUT;
 
 static int do_wd(int argc, char *argv[])
 {
+	struct watchdog *wd = watchdog_get_default();
+	int opt;
 	int rc;
 
-	if (argc > 1) {
-		if (isdigit(*argv[1])) {
-			timeout = simple_strtoul(argv[1], NULL, 0);
+	while ((opt = getopt(argc, argv, "d:")) > 0) {
+		switch (opt) {
+		case 'd':
+			wd = watchdog_get_by_name(optarg);
+			break;
+		default:
+			return COMMAND_ERROR_USAGE;
+		}
+	}
+
+	if (optind < argc) {
+		if (isdigit(*argv[optind])) {
+			timeout = simple_strtoul(argv[optind], NULL, 0);
 		} else {
 			printf("numerical parameter expected\n");
-			return 1;
+			return COMMAND_ERROR_USAGE;
 		}
 	}
 
-	rc = watchdog_set_timeout(watchdog_get_default(), timeout);
+	rc = watchdog_set_timeout(wd, timeout);
 	if (rc < 0) {
 		switch (rc) {
 		case -EINVAL:
@@ -43,12 +57,15 @@ static int do_wd(int argc, char *argv[])
 		case -ENOSYS:
 			printf("Watchdog cannot be disabled\n");
 			break;
+		case -ENODEV:
+			printf("Watchdog device doesn't exist.\n");
+			break;
 		default:
 			printf("Watchdog fails: '%s'\n", strerror(-rc));
 			break;
 		}
 
-		return 1;
+		return COMMAND_ERROR;
 	}
 
 	return 0;
@@ -58,12 +75,15 @@ BAREBOX_CMD_HELP_START(wd)
 BAREBOX_CMD_HELP_TEXT("Enable the watchdog to bark in TIME seconds.")
 BAREBOX_CMD_HELP_TEXT("When TIME is 0, the watchdog gets disabled,")
 BAREBOX_CMD_HELP_TEXT("Without a parameter the watchdog will be re-triggered.")
+BAREBOX_CMD_HELP_TEXT("Options:")
+BAREBOX_CMD_HELP_OPT("-d DEVICE\t", "watchdog name (default is highest priority watchdog)")
 BAREBOX_CMD_HELP_END
 
 BAREBOX_CMD_START(wd)
 	.cmd = do_wd,
 	BAREBOX_CMD_DESC("enable/disable/trigger the watchdog")
-	BAREBOX_CMD_OPTS("[TIME]")
+	BAREBOX_CMD_OPTS("[-d DEVICE] [TIME]")
 	BAREBOX_CMD_GROUP(CMD_GRP_HWMANIP)
 	BAREBOX_CMD_HELP(cmd_wd_help)
+	BAREBOX_CMD_COMPLETE(device_complete)
 BAREBOX_CMD_END
-- 
2.23.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/6] Documentation: efi: emphasize watchdog deactivation on ExitBootServices

[Oleksij Rempel]

On Wed, Oct 23, 2019 at 06:55:56PM +0200, Ahmad Fatoum wrote:
> The UEFI specification paragraph quoted above notes:
> > The watchdog timer is only used during boot services. On successful
> > completion of ExitBootServices() the watchdog timer is disabled.
> 
> Thus disabling the watchdog is _the_ only proper behavior. Adjust the
> wording accordingly.
> 
> Cc: Oleksij Rempel <o.rempel@pengutronix.de>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  Documentation/boards/efi.rst | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/boards/efi.rst b/Documentation/boards/efi.rst
> index 2178c9ab4293..f04b1d32378b 100644
> --- a/Documentation/boards/efi.rst
> +++ b/Documentation/boards/efi.rst
> @@ -350,7 +350,7 @@ https://uefi.org/sites/default/files/resources/UEFI_Spec_2_1_D.pdf
>  
>  Current linux kernel (v5.0) will execute ExitBootServices() during the early
>  boot stage and thus will automatically disable the (U)EFI watchdog. Since it is
> -a proper behavior according to the (U)EFI specification, it is impossible to
> +the proper behavior according to the (U)EFI specification, it is impossible to
>  protect full boot chain by using this watchdog only. It is recommended to use
>  an alternative hardware watchdog, preferably started before the bootloader. If (U)EFI
>  firmware lacks this feature, the bootloader should be able to start an alternative
> -- 
> 2.23.0
> 
> 

Reviewed-by: <o.rempel@pengutronix.de>

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/6] Documentation: efi: emphasize watchdog deactivation on ExitBootServices

[Sascha Hauer]

On Wed, Oct 23, 2019 at 06:55:56PM +0200, Ahmad Fatoum wrote:
> The UEFI specification paragraph quoted above notes:
> > The watchdog timer is only used during boot services. On successful
> > completion of ExitBootServices() the watchdog timer is disabled.
> 
> Thus disabling the watchdog is _the_ only proper behavior. Adjust the
> wording accordingly.
> 
> Cc: Oleksij Rempel <o.rempel@pengutronix.de>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  Documentation/boards/efi.rst | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied, thanks

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox