| Message ID | 20190605140628.618-4-rasmus.villemoes@prevas.dk (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | watchdog: allow setting deadline for opening /dev/watchdogN | expand |
On Wed, Jun 05, 2019 at 02:06:44PM +0000, Rasmus Villemoes wrote: > When the watchdog device is not open by userspace, the kernel takes > care of pinging it. When the open_timeout feature is in use, we should > ensure that the hardware fires close to open_timeout seconds after the > kernel has assumed responsibility for the device. > > To do this, simply reuse the logic that is already in place for > ensuring the same thing when userspace is responsible for regularly > pinging the device: > > - When watchdog_active(wdd), this patch doesn't change anything. > > - When !watchdoc_active(wdd), the "virtual timeout" should be taken to s/watchdoc_active/watchdog_active/ otherwise Reviewed-by: Guenter Roeck <linux@roeck-us.net> > be ->open_deadline". When the open_timeout feature is not used or the > device has been opened at least once, ->open_deadline is KTIME_MAX, > and the arithmetic ends up returning keepalive_interval as we used to. > > This has been tested on a Wandboard with various combinations of > open_timeout and timeout-sec properties for the on-board watchdog by > booting with 'init=/bin/sh', timestamping the lines on the serial > console, and comparing the timestamp of the 'imx2-wdt 20bc000.wdog: > timeout nnn sec' line with the timestamp of the 'U-Boot SPL ...' > line (which appears just after reset). > > Suggested-by: Guenter Roeck <linux@roeck-us.net> > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > --- > drivers/watchdog/watchdog_dev.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c > index 334b810db2cf..edfb884044e0 100644 > --- a/drivers/watchdog/watchdog_dev.c > +++ b/drivers/watchdog/watchdog_dev.c > @@ -133,14 +133,15 @@ static ktime_t watchdog_next_keepalive(struct watchdog_device *wdd) > ktime_t virt_timeout; > unsigned int hw_heartbeat_ms; > > - virt_timeout = ktime_add(wd_data->last_keepalive, > - ms_to_ktime(timeout_ms)); > + if (watchdog_active(wdd)) > + virt_timeout = ktime_add(wd_data->last_keepalive, > + ms_to_ktime(timeout_ms)); > + else > + virt_timeout = wd_data->open_deadline; > + > hw_heartbeat_ms = min_not_zero(timeout_ms, wdd->max_hw_heartbeat_ms); > keepalive_interval = ms_to_ktime(hw_heartbeat_ms / 2); > > - if (!watchdog_active(wdd)) > - return keepalive_interval; > - > /* > * To ensure that the watchdog times out wdd->timeout seconds > * after the most recent ping from userspace, the last
On 07/06/2019 20.38, Guenter Roeck wrote: > On Wed, Jun 05, 2019 at 02:06:44PM +0000, Rasmus Villemoes wrote: >> When the watchdog device is not open by userspace, the kernel takes >> care of pinging it. When the open_timeout feature is in use, we should >> ensure that the hardware fires close to open_timeout seconds after the >> kernel has assumed responsibility for the device. >> >> To do this, simply reuse the logic that is already in place for >> ensuring the same thing when userspace is responsible for regularly >> pinging the device: >> >> - When watchdog_active(wdd), this patch doesn't change anything. >> >> - When !watchdoc_active(wdd), the "virtual timeout" should be taken to > > s/watchdoc_active/watchdog_active/ > > otherwise > > Reviewed-by: Guenter Roeck <linux@roeck-us.net> Thanks! Wim, can you fix up if/when applying, or do you prefer I resend? Rasmus
On 6/14/19 1:41 AM, Rasmus Villemoes wrote: > On 07/06/2019 20.38, Guenter Roeck wrote: >> On Wed, Jun 05, 2019 at 02:06:44PM +0000, Rasmus Villemoes wrote: >>> When the watchdog device is not open by userspace, the kernel takes >>> care of pinging it. When the open_timeout feature is in use, we should >>> ensure that the hardware fires close to open_timeout seconds after the >>> kernel has assumed responsibility for the device. >>> >>> To do this, simply reuse the logic that is already in place for >>> ensuring the same thing when userspace is responsible for regularly >>> pinging the device: >>> >>> - When watchdog_active(wdd), this patch doesn't change anything. >>> >>> - When !watchdoc_active(wdd), the "virtual timeout" should be taken to >> >> s/watchdoc_active/watchdog_active/ >> >> otherwise >> >> Reviewed-by: Guenter Roeck <linux@roeck-us.net> > > Thanks! Wim, can you fix up if/when applying, or do you prefer I resend? > I made the change when applying the patch to my watchdog-next branch, and Wim usually picks up patches from there, so we should be good. Thanks, Guenter
Hi Rasmus, > > On Wed, Jun 05, 2019 at 02:06:44PM +0000, Rasmus Villemoes wrote: > >> When the watchdog device is not open by userspace, the kernel takes > >> care of pinging it. When the open_timeout feature is in use, we should > >> ensure that the hardware fires close to open_timeout seconds after the > >> kernel has assumed responsibility for the device. > >> > >> To do this, simply reuse the logic that is already in place for > >> ensuring the same thing when userspace is responsible for regularly > >> pinging the device: > >> > >> - When watchdog_active(wdd), this patch doesn't change anything. > >> > >> - When !watchdoc_active(wdd), the "virtual timeout" should be taken to > > > > s/watchdoc_active/watchdog_active/ > > > > otherwise > > > > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > > Thanks! Wim, can you fix up if/when applying, or do you prefer I resend? I'll fix up when applying. No need to resend a new patch for that. Kind regards, Wim.
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c index 334b810db2cf..edfb884044e0 100644 --- a/drivers/watchdog/watchdog_dev.c +++ b/drivers/watchdog/watchdog_dev.c @@ -133,14 +133,15 @@ static ktime_t watchdog_next_keepalive(struct watchdog_device *wdd) ktime_t virt_timeout; unsigned int hw_heartbeat_ms; - virt_timeout = ktime_add(wd_data->last_keepalive, - ms_to_ktime(timeout_ms)); + if (watchdog_active(wdd)) + virt_timeout = ktime_add(wd_data->last_keepalive, + ms_to_ktime(timeout_ms)); + else + virt_timeout = wd_data->open_deadline; + hw_heartbeat_ms = min_not_zero(timeout_ms, wdd->max_hw_heartbeat_ms); keepalive_interval = ms_to_ktime(hw_heartbeat_ms / 2); - if (!watchdog_active(wdd)) - return keepalive_interval; - /* * To ensure that the watchdog times out wdd->timeout seconds * after the most recent ping from userspace, the last
When the watchdog device is not open by userspace, the kernel takes care of pinging it. When the open_timeout feature is in use, we should ensure that the hardware fires close to open_timeout seconds after the kernel has assumed responsibility for the device. To do this, simply reuse the logic that is already in place for ensuring the same thing when userspace is responsible for regularly pinging the device: - When watchdog_active(wdd), this patch doesn't change anything. - When !watchdoc_active(wdd), the "virtual timeout" should be taken to be ->open_deadline". When the open_timeout feature is not used or the device has been opened at least once, ->open_deadline is KTIME_MAX, and the arithmetic ends up returning keepalive_interval as we used to. This has been tested on a Wandboard with various combinations of open_timeout and timeout-sec properties for the on-board watchdog by booting with 'init=/bin/sh', timestamping the lines on the serial console, and comparing the timestamp of the 'imx2-wdt 20bc000.wdog: timeout nnn sec' line with the timestamp of the 'U-Boot SPL ...' line (which appears just after reset). Suggested-by: Guenter Roeck <linux@roeck-us.net> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> --- drivers/watchdog/watchdog_dev.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)