mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH 1/2] watchdog: implement watchdog_get_alias_id_from
@ 2020-11-20 14:06 Ahmad Fatoum
  2020-11-20 14:06 ` [PATCH 2/2] boot: introduce option to pass barebox-enabled watchdog to systemd Ahmad Fatoum
  2020-11-24  8:08 ` [PATCH 1/2] watchdog: implement watchdog_get_alias_id_from Sascha Hauer
  0 siblings, 2 replies; 7+ messages in thread
From: Ahmad Fatoum @ 2020-11-20 14:06 UTC (permalink / raw)
  To: barebox; +Cc: Ahmad Fatoum

On device-tree enabled platforms, the Linux kernel will first attempt
to use watchdog%d as watchdog name, where %d is the alias id.

Add a function that given a barebox struct watchdog and the device tree
root node of the kernel device tree, computes the corresponding kernel
alias id.

This may then later be used to pass an appropriate argument on the
kernel command line.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/of/base.c          | 77 ++++++++++++++++++++++++++++++--------
 drivers/watchdog/wd_core.c | 19 ++++++++++
 include/of.h               |  8 ++++
 include/watchdog.h         |  8 ++++
 4 files changed, 97 insertions(+), 15 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 5b45c2023f3b..85a94f6ef159 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -149,6 +149,31 @@ static void of_alias_add(struct alias_prop *ap, struct device_node *np,
 		 ap->alias, ap->stem, ap->id, np->full_name);
 }
 
+static struct device_node *of_alias_resolve(struct device_node *root, struct property *pp)
+{
+	/* Skip those we do not want to proceed */
+	if (!of_prop_cmp(pp->name, "name") ||
+	    !of_prop_cmp(pp->name, "phandle") ||
+	    !of_prop_cmp(pp->name, "linux,phandle"))
+		return NULL;
+
+	return of_find_node_by_path_from(root, of_property_get_value(pp));
+}
+
+static int of_alias_id_parse(const char *start, int *len)
+{
+	const char *end = start + strlen(start);
+
+	/* walk the alias backwards to extract the id and work out
+	 * the 'stem' string */
+	while (isdigit(*(end-1)) && end > start)
+		end--;
+
+	*len = end - start;
+
+	return simple_strtol(end, NULL, 10);
+}
+
 /**
  * of_alias_scan - Scan all properties of 'aliases' node
  *
@@ -175,28 +200,15 @@ void of_alias_scan(void)
 
 	list_for_each_entry(pp, &of_aliases->properties, list) {
 		const char *start = pp->name;
-		const char *end = start + strlen(start);
 		struct device_node *np;
 		struct alias_prop *ap;
 		int id, len;
 
-		/* Skip those we do not want to proceed */
-		if (!of_prop_cmp(pp->name, "name") ||
-		    !of_prop_cmp(pp->name, "phandle") ||
-		    !of_prop_cmp(pp->name, "linux,phandle"))
-			continue;
-
-		np = of_find_node_by_path(of_property_get_value(pp));
+		np = of_alias_resolve(root_node, pp);
 		if (!np)
 			continue;
 
-		/* walk the alias backwards to extract the id and work out
-		 * the 'stem' string */
-		while (isdigit(*(end-1)) && end > start)
-			end--;
-		len = end - start;
-
-		id = simple_strtol(end, NULL, 10);
+		id = of_alias_id_parse(start, &len);
 		if (id < 0)
 			continue;
 
@@ -235,6 +247,41 @@ int of_alias_get_id(struct device_node *np, const char *stem)
 }
 EXPORT_SYMBOL_GPL(of_alias_get_id);
 
+extern int of_alias_get_id_from(struct device_node *root, struct device_node *np,
+				const char *stem)
+{
+	struct device_node *aliasnp, *entrynp;
+	struct property *pp;
+
+	if (!root)
+		return of_alias_get_id(np, stem);
+
+	aliasnp = of_find_node_by_path_from(root, "/aliases");
+	if (!aliasnp)
+		return -ENODEV;
+
+	for_each_property_of_node(aliasnp, pp) {
+		const char *start = pp->name;
+		int id, len;
+
+		entrynp = of_alias_resolve(root_node, pp);
+		if (entrynp != np)
+			continue;
+
+		id = of_alias_id_parse(start, &len);
+		if (id < 0)
+			continue;
+
+		if (strncasecmp(start, stem, len))
+			continue;
+
+		return id;
+	}
+
+	return -ENODEV;
+}
+EXPORT_SYMBOL_GPL(of_alias_get_id_from);
+
 const char *of_alias_get(struct device_node *np)
 {
 	struct alias_prop *app;
diff --git a/drivers/watchdog/wd_core.c b/drivers/watchdog/wd_core.c
index 665338af6197..643c53268fc8 100644
--- a/drivers/watchdog/wd_core.c
+++ b/drivers/watchdog/wd_core.c
@@ -261,6 +261,25 @@ struct watchdog *watchdog_get_default(void)
 }
 EXPORT_SYMBOL(watchdog_get_default);
 
+int watchdog_get_alias_id_from(struct watchdog *wd, struct device_node *root)
+{
+	struct device_node *dstnp, *srcnp = wd->hwdev ? wd->hwdev->device_node : NULL;
+	char *name;
+
+	if (!srcnp)
+		return -ENODEV;
+
+	name = of_get_reproducible_name(srcnp);
+	dstnp = of_find_node_by_reproducible_name(root, name);
+	free(name);
+
+	if (!dstnp)
+		return -ENODEV;
+
+	return of_alias_get_id_from(root, wd->hwdev->device_node, "watchdog");
+}
+EXPORT_SYMBOL(watchdog_get_alias_id_from);
+
 struct watchdog *watchdog_get_by_name(const char *name)
 {
 	struct watchdog *tmp;
diff --git a/include/of.h b/include/of.h
index 08a02e110522..dc3aa0730bf7 100644
--- a/include/of.h
+++ b/include/of.h
@@ -254,6 +254,8 @@ extern int of_count_phandle_with_args(const struct device_node *np,
 
 extern void of_alias_scan(void);
 extern int of_alias_get_id(struct device_node *np, const char *stem);
+extern int of_alias_get_id_from(struct device_node *root, struct device_node *np,
+				const char *stem);
 extern const char *of_alias_get(struct device_node *np);
 extern int of_modalias_node(struct device_node *node, char *modalias, int len);
 
@@ -677,6 +679,12 @@ static inline int of_alias_get_id(struct device_node *np, const char *stem)
 	return -ENOSYS;
 }
 
+static inline int of_alias_get_id_from(struct device_node *root, struct device_node *np,
+				       const char *stem)
+{
+	return -ENOSYS
+}
+
 static inline const char *of_alias_get(struct device_node *np)
 {
 	return NULL;
diff --git a/include/watchdog.h b/include/watchdog.h
index 81414ef8ecaa..ee8efdecd030 100644
--- a/include/watchdog.h
+++ b/include/watchdog.h
@@ -13,6 +13,8 @@ enum wdog_hw_runnning {
 	 WDOG_HW_NOT_RUNNING = PARAM_TRISTATE_FALSE
 };
 
+struct device_node;
+
 struct watchdog {
 	int (*set_timeout)(struct watchdog *, unsigned);
 	const char *name;
@@ -44,6 +46,7 @@ int watchdog_register(struct watchdog *);
 int watchdog_deregister(struct watchdog *);
 struct watchdog *watchdog_get_default(void);
 struct watchdog *watchdog_get_by_name(const char *name);
+int watchdog_get_alias_id_from(struct watchdog *, struct device_node *);
 int watchdog_set_timeout(struct watchdog*, unsigned);
 int watchdog_inhibit_all(void);
 #else
@@ -76,6 +79,11 @@ static inline int watchdog_inhibit_all(void)
 {
 	return -ENOSYS;
 }
+
+static inline int watchdog_get_alias_id_from(struct watchdog *, struct device_node *)
+{
+	return -ENOSYS;
+}
 #endif
 
 #define WATCHDOG_DEFAULT_PRIORITY 100
-- 
2.29.2


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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 2/2] boot: introduce option to pass barebox-enabled watchdog to systemd
  2020-11-20 14:06 [PATCH 1/2] watchdog: implement watchdog_get_alias_id_from Ahmad Fatoum
@ 2020-11-20 14:06 ` Ahmad Fatoum
  2020-11-24  8:13   ` [PATCH] fixup! watchdog: implement watchdog_get_alias_id_from Ahmad Fatoum
  2020-11-24  8:23   ` [PATCH 2/2] boot: introduce option to pass barebox-enabled watchdog to systemd Sascha Hauer
  2020-11-24  8:08 ` [PATCH 1/2] watchdog: implement watchdog_get_alias_id_from Sascha Hauer
  1 sibling, 2 replies; 7+ messages in thread
From: Ahmad Fatoum @ 2020-11-20 14:06 UTC (permalink / raw)
  To: barebox; +Cc: Ahmad Fatoum

Like Linux, barebox supports co-existence of multiple watchdog
devices. On boot, barebox enables only the default watchdog, which
is defined as the watchdog with highest non-zero priority.

The kernel handles all watchdogs the same and defers to userspace,
which watchdogs to service. It can be useful to have barebox tell
the system, which watchdog it activated, so it can service the same.

Having this feature behind a global variable adds 567 bytes to a
LZO compressed THUMB2 barebox. Allow users to opt out by having
a Kconfig option instead.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/Kconfig | 10 ++++++++++
 common/boot.c  | 42 +++++++++++++++++++++++++++++++++++++++---
 2 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/common/Kconfig b/common/Kconfig
index 9b73aa84549c..3cb43a7190bb 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -1006,6 +1006,16 @@ config MACHINE_ID
 	  Note: if no hashable information is available no machine id will be passed
 	  to the kernel.
 
+config SYSTEMD_OF_WATCHDOG
+	bool "inform devicetree-enabled kernel of used watchdog"
+	depends on WATCHDOG && OFTREE && FLEXIBLE_BOOTARGS
+	help
+	  Sets the linux.bootargs.dyn.watchdog global variable with a value of
+	  systemd.watchdog-device=/dev/WDOG if barebox succeeded in enabling
+	  the watchdog WDOG prior to boot. WDOG is the alias of the watchdog
+	  in the kernel device tree. If the kernel is booted without a device
+	  tree or with one that lacks aliases, nothing is added.
+
 menu "OP-TEE loading"
 
 config OPTEE_SIZE
diff --git a/common/boot.c b/common/boot.c
index 90d504e3c324..76d03c26c4f4 100644
--- a/common/boot.c
+++ b/common/boot.c
@@ -139,6 +139,39 @@ late_initcall(init_boot);
 BAREBOX_MAGICVAR(global.boot.watchdog_timeout,
 		"Watchdog enable timeout in seconds before booting");
 
+static struct watchdog *__watchdog;
+
+static int watchdog_of_fixup(struct device_node *root, void *arg)
+{
+	int alias_id;
+	char *buf;
+
+	if (!__watchdog)
+		return 0;
+
+	alias_id = watchdog_get_alias_id_from(__watchdog, root);
+	if (alias_id < 0)
+		return 0;
+
+	buf = basprintf("systemd.watchdog-device=/dev/watchdog%d", alias_id);
+	if (!buf)
+		return 0;
+
+	globalvar_add_simple("linux.bootargs.dyn.watchdog", buf);
+	free(buf);
+
+	return 0;
+}
+
+static int __maybe_unused of_register_watchdog_fixup(void)
+{
+	return of_register_fixup(watchdog_of_fixup, NULL);
+}
+#ifdef CONFIG_SYSTEMD_OF_WATCHDOG
+/* _must_ not be run after late_initcall(of_register_bootargs_fixup) */
+device_initcall(of_register_watchdog_fixup);
+#endif
+
 int boot_entry(struct bootentry *be, int verbose, int dryrun)
 {
 	int ret;
@@ -146,10 +179,13 @@ 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(watchdog_get_default(),
-					   boot_watchdog_timeout);
-		if (ret)
+		__watchdog = watchdog_get_default();
+
+		ret = watchdog_set_timeout(__watchdog, boot_watchdog_timeout);
+		if (ret) {
 			pr_warn("Failed to enable watchdog: %s\n", strerror(-ret));
+			__watchdog = NULL;
+		}
 	}
 
 	ret = be->boot(be, verbose, dryrun);
-- 
2.29.2


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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] watchdog: implement watchdog_get_alias_id_from
  2020-11-20 14:06 [PATCH 1/2] watchdog: implement watchdog_get_alias_id_from Ahmad Fatoum
  2020-11-20 14:06 ` [PATCH 2/2] boot: introduce option to pass barebox-enabled watchdog to systemd Ahmad Fatoum
@ 2020-11-24  8:08 ` Sascha Hauer
  1 sibling, 0 replies; 7+ messages in thread
From: Sascha Hauer @ 2020-11-24  8:08 UTC (permalink / raw)
  To: Ahmad Fatoum; +Cc: barebox

On Fri, Nov 20, 2020 at 03:06:48PM +0100, Ahmad Fatoum wrote:
> On device-tree enabled platforms, the Linux kernel will first attempt
> to use watchdog%d as watchdog name, where %d is the alias id.
> 
> Add a function that given a barebox struct watchdog and the device tree
> root node of the kernel device tree, computes the corresponding kernel
> alias id.
> 
> This may then later be used to pass an appropriate argument on the
> kernel command line.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  drivers/of/base.c          | 77 ++++++++++++++++++++++++++++++--------
>  drivers/watchdog/wd_core.c | 19 ++++++++++
>  include/of.h               |  8 ++++
>  include/watchdog.h         |  8 ++++
>  4 files changed, 97 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 5b45c2023f3b..85a94f6ef159 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -149,6 +149,31 @@ static void of_alias_add(struct alias_prop *ap, struct device_node *np,
>  		 ap->alias, ap->stem, ap->id, np->full_name);
>  }
>  
> +static struct device_node *of_alias_resolve(struct device_node *root, struct property *pp)
> +{
> +	/* Skip those we do not want to proceed */
> +	if (!of_prop_cmp(pp->name, "name") ||
> +	    !of_prop_cmp(pp->name, "phandle") ||
> +	    !of_prop_cmp(pp->name, "linux,phandle"))
> +		return NULL;
> +
> +	return of_find_node_by_path_from(root, of_property_get_value(pp));
> +}
> +
> +static int of_alias_id_parse(const char *start, int *len)
> +{
> +	const char *end = start + strlen(start);
> +
> +	/* walk the alias backwards to extract the id and work out
> +	 * the 'stem' string */
> +	while (isdigit(*(end-1)) && end > start)
> +		end--;
> +
> +	*len = end - start;
> +
> +	return simple_strtol(end, NULL, 10);
> +}
> +
>  /**
>   * of_alias_scan - Scan all properties of 'aliases' node
>   *
> @@ -175,28 +200,15 @@ void of_alias_scan(void)
>  
>  	list_for_each_entry(pp, &of_aliases->properties, list) {
>  		const char *start = pp->name;
> -		const char *end = start + strlen(start);
>  		struct device_node *np;
>  		struct alias_prop *ap;
>  		int id, len;
>  
> -		/* Skip those we do not want to proceed */
> -		if (!of_prop_cmp(pp->name, "name") ||
> -		    !of_prop_cmp(pp->name, "phandle") ||
> -		    !of_prop_cmp(pp->name, "linux,phandle"))
> -			continue;
> -
> -		np = of_find_node_by_path(of_property_get_value(pp));
> +		np = of_alias_resolve(root_node, pp);
>  		if (!np)
>  			continue;
>  
> -		/* walk the alias backwards to extract the id and work out
> -		 * the 'stem' string */
> -		while (isdigit(*(end-1)) && end > start)
> -			end--;
> -		len = end - start;
> -
> -		id = simple_strtol(end, NULL, 10);
> +		id = of_alias_id_parse(start, &len);
>  		if (id < 0)
>  			continue;
>  
> @@ -235,6 +247,41 @@ int of_alias_get_id(struct device_node *np, const char *stem)
>  }
>  EXPORT_SYMBOL_GPL(of_alias_get_id);
>  
> +extern int of_alias_get_id_from(struct device_node *root, struct device_node *np,
> +				const char *stem)

extern?

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH] fixup! watchdog: implement watchdog_get_alias_id_from
  2020-11-20 14:06 ` [PATCH 2/2] boot: introduce option to pass barebox-enabled watchdog to systemd Ahmad Fatoum
@ 2020-11-24  8:13   ` Ahmad Fatoum
  2020-11-24  8:23   ` [PATCH 2/2] boot: introduce option to pass barebox-enabled watchdog to systemd Sascha Hauer
  1 sibling, 0 replies; 7+ messages in thread
From: Ahmad Fatoum @ 2020-11-24  8:13 UTC (permalink / raw)
  To: sha; +Cc: barebox, Ahmad Fatoum

Fix (benign) copy pasta from <of.h>.

Cc: barebox@lists.infradead.org
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/of/base.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 85a94f6ef159..acbae3d8c4bb 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -247,8 +247,8 @@ int of_alias_get_id(struct device_node *np, const char *stem)
 }
 EXPORT_SYMBOL_GPL(of_alias_get_id);
 
-extern int of_alias_get_id_from(struct device_node *root, struct device_node *np,
-				const char *stem)
+int of_alias_get_id_from(struct device_node *root, struct device_node *np,
+			 const char *stem)
 {
 	struct device_node *aliasnp, *entrynp;
 	struct property *pp;
-- 
2.29.2


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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] boot: introduce option to pass barebox-enabled watchdog to systemd
  2020-11-20 14:06 ` [PATCH 2/2] boot: introduce option to pass barebox-enabled watchdog to systemd Ahmad Fatoum
  2020-11-24  8:13   ` [PATCH] fixup! watchdog: implement watchdog_get_alias_id_from Ahmad Fatoum
@ 2020-11-24  8:23   ` Sascha Hauer
  2020-11-24  8:26     ` Ahmad Fatoum
  1 sibling, 1 reply; 7+ messages in thread
From: Sascha Hauer @ 2020-11-24  8:23 UTC (permalink / raw)
  To: Ahmad Fatoum; +Cc: barebox

On Fri, Nov 20, 2020 at 03:06:49PM +0100, Ahmad Fatoum wrote:
> Like Linux, barebox supports co-existence of multiple watchdog
> devices. On boot, barebox enables only the default watchdog, which
> is defined as the watchdog with highest non-zero priority.
> 
> The kernel handles all watchdogs the same and defers to userspace,
> which watchdogs to service. It can be useful to have barebox tell
> the system, which watchdog it activated, so it can service the same.
> 
> Having this feature behind a global variable adds 567 bytes to a
> LZO compressed THUMB2 barebox. Allow users to opt out by having
> a Kconfig option instead.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  common/Kconfig | 10 ++++++++++
>  common/boot.c  | 42 +++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 49 insertions(+), 3 deletions(-)
> 
> diff --git a/common/Kconfig b/common/Kconfig
> index 9b73aa84549c..3cb43a7190bb 100644
> --- a/common/Kconfig
> +++ b/common/Kconfig
> @@ -1006,6 +1006,16 @@ config MACHINE_ID
>  	  Note: if no hashable information is available no machine id will be passed
>  	  to the kernel.
>  
> +config SYSTEMD_OF_WATCHDOG
> +	bool "inform devicetree-enabled kernel of used watchdog"
> +	depends on WATCHDOG && OFTREE && FLEXIBLE_BOOTARGS
> +	help
> +	  Sets the linux.bootargs.dyn.watchdog global variable with a value of
> +	  systemd.watchdog-device=/dev/WDOG if barebox succeeded in enabling
> +	  the watchdog WDOG prior to boot. WDOG is the alias of the watchdog
> +	  in the kernel device tree. If the kernel is booted without a device
> +	  tree or with one that lacks aliases, nothing is added.
> +
>  menu "OP-TEE loading"
>  
>  config OPTEE_SIZE
> diff --git a/common/boot.c b/common/boot.c
> index 90d504e3c324..76d03c26c4f4 100644
> --- a/common/boot.c
> +++ b/common/boot.c
> @@ -139,6 +139,39 @@ late_initcall(init_boot);
>  BAREBOX_MAGICVAR(global.boot.watchdog_timeout,
>  		"Watchdog enable timeout in seconds before booting");
>  
> +static struct watchdog *__watchdog;

The name looks like you have something to hide. Perhaps boot_enabled_watchdog?

> +
> +static int watchdog_of_fixup(struct device_node *root, void *arg)
> +{
> +	int alias_id;
> +	char *buf;
> +
> +	if (!__watchdog)
> +		return 0;
> +
> +	alias_id = watchdog_get_alias_id_from(__watchdog, root);
> +	if (alias_id < 0)
> +		return 0;
> +
> +	buf = basprintf("systemd.watchdog-device=/dev/watchdog%d", alias_id);
> +	if (!buf)
> +		return 0;
> +
> +	globalvar_add_simple("linux.bootargs.dyn.watchdog", buf);
> +	free(buf);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused of_register_watchdog_fixup(void)
> +{
> +	return of_register_fixup(watchdog_of_fixup, NULL);
> +}
> +#ifdef CONFIG_SYSTEMD_OF_WATCHDOG
> +/* _must_ not be run after late_initcall(of_register_bootargs_fixup) */

I smell problems here. How about calling this directly from
of_fixup_bootargs()?

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] boot: introduce option to pass barebox-enabled watchdog to systemd
  2020-11-24  8:23   ` [PATCH 2/2] boot: introduce option to pass barebox-enabled watchdog to systemd Sascha Hauer
@ 2020-11-24  8:26     ` Ahmad Fatoum
  2020-11-24  8:28       ` Sascha Hauer
  0 siblings, 1 reply; 7+ messages in thread
From: Ahmad Fatoum @ 2020-11-24  8:26 UTC (permalink / raw)
  To: Sascha Hauer; +Cc: barebox

Hi,

On 24.11.20 09:23, Sascha Hauer wrote:
>> +static struct watchdog *__watchdog;
> 
> The name looks like you have something to hide. Perhaps boot_enabled_watchdog?

Can do.

>> +
>> +static int watchdog_of_fixup(struct device_node *root, void *arg)
>> +{
>> +	int alias_id;
>> +	char *buf;
>> +
>> +	if (!__watchdog)
>> +		return 0;
>> +
>> +	alias_id = watchdog_get_alias_id_from(__watchdog, root);
>> +	if (alias_id < 0)
>> +		return 0;
>> +
>> +	buf = basprintf("systemd.watchdog-device=/dev/watchdog%d", alias_id);
>> +	if (!buf)
>> +		return 0;
>> +
>> +	globalvar_add_simple("linux.bootargs.dyn.watchdog", buf);
>> +	free(buf);
>> +
>> +	return 0;
>> +}
>> +
>> +static int __maybe_unused of_register_watchdog_fixup(void)
>> +{
>> +	return of_register_fixup(watchdog_of_fixup, NULL);
>> +}
>> +#ifdef CONFIG_SYSTEMD_OF_WATCHDOG
>> +/* _must_ not be run after late_initcall(of_register_bootargs_fixup) */
> 
> I smell problems here. How about calling this directly from
> of_fixup_bootargs()?

Feels wrong to have watchdog specific stuff over there.

> 
> Sascha
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] boot: introduce option to pass barebox-enabled watchdog to systemd
  2020-11-24  8:26     ` Ahmad Fatoum
@ 2020-11-24  8:28       ` Sascha Hauer
  0 siblings, 0 replies; 7+ messages in thread
From: Sascha Hauer @ 2020-11-24  8:28 UTC (permalink / raw)
  To: Ahmad Fatoum; +Cc: barebox

On Tue, Nov 24, 2020 at 09:26:32AM +0100, Ahmad Fatoum wrote:
> Hi,
> 
> On 24.11.20 09:23, Sascha Hauer wrote:
> >> +static struct watchdog *__watchdog;
> > 
> > The name looks like you have something to hide. Perhaps boot_enabled_watchdog?
> 
> Can do.
> 
> >> +
> >> +static int watchdog_of_fixup(struct device_node *root, void *arg)
> >> +{
> >> +	int alias_id;
> >> +	char *buf;
> >> +
> >> +	if (!__watchdog)
> >> +		return 0;
> >> +
> >> +	alias_id = watchdog_get_alias_id_from(__watchdog, root);
> >> +	if (alias_id < 0)
> >> +		return 0;
> >> +
> >> +	buf = basprintf("systemd.watchdog-device=/dev/watchdog%d", alias_id);
> >> +	if (!buf)
> >> +		return 0;
> >> +
> >> +	globalvar_add_simple("linux.bootargs.dyn.watchdog", buf);
> >> +	free(buf);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int __maybe_unused of_register_watchdog_fixup(void)
> >> +{
> >> +	return of_register_fixup(watchdog_of_fixup, NULL);
> >> +}
> >> +#ifdef CONFIG_SYSTEMD_OF_WATCHDOG
> >> +/* _must_ not be run after late_initcall(of_register_bootargs_fixup) */
> > 
> > I smell problems here. How about calling this directly from
> > of_fixup_bootargs()?
> 
> Feels wrong to have watchdog specific stuff over there.

Also feels wrong to put code dependencies into comments.

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-11-24  8:28 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-20 14:06 [PATCH 1/2] watchdog: implement watchdog_get_alias_id_from Ahmad Fatoum
2020-11-20 14:06 ` [PATCH 2/2] boot: introduce option to pass barebox-enabled watchdog to systemd Ahmad Fatoum
2020-11-24  8:13   ` [PATCH] fixup! watchdog: implement watchdog_get_alias_id_from Ahmad Fatoum
2020-11-24  8:23   ` [PATCH 2/2] boot: introduce option to pass barebox-enabled watchdog to systemd Sascha Hauer
2020-11-24  8:26     ` Ahmad Fatoum
2020-11-24  8:28       ` Sascha Hauer
2020-11-24  8:08 ` [PATCH 1/2] watchdog: implement watchdog_get_alias_id_from Sascha Hauer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox