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
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
