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

Guenter Roeck
2015-08-15T23:21:31Z

On 08/14/2015 04:23 AM, Uwe Kleine-König wrote:
> 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?
>
I thought it would reduce the amount of code, and I thought it would be
more confusing and complicated to first call cancel the worker followed
by a (conditional) start. No strong opinion, though; I'll be happy to
make that change in exchange for a Reviewed-by:.

>> + 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?
>
With the logical change I am making, to ignore max_timeout if max_hw_timeout_ms
is configured, it is indeed no longer necessary (nor desirable).

>> +
>> 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?
>
No idea. Changed.

Thanks,
Guenter