Slight interface change on the watchdog fido

From: Nick Hibma <nick_at_van-laarhoven.org>
Date: Sun, 10 Dec 2006 11:04:39 +0100 (CET)
I'm planning on committing the following change to make the implementations of 
the various hardware watchdogs more consistent. A timeout of 0 passed to the 
ioctl is now a valid input and will disable the watchdog. Previously it 
produced an error for the Elan chip watchdog, which is _not_ what you want to 
see when disarming a watchdog :-) I'd appreciate review of the individual 
changes in the files by the following people. The change as a whole was 
discussed with phk.

   cognet_at_freebsd.org	i80321_wdog.c (*)
   des			ichwd.c (**)
   ambrisko		ipmi.c
   marius			mk48txx.c
   phk			kern_clock.c, elan-mmcr.c, watchdog.c (**)

(*) The i80321_wdog.c cannot be disarmed. Is this correct?
(**) These have been tested to arm, disarm, and fire. Others have only been 
compile tested.

This change has been tested on 6.2-STABLE and 7-CURRENT.


Commit message:

Convert all watchdog event handlers to the following format:

- If the timeout value passed is >0 and acceptable arm the watchdog and set the 
*error to 0 (a watchdog is armed).
- Otherwise disarm the watchdog and leave *error as is (valid disarm).
- If the timeout value passed is 0, disarm the watchdog. set *error to an error 
if disarm failed.

The default of *error in the ioctl is a failed arm and a succesful disarm.

Also:
- Return an error if WD_PASSIVE is passed in to the ioctl as only
   WD_ACTIVE is implemented at the moment. See sys/watchdog.h for an
   explanation of the difference between WD_ACTIVE and WD_PASSIVE.
- Remove the I_HAVE_TOTALLY_LOST_MY_SENSE_OF_HUMOR define. If you've lost your
   sense of humor, than don't add a define.

i80321_wdog.c
 	Don't roll your own passive watchdog tickle as this would defeat the
 	purpose of an active (userland) watchdog tickle.

ichwd.c / ipmi.c:
 	WD_ACTIVE means userland pats of the watchdog, not whether the
 	watchdog is active. See sys/watchdog.h.

kern_clock.c:
 	Remove a check for WD_ACTIVE as this does not make sense here.
 	This reverts r1.181, as that commit doesn't make sense.

Index: sys/arm/xscale/i80321/i80321_wdog.c
===================================================================
RCS file: /home/ncvs/src/sys/arm/xscale/i80321/i80321_wdog.c,v
retrieving revision 1.2
diff -u -r1.2 i80321_wdog.c
--- sys/arm/xscale/i80321/i80321_wdog.c	15 Jan 2005 18:38:10 -0000	1.2
+++ sys/arm/xscale/i80321/i80321_wdog.c	9 Dec 2006 12:40:40 -0000
_at__at_ -62,7 +62,6 _at__at_
  	device_t dev;
  	int armed;
  	int wdog_period;
-	struct callout_handle wdog_callout;
  };

  static __inline void
_at__at_ -83,8 +82,6 _at__at_
  		return;
  	wdtcr_write(WDTCR_ENABLE1);
  	wdtcr_write(WDTCR_ENABLE2);
-	sc->wdog_callout = timeout(iopwdog_tickle, sc,
-	    hz * (sc->wdog_period - 1));
  }

  static int
_at__at_ -112,14 +109,19 _at__at_
  {
  	struct iopwdog_softc *sc = private;

-	if (cmd == 0)
-		return;
-	if ((((uint64_t)1 << (cmd & WD_INTERVAL))) >
-	    (uint64_t)sc->wdog_period * 1000000000)
-		return;
-	sc->armed = 1;
-	iopwdog_tickle(sc);
-	*error = 0;
+	cmd &= WD_INTERVAL;
+	if (cmd > 0 && cmd <= 63
+	    && (uint64_t)1 << (cmd & WD_INTERVAL) <=
+	       (uint64_t)sc->wdog_period * 1000000000) {
+		/* Valid value -> Enable watchdog */
+		iopwdog_tickle(sc);
+		sc->armed = 1;
+		*error = 0;
+	} else {
+		/* XXX Can't disable this watchdog? */
+		if (sc->armed)
+			*error = EOPNOTSUPP;
+	}
  }

  static int
Index: sys/dev/ichwd/ichwd.c
===================================================================
RCS file: /home/ncvs/src/sys/dev/ichwd/ichwd.c,v
retrieving revision 1.5.2.1
diff -u -r1.5.2.1 ichwd.c
--- sys/dev/ichwd/ichwd.c	15 Jun 2006 15:15:07 -0000	1.5.2.1
+++ sys/dev/ichwd/ichwd.c	9 Dec 2006 12:40:46 -0000
_at__at_ -178,38 +178,20 _at__at_
  	struct ichwd_softc *sc = arg;
  	unsigned int timeout;

+	/* convert from power-of-two-ns to WDT ticks */
+	cmd &= WD_INTERVAL;
+	timeout = ((uint64_t)1 << cmd) / ICHWD_TICK;
+	if (cmd > 0 && cmd <= 63
+	    && timeout >= ICHWD_MIN_TIMEOUT && timeout <= ICHWD_MAX_TIMEOUT) {
+		if (timeout != sc->timeout)
+			ichwd_tmr_set(sc, timeout);

-	/* disable / enable */
-	if (!(cmd & WD_ACTIVE)) {
+		ichwd_tmr_reload(sc);
+		*error = 0;
+	} else {
  		if (sc->active)
  			ichwd_tmr_disable(sc);
-		*error = 0;
-		return;
  	}
-	if (!sc->active)
-		ichwd_tmr_enable(sc);
-
-	cmd &= WD_INTERVAL;
-	/* convert from power-of-to-ns to WDT ticks */
-	if (cmd >= 64) {
-		*error = EINVAL;
-		return;
-	}
-	timeout = ((uint64_t)1 << cmd) / ICHWD_TICK;
-	if (timeout < ICHWD_MIN_TIMEOUT || timeout > ICHWD_MAX_TIMEOUT) {
-		*error = EINVAL;
-		return;
-	}
-
-	/* set new initial value */
-	if (timeout != sc->timeout)
-		ichwd_tmr_set(sc, timeout);
-
-	/* reload */
-	ichwd_tmr_reload(sc);
-
-	*error = 0;
-	return;
  }

  static unsigned int pmbase = 0;
Index: sys/dev/ipmi/ipmi.c
===================================================================
RCS file: /home/ncvs/src/sys/dev/ipmi/ipmi.c,v
retrieving revision 1.3.2.3
diff -u -r1.3.2.3 ipmi.c
--- sys/dev/ipmi/ipmi.c	19 Oct 2006 14:50:48 -0000	1.3.2.3
+++ sys/dev/ipmi/ipmi.c	9 Dec 2006 12:40:47 -0000
_at__at_ -649,25 +649,14 _at__at_
  	struct ipmi_softc *sc = arg;
  	unsigned int timeout;

-	/* disable / enable */
-	if (!(cmd & WD_ACTIVE)) {
-		ipmi_set_watchdog(sc, 0);
-		*error = 0;
-		return;
-	}
-
  	cmd &= WD_INTERVAL;
-	/* convert from power-of-to-ns to WDT ticks */
-	if (cmd >= 64) {
-		*error = EINVAL;
-		return;
+	if (cmd > 0 && cmd <= 63) {
+		timeout = ((uint64_t)1 << cmd) / 1800000000;
+		ipmi_set_watchdog(sc, timeout);
+		*error = 0;
+	} else {
+		ipmi_set_watchdog(sc, 0);
  	}
-	timeout = ((uint64_t)1 << cmd) / 1800000000;
-
-	/* reload */
-	ipmi_set_watchdog(sc, timeout);
-
-	*error = 0;
  }

  #ifdef CLONING
Index: sys/dev/watchdog/watchdog.c
===================================================================
RCS file: /home/ncvs/src/sys/dev/watchdog/watchdog.c,v
retrieving revision 1.2
diff -u -r1.2 watchdog.c
--- sys/dev/watchdog/watchdog.c	16 Jun 2004 09:47:02 -0000	1.2
+++ sys/dev/watchdog/watchdog.c	9 Dec 2006 12:40:51 -0000
_at__at_ -55,9 +55,16 _at__at_
  		return (EINVAL);
  	if ((u & (WD_ACTIVE | WD_PASSIVE)) == (WD_ACTIVE | WD_PASSIVE))
  		return (EINVAL);
-	if ((u & WD_INTERVAL) == WD_TO_NEVER)
+	if (u & WD_PASSIVE)
+		return (ENOSYS);	/* XXX Not implemented yet */
+	if ((u & WD_INTERVAL) == WD_TO_NEVER) {
  		u = 0;
-	error = EOPNOTSUPP;
+		/* Assume all is well; watchdog signals failure. */
+		error = 0;
+	} else {
+		/* Assume no watchdog available; watchdog flags success */
+		error = EOPNOTSUPP;
+	}
  	EVENTHANDLER_INVOKE(watchdog_list, u, &error);
  	return (error);
  }
Index: sys/i386/i386/elan-mmcr.c
===================================================================
RCS file: /home/ncvs/src/sys/i386/i386/elan-mmcr.c,v
retrieving revision 1.31.2.1
diff -u -r1.31.2.1 elan-mmcr.c
--- sys/i386/i386/elan-mmcr.c	16 Aug 2005 22:47:14 -0000	1.31.2.1
+++ sys/i386/i386/elan-mmcr.c	9 Dec 2006 12:40:53 -0000
_at__at_ -367,11 +367,11 _at__at_
  static void
  elan_watchdog(void *foo __unused, u_int spec, int *error)
  {
-	u_int u, v;
+	u_int u, v, w;
  	static u_int cur;

  	u = spec & WD_INTERVAL;
-	if (spec && u <= 35) {
+	if (spec && u > 0 && u <= 35) {
  		u = imax(u - 5, 24);
  		v = 2 << (u - 24);
  		v |= 0xc000;
_at__at_ -383,7 +383,7 _at__at_
  		 * for other reasons.  Save and restore the GP echo mode
  		 * around our hardware tom-foolery.
  		 */
-		u = elan_mmcr->GPECHO;
+		w = elan_mmcr->GPECHO;
  		elan_mmcr->GPECHO = 0;
  		if (v != cur) {
  			/* Clear the ENB bit */
_at__at_ -401,17 +401,17 _at__at_
  			elan_mmcr->WDTMRCTL = 0xaaaa;
  			elan_mmcr->WDTMRCTL = 0x5555;
  		}
-		elan_mmcr->GPECHO = u;
+		elan_mmcr->GPECHO = w;
  		*error = 0;
  		return;
  	} else {
-		u = elan_mmcr->GPECHO;
+		w = elan_mmcr->GPECHO;
  		elan_mmcr->GPECHO = 0;
  		elan_mmcr->WDTMRCTL = 0x3333;
  		elan_mmcr->WDTMRCTL = 0xcccc;
  		elan_mmcr->WDTMRCTL = 0x4080;
-		elan_mmcr->WDTMRCTL = u;
-		elan_mmcr->GPECHO = u;
+		elan_mmcr->WDTMRCTL = w;		/* XXX What does this 
statement do? */
+		elan_mmcr->GPECHO = w;
  		cur = 0;
  		return;
  	}
Index: sys/kern/kern_clock.c
===================================================================
RCS file: /home/ncvs/src/sys/kern/kern_clock.c,v
retrieving revision 1.178.2.3
diff -u -r1.178.2.3 kern_clock.c
--- sys/kern/kern_clock.c	10 Mar 2006 19:37:33 -0000	1.178.2.3
+++ sys/kern/kern_clock.c	9 Dec 2006 12:40:55 -0000
_at__at_ -541,7 +541,7 _at__at_
  	u_int u;

  	u = cmd & WD_INTERVAL;
-	if ((cmd & WD_ACTIVE) && u >= WD_TO_1SEC) {
+	if (u >= WD_TO_1SEC) {
  		watchdog_ticks = (1 << (u - WD_TO_1SEC)) * hz;
  		watchdog_enabled = 1;
  		*err = 0;
Index: sys/sys/watchdog.h
===================================================================
RCS file: /home/ncvs/src/sys/sys/watchdog.h,v
retrieving revision 1.3
diff -u -r1.3 watchdog.h
--- sys/sys/watchdog.h	28 Feb 2004 20:06:58 -0000	1.3
+++ sys/sys/watchdog.h	9 Dec 2006 12:41:08 -0000
_at__at_ -30,11 +30,7 _at__at_

  #include <sys/ioccom.h>

-#ifdef I_HAVE_TOTALLY_LOST_MY_SENSE_OF_HUMOUR
-#define	_PATH_WATCHDOG	"watchdog"
-#else
  #define	_PATH_WATCHDOG	"fido"
-#endif

  #define WDIOCPATPAT	_IOW('W', 42, u_int)

Index: share/man/man4/Makefile
===================================================================
RCS file: /home/ncvs/src/share/man/man4/Makefile,v
retrieving revision 1.320.2.21
diff -u -r1.320.2.21 Makefile
--- share/man/man4/Makefile	23 Nov 2006 09:21:24 -0000	1.320.2.21
+++ share/man/man4/Makefile	9 Dec 2006 12:41:08 -0000
_at__at_ -500,6 +500,7 _at__at_
  MLINKS+=wi.4 if_wi.4
  MLINKS+=xe.4 if_xe.4
  MLINKS+=xl.4 if_xl.4
+MLINKS+=watchdog.4 SW_WATCHDOG.4

  .if ${MACHINE_ARCH} == "amd64" || ${MACHINE_ARCH} == "i386"
  _amdsmb.4=	amdsmb.4
Index: share/man/man4/watchdog.4
===================================================================
RCS file: /home/ncvs/src/share/man/man4/watchdog.4,v
retrieving revision 1.6.8.1
diff -u -r1.6.8.1 watchdog.4
--- share/man/man4/watchdog.4	1 May 2006 19:42:45 -0000	1.6.8.1
+++ share/man/man4/watchdog.4	9 Dec 2006 12:41:08 -0000
_at__at_ -32,54 +32,84 _at__at_
  .Nm watchdog
  .Nd "hardware and software watchdog"
  .Sh SYNOPSIS
-.Cd "options CPU_ELAN"
-.Cd "options CPU_GEODE"
-.Cd "options SW_WATCHDOG"
-.Pp
  .In sys/watchdog.h
  .Sh DESCRIPTION
  The
  .Nm
  facility is used for controlling hardware and software watchdogs.
  .Pp
-The interface is through a device
  .Pa /dev/fido
-which responds to a single
+responds to a single
  .Xr ioctl 2
  call,
  .Dv WDIOCPATPAT .
+It takes a single argument which represents a timeout value specified as a
+power of two nanoseconds, or-ed with a flag selecting active or passive 
control
+of the watchdog.
  .Pp
-The call takes a single argument which represents a timeout value
-specified as an integer power of two nanoseconds.
-.Pp
-The
  .Dv WD_ACTIVE
-flag signals that the
+indicates that the
  .Nm
-will be kept from
-timing out from userland, for instance by the
+will be kept from timing out from userland, for instance by the
  .Xr watchdogd 8
  daemon.
-.Pp
-To disable the watchdogs, an argument of zero should be used.
+.Dv WD_PASSIVE
+indicates that the
+.Nm
+will be kept from timing out from the kernel.
  .Pp
  The
  .Xr ioctl 2
  call will return success if just one of the available
  .Xr watchdog 9
-implementations support the request.
-If the call fails, for instance if none of
+implementations supports setting the timeout to the specified timeout. This
+means that at least one watchdog is armed. If the call fails, for instance if
+none of
  .Xr watchdog 9
-implementations support the timeout
-length, all watchdogs are disabled and must be explicitly re-enabled.
+implementations support the timeout length, all watchdogs are disabled and 
must
+be explicitly re-enabled.
+.Pp
+To disable the watchdogs pass
+.Dv WD_TO_NEVER .
+If disarming the watchdog(s) failed an error is returned. The watchdog might
+still be armed!
  .Sh EXAMPLES
-.\" XXX insert some descriptive text here
  .Bd -literal -offset indent
-u_int u = WD_ACTIVE | WD_TO_8SEC;
-int fd = open("/dev/fido", O_RDWR);
+#include <paths.h>
+#include <sys/watchdog.h>
+
+#define WDPATH	"/dev/" _PATH_WATCHDOG
+int wdfd = -1;

-ioctl(fd, WDIOCPATPAT, &u);
+static void
+wd_init(void) +{
+	wdfd = open(WDPATH, O_RDWR);
+	if (wdfd == -1)
+		err(1, WDPATH);
+}
+static void
+wd_reset(u_int timeout)
+{
+	if (ioctl(wdfd, WDIOCPATPAT, &timeout) == -1)
+		err(1, "WDIOCPATPAT");
+}
+
+/* in main() */
+wd_init();
+wd_reset(WD_ACTIVE|WD_TO_8SEC);
+/* potential freeze point */
+wd_reset(WD_TO_NEVER);
  .Ed
+.Pp
+Enables a watchdog to recover from a potentially freezing piece of code.
+.Pp
+.Bd -literal -offset indent
+options SW_WATCHDOG
+.Ed
+.Pp
+in your kernel config adds a software watchdog in the kernel, dropping to KDB
+or panic-ing when firing.
  .Sh SEE ALSO
  .Xr watchdogd 8 ,
  .Xr watchdog 9
_at__at_ -88,14 +118,17 _at__at_
  .Nm
  code first appeared in
  .Fx 5.1 .
+.Sh BUGS
+The
+.Dv WD_PASSIVE
+option has not yet been implemented.
  .Sh AUTHORS
  .An -nosplit
  The
  .Nm
  facility was written by
  .An Poul-Henning Kamp Aq phk_at_FreeBSD.org .
-The software watchdog code
-and this manual page were written by
+The software watchdog code and this manual page were written by
  .An Sean Kelly Aq smkelly_at_FreeBSD.org .
  Some contributions were made by
  .An Jeff Roberson Aq jeff_at_FreeBSD.org .
Index: watchdog.9
===================================================================
RCS file: /home/ncvs/src/share/man/man9/watchdog.9,v
retrieving revision 1.3
diff -u -r1.3 watchdog.9
--- watchdog.9	6 Jul 2004 07:39:50 -0000	1.3
+++ watchdog.9	9 Dec 2006 13:14:54 -0000
_at__at_ -35,6 +35,7 _at__at_
  .Ft void
  .Fn watchdog_fn "void *private" "u_int cmd" "int *error"
  .Fn EVENTHANDLER_REGISTER watchdog_list watchdog_fn private 0
+.Fn EVENTHANDLER_DEREGISTER watchdog_list eventhandler_tag
  .Sh DESCRIPTION
  To implement a watchdog in software or hardware, only a single
  function needs to be written and registered on the global
_at__at_ -60,7 +61,9 _at__at_
  If the watchdog cannot be configured to the proposed timeout, it
  must be disabled and the
  .Fa error
-argument left untouched.
+argument left untouched. If the watchdog cannot be disabled, the
+.Fa error
+argument must be set to indicate the error.
  .Pp
  There is no specification of what the watchdog should do when it
  times out, but a hardware reset or similar
Received on Sun Dec 10 2006 - 09:04:43 UTC

This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:39:03 UTC