Re: [PATCH v2 1/2] watchdog: add pretimeout support to the core

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

On 06/25/2016 06:01 PM, Vladimir Zapolskiy wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> Since the watchdog framework centrializes the IOCTL interfaces of device
> drivers now, SETPRETIMEOUT and GETPRETIMEOUT need to be added in the
> common code.
>
> Signed-off-by: Robin Gong <b38343@freescale.com>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> [vzapolskiy: added conditional pretimeout sysfs attribute visibility]
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> ---

[ ... ]

> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -307,10 +307,14 @@ static int watchdog_set_timeout(struct watchdog_device *wdd,
>   	if (watchdog_timeout_invalid(wdd, timeout))
>   		return -EINVAL;
>
> -	if (wdd->ops->set_timeout)
> +	if (wdd->ops->set_timeout) {
>   		err = wdd->ops->set_timeout(wdd, timeout);
> -	else
> +	} else {
>   		wdd->timeout = timeout;
> +		/* Disable pretimeout if it doesn't fit the new timeout */
> +		if (wdd->pretimeout > wdd->timeout)

Nitpick: If we consider pretimeout == timeout to be invalid, we should reject it here
as well. So this should be >=.

Otherwise lgtm, so feel free to add

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

to the next version. I'd suggest to wait a bit to give Wolfram time to provide input,
though.

Thanks,
Guenter