Re: [RFC] [PATCH 5/10] Generic Watchdog Timer Driver

[Mike Waychison <mikew@google.com>]

On 02/23/11 12:43, Wim Van Sebroeck wrote:
> commit 1959ea403c69af855f5cd13e5c9b33123d2137b2
> Author: Wim Van Sebroeck<wim@iguana.be>
> Date:   Fri Jun 18 09:45:49 2010 +0000
>
>      watchdog: WatchDog Timer Driver Core - Part 5
>
>      This part add's the WDIOC_SETTIMEOUT and WDIOC_GETTIMEOUT ioctl
>      functionality to the WatchDog Timer Driver Core framework.
>
>      Signed-off-by: Alan Cox<alan@lxorguk.ukuu.org.uk>
>      Signed-off-by: Wim Van Sebroeck<wim@iguana.be>
>
> diff --git a/Documentation/watchdog/src/watchdog-with-timer-example.c b/Documentation/watchdog/src/watchdog-with-timer-example.c
> index 6a4af47..f1d4f217 100644
> --- a/Documentation/watchdog/src/watchdog-with-timer-example.c
> +++ b/Documentation/watchdog/src/watchdog-with-timer-example.c
> @@ -110,18 +110,27 @@ static int wdt_stop(struct watchdog_device *wdd)
>   	return 0;
>   }
>
> +static int wdt_set_timeout(struct watchdog_device *wdd, int new_timeout)
> +{
> +	if (new_timeout<  1)
> +		return -EINVAL;
> +	return 0;
> +}
> +
>   /*
>    * The watchdog kernel structures
>    */
>   static const struct watchdog_info wdt_info = {
>   	.identity =	DRV_NAME,
> -	.options =	WDIOF_KEEPALIVEPING,
> +	.options =	WDIOF_SETTIMEOUT |
> +			WDIOF_KEEPALIVEPING,
>   };
>
>   static const struct watchdog_ops wdt_ops = {
>   	.start =	wdt_start,
>   	.stop =		wdt_stop,
>   	.ping =		wdt_ping,
> +	.set_timeout =	wdt_set_timeout,
>   };
>
>   static struct watchdog_device wdt_dev = {
> @@ -139,6 +148,9 @@ static int __devinit wdt_probe(struct platform_device *pdev)
>
>   	/* Register other stuff */
>
> +	/* Set watchdog_device parameters */
> +	wdt_dev.timeout = timeout;
> +
>   	/* Register the watchdog timer device */
>   	res = register_watchdogdevice(&wdt_dev);
>   	if (res) {
> diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
> index 3de69e7..f15e8d4 100644
> --- a/Documentation/watchdog/watchdog-kernel-api.txt
> +++ b/Documentation/watchdog/watchdog-kernel-api.txt
> @@ -42,6 +42,7 @@ struct watchdog_device {
>   	char *name;
>   	const struct watchdog_info *info;
>   	const struct watchdog_ops *ops;
> +	int timeout;
>   	int bootstatus;
>   	long status;
>   };
> @@ -51,6 +52,7 @@ It contains following fields:
>   * info: a pointer to a watchdog_info structure. This structure gives some
>     additional information about the watchdog timer itself.
>   * ops: a pointer to the list of watchdog operations that the watchdog supports.
> +* timeout: the watchdog timer's timeout value (in seconds).
>   * bootstatus: status of the device after booting (reported with watchdog
>     WDIOF_* status bits).
>   * status: this field contains a number of status bits that give extra
> @@ -67,6 +69,7 @@ struct watchdog_ops {
>   	/* optional operations */
>   	int (*ping)(struct watchdog_device *);
>   	int (*status)(struct watchdog_device *);
> +	int (*set_timeout)(struct watchdog_device *, int);
>   };
>
>   Some operations are mandatory and some are optional. The mandatory operations
> @@ -102,6 +105,12 @@ they are supported. These optional routines/operations are:
>     info structure).
>   * status: this routine checks the status of the watchdog timer device. The
>     status of the device is reported with watchdog WDIOF_* status flags/bits.
> +* set_timeout: this routine checks and changes the timeout of the watchdog
> +  timer device. It returns 0 on success and an errno code on failure. On success
> +  the timeout value of the watchdog_device will be changed to the value that
> +  was just used to re-program the watchdog timer device.
> +  (Note: the WDIOF_SETTIMEOUT needs to be set in the options field of the
> +  watchdog's info structure).
>
>   The status bits should (preferably) be set with the set_bit and clear_bit alike
>   bit-operations. The status bit's that are defined are:
> diff --git a/drivers/watchdog/core/watchdog_dev.c b/drivers/watchdog/core/watchdog_dev.c
> index 2bf4f67..b80c6e6 100644
> --- a/drivers/watchdog/core/watchdog_dev.c
> +++ b/drivers/watchdog/core/watchdog_dev.c
> @@ -221,6 +221,26 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
>   			return -EOPNOTSUPP;
>   		watchdog_ping(wdd);
>   		return 0;
> +	case WDIOC_SETTIMEOUT:
> +		if ((wdd->ops->set_timeout == NULL) ||
> +		    !(wdd->info->options&  WDIOF_SETTIMEOUT))
> +			return -EOPNOTSUPP;
> +		if (get_user(val, p))
> +			return -EFAULT;

Should we sanity check that val > 0 here?  I realize that the driver 
would need to do it's own sanity checking, but a value of 0 or < 0 makes 
no sense to the core itself.

Maybe timeout is best expressed as u32 if this is the case.

> +		err = wdd->ops->set_timeout(wdd, val);
> +		if (err<  0)
> +			return err;
> +		wdd->timeout = val;
> +		/* If the watchdog is active then we sent a keepalive ping
> +		 * to make sure that the watchdog keep's running (and if
> +		 * possible that it takes the new timeout) */
> +		watchdog_ping(wdd);
> +		/* Fall */
> +	case WDIOC_GETTIMEOUT:
> +		/* timeout == 0 means that we don't know the timeout */
> +		if (wdd->timeout)
> +			return put_user(wdd->timeout, p);
> +		return -EOPNOTSUPP;
>   	default:
>   		return -ENOTTY;
>   	}
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index 736814f..e35f51f 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -70,6 +70,7 @@ struct watchdog_ops {
>   	/* optional operations */
>   	int (*ping)(struct watchdog_device *);
>   	int (*status)(struct watchdog_device *);
> +	int (*set_timeout)(struct watchdog_device *, int);
>   };
>
>   /* The structure that defines a watchdog device */
> @@ -78,6 +79,7 @@ struct watchdog_device {
>   	const struct watchdog_info *info;
>   	const struct watchdog_ops *ops;
>   	int bootstatus;
> +	int timeout;
>   	long status;
>   #define WDOG_ACTIVE		0	/* is the watchdog running/active */
>   #define WDOG_DEV_OPEN		1	/* is the watchdog opened via