> 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.
> /* 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.
