Re: [PATCH 2/2] watchdog: gpio_wdt: add "start-at-boot" feature

[Guenter Roeck <linux@roeck-us.net>]

On Thu, Apr 22, 2021 at 06:28:40PM +0200, Francesco Zanella wrote:
> 
> 
> On 21/04/21 18:42, Guenter Roeck wrote:
> > On Wed, Apr 21, 2021 at 06:26:21PM +0200, Francesco Zanella wrote:
> >> If "start-at-boot" property is present in the device tree, start pinging
> >> hw watchdog at probe, in order to take advantage of kernel configs:
> >> - WATCHDOG_HANDLE_BOOT_ENABLED: Avoid possible reboot if hw watchdog was
> >>   been enabled before the kernel (by uboot for example) and userspace
> >>   doesn't take control of /dev/watchdog in time;
> >> - WATCHDOG_OPEN_TIMEOUT: Reboot if userspace doesn't take control of
> >>   /dev/watchdog within the timeout.
> >>
> >> Signed-off-by: Francesco Zanella <francesco.zanella@vimar.com>
> >> ---
> >>  drivers/watchdog/gpio_wdt.c | 6 +++++-
> >>  1 file changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
> >> index 0923201ce874..1e6f0322ab7a 100644
> >> --- a/drivers/watchdog/gpio_wdt.c
> >> +++ b/drivers/watchdog/gpio_wdt.c
> >> @@ -31,6 +31,7 @@ struct gpio_wdt_priv {
> >>  	struct gpio_desc	*gpiod;
> >>  	bool			state;
> >>  	bool			always_running;
> >> +	bool			start_at_boot;
> >>  	unsigned int		hw_algo;
> >>  	struct watchdog_device	wdd;
> >>  };
> >> @@ -147,6 +148,9 @@ static int gpio_wdt_probe(struct platform_device *pdev)
> >>  	priv->always_running = of_property_read_bool(np,
> >>  						     "always-running");
> >>  
> >> +	priv->start_at_boot = of_property_read_bool(np,
> >> +						    "start-at-boot");
> >> +
> >>  	watchdog_set_drvdata(&priv->wdd, priv);
> >>  
> >>  	priv->wdd.info		= &gpio_wdt_ident;
> >> @@ -161,7 +165,7 @@ static int gpio_wdt_probe(struct platform_device *pdev)
> >>  
> >>  	watchdog_stop_on_reboot(&priv->wdd);
> >>  
> >> -	if (priv->always_running)
> >> +	if (priv->always_running || priv->start_at_boot)
> >>  		gpio_wdt_start(&priv->wdd);
> > 
> > So the only real difference to always_running is that always_running
> > doesn't stop the watchdog on close but keeps it running.
> > 
> > Does that really warrant another property ? Why not just use
> > always-running ?
> > 
> > The special use case of being able to stop the watchdog doesn't seem
> > to be worth the trouble. Please explain your use case.
> > 
> > Guenter
> > 
> 
> You got the point.
> I would like to be able to stop the watchdog when I want.
> From my point of view always_running is used in the very special
> case when the wdt chip can't be stopped.
> I want a normal wdt that can be stopped (for example during a system
> update), but I want it to start at boot for the 2 reasons that I
> explained before:
> - I need continuity in hw wdt pinging because I start in uboot,
>   so I take advantage of WATCHDOG_HANDLE_BOOT_ENABLED that makes
>   the kernel to do that job; but it is triggered only if it is
>   started at probe, if I'm not wrong.

That depends. If the driver can read the current state (ie if
it can detect if the watchdog is running) it can report it
to the watchdog core accordingly. That would be the preferred
mechanism. Everything else is just a workaround for bad hardware
(bad as in "it doesn't report its state").

Guenter

> - I would like to monitor with the wdt also the kernel at boot,
>   and more importantly handle the case when the userspace app
>   doesn't take control of /dev/watchdog for whatever reason
>   within the timeout set with WATCHDOG_OPEN_TIMEOUT.
> 
> Hope that this explain my target.
> Thanks for your attention,
> 
> Francesco