Re: [3/3] watchdog: i6300esb: do not hardcode heartbeat limits

[Guenter Roeck <linux@roeck-us.net>]

On Mon, Oct 16, 2017 at 07:25:28PM +0100, Radu Rendec wrote:
> The minimum, maximum and default values for the watchdog heartbeat
> (timeout) were hardcoded in several places (including module parameter
> description and warning message for invalid module parameter value).
> 
> This patch adds macros for the aforementioned values and replaces all
> occurences of hardcoded values by these macros.
> 
> Signed-off-by: Radu Rendec <rrendec@arista.com>
> ---
>  drivers/watchdog/i6300esb.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/watchdog/i6300esb.c b/drivers/watchdog/i6300esb.c
> index 855b4f80ad09..68d76512b08f 100644
> --- a/drivers/watchdog/i6300esb.c
> +++ b/drivers/watchdog/i6300esb.c
> @@ -82,12 +82,16 @@ static int cards_found;
>  
>  /* module parameters */
>  /* 30 sec default heartbeat (1 < heartbeat < 2*1023) */
> -#define WATCHDOG_HEARTBEAT 30
> -static int heartbeat = WATCHDOG_HEARTBEAT;  /* in seconds */
> +#define ESB_HEARTBEAT_MIN 1
> +#define ESB_HEARTBEAT_MAX 2046
> +#define ESB_HEARTBEAT_DEFAULT 30
> +static int heartbeat = ESB_HEARTBEAT_DEFAULT;  /* in seconds */

Ah, I just noticed. This defeats watchdog_init_timeout().

In patch 1, the timeout in struct watchdog_device to the default,
and set this variable to 0. watchdog_init_timeout() will then overwrite
the default (which is set in struct watchdog_device) with the module
parameter if the module parameter is set to a value != 0.

Thanks,
Guenter

>  module_param(heartbeat, int, 0);
>  MODULE_PARM_DESC(heartbeat,
> -		"Watchdog heartbeat in seconds. (1<heartbeat<2046, default="
> -				__MODULE_STRING(WATCHDOG_HEARTBEAT) ")");
> +	"Watchdog heartbeat in seconds. ("
> +	__MODULE_STRING(ESB_HEARTBEAT_MIN) "<heartbeat<"
> +	__MODULE_STRING(ESB_HEARTBEAT_MAX) ", default="
> +	__MODULE_STRING(ESB_HEARTBEAT_DEFAULT) ")");
>  
>  static bool nowayout = WATCHDOG_NOWAYOUT;
>  module_param(nowayout, bool, 0);
> @@ -325,17 +329,19 @@ static int esb_probe(struct pci_dev *pdev,
>  
>  	/* Check that the heartbeat value is within it's range;
>  	   if not reset to the default */
> -	if (heartbeat < 0x1 || heartbeat > 2 * 0x03ff) {
> -		heartbeat = WATCHDOG_HEARTBEAT;
> -		pr_info("heartbeat value must be 1<heartbeat<2046, using %d\n",
> +	if (heartbeat < ESB_HEARTBEAT_MIN || heartbeat > ESB_HEARTBEAT_MAX) {
> +		heartbeat = ESB_HEARTBEAT_DEFAULT;
> +		pr_info("heartbeat value must be "
> +			__MODULE_STRING(ESB_HEARTBEAT_MIN) "<heartbeat<"
> +			__MODULE_STRING(ESB_HEARTBEAT_MAX) ", using %d\n",
>  			heartbeat);
>  	}
>  
>  	/* Initialize the watchdog and make sure it does not run */
>  	edev->wdd.info = &esb_info;
>  	edev->wdd.ops = &esb_ops;
> -	edev->wdd.min_timeout = 1;
> -	edev->wdd.max_timeout = 2 * 0x03ff;
> +	edev->wdd.min_timeout = ESB_HEARTBEAT_MIN;
> +	edev->wdd.max_timeout = ESB_HEARTBEAT_MAX;
>  	watchdog_init_timeout(&edev->wdd, heartbeat, NULL);
>  	watchdog_set_nowayout(&edev->wdd, nowayout);
>  	esb_initdevice(edev);