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@xxxxxxxxxxx> > > >>>> > > >>>> 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. > 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). > > 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 ? Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
