On Mon, Dec 21, 2015 at 03:49:09PM +0100, Mark Kettenis wrote:
> > Date: Mon, 21 Dec 2015 22:43:46 +0900
> > From: Masao Uebayashi <[email protected]>
> >
> > When stopiing ipmi(4) watchdog, not only set action to disabled but also
> > stop
> > internal timer (clear don't stop flag). Without this our NEC servers fail
> > to stop IPMI watchdog and reset during shutdown process.
> >
> > OK?
>
> See comments below
>
> > diff --git a/sys/dev/ipmi.c b/sys/dev/ipmi.c
> > index 91aa147..d88b7c1 100644
> > --- a/sys/dev/ipmi.c
> > +++ b/sys/dev/ipmi.c
> > @@ -1708,13 +1708,15 @@ ipmi_watchdog(void *arg, int period)
> > APP_GET_WATCHDOG_TIMER, 0, NULL);
> > rc = ipmi_recvcmd(sc, IPMI_GET_WDOG_MAX, &len, wdog);
> >
> > + wdog[IPMI_SET_WDOG_TIMER] &= ~IPMI_WDOG_TIMER_DONTSTOP;
> > + wdog[IPMI_SET_WDOG_TIMER] |= (sc->sc_wdog_period == 0) ?
> > + 0 : IPMI_WDOG_TIMER_DONTSTOP;
> > + wdog[IPMI_SET_WDOG_ACTION] &= ~IPMI_WDOG_ACTION_ACTION_MASK;
> > + wdog[IPMI_SET_WDOG_ACTION] |= (sc->sc_wdog_period == 0) ?
> > + IPMI_WDOG_ACTION_ACTION_DISABLED : IPMI_WDOG_ACTION_ACTION_REBOOT;
>
> At this point, sc->sc_wdog_period hasn't been update yet, so I think
> this is wrong.
Good catch; the order was broken during diff shuffling.
> > /* Period is 10ths/sec */
> > uint16_t timo = htole16(period * 10);
> > -
> > memcpy(&wdog[IPMI_SET_WDOG_TIMOL], &timo, 2);
> > - wdog[IPMI_SET_WDOG_ACTION] &= ~IPMI_WDOG_MASK;
> > - wdog[IPMI_SET_WDOG_ACTION] |= (period == 0) ? IPMI_WDOG_DISABLED :
> > - IPMI_WDOG_REBOOT;
> >
> > rc = ipmi_sendcmd(sc, BMC_SA, BMC_LUN, APP_NETFN,
> > APP_SET_WATCHDOG_TIMER, IPMI_SET_WDOG_MAX, wdog);
> > diff --git a/sys/dev/ipmivar.h b/sys/dev/ipmivar.h
> > index c0ae492..de5ac49 100644
> > --- a/sys/dev/ipmivar.h
> > +++ b/sys/dev/ipmivar.h
> > @@ -106,11 +106,15 @@ struct ipmi_thread {
> > volatile int running;
> > };
> >
> > -#define IPMI_WDOG_MASK 0x03
> > -#define IPMI_WDOG_DISABLED 0x00
> > -#define IPMI_WDOG_REBOOT 0x01
> > -#define IPMI_WDOG_PWROFF 0x02
> > -#define IPMI_WDOG_PWRCYCLE 0x03
> > +#define IPMI_WDOG_TIMER_DONTLOG 0x80
> > +#define IPMI_WDOG_TIMER_DONTSTOP 0x40
> > +#define IPMI_WDOG_TIMER_USE_MASK 0x07
> > +
> > +#define IPMI_WDOG_ACTION_ACTION_MASK 0x03
> > +#define IPMI_WDOG_ACTION_ACTION_DISABLED 0x00
> > +#define IPMI_WDOG_ACTION_ACTION_REBOOT 0x01
> > +#define IPMI_WDOG_ACTION_ACTION_PWROFF 0x02
> > +#define IPMI_WDOG_ACTION_ACTION_PWRCYCLE 0x03
>
> Isn't that a little bit too much action? A single ACTION in the name
> of the define would be better I think.
It is "Timer Actions" byte, whose [2:0] bits are called "timeout action" and
[6:4] are "pre-timeout interrupt". I've renamed these to *_ACTION_TOACT_*.
diff --git a/sys/dev/ipmi.c b/sys/dev/ipmi.c
index 91aa147..82d1ee6 100644
--- a/sys/dev/ipmi.c
+++ b/sys/dev/ipmi.c
@@ -1701,6 +1701,7 @@ ipmi_watchdog(void *arg, int period)
if (period < 10 && period > 0)
period = 10;
+ sc->sc_wdog_period = period;
s = splsoftclock();
/* XXX what to do if poking wdog fails? */
@@ -1708,13 +1709,15 @@ ipmi_watchdog(void *arg, int period)
APP_GET_WATCHDOG_TIMER, 0, NULL);
rc = ipmi_recvcmd(sc, IPMI_GET_WDOG_MAX, &len, wdog);
+ wdog[IPMI_SET_WDOG_TIMER] &= ~IPMI_WDOG_TIMER_DONTSTOP;
+ wdog[IPMI_SET_WDOG_TIMER] |= (sc->sc_wdog_period == 0) ?
+ 0 : IPMI_WDOG_TIMER_DONTSTOP;
+ wdog[IPMI_SET_WDOG_ACTION] &= ~IPMI_WDOG_ACTION_TOACT_MASK;
+ wdog[IPMI_SET_WDOG_ACTION] |= (sc->sc_wdog_period == 0) ?
+ IPMI_WDOG_ACTION_TOACT_DISABLED : IPMI_WDOG_ACTION_TOACT_REBOOT;
/* Period is 10ths/sec */
uint16_t timo = htole16(period * 10);
-
memcpy(&wdog[IPMI_SET_WDOG_TIMOL], &timo, 2);
- wdog[IPMI_SET_WDOG_ACTION] &= ~IPMI_WDOG_MASK;
- wdog[IPMI_SET_WDOG_ACTION] |= (period == 0) ? IPMI_WDOG_DISABLED :
- IPMI_WDOG_REBOOT;
rc = ipmi_sendcmd(sc, BMC_SA, BMC_LUN, APP_NETFN,
APP_SET_WATCHDOG_TIMER, IPMI_SET_WDOG_MAX, wdog);
@@ -1722,6 +1725,5 @@ ipmi_watchdog(void *arg, int period)
splx(s);
- sc->sc_wdog_period = period;
return (period);
}
diff --git a/sys/dev/ipmivar.h b/sys/dev/ipmivar.h
index c0ae492..9939f79 100644
--- a/sys/dev/ipmivar.h
+++ b/sys/dev/ipmivar.h
@@ -106,11 +106,15 @@ struct ipmi_thread {
volatile int running;
};
-#define IPMI_WDOG_MASK 0x03
-#define IPMI_WDOG_DISABLED 0x00
-#define IPMI_WDOG_REBOOT 0x01
-#define IPMI_WDOG_PWROFF 0x02
-#define IPMI_WDOG_PWRCYCLE 0x03
+#define IPMI_WDOG_TIMER_DONTLOG 0x80
+#define IPMI_WDOG_TIMER_DONTSTOP 0x40
+#define IPMI_WDOG_TIMER_USE_MASK 0x07
+
+#define IPMI_WDOG_ACTION_TOACT_MASK 0x03
+#define IPMI_WDOG_ACTION_TOACT_DISABLED 0x00
+#define IPMI_WDOG_ACTION_TOACT_REBOOT 0x01
+#define IPMI_WDOG_ACTION_TOACT_PWROFF 0x02
+#define IPMI_WDOG_ACTION_TOACT_PWRCYCLE 0x03
#define IPMI_WDOG_PRE_DISABLED 0x00
#define IPMI_WDOG_PRE_SMI 0x01
