Re: [PATCH 1/1] watchdog:improve w83627hf_wdt to timeout in minutes

[Guenter Roeck]

On Sun, Mar 24, 2013 at 09:15:57PM -0700, Tony Chung wrote:
> The current maximum of 255 seconds is insufficient.
> For example, crash dump could take 5+ minutes.
> 
> Signed-off-by: Tony Chung <tonychung00@xxxxxxxxx>

I would suggest to use the port to the watchdog infrastructure for any
changes on this driver.

> ---
>  drivers/watchdog/w83627hf_wdt.c |   73 ++++++++++++++++++++++++++++++--------
>  1 files changed, 57 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/watchdog/w83627hf_wdt.c b/drivers/watchdog/w83627hf_wdt.c
> index 92f1326..d26adfb 100644
> --- a/drivers/watchdog/w83627hf_wdt.c
> +++ b/drivers/watchdog/w83627hf_wdt.c
> @@ -58,7 +58,7 @@ MODULE_PARM_DESC(wdt_io, "w83627hf/thf WDT io port (default 0x2E)");
>  static int timeout = WATCHDOG_TIMEOUT;	/* in seconds */
>  module_param(timeout, int, 0);
>  MODULE_PARM_DESC(timeout,
> -		"Watchdog timeout in seconds. 1 <= timeout <= 255, default="
> +		"Watchdog timeout in seconds. 1 <= timeout <= 15300, default="
>  				__MODULE_STRING(WATCHDOG_TIMEOUT) ".");
>  
>  static bool nowayout = WATCHDOG_NOWAYOUT;
> @@ -67,6 +67,29 @@ MODULE_PARM_DESC(nowayout,
>  		"Watchdog cannot be stopped once started (default="
>  				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>  
> +/* timeout unit in minute */
> +static bool use_minute;
> +static char *unit;
> +static int max_timeout = 15300; /* 255*60 seconds */
> +
> +static inline int wdt_round_secs_to_minute(int t)
> +{
> +	return (t + 59) / 60;
> +}

Use DIV_ROUND_UP().

> +
> +static inline int wdt_use_minute(int t)
> +{
> +	if (t > 255) {
> +		t = wdt_round_secs_to_minute(t);
> +		use_minute = 1;

bool has true / false.

> +		unit = "minutes";
> +	} else {
> +		use_minute = 0;
> +		unit = "secs";
> +	}
> +	return t;
> +}
> +
>  /*
>   *	Kernel methods.
>   */
> @@ -107,6 +130,23 @@ static void w83627hf_unselect_wd_register(void)
>  	outb_p(0xAA, WDT_EFER); /* Leave extended function mode */
>  }
>  
> +/* set CRF5 register */
> +static void w83627hf_set_crf5(void)
> +{
> +	unsigned char t;
> +
> +	outb_p(0xF5, WDT_EFER); /* Select CRF5 */
> +	t = inb_p(WDT_EFDR);      /* read CRF5 */
> +	t &= ~0x0C;               /* set second mode & disable keyboard
> +				    turning off watchdog */
> +	t |= 0x02;		  /* enable the WDTO# output low pulse
> +				    to the KBRST# pin (PIN60) */
> +	if (use_minute)
> +		t |= 0x80;        /* set timeout in minute */
> +
> +	outb_p(t, WDT_EFDR);    /* Write back to CRF5 */
> +}
> +
>  /* tyan motherboards seem to set F5 to 0x4C ?
>   * So explicitly init to appropriate value. */
>  
> @@ -116,21 +156,16 @@ static void w83627hf_init(void)
>  
>  	w83627hf_select_wd_register();
>  
> +	w83627hf_set_crf5();
> +
>  	outb_p(0xF6, WDT_EFER); /* Select CRF6 */
>  	t = inb_p(WDT_EFDR);      /* read CRF6 */
>  	if (t != 0) {
> -		pr_info("Watchdog already running. Resetting timeout to %d sec\n",
> -			timeout);
> +		pr_info("Watchdog already running.  Reset timeout to %d %s\n",
> +			timeout, unit);
>  		outb_p(timeout, WDT_EFDR);    /* Write back to CRF6 */
>  	}
>  
> -	outb_p(0xF5, WDT_EFER); /* Select CRF5 */
> -	t = inb_p(WDT_EFDR);      /* read CRF5 */
> -	t &= ~0x0C;               /* set second mode & disable keyboard
> -				    turning off watchdog */
> -	t |= 0x02;		  /* enable the WDTO# output low pulse
> -				    to the KBRST# pin (PIN60) */
> -	outb_p(t, WDT_EFDR);    /* Write back to CRF5 */
>  
>  	outb_p(0xF7, WDT_EFER); /* Select CRF7 */
>  	t = inb_p(WDT_EFDR);      /* read CRF7 */
> @@ -147,6 +182,9 @@ static void wdt_set_time(int timeout)
>  
>  	w83627hf_select_wd_register();
>  
> +	if (use_minute)
> +		w83627hf_set_crf5();

What if the new timeout is less than 256 seconds ?

> +
>  	outb_p(0xF6, WDT_EFER);    /* Select CRF6 */
>  	outb_p(timeout, WDT_EFDR); /* Write Timeout counter to CRF6 */
>  
> @@ -169,9 +207,9 @@ static int wdt_disable(void)
>  
>  static int wdt_set_heartbeat(int t)
>  {
> -	if (t < 1 || t > 255)
> +	if (t < 1 || t > max_timeout)
>  		return -EINVAL;
> -	timeout = t;
> +	timeout = wdt_use_minute(t);

This should probably also update the watchdog timer register.

>  	return 0;
>  }
>  
> @@ -262,9 +300,11 @@ static long wdt_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  		wdt_ping();
>  		/* Fall */
>  	case WDIOC_GETTIMEOUT:
> -		return put_user(timeout, p);
> +		timeval = (use_minute) ? timeout * 60 : timeout;

Isn't timeout always in seconds ?

> +		return put_user(timeval, p);
>  	case WDIOC_GETTIMELEFT:
>  		timeval = wdt_get_time();
> +		timeval = (use_minute) ? timeval * 60 : timeval;
>  		return put_user(timeval, p);
>  	default:
>  		return -ENOTTY;
> @@ -346,7 +386,8 @@ static int __init wdt_init(void)
>  
>  	if (wdt_set_heartbeat(timeout)) {
>  		wdt_set_heartbeat(WATCHDOG_TIMEOUT);
> -		pr_info("timeout value must be 1 <= timeout <= 255, using %d\n",
> +		pr_info("timeout value must be 1 <= timeout <= %d, using %d\n",
> +			max_timeout,

That is still a constant, isn't it ? Why use a variable ?

>  			WATCHDOG_TIMEOUT);
>  	}
>  
> @@ -371,8 +412,8 @@ static int __init wdt_init(void)
>  		goto unreg_reboot;
>  	}
>  
> -	pr_info("initialized. timeout=%d sec (nowayout=%d)\n",
> -		timeout, nowayout);
> +	pr_info("initialized. timeout=%d %s (nowayout=%d)\n",
> +		timeout, unit, nowayout);
>  
>  out:
>  	return ret;
> -- 
> 1.7.0.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/