[PATCH v2] watchdog: make open-write-close-open functionality a config knob

[tito]

On Fri, 26 Mar 2021 10:31:56 +0100
Rasmus Villemoes <rasmus.villemoes at prevas.dk> wrote:

> The behaviour introduced by commit 31c765081dc4 ("watchdog: stop
> watchdog first on startup") causes warnings in the kernel log when the
> nowayout feature is enabled:
> 
> [   16.212184] watchdog: watchdog0: nowayout prevents watchdog being
> stopped! [   16.212196] watchdog: watchdog0: watchdog did not stop!
> 
> The latter may also appear by itself in case the watchdog is of the
> type that cannot be stopped once started (e.g. the common
> always-running gpio_wdt kind).
> 
> These warnings can be somewhat ominous and distracting, so allow
> configuring whether to use this open-write-close-open sequence rather
> than just open. Also saves a bit of .text when disabled:
> 
> function                                             old     new
> delta shutdown_on_signal                                    31
> 58     +27 watchdog_main
> 339     306     -33
> shutdown_watchdog                                     34       -
> -34
> ------------------------------------------------------------------------------
> (add/remove: 0/1 grow/shrink: 1/1 up/down: 27/-67)            Total:
> -40 bytes
> 
> Make it default n:
> 
> - It's a workaround for one specific type of watchdog (and
>   that seems to be a defect in the kernel driver)
> - Even when not enabled in busybox config, it can easily be
>   implemented outside busybox
> - Code size
> - Commit 31c765081dc4 should be considered a regression for all the
>   boards that now end up with KERN_CRIT warnings in dmesg.
> - The author of that commit said "This use case is evidently rare, so
>   if it is indeed causing problems for other people I'd OK then I
>   understand whatever needs to be done." in the v1 thread.
> 
> Cc: Matt Spinler <mspinler at linux.vnet.ibm.com>
> Cc: deweloper at wp.pl
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes at prevas.dk>
> ---
>  miscutils/watchdog.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/miscutils/watchdog.c b/miscutils/watchdog.c
> index 0ed10bcf1..c2ee06dcf 100644
> --- a/miscutils/watchdog.c
> +++ b/miscutils/watchdog.c
> @@ -18,6 +18,21 @@
>  //config:	watchdog applet ever fails to write the magic
> character within a //config:	certain amount of time, the
> watchdog device assumes the system has //config:	hung, and
> will cause the hardware to reboot. +//config:
> +//config:config FEATURE_WATCHDOG_OPEN_TWICE
> +//config:	bool "Open watchdog device twice, closing it
> gracefully in between" +//config:	depends on WATCHDOG
> +//config:	default n
> +//config:	help
> +//config:	When enabled, the watchdog device is opened and
> then immediately +//config:	magic-closed, before being opened
> a second time. This may be necessary +//config:	for some
> watchdog devices, but can cause spurious warnings in the
> +//config:	kernel log if the nowayout feature is enabled.
> Also, if this workaround +//config:	is really needed for you
> machine to work properly, consider whether +//config:	it
> should be fixed in the kernel driver instead. Even when disabled,
> +//config:	the behaviour is easily emulated with a "printf 'V'
> > /dev/watchdog" +//config:	immediately before starting the
> > busybox watchdog daemnn. Say n unless +//config:	you know

Hi,
typo here:

+//config:	immediately before starting the busybox watchdog
__daemnn___. Say n unless


Ciao,
Tito

> > that you absolutely need this. 
>  //applet:IF_WATCHDOG(APPLET(watchdog, BB_DIR_SBIN, BB_SUID_DROP))
>  
> @@ -73,6 +88,7 @@ static void watchdog_open(const char* device)
>  	/* Use known fd # - avoid needing global 'int fd' */
>  	xmove_fd(xopen(device, O_WRONLY), 3);
>  
> +#if ENABLE_FEATURE_WATCHDOG_OPEN_TWICE
>  	/* If the watchdog driver can do something other than cause
> a reboot
>  	 * on a timeout, then it's possible this program may be
> starting from
>  	 * a state when the watchdog hadn't been previously stopped
> with @@ -82,6 +98,7 @@ static void watchdog_open(const char* device)
>  	shutdown_watchdog();
>  
>  	xmove_fd(xopen(device, O_WRONLY), 3);
> +#endif
>  }
>  
>  int watchdog_main(int argc, char **argv) MAIN_EXTERNALLY_VISIBLE;