Re: [PATCH v2 3/8] watchdog: Introduce WDOG_RUNNING flag
On 08/14/2015 12:04 PM, Uwe Kleine-König wrote: Hello Guenter, diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt index 25b00b878a7b..6a54dc15a556 100644 --- a/Documentation/watchdog/watchdog-kernel-api.txt +++ b/Documentation/watchdog/watchdog-kernel-api.txt [...] @@ -193,9 +194,12 @@ they are supported. These optional routines/operations are: The status bits should (preferably) be set with the set_bit and clear_bit alike bit-operations. The status bits that are defined are: * WDOG_ACTIVE: this status bit indicates whether or not a watchdog timer device - is active or not. When the watchdog is active after booting, then you should - set this status bit (Note: when you register the watchdog timer device with - this bit set, then opening /dev/watchdog will skip the start operation) + is active or not from user perspective. User space is expected to send + heartbeat requests to the driver while this flag is set. If the watchdog + is active after booting, and you don't want the infrastructure to send + heartbeats to the watchdog driver, then you should set this status bit. IMHO this should not be the driver author's choice! If you implement policy in the kernel it should at least be implemented in the framework and preferably easily changeable. (At least with Kconfig, but better use a kernel parameter (or both, the latter overriding the former).) Agreed. I'll change that and simply drop that part. After all, we now have WDOG_RUNNING to indicate that the watchdog is running. I'll just have to make sure that there are no drivers which set WDOG_ACTIVE at boot (afaics there are none). + Note: when you register the watchdog timer device with this bit set, + then opening /dev/watchdog will skip the start operation. * WDOG_DEV_OPEN: this status bit shows whether or not the watchdog device was opened via /dev/watchdog. (This bit should only be used by the WatchDog Timer Driver Core). @@ -209,6 +213,11 @@ bit-operations. The status bits that are defined are: any watchdog_ops, so that you can be sure that no operations (other then unref) will get called after unregister, even if userspace still holds a reference to /dev/watchdog +* WDOG_RUNNING: Set by the watchdog driver if the hardware watchdog is running. + The bit must be set if the watchdog timer hardware can not be stopped. + The bit may also be set if the watchdog timer is running aftyer booting, + before the watchdog device is opened. If set, the watchdog infrastructure + will send keepalives to the watchdog hardware while WDOG_ACTIVE is not set. To set the WDOG_NO_WAY_OUT status bit (before registering your watchdog timer device) you can either: [...] diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c index c04ba1a98cc8..676e233d5e7b 100644 --- a/drivers/watchdog/watchdog_dev.c +++ b/drivers/watchdog/watchdog_dev.c @@ -59,7 +59,8 @@ static inline bool watchdog_need_worker(struct watchdog_device *wdd) unsigned int m = wdd->max_timeout * 1000; unsigned int t = wdd->timeout * 1000; - return watchdog_active(wdd) && hm && (!m || hm < m) && t > hm; + return (watchdog_active(wdd) && hm && (!m || hm < m) && t > hm) || + (t && !watchdog_active(wdd) && watchdog_running(wdd)); What is the meaning of !t && !watchdog_active(wdd) && watchdog_running(wdd) ? Can this happen at all? If not, drop "t && "? t can be 0, meaning "the watchdog timeout is unknown", unless a driver sets min_timeout to a value larger than 0. Unfortunately, that is not explicitly specified. Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 3/8] watchdog: Introduce WDOG_RUNNING flag
On 08/14/2015 12:04 PM, Uwe Kleine-König wrote: Hello Guenter, diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt index 25b00b878a7b..6a54dc15a556 100644 --- a/Documentation/watchdog/watchdog-kernel-api.txt +++ b/Documentation/watchdog/watchdog-kernel-api.txt [...] @@ -193,9 +194,12 @@ they are supported. These optional routines/operations are: The status bits should (preferably) be set with the set_bit and clear_bit alike bit-operations. The status bits that are defined are: * WDOG_ACTIVE: this status bit indicates whether or not a watchdog timer device - is active or not. When the watchdog is active after booting, then you should - set this status bit (Note: when you register the watchdog timer device with - this bit set, then opening /dev/watchdog will skip the start operation) + is active or not from user perspective. User space is expected to send + heartbeat requests to the driver while this flag is set. If the watchdog + is active after booting, and you don't want the infrastructure to send + heartbeats to the watchdog driver, then you should set this status bit. IMHO this should not be the driver author's choice! If you implement policy in the kernel it should at least be implemented in the framework and preferably easily changeable. (At least with Kconfig, but better use a kernel parameter (or both, the latter overriding the former).) Agreed. I'll change that and simply drop that part. After all, we now have WDOG_RUNNING to indicate that the watchdog is running. I'll just have to make sure that there are no drivers which set WDOG_ACTIVE at boot (afaics there are none). + Note: when you register the watchdog timer device with this bit set, + then opening /dev/watchdog will skip the start operation. * WDOG_DEV_OPEN: this status bit shows whether or not the watchdog device was opened via /dev/watchdog. (This bit should only be used by the WatchDog Timer Driver Core). @@ -209,6 +213,11 @@ bit-operations. The status bits that are defined are: any watchdog_ops, so that you can be sure that no operations (other then unref) will get called after unregister, even if userspace still holds a reference to /dev/watchdog +* WDOG_RUNNING: Set by the watchdog driver if the hardware watchdog is running. + The bit must be set if the watchdog timer hardware can not be stopped. + The bit may also be set if the watchdog timer is running aftyer booting, + before the watchdog device is opened. If set, the watchdog infrastructure + will send keepalives to the watchdog hardware while WDOG_ACTIVE is not set. To set the WDOG_NO_WAY_OUT status bit (before registering your watchdog timer device) you can either: [...] diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c index c04ba1a98cc8..676e233d5e7b 100644 --- a/drivers/watchdog/watchdog_dev.c +++ b/drivers/watchdog/watchdog_dev.c @@ -59,7 +59,8 @@ static inline bool watchdog_need_worker(struct watchdog_device *wdd) unsigned int m = wdd-max_timeout * 1000; unsigned int t = wdd-timeout * 1000; - return watchdog_active(wdd) hm (!m || hm m) t hm; + return (watchdog_active(wdd) hm (!m || hm m) t hm) || + (t !watchdog_active(wdd) watchdog_running(wdd)); What is the meaning of !t !watchdog_active(wdd) watchdog_running(wdd) ? Can this happen at all? If not, drop t ? t can be 0, meaning the watchdog timeout is unknown, unless a driver sets min_timeout to a value larger than 0. Unfortunately, that is not explicitly specified. Thanks, Guenter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 3/8] watchdog: Introduce WDOG_RUNNING flag
Hello Guenter, > diff --git a/Documentation/watchdog/watchdog-kernel-api.txt > b/Documentation/watchdog/watchdog-kernel-api.txt > index 25b00b878a7b..6a54dc15a556 100644 > --- a/Documentation/watchdog/watchdog-kernel-api.txt > +++ b/Documentation/watchdog/watchdog-kernel-api.txt > [...] > @@ -193,9 +194,12 @@ they are supported. These optional routines/operations > are: > The status bits should (preferably) be set with the set_bit and clear_bit > alike > bit-operations. The status bits that are defined are: > * WDOG_ACTIVE: this status bit indicates whether or not a watchdog timer > device > - is active or not. When the watchdog is active after booting, then you > should > - set this status bit (Note: when you register the watchdog timer device with > - this bit set, then opening /dev/watchdog will skip the start operation) > + is active or not from user perspective. User space is expected to send > + heartbeat requests to the driver while this flag is set. If the watchdog > + is active after booting, and you don't want the infrastructure to send > + heartbeats to the watchdog driver, then you should set this status bit. IMHO this should not be the driver author's choice! If you implement policy in the kernel it should at least be implemented in the framework and preferably easily changeable. (At least with Kconfig, but better use a kernel parameter (or both, the latter overriding the former).) > + Note: when you register the watchdog timer device with this bit set, > + then opening /dev/watchdog will skip the start operation. > * WDOG_DEV_OPEN: this status bit shows whether or not the watchdog device >was opened via /dev/watchdog. >(This bit should only be used by the WatchDog Timer Driver Core). > @@ -209,6 +213,11 @@ bit-operations. The status bits that are defined are: >any watchdog_ops, so that you can be sure that no operations (other then >unref) will get called after unregister, even if userspace still holds a >reference to /dev/watchdog > +* WDOG_RUNNING: Set by the watchdog driver if the hardware watchdog is > running. > + The bit must be set if the watchdog timer hardware can not be stopped. > + The bit may also be set if the watchdog timer is running aftyer booting, > + before the watchdog device is opened. If set, the watchdog infrastructure > + will send keepalives to the watchdog hardware while WDOG_ACTIVE is not set. > >To set the WDOG_NO_WAY_OUT status bit (before registering your watchdog >timer device) you can either: > [...] > diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c > index c04ba1a98cc8..676e233d5e7b 100644 > --- a/drivers/watchdog/watchdog_dev.c > +++ b/drivers/watchdog/watchdog_dev.c > @@ -59,7 +59,8 @@ static inline bool watchdog_need_worker(struct > watchdog_device *wdd) > unsigned int m = wdd->max_timeout * 1000; > unsigned int t = wdd->timeout * 1000; > > - return watchdog_active(wdd) && hm && (!m || hm < m) && t > hm; > + return (watchdog_active(wdd) && hm && (!m || hm < m) && t > hm) || > +(t && !watchdog_active(wdd) && watchdog_running(wdd)); What is the meaning of !t && !watchdog_active(wdd) && watchdog_running(wdd) ? Can this happen at all? If not, drop "t && "? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | http://www.pengutronix.de/ | -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 3/8] watchdog: Introduce WDOG_RUNNING flag
Hello Guenter, diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt index 25b00b878a7b..6a54dc15a556 100644 --- a/Documentation/watchdog/watchdog-kernel-api.txt +++ b/Documentation/watchdog/watchdog-kernel-api.txt [...] @@ -193,9 +194,12 @@ they are supported. These optional routines/operations are: The status bits should (preferably) be set with the set_bit and clear_bit alike bit-operations. The status bits that are defined are: * WDOG_ACTIVE: this status bit indicates whether or not a watchdog timer device - is active or not. When the watchdog is active after booting, then you should - set this status bit (Note: when you register the watchdog timer device with - this bit set, then opening /dev/watchdog will skip the start operation) + is active or not from user perspective. User space is expected to send + heartbeat requests to the driver while this flag is set. If the watchdog + is active after booting, and you don't want the infrastructure to send + heartbeats to the watchdog driver, then you should set this status bit. IMHO this should not be the driver author's choice! If you implement policy in the kernel it should at least be implemented in the framework and preferably easily changeable. (At least with Kconfig, but better use a kernel parameter (or both, the latter overriding the former).) + Note: when you register the watchdog timer device with this bit set, + then opening /dev/watchdog will skip the start operation. * WDOG_DEV_OPEN: this status bit shows whether or not the watchdog device was opened via /dev/watchdog. (This bit should only be used by the WatchDog Timer Driver Core). @@ -209,6 +213,11 @@ bit-operations. The status bits that are defined are: any watchdog_ops, so that you can be sure that no operations (other then unref) will get called after unregister, even if userspace still holds a reference to /dev/watchdog +* WDOG_RUNNING: Set by the watchdog driver if the hardware watchdog is running. + The bit must be set if the watchdog timer hardware can not be stopped. + The bit may also be set if the watchdog timer is running aftyer booting, + before the watchdog device is opened. If set, the watchdog infrastructure + will send keepalives to the watchdog hardware while WDOG_ACTIVE is not set. To set the WDOG_NO_WAY_OUT status bit (before registering your watchdog timer device) you can either: [...] diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c index c04ba1a98cc8..676e233d5e7b 100644 --- a/drivers/watchdog/watchdog_dev.c +++ b/drivers/watchdog/watchdog_dev.c @@ -59,7 +59,8 @@ static inline bool watchdog_need_worker(struct watchdog_device *wdd) unsigned int m = wdd-max_timeout * 1000; unsigned int t = wdd-timeout * 1000; - return watchdog_active(wdd) hm (!m || hm m) t hm; + return (watchdog_active(wdd) hm (!m || hm m) t hm) || +(t !watchdog_active(wdd) watchdog_running(wdd)); What is the meaning of !t !watchdog_active(wdd) watchdog_running(wdd) ? Can this happen at all? If not, drop t ? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | http://www.pengutronix.de/ | -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 3/8] watchdog: Introduce WDOG_RUNNING flag
The WDOG_RUNNING flag is expected to be set by watchdog drivers if the hardware watchdog is running. If the flag is set, the watchdog subsystem will ping the watchdog even if the watchdog device is closed. The watchdog driver stop function is now optional and may be omitted if the watchdog can not be stopped. If stopping the watchdog is not possible but the driver implements a stop function, it is responsible to set the WDOG_RUNNING flag in its stop function. Cc: Timo Kokkonen Cc: Uwe Kleine-König Signed-off-by: Guenter Roeck --- v2: Improved documentation --- Documentation/watchdog/watchdog-kernel-api.txt | 29 --- drivers/watchdog/watchdog_core.c | 2 +- drivers/watchdog/watchdog_dev.c| 40 -- include/linux/watchdog.h | 7 + 4 files changed, 59 insertions(+), 19 deletions(-) diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt index 25b00b878a7b..6a54dc15a556 100644 --- a/Documentation/watchdog/watchdog-kernel-api.txt +++ b/Documentation/watchdog/watchdog-kernel-api.txt @@ -143,17 +143,18 @@ are: device. The routine needs a pointer to the watchdog timer device structure as a parameter. It returns zero on success or a negative errno code for failure. -* stop: with this routine the watchdog timer device is being stopped. - The routine needs a pointer to the watchdog timer device structure as a - parameter. It returns zero on success or a negative errno code for failure. - Some watchdog timer hardware can only be started and not be stopped. The - driver supporting this hardware needs to make sure that a start and stop - routine is being provided. This can be done by using a timer in the driver - that regularly sends a keepalive ping to the watchdog timer hardware. Not all watchdog timer hardware supports the same functionality. That's why all other routines/operations are optional. They only need to be provided if they are supported. These optional routines/operations are: +* stop: with this routine the watchdog timer device is being stopped. + The routine needs a pointer to the watchdog timer device structure as a + parameter. It returns zero on success or a negative errno code for failure. + Some watchdog timer hardware can only be started and not be stopped. A + driver supporting such hardware does not have to implement the stop routine. + If a driver has no stop function, the watchdog core will set WDOG_RUNNING and + start calling the driver's keepalive pings function after the watchdog device + is closed. * ping: this is the routine that sends a keepalive ping to the watchdog timer hardware. The routine needs a pointer to the watchdog timer device structure as a @@ -193,9 +194,12 @@ they are supported. These optional routines/operations are: The status bits should (preferably) be set with the set_bit and clear_bit alike bit-operations. The status bits that are defined are: * WDOG_ACTIVE: this status bit indicates whether or not a watchdog timer device - is active or not. When the watchdog is active after booting, then you should - set this status bit (Note: when you register the watchdog timer device with - this bit set, then opening /dev/watchdog will skip the start operation) + is active or not from user perspective. User space is expected to send + heartbeat requests to the driver while this flag is set. If the watchdog + is active after booting, and you don't want the infrastructure to send + heartbeats to the watchdog driver, then you should set this status bit. + Note: when you register the watchdog timer device with this bit set, + then opening /dev/watchdog will skip the start operation. * WDOG_DEV_OPEN: this status bit shows whether or not the watchdog device was opened via /dev/watchdog. (This bit should only be used by the WatchDog Timer Driver Core). @@ -209,6 +213,11 @@ bit-operations. The status bits that are defined are: any watchdog_ops, so that you can be sure that no operations (other then unref) will get called after unregister, even if userspace still holds a reference to /dev/watchdog +* WDOG_RUNNING: Set by the watchdog driver if the hardware watchdog is running. + The bit must be set if the watchdog timer hardware can not be stopped. + The bit may also be set if the watchdog timer is running aftyer booting, + before the watchdog device is opened. If set, the watchdog infrastructure + will send keepalives to the watchdog hardware while WDOG_ACTIVE is not set. To set the WDOG_NO_WAY_OUT status bit (before registering your watchdog timer device) you can either: diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c index 1a8059455413..b38d1b7ae10e 100644 --- a/drivers/watchdog/watchdog_core.c +++ b/drivers/watchdog/watchdog_core.c @@ -145,7 +145,7 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
[PATCH v2 3/8] watchdog: Introduce WDOG_RUNNING flag
The WDOG_RUNNING flag is expected to be set by watchdog drivers if the hardware watchdog is running. If the flag is set, the watchdog subsystem will ping the watchdog even if the watchdog device is closed. The watchdog driver stop function is now optional and may be omitted if the watchdog can not be stopped. If stopping the watchdog is not possible but the driver implements a stop function, it is responsible to set the WDOG_RUNNING flag in its stop function. Cc: Timo Kokkonen timo.kokko...@offcode.fi Cc: Uwe Kleine-König u.kleine-koe...@pengutronix.de Signed-off-by: Guenter Roeck li...@roeck-us.net --- v2: Improved documentation --- Documentation/watchdog/watchdog-kernel-api.txt | 29 --- drivers/watchdog/watchdog_core.c | 2 +- drivers/watchdog/watchdog_dev.c| 40 -- include/linux/watchdog.h | 7 + 4 files changed, 59 insertions(+), 19 deletions(-) diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt index 25b00b878a7b..6a54dc15a556 100644 --- a/Documentation/watchdog/watchdog-kernel-api.txt +++ b/Documentation/watchdog/watchdog-kernel-api.txt @@ -143,17 +143,18 @@ are: device. The routine needs a pointer to the watchdog timer device structure as a parameter. It returns zero on success or a negative errno code for failure. -* stop: with this routine the watchdog timer device is being stopped. - The routine needs a pointer to the watchdog timer device structure as a - parameter. It returns zero on success or a negative errno code for failure. - Some watchdog timer hardware can only be started and not be stopped. The - driver supporting this hardware needs to make sure that a start and stop - routine is being provided. This can be done by using a timer in the driver - that regularly sends a keepalive ping to the watchdog timer hardware. Not all watchdog timer hardware supports the same functionality. That's why all other routines/operations are optional. They only need to be provided if they are supported. These optional routines/operations are: +* stop: with this routine the watchdog timer device is being stopped. + The routine needs a pointer to the watchdog timer device structure as a + parameter. It returns zero on success or a negative errno code for failure. + Some watchdog timer hardware can only be started and not be stopped. A + driver supporting such hardware does not have to implement the stop routine. + If a driver has no stop function, the watchdog core will set WDOG_RUNNING and + start calling the driver's keepalive pings function after the watchdog device + is closed. * ping: this is the routine that sends a keepalive ping to the watchdog timer hardware. The routine needs a pointer to the watchdog timer device structure as a @@ -193,9 +194,12 @@ they are supported. These optional routines/operations are: The status bits should (preferably) be set with the set_bit and clear_bit alike bit-operations. The status bits that are defined are: * WDOG_ACTIVE: this status bit indicates whether or not a watchdog timer device - is active or not. When the watchdog is active after booting, then you should - set this status bit (Note: when you register the watchdog timer device with - this bit set, then opening /dev/watchdog will skip the start operation) + is active or not from user perspective. User space is expected to send + heartbeat requests to the driver while this flag is set. If the watchdog + is active after booting, and you don't want the infrastructure to send + heartbeats to the watchdog driver, then you should set this status bit. + Note: when you register the watchdog timer device with this bit set, + then opening /dev/watchdog will skip the start operation. * WDOG_DEV_OPEN: this status bit shows whether or not the watchdog device was opened via /dev/watchdog. (This bit should only be used by the WatchDog Timer Driver Core). @@ -209,6 +213,11 @@ bit-operations. The status bits that are defined are: any watchdog_ops, so that you can be sure that no operations (other then unref) will get called after unregister, even if userspace still holds a reference to /dev/watchdog +* WDOG_RUNNING: Set by the watchdog driver if the hardware watchdog is running. + The bit must be set if the watchdog timer hardware can not be stopped. + The bit may also be set if the watchdog timer is running aftyer booting, + before the watchdog device is opened. If set, the watchdog infrastructure + will send keepalives to the watchdog hardware while WDOG_ACTIVE is not set. To set the WDOG_NO_WAY_OUT status bit (before registering your watchdog timer device) you can either: diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c index 1a8059455413..b38d1b7ae10e 100644 --- a/drivers/watchdog/watchdog_core.c +++ b/drivers/watchdog/watchdog_core.c @@ -145,7 +145,7 @@
