Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934677Ab3DHIQq (ORCPT ); Mon, 8 Apr 2013 04:16:46 -0400 Received: from mail.active-venture.com ([67.228.131.205]:62113 "EHLO mail.active-venture.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934429Ab3DHIQo (ORCPT ); Mon, 8 Apr 2013 04:16:44 -0400 X-Originating-IP: 108.223.40.66 Date: Mon, 8 Apr 2013 01:16:44 -0700 From: Guenter Roeck To: Hector Palacios Cc: "linux-watchdog@vger.kernel.org" , "wim@iguana.be" , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH RFC] watchdog: core: don't try to stop device if not running Message-ID: <20130408081644.GA22722@roeck-us.net> References: <515EF748.6060504@digi.com> <20130405183450.GA29835@roeck-us.net> <51627669.1070900@digi.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <51627669.1070900@digi.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3300 Lines: 79 On Mon, Apr 08, 2013 at 09:48:57AM +0200, Hector Palacios wrote: > On 04/05/2013 08:34 PM, Guenter Roeck wrote: > >On Fri, Apr 05, 2013 at 06:09:44PM +0200, Hector Palacios wrote: > >>A watchdog device may be stopped from userspace using WDIOC_SETOPTIONS > >>ioctl and flag WDIOS_DISABLECARD. If the device is closed after this > >>operation, watchdog_release() is called and status bits checked for > >>stopping it. Besides, if the device has not been unregistered a critical > >>message "watchdog did not stop!" is printed, although the ioctl may have > >>successfully stopped it already. > >> > >>Without the patch a user application sample code like this will successfully > >>stop the watchdog, but the kernel will output the message > >>"watchdog did not stop!": > >> > >> wd_fd = open("/dev/watchdog", O_RDWR); > >> > >> flags = WDIOS_DISABLECARD; > >> ioctl(wd_fd, WDIOC_SETOPTIONS, &flags); > >> > >> close(wd_fd); > >> > >>Signed-off-by: Hector Palacios > > > >How about the following patch instead ? > > > >diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c > >index 08b48bb..9775e8d 100644 > >--- a/drivers/watchdog/watchdog_dev.c > >+++ b/drivers/watchdog/watchdog_dev.c > >@@ -469,7 +469,9 @@ static int watchdog_release(struct inode *inode, struct file *file) > > * or if WDIOF_MAGICCLOSE is not set. If nowayout was set then > > * watchdog_stop will fail. > > */ > >- if (test_and_clear_bit(WDOG_ALLOW_RELEASE, &wdd->status) || > >+ if (!test_bit(WDOG_ACTIVE, &wdd->status)) > >+ err = 0; > >+ else if (test_and_clear_bit(WDOG_ALLOW_RELEASE, &wdd->status) || > > !(wdd->info->options & WDIOF_MAGICCLOSE)) > > err = watchdog_stop(wdd); > > > >Much less invasive and the result is the same. > > I like the simplicity but it is kind of inverted logic to initially > define err = -EBUSY only to turn it to zero later, so I'm rebuilding > your approach like this: > > diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c > index ef8edec..a4163cd 100644 > --- a/drivers/watchdog/watchdog_dev.c > +++ b/drivers/watchdog/watchdog_dev.c > @@ -463,16 +463,19 @@ out: > static int watchdog_release(struct inode *inode, struct file *file) > { > struct watchdog_device *wdd = file->private_data; > - int err = -EBUSY; > + int err = 0; > > /* > * We only stop the watchdog if we received the magic character > * or if WDIOF_MAGICCLOSE is not set. If nowayout was set then > * watchdog_stop will fail. > */ > - if (test_and_clear_bit(WDOG_ALLOW_RELEASE, &wdd->status) || > - !(wdd->info->options & WDIOF_MAGICCLOSE)) > + if (test_bit(WDOG_ACTIVE, &wdd->status)) > + err = -EBUSY; > + else if (test_and_clear_bit(WDOG_ALLOW_RELEASE, &wdd->status) || > + !(wdd->info->options & WDIOF_MAGICCLOSE)) { > err = watchdog_stop(wdd); > + } Ok, but the added { } are unnecessary and violate coding style rules. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/