Re: [PATCH] ipmi/watchdog: Stop watchdog timer when the current action is 'none'

Re: [PATCH] ipmi/watchdog: Stop watchdog timer when the current action is 'none'

[Corey Minyard]

On Thu, May 13, 2021 at 02:26:36PM +0200, Petr Pavlu wrote:
> When an IPMI watchdog timer is being stopped in ipmi_close() or
> ipmi_ioctl(WDIOS_DISABLECARD), the current watchdog action is updated to
> WDOG_TIMEOUT_NONE and _ipmi_set_timeout(IPMI_SET_TIMEOUT_NO_HB) is called
> to install this action. The latter function ends up invoking
> __ipmi_set_timeout() which makes the actual 'Set Watchdog Timer' IPMI
> request.
> 
> For IPMI 1.0, this operation results in fully stopping the watchdog timer.
> For IPMI >= 1.5, function __ipmi_set_timeout() always specifies the "don't
> stop" flag in the prepared 'Set Watchdog Timer' IPMI request. This causes
> that the watchdog timer has its action correctly updated to 'none' but the
> timer continues to run. A problem is that IPMI firmware can then still log
> an expiration event when the configured timeout is reached, which is
> unexpected because the watchdog timer was requested to be stopped.
> 
> The patch fixes this problem by not setting the "don't stop" flag in
> __ipmi_set_timeout() when the current action is WDOG_TIMEOUT_NONE which
> results in stopping the watchdog timer. This makes the behaviour for
> IPMI >= 1.5 consistent with IPMI 1.0. It also matches the logic in
> __ipmi_heartbeat() which does not allow to reset the watchdog if the
> current action is WDOG_TIMEOUT_NONE as that would start the timer.

Yes, I believe this is correct, though it took a bit to be sure :).
Applied for linux-next.  I'm also requesting backport to stable kernels.

-corey

> 
> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> ---
>  drivers/char/ipmi/ipmi_watchdog.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/char/ipmi/ipmi_watchdog.c b/drivers/char/ipmi/ipmi_watchdog.c
> index 32c334e34d55..e4ff3b50de7f 100644
> --- a/drivers/char/ipmi/ipmi_watchdog.c
> +++ b/drivers/char/ipmi/ipmi_watchdog.c
> @@ -371,16 +371,18 @@ static int __ipmi_set_timeout(struct ipmi_smi_msg  *smi_msg,
>  	data[0] = 0;
>  	WDOG_SET_TIMER_USE(data[0], WDOG_TIMER_USE_SMS_OS);
>  
> -	if ((ipmi_version_major > 1)
> -	    || ((ipmi_version_major == 1) && (ipmi_version_minor >= 5))) {
> -		/* This is an IPMI 1.5-only feature. */
> -		data[0] |= WDOG_DONT_STOP_ON_SET;
> -	} else if (ipmi_watchdog_state != WDOG_TIMEOUT_NONE) {
> -		/*
> -		 * In ipmi 1.0, setting the timer stops the watchdog, we
> -		 * need to start it back up again.
> -		 */
> -		hbnow = 1;
> +	if (ipmi_watchdog_state != WDOG_TIMEOUT_NONE) {
> +		if ((ipmi_version_major > 1) ||
> +		    ((ipmi_version_major == 1) && (ipmi_version_minor >= 5))) {
> +			/* This is an IPMI 1.5-only feature. */
> +			data[0] |= WDOG_DONT_STOP_ON_SET;
> +		} else {
> +			/*
> +			 * In ipmi 1.0, setting the timer stops the watchdog, we
> +			 * need to start it back up again.
> +			 */
> +			hbnow = 1;
> +		}
>  	}
>  
>  	data[1] = 0;
> -- 
> 2.26.2
>