Re: Slight interface change on the watchdog fido

On Sunday 10 December 2006 05:04, Nick Hibma wrote:
> I'm planning on committing the following change to make the implementations 
of 
> the various hardware watchdogs more consistent. A timeout of 0 passed to the 
> ioctl is now a valid input and will disable the watchdog. Previously it 
> produced an error for the Elan chip watchdog, which is _not_ what you want 
to 
> see when disarming a watchdog :-) I'd appreciate review of the individual 
> changes in the files by the following people. The change as a whole was 
> discussed with phk.
> 
>    cognet_at_freebsd.org	i80321_wdog.c (*)
>    des			ichwd.c (**)
>    ambrisko		ipmi.c
>    marius			mk48txx.c
>    phk			kern_clock.c, elan-mmcr.c, watchdog.c (**)
> 
> (*) The i80321_wdog.c cannot be disarmed. Is this correct?
> (**) These have been tested to arm, disarm, and fire. Others have only been 
> compile tested.
> 
> This change has been tested on 6.2-STABLE and 7-CURRENT.
> 
> Index: sys/dev/ipmi/ipmi.c
> ===================================================================
> RCS file: /home/ncvs/src/sys/dev/ipmi/ipmi.c,v
> retrieving revision 1.3.2.3
> diff -u -r1.3.2.3 ipmi.c
> --- sys/dev/ipmi/ipmi.c	19 Oct 2006 14:50:48 -0000	1.3.2.3
> +++ sys/dev/ipmi/ipmi.c	9 Dec 2006 12:40:47 -0000
> _at__at_ -649,25 +649,14 _at__at_
>   	struct ipmi_softc *sc = arg;
>   	unsigned int timeout;
> 
> -	/* disable / enable */
> -	if (!(cmd & WD_ACTIVE)) {
> -		ipmi_set_watchdog(sc, 0);
> -		*error = 0;
> -		return;
> -	}
> -
>   	cmd &= WD_INTERVAL;
> -	/* convert from power-of-to-ns to WDT ticks */
> -	if (cmd >= 64) {
> -		*error = EINVAL;
> -		return;
> +	if (cmd > 0 && cmd <= 63) {
> +		timeout = ((uint64_t)1 << cmd) / 1800000000;
> +		ipmi_set_watchdog(sc, timeout);
> +		*error = 0;
> +	} else {
> +		ipmi_set_watchdog(sc, 0);
>   	}
> -	timeout = ((uint64_t)1 << cmd) / 1800000000;
> -
> -	/* reload */
> -	ipmi_set_watchdog(sc, timeout);
> -
> -	*error = 0;
>   }
> 
>   #ifdef CLONING

It would be nice to not lose the comments.  Might also be nice to reduce the 
diff (so it doesn't have to reindent everything) by just adding a simple test
after masking off WD_INTERVAL like so:

	if (cmd == 0 || cmd >= 64) {
		ipmi_set_watchdog(sc, 0);
		return;
	}

-- 
John Baldwin