* [PATCH] watchdog: softdog: make pretimeout support a compile option
@ 2017-01-06 8:41 Wolfram Sang
2017-01-10 17:58 ` Guenter Roeck
0 siblings, 1 reply; 4+ messages in thread
From: Wolfram Sang @ 2017-01-06 8:41 UTC (permalink / raw)
To: linux-watchdog; +Cc: linux-renesas-soc, Vladimir Zapolskiy, Wolfram Sang
It occured to me that the panic pretimeout governor will stall the
softdog, because it is purely software which simply halts on panic.
Testing governors with the softdog on the other hand is really useful.
So, make this feature a compile time option which needs 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 <wsa+renesas@sang-engineering.com>
---
drivers/watchdog/Kconfig | 8 ++++++++
drivers/watchdog/softdog.c | 27 +++++++++++++++++++--------
2 files changed, 27 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..bc8b7da61d8aa7 100644
--- a/drivers/watchdog/softdog.c
+++ b/drivers/watchdog/softdog.c
@@ -74,6 +74,7 @@ static struct timer_list softdog_ticktock =
static struct watchdog_device softdog_dev;
+#if IS_ENABLED(CONFIG_SOFT_WATCHDOG_PRETIMEOUT)
static void softdog_pretimeout(unsigned long data)
{
watchdog_notify_pretimeout(&softdog_dev);
@@ -82,16 +83,23 @@ static void softdog_pretimeout(unsigned long data)
static struct timer_list softdog_preticktock =
TIMER_INITIALIZER(softdog_pretimeout, 0, 0);
+static struct timer_list *softdog_preticktock_ptr = &softdog_preticktock;
+#else
+static void *softdog_preticktock_ptr = NULL;
+#endif /* CONFIG_SOFT_WATCHDOG_PRETIMEOUT */
+
static int softdog_ping(struct watchdog_device *w)
{
if (!mod_timer(&softdog_ticktock, jiffies + (w->timeout * HZ)))
__module_get(THIS_MODULE);
- if (w->pretimeout)
- mod_timer(&softdog_preticktock, jiffies +
- (w->timeout - w->pretimeout) * HZ);
- else
- del_timer(&softdog_preticktock);
+ if (softdog_preticktock_ptr) {
+ if (w->pretimeout)
+ mod_timer(softdog_preticktock_ptr, jiffies +
+ (w->timeout - w->pretimeout) * HZ);
+ else
+ del_timer(softdog_preticktock_ptr);
+ }
return 0;
}
@@ -101,15 +109,15 @@ static int softdog_stop(struct watchdog_device *w)
if (del_timer(&softdog_ticktock))
module_put(THIS_MODULE);
- del_timer(&softdog_preticktock);
+ if (softdog_preticktock_ptr)
+ del_timer(softdog_preticktock_ptr);
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 +142,9 @@ static int __init softdog_init(void)
watchdog_set_nowayout(&softdog_dev, nowayout);
watchdog_stop_on_reboot(&softdog_dev);
+ if (softdog_preticktock_ptr)
+ softdog_info.options |= WDIOF_PRETIMEOUT;
+
ret = watchdog_register_device(&softdog_dev);
if (ret)
return ret;
--
2.11.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] watchdog: softdog: make pretimeout support a compile option
2017-01-06 8:41 [PATCH] watchdog: softdog: make pretimeout support a compile option Wolfram Sang
@ 2017-01-10 17:58 ` Guenter Roeck
2017-01-19 20:14 ` Wolfram Sang
0 siblings, 1 reply; 4+ messages in thread
From: Guenter Roeck @ 2017-01-10 17:58 UTC (permalink / raw)
To: Wolfram Sang; +Cc: linux-watchdog, linux-renesas-soc, Vladimir Zapolskiy
On Fri, Jan 06, 2017 at 09:41:44AM +0100, Wolfram Sang wrote:
> It occured to me that the panic pretimeout governor will stall the
> softdog, because it is purely software which simply halts on panic.
> Testing governors with the softdog on the other hand is really useful.
> So, make this feature a compile time option which needs 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 <wsa+renesas@sang-engineering.com>
> ---
> drivers/watchdog/Kconfig | 8 ++++++++
> drivers/watchdog/softdog.c | 27 +++++++++++++++++++--------
> 2 files changed, 27 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..bc8b7da61d8aa7 100644
> --- a/drivers/watchdog/softdog.c
> +++ b/drivers/watchdog/softdog.c
> @@ -74,6 +74,7 @@ static struct timer_list softdog_ticktock =
>
> static struct watchdog_device softdog_dev;
>
> +#if IS_ENABLED(CONFIG_SOFT_WATCHDOG_PRETIMEOUT)
> static void softdog_pretimeout(unsigned long data)
I would prefer __maybe_unused here ..
> {
> watchdog_notify_pretimeout(&softdog_dev);
> @@ -82,16 +83,23 @@ static void softdog_pretimeout(unsigned long data)
> static struct timer_list softdog_preticktock =
> TIMER_INITIALIZER(softdog_pretimeout, 0, 0);
>
> +static struct timer_list *softdog_preticktock_ptr = &softdog_preticktock;
> +#else
> +static void *softdog_preticktock_ptr = NULL;
> +#endif /* CONFIG_SOFT_WATCHDOG_PRETIMEOUT */
> +
> static int softdog_ping(struct watchdog_device *w)
> {
> if (!mod_timer(&softdog_ticktock, jiffies + (w->timeout * HZ)))
> __module_get(THIS_MODULE);
>
> - if (w->pretimeout)
> - mod_timer(&softdog_preticktock, jiffies +
> - (w->timeout - w->pretimeout) * HZ);
> - else
> - del_timer(&softdog_preticktock);
> + if (softdog_preticktock_ptr) {
and "if (IS_ENABLED(CONFIG_SOFT_WATCHDOG_PRETIMEOUT))" here.
> + if (w->pretimeout)
> + mod_timer(softdog_preticktock_ptr, jiffies +
> + (w->timeout - w->pretimeout) * HZ);
> + else
> + del_timer(softdog_preticktock_ptr);
> + }
>
> return 0;
> }
> @@ -101,15 +109,15 @@ static int softdog_stop(struct watchdog_device *w)
> if (del_timer(&softdog_ticktock))
> module_put(THIS_MODULE);
>
> - del_timer(&softdog_preticktock);
> + if (softdog_preticktock_ptr)
Is this conditional needed (assuming we get rid of softdog_preticktock_ptr) ?
Ok though if you want to use it to drop the code if not needed.
> + del_timer(softdog_preticktock_ptr);
>
> 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 +142,9 @@ static int __init softdog_init(void)
> watchdog_set_nowayout(&softdog_dev, nowayout);
> watchdog_stop_on_reboot(&softdog_dev);
>
> + if (softdog_preticktock_ptr)
> + softdog_info.options |= WDIOF_PRETIMEOUT;
> +
> ret = watchdog_register_device(&softdog_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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] watchdog: softdog: make pretimeout support a compile option
2017-01-10 17:58 ` Guenter Roeck
@ 2017-01-19 20:14 ` Wolfram Sang
2017-01-21 19:01 ` Guenter Roeck
0 siblings, 1 reply; 4+ messages in thread
From: Wolfram Sang @ 2017-01-19 20:14 UTC (permalink / raw)
To: Guenter Roeck
Cc: Wolfram Sang, linux-watchdog, linux-renesas-soc,
Vladimir Zapolskiy
> > +#if IS_ENABLED(CONFIG_SOFT_WATCHDOG_PRETIMEOUT)
> > static void softdog_pretimeout(unsigned long data)
>
> I would prefer __maybe_unused here ..
>
> > {
> > watchdog_notify_pretimeout(&softdog_dev);
> > @@ -82,16 +83,23 @@ static void softdog_pretimeout(unsigned long data)
> > static struct timer_list softdog_preticktock =
> > TIMER_INITIALIZER(softdog_pretimeout, 0, 0);
> >
> > +static struct timer_list *softdog_preticktock_ptr = &softdog_preticktock;
> > +#else
> > +static void *softdog_preticktock_ptr = NULL;
> > +#endif /* CONFIG_SOFT_WATCHDOG_PRETIMEOUT */
> > +
> > static int softdog_ping(struct watchdog_device *w)
> > {
> > if (!mod_timer(&softdog_ticktock, jiffies + (w->timeout * HZ)))
> > __module_get(THIS_MODULE);
> >
> > - if (w->pretimeout)
> > - mod_timer(&softdog_preticktock, jiffies +
> > - (w->timeout - w->pretimeout) * HZ);
> > - else
> > - del_timer(&softdog_preticktock);
> > + if (softdog_preticktock_ptr) {
>
> and "if (IS_ENABLED(CONFIG_SOFT_WATCHDOG_PRETIMEOUT))" here.
>
> > + if (w->pretimeout)
> > + mod_timer(softdog_preticktock_ptr, jiffies +
> > + (w->timeout - w->pretimeout) * HZ);
> > + else
> > + del_timer(softdog_preticktock_ptr);
> > + }
> >
> > return 0;
> > }
> > @@ -101,15 +109,15 @@ static int softdog_stop(struct watchdog_device *w)
> > if (del_timer(&softdog_ticktock))
> > module_put(THIS_MODULE);
> >
> > - del_timer(&softdog_preticktock);
> > + if (softdog_preticktock_ptr)
>
> Is this conditional needed (assuming we get rid of softdog_preticktock_ptr) ?
> Ok though if you want to use it to drop the code if not needed.
Yes. I tried a few variations and this is the outcome which I liked
best, because it is quite readable, keeps all the extra stuff within one
block and has 0 size penalty when the feature is not enabled.
I'd like to keep it this way unless you have a strong opinion.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] watchdog: softdog: make pretimeout support a compile option
2017-01-19 20:14 ` Wolfram Sang
@ 2017-01-21 19:01 ` Guenter Roeck
0 siblings, 0 replies; 4+ messages in thread
From: Guenter Roeck @ 2017-01-21 19:01 UTC (permalink / raw)
To: Wolfram Sang
Cc: Wolfram Sang, linux-watchdog, linux-renesas-soc,
Vladimir Zapolskiy
On 01/19/2017 12:14 PM, Wolfram Sang wrote:
>
>>> +#if IS_ENABLED(CONFIG_SOFT_WATCHDOG_PRETIMEOUT)
>>> static void softdog_pretimeout(unsigned long data)
>>
>> I would prefer __maybe_unused here ..
>>
>>> {
>>> watchdog_notify_pretimeout(&softdog_dev);
>>> @@ -82,16 +83,23 @@ static void softdog_pretimeout(unsigned long data)
>>> static struct timer_list softdog_preticktock =
>>> TIMER_INITIALIZER(softdog_pretimeout, 0, 0);
>>>
>>> +static struct timer_list *softdog_preticktock_ptr = &softdog_preticktock;
>>> +#else
>>> +static void *softdog_preticktock_ptr = NULL;
>>> +#endif /* CONFIG_SOFT_WATCHDOG_PRETIMEOUT */
>>> +
>>> static int softdog_ping(struct watchdog_device *w)
>>> {
>>> if (!mod_timer(&softdog_ticktock, jiffies + (w->timeout * HZ)))
>>> __module_get(THIS_MODULE);
>>>
>>> - if (w->pretimeout)
>>> - mod_timer(&softdog_preticktock, jiffies +
>>> - (w->timeout - w->pretimeout) * HZ);
>>> - else
>>> - del_timer(&softdog_preticktock);
>>> + if (softdog_preticktock_ptr) {
>>
>> and "if (IS_ENABLED(CONFIG_SOFT_WATCHDOG_PRETIMEOUT))" here.
>>
>>> + if (w->pretimeout)
>>> + mod_timer(softdog_preticktock_ptr, jiffies +
>>> + (w->timeout - w->pretimeout) * HZ);
>>> + else
>>> + del_timer(softdog_preticktock_ptr);
>>> + }
>>>
>>> return 0;
>>> }
>>> @@ -101,15 +109,15 @@ static int softdog_stop(struct watchdog_device *w)
>>> if (del_timer(&softdog_ticktock))
>>> module_put(THIS_MODULE);
>>>
>>> - del_timer(&softdog_preticktock);
>>> + if (softdog_preticktock_ptr)
>>
>> Is this conditional needed (assuming we get rid of softdog_preticktock_ptr) ?
>> Ok though if you want to use it to drop the code if not needed.
>
> Yes. I tried a few variations and this is the outcome which I liked
> best, because it is quite readable, keeps all the extra stuff within one
> block and has 0 size penalty when the feature is not enabled.
>
> I'd like to keep it this way unless you have a strong opinion.
>
>
I personally prefer the use of IS_ENABLED over #ifdefs, because it ensures
that the code always compiles, and it doesn't incur any runtime penalties either.
Guenter
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-01-21 19:01 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-01-06 8:41 [PATCH] watchdog: softdog: make pretimeout support a compile option Wolfram Sang
2017-01-10 17:58 ` Guenter Roeck
2017-01-19 20:14 ` Wolfram Sang
2017-01-21 19:01 ` Guenter Roeck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).