Re: Add NULL power handler to x86/ipmi(4)
On Jul 16, 2010, at 5:04 PM, Steven Bellovin wrote: On Jul 16, 2010, at 7:52 25PM, David Young wrote: On Fri, Jul 16, 2010 at 04:43:02PM -0700, Paul Goyette wrote: Is there any reason anyone can think of to not add a NULL power handler to the ipmi(4) driver? I can't see any reason for anything special to happen either at suspend or resume, and the lack of a power handler prevents the system from going to sleep at all. ipmi(4) should probably not suspend if its watchdog timer is active. Watchdog timers and suspend pose an interesting philosophical problem: should they be running when the system is suspended? I suspect not -- but what if the system suspends when it shouldn't? Note that some watchdogs can't be disabled.
Re: Add NULL power handler to x86/ipmi(4)
On Fri, Jul 16, 2010 at 06:14:39PM -0700, Paul Goyette wrote: On Fri, 16 Jul 2010, David Young wrote: On Fri, Jul 16, 2010 at 04:43:02PM -0700, Paul Goyette wrote: Is there any reason anyone can think of to not add a NULL power handler to the ipmi(4) driver? I can't see any reason for anything special to happen either at suspend or resume, and the lack of a power handler prevents the system from going to sleep at all. 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. 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 2 Watchdog keeps counting down during suspend (we'll wake periodically to tickle it) 3 Watchdog prevents suspension Dave -- David Young OJC Technologies dyo...@ojctech.com Urbana, IL * (217) 278-3933
Re: Add NULL power handler to x86/ipmi(4)
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 - 1.46
+++ ipmi.c 17 Jul 2010 02:40:27 -
@@ -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;
+}
Re: Add NULL power handler to x86/ipmi(4)
On Sat, 17 Jul 2010, David Young wrote: It is a generic capability. ... That's the conclusion I came to. ... ISTM watchdog timers should eventually be refactored in this way: each watchdog timer in the system should have a corresponding pseudo-device, an instance of wdog(4). wdog(4) provides its own suspension/resumption/detachment routines. Let wdog_suspend() return false if the watchdog is active (for now). ipmi0 should attach wdog0 using a wdogbus interface attribute. Let struct sysmon_wdog become the watchdog attachment arguments, wdog_attach_args. That's probably a bit more work than I want to commit to right now. I was hoping there would be an easier way, since the sysmon pseudo-device already exists (with exactly one device-minor for wdogs). But I could not find anywhere a device_t, so no way to register a power handler. A also looked at just creating a new pmf device class for watchdogs, since most watchdogs are actually associated with some other hardware device (and therefore there's a device_t to use when registering the handler). But there's pseudo-device swwdog(4) that has no hardware behind it, so I ended up back with the same problem! Your patch looks fine. Thanks. I will commit this for now, with a big XXX in the commit log. I've also got a patch for itesio(4). Currently, itesio registers a NULL suspend handler, whether or not the particular device includes a wdog. My patch will register a real suspend handler if the wdog is present. I'll add watchdog-factoring to my To-Do list, but for now I'll just commit the changes for itesio(4) and ipmi(4). - | 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 | -
Re: Add NULL power handler to x86/ipmi(4)
On Fri, Jul 16, 2010 at 04:43:02PM -0700, Paul Goyette wrote: Is there any reason anyone can think of to not add a NULL power handler to the ipmi(4) driver? I can't see any reason for anything special to happen either at suspend or resume, and the lack of a power handler prevents the system from going to sleep at all. ipmi(4) should probably not suspend if its watchdog timer is active. Dave -- David Young OJC Technologies dyo...@ojctech.com Urbana, IL * (217) 278-3933
