On Mon, 19 Jul 2010, Antti Kantee wrote:

Include "ioconf.h" I think.

Tried that.  It works for compiling the kernel.  Unfortunately,
swwdog is included in rump, and there doesn't seem to be an ioconf.h
available for the librump build.

Well, whatever.  I don't think I want to look at that.

:)

Actually, I still think this is wrong.  It might make librump compile
and even link, that doesn't mean it will be usable if nothing ever
creates that extern symbol.  You'll have to check with pooka or explore
the code, but there have to be some components of rump that have partial
configuration files.  After all, he created the "ioconf" directive for
that purpose.

First of all, let's take a step up from the trenches and try to understand
the problem we're dealing with instead of trying to arbitrarily guess how
to fix the build.  We have two ways of building kernel code: monolithic
(./build.sh kernel=CONF) and modular (kernel modules, rump components).
The latter ones currently always do config stuff on-demand, so changes
cause breakage.  Should a swwdog kernel module exist (why isn't there
one?), it would run into the same problem.

Now, the ioconf keyword for config(1) is meant to help build modular
kernel code by allowing to specify a partial config file.  Currently,
as the name implies, it takes care of creating ioconf.[hc], namely in
this case struct cfdriver swwdog_cd.

Hmmm, I don't seem to see any mention of the ioconf keyword in the current config(1) or config(5) man pages.

Adding a SYSMON.ioconf will solve the problem:
=== snip ===
ioconf sysmon

include "conf/files"

pseudo-device swwdog
=== snip ===

The good news is that if some day a sysmon kernel module is added,
the exact same ioconf can be used without having to once again run
into trouble.

Yay - it appears to work!  Revised diffs are attached to this E-mail.


Then let's view the broader scale.  I think acpibat is currently the
only kernel module using ioconf, and I haven't bothered converting others
since I have realized that the scope of ioconf was a little too narrow.
I'm planning to change the keyword to "module" and add support for source
files.  This way the default build for every modular kernel component
goes through config, and we can avoid issues due to config changes.
IIRC I have the config(1) part for this done already, but being able to
use an autogenerated SRCS list requires some bsd.subdir.mk style advanced
make hackery which I haven't been in the mood for.  Plus, there's of
course the mess with "file some.c foo | bar & (baz ^ xyzzy) & !!!frobnitz".

Finally, on the eternal "someone should" astral plane, someone should
fix the kernel build to consist of building a set of modules and linking
them together, so we don't have more than one way to skin a kernel.

I'm pretty sure "someone" != "me"  :)



-------------------------------------------------------------------------
| Paul Goyette     | PGP Key fingerprint:     | E-mail addresses:       |
| Customer Service | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com    |
| Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at juniper.net |
| Kernel Developer |                          | pgoyette at netbsd.org  |
-------------------------------------------------------------------------
Index: sys/rump/dev/lib/libsysmon/SYSMON.ioconf
===================================================================
RCS file: sys/rump/dev/lib/libsysmon/SYSMON.ioconf
diff -N sys/rump/dev/lib/libsysmon/SYSMON.ioconf
--- /dev/null   1 Jan 1970 00:00:00 -0000
+++ sys/rump/dev/lib/libsysmon/SYSMON.ioconf    19 Jul 2010 12:50:54 -0000
@@ -0,0 +1,8 @@
+#      $NetBSD$
+#
+
+ioconf swwdog
+
+include "conf/files"
+
+pseudo-device  swwdog
Index: sys/rump/dev/lib/libsysmon/Makefile
===================================================================
RCS file: /cvsroot/src/sys/rump/dev/lib/libsysmon/Makefile,v
retrieving revision 1.2
diff -u -p -r1.2 Makefile
--- sys/rump/dev/lib/libsysmon/Makefile 16 Feb 2010 20:42:45 -0000      1.2
+++ sys/rump/dev/lib/libsysmon/Makefile 19 Jul 2010 12:50:54 -0000
@@ -4,6 +4,7 @@
 .PATH: ${.CURDIR}/../../../../dev/sysmon
 
 LIB=   rumpdev_sysmon
+IOCONF=        SYSMON.ioconf
 
 SRCS=  sysmon_taskq.c sysmon_power.c sysmon_envsys.c sysmon_envsys_events.c \
        sysmon_envsys_tables.c sysmon_envsys_util.c sysmon_wdog.c sysmon.c \
Index: sys/dev/sysmon/files.sysmon
===================================================================
RCS file: /cvsroot/src/sys/dev/sysmon/files.sysmon,v
retrieving revision 1.11
diff -u -p -r1.11 files.sysmon
--- sys/dev/sysmon/files.sysmon 30 Jan 2010 21:55:28 -0000      1.11
+++ sys/dev/sysmon/files.sysmon 19 Jul 2010 12:50:54 -0000
@@ -18,5 +18,5 @@ file  dev/sysmon/sysmon_wdog.c        sysmon_wdo
 file   dev/sysmon/sysmon.c             sysmon_envsys | sysmon_wdog |
                                        sysmon_power
 
-defpseudo swwdog: sysmon_wdog
+defpseudodev swwdog: sysmon_wdog
 file    dev/sysmon/swwdog.c            swwdog
Index: sys/dev/sysmon/swwdog.c
===================================================================
RCS file: /cvsroot/src/sys/dev/sysmon/swwdog.c,v
retrieving revision 1.9
diff -u -p -r1.9 swwdog.c
--- sys/dev/sysmon/swwdog.c     31 Jan 2010 02:54:56 -0000      1.9
+++ sys/dev/sysmon/swwdog.c     19 Jul 2010 12:50:54 -0000
@@ -44,20 +44,28 @@ __KERNEL_RCSID(0, "$NetBSD: swwdog.c,v 1
 #include <sys/callout.h>
 #include <sys/device.h>
 #include <sys/kernel.h>
+#include <sys/kmem.h>
 #include <sys/reboot.h>
 #include <sys/systm.h>
+#include <sys/sysctl.h>
 #include <sys/wdog.h>
 #include <dev/sysmon/sysmonvar.h>
 
-#define NSWWDOG 1
+#include "ioconf.h"
+
 struct swwdog_softc {
+       device_t sc_dev;
        struct sysmon_wdog sc_smw;
        struct callout sc_c;
-       char sc_name[20];
        int sc_wdog_armed;
-} sc_wdog[NSWWDOG];
+};
+
+void           swwdogattach(int);
 
-void   swwdogattach(int);
+static int     swwdog_match(device_t, cfdata_t, void *);
+static void    swwdog_attach(device_t, device_t, void *);
+static int     swwdog_detach(device_t, int);
+static bool    swwdog_suspend(device_t, const pmf_qual_t *);
 
 static int swwdog_setmode(struct sysmon_wdog *);
 static int swwdog_tickle(struct sysmon_wdog *);
@@ -67,34 +75,87 @@ static int swwdog_disarm(struct swwdog_s
 
 static void swwdog_panic(void *);
 
-int swwdog_reboot = 0;         /* set for panic instead of reboot */
+bool swwdog_reboot = false;            /* set for panic instead of reboot */
 
 #define        SWDOG_DEFAULT   60              /* 60-second default period */
 
+CFATTACH_DECL_NEW(swwdog, sizeof(struct swwdog_softc),
+       swwdog_match, swwdog_attach, swwdog_detach, NULL);
+
 void
-swwdogattach(int count __unused)
+swwdogattach(int n __unused)
 {
-       int i;
+       int err;
+       static struct cfdata cf;
 
-       for (i = 0; i < NSWWDOG; i++) {
-               struct swwdog_softc *sc = &sc_wdog[i];
+       err = config_cfattach_attach(swwdog_cd.cd_name, &swwdog_ca);
+       if (err) {
+               aprint_error("%s: couldn't register cfattach: %d\n",
+                   swwdog_cd.cd_name, err);
+               config_cfdriver_detach(&swwdog_cd);
+               return;
+       }
 
-               snprintf(sc->sc_name, sizeof sc->sc_name, "swwdog%d", i);
-               printf("%s: ", sc->sc_name);
-               sc->sc_smw.smw_name = sc->sc_name;
-               sc->sc_smw.smw_cookie = sc;
-               sc->sc_smw.smw_setmode = swwdog_setmode;
-               sc->sc_smw.smw_tickle = swwdog_tickle;
-               sc->sc_smw.smw_period = SWDOG_DEFAULT;
-               callout_init(&sc->sc_c, 0);
-               callout_setfunc(&sc->sc_c, swwdog_panic, sc);
+       cf.cf_name = swwdog_cd.cd_name;
+       cf.cf_atname = swwdog_cd.cd_name;
+       cf.cf_unit = 0;
+       cf.cf_fstate = FSTATE_STAR;
 
-               if (sysmon_wdog_register(&sc->sc_smw) == 0)
-                       printf("software watchdog initialized\n");
-               else
-                       printf("unable to register software watchdog "
-                           "with sysmon\n");
-       }
+       (void)config_attach_pseudo(&cf);
+
+       return;
+}
+
+static int
+swwdog_match(device_t parent, cfdata_t data, void *aux)
+{
+       return 1;
+}
+
+static void
+swwdog_attach(device_t parent, device_t self, void *aux)
+{
+       struct swwdog_softc *sc = device_private(self);
+
+       sc->sc_dev = self;
+       sc->sc_smw.smw_name = device_xname(self);
+       sc->sc_smw.smw_cookie = sc;
+       sc->sc_smw.smw_setmode = swwdog_setmode;
+       sc->sc_smw.smw_tickle = swwdog_tickle;
+       sc->sc_smw.smw_period = SWDOG_DEFAULT;
+       callout_init(&sc->sc_c, 0);
+       callout_setfunc(&sc->sc_c, swwdog_panic, sc);
+
+       if (sysmon_wdog_register(&sc->sc_smw) == 0)
+               aprint_normal_dev(self, "software watchdog initialized\n");
+       else
+               aprint_error_dev(self, "unable to register software "
+                   "watchdog with sysmon\n");
+
+       if (!pmf_device_register(self, swwdog_suspend, NULL))
+               aprint_error_dev(self, "couldn't establish power handler\n");
+}
+
+static int
+swwdog_detach(device_t self, int flags)
+{
+       struct swwdog_softc *sc = device_private(self);
+
+       swwdog_disarm(sc);
+       callout_destroy(&sc->sc_c);
+
+       return 1;
+}
+
+static bool
+swwdog_suspend(device_t dev, const pmf_qual_t *qual)
+{
+       struct swwdog_softc *sc = device_private(dev);
+
+       /* Don't allow suspend if watchdog is armed */
+       if ((sc->sc_smw.smw_mode & WDOG_MODE_MASK) != WDOG_MODE_DISARMED)
+               return false;
+       return true;
 }
 
 static int
@@ -144,13 +205,13 @@ static void
 swwdog_panic(void *vsc)
 {
        struct swwdog_softc *sc = vsc;
-       int do_panic;
+       bool do_panic;
 
        do_panic = swwdog_reboot;
        swwdog_reboot = 1;
        callout_schedule(&sc->sc_c, 60 * hz);   /* deliberate double-panic */
 
-       printf("%s: %d second timer expired\n", sc->sc_name,
+       printf("%s: %d second timer expired\n", device_xname(sc->sc_dev),
            sc->sc_smw.smw_period);
 
        if (do_panic)
@@ -158,3 +219,26 @@ swwdog_panic(void *vsc)
        else
                cpu_reboot(0, NULL);
 }
+
+SYSCTL_SETUP(sysctl_swwdog, "swwdog subtree setup")
+{
+       int err;
+       const struct sysctlnode *me;
+
+       err = sysctl_createv(NULL, 0, NULL, NULL, CTLFLAG_PERMANENT,
+           CTLTYPE_NODE, "machdep", NULL,
+           NULL, 0, NULL, 0,
+           CTL_HW, CTL_EOL);
+
+       if (err == 0)
+               err = sysctl_createv(NULL, 0, NULL, &me, CTLFLAG_READWRITE,
+                   CTLTYPE_NODE, "swwdog", NULL,
+                   NULL, 0, NULL, 0,
+                   CTL_HW, CTL_CREATE, CTL_EOL);
+
+       if (err == 0)
+               err = sysctl_createv(NULL, 0, NULL, NULL, CTLFLAG_READWRITE,
+                   CTLTYPE_BOOL, "reboot", "reboot if timer expires",
+                   NULL, 0, &swwdog_reboot, sizeof(bool),
+                   CTL_HW, me->sysctl_num, CTL_CREATE, CTL_EOL);
+}
Index: share/man/man4/swwdog.4
===================================================================
RCS file: /cvsroot/src/share/man/man4/swwdog.4,v
retrieving revision 1.4
diff -u -p -r1.4 swwdog.4
--- share/man/man4/swwdog.4     30 Jan 2010 21:55:28 -0000      1.4
+++ share/man/man4/swwdog.4     19 Jul 2010 12:50:54 -0000
@@ -31,7 +31,7 @@
 .\" ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
 .\" POSSIBILITY OF SUCH DAMAGE.
 .\"
-.Dd January 30, 2010
+.Dd July 18, 2010
 .\" Written by Steven M. Bellovin
 .Dt SWWDOG 4
 .Os
@@ -45,14 +45,22 @@ The
 .Nm
 driver provides a software watchdog timer that works with
 .Xr wdogctl 8 .
-If the timer expires, the system reboots unless the variable
+If the timer expires, the system reboots unless the boolean variable
 .Va swwdog_panic
-is non-zero; if it is, the system will panic instead.
+is
+.Dv true ;
+if it is, the system will panic instead.
+.Va swwdog_reboot
+is accessible as a
+.Xr sysctl 8
+variable, machdep.swwdog0.reboot and defaults to
+.Dv false .
 .Pp
 The default period of
 .Nm
 is 60 seconds.
 .Sh SEE ALSO
+.Xr sysctl 8
 .Xr wdogctl 8
 .Sh HISTORY
 The
@@ -61,7 +69,10 @@ driver was written by
 .An Steven M. Bellovin .
 .Sh BUGS
 Only one watchdog timer can be active at any given time.
-Arguably, this is a bug in the watchdog timer framework.
+(Arguably, this is a bug in the watchdog timer framework.)
+Therefore, only a single instance of the
+.Nm
+device can be created.
 .Pp
 Kernel tickle mode is useless with
 .Nm

Reply via email to