Re: Add NULL power handler to x86/ipmi(4)

[Paul Goyette <paul%whooppee.com@localhost>]

On Sat, 17 Jul 2010, David Young wrote:

> > > ipmi(4) should probably not suspend if its watchdog timer is active.
> >
> > Would it be sufficient for ipmi(4) to refuse to suspend (return
> > false from the suspend method) if the watchdog is active?
>
> Yes.  I think that's the right thing to do for now.

This is actually fairly easy to do.

> > Or should it make some sort of effort to disable the watchdog, and
> > attempt to reenable the watchdog during the subsequent resume?
>
> That defeats the purpose of a watchdog timer, at least for my purposes.
> This is a local policy choice that should probably not be hard-coded.
> Maybe we need additional watchdog modes?
>
> 1 Watchdog countdown stops during suspension, restarts after

Probably a bit more difficult than #3 below, and it assumes that the 
remaining time is available.

> 2 Watchdog keeps counting down during suspend (we'll wake periodically
>  to tickle it)

This might be rather tricky.  Depending on the type of suspend, the time 
for a wake-up to occur could be faily long and/or variable.  And there 
might not even be a way for the system to schedule its own wake event.

> 3 Watchdog prevents suspension

As I indicated, this is fairly easy to do, simply check if the w-dog is 
armed or not.

The attached patch provides an ipmi_suspend() method to implement #3. 
But I suspect that this is really a more generic watchdog capability, 
and it should be implemented for the entire class of watchdogs rather 
than only for ipmi(4)?

Comments?


-------------------------------------------------------------------------
| 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: ipmi.c
===================================================================
RCS file: /cvsroot/src/sys/arch/x86/x86/ipmi.c,v
retrieving revision 1.46
diff -u -p -r1.46 ipmi.c
--- ipmi.c      10 Apr 2010 19:02:39 -0000      1.46
+++ ipmi.c      17 Jul 2010 02:40:27 -0000
@@ -225,6 +225,8 @@ int ipmi_sensor_status(struct ipmi_softc
 int     add_child_sensors(struct ipmi_softc *, uint8_t *, int, int, int,
     int, int, int, const char *);
 
+bool   ipmi_suspend(device_t, const pmf_qual_t *);
+
 struct ipmi_if kcs_if = {
        "KCS",
        IPMI_IF_KCS_NREGS,
@@ -1934,6 +1936,10 @@ ipmi_thread(void *cookie)
        sc->sc_wdog.smw_tickle = ipmi_watchdog_tickle;
        sysmon_wdog_register(&sc->sc_wdog);
 
+       /* Set up a power handler so we can possibly sleep */
+       if (!pmf_device_register(self, ipmi_suspend, NULL))
+                aprint_error_dev(self, "couldn't establish a power handler\n");
+
        mutex_enter(&sc->sc_poll_mtx);
        while (sc->sc_thread_running) {
                ipmi_refresh_sensors(sc);
@@ -2093,3 +2099,14 @@ ipmi_dotickle(struct ipmi_softc *sc)
                    device_xname(sc->sc_dev), rc);
        }
 }
+
+bool
+ipmi_suspend(device_t dev, const pmf_qual_t *qual)
+{
+       struct ipmi_softc *sc = device_private(dev);
+
+       /* Don't allow suspend if watchdog is armed */
+       if ((sc->sc_wdog.smw_mode & WDOG_MODE_MASK) != WDOG_MODE_DISARMED)
+               return false;
+       return true;
+}