mbox series

[00/10] Convert the IPMI watchdog to use the watchdog

Message ID 20200620174907.20229-1-minyard@acm.org
Headers show
Series Convert the IPMI watchdog to use the watchdog | expand

Message

Corey Minyard June 20, 2020, 5:48 p.m. UTC 
This has been discussed before, and I've been working on it.  I think
this is the minimum that can be done to keep the IPMI watchdog
functionality exactly the same and use the watchdog subsystem.

The first patch is something that I thought might be a bug, and is not
directly related to the changes.

The IPMI watchdog has the following capabilities that need extensions to
the watchdog subsystem:

* The ability to provide read data on the device when a pretimeout
  occurs.
* The ability to dynamically set the timeout and pretimeout with module
  parameters.
* The ability to start the watchdog as soon as the driver starts.

I have no idea if anyone is using the capabilities, but they are there,
unfortunately.

A patch later in the series adds the ability to modify the watchdog time
on a reboot.  This was specifically requested in the past, so I know it
was at least used in the past.  The IPMI watchdog driver can do this
itself, but it's simple to add to the watchdog framework and gets rid of
some redundant code.

This leaves the driver code in the ipmi directory.  After these changes
it could be moved, but it doesn't matter much to me.

Thanks,

-corey

Comments

Guenter Roeck July 19, 2020, 2:25 p.m. UTC | #1 
On Sat, Jun 20, 2020 at 12:49:05PM -0500, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>

> 

> If reboot_timeout is set in the watchdog device struct, set the timeout

> to that value on a reboot.  This way more time can be given for a reboot

> to complete.

> 

I think this should be aligned with watchdog_set_open_deadline(),
ie use a function to set the reboot timeout instead of adding it
to struct watchdog_device.

> Signed-off-by: Corey Minyard <cminyard@mvista.com>

> ---

>  drivers/watchdog/watchdog_core.c | 2 ++

>  include/linux/watchdog.h         | 4 ++++

>  2 files changed, 6 insertions(+)

> 

> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c

> index 03943a34e9fb..5792f9bca645 100644

> --- a/drivers/watchdog/watchdog_core.c

> +++ b/drivers/watchdog/watchdog_core.c

> @@ -165,6 +165,8 @@ static int watchdog_reboot_notifier(struct notifier_block *nb,

>  			if (ret)

>  				return NOTIFY_BAD;

>  		}

> +	} else if (wdd->reboot_timeout) {

> +		watchdog_set_timeout(wdd, wdd->reboot_timeout);


This has no practical impact; the code above checks for SYS_DOWN,
and for whatever reason SYS_DOWN has the same value as SYS_RESTART.
So the only value is for SYS_POWER_OFF, and that should arguably
be included in the first check (meaning we should probably remove
that check entirely, if anything).

Also, the reboot notifier is only called if WDOG_STOP_ON_REBOOT
is set, which contradicts the idea behind this change.

Guenter

>  	}

>  

>  	return NOTIFY_DONE;

> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h

> index 1eefae61215d..0fb57f29346c 100644

> --- a/include/linux/watchdog.h

> +++ b/include/linux/watchdog.h

> @@ -92,6 +92,9 @@ struct watchdog_ops {

>   * @status:	Field that contains the devices internal status bits.

>   * @deferred:	Entry in wtd_deferred_reg_list which is used to

>   *		register early initialized watchdogs.

> + * @reboot_timeout:

> + *		If non-zero, the timeout will be set to this value

> + *		on a reboot.  This lets a reboot be given more time.

>   *

>   * The watchdog_device structure contains all information about a

>   * watchdog timer device.

> @@ -125,6 +128,7 @@ struct watchdog_device {

>  #define WDOG_HW_RUNNING		3	/* True if HW watchdog running */

>  #define WDOG_STOP_ON_UNREGISTER	4	/* Should be stopped on unregister */

>  	struct list_head deferred;

> +	unsigned int reboot_timeout;

>  };

>  

>  #define WATCHDOG_NOWAYOUT		IS_BUILTIN(CONFIG_WATCHDOG_NOWAYOUT)