From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from bh-25.webhostbox.net ([208.91.199.152]:41171 "EHLO bh-25.webhostbox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751251AbbHOXVb (ORCPT ); Sat, 15 Aug 2015 19:21:31 -0400 Message-ID: <55CFC974.9070308@roeck-us.net> Date: Sat, 15 Aug 2015 16:21:24 -0700 From: Guenter Roeck MIME-Version: 1.0 To: =?windows-1252?Q?Uwe_Kleine-K=F6nig?= CC: linux-watchdog@vger.kernel.org, Wim Van Sebroeck , linux-kernel@vger.kernel.org, Timo Kokkonen , linux-doc@vger.kernel.org, Jonathan Corbet Subject: Re: [PATCH v2 2/8] watchdog: Introduce hardware maximum timeout in watchdog core References: <1439010167-13818-1-git-send-email-linux@roeck-us.net> <1439010167-13818-3-git-send-email-linux@roeck-us.net> <20150814112328.GT9999@pengutronix.de> In-Reply-To: <20150814112328.GT9999@pengutronix.de> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-watchdog-owner@vger.kernel.org List-Id: linux-watchdog@vger.kernel.org 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