tech-kern archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: power management and pseudo-devices
On Sun, 18 Jul 2010, Quentin Garnier wrote:
Not really objections, just a few comments.
+struct swwdog_softc *sc_swwdog = NULL;
That can't possibly be needed (and, if it was, you'd have to mutexify it
wouldn't you?).
The only purpose of this was to make sure we didn't "magically" get a
request to configure a second swwdog(4).
+
+extern struct cfdriver swwdog_cd;
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.
+swwdogattach(int n)
{
- int i;
+ int err;
+ cfdata_t cf;
+
+ if (n > 1)
+ aprint_verbose("%s: ignoring count of %d\n",
+ swwdog_cd.cd_name, n);
+
+ 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;
+ }
+
+ cf = kmem_alloc(sizeof(struct cfdata), KM_NOSLEEP);
+ if (cf == NULL)
+ aprint_error("%s: couldn't allocate cfdata\n",
+ swwdog_cd.cd_name);
+ else {
+ cf->cf_name = swwdog_cd.cd_name;
+ cf->cf_atname = swwdog_cd.cd_name;
+ cf->cf_unit = 0;
+ cf->cf_fstate = FSTATE_STAR;
- for (i = 0; i < NSWWDOG; i++) {
- struct swwdog_softc *sc = &sc_wdog[i];
+ (void)config_attach_pseudo(cf);
Ok, so you should be aware here that you're changing the logic of that
function, and you're only allowing for the creation of one swwdog(4).
The limitation of only one swwdog(4) was deliberate, especially since
the original code had a "#define NSWWDOG 1" anyway! Furthermore, even
though you might have more than one watchdog provider, the whole code
was written to permit only one to be armed at any given time. So it
really doesn't make much sense to permit multiple swwdog(4)s.
That feature of the old code--implemented with needs-count no less,
brrr--is highly debatable, and the manual page actually says it's
pointless.
So yeah, go ahead and kill it (you can keep the __unused on int count by
the way), but change the manual page.
I will update the man page accordingly, and silently ignore the count.
Also, you don't have to allocate a struct cfdata, you can use a static
one (indepedently of the fact that you only use it once, by the way, all
the other pseudo-devices could be changed in that respect).
Yes, I "borrowed" that part from another driver - dev/pad/pad.c I will
make it a static item.
+static int
+swwdog_match(device_t parent, cfdata_t data, void *aux)
+{
+ /* Match unless we already have a swwdog */
+ if (sc_swwdog == NULL)
+ return 1;
+
+ return 0;
+}
What could possibly trigger another config_attach_pseudo? I know that
config(9) is special, but not magic :-)
Sometimes it feels like magic! :)
+ if (!pmf_device_register(self, NULL, NULL))
+ aprint_error_dev(self, "couldn't establish power "
+ "handler\n");
So what was your initial point about swwdog(4) and pmf(9), anyway?
HeHeHe - yeah, I actually forgot to put that little detail in in the
first round! The swwdog_suspend() routine looks like this in the
current code:
if (!pmf_device_register(self, swwdog_suspend, NULL))
aprint_error_dev(self, "couldn't establish power "
"handler\n");
And the referenced routine is as follows:
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;
}
While you're there, I guess you could make swwdog_reboot a sysctl node.
Good point. Would this belong in the kern or machdep sysctl tree? I am
assuming it would be machdep.swwdog0.reboot for now, but it is easily
changed.
Revised diffs (including updated man page) attached.
-------------------------------------------------------------------------
| 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: src/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
--- src/share/man/man4/swwdog.4 30 Jan 2010 21:55:28 -0000 1.4
+++ src/share/man/man4/swwdog.4 18 Jul 2010 23:21:06 -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
Index: src/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
--- src/sys/dev/sysmon/swwdog.c 31 Jan 2010 02:54:56 -0000 1.9
+++ src/sys/dev/sysmon/swwdog.c 18 Jul 2010 23:21:06 -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
+extern struct cfdriver swwdog_cd;
+
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,100 @@ 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 = 0; /* 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;
+
+ 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;
+ }
- for (i = 0; i < NSWWDOG; i++) {
- struct swwdog_softc *sc = &sc_wdog[i];
+ cf.cf_name = swwdog_cd.cd_name;
+ cf.cf_atname = swwdog_cd.cd_name;
+ cf.cf_unit = 0;
+ cf.cf_fstate = FSTATE_STAR;
- 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);
+ (void)config_attach_pseudo(&cf);
- if (sysmon_wdog_register(&sc->sc_smw) == 0)
- printf("software watchdog initialized\n");
- else
- printf("unable to register software watchdog "
- "with sysmon\n");
- }
+ 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);
+ const struct sysctlnode *me;
+ int err;
+
+ 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");
+
+ err = sysctl_createv(NULL, 0, NULL, &me, CTLFLAG_READWRITE,
+ CTLTYPE_NODE, sc->sc_smw.smw_name, NULL,
+ NULL, 0, NULL, 0,
+ CTL_MACHDEP, 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_MACHDEP, me->sysctl_num, CTL_CREATE, CTL_EOL);
+}
+
+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 +218,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 +232,11 @@ swwdog_panic(void *vsc)
else
cpu_reboot(0, NULL);
}
+
+SYSCTL_SETUP(sysctl_swwdog, "swwdog subtree setup")
+{
+ sysctl_createv(NULL, 0, NULL, NULL, CTLFLAG_PERMANENT,
+ CTLTYPE_NODE, "machdep", NULL,
+ NULL, 0, NULL, 0,
+ CTL_MACHDEP, CTL_EOL);
+}
Index: src/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
--- src/sys/dev/sysmon/files.sysmon 30 Jan 2010 21:55:28 -0000 1.11
+++ src/sys/dev/sysmon/files.sysmon 18 Jul 2010 23:21:06 -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
Home |
Main Index |
Thread Index |
Old Index