Re: [PATCH v2] watchdog: softdog: make pretimeout support a compile option

[Guenter Roeck]

On Tue, Feb 07, 2017 at 03:03:29PM +0100, Wolfram Sang wrote:
> It occurred to me that the panic pretimeout governor will stall the
> softdog, because it is purely software which simply breaks when the
> kernel panics. Testing governors with the softdog on the other hand is
> really useful, so make this feature a compile time option which nees to
> be enabled explicitly. This also removes the overhead if pretimeout
> support is not used because it will now be compiled away (saving ~10% on
> ARM32).
> 
> Signed-off-by: Wolfram Sang 

Reviewed-by: Guenter Roeck 

> ---
> 
> Change since V1:
> 
> * use IS_DEFINED as if-conditions, removing the need for #ifdef
> 
> I have to admit that this result is better readable and what the compiler
> compiles away is sufficient.
> 
>  drivers/watchdog/Kconfig   |  8 
>  drivers/watchdog/softdog.c | 21 +
>  2 files changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index acb00b53a5207b..70726ce3d166e8 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -71,6 +71,14 @@ config SOFT_WATCHDOG
> To compile this driver as a module, choose M here: the
> module will be called softdog.
>  
> +config SOFT_WATCHDOG_PRETIMEOUT
> + bool "Software watchdog pretimeout governor support"
> + depends on SOFT_WATCHDOG && WATCHDOG_PRETIMEOUT_GOV
> + help
> +   Enable this if you want to use pretimeout governors with the software
> +   watchdog. Be aware that governors might affect the watchdog because it
> +   is purely software, e.g. the panic governor will stall it!
> +
>  config DA9052_WATCHDOG
>   tristate "Dialog DA9052 Watchdog"
>   depends on PMIC_DA9052
> diff --git a/drivers/watchdog/softdog.c b/drivers/watchdog/softdog.c
> index c7bdc986dca1c2..7983029852ab0d 100644
> --- a/drivers/watchdog/softdog.c
> +++ b/drivers/watchdog/softdog.c
> @@ -87,11 +87,13 @@ static int softdog_ping(struct watchdog_device *w)
>   if (!mod_timer(_ticktock, jiffies + (w->timeout * HZ)))
>   __module_get(THIS_MODULE);
>  
> - if (w->pretimeout)
> - mod_timer(_preticktock, jiffies +
> -   (w->timeout - w->pretimeout) * HZ);
> - else
> - del_timer(_preticktock);
> + if (IS_ENABLED(CONFIG_SOFT_WATCHDOG_PRETIMEOUT)) {
> + if (w->pretimeout)
> + mod_timer(_preticktock, jiffies +
> +   (w->timeout - w->pretimeout) * HZ);
> + else
> + del_timer(_preticktock);
> + }
>  
>   return 0;
>  }
> @@ -101,15 +103,15 @@ static int softdog_stop(struct watchdog_device *w)
>   if (del_timer(_ticktock))
>   module_put(THIS_MODULE);
>  
> - del_timer(_preticktock);
> + if (IS_ENABLED(CONFIG_SOFT_WATCHDOG_PRETIMEOUT))
> + del_timer(_preticktock);
>  
>   return 0;
>  }
>  
>  static struct watchdog_info softdog_info = {
>   .identity = "Software Watchdog",
> - .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE |
> -WDIOF_PRETIMEOUT,
> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
>  };
>  
>  static const struct watchdog_ops softdog_ops = {
> @@ -134,6 +136,9 @@ static int __init softdog_init(void)
>   watchdog_set_nowayout(_dev, nowayout);
>   watchdog_stop_on_reboot(_dev);
>  
> + if (IS_ENABLED(CONFIG_SOFT_WATCHDOG_PRETIMEOUT))
> + softdog_info.options |= WDIOF_PRETIMEOUT;
> +
>   ret = watchdog_register_device(_dev);
>   if (ret)
>   return ret;
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] watchdog: softdog: make pretimeout support a compile option

[Vladimir Zapolskiy]

Hi Wolfram,

On 02/07/2017 04:03 PM, Wolfram Sang wrote:
> It occurred to me that the panic pretimeout governor will stall the
> softdog, because it is purely software which simply breaks when the
> kernel panics. Testing governors with the softdog on the other hand is
> really useful, so make this feature a compile time option which nees to
> be enabled explicitly. This also removes the overhead if pretimeout
> support is not used because it will now be compiled away (saving ~10% on
> ARM32).
> 
> Signed-off-by: Wolfram Sang 

the changes looks perfect,

Reviewed-by: Vladimir Zapolskiy 

--
With best wishes,
Vladimir

[PATCH v2] watchdog: softdog: make pretimeout support a compile option

[Wolfram Sang]

It occurred to me that the panic pretimeout governor will stall the
softdog, because it is purely software which simply breaks when the
kernel panics. Testing governors with the softdog on the other hand is
really useful, so make this feature a compile time option which nees to
be enabled explicitly. This also removes the overhead if pretimeout
support is not used because it will now be compiled away (saving ~10% on
ARM32).

Signed-off-by: Wolfram Sang 
---

Change since V1:

* use IS_DEFINED as if-conditions, removing the need for #ifdef

I have to admit that this result is better readable and what the compiler
compiles away is sufficient.

 drivers/watchdog/Kconfig   |  8 
 drivers/watchdog/softdog.c | 21 +
 2 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index acb00b53a5207b..70726ce3d166e8 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -71,6 +71,14 @@ config SOFT_WATCHDOG
  To compile this driver as a module, choose M here: the
  module will be called softdog.
 
+config SOFT_WATCHDOG_PRETIMEOUT
+   bool "Software watchdog pretimeout governor support"
+   depends on SOFT_WATCHDOG && WATCHDOG_PRETIMEOUT_GOV
+   help
+ Enable this if you want to use pretimeout governors with the software
+ watchdog. Be aware that governors might affect the watchdog because it
+ is purely software, e.g. the panic governor will stall it!
+
 config DA9052_WATCHDOG
tristate "Dialog DA9052 Watchdog"
depends on PMIC_DA9052
diff --git a/drivers/watchdog/softdog.c b/drivers/watchdog/softdog.c
index c7bdc986dca1c2..7983029852ab0d 100644
--- a/drivers/watchdog/softdog.c
+++ b/drivers/watchdog/softdog.c
@@ -87,11 +87,13 @@ static int softdog_ping(struct watchdog_device *w)
if (!mod_timer(_ticktock, jiffies + (w->timeout * HZ)))
__module_get(THIS_MODULE);
 
-   if (w->pretimeout)
-   mod_timer(_preticktock, jiffies +
- (w->timeout - w->pretimeout) * HZ);
-   else
-   del_timer(_preticktock);
+   if (IS_ENABLED(CONFIG_SOFT_WATCHDOG_PRETIMEOUT)) {
+   if (w->pretimeout)
+   mod_timer(_preticktock, jiffies +
+ (w->timeout - w->pretimeout) * HZ);
+   else
+   del_timer(_preticktock);
+   }
 
return 0;
 }
@@ -101,15 +103,15 @@ static int softdog_stop(struct watchdog_device *w)
if (del_timer(_ticktock))
module_put(THIS_MODULE);
 
-   del_timer(_preticktock);
+   if (IS_ENABLED(CONFIG_SOFT_WATCHDOG_PRETIMEOUT))
+   del_timer(_preticktock);
 
return 0;
 }
 
 static struct watchdog_info softdog_info = {
.identity = "Software Watchdog",
-   .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE |
-  WDIOF_PRETIMEOUT,
+   .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
 };
 
 static const struct watchdog_ops softdog_ops = {
@@ -134,6 +136,9 @@ static int __init softdog_init(void)
watchdog_set_nowayout(_dev, nowayout);
watchdog_stop_on_reboot(_dev);
 
+   if (IS_ENABLED(CONFIG_SOFT_WATCHDOG_PRETIMEOUT))
+   softdog_info.options |= WDIOF_PRETIMEOUT;
+
ret = watchdog_register_device(_dev);
if (ret)
return ret;
-- 
2.11.0