Re: [PATCH 2/2] boot: introduce option to pass barebox-enabled watchdog to systemd

[Sascha Hauer <s.hauer@pengutronix.de>]

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