Re: [PATCH] watchdog: gpio: Add "keep-armed-on-close" feature

[Sylvain Lemieux <slemieux.tyco@gmail.com>]

On Tue, 2017-01-03 at 10:59 -0800, Guenter Roeck wrote:
> On Tue, Jan 03, 2017 at 10:20:50AM -0500, Sylvain Lemieux wrote:
> > On Tue, 2016-12-20 at 13:25 -0500, Sylvain Lemieux wrote:
> > > Hi Guenter,
> > > 
> > > On Thu, 2016-12-15 at 12:46 -0500, Sylvain Lemieux wrote:
> > > > Hi Guenter,
> > > > 
> > > > On Fri, 2016-12-09 at 13:07 -0800, Guenter Roeck wrote:
> > > > > On Wed, Dec 07, 2016 at 02:46:20PM -0500, Sylvain Lemieux wrote:
> > > > > > From: Sylvain Lemieux <slemieux@tycoint.com>
> > > > > > 
> > > > > > There is a need to allow a grace period after the watchdog software
> > > > > > client has closed. It could be used for syncing the filesystem or
> > > > > > allow graceful termination while still providing a hardware reset
> > > > > > in case the system has hung.
> > > > > > 
> > > > > > The "always-running" configuration from device-tree does not provide
> > > > > > this since it will automatically keep the hardware watchdog alive as
> > > > > > soon as the software client closes (i.e. keep toggling the GPIO line
> > > > > > regardless of the state of the soft part of the watchdog).
> > > > > > 
> > > > > > The "keep-armed-on-close" member in the GPIO watchdog implementation
> > > > > > indicates if an expired timeout should cause a reset.
> > > > > > 
> > > > > > This patch add a new "keep-armed-on-close" device-tree configuration
> > > > > > that will keep the watchdog "armed" until the next timeout period after
> > > > > > a close. During this period, the hardware watchdog is kept alive.
> > > > > > 
> > > > > > A software watchdog client that wants to provide a grace period,
> > > > > > before a hard reset, can set the timeout before properly closing.
> > > > > > 
> > > > > > Signed-off-by: Sylvain Lemieux <slemieux@tycoint.com>
> > > > > 
> > > > > Rather than changing the current gpio watchdog code, I would prefer to start
> > > > > with https://lkml.org/lkml/2016/1/26/517, which moves part of its code into
> > > > > the watchdog core - even more so here since I suspect that the suggested
> > > > > changes won't work anymore after the above patch is applied.
> > > > > 
> > > > I agree that we should apply the "keepalive" change for this driver
> > > > first; ref. https://lkml.org/lkml/2016/2/28/239 (version v8).
> > > > 
> > > [...]
> > > > 
> > > I took the "watchdog: gpio: keepalives" patch and rebase this
> > > patch on top of it.
> > > 
> > > We have a scenario that will open and close the GPIO watchdog
> > > device a few times during the user-space initialization. This
> > > scenario is working properly with this patch on top of v4.9.
> > > 
> > > With the "watchdog: gpio: keepalives", I am getting an error
> > > when the GPIO watchdog driver is open for the 3rd time
> > > during this initialization process.
> > > 
> > > This is a short summary of stack trace:
> > > ------------[ cut here ]------------
> > > WARNING: CPU: 0 PID: 808 at lib/kobject.c:597 kobject_get+0x6c/0xac
> > > kobject: '(null)' (c397f304): is not initialized, yet kobject_get()
> > > is being called.
> > > ...
> > > [<c01a97f8>] (kobject_get) from [<c00c1278>] (chrdev_open+0x54/0x21c)
> > > [<c00c1224>] (chrdev_open) from [<c00b9df0>] (do_dentry_open
> > > +0x188/0x2d8)
> > > ...
> > > [<c00cabcc>] (do_filp_open) from [<c00bb668>] (do_sys_open+0x114/0x1cc)
> > > [<c00bb554>] (do_sys_open) from [<c00bb74c>] (SyS_open+0x2c/0x30)
> > > [<c00bb720>] (SyS_open) from [<c000f840>] (ret_fast_syscall+0x0/0x38)
> > > [   16.807133] ---[ end trace 7d55a0fe58991d4c ]---
> > > [   16.807143] ------------[ cut here ]------------
> > > 
> > > Regards,
> > > Sylvain
> > > 
> > ping
> > 
> Busy, sorry.
> 
> Guenter
> 
Just doing a follow-up.

> > Sylvain
> > 
> > > > 
> > > > > Also, isn't this quite identical to nowayout ?
> > > > > 
> > > > Nowayout block the watchdog driver from being stop.
> > > > 
> > > > In this case, we want to stop the watchdog (i.e. close) and be able
> > > > to reopen it within the timeout period. If the driver is not reopen
> > > > within the timeout period, the board should reset.
> > > > 
> > > > Regards,
> > > > Sylvain
> > > > 
> > > > > Thanks,
> > > > > Guenter
> > > > > 
> > > > > > ---
> > > > > >  Documentation/devicetree/bindings/watchdog/gpio-wdt.txt | 3 +++
> > > > > >  drivers/watchdog/gpio_wdt.c                             | 6 ++++++
> > > > > >  2 files changed, 9 insertions(+)
> > > > > > 
> > > > > > diff --git a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
> > > > > > index 83d2814..2669735 100644
> > > > > > --- a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
> > > > > > +++ b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
> > > > > > @@ -17,6 +17,9 @@ Optional Properties:
> > > > > >  - always-running: If the watchdog timer cannot be disabled, add this flag to
> > > > > >    have the driver keep toggling the signal without a client. It will only cease
> > > > > >    to toggle the signal when the device is open and the timeout elapsed.
> > > > > > +- keep-armed-on-close: if the watchdog timer need to keep toggling the signal
> > > > > > +  when close, until the timeout elapsed, add this flag to have the driver
> > > > > > +  keep toggling the signal, until the timeout elapsed.
> > > > > >  - timeout-sec: Contains the watchdog timeout in seconds.
> > > > > >  - start-at-init: Start kicking watchdog as soon as driver is loaded.
> > > > > >  
> > > > > > diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
> > > > > > index ef9ab91..ba9091a 100644
> > > > > > --- a/drivers/watchdog/gpio_wdt.c
> > > > > > +++ b/drivers/watchdog/gpio_wdt.c
> > > > > > @@ -32,6 +32,7 @@ struct gpio_wdt_priv {
> > > > > >  	bool			active_low;
> > > > > >  	bool			state;
> > > > > >  	bool			always_running;
> > > > > > +	bool			keep_armed_on_close;
> > > > > >  	bool			armed;
> > > > > >  	unsigned int		hw_algo;
> > > > > >  	unsigned int		hw_margin;
> > > > > > @@ -72,6 +73,9 @@ static int gpio_wdt_stop(struct watchdog_device *wdd)
> > > > > >  {
> > > > > >  	struct gpio_wdt_priv *priv = watchdog_get_drvdata(wdd);
> > > > > >  
> > > > > > +	if(priv->keep_armed_on_close)
> > > > > > +		return 0;
> > > > > > +
> > > > > >  	priv->armed = false;
> > > > > >  	if (!priv->always_running) {
> > > > > >  		mod_timer(&priv->timer, 0);
> > > > > > @@ -210,6 +214,8 @@ static int gpio_wdt_probe(struct platform_device *pdev)
> > > > > >  
> > > > > >  	priv->always_running = of_property_read_bool(pdev->dev.of_node,
> > > > > >  						     "always-running");
> > > > > > +	priv->keep_armed_on_close = of_property_read_bool(pdev->dev.of_node,
> > > > > > +						 "keep-armed-on-close");
> > > > > >  
> > > > > >  	watchdog_set_drvdata(&priv->wdd, priv);
> > > > > >  
> > > > > > -- 
> > > > > > 1.8.3.1
> > > > > > 
> > > > 
> > > > 
> > > 
> > > 
> > 
> >