Re: [PATCH] watchdog: core: module param to activate watchdog

[Guenter Roeck <linux@roeck-us.net>]

On 02/28/2014 08:16 AM, Markus Pargmann wrote:
> Many watchdog driver reset the watchdog device on initialization. This
> is a problem if the watchdog is activated by the bootloader and should
> be active the whole time until the userspace can write to it.
>

Shouldn't those watchdog drivers be fixed to keep the watchdog running
if it was already active ? Otherwise there is still the time between
driver initialization, which disables the watchdog, and it being re-enabled
by watchdog registration.

Also, does this affect or impact drivers which _do_ keep the watchdog
enabled ?

> This patch adds a module parameter (watchdog.activate_first) that
> activates the first registered watchdog. Using this parameter it is
> possible to have an active watchdog during the whole boot process.
>
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> ---
>   drivers/watchdog/watchdog_core.c |  4 ++++
>   drivers/watchdog/watchdog_core.h |  4 ++++
>   drivers/watchdog/watchdog_dev.c  | 21 ++++++++++++++++++++-
>   3 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index cec9b55..6db16eb 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -43,6 +43,10 @@
>   static DEFINE_IDA(watchdog_ida);
>   static struct class *watchdog_class;
>
> +unsigned int activate_first = 0;

Bad variable name for a global variable.

> +module_param(activate_first, uint, 0);
> +MODULE_PARM_DESC(activate_first, "Activation timeout for first watchdog registered. 0 means no activation.");
> +

Does that _have_ to be in this file ? Would be much easier
if it was in watchdog_dev.c, where it is used.

>   static void watchdog_check_min_max_timeout(struct watchdog_device *wdd)
>   {
>   	/*
> diff --git a/drivers/watchdog/watchdog_core.h b/drivers/watchdog/watchdog_core.h
> index 6c95141..4526170 100644
> --- a/drivers/watchdog/watchdog_core.h
> +++ b/drivers/watchdog/watchdog_core.h
> @@ -28,6 +28,10 @@
>
>   #define MAX_DOGS	32	/* Maximum number of watchdog devices */
>
> +/* Activate first registered watchdog and set this as the timeout value
> + * (only != 0) */

Coding style.

> +extern unsigned int activate_first;
> +
>   /*
>    *	Functions/procedures to be called by the core
>    */
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 6aaefba..fe60fdc 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -553,8 +553,27 @@ int watchdog_dev_register(struct watchdog_device *watchdog)
>   			misc_deregister(&watchdog_miscdev);
>   			old_wdd = NULL;
>   		}
> +

Is that extra empty line really necessary ?

> +		return err;
>   	}
> -	return err;
> +
> +	if (activate_first && watchdog->id == 0) {
> +		err = watchdog_set_timeout(watchdog, activate_first);
> +		if (err && err != -EOPNOTSUPP) {
> +			pr_err("watchdog%d failed to set timeout for first activation\n",
> +					watchdog->id);
> +			return err;
> +		}
> +
> +		err = watchdog_start(watchdog);
> +		if (err) {
> +			pr_err("watchdog%d failed to start first watchdog\n",
> +					watchdog->id);
> +			return err;
> +		}
> +	}
> +
> +	return 0;
>   }
>
>   /*
>