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

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

On Thu, 2017-05-18 at 09:58 -0700, Guenter Roeck wrote:
> On Thu, May 18, 2017 at 12:39:23PM -0400, Sylvain Lemieux wrote:
> > On Thu, 2017-05-18 at 06:50 -0700, Guenter Roeck wrote:
> > > On 05/18/2017 06:01 AM, Sylvain Lemieux wrote:
> > > > On Thu, 2017-03-30 at 10:46 -0400, Sylvain Lemieux wrote:
> > > >> Hi,
> > > >>
> > > >> On Thu, 2017-03-30 at 06:11 -0700, Guenter Roeck wrote:
> > > >>> On 03/14/2017 07:11 AM, 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.
> > > >>>>
> > > >>>
> > > >>> The description doesn't match what the code actually does, at least from
> > > >>> an infrastructure perspective. The infrastructure would just keep it running.
> > > >>>
> > > >> I will need to send a new version with an updated description;
> > > >>
> > > > I will submit v3 later today or tomorrow.
> > > > 
> > > >> I did not update the description after this patch was rebased on-top
> > > >> of the "watchdog: gpio: keepalives" patch.
> > > >>
> > > >>> What you are really asking for is something the infrastructure should possibly
> > > >>> do by itself automatically: To keep pinging a HW watchdog after close until
> > > >>> the configured (software) timeout period expires. This would be in line with
> > > >>> expectations.
> > > >>>
> > > > Do you want me to work on a generic version for this option?
> > > > 
> > > 
> > > I am not sure I understand the value of the current version (as implemented)
> > > in the first place. It seems to be similar to "always-running", with the exception
> > > that it doesn't start the watchdog immediately when loading the module. That means
> > > it protects the system against hard lockups, but only if the watchdog was opened
> > > at least once. That just seems odd, and you'll have to explain the benefit over
> > > "always-running", and why it would make sense to have such a selective protection.
> > > 
> > The only difference between this implementation and the "always-running"
> > is the way the close operation is handle; when "keep_armed_on_close"
> > option is selected, the watchdog will generate a timeout at the end
> > of the grace period.
> > 
> 
> Not as currently implemented, though.
> 
I will retest; this was the case before this patch was apply to
the GPIO watchdog (https://lkml.org/lkml/2016/2/28/239).

> > Regarding the loading of the module, we have a separate patch, that
> > is apply to the GPIO watchdog to perform an early start (same way as
> > "always-running"); this is not part of this change, as this change
> > only modify the behavior of the driver on close.
> > 
> > > Note that devicetree property changes need to be Acked by DT maintainers.
> > > 
> > I will cc them on the new patch.
> > 
> 
> There is no need for a devicetree property; this is a bug fix, not a feature
> (user space can expect a watchdog to expire only after the configured grace
> period expired).
> 
As stated earlier, I will retest and get back to you.

> > > Having said that, if what you want is what the description says, not what is
> > > implemented, I'll be happy to accept a patch to change the infrastructure
> > > accordingly.
> > > 
> > I will look into modifying the infrastructure to add the support
> > for "keep_running_on_close"; this will replace this patch.
> > 
> > I will need to submit my other patch for this driver to allow
> > an "early start" of the watchdog
> > (same as what the "always-running" is doing).
> > 
> Confused. If the functionality is already there, what would this patch do ?
> 
This functionality is there with the "always-running"; I need this
behavior on loading, but not the "always-running" behavior on close;

This patch is adding an option to start the watchdog at init
(i.e. loading).

> Thanks,
> Guenter