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@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. > 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 -- 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
