Re: [PATCH v2 2/8] watchdog: Introduce hardware maximum timeout in watchdog core

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

Hello Guenter,

On Fri, Aug 07, 2015 at 10:02:41PM -0700, Guenter Roeck wrote:
> [...]
> @@ -61,26 +135,27 @@ static struct watchdog_device *old_wdd;
>  
>  static int watchdog_ping(struct watchdog_device *wdd)
>  {
> -	int err = 0;
> +	int err;
>  
>  	mutex_lock(&wdd->lock);
> +	err = _watchdog_ping(wdd);
> +	wdd->last_keepalive = jiffies;
> +	watchdog_update_worker(wdd, false, false);
> +	mutex_unlock(&wdd->lock);
>  
> -	if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
> -		err = -ENODEV;
> -		goto out_ping;
> -	}
> +	return err;
> +}
>  
> -	if (!watchdog_active(wdd))
> -		goto out_ping;
> +static void watchdog_ping_work(struct work_struct *work)
> +{
> +	struct watchdog_device *wdd;
>  
> -	if (wdd->ops->ping)
> -		err = wdd->ops->ping(wdd);	/* ping the watchdog */
> -	else
> -		err = wdd->ops->start(wdd);	/* restart watchdog */
> +	wdd = container_of(to_delayed_work(work), struct watchdog_device, work);
>  
> -out_ping:
> +	mutex_lock(&wdd->lock);
> +	_watchdog_ping(wdd);
> +	watchdog_update_worker(wdd, false, false);
>  	mutex_unlock(&wdd->lock);
> -	return err;
>  }
>  
>  /*
> @@ -107,8 +182,11 @@ static int watchdog_start(struct watchdog_device *wdd)
>  		goto out_start;
>  
>  	err = wdd->ops->start(wdd);
> -	if (err == 0)
> +	if (err == 0) {
>  		set_bit(WDOG_ACTIVE, &wdd->status);
> +		wdd->last_keepalive = jiffies;
> +		watchdog_update_worker(wdd, false, false);
> +	}
>  
>  out_start:
>  	mutex_unlock(&wdd->lock);
> @@ -146,8 +224,10 @@ static int watchdog_stop(struct watchdog_device *wdd)
>  	}
>  
>  	err = wdd->ops->stop(wdd);
> -	if (err == 0)
> +	if (err == 0) {
>  		clear_bit(WDOG_ACTIVE, &wdd->status);
> +		watchdog_update_worker(wdd, true, false);
> +	}
>  
>  out_stop:
>  	mutex_unlock(&wdd->lock);
> @@ -211,6 +291,8 @@ static int watchdog_set_timeout(struct watchdog_device *wdd,
>  
>  	err = wdd->ops->set_timeout(wdd, timeout);
>  
> +	watchdog_update_worker(wdd, true, false);

I still try to wrap my head around this function. You pass cancel=true
for stop and set_timeout to ensure that the worker doesn't continue to
run. That's fine.

For watchdog_start you pass cancel=false. I guess the background is that
after one of the next patches the worker might already run for handling
the watchdog being unstoppable. Maybe it's easier to grasp the logic if
you don't try to be too clever here: stop the worker on start
unconditionally and possibly restart it if the hardware needs extra
poking to fulfil the timeout set?

> +	if (!watchdog_wq)
> +		return -ENODEV;
> +
> +	INIT_DELAYED_WORK(&wdd->work, watchdog_ping_work);
> +
> +	if (!wdd->max_hw_timeout_ms)
> +		wdd->max_hw_timeout_ms = wdd->max_timeout * 1000;

With this (and assuming wdd->max_timeout > 0) the check for
max_hw_timeout_ms != 0 is not necessary, is it?

> +
>  	if (wdd->id == 0) {
>  		old_wdd = wdd;
>  		watchdog_miscdev.parent = wdd->parent;
> [...]
> @@ -585,9 +680,21 @@ int watchdog_dev_unregister(struct watchdog_device *wdd)
>  
>  int __init watchdog_dev_init(void)
>  {
> -	int err = alloc_chrdev_region(&watchdog_devt, 0, MAX_DOGS, "watchdog");
> +	int err;
> +
> +	watchdog_wq = alloc_workqueue("watchdogd",
> +				      WQ_HIGHPRI | WQ_MEM_RECLAIM, 0);
> +	if (!watchdog_wq) {
> +		pr_err("Failed to create watchdog workqueue\n");
> +		err = -ENOMEM;
> +		goto abort;

Why not return -ENOMEM directly?

> +	}
> +
> +	err = alloc_chrdev_region(&watchdog_devt, 0, MAX_DOGS, "watchdog");
>  	if (err < 0)
>  		pr_err("watchdog: unable to allocate char dev region\n");
> +
> +abort:
>  	return err;

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |