WatchDog Driver Updates

WatchDog Driver Updates

[Rob Radez]

Hi,
I've put up a patch on http://osinvestor.com/bigwatchdog.diff against
2.4.19-pre5-ac3.  The diff is 33k, and it affects 19 files in drivers/char/,
here's a diffstat of it:
 acquirewdt.c   |   25 ++++-----------------
 advantechwdt.c |   21 ++++--------------
 alim7101_wdt.c |   27 +++++++++++------------
 eurotechwdt.c  |   23 ++++++--------------
 i810-tco.c     |    7 ++----
 ib700wdt.c     |   21 ++++--------------
 machzwd.c      |   31 +++++++--------------------
 mixcomwd.c     |   11 +++------
 sbc60xxwdt.c   |   23 ++++++++++----------
 sc1200wdt.c    |    8 -------
 sc520_wdt.c    |   28 +++++++++---------------
 shwdt.c        |   16 ++++----------
 softdog.c      |    5 +++-
 w83877f_wdt.c  |   19 ++++++++--------
 wafer5823wdt.c |    3 --
 wdt.c          |    1
 wdt285.c       |   23 +++++++++-----------
 wdt977.c       |   65 +++++++++++++++++++++++++++++++++++++++++++++++---------- wdt_pci.c      |    9 +------
 19 files changed, 162 insertions(+), 204 deletions(-)

It's all fairly minor changes, I think, but I would appreciate any comments
on it.

Regards,
Rob Radez

Re: WatchDog Driver Updates

[Russell King]

On Sat, Apr 06, 2002 at 09:49:57PM -0500, Rob Radez wrote:
> I've put up a patch on http://osinvestor.com/bigwatchdog.diff against
> 2.4.19-pre5-ac3.  The diff is 33k, and it affects 19 files in drivers/char/,

And the purpose of this patch is...

1. copy_to_user is like memcpy() - it takes two pointers and an integer
   size.  You're passing it two integers and a pointer:

   static int wdt977_ioctl(... unsigned long arg)
   ...
            case WDIOC_GETSTATUS:
                   if(copy_to_user(arg, &timer_alive, sizeof(timer_alive)))
                              return -EFAULT;
                   return 0;

2. sizeof(int) != sizeof(unsigned long).  You've changed the ABI in an
   unsafe manner on 64-bit machines.  The ABI is defined by the ioctl
   numbers, which specify an 'int' type.

3. copy_to_user of an int or unsigned long is a bit inefficient.  Why
   not use put_user to write an int or unsigned long?

4. Unless I'm missing something, WDIOC_GETSTATUS can only ever return
   an indication that the timer is open/alive - it's set when the driver
   is opened, and cleared when it is closed.  Since you can't perform
   an ioctl on a closed device, the times that you can perform the ioctl,
   it'll always give you a non-zero result.

5. WDIOC_GETSTATUS is supposed to return the WDIOF_* flags.  Returning
   "watchdog active" as bit 0 causes it to indicate WDIOF_OVERHEAT.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: WatchDog Driver Updates

[Rob Radez]

On Sun, 7 Apr 2002, Russell King wrote:

> On Sat, Apr 06, 2002 at 09:49:57PM -0500, Rob Radez wrote:
> > I've put up a patch on http://osinvestor.com/bigwatchdog.diff against
> > 2.4.19-pre5-ac3.  The diff is 33k, and it affects 19 files in drivers/char/,
>
> And the purpose of this patch is...
>
> 1. copy_to_user is like memcpy() - it takes two pointers and an integer
>    size.  You're passing it two integers and a pointer:
>
>    static int wdt977_ioctl(... unsigned long arg)
>    ...
>             case WDIOC_GETSTATUS:
>                    if(copy_to_user(arg, &timer_alive, sizeof(timer_alive)))
>                               return -EFAULT;
>                    return 0;
>
> 2. sizeof(int) != sizeof(unsigned long).  You've changed the ABI in an
>    unsafe manner on 64-bit machines.  The ABI is defined by the ioctl
>    numbers, which specify an 'int' type.
>
> 3. copy_to_user of an int or unsigned long is a bit inefficient.  Why
>    not use put_user to write an int or unsigned long?

Ok, these first three points, yep, I screwed up.  I'll fix those up. Oops.

>
> 4. Unless I'm missing something, WDIOC_GETSTATUS can only ever return
>    an indication that the timer is open/alive - it's set when the driver
>    is opened, and cleared when it is closed.  Since you can't perform
>    an ioctl on a closed device, the times that you can perform the ioctl,
>    it'll always give you a non-zero result.

This is true, see below.

>
> 5. WDIOC_GETSTATUS is supposed to return the WDIOF_* flags.  Returning
>    "watchdog active" as bit 0 causes it to indicate WDIOF_OVERHEAT.

Hmm...I'm not seeing any standards here.  Some drivers would just send
whether the watchdog device was open, some would only send 0, sc1200
would send whether the device was enabled or disabled, one did 'int one=1'
and then a few lines later copy_to_user'd 'one', and it looks like all of
three of twenty would actually return proper WDIOF flags.  The lack of
proper documentation is fun, so I'll change them all to be consistent and
then add in the diffs for the documentation to bring those up to date too.
They'll all implement WDIOC_GETSTATUS, even if that implementation is just
put_user'ing 0.  None of them will return open status since of course, to
run the ioctl they have to be open and since open status would conflict
with the overheat flag.

Thanks,
Rob Radez

Re: WatchDog Driver Updates

[Russell King]

On Sun, Apr 07, 2002 at 06:35:55AM -0400, Rob Radez wrote:
> Hmm...I'm not seeing any standards here.  Some drivers would just send
> whether the watchdog device was open, some would only send 0, sc1200
> would send whether the device was enabled or disabled, one did 'int one=1'
> and then a few lines later copy_to_user'd 'one', and it looks like all of
> three of twenty would actually return proper WDIOF flags.

Maybe Alan would like to comment and clear up this issue - I believe the
interface was Alan's design.  Certainly Alan wrote most of the early
watchdog drivers.

Thanks.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: WatchDog Driver Updates

[Rob Radez]

On Sun, 7 Apr 2002, Russell King wrote:

> On Sun, Apr 07, 2002 at 06:35:55AM -0400, Rob Radez wrote:
> > Hmm...I'm not seeing any standards here.  Some drivers would just send
> > whether the watchdog device was open, some would only send 0, sc1200
> > would send whether the device was enabled or disabled, one did 'int one=1'
> > and then a few lines later copy_to_user'd 'one', and it looks like all of
> > three of twenty would actually return proper WDIOF flags.
>
> Maybe Alan would like to comment and clear up this issue - I believe the
> interface was Alan's design.  Certainly Alan wrote most of the early
> watchdog drivers.
>
> Thanks.

Ok, well, there's a new patch up at http://osinvestor.com/bigwatchdog-2.diff
that does these changes.

Regards,
Rob Radez

Re: WatchDog Driver Updates

[Alan Cox]

> > Maybe Alan would like to comment and clear up this issue - I believe the
> > interface was Alan's design.  Certainly Alan wrote most of the early
> > watchdog drivers.
> 
> Ok, well, there's a new patch up at http://osinvestor.com/bigwatchdog-2.diff
> that does these changes.

They should follow the spec. Most follow it but sometimes somewhat loosely.
In the case of checking if its running many cards can only trust the
hardware not probe it

Re: WatchDog Driver Updates

[Alan Cox]

> > 5. WDIOC_GETSTATUS is supposed to return the WDIOF_* flags.  Returning
> >    "watchdog active" as bit 0 causes it to indicate WDIOF_OVERHEAT.
> 
> Hmm...I'm not seeing any standards here.  Some drivers would just send

Documentation/* in current -ac

Re: WatchDog Driver Updates

[Rob Radez]

On Sun, 7 Apr 2002, Alan Cox wrote:

> > > 5. WDIOC_GETSTATUS is supposed to return the WDIOF_* flags.  Returning
> > >    "watchdog active" as bit 0 causes it to indicate WDIOF_OVERHEAT.
> >
> > Hmm...I'm not seeing any standards here.  Some drivers would just send
>
> Documentation/* in current -ac

2.4.19-pre5-ac3

Right, Documentation/watchdog.txt:
The i810 TCO watchdog driver also implements the WDIOC_GETSTATUS and
WDIOC_GETBOOTSTATUS ioctl()s. WDIOC_GETSTATUS returns the actual counter value
and WDIOC_GETBOOTSTATUS returns the value of TCO2 Status Register (see Intel's
documentation for the 82801AA and 82801AB datasheet).

Documentation/watchdog-api.txt lists each driver and how the ones that
implement GETSTATUS do so in a 'silly' manner.  While it also does mention
how cards are supposed to return WDIOF_* (and list supported flags in the
options), it also shows how all of 3 out of 20 watchdog drivers actually
follow that convention.  7 return the number 1, 1 returns the number of ticks,
and 1 returns whether the card is enabled or disabled.  So following the
standards embodied in the code, every driver should just return 1 ;-).

Ah-ha, the crown jewels!
Documentation/pcwd-watchdog.txt:
        WDIOC_GETSTATUS
                This returns the status of the card, with the bits of
                WDIOF_* bitwise-anded into the value.  (The comments
                are in linux/pcwd.h)
Although, this is hidden away in a driver specific file (and even references
a file which no longer exists).  I guess I'll clean all this up in my next
patch.

Regards,
Rob Radez

Re: WatchDog Driver Updates

[Zwane Mwaikambo]

static int sc1200wdt_release(struct inode *inode, struct file *file)
 {
-#ifndef CONFIG_WATCHDOG_NOWAYOUT
 	if (expect_close) {
 		sc1200wdt_write_data(WDTO, 0);
 		printk(KERN_INFO PFX "Watchdog disabled\n");
@@ -202,7 +197,6 @@
 		sc1200wdt_write_data(WDTO, timeout);
 		printk(KERN_CRIT PFX "Unexpected close!, timeout = %d 
min(s)\n", timeout);
 	}	
-#endif

hmm, that would allow closing of the watchdog even if 
CONFIG_WATCHDOG_NOWAYOUT is specified. Was this your intention?

	Zwane
-- 
http://function.linuxpower.ca
		

Re: WatchDog Driver Updates

[Rob Radez]

On Sun, 7 Apr 2002, Zwane Mwaikambo wrote:

> static int sc1200wdt_release(struct inode *inode, struct file *file)
>  {
> -#ifndef CONFIG_WATCHDOG_NOWAYOUT
>  	if (expect_close) {
>  		sc1200wdt_write_data(WDTO, 0);
>  		printk(KERN_INFO PFX "Watchdog disabled\n");
> @@ -202,7 +197,6 @@
>  		sc1200wdt_write_data(WDTO, timeout);
>  		printk(KERN_CRIT PFX "Unexpected close!, timeout = %d
> min(s)\n", timeout);
>  	}
> -#endif
>
> hmm, that would allow closing of the watchdog even if
> CONFIG_WATCHDOG_NOWAYOUT is specified. Was this your intention?

No it doesn't because expect_close will never be set to anything but 0 if
CONFIG_WATCHDOG_NOWAYOUT is set.  So the if(expect_close) will never be
true.

Regards,
Rob Radez