[PATCH 01/12] watchdog: NULL the default governor if it is unregistered

[PATCH 01/12] watchdog: NULL the default governor if it is unregistered

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Otherwise it could be used after being freed.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 drivers/watchdog/watchdog_pretimeout.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/watchdog/watchdog_pretimeout.c b/drivers/watchdog/watchdog_pretimeout.c
index 01ca84be240f..b45041b0ef39 100644
--- a/drivers/watchdog/watchdog_pretimeout.c
+++ b/drivers/watchdog/watchdog_pretimeout.c
@@ -162,6 +162,8 @@ void watchdog_unregister_governor(struct watchdog_governor *gov)
 			break;
 		}
 	}
+	if (gov == default_gov)
+		default_gov = NULL;
 
 	spin_lock_irq(&pretimeout_lock);
 	list_for_each_entry(p, &pretimeout_list, entry)
-- 
2.17.1

Re: [PATCH 01/12] watchdog: NULL the default governor if it is unregistered

[Guenter Roeck]

On Mon, Aug 19, 2019 at 03:37:00PM -0500, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Otherwise it could be used after being freed.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  drivers/watchdog/watchdog_pretimeout.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/watchdog/watchdog_pretimeout.c b/drivers/watchdog/watchdog_pretimeout.c
> index 01ca84be240f..b45041b0ef39 100644
> --- a/drivers/watchdog/watchdog_pretimeout.c
> +++ b/drivers/watchdog/watchdog_pretimeout.c
> @@ -162,6 +162,8 @@ void watchdog_unregister_governor(struct watchdog_governor *gov)
>  			break;
>  		}
>  	}
> +	if (gov == default_gov)
> +		default_gov = NULL;
>  

Good catch.

I think this should be inside the spinlock. I do wonder though why it
was removed with commit da0d12ff2b82 ("watchdog: pretimeout: add panic
pretimeout governor"). I think it was wrong to remove it, but I may be
missing something. Vladimir, do you recall ?

Guenter

>  	spin_lock_irq(&pretimeout_lock);
>  	list_for_each_entry(p, &pretimeout_list, entry)
> -- 
> 2.17.1
> 

[PATCH 02/12] watchdog: Add the ability to provide data to read

[minyard]

From: Corey Minyard <cminyard@mvista.com>

This is for the read data pretimeout governor.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 drivers/watchdog/watchdog_core.c |   3 +
 drivers/watchdog/watchdog_dev.c  | 113 +++++++++++++++++++++++++++++++
 include/linux/watchdog.h         |   5 ++
 3 files changed, 121 insertions(+)

diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index 21e8085b848b..80149ac229fc 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -216,6 +216,9 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
 		return id;
 	wdd->id = id;
 
+	spin_lock_init(&wdd->readlock);
+	init_waitqueue_head(&wdd->read_q);
+
 	ret = watchdog_dev_register(wdd);
 	if (ret) {
 		ida_simple_remove(&watchdog_ida, id);
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index dbd2ad4c9294..8e8304607a8c 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -44,6 +44,8 @@
 #include <linux/types.h>	/* For standard types (like size_t) */
 #include <linux/watchdog.h>	/* For watchdog specific items */
 #include <linux/uaccess.h>	/* For copy_to_user/put_user/... */
+#include <linux/poll.h>		/* For poll_table/... */
+#include <linux/sched/signal.h>	/* For signal_pending */
 
 #include <uapi/linux/sched/types.h>	/* For struct sched_param */
 
@@ -929,12 +931,120 @@ static int watchdog_release(struct inode *inode, struct file *file)
 	return 0;
 }
 
+static ssize_t watchdog_read(struct file *file,
+			     char        __user *buf,
+			     size_t      count,
+			     loff_t      *ppos)
+{
+	struct watchdog_core_data *wd_data = file->private_data;
+	struct watchdog_device *wdd;
+	int err = 0;
+	wait_queue_entry_t wait;
+	char dummy = 1;
+
+	if (count <= 0)
+		return 0;
+
+	mutex_lock(&wd_data->lock);
+
+	wdd = wd_data->wdd;
+	if (!wdd)
+		goto done;
+
+	/*
+	 * Reading returns if the pretimeout has gone off, and it only does
+	 * it once per pretimeout.
+	 */
+	spin_lock_irq(&wdd->readlock);
+	while (!wdd->data_to_read) {
+		if (file->f_flags & O_NONBLOCK) {
+			err = -EAGAIN;
+			goto out;
+		}
+
+		init_waitqueue_entry(&wait, current);
+		add_wait_queue(&wdd->read_q, &wait);
+		set_current_state(TASK_INTERRUPTIBLE);
+		spin_unlock_irq(&wdd->readlock);
+		schedule();
+		spin_lock_irq(&wdd->readlock);
+		remove_wait_queue(&wdd->read_q, &wait);
+
+		if (signal_pending(current)) {
+			err = -ERESTARTSYS;
+			goto out;
+		}
+	}
+	dummy = wdd->data_to_read;
+	wdd->data_to_read = 0;
+
+ out:
+	spin_unlock_irq(&wdd->readlock);
+
+	if (err == 0) {
+		if (copy_to_user(buf, &dummy, 1))
+			err = -EFAULT;
+		else
+			err = 1;
+	}
+
+ done:
+	mutex_unlock(&wd_data->lock);
+
+	return err;
+}
+
+static __poll_t watchdog_poll(struct file *file, poll_table *wait)
+{
+	struct watchdog_core_data *wd_data = file->private_data;
+	struct watchdog_device *wdd;
+	__poll_t mask = 0;
+
+	mutex_lock(&wd_data->lock);
+
+	wdd = wd_data->wdd;
+	if (!wdd)
+		goto done;
+
+	poll_wait(file, &wdd->read_q, wait);
+
+	spin_lock_irq(&wdd->readlock);
+	if (wdd->data_to_read)
+		mask |= (EPOLLIN | EPOLLRDNORM);
+	spin_unlock_irq(&wdd->readlock);
+
+done:
+	mutex_unlock(&wd_data->lock);
+	return mask;
+}
+
+static int watchdog_fasync(int fd, struct file *file, int on)
+{
+	struct watchdog_core_data *wd_data = file->private_data;
+	struct watchdog_device *wdd;
+	int err = -ENODEV;
+
+	mutex_lock(&wd_data->lock);
+
+	wdd = wd_data->wdd;
+	if (!wdd)
+		goto done;
+
+	err = fasync_helper(fd, file, on, &wdd->fasync_q);
+done:
+	mutex_unlock(&wd_data->lock);
+	return err;
+}
+
 static const struct file_operations watchdog_fops = {
 	.owner		= THIS_MODULE,
 	.write		= watchdog_write,
 	.unlocked_ioctl	= watchdog_ioctl,
 	.open		= watchdog_open,
 	.release	= watchdog_release,
+	.read		= watchdog_read,
+	.poll		= watchdog_poll,
+	.fasync		= watchdog_fasync,
 };
 
 static struct miscdevice watchdog_miscdev = {
@@ -970,6 +1080,9 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
 	if (IS_ERR_OR_NULL(watchdog_kworker))
 		return -ENODEV;
 
+	spin_lock_init(&wdd->readlock);
+	init_waitqueue_head(&wdd->read_q);
+
 	kthread_init_work(&wd_data->work, watchdog_ping_work);
 	hrtimer_init(&wd_data->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
 	wd_data->timer.function = watchdog_timer_expired;
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 417d9f37077a..e34501a822f0 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -117,6 +117,11 @@ 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;
+
+	spinlock_t readlock;
+	bool data_to_read;
+	struct wait_queue_head read_q;
+	struct fasync_struct *fasync_q;
 };
 
 #define WATCHDOG_NOWAYOUT		IS_BUILTIN(CONFIG_WATCHDOG_NOWAYOUT)
-- 
2.17.1

Re: [PATCH 02/12] watchdog: Add the ability to provide data to read

[Guenter Roeck]

On Mon, Aug 19, 2019 at 03:37:01PM -0500, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> This is for the read data pretimeout governor.
> 

I am missing an explanation how this is useful and necessary,
and what problem it solves that can not be solved differently.
For my part I don't immediately see the benefits.

Guenter

> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  drivers/watchdog/watchdog_core.c |   3 +
>  drivers/watchdog/watchdog_dev.c  | 113 +++++++++++++++++++++++++++++++
>  include/linux/watchdog.h         |   5 ++
>  3 files changed, 121 insertions(+)
> 
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index 21e8085b848b..80149ac229fc 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -216,6 +216,9 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
>  		return id;
>  	wdd->id = id;
>  
> +	spin_lock_init(&wdd->readlock);
> +	init_waitqueue_head(&wdd->read_q);
> +
>  	ret = watchdog_dev_register(wdd);
>  	if (ret) {
>  		ida_simple_remove(&watchdog_ida, id);
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index dbd2ad4c9294..8e8304607a8c 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -44,6 +44,8 @@
>  #include <linux/types.h>	/* For standard types (like size_t) */
>  #include <linux/watchdog.h>	/* For watchdog specific items */
>  #include <linux/uaccess.h>	/* For copy_to_user/put_user/... */
> +#include <linux/poll.h>		/* For poll_table/... */
> +#include <linux/sched/signal.h>	/* For signal_pending */
>  
>  #include <uapi/linux/sched/types.h>	/* For struct sched_param */
>  
> @@ -929,12 +931,120 @@ static int watchdog_release(struct inode *inode, struct file *file)
>  	return 0;
>  }
>  
> +static ssize_t watchdog_read(struct file *file,
> +			     char        __user *buf,
> +			     size_t      count,
> +			     loff_t      *ppos)
> +{
> +	struct watchdog_core_data *wd_data = file->private_data;
> +	struct watchdog_device *wdd;
> +	int err = 0;
> +	wait_queue_entry_t wait;
> +	char dummy = 1;
> +
> +	if (count <= 0)
> +		return 0;
> +
> +	mutex_lock(&wd_data->lock);
> +
> +	wdd = wd_data->wdd;
> +	if (!wdd)
> +		goto done;
> +
> +	/*
> +	 * Reading returns if the pretimeout has gone off, and it only does
> +	 * it once per pretimeout.
> +	 */
> +	spin_lock_irq(&wdd->readlock);
> +	while (!wdd->data_to_read) {
> +		if (file->f_flags & O_NONBLOCK) {
> +			err = -EAGAIN;
> +			goto out;
> +		}
> +
> +		init_waitqueue_entry(&wait, current);
> +		add_wait_queue(&wdd->read_q, &wait);
> +		set_current_state(TASK_INTERRUPTIBLE);
> +		spin_unlock_irq(&wdd->readlock);
> +		schedule();
> +		spin_lock_irq(&wdd->readlock);
> +		remove_wait_queue(&wdd->read_q, &wait);
> +
> +		if (signal_pending(current)) {
> +			err = -ERESTARTSYS;
> +			goto out;
> +		}
> +	}
> +	dummy = wdd->data_to_read;
> +	wdd->data_to_read = 0;
> +
> + out:
> +	spin_unlock_irq(&wdd->readlock);
> +
> +	if (err == 0) {
> +		if (copy_to_user(buf, &dummy, 1))
> +			err = -EFAULT;
> +		else
> +			err = 1;
> +	}
> +
> + done:
> +	mutex_unlock(&wd_data->lock);
> +
> +	return err;
> +}
> +
> +static __poll_t watchdog_poll(struct file *file, poll_table *wait)
> +{
> +	struct watchdog_core_data *wd_data = file->private_data;
> +	struct watchdog_device *wdd;
> +	__poll_t mask = 0;
> +
> +	mutex_lock(&wd_data->lock);
> +
> +	wdd = wd_data->wdd;
> +	if (!wdd)
> +		goto done;
> +
> +	poll_wait(file, &wdd->read_q, wait);
> +
> +	spin_lock_irq(&wdd->readlock);
> +	if (wdd->data_to_read)
> +		mask |= (EPOLLIN | EPOLLRDNORM);
> +	spin_unlock_irq(&wdd->readlock);
> +
> +done:
> +	mutex_unlock(&wd_data->lock);
> +	return mask;
> +}
> +
> +static int watchdog_fasync(int fd, struct file *file, int on)
> +{
> +	struct watchdog_core_data *wd_data = file->private_data;
> +	struct watchdog_device *wdd;
> +	int err = -ENODEV;
> +
> +	mutex_lock(&wd_data->lock);
> +
> +	wdd = wd_data->wdd;
> +	if (!wdd)
> +		goto done;
> +
> +	err = fasync_helper(fd, file, on, &wdd->fasync_q);
> +done:
> +	mutex_unlock(&wd_data->lock);
> +	return err;
> +}
> +
>  static const struct file_operations watchdog_fops = {
>  	.owner		= THIS_MODULE,
>  	.write		= watchdog_write,
>  	.unlocked_ioctl	= watchdog_ioctl,
>  	.open		= watchdog_open,
>  	.release	= watchdog_release,
> +	.read		= watchdog_read,
> +	.poll		= watchdog_poll,
> +	.fasync		= watchdog_fasync,
>  };
>  
>  static struct miscdevice watchdog_miscdev = {
> @@ -970,6 +1080,9 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
>  	if (IS_ERR_OR_NULL(watchdog_kworker))
>  		return -ENODEV;
>  
> +	spin_lock_init(&wdd->readlock);
> +	init_waitqueue_head(&wdd->read_q);
> +
>  	kthread_init_work(&wd_data->work, watchdog_ping_work);
>  	hrtimer_init(&wd_data->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
>  	wd_data->timer.function = watchdog_timer_expired;
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index 417d9f37077a..e34501a822f0 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -117,6 +117,11 @@ 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;
> +
> +	spinlock_t readlock;
> +	bool data_to_read;
> +	struct wait_queue_head read_q;
> +	struct fasync_struct *fasync_q;
>  };
>  
>  #define WATCHDOG_NOWAYOUT		IS_BUILTIN(CONFIG_WATCHDOG_NOWAYOUT)
> -- 
> 2.17.1
> 

Re: [PATCH 02/12] watchdog: Add the ability to provide data to read

[Guenter Roeck]

On Mon, Aug 19, 2019 at 03:37:01PM -0500, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> This is for the read data pretimeout governor.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>

On further thought, I think it would be a bad idea to add this
functionality: It changes the userspace ABI for accessing the watchdog
device. Today, when a watchdog device is opened, it does not provide
read data, it does not hang, and returns immediately. A "cat" from it
is an easy and quick means to test if a watchdog works.

Guenter

> ---
>  drivers/watchdog/watchdog_core.c |   3 +
>  drivers/watchdog/watchdog_dev.c  | 113 +++++++++++++++++++++++++++++++
>  include/linux/watchdog.h         |   5 ++
>  3 files changed, 121 insertions(+)
> 
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index 21e8085b848b..80149ac229fc 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -216,6 +216,9 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
>  		return id;
>  	wdd->id = id;
>  
> +	spin_lock_init(&wdd->readlock);
> +	init_waitqueue_head(&wdd->read_q);
> +
>  	ret = watchdog_dev_register(wdd);
>  	if (ret) {
>  		ida_simple_remove(&watchdog_ida, id);
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index dbd2ad4c9294..8e8304607a8c 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -44,6 +44,8 @@
>  #include <linux/types.h>	/* For standard types (like size_t) */
>  #include <linux/watchdog.h>	/* For watchdog specific items */
>  #include <linux/uaccess.h>	/* For copy_to_user/put_user/... */
> +#include <linux/poll.h>		/* For poll_table/... */
> +#include <linux/sched/signal.h>	/* For signal_pending */
>  
>  #include <uapi/linux/sched/types.h>	/* For struct sched_param */
>  
> @@ -929,12 +931,120 @@ static int watchdog_release(struct inode *inode, struct file *file)
>  	return 0;
>  }
>  
> +static ssize_t watchdog_read(struct file *file,
> +			     char        __user *buf,
> +			     size_t      count,
> +			     loff_t      *ppos)
> +{
> +	struct watchdog_core_data *wd_data = file->private_data;
> +	struct watchdog_device *wdd;
> +	int err = 0;
> +	wait_queue_entry_t wait;
> +	char dummy = 1;
> +
> +	if (count <= 0)
> +		return 0;
> +
> +	mutex_lock(&wd_data->lock);
> +
> +	wdd = wd_data->wdd;
> +	if (!wdd)
> +		goto done;
> +
> +	/*
> +	 * Reading returns if the pretimeout has gone off, and it only does
> +	 * it once per pretimeout.
> +	 */
> +	spin_lock_irq(&wdd->readlock);
> +	while (!wdd->data_to_read) {
> +		if (file->f_flags & O_NONBLOCK) {
> +			err = -EAGAIN;
> +			goto out;
> +		}
> +
> +		init_waitqueue_entry(&wait, current);
> +		add_wait_queue(&wdd->read_q, &wait);
> +		set_current_state(TASK_INTERRUPTIBLE);
> +		spin_unlock_irq(&wdd->readlock);
> +		schedule();
> +		spin_lock_irq(&wdd->readlock);
> +		remove_wait_queue(&wdd->read_q, &wait);
> +
> +		if (signal_pending(current)) {
> +			err = -ERESTARTSYS;
> +			goto out;
> +		}
> +	}
> +	dummy = wdd->data_to_read;
> +	wdd->data_to_read = 0;
> +
> + out:
> +	spin_unlock_irq(&wdd->readlock);
> +
> +	if (err == 0) {
> +		if (copy_to_user(buf, &dummy, 1))
> +			err = -EFAULT;
> +		else
> +			err = 1;
> +	}
> +
> + done:
> +	mutex_unlock(&wd_data->lock);
> +
> +	return err;
> +}
> +
> +static __poll_t watchdog_poll(struct file *file, poll_table *wait)
> +{
> +	struct watchdog_core_data *wd_data = file->private_data;
> +	struct watchdog_device *wdd;
> +	__poll_t mask = 0;
> +
> +	mutex_lock(&wd_data->lock);
> +
> +	wdd = wd_data->wdd;
> +	if (!wdd)
> +		goto done;
> +
> +	poll_wait(file, &wdd->read_q, wait);
> +
> +	spin_lock_irq(&wdd->readlock);
> +	if (wdd->data_to_read)
> +		mask |= (EPOLLIN | EPOLLRDNORM);
> +	spin_unlock_irq(&wdd->readlock);
> +
> +done:
> +	mutex_unlock(&wd_data->lock);
> +	return mask;
> +}
> +
> +static int watchdog_fasync(int fd, struct file *file, int on)
> +{
> +	struct watchdog_core_data *wd_data = file->private_data;
> +	struct watchdog_device *wdd;
> +	int err = -ENODEV;
> +
> +	mutex_lock(&wd_data->lock);
> +
> +	wdd = wd_data->wdd;
> +	if (!wdd)
> +		goto done;
> +
> +	err = fasync_helper(fd, file, on, &wdd->fasync_q);
> +done:
> +	mutex_unlock(&wd_data->lock);
> +	return err;
> +}
> +
>  static const struct file_operations watchdog_fops = {
>  	.owner		= THIS_MODULE,
>  	.write		= watchdog_write,
>  	.unlocked_ioctl	= watchdog_ioctl,
>  	.open		= watchdog_open,
>  	.release	= watchdog_release,
> +	.read		= watchdog_read,
> +	.poll		= watchdog_poll,
> +	.fasync		= watchdog_fasync,
>  };
>  
>  static struct miscdevice watchdog_miscdev = {
> @@ -970,6 +1080,9 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
>  	if (IS_ERR_OR_NULL(watchdog_kworker))
>  		return -ENODEV;
>  
> +	spin_lock_init(&wdd->readlock);
> +	init_waitqueue_head(&wdd->read_q);
> +
>  	kthread_init_work(&wd_data->work, watchdog_ping_work);
>  	hrtimer_init(&wd_data->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
>  	wd_data->timer.function = watchdog_timer_expired;
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index 417d9f37077a..e34501a822f0 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -117,6 +117,11 @@ 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;
> +
> +	spinlock_t readlock;
> +	bool data_to_read;
> +	struct wait_queue_head read_q;
> +	struct fasync_struct *fasync_q;
>  };
>  
>  #define WATCHDOG_NOWAYOUT		IS_BUILTIN(CONFIG_WATCHDOG_NOWAYOUT)
> -- 
> 2.17.1
> 

Re: [PATCH 02/12] watchdog: Add the ability to provide data to read

[Corey Minyard]

On Mon, Aug 19, 2019 at 03:43:45PM -0700, Guenter Roeck wrote:
> On Mon, Aug 19, 2019 at 03:37:01PM -0500, minyard@acm.org wrote:
> > From: Corey Minyard <cminyard@mvista.com>
> > 
> > This is for the read data pretimeout governor.
> > 
> > Signed-off-by: Corey Minyard <cminyard@mvista.com>
> 
> On further thought, I think it would be a bad idea to add this
> functionality: It changes the userspace ABI for accessing the watchdog
> device. Today, when a watchdog device is opened, it does not provide
> read data, it does not hang, and returns immediately. A "cat" from it
> is an easy and quick means to test if a watchdog works.

Umm, why would a "cat" from a watchdog tell you if a watchdog works?
If you want to know if the device exists, I would think that
"if [ -c /dev/watchdog ]..." would be a lot more clear and wouldn't
print a useless error and wouldn't start the watchdog.

That said, this is useful if a userland program wants to know if a
pretimeout occurred.  I'm not sure if anyone is using this with the
IPMI watchdog, but to preserve the IPMI interface function, this
will be needed.

It might be better to set it to immediately return an error if the
pretimeout governer is not one that reads data.  I didn't think of
that before and it will be better to do that.

-corey

> 
> Guenter
> 
> > ---
> >  drivers/watchdog/watchdog_core.c |   3 +
> >  drivers/watchdog/watchdog_dev.c  | 113 +++++++++++++++++++++++++++++++
> >  include/linux/watchdog.h         |   5 ++
> >  3 files changed, 121 insertions(+)
> > 
> > diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> > index 21e8085b848b..80149ac229fc 100644
> > --- a/drivers/watchdog/watchdog_core.c
> > +++ b/drivers/watchdog/watchdog_core.c
> > @@ -216,6 +216,9 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
> >  		return id;
> >  	wdd->id = id;
> >  
> > +	spin_lock_init(&wdd->readlock);
> > +	init_waitqueue_head(&wdd->read_q);
> > +
> >  	ret = watchdog_dev_register(wdd);
> >  	if (ret) {
> >  		ida_simple_remove(&watchdog_ida, id);
> > diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> > index dbd2ad4c9294..8e8304607a8c 100644
> > --- a/drivers/watchdog/watchdog_dev.c
> > +++ b/drivers/watchdog/watchdog_dev.c
> > @@ -44,6 +44,8 @@
> >  #include <linux/types.h>	/* For standard types (like size_t) */
> >  #include <linux/watchdog.h>	/* For watchdog specific items */
> >  #include <linux/uaccess.h>	/* For copy_to_user/put_user/... */
> > +#include <linux/poll.h>		/* For poll_table/... */
> > +#include <linux/sched/signal.h>	/* For signal_pending */
> >  
> >  #include <uapi/linux/sched/types.h>	/* For struct sched_param */
> >  
> > @@ -929,12 +931,120 @@ static int watchdog_release(struct inode *inode, struct file *file)
> >  	return 0;
> >  }
> >  
> > +static ssize_t watchdog_read(struct file *file,
> > +			     char        __user *buf,
> > +			     size_t      count,
> > +			     loff_t      *ppos)
> > +{
> > +	struct watchdog_core_data *wd_data = file->private_data;
> > +	struct watchdog_device *wdd;
> > +	int err = 0;
> > +	wait_queue_entry_t wait;
> > +	char dummy = 1;
> > +
> > +	if (count <= 0)
> > +		return 0;
> > +
> > +	mutex_lock(&wd_data->lock);
> > +
> > +	wdd = wd_data->wdd;
> > +	if (!wdd)
> > +		goto done;
> > +
> > +	/*
> > +	 * Reading returns if the pretimeout has gone off, and it only does
> > +	 * it once per pretimeout.
> > +	 */
> > +	spin_lock_irq(&wdd->readlock);
> > +	while (!wdd->data_to_read) {
> > +		if (file->f_flags & O_NONBLOCK) {
> > +			err = -EAGAIN;
> > +			goto out;
> > +		}
> > +
> > +		init_waitqueue_entry(&wait, current);
> > +		add_wait_queue(&wdd->read_q, &wait);
> > +		set_current_state(TASK_INTERRUPTIBLE);
> > +		spin_unlock_irq(&wdd->readlock);
> > +		schedule();
> > +		spin_lock_irq(&wdd->readlock);
> > +		remove_wait_queue(&wdd->read_q, &wait);
> > +
> > +		if (signal_pending(current)) {
> > +			err = -ERESTARTSYS;
> > +			goto out;
> > +		}
> > +	}
> > +	dummy = wdd->data_to_read;
> > +	wdd->data_to_read = 0;
> > +
> > + out:
> > +	spin_unlock_irq(&wdd->readlock);
> > +
> > +	if (err == 0) {
> > +		if (copy_to_user(buf, &dummy, 1))
> > +			err = -EFAULT;
> > +		else
> > +			err = 1;
> > +	}
> > +
> > + done:
> > +	mutex_unlock(&wd_data->lock);
> > +
> > +	return err;
> > +}
> > +
> > +static __poll_t watchdog_poll(struct file *file, poll_table *wait)
> > +{
> > +	struct watchdog_core_data *wd_data = file->private_data;
> > +	struct watchdog_device *wdd;
> > +	__poll_t mask = 0;
> > +
> > +	mutex_lock(&wd_data->lock);
> > +
> > +	wdd = wd_data->wdd;
> > +	if (!wdd)
> > +		goto done;
> > +
> > +	poll_wait(file, &wdd->read_q, wait);
> > +
> > +	spin_lock_irq(&wdd->readlock);
> > +	if (wdd->data_to_read)
> > +		mask |= (EPOLLIN | EPOLLRDNORM);
> > +	spin_unlock_irq(&wdd->readlock);
> > +
> > +done:
> > +	mutex_unlock(&wd_data->lock);
> > +	return mask;
> > +}
> > +
> > +static int watchdog_fasync(int fd, struct file *file, int on)
> > +{
> > +	struct watchdog_core_data *wd_data = file->private_data;
> > +	struct watchdog_device *wdd;
> > +	int err = -ENODEV;
> > +
> > +	mutex_lock(&wd_data->lock);
> > +
> > +	wdd = wd_data->wdd;
> > +	if (!wdd)
> > +		goto done;
> > +
> > +	err = fasync_helper(fd, file, on, &wdd->fasync_q);
> > +done:
> > +	mutex_unlock(&wd_data->lock);
> > +	return err;
> > +}
> > +
> >  static const struct file_operations watchdog_fops = {
> >  	.owner		= THIS_MODULE,
> >  	.write		= watchdog_write,
> >  	.unlocked_ioctl	= watchdog_ioctl,
> >  	.open		= watchdog_open,
> >  	.release	= watchdog_release,
> > +	.read		= watchdog_read,
> > +	.poll		= watchdog_poll,
> > +	.fasync		= watchdog_fasync,
> >  };
> >  
> >  static struct miscdevice watchdog_miscdev = {
> > @@ -970,6 +1080,9 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
> >  	if (IS_ERR_OR_NULL(watchdog_kworker))
> >  		return -ENODEV;
> >  
> > +	spin_lock_init(&wdd->readlock);
> > +	init_waitqueue_head(&wdd->read_q);
> > +
> >  	kthread_init_work(&wd_data->work, watchdog_ping_work);
> >  	hrtimer_init(&wd_data->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> >  	wd_data->timer.function = watchdog_timer_expired;
> > diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> > index 417d9f37077a..e34501a822f0 100644
> > --- a/include/linux/watchdog.h
> > +++ b/include/linux/watchdog.h
> > @@ -117,6 +117,11 @@ 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;
> > +
> > +	spinlock_t readlock;
> > +	bool data_to_read;
> > +	struct wait_queue_head read_q;
> > +	struct fasync_struct *fasync_q;
> >  };
> >  
> >  #define WATCHDOG_NOWAYOUT		IS_BUILTIN(CONFIG_WATCHDOG_NOWAYOUT)
> > -- 
> > 2.17.1
> > 

Re: [PATCH 02/12] watchdog: Add the ability to provide data to read

[Jerry Hoemann]

On Mon, Aug 19, 2019 at 07:23:09PM -0500, Corey Minyard wrote:
> On Mon, Aug 19, 2019 at 03:43:45PM -0700, Guenter Roeck wrote:
> > On Mon, Aug 19, 2019 at 03:37:01PM -0500, minyard@acm.org wrote:
> > > From: Corey Minyard <cminyard@mvista.com>
> > > 
> > > This is for the read data pretimeout governor.
> > > 
> > > Signed-off-by: Corey Minyard <cminyard@mvista.com>
> > 
> > On further thought, I think it would be a bad idea to add this
> > functionality: It changes the userspace ABI for accessing the watchdog
> > device. Today, when a watchdog device is opened, it does not provide
> > read data, it does not hang, and returns immediately. A "cat" from it
> > is an easy and quick means to test if a watchdog works.
> 
> Umm, why would a "cat" from a watchdog tell you if a watchdog works?

cat /dev/watchdog starts the watchdog running.

Then one can do useful things like monitor /sys/class/watchdog/watchdogN and see
time ticking down, etc..,

echo V > /dev/watchdog stops the watchdog assuming driver supports WDIOF_MAGICCLOSE.

So I can test without having to reboot.

One can't test magic close with the proposed change as /dev/watchdog
is exclusive open.




-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

Re: [PATCH 02/12] watchdog: Add the ability to provide data to read

[Corey Minyard]

On Mon, Aug 19, 2019 at 07:09:46PM -0600, Jerry Hoemann wrote:
> On Mon, Aug 19, 2019 at 07:23:09PM -0500, Corey Minyard wrote:
> > On Mon, Aug 19, 2019 at 03:43:45PM -0700, Guenter Roeck wrote:
> > > On Mon, Aug 19, 2019 at 03:37:01PM -0500, minyard@acm.org wrote:
> > > > From: Corey Minyard <cminyard@mvista.com>
> > > > 
> > > > This is for the read data pretimeout governor.
> > > > 
> > > > Signed-off-by: Corey Minyard <cminyard@mvista.com>
> > > 
> > > On further thought, I think it would be a bad idea to add this
> > > functionality: It changes the userspace ABI for accessing the watchdog
> > > device. Today, when a watchdog device is opened, it does not provide
> > > read data, it does not hang, and returns immediately. A "cat" from it
> > > is an easy and quick means to test if a watchdog works.
> > 
> > Umm, why would a "cat" from a watchdog tell you if a watchdog works?
> 
> cat /dev/watchdog starts the watchdog running.
> 
> Then one can do useful things like monitor /sys/class/watchdog/watchdogN and see
> time ticking down, etc..,
> 
> echo V > /dev/watchdog stops the watchdog assuming driver supports WDIOF_MAGICCLOSE.
> 
> So I can test without having to reboot.
> 
> One can't test magic close with the proposed change as /dev/watchdog
> is exclusive open.

Sure you can:

# echo "" >/dev/watchdog0
[   92.390649] watchdog: watchdog0: watchdog did not stop!
# sleep 2
# cat /sys/class/watchdog/watchdog0/timeleft
8
# echo "V" >/dev/watchdog0

Works just fine.  But I can make it so that reading returns an error
unless the governor is the read one.

The question is if this is required to transfer the IPMI watchdog
over to the standard interface.  It currently has this function,
do we do an API change to move it over?

-corey

> 
> 
> 
> 
> -- 
> 
> -----------------------------------------------------------------------------
> Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
> -----------------------------------------------------------------------------

Re: [PATCH 02/12] watchdog: Add the ability to provide data to read

[Guenter Roeck]

On 8/20/19 5:12 AM, Corey Minyard wrote:
> On Mon, Aug 19, 2019 at 07:09:46PM -0600, Jerry Hoemann wrote:
>> On Mon, Aug 19, 2019 at 07:23:09PM -0500, Corey Minyard wrote:
>>> On Mon, Aug 19, 2019 at 03:43:45PM -0700, Guenter Roeck wrote:
>>>> On Mon, Aug 19, 2019 at 03:37:01PM -0500, minyard@acm.org wrote:
>>>>> From: Corey Minyard <cminyard@mvista.com>
>>>>>
>>>>> This is for the read data pretimeout governor.
>>>>>
>>>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>>>
>>>> On further thought, I think it would be a bad idea to add this
>>>> functionality: It changes the userspace ABI for accessing the watchdog
>>>> device. Today, when a watchdog device is opened, it does not provide
>>>> read data, it does not hang, and returns immediately. A "cat" from it
>>>> is an easy and quick means to test if a watchdog works.
>>>
>>> Umm, why would a "cat" from a watchdog tell you if a watchdog works?
>>
>> cat /dev/watchdog starts the watchdog running.
>>
>> Then one can do useful things like monitor /sys/class/watchdog/watchdogN and see
>> time ticking down, etc..,
>>
>> echo V > /dev/watchdog stops the watchdog assuming driver supports WDIOF_MAGICCLOSE.
>>
>> So I can test without having to reboot.
>>
>> One can't test magic close with the proposed change as /dev/watchdog
>> is exclusive open.
> 
> Sure you can:
> 
> # echo "" >/dev/watchdog0
> [   92.390649] watchdog: watchdog0: watchdog did not stop!
> # sleep 2
> # cat /sys/class/watchdog/watchdog0/timeleft
> 8
> # echo "V" >/dev/watchdog0
> 
> Works just fine.  But I can make it so that reading returns an error
> unless the governor is the read one.
> 
> The question is if this is required to transfer the IPMI watchdog
> over to the standard interface.  It currently has this function,
> do we do an API change to move it over?
> 
Having to change the standard watchdog API to accommodate a non-standard driver
is most definitely not the right approach. If it was, anyone could use it to
force standard API/ABI changes. Just implement driver X outside its subsystem
and then claim you need to change the subsystem to accommodate it.

On a side note, a standard watchdog driver can implement its own ioctl functions.

Guenter

> -corey
> 
>>
>>
>>
>>
>> -- 
>>
>> -----------------------------------------------------------------------------
>> Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
>> -----------------------------------------------------------------------------
> 

Re: [PATCH 02/12] watchdog: Add the ability to provide data to read

[Corey Minyard]

On Tue, Aug 20, 2019 at 06:53:40AM -0700, Guenter Roeck wrote:
> On 8/20/19 5:12 AM, Corey Minyard wrote:
> > On Mon, Aug 19, 2019 at 07:09:46PM -0600, Jerry Hoemann wrote:
> > > On Mon, Aug 19, 2019 at 07:23:09PM -0500, Corey Minyard wrote:
> > > > On Mon, Aug 19, 2019 at 03:43:45PM -0700, Guenter Roeck wrote:
> > > > > On Mon, Aug 19, 2019 at 03:37:01PM -0500, minyard@acm.org wrote:
> > > > > > From: Corey Minyard <cminyard@mvista.com>
> > > > > > 
> > > > > > This is for the read data pretimeout governor.
> > > > > > 
> > > > > > Signed-off-by: Corey Minyard <cminyard@mvista.com>
> > > > > 
> > > > > On further thought, I think it would be a bad idea to add this
> > > > > functionality: It changes the userspace ABI for accessing the watchdog
> > > > > device. Today, when a watchdog device is opened, it does not provide
> > > > > read data, it does not hang, and returns immediately. A "cat" from it
> > > > > is an easy and quick means to test if a watchdog works.
> > > > 
> > > > Umm, why would a "cat" from a watchdog tell you if a watchdog works?
> > > 
> > > cat /dev/watchdog starts the watchdog running.
> > > 
> > > Then one can do useful things like monitor /sys/class/watchdog/watchdogN and see
> > > time ticking down, etc..,
> > > 
> > > echo V > /dev/watchdog stops the watchdog assuming driver supports WDIOF_MAGICCLOSE.
> > > 
> > > So I can test without having to reboot.
> > > 
> > > One can't test magic close with the proposed change as /dev/watchdog
> > > is exclusive open.
> > 
> > Sure you can:
> > 
> > # echo "" >/dev/watchdog0
> > [   92.390649] watchdog: watchdog0: watchdog did not stop!
> > # sleep 2
> > # cat /sys/class/watchdog/watchdog0/timeleft
> > 8
> > # echo "V" >/dev/watchdog0
> > 
> > Works just fine.  But I can make it so that reading returns an error
> > unless the governor is the read one.
> > 
> > The question is if this is required to transfer the IPMI watchdog
> > over to the standard interface.  It currently has this function,
> > do we do an API change to move it over?
> > 
> Having to change the standard watchdog API to accommodate a non-standard driver
> is most definitely not the right approach. If it was, anyone could use it to
> force standard API/ABI changes. Just implement driver X outside its subsystem
> and then claim you need to change the subsystem to accommodate it.

I'm not advocating anything of the sort.  I think it can be done in
a way that keeps the API the same unless you enable a new pretimeout
governor.  I would not suggest that the API be changed, and I should
have handled that in the original design.

> 
> On a side note, a standard watchdog driver can implement its own ioctl functions.

I am aware of that, but you can't provide read data on a file descriptor
through that interface.  The actions and preactions could be done that
way, but that seemed a more general function that could benefit other
drivers. 

The function to provide read data might be useful, I don't know, but
it could be used with any driver that did a normal interrupt pretimeout.
I can't remember why it was originally done.  I vaguely remember someone
asking for it, but that was 17 years ago.

It could just be left out and added back if someone complains.  That's
not very friendly since it's an API change, but then we would know if
anyone was using it.

-corey

> 
> Guenter
> 
> > -corey
> > 
> > > 
> > > 
> > > 
> > > 
> > > -- 
> > > 
> > > -----------------------------------------------------------------------------
> > > Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
> > > -----------------------------------------------------------------------------
> > 
> 

Re: [PATCH 02/12] watchdog: Add the ability to provide data to read

[Guenter Roeck]

On 8/20/19 8:58 AM, Corey Minyard wrote:
> On Tue, Aug 20, 2019 at 06:53:40AM -0700, Guenter Roeck wrote:
>> On 8/20/19 5:12 AM, Corey Minyard wrote:
>>> On Mon, Aug 19, 2019 at 07:09:46PM -0600, Jerry Hoemann wrote:
>>>> On Mon, Aug 19, 2019 at 07:23:09PM -0500, Corey Minyard wrote:
>>>>> On Mon, Aug 19, 2019 at 03:43:45PM -0700, Guenter Roeck wrote:
>>>>>> On Mon, Aug 19, 2019 at 03:37:01PM -0500, minyard@acm.org wrote:
>>>>>>> From: Corey Minyard <cminyard@mvista.com>
>>>>>>>
>>>>>>> This is for the read data pretimeout governor.
>>>>>>>
>>>>>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>>>>>
>>>>>> On further thought, I think it would be a bad idea to add this
>>>>>> functionality: It changes the userspace ABI for accessing the watchdog
>>>>>> device. Today, when a watchdog device is opened, it does not provide
>>>>>> read data, it does not hang, and returns immediately. A "cat" from it
>>>>>> is an easy and quick means to test if a watchdog works.
>>>>>
>>>>> Umm, why would a "cat" from a watchdog tell you if a watchdog works?
>>>>
>>>> cat /dev/watchdog starts the watchdog running.
>>>>
>>>> Then one can do useful things like monitor /sys/class/watchdog/watchdogN and see
>>>> time ticking down, etc..,
>>>>
>>>> echo V > /dev/watchdog stops the watchdog assuming driver supports WDIOF_MAGICCLOSE.
>>>>
>>>> So I can test without having to reboot.
>>>>
>>>> One can't test magic close with the proposed change as /dev/watchdog
>>>> is exclusive open.
>>>
>>> Sure you can:
>>>
>>> # echo "" >/dev/watchdog0
>>> [   92.390649] watchdog: watchdog0: watchdog did not stop!
>>> # sleep 2
>>> # cat /sys/class/watchdog/watchdog0/timeleft
>>> 8
>>> # echo "V" >/dev/watchdog0
>>>
>>> Works just fine.  But I can make it so that reading returns an error
>>> unless the governor is the read one.
>>>
>>> The question is if this is required to transfer the IPMI watchdog
>>> over to the standard interface.  It currently has this function,
>>> do we do an API change to move it over?
>>>
>> Having to change the standard watchdog API to accommodate a non-standard driver
>> is most definitely not the right approach. If it was, anyone could use it to
>> force standard API/ABI changes. Just implement driver X outside its subsystem
>> and then claim you need to change the subsystem to accommodate it.
> 
> I'm not advocating anything of the sort.  I think it can be done in
> a way that keeps the API the same unless you enable a new pretimeout
> governor.  I would not suggest that the API be changed, and I should
> have handled that in the original design.
> 
>>
>> On a side note, a standard watchdog driver can implement its own ioctl functions.
> 
> I am aware of that, but you can't provide read data on a file descriptor
> through that interface.  The actions and preactions could be done that
> way, but that seemed a more general function that could benefit other
> drivers.
> 
That comment was more directed towards the ioctls you are adding to the
watchdog core, which I think would require more discussion.

> The function to provide read data might be useful, I don't know, but
> it could be used with any driver that did a normal interrupt pretimeout.
> I can't remember why it was originally done.  I vaguely remember someone
> asking for it, but that was 17 years ago.
> 

I find it odd. Only one driver can have the watchdog device open,
and that open file should be used to ping the watchdog. It can't do that
while waiting for a pretimeout. This almost sounds like the user wrote
an application which waited for the pretimeout to happen before pinging
the watchdog.

Talking about a standardized ABI to inform userspace if a pretimeout
occurred... if there is a use case for this, I'd rather trigger a udev
event on the "pretimeout" sysfs attribute. That would make much more
sense to me than sending a random data byte to the "read" function.
The WDIOC_GETSTATUS ioctl could then report that a pretimeout occurred.
Or, depending on the use case, just report the fact that a pretimeout
occurred with WDIOC_GETSTATUS. That would be really simple to add,
and I would support it.

> It could just be left out and added back if someone complains.  That's
> not very friendly since it's an API change, but then we would know if
> anyone was using it.

It is still better than an API change for standard watchdog drivers,
and a somewhat awkward interface to inform userspace about pretimeout
events.

Thanks,
Guenter

> 
> -corey
> 
>>
>> Guenter
>>
>>> -corey
>>>
>>>>
>>>>
>>>>
>>>>
>>>> -- 
>>>>
>>>> -----------------------------------------------------------------------------
>>>> Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
>>>> -----------------------------------------------------------------------------
>>>
>>
> 

Re: [PATCH 02/12] watchdog: Add the ability to provide data to read

[Corey Minyard]

On Tue, Aug 20, 2019 at 10:14:50AM -0700, Guenter Roeck wrote:
> On 8/20/19 8:58 AM, Corey Minyard wrote:
> > On Tue, Aug 20, 2019 at 06:53:40AM -0700, Guenter Roeck wrote:
> > > On 8/20/19 5:12 AM, Corey Minyard wrote:
> > > > On Mon, Aug 19, 2019 at 07:09:46PM -0600, Jerry Hoemann wrote:
> > > > > On Mon, Aug 19, 2019 at 07:23:09PM -0500, Corey Minyard wrote:
> > > > > > On Mon, Aug 19, 2019 at 03:43:45PM -0700, Guenter Roeck wrote:
> > > > > > > On Mon, Aug 19, 2019 at 03:37:01PM -0500, minyard@acm.org wrote:
> > > > > > > > From: Corey Minyard <cminyard@mvista.com>
> > > > > > > > 
> > > > > > > > This is for the read data pretimeout governor.
> > > > > > > > 
> > > > > > > > Signed-off-by: Corey Minyard <cminyard@mvista.com>
> > > > > > > 
> > > > > > > On further thought, I think it would be a bad idea to add this
> > > > > > > functionality: It changes the userspace ABI for accessing the watchdog
> > > > > > > device. Today, when a watchdog device is opened, it does not provide
> > > > > > > read data, it does not hang, and returns immediately. A "cat" from it
> > > > > > > is an easy and quick means to test if a watchdog works.
> > > > > > 
> > > > > > Umm, why would a "cat" from a watchdog tell you if a watchdog works?
> > > > > 
> > > > > cat /dev/watchdog starts the watchdog running.
> > > > > 
> > > > > Then one can do useful things like monitor /sys/class/watchdog/watchdogN and see
> > > > > time ticking down, etc..,
> > > > > 
> > > > > echo V > /dev/watchdog stops the watchdog assuming driver supports WDIOF_MAGICCLOSE.
> > > > > 
> > > > > So I can test without having to reboot.
> > > > > 
> > > > > One can't test magic close with the proposed change as /dev/watchdog
> > > > > is exclusive open.
> > > > 
> > > > Sure you can:
> > > > 
> > > > # echo "" >/dev/watchdog0
> > > > [   92.390649] watchdog: watchdog0: watchdog did not stop!
> > > > # sleep 2
> > > > # cat /sys/class/watchdog/watchdog0/timeleft
> > > > 8
> > > > # echo "V" >/dev/watchdog0
> > > > 
> > > > Works just fine.  But I can make it so that reading returns an error
> > > > unless the governor is the read one.
> > > > 
> > > > The question is if this is required to transfer the IPMI watchdog
> > > > over to the standard interface.  It currently has this function,
> > > > do we do an API change to move it over?
> > > > 
> > > Having to change the standard watchdog API to accommodate a non-standard driver
> > > is most definitely not the right approach. If it was, anyone could use it to
> > > force standard API/ABI changes. Just implement driver X outside its subsystem
> > > and then claim you need to change the subsystem to accommodate it.
> > 
> > I'm not advocating anything of the sort.  I think it can be done in
> > a way that keeps the API the same unless you enable a new pretimeout
> > governor.  I would not suggest that the API be changed, and I should
> > have handled that in the original design.
> > 
> > > 
> > > On a side note, a standard watchdog driver can implement its own ioctl functions.
> > 
> > I am aware of that, but you can't provide read data on a file descriptor
> > through that interface.  The actions and preactions could be done that
> > way, but that seemed a more general function that could benefit other
> > drivers.
> > 
> That comment was more directed towards the ioctls you are adding to the
> watchdog core, which I think would require more discussion.

Ok, that's kind of a separate discussion.  We should probably have it
on that email.

> 
> > The function to provide read data might be useful, I don't know, but
> > it could be used with any driver that did a normal interrupt pretimeout.
> > I can't remember why it was originally done.  I vaguely remember someone
> > asking for it, but that was 17 years ago.
> > 
> 
> I find it odd. Only one driver can have the watchdog device open,
> and that open file should be used to ping the watchdog. It can't do that
> while waiting for a pretimeout. This almost sounds like the user wrote
> an application which waited for the pretimeout to happen before pinging
> the watchdog.

No, poll() is also implemented, so you can wait for poll input and
also wait for a timeout.  And fasync is also implemented.  And you
can have multiple threads in the same program, one for pinging and
one for reading.  Single open does not mean single thread through
the driver.

> 
> Talking about a standardized ABI to inform userspace if a pretimeout
> occurred... if there is a use case for this, I'd rather trigger a udev
> event on the "pretimeout" sysfs attribute. That would make much more
> sense to me than sending a random data byte to the "read" function.
> The WDIOC_GETSTATUS ioctl could then report that a pretimeout occurred.
> Or, depending on the use case, just report the fact that a pretimeout
> occurred with WDIOC_GETSTATUS. That would be really simple to add,
> and I would support it.

I'd agree on the udev event, but none of that existed when this was
originally written.  I'd prefer to not implement interfaces that
require periodic polling.

To me the only sensible thing to do on a pretimeout is to panic.
But people come up with all kinds of wild things, and I almost
certainly had some sort of external impetus to add this.

> 
> > It could just be left out and added back if someone complains.  That's
> > not very friendly since it's an API change, but then we would know if
> > anyone was using it.
> 
> It is still better than an API change for standard watchdog drivers,
> and a somewhat awkward interface to inform userspace about pretimeout
> events.

Like I said before, I think we can avoid an API change.  It's not an
API change if you add a new function that has to be enabled separately
that then makes it work differently.  Othersize you could never add
any new function.  And one can argue whether it's an API change, really,
as API changes only affect non-erroneous uses of an API.  Whether
reading from a write-only device is erroneous is questionable in my
mind ;-).

But I'm fine with leaving it out, then we can discuss how to add
something back in if someone complains.  That's probably the best
way forward.  I separated it out so those changes can be dropped
without affecting anything else.  And if someone complains, we would
get a real user who would know why they are using it.

Thanks,

-corey

> 
> Thanks,
> Guenter
> 
> > 
> > -corey
> > 
> > > 
> > > Guenter
> > > 
> > > > -corey
> > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > -- 
> > > > > 
> > > > > -----------------------------------------------------------------------------
> > > > > Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
> > > > > -----------------------------------------------------------------------------
> > > > 
> > > 
> > 
> 

[PATCH 03/12] watchdog: Add a pretimeout governor to provide read data

[minyard]

From: Corey Minyard <cminyard@mvista.com>

When a pretimeout occurs, provide a byte of data on the watchdog
device.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 drivers/watchdog/Kconfig                | 16 +++++++++
 drivers/watchdog/Makefile               |  1 +
 drivers/watchdog/pretimeout_read_data.c | 47 +++++++++++++++++++++++++
 drivers/watchdog/watchdog_pretimeout.h  |  2 ++
 4 files changed, 66 insertions(+)
 create mode 100644 drivers/watchdog/pretimeout_read_data.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 8188963a405b..3578b7bc863c 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -105,6 +105,14 @@ config WATCHDOG_PRETIMEOUT_GOV_PANIC
 	  Panic watchdog pretimeout governor, on watchdog pretimeout
 	  event put the kernel into panic.
 
+config WATCHDOG_PRETIMEOUT_GOV_READ_DATA
+	tristate "Read data watchdog pretimeout governor"
+	depends on WATCHDOG_CORE
+	default WATCHDOG_CORE
+	help
+	  Read data watchdog pretimeout governor, on watchdog pretimeout
+	  event provide a byte of data on the watchdog device.
+
 choice
 	prompt "Default Watchdog Pretimeout Governor"
 	default WATCHDOG_PRETIMEOUT_DEFAULT_GOV_PANIC
@@ -129,6 +137,14 @@ config WATCHDOG_PRETIMEOUT_DEFAULT_GOV_PANIC
 	  a watchdog pretimeout event happens, consider that
 	  a watchdog feeder is dead and reboot is unavoidable.
 
+config WATCHDOG_PRETIMEOUT_DEFAULT_GOV_READ_DATA
+	bool "read_data"
+	depends on WATCHDOG_PRETIMEOUT_GOV_READ_DATA
+	help
+	  Use read data watchdog pretimeout governor by default, if
+	  a watchdog pretimeout event happens, provide a byte of read
+	  data on the watchdog device.
+
 endchoice
 
 endif # WATCHDOG_PRETIMEOUT_GOV
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 7caa920e7e60..9cfe4ad32dc4 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -12,6 +12,7 @@ watchdog-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV)	+= watchdog_pretimeout.o
 
 obj-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV_NOOP)	+= pretimeout_noop.o
 obj-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV_PANIC)	+= pretimeout_panic.o
+obj-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV_READ_DATA)	+= pretimeout_read_data.o
 
 # Only one watchdog can succeed. We probe the ISA/PCI/USB based
 # watchdog-cards first, then the architecture specific watchdog
diff --git a/drivers/watchdog/pretimeout_read_data.c b/drivers/watchdog/pretimeout_read_data.c
new file mode 100644
index 000000000000..197e9d692044
--- /dev/null
+++ b/drivers/watchdog/pretimeout_read_data.c
@@ -0,0 +1,47 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2019 MontaVista Software, LLC
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/fs.h>
+#include <linux/watchdog.h>
+
+#include "watchdog_pretimeout.h"
+
+/**
+ * pretimeout_read_data - Cause a read to return on the watchdog device.
+ * @wdd - watchdog_device
+ */
+static void pretimeout_read_data(struct watchdog_device *wdd)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&wdd->readlock, flags);
+	wdd->data_to_read = 1;
+	wake_up_interruptible(&wdd->read_q);
+	kill_fasync(&wdd->fasync_q, SIGIO, POLL_IN);
+	spin_unlock_irqrestore(&wdd->readlock, flags);
+}
+
+static struct watchdog_governor watchdog_gov_read_data = {
+	.name		= "read_data",
+	.pretimeout	= pretimeout_read_data,
+};
+
+static int __init watchdog_gov_read_data_register(void)
+{
+	return watchdog_register_governor(&watchdog_gov_read_data);
+}
+
+static void __exit watchdog_gov_read_data_unregister(void)
+{
+	watchdog_unregister_governor(&watchdog_gov_read_data);
+}
+module_init(watchdog_gov_read_data_register);
+module_exit(watchdog_gov_read_data_unregister);
+
+MODULE_AUTHOR("Corey Minyard <cminyard@mvista.com>");
+MODULE_DESCRIPTION("Read data watchdog pretimeout governor");
+MODULE_LICENSE("GPL");
diff --git a/drivers/watchdog/watchdog_pretimeout.h b/drivers/watchdog/watchdog_pretimeout.h
index a3f1abc68839..819517ed0138 100644
--- a/drivers/watchdog/watchdog_pretimeout.h
+++ b/drivers/watchdog/watchdog_pretimeout.h
@@ -28,6 +28,8 @@ int watchdog_pretimeout_governor_set(struct watchdog_device *wdd,
 #define WATCHDOG_PRETIMEOUT_DEFAULT_GOV		"noop"
 #elif IS_ENABLED(CONFIG_WATCHDOG_PRETIMEOUT_DEFAULT_GOV_PANIC)
 #define WATCHDOG_PRETIMEOUT_DEFAULT_GOV		"panic"
+#elif IS_ENABLED(CONFIG_WATCHDOG_PRETIMEOUT_DEFAULT_GOV_READ_DATA)
+#define WATCHDOG_PRETIMEOUT_DEFAULT_GOV		"read_data"
 #endif
 
 #else
-- 
2.17.1

[PATCH 04/12] watchdog: Allow pretimeout governor setting to be accessed from modules

[minyard]

From: Corey Minyard <cminyard@mvista.com>

This is so watchdog driver (like IPMI) can set it.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 drivers/watchdog/watchdog_pretimeout.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/watchdog/watchdog_pretimeout.c b/drivers/watchdog/watchdog_pretimeout.c
index b45041b0ef39..270baf7b3fa0 100644
--- a/drivers/watchdog/watchdog_pretimeout.c
+++ b/drivers/watchdog/watchdog_pretimeout.c
@@ -95,6 +95,7 @@ int watchdog_pretimeout_governor_set(struct watchdog_device *wdd,
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(watchdog_pretimeout_governor_set);
 
 void watchdog_notify_pretimeout(struct watchdog_device *wdd)
 {
-- 
2.17.1

Re: [PATCH 04/12] watchdog: Allow pretimeout governor setting to be accessed from modules

[Guenter Roeck]

On Mon, Aug 19, 2019 at 03:37:03PM -0500, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> This is so watchdog driver (like IPMI) can set it.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  drivers/watchdog/watchdog_pretimeout.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/watchdog/watchdog_pretimeout.c b/drivers/watchdog/watchdog_pretimeout.c
> index b45041b0ef39..270baf7b3fa0 100644
> --- a/drivers/watchdog/watchdog_pretimeout.c
> +++ b/drivers/watchdog/watchdog_pretimeout.c
> @@ -95,6 +95,7 @@ int watchdog_pretimeout_governor_set(struct watchdog_device *wdd,
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(watchdog_pretimeout_governor_set);
>  

I don't think that is a good idea. The whole point of pretimeout governor
selection was to be able to configure it from userspace.

>  void watchdog_notify_pretimeout(struct watchdog_device *wdd)
>  {
> -- 
> 2.17.1
> 

Re: [PATCH 04/12] watchdog: Allow pretimeout governor setting to be accessed from modules

[Corey Minyard]

On Mon, Aug 19, 2019 at 02:49:53PM -0700, Guenter Roeck wrote:
> On Mon, Aug 19, 2019 at 03:37:03PM -0500, minyard@acm.org wrote:
> > From: Corey Minyard <cminyard@mvista.com>
> > 
> > This is so watchdog driver (like IPMI) can set it.
> > 
> > Signed-off-by: Corey Minyard <cminyard@mvista.com>
> > ---
> >  drivers/watchdog/watchdog_pretimeout.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/watchdog/watchdog_pretimeout.c b/drivers/watchdog/watchdog_pretimeout.c
> > index b45041b0ef39..270baf7b3fa0 100644
> > --- a/drivers/watchdog/watchdog_pretimeout.c
> > +++ b/drivers/watchdog/watchdog_pretimeout.c
> > @@ -95,6 +95,7 @@ int watchdog_pretimeout_governor_set(struct watchdog_device *wdd,
> >  
> >  	return 0;
> >  }
> > +EXPORT_SYMBOL_GPL(watchdog_pretimeout_governor_set);
> >  
> 
> I don't think that is a good idea. The whole point of pretimeout governor
> selection was to be able to configure it from userspace.

Yeah, this is really just a temporary thing.  There is a module parameter in
the IPMI watchdog that does basically the same thing as the device parameter
for setting the watchdog governor.  This is so that code can perform its
function.

I would expect this to go away eventually.

-corey

> 
> >  void watchdog_notify_pretimeout(struct watchdog_device *wdd)
> >  {
> > -- 
> > 2.17.1
> > 

[PATCH 05/12] watchdog:ipmi: Move the IPMI watchdog to drivers/watchdog

[minyard]

From: Corey Minyard <cminyard@mvista.com>

It probably belongs there, and it will need access to the
watchdog_pretimeout.h file when converted over to the standard
watchdog interface.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 MAINTAINERS                                     | 1 +
 drivers/char/ipmi/Kconfig                       | 5 -----
 drivers/char/ipmi/Makefile                      | 1 -
 drivers/watchdog/Kconfig                        | 6 ++++++
 drivers/watchdog/Makefile                       | 1 +
 drivers/{char/ipmi => watchdog}/ipmi_watchdog.c | 0
 6 files changed, 8 insertions(+), 6 deletions(-)
 rename drivers/{char/ipmi => watchdog}/ipmi_watchdog.c (100%)

diff --git a/MAINTAINERS b/MAINTAINERS
index 6426db5198f0..760bcf92fde1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8456,6 +8456,7 @@ F:	Documentation/IPMI.txt
 F:	drivers/char/ipmi/
 F:	include/linux/ipmi*
 F:	include/uapi/linux/ipmi*
+F:	drivers/watchdog/ipmi_watchdog.c
 
 IPS SCSI RAID DRIVER
 M:	Adaptec OEM Raid Solutions <aacraid@microsemi.com>
diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
index 4bad0614109b..5f310ff0bd89 100644
--- a/drivers/char/ipmi/Kconfig
+++ b/drivers/char/ipmi/Kconfig
@@ -81,11 +81,6 @@ config IPMI_POWERNV
        help
          Provides a driver for OPAL firmware-based IPMI interfaces.
 
-config IPMI_WATCHDOG
-       tristate 'IPMI Watchdog Timer'
-       help
-         This enables the IPMI watchdog timer.
-
 config IPMI_POWEROFF
        tristate 'IPMI Poweroff'
        help
diff --git a/drivers/char/ipmi/Makefile b/drivers/char/ipmi/Makefile
index 0822adc2ec41..a9edcd473615 100644
--- a/drivers/char/ipmi/Makefile
+++ b/drivers/char/ipmi/Makefile
@@ -20,7 +20,6 @@ obj-$(CONFIG_IPMI_DMI_DECODE) += ipmi_dmi.o
 obj-$(CONFIG_IPMI_PLAT_DATA) += ipmi_plat_data.o
 obj-$(CONFIG_IPMI_SSIF) += ipmi_ssif.o
 obj-$(CONFIG_IPMI_POWERNV) += ipmi_powernv.o
-obj-$(CONFIG_IPMI_WATCHDOG) += ipmi_watchdog.o
 obj-$(CONFIG_IPMI_POWEROFF) += ipmi_poweroff.o
 obj-$(CONFIG_IPMI_KCS_BMC) += kcs_bmc.o
 obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += bt-bmc.o
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 3578b7bc863c..de462f995329 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -960,6 +960,12 @@ config STPMIC1_WATCHDOG
 	  To compile this driver as a module, choose M here: the
 	  module will be called spmic1_wdt.
 
+config IPMI_WATCHDOG
+       tristate 'IPMI Watchdog Timer'
+       depends on IPMI_HANDLER
+       help
+         This enables the IPMI watchdog timer.
+
 config UNIPHIER_WATCHDOG
 	tristate "UniPhier watchdog support"
 	depends on ARCH_UNIPHIER || COMPILE_TEST
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 9cfe4ad32dc4..5840773bf2b4 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -225,3 +225,4 @@ obj-$(CONFIG_MENF21BMC_WATCHDOG) += menf21bmc_wdt.o
 obj-$(CONFIG_MENZ069_WATCHDOG) += menz69_wdt.o
 obj-$(CONFIG_RAVE_SP_WATCHDOG) += rave-sp-wdt.o
 obj-$(CONFIG_STPMIC1_WATCHDOG) += stpmic1_wdt.o
+obj-$(CONFIG_IPMI_WATCHDOG) += ipmi_watchdog.o
diff --git a/drivers/char/ipmi/ipmi_watchdog.c b/drivers/watchdog/ipmi_watchdog.c
similarity index 100%
rename from drivers/char/ipmi/ipmi_watchdog.c
rename to drivers/watchdog/ipmi_watchdog.c
-- 
2.17.1

[PATCH 06/12] watchdog:ipmi: Convert over to the standard watchdog infrastructure

[minyard]

From: Corey Minyard <cminyard@mvista.com>

The IPMI watchdog was using its own ioctl and misc registration.
Switch over to using the standard watchdog code.  This required
almost a complete rewrite, but this allowed a lot of improvements.
No functional changes.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 drivers/watchdog/ipmi_watchdog.c | 1350 ++++++++++++++----------------
 1 file changed, 621 insertions(+), 729 deletions(-)

diff --git a/drivers/watchdog/ipmi_watchdog.c b/drivers/watchdog/ipmi_watchdog.c
index 74c6d1f34132..c8c11a61ebb9 100644
--- a/drivers/watchdog/ipmi_watchdog.c
+++ b/drivers/watchdog/ipmi_watchdog.c
@@ -36,6 +36,9 @@
 #include <linux/delay.h>
 #include <linux/atomic.h>
 #include <linux/sched/signal.h>
+#include <linux/rcupdate.h>
+
+#include "watchdog_pretimeout.h"
 
 #ifdef CONFIG_X86
 /*
@@ -122,20 +125,69 @@
 
 #define IPMI_WDOG_TIMER_NOT_INIT_RESP	0x80
 
-static DEFINE_MUTEX(ipmi_watchdog_mutex);
-static bool nowayout = WATCHDOG_NOWAYOUT;
+#define IPMI_MAX_TIMEOUT	6553	/* 16 bit value in 1/10 of a second */
+#define IPMI_MIN_TIMEOUT	0
+
+struct ipmi_wdt {
+	struct watchdog_device wdd;
+	struct watchdog_info info;
+	struct ipmi_user *user;
+	int ifnum;		/* IPMI interface number. */
+	u8 ipmi_version_major;
+	u8 ipmi_version_minor;
+
+	struct mutex lock;
+
+	struct completion msg_wait;
+	atomic_t msg_tofree;
+	struct ipmi_smi_msg smi_msg;
+	struct ipmi_recv_msg recv_msg;
+
+	int state; /* One of WDOG_TIMEOUT_xxx, NONE if disabled. */
+
+	bool nmi_works;
+
+	atomic_t pretimeout_since_last_heartbeat;
+	bool panic_event_handled;
+};
+
+#define wdd_to_ipmi_wdt(wdd) container_of(wdd, struct ipmi_wdt, wdd)
+
+/* Parameters to ipmi_set_timeout(), _ipmi_update_timeout() */
+#define IPMI_SET_TIMEOUT_NO_HB			0
+#define IPMI_SET_TIMEOUT_HB_IF_NECESSARY	1
+#define IPMI_SET_TIMEOUT_FORCE_HB		2
+
+static int _ipmi_update_timeout(struct ipmi_wdt *iwd, int do_heartbeat,
+				bool do_poll);
+static int ipmi_set_timeout(struct ipmi_wdt *iwd, int timeout,
+			    int do_heartbeat);
+static int ipmi_set_pretimeout(struct ipmi_wdt *iwd, int timeout,
+			       int do_heartbeat);
+static void ipmi_register_watchdog(int ipmi_intf, struct device *dev);
+static void _ipmi_unregister_watchdog(struct ipmi_wdt *iwd);
+static void ipmi_unregister_watchdog(int ipmi_intf);
+
+/*
+ * Only used if HAVE_DIE_NMI is set, but pretimeout will not be delivered
+ * if this is set.
+ * 0 = not testing
+ * 1 = test running, no NMI
+ * 2 = test running, got an NMI
+ */
+static int testing_nmi;
 
-static struct ipmi_user *watchdog_user;
-static int watchdog_ifnum;
 
-/* Default the timeout to 10 seconds. */
-static int timeout = 10;
+/* Protects the following values. */
+static DEFINE_MUTEX(ipmi_wdt_data_mutex);
+static int ifnum_to_use = -1;
+static struct ipmi_wdt *ipmi_wdt;
 
-/* The pre-timeout is disabled by default. */
-static int pretimeout;
+static bool nowayout = WATCHDOG_NOWAYOUT;
 
-/* Default timeout to set on panic */
-static int panic_wdt_timeout = 255;
+static int timeout = 10; /* Default timeout. */
+static int pretimeout; /* Default pretimeout. */
+static int panic_wdt_timeout = 255; /* Default timeout to set on panic */
 
 /* Default action is to reset the board on a timeout. */
 static unsigned char action_val = WDOG_TIMEOUT_RESET;
@@ -149,23 +201,6 @@ static char preaction[16] = "pre_none";
 static unsigned char preop_val = WDOG_PREOP_NONE;
 
 static char preop[16] = "preop_none";
-static DEFINE_SPINLOCK(ipmi_read_lock);
-static char data_to_read;
-static DECLARE_WAIT_QUEUE_HEAD(read_q);
-static struct fasync_struct *fasync_q;
-static atomic_t pretimeout_since_last_heartbeat;
-static char expect_close;
-
-static int ifnum_to_use = -1;
-
-/* Parameters to ipmi_set_timeout */
-#define IPMI_SET_TIMEOUT_NO_HB			0
-#define IPMI_SET_TIMEOUT_HB_IF_NECESSARY	1
-#define IPMI_SET_TIMEOUT_FORCE_HB		2
-
-static int ipmi_set_timeout(int do_heartbeat);
-static void ipmi_register_watchdog(int ipmi_intf);
-static void ipmi_unregister_watchdog(int ipmi_intf);
 
 /*
  * If true, the driver will start running as soon as it is configured
@@ -173,21 +208,162 @@ static void ipmi_unregister_watchdog(int ipmi_intf);
  */
 static int start_now;
 
+static int action_op(const char *inval, char *outval)
+{
+	int rv = 0;
+
+	mutex_lock(&ipmi_wdt_data_mutex);
+	if (outval)
+		strcpy(outval, action);
+
+	if (!inval)
+		goto out_unlock;
+
+	if (strcmp(inval, "reset") == 0)
+		action_val = WDOG_TIMEOUT_RESET;
+	else if (strcmp(inval, "none") == 0)
+		action_val = WDOG_TIMEOUT_NONE;
+	else if (strcmp(inval, "power_cycle") == 0)
+		action_val = WDOG_TIMEOUT_POWER_CYCLE;
+	else if (strcmp(inval, "power_off") == 0)
+		action_val = WDOG_TIMEOUT_POWER_DOWN;
+	else
+		rv = -EINVAL;
+	if (!rv) {
+		strcpy(action, inval);
+		if (ipmi_wdt && ipmi_wdt->state != WDOG_TIMEOUT_NONE) {
+			ipmi_wdt->state = action_val;
+			rv = _ipmi_update_timeout(ipmi_wdt,
+					IPMI_SET_TIMEOUT_HB_IF_NECESSARY,
+					false);
+		}
+	}
+out_unlock:
+	mutex_unlock(&ipmi_wdt_data_mutex);
+
+	return rv;
+}
+
+static int preaction_op(const char *inval, char *outval)
+{
+	int rv = 0;
+
+	mutex_lock(&ipmi_wdt_data_mutex);
+	if (outval)
+		strcpy(outval, preaction);
+
+	if (!inval)
+		goto out_unlock;
+
+	if (strcmp(inval, "pre_none") == 0)
+		preaction_val = WDOG_PRETIMEOUT_NONE;
+	else if (strcmp(inval, "pre_smi") == 0)
+		preaction_val = WDOG_PRETIMEOUT_SMI;
+#ifdef HAVE_DIE_NMI
+	else if (strcmp(inval, "pre_nmi") == 0) {
+		if (preop_val == WDOG_PREOP_GIVE_DATA)
+			rv = -EINVAL;
+		else if (ipmi_wdt && !ipmi_wdt->nmi_works)
+			rv = -EINVAL;
+		else
+			preaction_val = WDOG_PRETIMEOUT_NMI;
+	}
+#endif
+	else if (strcmp(inval, "pre_int") == 0)
+		preaction_val = WDOG_PRETIMEOUT_MSG_INT;
+	else
+		rv = -EINVAL;
+	if (!rv) {
+		strcpy(preaction, inval);
+		if (ipmi_wdt && ipmi_wdt->state != WDOG_TIMEOUT_NONE &&
+		    ipmi_wdt->wdd.pretimeout) {
+			rv = _ipmi_update_timeout(ipmi_wdt,
+					IPMI_SET_TIMEOUT_HB_IF_NECESSARY,
+					false);
+		}
+	}
+out_unlock:
+	mutex_unlock(&ipmi_wdt_data_mutex);
+
+	return rv;
+}
+
+static int preop_op(const char *inval, char *outval)
+{
+	int rv = 0;
+	const char *gov = NULL;
+	unsigned int orig_val;
+
+	mutex_lock(&ipmi_wdt_data_mutex);
+	if (outval)
+		strcpy(outval, preop);
+
+	if (!inval)
+		goto out_unlock;
+
+	orig_val = preop_val;
+	if (strcmp(inval, "preop_none") == 0) {
+		preop_val = WDOG_PREOP_NONE;
+		gov = "noop";
+	} else if (strcmp(inval, "preop_panic") == 0) {
+		preop_val = WDOG_PREOP_PANIC;
+		gov = "panic";
+	} else if (strcmp(inval, "preop_give_data") == 0) {
+		if (preaction_val == WDOG_PRETIMEOUT_NMI)
+			rv = -EINVAL;
+		else {
+			preop_val = WDOG_PREOP_GIVE_DATA;
+			gov = "read_data";
+		}
+	} else {
+		rv = -EINVAL;
+	}
+	if (!rv) {
+		rv = watchdog_pretimeout_governor_set(&ipmi_wdt->wdd, gov);
+		if (rv)
+			preaction_val = orig_val;
+		else
+			strcpy(preop, inval);
+	}
+out_unlock:
+	mutex_unlock(&ipmi_wdt_data_mutex);
+
+	return rv;
+}
+
+static struct ipmi_smi_watcher smi_watcher = {
+	.owner    = THIS_MODULE,
+	.new_smi  = ipmi_register_watchdog,
+	.smi_gone = ipmi_unregister_watchdog
+};
+
 static int set_param_timeout(const char *val, const struct kernel_param *kp)
 {
-	char *endp;
-	int  l;
+	unsigned long l;
 	int  rv = 0;
 
 	if (!val)
 		return -EINVAL;
-	l = simple_strtoul(val, &endp, 0);
-	if (endp == val)
+	rv = kstrtoul(val, 0, &l);
+	if (rv)
+		return rv;
+
+	if (l < IPMI_MIN_TIMEOUT || l > IPMI_MAX_TIMEOUT)
 		return -EINVAL;
 
+	mutex_lock(&ipmi_wdt_data_mutex);
 	*((int *)kp->arg) = l;
-	if (watchdog_user)
-		rv = ipmi_set_timeout(IPMI_SET_TIMEOUT_HB_IF_NECESSARY);
+	if (!ipmi_wdt)
+		goto out_unlock;
+	if (kp->arg == &timeout)
+		rv = ipmi_set_timeout(ipmi_wdt, l,
+				      IPMI_SET_TIMEOUT_HB_IF_NECESSARY);
+	else if (kp->arg == &pretimeout)
+		rv = ipmi_set_pretimeout(ipmi_wdt, l,
+					 IPMI_SET_TIMEOUT_HB_IF_NECESSARY);
+
+out_unlock:
+	mutex_unlock(&ipmi_wdt_data_mutex);
 
 	return rv;
 }
@@ -200,15 +376,9 @@ static const struct kernel_param_ops param_ops_timeout = {
 
 typedef int (*action_fn)(const char *intval, char *outval);
 
-static int action_op(const char *inval, char *outval);
-static int preaction_op(const char *inval, char *outval);
-static int preop_op(const char *inval, char *outval);
-static void check_parms(void);
-
 static int set_param_str(const char *val, const struct kernel_param *kp)
 {
 	action_fn  fn = (action_fn) kp->arg;
-	int        rv = 0;
 	char       valcp[16];
 	char       *s;
 
@@ -217,16 +387,7 @@ static int set_param_str(const char *val, const struct kernel_param *kp)
 
 	s = strstrip(valcp);
 
-	rv = fn(s, NULL);
-	if (rv)
-		goto out;
-
-	check_parms();
-	if (watchdog_user)
-		rv = ipmi_set_timeout(IPMI_SET_TIMEOUT_HB_IF_NECESSARY);
-
- out:
-	return rv;
+	return fn(s, NULL);
 }
 
 static int get_param_str(char *buffer, const struct kernel_param *kp)
@@ -240,17 +401,45 @@ static int get_param_str(char *buffer, const struct kernel_param *kp)
 	return strlen(buffer);
 }
 
-
 static int set_param_wdog_ifnum(const char *val, const struct kernel_param *kp)
 {
-	int rv = param_set_int(val, kp);
+	int rv;
+	unsigned long l;
+	struct ipmi_wdt *old_iwd = NULL;
+
+	if (!val)
+		return 0;
+
+	rv = kstrtoul(val, 0, &l);
 	if (rv)
 		return rv;
-	if ((ifnum_to_use < 0) || (ifnum_to_use == watchdog_ifnum))
+
+	mutex_lock(&ipmi_wdt_data_mutex);
+	if (l == ifnum_to_use) {
+		mutex_unlock(&ipmi_wdt_data_mutex);
 		return 0;
+	}
+
+	if (ipmi_wdt) {
+		old_iwd = ipmi_wdt;
+		ipmi_wdt = NULL;
+	}
+	mutex_unlock(&ipmi_wdt_data_mutex);
+
+	if (old_iwd)
+		_ipmi_unregister_watchdog(old_iwd);
+
+	/*
+	 * Re-register the smi watcher to run through all the IPMI
+	 * interfaces again.
+	 */
+	ipmi_smi_watcher_unregister(&smi_watcher);
+	rv = ipmi_smi_watcher_register(&smi_watcher);
+	if (rv) {
+		pr_warn("can't register smi watcher\n");
+		return rv;
+	}
 
-	ipmi_unregister_watchdog(watchdog_ifnum);
-	ipmi_register_watchdog(ifnum_to_use);
 	return 0;
 }
 
@@ -300,61 +489,39 @@ module_param(nowayout, bool, 0644);
 MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
 		 "(default=CONFIG_WATCHDOG_NOWAYOUT)");
 
-/* Default state of the timer. */
-static unsigned char ipmi_watchdog_state = WDOG_TIMEOUT_NONE;
-
-/* Is someone using the watchdog?  Only one user is allowed. */
-static unsigned long ipmi_wdog_open;
-
-/*
- * If set to 1, the heartbeat command will set the state to reset and
- * start the timer.  The timer doesn't normally run when the driver is
- * first opened until the heartbeat is set the first time, this
- * variable is used to accomplish this.
- */
-static int ipmi_start_timer_on_heartbeat;
-
-/* IPMI version of the BMC. */
-static unsigned char ipmi_version_major;
-static unsigned char ipmi_version_minor;
-
-/* If a pretimeout occurs, this is used to allow only one panic to happen. */
-static atomic_t preop_panic_excl = ATOMIC_INIT(-1);
-
-#ifdef HAVE_DIE_NMI
-static int testing_nmi;
-static int nmi_handler_registered;
-#endif
-
-static int __ipmi_heartbeat(void);
+static int __ipmi_heartbeat(struct ipmi_wdt *iwd, bool do_poll);
 
 /*
  * We use a mutex to make sure that only one thing can send a set a
  * message at one time.  The mutex is claimed when a message is sent
  * and freed when both the send and receive messages are free.
  */
-static atomic_t msg_tofree = ATOMIC_INIT(0);
-static DECLARE_COMPLETION(msg_wait);
 static void msg_free_smi(struct ipmi_smi_msg *msg)
 {
-	if (atomic_dec_and_test(&msg_tofree))
-		complete(&msg_wait);
+	struct ipmi_wdt *iwd = container_of(msg, struct ipmi_wdt, smi_msg);
+
+	if (atomic_dec_and_test(&iwd->msg_tofree))
+		complete(&iwd->msg_wait);
 }
 static void msg_free_recv(struct ipmi_recv_msg *msg)
 {
-	if (atomic_dec_and_test(&msg_tofree))
-		complete(&msg_wait);
+	struct ipmi_wdt *iwd = container_of(msg, struct ipmi_wdt, recv_msg);
+
+	if (atomic_dec_and_test(&iwd->msg_tofree))
+		complete(&iwd->msg_wait);
 }
-static struct ipmi_smi_msg smi_msg = {
-	.done = msg_free_smi
-};
-static struct ipmi_recv_msg recv_msg = {
-	.done = msg_free_recv
-};
 
-static int __ipmi_set_timeout(struct ipmi_smi_msg  *smi_msg,
-			      struct ipmi_recv_msg *recv_msg,
-			      int                  *send_heartbeat_now)
+static void wait_msg_done(struct ipmi_wdt *iwd, bool do_poll)
+{
+	if (do_poll) {
+		while (atomic_read(&iwd->msg_tofree) != 0)
+			ipmi_poll_interface(iwd->user);
+	} else {
+		wait_for_completion(&iwd->msg_wait);
+	}
+}
+
+static int __ipmi_update_timeout(struct ipmi_wdt *iwd, int *send_heartbeat_now)
 {
 	struct kernel_ipmi_msg            msg;
 	unsigned char                     data[6];
@@ -362,15 +529,17 @@ static int __ipmi_set_timeout(struct ipmi_smi_msg  *smi_msg,
 	struct ipmi_system_interface_addr addr;
 	int                               hbnow = 0;
 
-
 	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) {
+	/* If timer is running, keep it running. */
+	if (iwd->state != WDOG_TIMEOUT_NONE) {
+		if ((iwd->ipmi_version_major > 1)
+		    || ((iwd->ipmi_version_major == 1) &&
+			(iwd->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.
@@ -379,16 +548,17 @@ static int __ipmi_set_timeout(struct ipmi_smi_msg  *smi_msg,
 	}
 
 	data[1] = 0;
-	WDOG_SET_TIMEOUT_ACT(data[1], ipmi_watchdog_state);
-	if ((pretimeout > 0) && (ipmi_watchdog_state != WDOG_TIMEOUT_NONE)) {
-	    WDOG_SET_PRETIMEOUT_ACT(data[1], preaction_val);
-	    data[2] = pretimeout;
+	WDOG_SET_TIMEOUT_ACT(data[1], iwd->state);
+	if ((iwd->wdd.pretimeout > 0) &&
+	    (iwd->state != WDOG_TIMEOUT_NONE)) {
+		WDOG_SET_PRETIMEOUT_ACT(data[1], preaction_val);
+		data[2] = iwd->wdd.pretimeout;
 	} else {
-	    WDOG_SET_PRETIMEOUT_ACT(data[1], WDOG_PRETIMEOUT_NONE);
-	    data[2] = 0; /* No pretimeout. */
+		WDOG_SET_PRETIMEOUT_ACT(data[1], WDOG_PRETIMEOUT_NONE);
+		data[2] = 0; /* No pretimeout. */
 	}
 	data[3] = 0;
-	WDOG_SET_TIMEOUT(data[4], data[5], timeout);
+	WDOG_SET_TIMEOUT(data[4], data[5], iwd->wdd.timeout);
 
 	addr.addr_type = IPMI_SYSTEM_INTERFACE_ADDR_TYPE;
 	addr.channel = IPMI_BMC_CHANNEL;
@@ -398,13 +568,13 @@ static int __ipmi_set_timeout(struct ipmi_smi_msg  *smi_msg,
 	msg.cmd = IPMI_WDOG_SET_TIMER;
 	msg.data = data;
 	msg.data_len = sizeof(data);
-	rv = ipmi_request_supply_msgs(watchdog_user,
+	rv = ipmi_request_supply_msgs(iwd->user,
 				      (struct ipmi_addr *) &addr,
 				      0,
 				      &msg,
 				      NULL,
-				      smi_msg,
-				      recv_msg,
+				      &iwd->smi_msg,
+				      &iwd->recv_msg,
 				      1);
 	if (rv)
 		pr_warn("set timeout error: %d\n", rv);
@@ -414,132 +584,79 @@ static int __ipmi_set_timeout(struct ipmi_smi_msg  *smi_msg,
 	return rv;
 }
 
-static int _ipmi_set_timeout(int do_heartbeat)
+static int _ipmi_update_timeout(struct ipmi_wdt *iwd, int do_heartbeat,
+				bool do_poll)
 {
 	int send_heartbeat_now;
 	int rv;
 
-	if (!watchdog_user)
-		return -ENODEV;
-
-	atomic_set(&msg_tofree, 2);
+	atomic_set(&iwd->msg_tofree, 2);
 
-	rv = __ipmi_set_timeout(&smi_msg,
-				&recv_msg,
-				&send_heartbeat_now);
-	if (rv)
+	rv = __ipmi_update_timeout(iwd, &send_heartbeat_now);
+	if (rv) {
+		atomic_set(&iwd->msg_tofree, 0);
 		return rv;
+	}
 
-	wait_for_completion(&msg_wait);
+	wait_msg_done(iwd, do_poll);
 
 	if ((do_heartbeat == IPMI_SET_TIMEOUT_FORCE_HB)
 		|| ((send_heartbeat_now)
 		    && (do_heartbeat == IPMI_SET_TIMEOUT_HB_IF_NECESSARY)))
-		rv = __ipmi_heartbeat();
+		rv = __ipmi_heartbeat(iwd, do_poll);
 
 	return rv;
 }
 
-static int ipmi_set_timeout(int do_heartbeat)
+static int ipmi_set_timeout(struct ipmi_wdt *iwd, int val, int do_heartbeat)
 {
 	int rv;
 
-	mutex_lock(&ipmi_watchdog_mutex);
-	rv = _ipmi_set_timeout(do_heartbeat);
-	mutex_unlock(&ipmi_watchdog_mutex);
+	mutex_lock(&iwd->lock);
+	if (val <= iwd->wdd.pretimeout) {
+		pr_warn("pretimeout set >= timeout, pretimeout disabled\n");
+		iwd->wdd.pretimeout = 0;
+	}
+	iwd->wdd.timeout = val;
+	rv = _ipmi_update_timeout(iwd, do_heartbeat, false);
+	mutex_unlock(&iwd->lock);
 
 	return rv;
 }
 
-static atomic_t panic_done_count = ATOMIC_INIT(0);
-
-static void panic_smi_free(struct ipmi_smi_msg *msg)
-{
-	atomic_dec(&panic_done_count);
-}
-static void panic_recv_free(struct ipmi_recv_msg *msg)
-{
-	atomic_dec(&panic_done_count);
-}
-
-static struct ipmi_smi_msg panic_halt_heartbeat_smi_msg = {
-	.done = panic_smi_free
-};
-static struct ipmi_recv_msg panic_halt_heartbeat_recv_msg = {
-	.done = panic_recv_free
-};
-
-static void panic_halt_ipmi_heartbeat(void)
+static int ipmi_set_pretimeout(struct ipmi_wdt *iwd, int val, int do_heartbeat)
 {
-	struct kernel_ipmi_msg             msg;
-	struct ipmi_system_interface_addr addr;
 	int rv;
 
-	/*
-	 * Don't reset the timer if we have the timer turned off, that
-	 * re-enables the watchdog.
-	 */
-	if (ipmi_watchdog_state == WDOG_TIMEOUT_NONE)
-		return;
-
-	addr.addr_type = IPMI_SYSTEM_INTERFACE_ADDR_TYPE;
-	addr.channel = IPMI_BMC_CHANNEL;
-	addr.lun = 0;
+	mutex_lock(&iwd->lock);
+	if (val >= iwd->wdd.timeout) {
+		pr_warn("pretimeout set >= timeout, pretimeout disabled\n");
+		val = 0;
+	}
+	iwd->wdd.pretimeout = val;
+	rv = _ipmi_update_timeout(iwd, do_heartbeat, false);
+	mutex_unlock(&iwd->lock);
 
-	msg.netfn = 0x06;
-	msg.cmd = IPMI_WDOG_RESET_TIMER;
-	msg.data = NULL;
-	msg.data_len = 0;
-	atomic_add(1, &panic_done_count);
-	rv = ipmi_request_supply_msgs(watchdog_user,
-				      (struct ipmi_addr *) &addr,
-				      0,
-				      &msg,
-				      NULL,
-				      &panic_halt_heartbeat_smi_msg,
-				      &panic_halt_heartbeat_recv_msg,
-				      1);
-	if (rv)
-		atomic_sub(1, &panic_done_count);
+	return rv;
 }
 
-static struct ipmi_smi_msg panic_halt_smi_msg = {
-	.done = panic_smi_free
-};
-static struct ipmi_recv_msg panic_halt_recv_msg = {
-	.done = panic_recv_free
-};
-
 /*
  * Special call, doesn't claim any locks.  This is only to be called
  * at panic or halt time, in run-to-completion mode, when the caller
  * is the only CPU and the only thing that will be going is these IPMI
  * calls.
  */
-static void panic_halt_ipmi_set_timeout(void)
+static void panic_halt_ipmi_update_timeout(struct ipmi_wdt *iwd)
 {
-	int send_heartbeat_now;
 	int rv;
 
-	/* Wait for the messages to be free. */
-	while (atomic_read(&panic_done_count) != 0)
-		ipmi_poll_interface(watchdog_user);
-	atomic_add(1, &panic_done_count);
-	rv = __ipmi_set_timeout(&panic_halt_smi_msg,
-				&panic_halt_recv_msg,
-				&send_heartbeat_now);
-	if (rv) {
-		atomic_sub(1, &panic_done_count);
+	wait_msg_done(iwd, true);
+	rv = _ipmi_update_timeout(iwd, IPMI_SET_TIMEOUT_HB_IF_NECESSARY, true);
+	if (rv)
 		pr_warn("Unable to extend the watchdog timeout\n");
-	} else {
-		if (send_heartbeat_now)
-			panic_halt_ipmi_heartbeat();
-	}
-	while (atomic_read(&panic_done_count) != 0)
-		ipmi_poll_interface(watchdog_user);
 }
 
-static int __ipmi_heartbeat(void)
+static int __ipmi_heartbeat(struct ipmi_wdt *iwd, bool do_poll)
 {
 	struct kernel_ipmi_msg msg;
 	int rv;
@@ -551,10 +668,10 @@ static int __ipmi_heartbeat(void)
 	 * Don't reset the timer if we have the timer turned off, that
 	 * re-enables the watchdog.
 	 */
-	if (ipmi_watchdog_state == WDOG_TIMEOUT_NONE)
+	if (iwd->state == WDOG_TIMEOUT_NONE)
 		return 0;
 
-	atomic_set(&msg_tofree, 2);
+	atomic_set(&iwd->msg_tofree, 2);
 
 	addr.addr_type = IPMI_SYSTEM_INTERFACE_ADDR_TYPE;
 	addr.channel = IPMI_BMC_CHANNEL;
@@ -564,23 +681,24 @@ static int __ipmi_heartbeat(void)
 	msg.cmd = IPMI_WDOG_RESET_TIMER;
 	msg.data = NULL;
 	msg.data_len = 0;
-	rv = ipmi_request_supply_msgs(watchdog_user,
+	rv = ipmi_request_supply_msgs(iwd->user,
 				      (struct ipmi_addr *) &addr,
 				      0,
 				      &msg,
 				      NULL,
-				      &smi_msg,
-				      &recv_msg,
+				      &iwd->smi_msg,
+				      &iwd->recv_msg,
 				      1);
 	if (rv) {
+		atomic_set(&iwd->msg_tofree, 0);
 		pr_warn("heartbeat send failure: %d\n", rv);
 		return rv;
 	}
 
 	/* Wait for the heartbeat to be sent. */
-	wait_for_completion(&msg_wait);
+	wait_msg_done(iwd, do_poll);
 
-	if (recv_msg.msg.data[0] == IPMI_WDOG_TIMER_NOT_INIT_RESP)  {
+	if (iwd->recv_msg.msg.data[0] == IPMI_WDOG_TIMER_NOT_INIT_RESP)  {
 		timeout_retries++;
 		if (timeout_retries > 3) {
 			pr_err("Unable to restore the IPMI watchdog's settings, giving up\n");
@@ -596,7 +714,8 @@ static int __ipmi_heartbeat(void)
 		 * in this process, so must say no heartbeat to avoid a
 		 * deadlock on this mutex
 		 */
-		rv = _ipmi_set_timeout(IPMI_SET_TIMEOUT_NO_HB);
+		rv = _ipmi_update_timeout(iwd, IPMI_SET_TIMEOUT_NO_HB,
+					  do_poll);
 		if (rv) {
 			pr_err("Unable to send the command to set the watchdog's settings, giving up\n");
 			goto out;
@@ -604,7 +723,7 @@ static int __ipmi_heartbeat(void)
 
 		/* Might need a heartbeat send, go ahead and do it. */
 		goto restart;
-	} else if (recv_msg.msg.data[0] != 0) {
+	} else if (iwd->recv_msg.msg.data[0] != 0) {
 		/*
 		 * Got an error in the heartbeat response.  It was already
 		 * reported in ipmi_wdog_msg_handler, but we should return
@@ -617,545 +736,434 @@ static int __ipmi_heartbeat(void)
 	return rv;
 }
 
-static int _ipmi_heartbeat(void)
+static int _ipmi_heartbeat(struct ipmi_wdt *iwd)
 {
 	int rv;
 
-	if (!watchdog_user)
-		return -ENODEV;
-
-	if (ipmi_start_timer_on_heartbeat) {
-		ipmi_start_timer_on_heartbeat = 0;
-		ipmi_watchdog_state = action_val;
-		rv = _ipmi_set_timeout(IPMI_SET_TIMEOUT_FORCE_HB);
-	} else if (atomic_cmpxchg(&pretimeout_since_last_heartbeat, 1, 0)) {
+	if (atomic_cmpxchg(&iwd->pretimeout_since_last_heartbeat, 1, 0)) {
 		/*
 		 * A pretimeout occurred, make sure we set the timeout.
 		 * We don't want to set the action, though, we want to
 		 * leave that alone (thus it can't be combined with the
 		 * above operation.
 		 */
-		rv = _ipmi_set_timeout(IPMI_SET_TIMEOUT_HB_IF_NECESSARY);
+		rv = _ipmi_update_timeout(iwd,
+					  IPMI_SET_TIMEOUT_HB_IF_NECESSARY,
+			false);
 	} else {
-		rv = __ipmi_heartbeat();
+		rv = __ipmi_heartbeat(iwd, false);
 	}
 
 	return rv;
 }
 
-static int ipmi_heartbeat(void)
+static void ipmi_wdog_msg_handler(struct ipmi_recv_msg *msg,
+				  void                 *handler_data)
 {
-	int rv;
-
-	mutex_lock(&ipmi_watchdog_mutex);
-	rv = _ipmi_heartbeat();
-	mutex_unlock(&ipmi_watchdog_mutex);
+	if (msg->msg.cmd == IPMI_WDOG_RESET_TIMER &&
+			msg->msg.data[0] == IPMI_WDOG_TIMER_NOT_INIT_RESP)
+		pr_info("response: The IPMI controller appears to have been reset, will attempt to reinitialize the watchdog timer\n");
+	else if (msg->msg.data[0] != 0)
+		pr_err("response: Error %x on cmd %x\n",
+		       msg->msg.data[0],
+		       msg->msg.cmd);
 
-	return rv;
+	ipmi_free_recv_msg(msg);
 }
 
-static struct watchdog_info ident = {
-	.options	= 0,	/* WDIOF_SETTIMEOUT, */
-	.firmware_version = 1,
-	.identity	= "IPMI"
-};
-
-static int ipmi_ioctl(struct file *file,
-		      unsigned int cmd, unsigned long arg)
+static void ipmi_wdog_pretimeout_handler(void *handler_data)
 {
-	void __user *argp = (void __user *)arg;
-	int i;
-	int val;
-
-	switch (cmd) {
-	case WDIOC_GETSUPPORT:
-		i = copy_to_user(argp, &ident, sizeof(ident));
-		return i ? -EFAULT : 0;
-
-	case WDIOC_SETTIMEOUT:
-		i = copy_from_user(&val, argp, sizeof(int));
-		if (i)
-			return -EFAULT;
-		timeout = val;
-		return _ipmi_set_timeout(IPMI_SET_TIMEOUT_HB_IF_NECESSARY);
-
-	case WDIOC_GETTIMEOUT:
-		i = copy_to_user(argp, &timeout, sizeof(timeout));
-		if (i)
-			return -EFAULT;
-		return 0;
+	struct ipmi_wdt *iwd = handler_data;
 
-	case WDIOC_SETPRETIMEOUT:
-		i = copy_from_user(&val, argp, sizeof(int));
-		if (i)
-			return -EFAULT;
-		pretimeout = val;
-		return _ipmi_set_timeout(IPMI_SET_TIMEOUT_HB_IF_NECESSARY);
-
-	case WDIOC_GETPRETIMEOUT:
-		i = copy_to_user(argp, &pretimeout, sizeof(pretimeout));
-		if (i)
-			return -EFAULT;
-		return 0;
+	/*
+	 * On some machines, the heartbeat will give an error and not
+	 * work unless we re-enable the timer.  So do so.  Do this
+	 * before the notify because it may panic.
+	 */
+	atomic_set(&iwd->pretimeout_since_last_heartbeat, 1);
 
-	case WDIOC_KEEPALIVE:
-		return _ipmi_heartbeat();
-
-	case WDIOC_SETOPTIONS:
-		i = copy_from_user(&val, argp, sizeof(int));
-		if (i)
-			return -EFAULT;
-		if (val & WDIOS_DISABLECARD) {
-			ipmi_watchdog_state = WDOG_TIMEOUT_NONE;
-			_ipmi_set_timeout(IPMI_SET_TIMEOUT_NO_HB);
-			ipmi_start_timer_on_heartbeat = 0;
-		}
+	if (!testing_nmi)
+		watchdog_notify_pretimeout(&iwd->wdd);
+}
 
-		if (val & WDIOS_ENABLECARD) {
-			ipmi_watchdog_state = action_val;
-			_ipmi_set_timeout(IPMI_SET_TIMEOUT_FORCE_HB);
-		}
-		return 0;
+static void ipmi_wdog_panic_handler(void *user_data)
+{
+	struct ipmi_wdt *iwd = user_data;
 
-	case WDIOC_GETSTATUS:
-		val = 0;
-		i = copy_to_user(argp, &val, sizeof(val));
-		if (i)
-			return -EFAULT;
-		return 0;
+	/*
+	 * On a panic, if we have a panic timeout, make sure to extend
+	 * the watchdog timer to a reasonable value to complete the
+	 * panic, if the watchdog timer is running.  Plus the
+	 * pretimeout is meaningless at panic time.
+	 */
+	if (!iwd->panic_event_handled &&
+	    iwd->state != WDOG_TIMEOUT_NONE) {
+		/* Make sure we do this only once. */
+		iwd->panic_event_handled = true;
 
-	default:
-		return -ENOIOCTLCMD;
+		iwd->wdd.timeout = panic_wdt_timeout;
+		iwd->wdd.pretimeout = 0;
+		panic_halt_ipmi_update_timeout(iwd);
 	}
 }
 
-static long ipmi_unlocked_ioctl(struct file *file,
-				unsigned int cmd,
-				unsigned long arg)
+static int ipmi_wdt_start(struct watchdog_device *wdd)
 {
-	int ret;
+	struct ipmi_wdt *iwd = wdd_to_ipmi_wdt(wdd);
+	int rv;
 
-	mutex_lock(&ipmi_watchdog_mutex);
-	ret = ipmi_ioctl(file, cmd, arg);
-	mutex_unlock(&ipmi_watchdog_mutex);
+	mutex_lock(&iwd->lock);
+	iwd->state = action_val;
+	rv = _ipmi_update_timeout(iwd, IPMI_SET_TIMEOUT_FORCE_HB, false);
+	mutex_unlock(&iwd->lock);
 
-	return ret;
+	return rv;
 }
 
-static ssize_t ipmi_write(struct file *file,
-			  const char  __user *buf,
-			  size_t      len,
-			  loff_t      *ppos)
+static int ipmi_wdt_stop(struct watchdog_device *wdd)
 {
+	struct ipmi_wdt *iwd = wdd_to_ipmi_wdt(wdd);
 	int rv;
 
-	if (len) {
-		if (!nowayout) {
-			size_t i;
+	mutex_lock(&iwd->lock);
+	iwd->state = WDOG_TIMEOUT_NONE;
+	rv = _ipmi_update_timeout(iwd, IPMI_SET_TIMEOUT_NO_HB, false);
+	mutex_unlock(&iwd->lock);
+
+	return rv;
+}
 
-			/* In case it was set long ago */
-			expect_close = 0;
+static int ipmi_wdt_ping(struct watchdog_device *wdd)
+{
+	struct ipmi_wdt *iwd = wdd_to_ipmi_wdt(wdd);
+	int rv;
 
-			for (i = 0; i != len; i++) {
-				char c;
+	mutex_lock(&iwd->lock);
+	rv = _ipmi_heartbeat(iwd);
+	mutex_unlock(&iwd->lock);
 
-				if (get_user(c, buf + i))
-					return -EFAULT;
-				if (c == 'V')
-					expect_close = 42;
-			}
-		}
-		rv = ipmi_heartbeat();
-		if (rv)
-			return rv;
-	}
-	return len;
+	return rv;
 }
 
-static ssize_t ipmi_read(struct file *file,
-			 char        __user *buf,
-			 size_t      count,
-			 loff_t      *ppos)
+static int ipmi_wdt_set_timeout(struct watchdog_device *wdd, unsigned int val)
 {
-	int          rv = 0;
-	wait_queue_entry_t wait;
+	struct ipmi_wdt *iwd = wdd_to_ipmi_wdt(wdd);
 
-	if (count <= 0)
-		return 0;
+	return ipmi_set_timeout(iwd, val, IPMI_SET_TIMEOUT_HB_IF_NECESSARY);
+}
 
-	/*
-	 * Reading returns if the pretimeout has gone off, and it only does
-	 * it once per pretimeout.
-	 */
-	spin_lock_irq(&ipmi_read_lock);
-	if (!data_to_read) {
-		if (file->f_flags & O_NONBLOCK) {
-			rv = -EAGAIN;
-			goto out;
-		}
-
-		init_waitqueue_entry(&wait, current);
-		add_wait_queue(&read_q, &wait);
-		while (!data_to_read) {
-			set_current_state(TASK_INTERRUPTIBLE);
-			spin_unlock_irq(&ipmi_read_lock);
-			schedule();
-			spin_lock_irq(&ipmi_read_lock);
-		}
-		remove_wait_queue(&read_q, &wait);
-
-		if (signal_pending(current)) {
-			rv = -ERESTARTSYS;
-			goto out;
-		}
-	}
-	data_to_read = 0;
+static int ipmi_wdt_set_pretimeout(struct watchdog_device *wdd,
+				   unsigned int val)
+{
+	struct ipmi_wdt *iwd = wdd_to_ipmi_wdt(wdd);
+	int rv;
 
- out:
-	spin_unlock_irq(&ipmi_read_lock);
-
-	if (rv == 0) {
-		if (copy_to_user(buf, &data_to_read, 1))
-			rv = -EFAULT;
-		else
-			rv = 1;
-	}
+	mutex_lock(&iwd->lock);
+	iwd->wdd.pretimeout = val;
+	rv = _ipmi_update_timeout(iwd, IPMI_SET_TIMEOUT_NO_HB, false);
+	mutex_unlock(&iwd->lock);
 
 	return rv;
 }
 
-static int ipmi_open(struct inode *ino, struct file *filep)
-{
-	switch (iminor(ino)) {
-	case WATCHDOG_MINOR:
-		if (test_and_set_bit(0, &ipmi_wdog_open))
-			return -EBUSY;
+static const struct watchdog_info ipmi_wdt_ident = {
+	.options	= WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT |
+			  WDIOF_PRETIMEOUT | WDIOF_MAGICCLOSE,
+	.firmware_version = 1,
+	.identity	= "IPMI"
+};
 
+static const struct watchdog_ops ipmi_wdt_ops = {
+	.owner		= THIS_MODULE,
+	.start		= ipmi_wdt_start,
+	.stop		= ipmi_wdt_stop,
+	.ping		= ipmi_wdt_ping,
+	.set_timeout	= ipmi_wdt_set_timeout,
+	.set_pretimeout = ipmi_wdt_set_pretimeout,
+};
 
-		/*
-		 * Don't start the timer now, let it start on the
-		 * first heartbeat.
-		 */
-		ipmi_start_timer_on_heartbeat = 1;
-		return stream_open(ino, filep);
+static const struct ipmi_user_hndl ipmi_hndlrs = {
+	.ipmi_recv_hndl           = ipmi_wdog_msg_handler,
+	.ipmi_watchdog_pretimeout = ipmi_wdog_pretimeout_handler,
+	.ipmi_panic_handler       = ipmi_wdog_panic_handler
+};
 
-	default:
-		return (-ENODEV);
-	}
-}
+#ifdef HAVE_DIE_NMI
+static DEFINE_MUTEX(ipmi_nmi_test_mutex);
+static bool nmi_handler_registered;
 
-static __poll_t ipmi_poll(struct file *file, poll_table *wait)
-{
-	__poll_t mask = 0;
+/* If a pretimeout occurs, this is used to allow only one panic to happen. */
+static atomic_t preop_panic_excl = ATOMIC_INIT(-1);
 
-	poll_wait(file, &read_q, wait);
+static int
+ipmi_nmi(unsigned int val, struct pt_regs *regs)
+{
+	struct ipmi_wdt *iwd = READ_ONCE(ipmi_wdt);
 
-	spin_lock_irq(&ipmi_read_lock);
-	if (data_to_read)
-		mask |= (EPOLLIN | EPOLLRDNORM);
-	spin_unlock_irq(&ipmi_read_lock);
+	/*
+	 * If we get here, it's an NMI that's not a memory or I/O
+	 * error.  We can't truly tell if it's from IPMI or not
+	 * without sending a message, and sending a message is almost
+	 * impossible because of locking.
+	 */
 
-	return mask;
-}
+	if (testing_nmi) {
+		testing_nmi = 2;
+		return NMI_HANDLED;
+	}
 
-static int ipmi_fasync(int fd, struct file *file, int on)
-{
-	int result;
+	if (!iwd)
+		return NMI_DONE;
 
-	result = fasync_helper(fd, file, on, &fasync_q);
+	/* If we are not expecting a timeout, ignore it. */
+	if (iwd->state == WDOG_TIMEOUT_NONE)
+		return NMI_DONE;
 
-	return (result);
-}
+	if (preaction_val != WDOG_PRETIMEOUT_NMI)
+		return NMI_DONE;
 
-static int ipmi_close(struct inode *ino, struct file *filep)
-{
-	if (iminor(ino) == WATCHDOG_MINOR) {
-		if (expect_close == 42) {
-			mutex_lock(&ipmi_watchdog_mutex);
-			ipmi_watchdog_state = WDOG_TIMEOUT_NONE;
-			_ipmi_set_timeout(IPMI_SET_TIMEOUT_NO_HB);
-			mutex_unlock(&ipmi_watchdog_mutex);
-		} else {
-			pr_crit("Unexpected close, not stopping watchdog!\n");
-			ipmi_heartbeat();
-		}
-		clear_bit(0, &ipmi_wdog_open);
+	/*
+	 * If no one else handled the NMI, we assume it was the IPMI
+	 * watchdog.
+	 */
+	if (preop_val == WDOG_PREOP_PANIC) {
+		/*
+		 * On some machines, the heartbeat will give an error
+		 * and not work unless we re-enable the timer.  So
+		 * make it do so on the next heartbeat.
+		 */
+		atomic_set(&iwd->pretimeout_since_last_heartbeat, 1);
+		if (atomic_inc_and_test(&preop_panic_excl))
+			nmi_panic(regs, "pre-timeout");
 	}
 
-	expect_close = 0;
-
-	return 0;
+	return NMI_HANDLED;
 }
 
-static const struct file_operations ipmi_wdog_fops = {
-	.owner   = THIS_MODULE,
-	.read    = ipmi_read,
-	.poll    = ipmi_poll,
-	.write   = ipmi_write,
-	.unlocked_ioctl = ipmi_unlocked_ioctl,
-	.open    = ipmi_open,
-	.release = ipmi_close,
-	.fasync  = ipmi_fasync,
-	.llseek  = no_llseek,
-};
-
-static struct miscdevice ipmi_wdog_miscdev = {
-	.minor		= WATCHDOG_MINOR,
-	.name		= "watchdog",
-	.fops		= &ipmi_wdog_fops
-};
-
-static void ipmi_wdog_msg_handler(struct ipmi_recv_msg *msg,
-				  void                 *handler_data)
+static void nmi_check(struct ipmi_wdt *iwd)
 {
-	if (msg->msg.cmd == IPMI_WDOG_RESET_TIMER &&
-			msg->msg.data[0] == IPMI_WDOG_TIMER_NOT_INIT_RESP)
-		pr_info("response: The IPMI controller appears to have been reset, will attempt to reinitialize the watchdog timer\n");
-	else if (msg->msg.data[0] != 0)
-		pr_err("response: Error %x on cmd %x\n",
-		       msg->msg.data[0],
-		       msg->msg.cmd);
+	int old_state = iwd->state;
+	int old_pretimeout = iwd->wdd.pretimeout;
+	int old_timeout = iwd->wdd.timeout;
+	int rv;
+	unsigned int count;
 
-	ipmi_free_recv_msg(msg);
-}
+	if (iwd->nmi_works)
+		return;
 
-static void ipmi_wdog_pretimeout_handler(void *handler_data)
-{
-	if (preaction_val != WDOG_PRETIMEOUT_NONE) {
-		if (preop_val == WDOG_PREOP_PANIC) {
-			if (atomic_inc_and_test(&preop_panic_excl))
-				panic("Watchdog pre-timeout");
-		} else if (preop_val == WDOG_PREOP_GIVE_DATA) {
-			unsigned long flags;
-
-			spin_lock_irqsave(&ipmi_read_lock, flags);
-			data_to_read = 1;
-			wake_up_interruptible(&read_q);
-			kill_fasync(&fasync_q, SIGIO, POLL_IN);
-			spin_unlock_irqrestore(&ipmi_read_lock, flags);
+	mutex_lock(&ipmi_nmi_test_mutex);
+	if (!nmi_handler_registered) {
+		rv = register_nmi_handler(NMI_UNKNOWN, ipmi_nmi, 0,
+					  "ipmi");
+		if (rv) {
+			iwd->nmi_works = false;
+			pr_warn("Can't register nmi handler\n");
+			goto out_restore;
+		} else {
+			nmi_handler_registered = true;
 		}
 	}
 
 	/*
-	 * On some machines, the heartbeat will give an error and not
-	 * work unless we re-enable the timer.  So do so.
+	 * Set the pretimeout to go off in a second and give
+	 * ourselves 99 seconds to stop the timer.
 	 */
-	atomic_set(&pretimeout_since_last_heartbeat, 1);
-}
+	iwd->state = WDOG_TIMEOUT_RESET;
+	iwd->wdd.pretimeout = 99;
+	iwd->wdd.timeout = 100;
 
-static void ipmi_wdog_panic_handler(void *user_data)
-{
-	static int panic_event_handled;
+	testing_nmi = 1;
 
-	/*
-	 * On a panic, if we have a panic timeout, make sure to extend
-	 * the watchdog timer to a reasonable value to complete the
-	 * panic, if the watchdog timer is running.  Plus the
-	 * pretimeout is meaningless at panic time.
-	 */
-	if (watchdog_user && !panic_event_handled &&
-	    ipmi_watchdog_state != WDOG_TIMEOUT_NONE) {
-		/* Make sure we do this only once. */
-		panic_event_handled = 1;
+	rv = _ipmi_update_timeout(iwd, IPMI_SET_TIMEOUT_FORCE_HB, false);
+	if (rv) {
+		pr_warn("Error starting timer to test NMI: 0x%x.  The NMI pretimeout will likely not work\n",
+			rv);
+		iwd->nmi_works = false;
+		goto out_restore;
+	}
 
-		timeout = panic_wdt_timeout;
-		pretimeout = 0;
-		panic_halt_ipmi_set_timeout();
+	if (iwd->recv_msg.msg.data[0] != 0)  {
+		pr_warn("Error in IPMI NMI pretimeout set: 0x%x.  The NMI pretimeout will likely not work\n",
+			iwd->recv_msg.msg.data[0]);
+		iwd->nmi_works = false;
+		goto out_restore;
 	}
-}
 
-static const struct ipmi_user_hndl ipmi_hndlrs = {
-	.ipmi_recv_hndl           = ipmi_wdog_msg_handler,
-	.ipmi_watchdog_pretimeout = ipmi_wdog_pretimeout_handler,
-	.ipmi_panic_handler       = ipmi_wdog_panic_handler
-};
+	for (count = 0; count < 50; count++) {
+		if (testing_nmi == 2)
+			break;
+		msleep(20);
+	}
+
+	if (testing_nmi == 2) {
+		iwd->nmi_works = true;
+	} else {
+		iwd->nmi_works = false;
+		pr_warn("IPMI NMI didn't seem to occur.  The NMI pretimeout will likely not work\n");
+	}
+ out_restore:
+	testing_nmi = 0;
+	mutex_unlock(&ipmi_nmi_test_mutex);
+	iwd->wdd.pretimeout = old_pretimeout;
+	iwd->wdd.timeout = old_timeout;
+	iwd->state = old_state;
+	rv = _ipmi_update_timeout(iwd, IPMI_SET_TIMEOUT_FORCE_HB, false);
+	if (rv)
+		pr_warn("Unable to stop timer after NMI test: 0x%x.\n", rv);
+}
+#else
+static void nmi_check(struct ipmi_wdt *iwd)
+{
+	iwd->nmi_works = false;
+}
+#endif
 
-static void ipmi_register_watchdog(int ipmi_intf)
+static void ipmi_register_watchdog(int ipmi_intf, struct device *dev)
 {
+	struct ipmi_wdt *iwd = NULL;
 	int rv = -EBUSY;
 
-	if (watchdog_user)
+	mutex_lock(&ipmi_wdt_data_mutex);
+	if ((ifnum_to_use != -1) && (ifnum_to_use != ipmi_intf))
 		goto out;
-
-	if ((ifnum_to_use >= 0) && (ifnum_to_use != ipmi_intf))
+	/* -1 means allow the first interface. */
+	if (ifnum_to_use == -1 && ipmi_wdt)
 		goto out;
 
-	watchdog_ifnum = ipmi_intf;
-
-	rv = ipmi_create_user(ipmi_intf, &ipmi_hndlrs, NULL, &watchdog_user);
+	iwd = kzalloc(sizeof(*iwd), GFP_KERNEL);
+	if (!iwd) {
+		rv = -ENOMEM;
+		goto out;
+	}
+	iwd->ifnum = ipmi_intf;
+	init_completion(&iwd->msg_wait);
+	iwd->smi_msg.done = msg_free_smi;
+	iwd->recv_msg.done = msg_free_recv;
+	iwd->state = WDOG_TIMEOUT_NONE;
+	mutex_init(&iwd->lock);
+
+	rv = ipmi_create_user(ipmi_intf, &ipmi_hndlrs, iwd, &iwd->user);
 	if (rv < 0) {
 		pr_crit("Unable to register with ipmi\n");
 		goto out;
 	}
 
-	rv = ipmi_get_version(watchdog_user,
-			      &ipmi_version_major,
-			      &ipmi_version_minor);
+	rv = ipmi_get_version(iwd->user,
+			      &iwd->ipmi_version_major,
+			      &iwd->ipmi_version_minor);
 	if (rv) {
 		pr_warn("Unable to get IPMI version, assuming 1.0\n");
-		ipmi_version_major = 1;
-		ipmi_version_minor = 0;
+		iwd->ipmi_version_major = 1;
+		iwd->ipmi_version_minor = 0;
 	}
 
-	rv = misc_register(&ipmi_wdog_miscdev);
-	if (rv < 0) {
-		ipmi_destroy_user(watchdog_user);
-		watchdog_user = NULL;
-		pr_crit("Unable to register misc device\n");
+	iwd->wdd.ops = &ipmi_wdt_ops;
+	iwd->info = ipmi_wdt_ident;
+	iwd->info.firmware_version = (iwd->ipmi_version_major << 8 |
+				      iwd->ipmi_version_minor);
+	iwd->wdd.info = &iwd->info;
+	iwd->wdd.max_timeout = IPMI_MAX_TIMEOUT;
+	iwd->wdd.min_timeout = IPMI_MIN_TIMEOUT;
+	watchdog_stop_on_unregister(&iwd->wdd);
+	watchdog_set_nowayout(&iwd->wdd, nowayout);
+	watchdog_init_timeout(&iwd->wdd, timeout, NULL);
+	iwd->wdd.pretimeout = pretimeout;
+	iwd->wdd.parent = dev;
+
+	rv = watchdog_register_device(&iwd->wdd);
+	if (rv) {
+		ipmi_destroy_user(iwd->user);
+		pr_crit("Unable to register IPMI watchdog device\n");
+		goto out;
 	}
 
-#ifdef HAVE_DIE_NMI
-	if (nmi_handler_registered) {
-		int old_pretimeout = pretimeout;
-		int old_timeout = timeout;
-		int old_preop_val = preop_val;
-
-		/*
-		 * Set the pretimeout to go off in a second and give
-		 * ourselves plenty of time to stop the timer.
-		 */
-		ipmi_watchdog_state = WDOG_TIMEOUT_RESET;
-		preop_val = WDOG_PREOP_NONE; /* Make sure nothing happens */
-		pretimeout = 99;
-		timeout = 100;
+	nmi_check(iwd);
 
-		testing_nmi = 1;
-
-		rv = ipmi_set_timeout(IPMI_SET_TIMEOUT_FORCE_HB);
-		if (rv) {
-			pr_warn("Error starting timer to test NMI: 0x%x.  The NMI pretimeout will likely not work\n",
-				rv);
-			rv = 0;
-			goto out_restore;
-		}
-
-		msleep(1500);
-
-		if (testing_nmi != 2) {
-			pr_warn("IPMI NMI didn't seem to occur.  The NMI pretimeout will likely not work\n");
-		}
- out_restore:
-		testing_nmi = 0;
-		preop_val = old_preop_val;
-		pretimeout = old_pretimeout;
-		timeout = old_timeout;
+	if (preaction_val == WDOG_PRETIMEOUT_NMI && !iwd->nmi_works) {
+		pr_warn("NMI pretimeout test failed, disabling pretimeout.\n");
+		preaction_val = WDOG_PRETIMEOUT_NONE;
 	}
-#endif
+
+	ipmi_wdt = iwd;
 
  out:
-	if ((start_now) && (rv == 0)) {
+	mutex_unlock(&ipmi_wdt_data_mutex);
+	if (start_now && rv == 0) {
 		/* Run from startup, so start the timer now. */
 		start_now = 0; /* Disable this function after first startup. */
-		ipmi_watchdog_state = action_val;
-		ipmi_set_timeout(IPMI_SET_TIMEOUT_FORCE_HB);
+		iwd->state = action_val;
+		mutex_lock(&iwd->lock);
+		_ipmi_update_timeout(iwd, IPMI_SET_TIMEOUT_FORCE_HB, false);
+		mutex_unlock(&iwd->lock);
 		pr_info("Starting now!\n");
-	} else {
-		/* Stop the timer now. */
-		ipmi_watchdog_state = WDOG_TIMEOUT_NONE;
-		ipmi_set_timeout(IPMI_SET_TIMEOUT_NO_HB);
+	} else if (iwd && rv != 0) {
+		kfree(iwd);
 	}
 }
 
-static void ipmi_unregister_watchdog(int ipmi_intf)
+static void _ipmi_unregister_watchdog(struct ipmi_wdt *iwd)
 {
 	int rv;
-	struct ipmi_user *loc_user = watchdog_user;
-
-	if (!loc_user)
-		return;
-
-	if (watchdog_ifnum != ipmi_intf)
-		return;
-
-	/* Make sure no one can call us any more. */
-	misc_deregister(&ipmi_wdog_miscdev);
-
-	watchdog_user = NULL;
 
 	/*
-	 * Wait to make sure the message makes it out.  The lower layer has
-	 * pointers to our buffers, we want to make sure they are done before
-	 * we release our memory.
+	 * This function should make sure that the misc device has no
+	 * outstanding calls, thus we should have no messages being
+	 * used after this.
 	 */
-	while (atomic_read(&msg_tofree))
-		msg_free_smi(NULL);
+	watchdog_unregister_device(&iwd->wdd);
 
-	mutex_lock(&ipmi_watchdog_mutex);
+	/* Make sure no NMIs or interrupts are running. */
+	synchronize_rcu();
 
 	/* Disconnect from IPMI. */
-	rv = ipmi_destroy_user(loc_user);
+	rv = ipmi_destroy_user(iwd->user);
 	if (rv)
 		pr_warn("error unlinking from IPMI: %d\n",  rv);
 
-	/* If it comes back, restart it properly. */
-	ipmi_start_timer_on_heartbeat = 1;
-
-	mutex_unlock(&ipmi_watchdog_mutex);
+	kfree(iwd);
 }
 
-#ifdef HAVE_DIE_NMI
-static int
-ipmi_nmi(unsigned int val, struct pt_regs *regs)
+static void ipmi_unregister_watchdog(int ipmi_intf)
 {
-	/*
-	 * If we get here, it's an NMI that's not a memory or I/O
-	 * error.  We can't truly tell if it's from IPMI or not
-	 * without sending a message, and sending a message is almost
-	 * impossible because of locking.
-	 */
+	struct ipmi_wdt *iwd = NULL;
 
-	if (testing_nmi) {
-		testing_nmi = 2;
-		return NMI_HANDLED;
-	}
-
-	/* If we are not expecting a timeout, ignore it. */
-	if (ipmi_watchdog_state == WDOG_TIMEOUT_NONE)
-		return NMI_DONE;
-
-	if (preaction_val != WDOG_PRETIMEOUT_NMI)
-		return NMI_DONE;
-
-	/*
-	 * If no one else handled the NMI, we assume it was the IPMI
-	 * watchdog.
-	 */
-	if (preop_val == WDOG_PREOP_PANIC) {
-		/* On some machines, the heartbeat will give
-		   an error and not work unless we re-enable
-		   the timer.   So do so. */
-		atomic_set(&pretimeout_since_last_heartbeat, 1);
-		if (atomic_inc_and_test(&preop_panic_excl))
-			nmi_panic(regs, "pre-timeout");
+	mutex_lock(&ipmi_wdt_data_mutex);
+	if (ipmi_wdt->ifnum == ipmi_intf) {
+		iwd = ipmi_wdt;
+		ipmi_wdt = NULL;
 	}
+	mutex_unlock(&ipmi_wdt_data_mutex);
+	if (!iwd)
+		return;
 
-	return NMI_HANDLED;
+	_ipmi_unregister_watchdog(iwd);
 }
-#endif
 
 static int wdog_reboot_handler(struct notifier_block *this,
 			       unsigned long         code,
 			       void                  *unused)
 {
 	static int reboot_event_handled;
+	struct ipmi_wdt *iwd = READ_ONCE(ipmi_wdt);
 
-	if ((watchdog_user) && (!reboot_event_handled)) {
+	if (iwd && !reboot_event_handled) {
 		/* Make sure we only do this once. */
 		reboot_event_handled = 1;
 
 		if (code == SYS_POWER_OFF || code == SYS_HALT) {
 			/* Disable the WDT if we are shutting down. */
-			ipmi_watchdog_state = WDOG_TIMEOUT_NONE;
-			ipmi_set_timeout(IPMI_SET_TIMEOUT_NO_HB);
-		} else if (ipmi_watchdog_state != WDOG_TIMEOUT_NONE) {
+			iwd->state = WDOG_TIMEOUT_NONE;
+			_ipmi_update_timeout(iwd, IPMI_SET_TIMEOUT_NO_HB,
+					     false);
+		} else if (iwd->state != WDOG_TIMEOUT_NONE) {
 			/* Set a long timer to let the reboot happen or
 			   reset if it hangs, but only if the watchdog
 			   timer was already running. */
-			if (timeout < 120)
-				timeout = 120;
-			pretimeout = 0;
-			ipmi_watchdog_state = WDOG_TIMEOUT_RESET;
-			ipmi_set_timeout(IPMI_SET_TIMEOUT_NO_HB);
+			if (iwd->wdd.timeout < panic_wdt_timeout)
+				iwd->wdd.timeout = panic_wdt_timeout;
+			iwd->wdd.pretimeout = 0;
+			_ipmi_update_timeout(iwd, IPMI_SET_TIMEOUT_NO_HB,
+					     false);
 		}
 	}
 	return NOTIFY_OK;
@@ -1167,147 +1175,30 @@ static struct notifier_block wdog_reboot_notifier = {
 	.priority	= 0
 };
 
-static void ipmi_new_smi(int if_num, struct device *device)
-{
-	ipmi_register_watchdog(if_num);
-}
-
-static void ipmi_smi_gone(int if_num)
-{
-	ipmi_unregister_watchdog(if_num);
-}
-
-static struct ipmi_smi_watcher smi_watcher = {
-	.owner    = THIS_MODULE,
-	.new_smi  = ipmi_new_smi,
-	.smi_gone = ipmi_smi_gone
-};
-
-static int action_op(const char *inval, char *outval)
-{
-	if (outval)
-		strcpy(outval, action);
-
-	if (!inval)
-		return 0;
-
-	if (strcmp(inval, "reset") == 0)
-		action_val = WDOG_TIMEOUT_RESET;
-	else if (strcmp(inval, "none") == 0)
-		action_val = WDOG_TIMEOUT_NONE;
-	else if (strcmp(inval, "power_cycle") == 0)
-		action_val = WDOG_TIMEOUT_POWER_CYCLE;
-	else if (strcmp(inval, "power_off") == 0)
-		action_val = WDOG_TIMEOUT_POWER_DOWN;
-	else
-		return -EINVAL;
-	strcpy(action, inval);
-	return 0;
-}
-
-static int preaction_op(const char *inval, char *outval)
-{
-	if (outval)
-		strcpy(outval, preaction);
-
-	if (!inval)
-		return 0;
-
-	if (strcmp(inval, "pre_none") == 0)
-		preaction_val = WDOG_PRETIMEOUT_NONE;
-	else if (strcmp(inval, "pre_smi") == 0)
-		preaction_val = WDOG_PRETIMEOUT_SMI;
-#ifdef HAVE_DIE_NMI
-	else if (strcmp(inval, "pre_nmi") == 0)
-		preaction_val = WDOG_PRETIMEOUT_NMI;
-#endif
-	else if (strcmp(inval, "pre_int") == 0)
-		preaction_val = WDOG_PRETIMEOUT_MSG_INT;
-	else
-		return -EINVAL;
-	strcpy(preaction, inval);
-	return 0;
-}
-
-static int preop_op(const char *inval, char *outval)
-{
-	if (outval)
-		strcpy(outval, preop);
-
-	if (!inval)
-		return 0;
-
-	if (strcmp(inval, "preop_none") == 0)
-		preop_val = WDOG_PREOP_NONE;
-	else if (strcmp(inval, "preop_panic") == 0)
-		preop_val = WDOG_PREOP_PANIC;
-	else if (strcmp(inval, "preop_give_data") == 0)
-		preop_val = WDOG_PREOP_GIVE_DATA;
-	else
-		return -EINVAL;
-	strcpy(preop, inval);
-	return 0;
-}
-
-static void check_parms(void)
-{
-#ifdef HAVE_DIE_NMI
-	int do_nmi = 0;
-	int rv;
-
-	if (preaction_val == WDOG_PRETIMEOUT_NMI) {
-		do_nmi = 1;
-		if (preop_val == WDOG_PREOP_GIVE_DATA) {
-			pr_warn("Pretimeout op is to give data but NMI pretimeout is enabled, setting pretimeout op to none\n");
-			preop_op("preop_none", NULL);
-			do_nmi = 0;
-		}
-	}
-	if (do_nmi && !nmi_handler_registered) {
-		rv = register_nmi_handler(NMI_UNKNOWN, ipmi_nmi, 0,
-						"ipmi");
-		if (rv) {
-			pr_warn("Can't register nmi handler\n");
-			return;
-		} else
-			nmi_handler_registered = 1;
-	} else if (!do_nmi && nmi_handler_registered) {
-		unregister_nmi_handler(NMI_UNKNOWN, "ipmi");
-		nmi_handler_registered = 0;
-	}
-#endif
-}
-
 static int __init ipmi_wdog_init(void)
 {
 	int rv;
 
 	if (action_op(action, NULL)) {
-		action_op("reset", NULL);
 		pr_info("Unknown action '%s', defaulting to reset\n", action);
+		action_op("reset", NULL);
 	}
 
 	if (preaction_op(preaction, NULL)) {
-		preaction_op("pre_none", NULL);
 		pr_info("Unknown preaction '%s', defaulting to none\n",
 			preaction);
+		preaction_op("pre_none", NULL);
 	}
 
 	if (preop_op(preop, NULL)) {
-		preop_op("preop_none", NULL);
 		pr_info("Unknown preop '%s', defaulting to none\n", preop);
+		preop_op("preop_none", NULL);
 	}
 
-	check_parms();
-
 	register_reboot_notifier(&wdog_reboot_notifier);
 
 	rv = ipmi_smi_watcher_register(&smi_watcher);
 	if (rv) {
-#ifdef HAVE_DIE_NMI
-		if (nmi_handler_registered)
-			unregister_nmi_handler(NMI_UNKNOWN, "ipmi");
-#endif
 		unregister_reboot_notifier(&wdog_reboot_notifier);
 		pr_warn("can't register smi watcher\n");
 		return rv;
@@ -1321,7 +1212,8 @@ static int __init ipmi_wdog_init(void)
 static void __exit ipmi_wdog_exit(void)
 {
 	ipmi_smi_watcher_unregister(&smi_watcher);
-	ipmi_unregister_watchdog(watchdog_ifnum);
+	if (ipmi_wdt)
+		ipmi_unregister_watchdog(ipmi_wdt->ifnum);
 
 #ifdef HAVE_DIE_NMI
 	if (nmi_handler_registered)
-- 
2.17.1

[PATCH 07/12] watchdog:ipmi: Add the ability to fetch the current time left

[minyard]

From: Corey Minyard <cminyard@mvista.com>

The new API has the capability, and so do the IPMI interface, so do
it.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 drivers/watchdog/ipmi_watchdog.c | 57 ++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/drivers/watchdog/ipmi_watchdog.c b/drivers/watchdog/ipmi_watchdog.c
index c8c11a61ebb9..218c01707c0b 100644
--- a/drivers/watchdog/ipmi_watchdog.c
+++ b/drivers/watchdog/ipmi_watchdog.c
@@ -757,6 +757,62 @@ static int _ipmi_heartbeat(struct ipmi_wdt *iwd)
 	return rv;
 }
 
+static unsigned int ipmi_wdt_get_timeleft(struct watchdog_device *wdd)
+{
+	struct ipmi_wdt *iwd = wdd_to_ipmi_wdt(wdd);
+	struct kernel_ipmi_msg msg;
+	int rv = 0;
+	struct ipmi_system_interface_addr addr;
+
+	mutex_lock(&iwd->lock);
+	if (iwd->state == WDOG_TIMEOUT_NONE)
+		goto out_unlock;
+
+	addr.addr_type = IPMI_SYSTEM_INTERFACE_ADDR_TYPE;
+	addr.channel = IPMI_BMC_CHANNEL;
+	addr.lun = 0;
+
+	atomic_set(&iwd->msg_tofree, 2);
+
+	msg.netfn = 0x06;
+	msg.cmd = IPMI_WDOG_GET_TIMER;
+	msg.data = NULL;
+	msg.data_len = 0;
+	rv = ipmi_request_supply_msgs(iwd->user,
+				      (struct ipmi_addr *) &addr,
+				      0,
+				      &msg,
+				      NULL,
+				      &iwd->smi_msg,
+				      &iwd->recv_msg,
+				      1);
+	if (rv) {
+		pr_warn("get timeout error: %d\n", rv);
+		goto out_unlock;
+	}
+
+	wait_msg_done(iwd, false);
+
+	if (iwd->recv_msg.msg.data[0] != 0)  {
+		pr_warn("get timeout IPMI error: %d\n",
+			iwd->recv_msg.msg.data[0]);
+		goto out_unlock;
+	}
+
+	if (iwd->recv_msg.msg.data_len < 9) {
+		pr_warn("get timeout IPMI response too short: %d\n",
+			iwd->recv_msg.msg.data_len);
+		goto out_unlock;
+	}
+
+	rv = iwd->recv_msg.msg.data[8] << 8 | iwd->recv_msg.msg.data[7];
+	rv /= 10; /* IPMI time is in 100s of milliseconds. */
+
+out_unlock:
+	mutex_unlock(&iwd->lock);
+	return rv;
+}
+
 static void ipmi_wdog_msg_handler(struct ipmi_recv_msg *msg,
 				  void                 *handler_data)
 {
@@ -880,6 +936,7 @@ static const struct watchdog_ops ipmi_wdt_ops = {
 	.ping		= ipmi_wdt_ping,
 	.set_timeout	= ipmi_wdt_set_timeout,
 	.set_pretimeout = ipmi_wdt_set_pretimeout,
+	.get_timeleft   = ipmi_wdt_get_timeleft,
 };
 
 static const struct ipmi_user_hndl ipmi_hndlrs = {
-- 
2.17.1

[PATCH 08/12] watchdog: Add the ability to set the action of a timeout

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Add a way to tell the watchdog what to do on a timeout and on
a pretimeout.  This is to support the IPMI watchdog's ability
to do this.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 Documentation/watchdog/watchdog-api.rst | 40 ++++++++++++
 drivers/watchdog/watchdog_dev.c         | 82 +++++++++++++++++++++++++
 include/linux/watchdog.h                |  4 ++
 include/uapi/linux/watchdog.h           | 14 +++++
 4 files changed, 140 insertions(+)

diff --git a/Documentation/watchdog/watchdog-api.rst b/Documentation/watchdog/watchdog-api.rst
index c6c1e9fa9f73..927be9e56b5d 100644
--- a/Documentation/watchdog/watchdog-api.rst
+++ b/Documentation/watchdog/watchdog-api.rst
@@ -112,6 +112,24 @@ current timeout using the GETTIMEOUT ioctl::
     ioctl(fd, WDIOC_GETTIMEOUT, &timeout);
     printf("The timeout was is %d seconds\n", timeout);
 
+Actions
+=======
+
+Some watchdog timers can perform different actions when they time out.
+Most will only reset.  The values are::
+
+    WDIOA_RESET - Reset the system
+    WDIOA_POWER_OFF - Power off the system
+    WDIOA_POWER_CYCLE - Power off the system then power it back on
+
+The value can be set::
+
+    ioctl(fd, WDIOC_SETACTION, &action);
+
+and queried::
+
+    ioctl(fd, WDIOC_GETACTION, &action);
+
 Pretimeouts
 ===========
 
@@ -137,6 +155,28 @@ There is also a get function for getting the pretimeout::
 
 Not all watchdog drivers will support a pretimeout.
 
+Preactions
+==========
+
+Like actions some watchdog timers can perform different actions when
+they pretimeout.  The values are::
+
+    WDIOP_NONE - Don't do anything on a pretimeout
+    WDIOP_NMI - Issue an NMI
+    WDIOP_SMI - Issue a system management interrupt
+    WDIOP_INTERRUPT - Issue a normal interrupt
+
+The value can be set::
+
+    ioctl(fd, WDIOC_SETPREACTION, &preaction);
+
+and queried::
+
+    ioctl(fd, WDIOC_GETPREACTION, &preaction);
+
+Note that the pretimeout governor that reads data is not compatible with
+the NMI preaction.  The NMI preaction can only do nothing or panic.
+
 Get the number of seconds before reboot
 =======================================
 
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 8e8304607a8c..0e70f510a491 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -423,6 +423,48 @@ static int watchdog_set_pretimeout(struct watchdog_device *wdd,
 	return err;
 }
 
+/*
+ *	watchdog_set_action: set the action the watchdog performs.
+ *	@wdd: the watchdog device to set the timeout for
+ *	@action: The action, one of WDIOA_xxx
+ *
+ *	The caller must hold wd_data->lock.
+ */
+
+static int watchdog_set_action(struct watchdog_device *wdd,
+			       unsigned int action)
+{
+	int err = 0;
+
+	if (wdd->ops->set_action)
+		err = wdd->ops->set_action(wdd, action);
+	else if (action != WDIOA_RESET)
+		err = -EINVAL;
+
+	return err;
+}
+
+/*
+ *	watchdog_set_preaction: set the action the watchdog pretimeout performs.
+ *	@wdd: the watchdog device to set the timeout for
+ *	@action: The action, one of WDIOP_xxx
+ *
+ *	The caller must hold wd_data->lock.
+ */
+
+static int watchdog_set_preaction(struct watchdog_device *wdd,
+				  unsigned int action)
+{
+	int err;
+
+	if (wdd->ops->set_preaction)
+		err = wdd->ops->set_preaction(wdd, action);
+	else
+		err = -EOPNOTSUPP;
+
+	return err;
+}
+
 /*
  *	watchdog_get_timeleft: wrapper to get the time left before a reboot
  *	@wdd: the watchdog device to get the remaining time from
@@ -516,6 +558,24 @@ static ssize_t pretimeout_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(pretimeout);
 
+static ssize_t action_show(struct device *dev,
+			   struct device_attribute *attr, char *buf)
+{
+	struct watchdog_device *wdd = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", wdd->action);
+}
+static DEVICE_ATTR_RO(action);
+
+static ssize_t preaction_show(struct device *dev,
+			      struct device_attribute *attr, char *buf)
+{
+	struct watchdog_device *wdd = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", wdd->preaction);
+}
+static DEVICE_ATTR_RO(preaction);
+
 static ssize_t identity_show(struct device *dev, struct device_attribute *attr,
 				char *buf)
 {
@@ -592,6 +652,8 @@ static struct attribute *wdt_attrs[] = {
 	&dev_attr_identity.attr,
 	&dev_attr_timeout.attr,
 	&dev_attr_pretimeout.attr,
+	&dev_attr_action.attr,
+	&dev_attr_preaction.attr,
 	&dev_attr_timeleft.attr,
 	&dev_attr_bootstatus.attr,
 	&dev_attr_status.attr,
@@ -784,6 +846,26 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
 	case WDIOC_GETPRETIMEOUT:
 		err = put_user(wdd->pretimeout, p);
 		break;
+	case WDIOC_SETACTION:
+		if (get_user(val, p)) {
+			err = -EFAULT;
+			break;
+		}
+		err = watchdog_set_action(wdd, val);
+		break;
+	case WDIOC_GETACTION:
+		err = put_user(wdd->action, p);
+		break;
+	case WDIOC_SETPREACTION:
+		if (get_user(val, p)) {
+			err = -EFAULT;
+			break;
+		}
+		err = watchdog_set_preaction(wdd, val);
+		break;
+	case WDIOC_GETPREACTION:
+		err = put_user(wdd->preaction, p);
+		break;
 	default:
 		err = -ENOTTY;
 		break;
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index e34501a822f0..d4644994106e 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -53,6 +53,8 @@ struct watchdog_ops {
 	unsigned int (*get_timeleft)(struct watchdog_device *);
 	int (*restart)(struct watchdog_device *, unsigned long, void *);
 	long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
+	int (*set_action)(struct watchdog_device *wdd, unsigned int val);
+	int (*set_preaction)(struct watchdog_device *wdd, unsigned int val);
 };
 
 /** struct watchdog_device - The structure that defines a watchdog device
@@ -101,6 +103,8 @@ struct watchdog_device {
 	unsigned int bootstatus;
 	unsigned int timeout;
 	unsigned int pretimeout;
+	unsigned int action;
+	unsigned int preaction;
 	unsigned int min_timeout;
 	unsigned int max_timeout;
 	unsigned int min_hw_heartbeat_ms;
diff --git a/include/uapi/linux/watchdog.h b/include/uapi/linux/watchdog.h
index b15cde5c9054..bf13cf25f9e0 100644
--- a/include/uapi/linux/watchdog.h
+++ b/include/uapi/linux/watchdog.h
@@ -32,6 +32,10 @@ struct watchdog_info {
 #define	WDIOC_SETPRETIMEOUT	_IOWR(WATCHDOG_IOCTL_BASE, 8, int)
 #define	WDIOC_GETPRETIMEOUT	_IOR(WATCHDOG_IOCTL_BASE, 9, int)
 #define	WDIOC_GETTIMELEFT	_IOR(WATCHDOG_IOCTL_BASE, 10, int)
+#define	WDIOC_SETACTION		_IOWR(WATCHDOG_IOCTL_BASE, 11, int)
+#define	WDIOC_GETACTION		_IOR(WATCHDOG_IOCTL_BASE, 12, int)
+#define	WDIOC_SETPREACTION	_IOWR(WATCHDOG_IOCTL_BASE, 13, int)
+#define	WDIOC_GETPREACTION	_IOR(WATCHDOG_IOCTL_BASE, 14, int)
 
 #define	WDIOF_UNKNOWN		-1	/* Unknown flag error */
 #define	WDIOS_UNKNOWN		-1	/* Unknown status error */
@@ -54,5 +58,15 @@ struct watchdog_info {
 #define	WDIOS_ENABLECARD	0x0002	/* Turn on the watchdog timer */
 #define	WDIOS_TEMPPANIC		0x0004	/* Kernel panic on temperature trip */
 
+/* Actions for WDIOC_xxxACTION ioctls. */
+#define WDIOA_RESET		0	/* Reset the system. */
+#define WDIOA_POWER_OFF		1	/* Power off the system. */
+#define WDIOA_POWER_CYCLE	2	/* Power cycle the system. */
+
+/* Actions for WDIOC_xxxPREACTION ioctls. */
+#define WDIOP_NONE		0	/* Do nothing. */
+#define WDIOP_NMI		1	/* Issue an NMI. */
+#define WDIOP_SMI		2	/* Issue a system management irq. */
+#define WDIOP_INTERRUPT		3	/* Issue a normal irq. */
 
 #endif /* _UAPI_LINUX_WATCHDOG_H */
-- 
2.17.1

Re: [PATCH 08/12] watchdog: Add the ability to set the action of a timeout

[Guenter Roeck]

On Mon, Aug 19, 2019 at 03:37:07PM -0500, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Add a way to tell the watchdog what to do on a timeout and on
> a pretimeout.  This is to support the IPMI watchdog's ability
> to do this.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  Documentation/watchdog/watchdog-api.rst | 40 ++++++++++++
>  drivers/watchdog/watchdog_dev.c         | 82 +++++++++++++++++++++++++
>  include/linux/watchdog.h                |  4 ++
>  include/uapi/linux/watchdog.h           | 14 +++++
>  4 files changed, 140 insertions(+)
> 
> diff --git a/Documentation/watchdog/watchdog-api.rst b/Documentation/watchdog/watchdog-api.rst
> index c6c1e9fa9f73..927be9e56b5d 100644
> --- a/Documentation/watchdog/watchdog-api.rst
> +++ b/Documentation/watchdog/watchdog-api.rst
> @@ -112,6 +112,24 @@ current timeout using the GETTIMEOUT ioctl::
>      ioctl(fd, WDIOC_GETTIMEOUT, &timeout);
>      printf("The timeout was is %d seconds\n", timeout);
>  
> +Actions
> +=======
> +
> +Some watchdog timers can perform different actions when they time out.
> +Most will only reset.  The values are::
> +
> +    WDIOA_RESET - Reset the system
> +    WDIOA_POWER_OFF - Power off the system
> +    WDIOA_POWER_CYCLE - Power off the system then power it back on
> +
> +The value can be set::
> +
> +    ioctl(fd, WDIOC_SETACTION, &action);
> +
> +and queried::
> +
> +    ioctl(fd, WDIOC_GETACTION, &action);
> +
>  Pretimeouts
>  ===========
>  
> @@ -137,6 +155,28 @@ There is also a get function for getting the pretimeout::
>  
>  Not all watchdog drivers will support a pretimeout.
>  
> +Preactions
> +==========
> +
> +Like actions some watchdog timers can perform different actions when
> +they pretimeout.  The values are::
> +
> +    WDIOP_NONE - Don't do anything on a pretimeout
> +    WDIOP_NMI - Issue an NMI
> +    WDIOP_SMI - Issue a system management interrupt
> +    WDIOP_INTERRUPT - Issue a normal interrupt
> +
> +The value can be set::
> +
> +    ioctl(fd, WDIOC_SETPREACTION, &preaction);
> +
> +and queried::
> +
> +    ioctl(fd, WDIOC_GETPREACTION, &preaction);
> +
> +Note that the pretimeout governor that reads data is not compatible with
> +the NMI preaction.  The NMI preaction can only do nothing or panic.
> +

I find this quite confusing. We would now have this ioctl, and then we
have the pretimeout sysfs attributes which are also supposed to set
pretimeout actions. This will require a bit more discussion for a
more concise and less confusing interface/API/ABI.

Guenter

>  Get the number of seconds before reboot
>  =======================================
>  
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 8e8304607a8c..0e70f510a491 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -423,6 +423,48 @@ static int watchdog_set_pretimeout(struct watchdog_device *wdd,
>  	return err;
>  }
>  
> +/*
> + *	watchdog_set_action: set the action the watchdog performs.
> + *	@wdd: the watchdog device to set the timeout for
> + *	@action: The action, one of WDIOA_xxx
> + *
> + *	The caller must hold wd_data->lock.
> + */
> +
> +static int watchdog_set_action(struct watchdog_device *wdd,
> +			       unsigned int action)
> +{
> +	int err = 0;
> +
> +	if (wdd->ops->set_action)
> +		err = wdd->ops->set_action(wdd, action);
> +	else if (action != WDIOA_RESET)
> +		err = -EINVAL;
> +
> +	return err;
> +}
> +
> +/*
> + *	watchdog_set_preaction: set the action the watchdog pretimeout performs.
> + *	@wdd: the watchdog device to set the timeout for
> + *	@action: The action, one of WDIOP_xxx
> + *
> + *	The caller must hold wd_data->lock.
> + */
> +
> +static int watchdog_set_preaction(struct watchdog_device *wdd,
> +				  unsigned int action)
> +{
> +	int err;
> +
> +	if (wdd->ops->set_preaction)
> +		err = wdd->ops->set_preaction(wdd, action);
> +	else
> +		err = -EOPNOTSUPP;
> +
> +	return err;
> +}
> +
>  /*
>   *	watchdog_get_timeleft: wrapper to get the time left before a reboot
>   *	@wdd: the watchdog device to get the remaining time from
> @@ -516,6 +558,24 @@ static ssize_t pretimeout_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RO(pretimeout);
>  
> +static ssize_t action_show(struct device *dev,
> +			   struct device_attribute *attr, char *buf)
> +{
> +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%u\n", wdd->action);
> +}
> +static DEVICE_ATTR_RO(action);
> +
> +static ssize_t preaction_show(struct device *dev,
> +			      struct device_attribute *attr, char *buf)
> +{
> +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%u\n", wdd->preaction);
> +}
> +static DEVICE_ATTR_RO(preaction);
> +
>  static ssize_t identity_show(struct device *dev, struct device_attribute *attr,
>  				char *buf)
>  {
> @@ -592,6 +652,8 @@ static struct attribute *wdt_attrs[] = {
>  	&dev_attr_identity.attr,
>  	&dev_attr_timeout.attr,
>  	&dev_attr_pretimeout.attr,
> +	&dev_attr_action.attr,
> +	&dev_attr_preaction.attr,
>  	&dev_attr_timeleft.attr,
>  	&dev_attr_bootstatus.attr,
>  	&dev_attr_status.attr,
> @@ -784,6 +846,26 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
>  	case WDIOC_GETPRETIMEOUT:
>  		err = put_user(wdd->pretimeout, p);
>  		break;
> +	case WDIOC_SETACTION:
> +		if (get_user(val, p)) {
> +			err = -EFAULT;
> +			break;
> +		}
> +		err = watchdog_set_action(wdd, val);
> +		break;
> +	case WDIOC_GETACTION:
> +		err = put_user(wdd->action, p);
> +		break;
> +	case WDIOC_SETPREACTION:
> +		if (get_user(val, p)) {
> +			err = -EFAULT;
> +			break;
> +		}
> +		err = watchdog_set_preaction(wdd, val);
> +		break;
> +	case WDIOC_GETPREACTION:
> +		err = put_user(wdd->preaction, p);
> +		break;
>  	default:
>  		err = -ENOTTY;
>  		break;
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index e34501a822f0..d4644994106e 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -53,6 +53,8 @@ struct watchdog_ops {
>  	unsigned int (*get_timeleft)(struct watchdog_device *);
>  	int (*restart)(struct watchdog_device *, unsigned long, void *);
>  	long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
> +	int (*set_action)(struct watchdog_device *wdd, unsigned int val);
> +	int (*set_preaction)(struct watchdog_device *wdd, unsigned int val);
>  };
>  
>  /** struct watchdog_device - The structure that defines a watchdog device
> @@ -101,6 +103,8 @@ struct watchdog_device {
>  	unsigned int bootstatus;
>  	unsigned int timeout;
>  	unsigned int pretimeout;
> +	unsigned int action;
> +	unsigned int preaction;
>  	unsigned int min_timeout;
>  	unsigned int max_timeout;
>  	unsigned int min_hw_heartbeat_ms;
> diff --git a/include/uapi/linux/watchdog.h b/include/uapi/linux/watchdog.h
> index b15cde5c9054..bf13cf25f9e0 100644
> --- a/include/uapi/linux/watchdog.h
> +++ b/include/uapi/linux/watchdog.h
> @@ -32,6 +32,10 @@ struct watchdog_info {
>  #define	WDIOC_SETPRETIMEOUT	_IOWR(WATCHDOG_IOCTL_BASE, 8, int)
>  #define	WDIOC_GETPRETIMEOUT	_IOR(WATCHDOG_IOCTL_BASE, 9, int)
>  #define	WDIOC_GETTIMELEFT	_IOR(WATCHDOG_IOCTL_BASE, 10, int)
> +#define	WDIOC_SETACTION		_IOWR(WATCHDOG_IOCTL_BASE, 11, int)
> +#define	WDIOC_GETACTION		_IOR(WATCHDOG_IOCTL_BASE, 12, int)
> +#define	WDIOC_SETPREACTION	_IOWR(WATCHDOG_IOCTL_BASE, 13, int)
> +#define	WDIOC_GETPREACTION	_IOR(WATCHDOG_IOCTL_BASE, 14, int)
>  
>  #define	WDIOF_UNKNOWN		-1	/* Unknown flag error */
>  #define	WDIOS_UNKNOWN		-1	/* Unknown status error */
> @@ -54,5 +58,15 @@ struct watchdog_info {
>  #define	WDIOS_ENABLECARD	0x0002	/* Turn on the watchdog timer */
>  #define	WDIOS_TEMPPANIC		0x0004	/* Kernel panic on temperature trip */
>  
> +/* Actions for WDIOC_xxxACTION ioctls. */
> +#define WDIOA_RESET		0	/* Reset the system. */
> +#define WDIOA_POWER_OFF		1	/* Power off the system. */
> +#define WDIOA_POWER_CYCLE	2	/* Power cycle the system. */
> +
> +/* Actions for WDIOC_xxxPREACTION ioctls. */
> +#define WDIOP_NONE		0	/* Do nothing. */
> +#define WDIOP_NMI		1	/* Issue an NMI. */
> +#define WDIOP_SMI		2	/* Issue a system management irq. */
> +#define WDIOP_INTERRUPT		3	/* Issue a normal irq. */
>  
>  #endif /* _UAPI_LINUX_WATCHDOG_H */
> -- 
> 2.17.1
> 

Re: [PATCH 08/12] watchdog: Add the ability to set the action of a timeout

[Corey Minyard]

On Mon, Aug 19, 2019 at 02:58:58PM -0700, Guenter Roeck wrote:
> On Mon, Aug 19, 2019 at 03:37:07PM -0500, minyard@acm.org wrote:
> > From: Corey Minyard <cminyard@mvista.com>
> > 
> > Add a way to tell the watchdog what to do on a timeout and on
> > a pretimeout.  This is to support the IPMI watchdog's ability
> > to do this.
> > 
> > Signed-off-by: Corey Minyard <cminyard@mvista.com>
> > ---
> >  Documentation/watchdog/watchdog-api.rst | 40 ++++++++++++
> >  drivers/watchdog/watchdog_dev.c         | 82 +++++++++++++++++++++++++
> >  include/linux/watchdog.h                |  4 ++
> >  include/uapi/linux/watchdog.h           | 14 +++++
> >  4 files changed, 140 insertions(+)
> > 
> > diff --git a/Documentation/watchdog/watchdog-api.rst b/Documentation/watchdog/watchdog-api.rst
> > index c6c1e9fa9f73..927be9e56b5d 100644
> > --- a/Documentation/watchdog/watchdog-api.rst
> > +++ b/Documentation/watchdog/watchdog-api.rst
> > @@ -112,6 +112,24 @@ current timeout using the GETTIMEOUT ioctl::
> >      ioctl(fd, WDIOC_GETTIMEOUT, &timeout);
> >      printf("The timeout was is %d seconds\n", timeout);
> >  
> > +Actions
> > +=======
> > +
> > +Some watchdog timers can perform different actions when they time out.
> > +Most will only reset.  The values are::
> > +
> > +    WDIOA_RESET - Reset the system
> > +    WDIOA_POWER_OFF - Power off the system
> > +    WDIOA_POWER_CYCLE - Power off the system then power it back on
> > +
> > +The value can be set::
> > +
> > +    ioctl(fd, WDIOC_SETACTION, &action);
> > +
> > +and queried::
> > +
> > +    ioctl(fd, WDIOC_GETACTION, &action);
> > +
> >  Pretimeouts
> >  ===========
> >  
> > @@ -137,6 +155,28 @@ There is also a get function for getting the pretimeout::
> >  
> >  Not all watchdog drivers will support a pretimeout.
> >  
> > +Preactions
> > +==========
> > +
> > +Like actions some watchdog timers can perform different actions when
> > +they pretimeout.  The values are::
> > +
> > +    WDIOP_NONE - Don't do anything on a pretimeout
> > +    WDIOP_NMI - Issue an NMI
> > +    WDIOP_SMI - Issue a system management interrupt
> > +    WDIOP_INTERRUPT - Issue a normal interrupt
> > +
> > +The value can be set::
> > +
> > +    ioctl(fd, WDIOC_SETPREACTION, &preaction);
> > +
> > +and queried::
> > +
> > +    ioctl(fd, WDIOC_GETPREACTION, &preaction);
> > +
> > +Note that the pretimeout governor that reads data is not compatible with
> > +the NMI preaction.  The NMI preaction can only do nothing or panic.
> > +
> 
> I find this quite confusing. We would now have this ioctl, and then we
> have the pretimeout sysfs attributes which are also supposed to set
> pretimeout actions. This will require a bit more discussion for a
> more concise and less confusing interface/API/ABI.

I'm a little confused.  The sysfs interfaces I added are read-only,
and I added them for consistency since everything else there is also
readable/settable by the ioctl (except the governor, which seemed odd).
What do you find confusing about this?  The action is just what the
watchdog does when it times out.  Is there a better way I could
explain this in the documentation?

I could leave the action/preaction handling in the IPMI watchdog
through the current interfaces, but that seems unnatural.  It seems
handy to be able to know what the timeout and pretimeout is going
to do.

This whole series is in general what I think it would take to
preserve current functionality in the IPMI watchdog and convert
it to the standard interface.  It seems like the goal is to
convert all the watchdogs over to the standard interface, which
I am fine with, but I am fine with leaving things the way they
are, too.

-corey

> 
> Guenter
> 
> >  Get the number of seconds before reboot
> >  =======================================
> >  
> > diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> > index 8e8304607a8c..0e70f510a491 100644
> > --- a/drivers/watchdog/watchdog_dev.c
> > +++ b/drivers/watchdog/watchdog_dev.c
> > @@ -423,6 +423,48 @@ static int watchdog_set_pretimeout(struct watchdog_device *wdd,
> >  	return err;
> >  }
> >  
> > +/*
> > + *	watchdog_set_action: set the action the watchdog performs.
> > + *	@wdd: the watchdog device to set the timeout for
> > + *	@action: The action, one of WDIOA_xxx
> > + *
> > + *	The caller must hold wd_data->lock.
> > + */
> > +
> > +static int watchdog_set_action(struct watchdog_device *wdd,
> > +			       unsigned int action)
> > +{
> > +	int err = 0;
> > +
> > +	if (wdd->ops->set_action)
> > +		err = wdd->ops->set_action(wdd, action);
> > +	else if (action != WDIOA_RESET)
> > +		err = -EINVAL;
> > +
> > +	return err;
> > +}
> > +
> > +/*
> > + *	watchdog_set_preaction: set the action the watchdog pretimeout performs.
> > + *	@wdd: the watchdog device to set the timeout for
> > + *	@action: The action, one of WDIOP_xxx
> > + *
> > + *	The caller must hold wd_data->lock.
> > + */
> > +
> > +static int watchdog_set_preaction(struct watchdog_device *wdd,
> > +				  unsigned int action)
> > +{
> > +	int err;
> > +
> > +	if (wdd->ops->set_preaction)
> > +		err = wdd->ops->set_preaction(wdd, action);
> > +	else
> > +		err = -EOPNOTSUPP;
> > +
> > +	return err;
> > +}
> > +
> >  /*
> >   *	watchdog_get_timeleft: wrapper to get the time left before a reboot
> >   *	@wdd: the watchdog device to get the remaining time from
> > @@ -516,6 +558,24 @@ static ssize_t pretimeout_show(struct device *dev,
> >  }
> >  static DEVICE_ATTR_RO(pretimeout);
> >  
> > +static ssize_t action_show(struct device *dev,
> > +			   struct device_attribute *attr, char *buf)
> > +{
> > +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> > +
> > +	return sprintf(buf, "%u\n", wdd->action);
> > +}
> > +static DEVICE_ATTR_RO(action);
> > +
> > +static ssize_t preaction_show(struct device *dev,
> > +			      struct device_attribute *attr, char *buf)
> > +{
> > +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> > +
> > +	return sprintf(buf, "%u\n", wdd->preaction);
> > +}
> > +static DEVICE_ATTR_RO(preaction);
> > +
> >  static ssize_t identity_show(struct device *dev, struct device_attribute *attr,
> >  				char *buf)
> >  {
> > @@ -592,6 +652,8 @@ static struct attribute *wdt_attrs[] = {
> >  	&dev_attr_identity.attr,
> >  	&dev_attr_timeout.attr,
> >  	&dev_attr_pretimeout.attr,
> > +	&dev_attr_action.attr,
> > +	&dev_attr_preaction.attr,
> >  	&dev_attr_timeleft.attr,
> >  	&dev_attr_bootstatus.attr,
> >  	&dev_attr_status.attr,
> > @@ -784,6 +846,26 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
> >  	case WDIOC_GETPRETIMEOUT:
> >  		err = put_user(wdd->pretimeout, p);
> >  		break;
> > +	case WDIOC_SETACTION:
> > +		if (get_user(val, p)) {
> > +			err = -EFAULT;
> > +			break;
> > +		}
> > +		err = watchdog_set_action(wdd, val);
> > +		break;
> > +	case WDIOC_GETACTION:
> > +		err = put_user(wdd->action, p);
> > +		break;
> > +	case WDIOC_SETPREACTION:
> > +		if (get_user(val, p)) {
> > +			err = -EFAULT;
> > +			break;
> > +		}
> > +		err = watchdog_set_preaction(wdd, val);
> > +		break;
> > +	case WDIOC_GETPREACTION:
> > +		err = put_user(wdd->preaction, p);
> > +		break;
> >  	default:
> >  		err = -ENOTTY;
> >  		break;
> > diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> > index e34501a822f0..d4644994106e 100644
> > --- a/include/linux/watchdog.h
> > +++ b/include/linux/watchdog.h
> > @@ -53,6 +53,8 @@ struct watchdog_ops {
> >  	unsigned int (*get_timeleft)(struct watchdog_device *);
> >  	int (*restart)(struct watchdog_device *, unsigned long, void *);
> >  	long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
> > +	int (*set_action)(struct watchdog_device *wdd, unsigned int val);
> > +	int (*set_preaction)(struct watchdog_device *wdd, unsigned int val);
> >  };
> >  
> >  /** struct watchdog_device - The structure that defines a watchdog device
> > @@ -101,6 +103,8 @@ struct watchdog_device {
> >  	unsigned int bootstatus;
> >  	unsigned int timeout;
> >  	unsigned int pretimeout;
> > +	unsigned int action;
> > +	unsigned int preaction;
> >  	unsigned int min_timeout;
> >  	unsigned int max_timeout;
> >  	unsigned int min_hw_heartbeat_ms;
> > diff --git a/include/uapi/linux/watchdog.h b/include/uapi/linux/watchdog.h
> > index b15cde5c9054..bf13cf25f9e0 100644
> > --- a/include/uapi/linux/watchdog.h
> > +++ b/include/uapi/linux/watchdog.h
> > @@ -32,6 +32,10 @@ struct watchdog_info {
> >  #define	WDIOC_SETPRETIMEOUT	_IOWR(WATCHDOG_IOCTL_BASE, 8, int)
> >  #define	WDIOC_GETPRETIMEOUT	_IOR(WATCHDOG_IOCTL_BASE, 9, int)
> >  #define	WDIOC_GETTIMELEFT	_IOR(WATCHDOG_IOCTL_BASE, 10, int)
> > +#define	WDIOC_SETACTION		_IOWR(WATCHDOG_IOCTL_BASE, 11, int)
> > +#define	WDIOC_GETACTION		_IOR(WATCHDOG_IOCTL_BASE, 12, int)
> > +#define	WDIOC_SETPREACTION	_IOWR(WATCHDOG_IOCTL_BASE, 13, int)
> > +#define	WDIOC_GETPREACTION	_IOR(WATCHDOG_IOCTL_BASE, 14, int)
> >  
> >  #define	WDIOF_UNKNOWN		-1	/* Unknown flag error */
> >  #define	WDIOS_UNKNOWN		-1	/* Unknown status error */
> > @@ -54,5 +58,15 @@ struct watchdog_info {
> >  #define	WDIOS_ENABLECARD	0x0002	/* Turn on the watchdog timer */
> >  #define	WDIOS_TEMPPANIC		0x0004	/* Kernel panic on temperature trip */
> >  
> > +/* Actions for WDIOC_xxxACTION ioctls. */
> > +#define WDIOA_RESET		0	/* Reset the system. */
> > +#define WDIOA_POWER_OFF		1	/* Power off the system. */
> > +#define WDIOA_POWER_CYCLE	2	/* Power cycle the system. */
> > +
> > +/* Actions for WDIOC_xxxPREACTION ioctls. */
> > +#define WDIOP_NONE		0	/* Do nothing. */
> > +#define WDIOP_NMI		1	/* Issue an NMI. */
> > +#define WDIOP_SMI		2	/* Issue a system management irq. */
> > +#define WDIOP_INTERRUPT		3	/* Issue a normal irq. */
> >  
> >  #endif /* _UAPI_LINUX_WATCHDOG_H */
> > -- 
> > 2.17.1
> > 

Re: [PATCH 08/12] watchdog: Add the ability to set the action of a timeout

[Guenter Roeck]

On 8/19/19 5:39 PM, Corey Minyard wrote:
> On Mon, Aug 19, 2019 at 02:58:58PM -0700, Guenter Roeck wrote:
>> On Mon, Aug 19, 2019 at 03:37:07PM -0500, minyard@acm.org wrote:
>>> From: Corey Minyard <cminyard@mvista.com>
>>>
>>> Add a way to tell the watchdog what to do on a timeout and on
>>> a pretimeout.  This is to support the IPMI watchdog's ability
>>> to do this.
>>>
>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>> ---
>>>   Documentation/watchdog/watchdog-api.rst | 40 ++++++++++++
>>>   drivers/watchdog/watchdog_dev.c         | 82 +++++++++++++++++++++++++
>>>   include/linux/watchdog.h                |  4 ++
>>>   include/uapi/linux/watchdog.h           | 14 +++++
>>>   4 files changed, 140 insertions(+)
>>>
>>> diff --git a/Documentation/watchdog/watchdog-api.rst b/Documentation/watchdog/watchdog-api.rst
>>> index c6c1e9fa9f73..927be9e56b5d 100644
>>> --- a/Documentation/watchdog/watchdog-api.rst
>>> +++ b/Documentation/watchdog/watchdog-api.rst
>>> @@ -112,6 +112,24 @@ current timeout using the GETTIMEOUT ioctl::
>>>       ioctl(fd, WDIOC_GETTIMEOUT, &timeout);
>>>       printf("The timeout was is %d seconds\n", timeout);
>>>   
>>> +Actions
>>> +=======
>>> +
>>> +Some watchdog timers can perform different actions when they time out.
>>> +Most will only reset.  The values are::
>>> +
>>> +    WDIOA_RESET - Reset the system
>>> +    WDIOA_POWER_OFF - Power off the system
>>> +    WDIOA_POWER_CYCLE - Power off the system then power it back on
>>> +
>>> +The value can be set::
>>> +
>>> +    ioctl(fd, WDIOC_SETACTION, &action);
>>> +
>>> +and queried::
>>> +
>>> +    ioctl(fd, WDIOC_GETACTION, &action);
>>> +
>>>   Pretimeouts
>>>   ===========
>>>   
>>> @@ -137,6 +155,28 @@ There is also a get function for getting the pretimeout::
>>>   
>>>   Not all watchdog drivers will support a pretimeout.
>>>   
>>> +Preactions
>>> +==========
>>> +
>>> +Like actions some watchdog timers can perform different actions when
>>> +they pretimeout.  The values are::
>>> +
>>> +    WDIOP_NONE - Don't do anything on a pretimeout
>>> +    WDIOP_NMI - Issue an NMI
>>> +    WDIOP_SMI - Issue a system management interrupt
>>> +    WDIOP_INTERRUPT - Issue a normal interrupt
>>> +
>>> +The value can be set::
>>> +
>>> +    ioctl(fd, WDIOC_SETPREACTION, &preaction);
>>> +
>>> +and queried::
>>> +
>>> +    ioctl(fd, WDIOC_GETPREACTION, &preaction);
>>> +
>>> +Note that the pretimeout governor that reads data is not compatible with
>>> +the NMI preaction.  The NMI preaction can only do nothing or panic.
>>> +
>>
>> I find this quite confusing. We would now have this ioctl, and then we
>> have the pretimeout sysfs attributes which are also supposed to set
>> pretimeout actions. This will require a bit more discussion for a
>> more concise and less confusing interface/API/ABI.
> 
> I'm a little confused.  The sysfs interfaces I added are read-only,

> and I added them for consistency since everything else there is also
> readable/settable by the ioctl (except the governor, which seemed odd).

The ioctl to set the pretimeout governor is questionable; the reason
for using sysfs to set it was that governor presence is controlled by
configuration options and module loads. An ioctl would fail unpredictably,
and the associated program would have no means to determine the means
why a request to set a specific governor failed.

> What do you find confusing about this?  The action is just what the
> watchdog does when it times out.  Is there a better way I could

With your proposal, there are two sets of actions for pretimeouts:
The actions determined by the pretimeout governor, and the actions
determined by the hardware/driver. Those actions are overlapping,
especially in the case of WDIOP_NONE. What if the governor is set
to panic, and the ioctl configured WDIOP_NONE ?

> explain this in the documentation?
> 
> I could leave the action/preaction handling in the IPMI watchdog
> through the current interfaces, but that seems unnatural.  It seems
> handy to be able to know what the timeout and pretimeout is going
> to do.
> 
> This whole series is in general what I think it would take to
> preserve current functionality in the IPMI watchdog and convert
> it to the standard interface.  It seems like the goal is to
> convert all the watchdogs over to the standard interface, which

It is, but any changes to the core API / ABI need to make sense and
have to be discussed on their own merits. We can not change the core
API to accommodate the private API/ABI used by each individual driver
that isn't following the standard. A better approach here would be to
determine if there is a valid need for an API/ABI extension which would
enhance support for more than one driver, and to find a means to ensure
that the needs of all drivers are met. That is not the case here.

Sure, we can discuss an API extension to be able to set the action to
be taken when a watchdog fires. But does it accommodate more than
the IPMI driver ? Are the actions complete, or may we need to support
some other actions (whatever those might be) ? Is it ever appropriate
to power off a system as result of a watchdog firing ? This needs to
be discussed separately, and this discussion should not be bundled
with a driver conversion to the watchdog subsystem. Until it is
discussed, it would be more appropriate to implement such ioctls
as driver-private ioctls (which the watchdog subsystem _does_ support).

Guenter

> I am fine with, but I am fine with leaving things the way they
> are, too.
> 
> -corey
> 
>>
>> Guenter
>>
>>>   Get the number of seconds before reboot
>>>   =======================================
>>>   
>>> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
>>> index 8e8304607a8c..0e70f510a491 100644
>>> --- a/drivers/watchdog/watchdog_dev.c
>>> +++ b/drivers/watchdog/watchdog_dev.c
>>> @@ -423,6 +423,48 @@ static int watchdog_set_pretimeout(struct watchdog_device *wdd,
>>>   	return err;
>>>   }
>>>   
>>> +/*
>>> + *	watchdog_set_action: set the action the watchdog performs.
>>> + *	@wdd: the watchdog device to set the timeout for
>>> + *	@action: The action, one of WDIOA_xxx
>>> + *
>>> + *	The caller must hold wd_data->lock.
>>> + */
>>> +
>>> +static int watchdog_set_action(struct watchdog_device *wdd,
>>> +			       unsigned int action)
>>> +{
>>> +	int err = 0;
>>> +
>>> +	if (wdd->ops->set_action)
>>> +		err = wdd->ops->set_action(wdd, action);
>>> +	else if (action != WDIOA_RESET)
>>> +		err = -EINVAL;
>>> +
>>> +	return err;
>>> +}
>>> +
>>> +/*
>>> + *	watchdog_set_preaction: set the action the watchdog pretimeout performs.
>>> + *	@wdd: the watchdog device to set the timeout for
>>> + *	@action: The action, one of WDIOP_xxx
>>> + *
>>> + *	The caller must hold wd_data->lock.
>>> + */
>>> +
>>> +static int watchdog_set_preaction(struct watchdog_device *wdd,
>>> +				  unsigned int action)
>>> +{
>>> +	int err;
>>> +
>>> +	if (wdd->ops->set_preaction)
>>> +		err = wdd->ops->set_preaction(wdd, action);
>>> +	else
>>> +		err = -EOPNOTSUPP;
>>> +
>>> +	return err;
>>> +}
>>> +
>>>   /*
>>>    *	watchdog_get_timeleft: wrapper to get the time left before a reboot
>>>    *	@wdd: the watchdog device to get the remaining time from
>>> @@ -516,6 +558,24 @@ static ssize_t pretimeout_show(struct device *dev,
>>>   }
>>>   static DEVICE_ATTR_RO(pretimeout);
>>>   
>>> +static ssize_t action_show(struct device *dev,
>>> +			   struct device_attribute *attr, char *buf)
>>> +{
>>> +	struct watchdog_device *wdd = dev_get_drvdata(dev);
>>> +
>>> +	return sprintf(buf, "%u\n", wdd->action);
>>> +}
>>> +static DEVICE_ATTR_RO(action);
>>> +
>>> +static ssize_t preaction_show(struct device *dev,
>>> +			      struct device_attribute *attr, char *buf)
>>> +{
>>> +	struct watchdog_device *wdd = dev_get_drvdata(dev);
>>> +
>>> +	return sprintf(buf, "%u\n", wdd->preaction);
>>> +}
>>> +static DEVICE_ATTR_RO(preaction);
>>> +
>>>   static ssize_t identity_show(struct device *dev, struct device_attribute *attr,
>>>   				char *buf)
>>>   {
>>> @@ -592,6 +652,8 @@ static struct attribute *wdt_attrs[] = {
>>>   	&dev_attr_identity.attr,
>>>   	&dev_attr_timeout.attr,
>>>   	&dev_attr_pretimeout.attr,
>>> +	&dev_attr_action.attr,
>>> +	&dev_attr_preaction.attr,
>>>   	&dev_attr_timeleft.attr,
>>>   	&dev_attr_bootstatus.attr,
>>>   	&dev_attr_status.attr,
>>> @@ -784,6 +846,26 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
>>>   	case WDIOC_GETPRETIMEOUT:
>>>   		err = put_user(wdd->pretimeout, p);
>>>   		break;
>>> +	case WDIOC_SETACTION:
>>> +		if (get_user(val, p)) {
>>> +			err = -EFAULT;
>>> +			break;
>>> +		}
>>> +		err = watchdog_set_action(wdd, val);
>>> +		break;
>>> +	case WDIOC_GETACTION:
>>> +		err = put_user(wdd->action, p);
>>> +		break;
>>> +	case WDIOC_SETPREACTION:
>>> +		if (get_user(val, p)) {
>>> +			err = -EFAULT;
>>> +			break;
>>> +		}
>>> +		err = watchdog_set_preaction(wdd, val);
>>> +		break;
>>> +	case WDIOC_GETPREACTION:
>>> +		err = put_user(wdd->preaction, p);
>>> +		break;
>>>   	default:
>>>   		err = -ENOTTY;
>>>   		break;
>>> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
>>> index e34501a822f0..d4644994106e 100644
>>> --- a/include/linux/watchdog.h
>>> +++ b/include/linux/watchdog.h
>>> @@ -53,6 +53,8 @@ struct watchdog_ops {
>>>   	unsigned int (*get_timeleft)(struct watchdog_device *);
>>>   	int (*restart)(struct watchdog_device *, unsigned long, void *);
>>>   	long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
>>> +	int (*set_action)(struct watchdog_device *wdd, unsigned int val);
>>> +	int (*set_preaction)(struct watchdog_device *wdd, unsigned int val);
>>>   };
>>>   
>>>   /** struct watchdog_device - The structure that defines a watchdog device
>>> @@ -101,6 +103,8 @@ struct watchdog_device {
>>>   	unsigned int bootstatus;
>>>   	unsigned int timeout;
>>>   	unsigned int pretimeout;
>>> +	unsigned int action;
>>> +	unsigned int preaction;
>>>   	unsigned int min_timeout;
>>>   	unsigned int max_timeout;
>>>   	unsigned int min_hw_heartbeat_ms;
>>> diff --git a/include/uapi/linux/watchdog.h b/include/uapi/linux/watchdog.h
>>> index b15cde5c9054..bf13cf25f9e0 100644
>>> --- a/include/uapi/linux/watchdog.h
>>> +++ b/include/uapi/linux/watchdog.h
>>> @@ -32,6 +32,10 @@ struct watchdog_info {
>>>   #define	WDIOC_SETPRETIMEOUT	_IOWR(WATCHDOG_IOCTL_BASE, 8, int)
>>>   #define	WDIOC_GETPRETIMEOUT	_IOR(WATCHDOG_IOCTL_BASE, 9, int)
>>>   #define	WDIOC_GETTIMELEFT	_IOR(WATCHDOG_IOCTL_BASE, 10, int)
>>> +#define	WDIOC_SETACTION		_IOWR(WATCHDOG_IOCTL_BASE, 11, int)
>>> +#define	WDIOC_GETACTION		_IOR(WATCHDOG_IOCTL_BASE, 12, int)
>>> +#define	WDIOC_SETPREACTION	_IOWR(WATCHDOG_IOCTL_BASE, 13, int)
>>> +#define	WDIOC_GETPREACTION	_IOR(WATCHDOG_IOCTL_BASE, 14, int)
>>>   
>>>   #define	WDIOF_UNKNOWN		-1	/* Unknown flag error */
>>>   #define	WDIOS_UNKNOWN		-1	/* Unknown status error */
>>> @@ -54,5 +58,15 @@ struct watchdog_info {
>>>   #define	WDIOS_ENABLECARD	0x0002	/* Turn on the watchdog timer */
>>>   #define	WDIOS_TEMPPANIC		0x0004	/* Kernel panic on temperature trip */
>>>   
>>> +/* Actions for WDIOC_xxxACTION ioctls. */
>>> +#define WDIOA_RESET		0	/* Reset the system. */
>>> +#define WDIOA_POWER_OFF		1	/* Power off the system. */
>>> +#define WDIOA_POWER_CYCLE	2	/* Power cycle the system. */
>>> +
>>> +/* Actions for WDIOC_xxxPREACTION ioctls. */
>>> +#define WDIOP_NONE		0	/* Do nothing. */
>>> +#define WDIOP_NMI		1	/* Issue an NMI. */
>>> +#define WDIOP_SMI		2	/* Issue a system management irq. */
>>> +#define WDIOP_INTERRUPT		3	/* Issue a normal irq. */
>>>   
>>>   #endif /* _UAPI_LINUX_WATCHDOG_H */
>>> -- 
>>> 2.17.1
>>>
> 

Re: [PATCH 08/12] watchdog: Add the ability to set the action of a timeout

[Corey Minyard]

On Tue, Aug 20, 2019 at 07:17:30AM -0700, Guenter Roeck wrote:
> On 8/19/19 5:39 PM, Corey Minyard wrote:
> > On Mon, Aug 19, 2019 at 02:58:58PM -0700, Guenter Roeck wrote:
> > > On Mon, Aug 19, 2019 at 03:37:07PM -0500, minyard@acm.org wrote:
> > > > From: Corey Minyard <cminyard@mvista.com>
> > > > 
> > > > Add a way to tell the watchdog what to do on a timeout and on
> > > > a pretimeout.  This is to support the IPMI watchdog's ability
> > > > to do this.
> > > > 
> > > > Signed-off-by: Corey Minyard <cminyard@mvista.com>
> > > > ---
> > > >   Documentation/watchdog/watchdog-api.rst | 40 ++++++++++++
> > > >   drivers/watchdog/watchdog_dev.c         | 82 +++++++++++++++++++++++++
> > > >   include/linux/watchdog.h                |  4 ++
> > > >   include/uapi/linux/watchdog.h           | 14 +++++
> > > >   4 files changed, 140 insertions(+)
> > > > 
> > > > diff --git a/Documentation/watchdog/watchdog-api.rst b/Documentation/watchdog/watchdog-api.rst
> > > > index c6c1e9fa9f73..927be9e56b5d 100644
> > > > --- a/Documentation/watchdog/watchdog-api.rst
> > > > +++ b/Documentation/watchdog/watchdog-api.rst
> > > > @@ -112,6 +112,24 @@ current timeout using the GETTIMEOUT ioctl::
> > > >       ioctl(fd, WDIOC_GETTIMEOUT, &timeout);
> > > >       printf("The timeout was is %d seconds\n", timeout);
> > > > +Actions
> > > > +=======
> > > > +
> > > > +Some watchdog timers can perform different actions when they time out.
> > > > +Most will only reset.  The values are::
> > > > +
> > > > +    WDIOA_RESET - Reset the system
> > > > +    WDIOA_POWER_OFF - Power off the system
> > > > +    WDIOA_POWER_CYCLE - Power off the system then power it back on
> > > > +
> > > > +The value can be set::
> > > > +
> > > > +    ioctl(fd, WDIOC_SETACTION, &action);
> > > > +
> > > > +and queried::
> > > > +
> > > > +    ioctl(fd, WDIOC_GETACTION, &action);
> > > > +
> > > >   Pretimeouts
> > > >   ===========
> > > > @@ -137,6 +155,28 @@ There is also a get function for getting the pretimeout::
> > > >   Not all watchdog drivers will support a pretimeout.
> > > > +Preactions
> > > > +==========
> > > > +
> > > > +Like actions some watchdog timers can perform different actions when
> > > > +they pretimeout.  The values are::
> > > > +
> > > > +    WDIOP_NONE - Don't do anything on a pretimeout
> > > > +    WDIOP_NMI - Issue an NMI
> > > > +    WDIOP_SMI - Issue a system management interrupt
> > > > +    WDIOP_INTERRUPT - Issue a normal interrupt
> > > > +
> > > > +The value can be set::
> > > > +
> > > > +    ioctl(fd, WDIOC_SETPREACTION, &preaction);
> > > > +
> > > > +and queried::
> > > > +
> > > > +    ioctl(fd, WDIOC_GETPREACTION, &preaction);
> > > > +
> > > > +Note that the pretimeout governor that reads data is not compatible with
> > > > +the NMI preaction.  The NMI preaction can only do nothing or panic.
> > > > +
> > > 
> > > I find this quite confusing. We would now have this ioctl, and then we
> > > have the pretimeout sysfs attributes which are also supposed to set
> > > pretimeout actions. This will require a bit more discussion for a
> > > more concise and less confusing interface/API/ABI.
> > 
> > I'm a little confused.  The sysfs interfaces I added are read-only,
> 
> > and I added them for consistency since everything else there is also
> > readable/settable by the ioctl (except the governor, which seemed odd).
> 
> The ioctl to set the pretimeout governor is questionable; the reason
> for using sysfs to set it was that governor presence is controlled by
> configuration options and module loads. An ioctl would fail unpredictably,
> and the associated program would have no means to determine the means
> why a request to set a specific governor failed.
> 
> > What do you find confusing about this?  The action is just what the
> > watchdog does when it times out.  Is there a better way I could
> 
> With your proposal, there are two sets of actions for pretimeouts:
> The actions determined by the pretimeout governor, and the actions
> determined by the hardware/driver. Those actions are overlapping,
> especially in the case of WDIOP_NONE. What if the governor is set
> to panic, and the ioctl configured WDIOP_NONE ?

The same thing that happens if the governor is set to panic and
the pretimeout is set to zero.

WDIOP_NONE is a strange one, but having a noop governor is just
as strange.  Just set the pretimeout to zero to disable it.
Why have a noop?  WDIOP_NONE could be removed for the same
reason, and that's probably good.  Just set the pretimeout to
zero.

Other inconsistencies exist today, like you can't use a governor
for an NMI pretimeout action, the only thing you can really do is
nmi_panic().

> 
> > explain this in the documentation?
> > 
> > I could leave the action/preaction handling in the IPMI watchdog
> > through the current interfaces, but that seems unnatural.  It seems
> > handy to be able to know what the timeout and pretimeout is going
> > to do.
> > 
> > This whole series is in general what I think it would take to
> > preserve current functionality in the IPMI watchdog and convert
> > it to the standard interface.  It seems like the goal is to
> > convert all the watchdogs over to the standard interface, which
> 
> It is, but any changes to the core API / ABI need to make sense and
> have to be discussed on their own merits. We can not change the core
> API to accommodate the private API/ABI used by each individual driver
> that isn't following the standard. A better approach here would be to
> determine if there is a valid need for an API/ABI extension which would
> enhance support for more than one driver, and to find a means to ensure
> that the needs of all drivers are met. That is not the case here.
> 
> Sure, we can discuss an API extension to be able to set the action to
> be taken when a watchdog fires. But does it accommodate more than
> the IPMI driver ?

Maybe.  I don't know what other watchdog interfaces can do.  It seems
there is one that can sound an alarm.  Perhaps there are some that
can do multiple things, like sound an alarm and reset the system?
Flash lights on the from of the system?  Self destruct?

For pretimeouts, I added a patch to set the pretimeout actions for
all the drivers that supported pretimeouts.  That way you can query
what the driver will do on a pretimeout, even if you can't set it.
That seems handy to be able to query.  Of course, then you might
want a way to query all the things it can do, and WDIOF_xxx bits
are running out, so that would be yet another interface.

> Are the actions complete, or may we need to support
> some other actions (whatever those might be) ?

It's easy enough to add the new actions if they come.  It seems
to me that putting that into the options isn't a good way to do it.

> Is it ever appropriate
> to power off a system as result of a watchdog firing ?

I have wondered that for many years :).  I don't know, but it's
a capability the hardware has.  It got added to the spec for some
reason.

> This needs to
> be discussed separately, and this discussion should not be bundled
> with a driver conversion to the watchdog subsystem. Until it is
> discussed, it would be more appropriate to implement such ioctls
> as driver-private ioctls (which the watchdog subsystem _does_ support).

If you prefer that, I can add an ipmi_watchdog.h file with those
ioctls.  I was proposing this as something more general that might
be useful.  But if it gets added as a general function in the future,
doing a special one for the IPMI one makes things kind of a pain.

Thanks,

-corey

> 
> Guenter
> 
> > I am fine with, but I am fine with leaving things the way they
> > are, too.
> > 
> > -corey
> > 
> > > 
> > > Guenter
> > > 
> > > >   Get the number of seconds before reboot
> > > >   =======================================
> > > > diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> > > > index 8e8304607a8c..0e70f510a491 100644
> > > > --- a/drivers/watchdog/watchdog_dev.c
> > > > +++ b/drivers/watchdog/watchdog_dev.c
> > > > @@ -423,6 +423,48 @@ static int watchdog_set_pretimeout(struct watchdog_device *wdd,
> > > >   	return err;
> > > >   }
> > > > +/*
> > > > + *	watchdog_set_action: set the action the watchdog performs.
> > > > + *	@wdd: the watchdog device to set the timeout for
> > > > + *	@action: The action, one of WDIOA_xxx
> > > > + *
> > > > + *	The caller must hold wd_data->lock.
> > > > + */
> > > > +
> > > > +static int watchdog_set_action(struct watchdog_device *wdd,
> > > > +			       unsigned int action)
> > > > +{
> > > > +	int err = 0;
> > > > +
> > > > +	if (wdd->ops->set_action)
> > > > +		err = wdd->ops->set_action(wdd, action);
> > > > +	else if (action != WDIOA_RESET)
> > > > +		err = -EINVAL;
> > > > +
> > > > +	return err;
> > > > +}
> > > > +
> > > > +/*
> > > > + *	watchdog_set_preaction: set the action the watchdog pretimeout performs.
> > > > + *	@wdd: the watchdog device to set the timeout for
> > > > + *	@action: The action, one of WDIOP_xxx
> > > > + *
> > > > + *	The caller must hold wd_data->lock.
> > > > + */
> > > > +
> > > > +static int watchdog_set_preaction(struct watchdog_device *wdd,
> > > > +				  unsigned int action)
> > > > +{
> > > > +	int err;
> > > > +
> > > > +	if (wdd->ops->set_preaction)
> > > > +		err = wdd->ops->set_preaction(wdd, action);
> > > > +	else
> > > > +		err = -EOPNOTSUPP;
> > > > +
> > > > +	return err;
> > > > +}
> > > > +
> > > >   /*
> > > >    *	watchdog_get_timeleft: wrapper to get the time left before a reboot
> > > >    *	@wdd: the watchdog device to get the remaining time from
> > > > @@ -516,6 +558,24 @@ static ssize_t pretimeout_show(struct device *dev,
> > > >   }
> > > >   static DEVICE_ATTR_RO(pretimeout);
> > > > +static ssize_t action_show(struct device *dev,
> > > > +			   struct device_attribute *attr, char *buf)
> > > > +{
> > > > +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> > > > +
> > > > +	return sprintf(buf, "%u\n", wdd->action);
> > > > +}
> > > > +static DEVICE_ATTR_RO(action);
> > > > +
> > > > +static ssize_t preaction_show(struct device *dev,
> > > > +			      struct device_attribute *attr, char *buf)
> > > > +{
> > > > +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> > > > +
> > > > +	return sprintf(buf, "%u\n", wdd->preaction);
> > > > +}
> > > > +static DEVICE_ATTR_RO(preaction);
> > > > +
> > > >   static ssize_t identity_show(struct device *dev, struct device_attribute *attr,
> > > >   				char *buf)
> > > >   {
> > > > @@ -592,6 +652,8 @@ static struct attribute *wdt_attrs[] = {
> > > >   	&dev_attr_identity.attr,
> > > >   	&dev_attr_timeout.attr,
> > > >   	&dev_attr_pretimeout.attr,
> > > > +	&dev_attr_action.attr,
> > > > +	&dev_attr_preaction.attr,
> > > >   	&dev_attr_timeleft.attr,
> > > >   	&dev_attr_bootstatus.attr,
> > > >   	&dev_attr_status.attr,
> > > > @@ -784,6 +846,26 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
> > > >   	case WDIOC_GETPRETIMEOUT:
> > > >   		err = put_user(wdd->pretimeout, p);
> > > >   		break;
> > > > +	case WDIOC_SETACTION:
> > > > +		if (get_user(val, p)) {
> > > > +			err = -EFAULT;
> > > > +			break;
> > > > +		}
> > > > +		err = watchdog_set_action(wdd, val);
> > > > +		break;
> > > > +	case WDIOC_GETACTION:
> > > > +		err = put_user(wdd->action, p);
> > > > +		break;
> > > > +	case WDIOC_SETPREACTION:
> > > > +		if (get_user(val, p)) {
> > > > +			err = -EFAULT;
> > > > +			break;
> > > > +		}
> > > > +		err = watchdog_set_preaction(wdd, val);
> > > > +		break;
> > > > +	case WDIOC_GETPREACTION:
> > > > +		err = put_user(wdd->preaction, p);
> > > > +		break;
> > > >   	default:
> > > >   		err = -ENOTTY;
> > > >   		break;
> > > > diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> > > > index e34501a822f0..d4644994106e 100644
> > > > --- a/include/linux/watchdog.h
> > > > +++ b/include/linux/watchdog.h
> > > > @@ -53,6 +53,8 @@ struct watchdog_ops {
> > > >   	unsigned int (*get_timeleft)(struct watchdog_device *);
> > > >   	int (*restart)(struct watchdog_device *, unsigned long, void *);
> > > >   	long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
> > > > +	int (*set_action)(struct watchdog_device *wdd, unsigned int val);
> > > > +	int (*set_preaction)(struct watchdog_device *wdd, unsigned int val);
> > > >   };
> > > >   /** struct watchdog_device - The structure that defines a watchdog device
> > > > @@ -101,6 +103,8 @@ struct watchdog_device {
> > > >   	unsigned int bootstatus;
> > > >   	unsigned int timeout;
> > > >   	unsigned int pretimeout;
> > > > +	unsigned int action;
> > > > +	unsigned int preaction;
> > > >   	unsigned int min_timeout;
> > > >   	unsigned int max_timeout;
> > > >   	unsigned int min_hw_heartbeat_ms;
> > > > diff --git a/include/uapi/linux/watchdog.h b/include/uapi/linux/watchdog.h
> > > > index b15cde5c9054..bf13cf25f9e0 100644
> > > > --- a/include/uapi/linux/watchdog.h
> > > > +++ b/include/uapi/linux/watchdog.h
> > > > @@ -32,6 +32,10 @@ struct watchdog_info {
> > > >   #define	WDIOC_SETPRETIMEOUT	_IOWR(WATCHDOG_IOCTL_BASE, 8, int)
> > > >   #define	WDIOC_GETPRETIMEOUT	_IOR(WATCHDOG_IOCTL_BASE, 9, int)
> > > >   #define	WDIOC_GETTIMELEFT	_IOR(WATCHDOG_IOCTL_BASE, 10, int)
> > > > +#define	WDIOC_SETACTION		_IOWR(WATCHDOG_IOCTL_BASE, 11, int)
> > > > +#define	WDIOC_GETACTION		_IOR(WATCHDOG_IOCTL_BASE, 12, int)
> > > > +#define	WDIOC_SETPREACTION	_IOWR(WATCHDOG_IOCTL_BASE, 13, int)
> > > > +#define	WDIOC_GETPREACTION	_IOR(WATCHDOG_IOCTL_BASE, 14, int)
> > > >   #define	WDIOF_UNKNOWN		-1	/* Unknown flag error */
> > > >   #define	WDIOS_UNKNOWN		-1	/* Unknown status error */
> > > > @@ -54,5 +58,15 @@ struct watchdog_info {
> > > >   #define	WDIOS_ENABLECARD	0x0002	/* Turn on the watchdog timer */
> > > >   #define	WDIOS_TEMPPANIC		0x0004	/* Kernel panic on temperature trip */
> > > > +/* Actions for WDIOC_xxxACTION ioctls. */
> > > > +#define WDIOA_RESET		0	/* Reset the system. */
> > > > +#define WDIOA_POWER_OFF		1	/* Power off the system. */
> > > > +#define WDIOA_POWER_CYCLE	2	/* Power cycle the system. */
> > > > +
> > > > +/* Actions for WDIOC_xxxPREACTION ioctls. */
> > > > +#define WDIOP_NONE		0	/* Do nothing. */
> > > > +#define WDIOP_NMI		1	/* Issue an NMI. */
> > > > +#define WDIOP_SMI		2	/* Issue a system management irq. */
> > > > +#define WDIOP_INTERRUPT		3	/* Issue a normal irq. */
> > > >   #endif /* _UAPI_LINUX_WATCHDOG_H */
> > > > -- 
> > > > 2.17.1
> > > > 
> > 
> 

[PATCH 09/12] watchdog:ipmi: Implement action and preaction functions

[minyard]

From: Corey Minyard <cminyard@mvista.com>

This required cleanup of how actions and preactions were done.  Setting
the values through the sysfs interface still works, though the sysfs
parameters it will not display changes that come in through the
standard watchdog interface.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 drivers/watchdog/ipmi_watchdog.c | 365 +++++++++++++++++++++----------
 1 file changed, 246 insertions(+), 119 deletions(-)

diff --git a/drivers/watchdog/ipmi_watchdog.c b/drivers/watchdog/ipmi_watchdog.c
index 218c01707c0b..77dbd3851fdc 100644
--- a/drivers/watchdog/ipmi_watchdog.c
+++ b/drivers/watchdog/ipmi_watchdog.c
@@ -80,12 +80,6 @@
 #define WDOG_PRETIMEOUT_NMI		2
 #define WDOG_PRETIMEOUT_MSG_INT		3
 
-/* Operations that can be performed on a pretimout. */
-#define WDOG_PREOP_NONE		0
-#define WDOG_PREOP_PANIC	1
-/* Cause data to be available to read.  Doesn't work in NMI mode. */
-#define WDOG_PREOP_GIVE_DATA	2
-
 /* Actions to perform on a full timeout. */
 #define WDOG_SET_TIMEOUT_ACT(byte, use) \
 	byte = ((byte) & 0xf8) | ((use) & 0x7)
@@ -144,6 +138,7 @@ struct ipmi_wdt {
 	struct ipmi_recv_msg recv_msg;
 
 	int state; /* One of WDOG_TIMEOUT_xxx, NONE if disabled. */
+	int prestate;  /* One of WDOG_PRETIMEOUT_xxx. */
 
 	bool nmi_works;
 
@@ -164,6 +159,8 @@ static int ipmi_set_timeout(struct ipmi_wdt *iwd, int timeout,
 			    int do_heartbeat);
 static int ipmi_set_pretimeout(struct ipmi_wdt *iwd, int timeout,
 			       int do_heartbeat);
+static int ipmi_set_action(struct ipmi_wdt *iwd, unsigned int action);
+static int ipmi_set_preaction(struct ipmi_wdt *iwd, unsigned int action);
 static void ipmi_register_watchdog(int ipmi_intf, struct device *dev);
 static void _ipmi_unregister_watchdog(struct ipmi_wdt *iwd);
 static void ipmi_unregister_watchdog(int ipmi_intf);
@@ -189,18 +186,14 @@ static int timeout = 10; /* Default timeout. */
 static int pretimeout; /* Default pretimeout. */
 static int panic_wdt_timeout = 255; /* Default timeout to set on panic */
 
-/* Default action is to reset the board on a timeout. */
-static unsigned char action_val = WDOG_TIMEOUT_RESET;
-
-static char action[16] = "reset";
-
-static unsigned char preaction_val = WDOG_PRETIMEOUT_NONE;
-
-static char preaction[16] = "pre_none";
-
-static unsigned char preop_val = WDOG_PREOP_NONE;
+#define WDOG_PREOP_NONE		0
+#define WDOG_PREOP_PANIC	1
+#define WDOG_PREOP_GIVE_DATA	2
 
-static char preop[16] = "preop_none";
+/* Default action is to reset the board on a timeout. */
+static unsigned char def_action_val = WDIOA_RESET;
+static unsigned char def_preaction_val = WDIOP_NONE;
+static unsigned char def_preop_val = WDOG_PREOP_NONE;
 
 /*
  * If true, the driver will start running as soon as it is configured
@@ -211,33 +204,41 @@ static int start_now;
 static int action_op(const char *inval, char *outval)
 {
 	int rv = 0;
+	unsigned int new_val;
 
 	mutex_lock(&ipmi_wdt_data_mutex);
-	if (outval)
-		strcpy(outval, action);
+	if (outval) {
+		switch (def_action_val) {
+		case WDIOA_RESET:
+			strcpy(outval, "reset");
+			break;
+		case WDIOA_POWER_OFF:
+			strcpy(outval, "power_off");
+			break;
+		case WDIOA_POWER_CYCLE:
+			strcpy(outval, "power_cycle");
+			break;
+		default:
+			strcpy(outval, "?");
+			break;
+		}
+	}
 
 	if (!inval)
 		goto out_unlock;
 
 	if (strcmp(inval, "reset") == 0)
-		action_val = WDOG_TIMEOUT_RESET;
-	else if (strcmp(inval, "none") == 0)
-		action_val = WDOG_TIMEOUT_NONE;
+		new_val = WDIOA_RESET;
 	else if (strcmp(inval, "power_cycle") == 0)
-		action_val = WDOG_TIMEOUT_POWER_CYCLE;
+		new_val = WDIOA_POWER_CYCLE;
 	else if (strcmp(inval, "power_off") == 0)
-		action_val = WDOG_TIMEOUT_POWER_DOWN;
+		new_val = WDIOA_POWER_OFF;
 	else
 		rv = -EINVAL;
-	if (!rv) {
-		strcpy(action, inval);
-		if (ipmi_wdt && ipmi_wdt->state != WDOG_TIMEOUT_NONE) {
-			ipmi_wdt->state = action_val;
-			rv = _ipmi_update_timeout(ipmi_wdt,
-					IPMI_SET_TIMEOUT_HB_IF_NECESSARY,
-					false);
-		}
-	}
+	if (!rv && ipmi_wdt)
+		rv = ipmi_set_action(ipmi_wdt, new_val);
+	if (!rv)
+		def_action_val = new_val;
 out_unlock:
 	mutex_unlock(&ipmi_wdt_data_mutex);
 
@@ -247,84 +248,115 @@ static int action_op(const char *inval, char *outval)
 static int preaction_op(const char *inval, char *outval)
 {
 	int rv = 0;
+	unsigned int new_val;
 
 	mutex_lock(&ipmi_wdt_data_mutex);
-	if (outval)
-		strcpy(outval, preaction);
+	if (outval) {
+		switch (def_preaction_val) {
+		case WDIOP_NONE:
+			strcpy(outval, "pre_none");
+			break;
+		case WDIOP_NMI:
+			strcpy(outval, "pre_nmi");
+			break;
+		case WDIOP_SMI:
+			strcpy(outval, "pre_smi");
+			break;
+		case WDIOP_INTERRUPT:
+			strcpy(outval, "pre_int");
+			break;
+		default:
+			strcpy(outval, "?");
+			break;
+		}
+	}
 
 	if (!inval)
 		goto out_unlock;
 
-	if (strcmp(inval, "pre_none") == 0)
-		preaction_val = WDOG_PRETIMEOUT_NONE;
-	else if (strcmp(inval, "pre_smi") == 0)
-		preaction_val = WDOG_PRETIMEOUT_SMI;
+	if (strcmp(inval, "pre_none") == 0) {
+		new_val = WDIOP_NONE;
+	} else if (strcmp(inval, "pre_smi") == 0) {
+		new_val = WDIOP_SMI;
 #ifdef HAVE_DIE_NMI
-	else if (strcmp(inval, "pre_nmi") == 0) {
-		if (preop_val == WDOG_PREOP_GIVE_DATA)
-			rv = -EINVAL;
-		else if (ipmi_wdt && !ipmi_wdt->nmi_works)
-			rv = -EINVAL;
-		else
-			preaction_val = WDOG_PRETIMEOUT_NMI;
-	}
+	} else if (strcmp(inval, "pre_nmi") == 0) {
+		new_val = WDIOP_NMI;
 #endif
-	else if (strcmp(inval, "pre_int") == 0)
-		preaction_val = WDOG_PRETIMEOUT_MSG_INT;
-	else
+	} else if (strcmp(inval, "pre_int") == 0) {
+		new_val = WDIOP_INTERRUPT;
+	} else {
 		rv = -EINVAL;
-	if (!rv) {
-		strcpy(preaction, inval);
-		if (ipmi_wdt && ipmi_wdt->state != WDOG_TIMEOUT_NONE &&
-		    ipmi_wdt->wdd.pretimeout) {
-			rv = _ipmi_update_timeout(ipmi_wdt,
-					IPMI_SET_TIMEOUT_HB_IF_NECESSARY,
-					false);
-		}
 	}
+	if (!rv && ipmi_wdt)
+		rv = ipmi_set_preaction(ipmi_wdt, new_val);
+	if (!rv)
+		def_preaction_val = new_val;
 out_unlock:
 	mutex_unlock(&ipmi_wdt_data_mutex);
 
 	return rv;
 }
 
+static const char *preop_to_governor(unsigned int preop)
+{
+	switch (preop) {
+	case WDOG_PREOP_NONE:
+		return "noop";
+	case WDOG_PREOP_PANIC:
+		return "panic";
+	case WDOG_PREOP_GIVE_DATA:
+		return "read_data";
+	default:
+		return NULL;
+	}
+}
 static int preop_op(const char *inval, char *outval)
 {
 	int rv = 0;
 	const char *gov = NULL;
-	unsigned int orig_val;
+	unsigned int new_val;
 
 	mutex_lock(&ipmi_wdt_data_mutex);
-	if (outval)
-		strcpy(outval, preop);
+	if (outval) {
+		switch (def_preop_val) {
+		case WDOG_PREOP_NONE:
+			strcpy(outval, "preop_none");
+			break;
+		case WDOG_PREOP_PANIC:
+			strcpy(outval, "preop_panic");
+			break;
+		case WDOG_PREOP_GIVE_DATA:
+			strcpy(outval, "preop_give_data");
+			break;
+		default:
+			strcpy(outval, "?");
+			break;
+		}
+	}
 
 	if (!inval)
 		goto out_unlock;
 
-	orig_val = preop_val;
 	if (strcmp(inval, "preop_none") == 0) {
-		preop_val = WDOG_PREOP_NONE;
-		gov = "noop";
+		new_val = WDOG_PREOP_NONE;
 	} else if (strcmp(inval, "preop_panic") == 0) {
-		preop_val = WDOG_PREOP_PANIC;
-		gov = "panic";
+		new_val = WDOG_PREOP_PANIC;
 	} else if (strcmp(inval, "preop_give_data") == 0) {
-		if (preaction_val == WDOG_PRETIMEOUT_NMI)
-			rv = -EINVAL;
-		else {
-			preop_val = WDOG_PREOP_GIVE_DATA;
-			gov = "read_data";
-		}
+		new_val = WDOG_PREOP_GIVE_DATA;
 	} else {
 		rv = -EINVAL;
 	}
-	if (!rv) {
-		rv = watchdog_pretimeout_governor_set(&ipmi_wdt->wdd, gov);
-		if (rv)
-			preaction_val = orig_val;
-		else
-			strcpy(preop, inval);
+	if (!rv && ipmi_wdt) {
+		gov = preop_to_governor(new_val);
+		if (!gov)
+			rv = -EINVAL;
+		if (!rv) {
+			rv = watchdog_pretimeout_governor_set(&ipmi_wdt->wdd,
+							      gov);
+		}
 	}
+	if (!rv)
+		def_preop_val = new_val;
 out_unlock:
 	mutex_unlock(&ipmi_wdt_data_mutex);
 
@@ -551,7 +583,7 @@ static int __ipmi_update_timeout(struct ipmi_wdt *iwd, int *send_heartbeat_now)
 	WDOG_SET_TIMEOUT_ACT(data[1], iwd->state);
 	if ((iwd->wdd.pretimeout > 0) &&
 	    (iwd->state != WDOG_TIMEOUT_NONE)) {
-		WDOG_SET_PRETIMEOUT_ACT(data[1], preaction_val);
+		WDOG_SET_PRETIMEOUT_ACT(data[1], iwd->prestate);
 		data[2] = iwd->wdd.pretimeout;
 	} else {
 		WDOG_SET_PRETIMEOUT_ACT(data[1], WDOG_PRETIMEOUT_NONE);
@@ -863,19 +895,36 @@ static void ipmi_wdog_panic_handler(void *user_data)
 	}
 }
 
-static int ipmi_wdt_start(struct watchdog_device *wdd)
+static int _ipmi_wdt_start(struct ipmi_wdt *iwd)
 {
-	struct ipmi_wdt *iwd = wdd_to_ipmi_wdt(wdd);
-	int rv;
+	int rv = 0;
 
-	mutex_lock(&iwd->lock);
-	iwd->state = action_val;
-	rv = _ipmi_update_timeout(iwd, IPMI_SET_TIMEOUT_FORCE_HB, false);
-	mutex_unlock(&iwd->lock);
+	switch (iwd->wdd.action) {
+	case WDIOA_RESET:
+		iwd->state = WDOG_TIMEOUT_RESET;
+		break;
+	case WDIOA_POWER_OFF:
+		iwd->state = WDOG_TIMEOUT_RESET;
+		break;
+	case WDIOA_POWER_CYCLE:
+		iwd->state = WDOG_TIMEOUT_RESET;
+		break;
+	default:
+		rv = -EINVAL;
+		break;
+	}
+	if (!rv)
+		rv = _ipmi_update_timeout(iwd, IPMI_SET_TIMEOUT_FORCE_HB,
+					  false);
 
 	return rv;
 }
 
+static int ipmi_wdt_start(struct watchdog_device *wdd)
+{
+	return _ipmi_wdt_start(wdd_to_ipmi_wdt(wdd));
+}
+
 static int ipmi_wdt_stop(struct watchdog_device *wdd)
 {
 	struct ipmi_wdt *iwd = wdd_to_ipmi_wdt(wdd);
@@ -903,25 +952,101 @@ static int ipmi_wdt_ping(struct watchdog_device *wdd)
 
 static int ipmi_wdt_set_timeout(struct watchdog_device *wdd, unsigned int val)
 {
-	struct ipmi_wdt *iwd = wdd_to_ipmi_wdt(wdd);
-
-	return ipmi_set_timeout(iwd, val, IPMI_SET_TIMEOUT_HB_IF_NECESSARY);
+	return ipmi_set_timeout(wdd_to_ipmi_wdt(wdd), val,
+				IPMI_SET_TIMEOUT_HB_IF_NECESSARY);
 }
 
 static int ipmi_wdt_set_pretimeout(struct watchdog_device *wdd,
 				   unsigned int val)
 {
 	struct ipmi_wdt *iwd = wdd_to_ipmi_wdt(wdd);
-	int rv;
+
+	return ipmi_set_pretimeout(iwd, val, IPMI_SET_TIMEOUT_HB_IF_NECESSARY);
+}
+
+static int ipmi_set_action(struct ipmi_wdt *iwd, unsigned int val)
+{
+	int rv = 0;
 
 	mutex_lock(&iwd->lock);
-	iwd->wdd.pretimeout = val;
-	rv = _ipmi_update_timeout(iwd, IPMI_SET_TIMEOUT_NO_HB, false);
+	if (val == iwd->wdd.action)
+		goto out_unlock;
+	switch (val) {
+	case WDIOA_RESET:
+	case WDIOA_POWER_OFF:
+	case WDIOA_POWER_CYCLE:
+		iwd->wdd.action = val;
+		break;
+	default:
+		rv = -EINVAL;
+	}
+	if (!rv)
+		rv = _ipmi_wdt_start(iwd);
+out_unlock:
 	mutex_unlock(&iwd->lock);
 
 	return rv;
 }
 
+static int ipmi_wdt_set_action(struct watchdog_device *wdd,
+			       unsigned int val)
+{
+	return ipmi_set_action(wdd_to_ipmi_wdt(wdd), val);
+}
+
+static int ipmi_set_preaction(struct ipmi_wdt *iwd, unsigned int val)
+{
+	int rv = 0;
+	int new_state = 0;
+
+	mutex_lock(&iwd->lock);
+	if (val == iwd->wdd.preaction)
+		goto out_unlock;
+	switch (val) {
+	case WDIOP_NONE:
+		new_state = WDOG_PRETIMEOUT_NONE;
+		break;
+	case WDIOP_SMI:
+		new_state = WDOG_PRETIMEOUT_SMI;
+		break;
+	case WDIOP_INTERRUPT:
+		new_state = WDOG_PRETIMEOUT_MSG_INT;
+		break;
+#ifdef HAVE_DIE_NMI
+	case WDIOP_NMI:
+		if (!iwd->nmi_works)
+			rv = -EINVAL;
+		else
+			new_state = WDOG_PRETIMEOUT_NMI;
+		break;
+#endif
+	default:
+		rv = -EINVAL;
+	}
+	if (!rv) {
+		int old_preaction = iwd->wdd.preaction;
+		int old_prestate = iwd->prestate;
+
+		iwd->wdd.preaction = val;
+		iwd->prestate = new_state;
+		rv = _ipmi_wdt_start(iwd);
+		if (rv) {
+			iwd->wdd.preaction = old_preaction;
+			iwd->prestate = old_prestate;
+		}
+	}
+out_unlock:
+	mutex_unlock(&iwd->lock);
+
+	return rv;
+}
+
+static int ipmi_wdt_set_preaction(struct watchdog_device *wdd,
+				  unsigned int val)
+{
+	return ipmi_set_preaction(wdd_to_ipmi_wdt(wdd), val);
+}
+
 static const struct watchdog_info ipmi_wdt_ident = {
 	.options	= WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT |
 			  WDIOF_PRETIMEOUT | WDIOF_MAGICCLOSE,
@@ -936,7 +1061,9 @@ static const struct watchdog_ops ipmi_wdt_ops = {
 	.ping		= ipmi_wdt_ping,
 	.set_timeout	= ipmi_wdt_set_timeout,
 	.set_pretimeout = ipmi_wdt_set_pretimeout,
-	.get_timeleft   = ipmi_wdt_get_timeleft,
+	.get_timeleft	= ipmi_wdt_get_timeleft,
+	.set_action	= ipmi_wdt_set_action,
+	.set_preaction	= ipmi_wdt_set_preaction,
 };
 
 static const struct ipmi_user_hndl ipmi_hndlrs = {
@@ -976,14 +1103,14 @@ ipmi_nmi(unsigned int val, struct pt_regs *regs)
 	if (iwd->state == WDOG_TIMEOUT_NONE)
 		return NMI_DONE;
 
-	if (preaction_val != WDOG_PRETIMEOUT_NMI)
+	if (iwd->wdd.preaction != WDIOP_NMI)
 		return NMI_DONE;
 
 	/*
 	 * If no one else handled the NMI, we assume it was the IPMI
 	 * watchdog.
 	 */
-	if (preop_val == WDOG_PREOP_PANIC) {
+	if (iwd->wdd.gov && strcmp(iwd->wdd.gov->name, "panic") == 0) {
 		/*
 		 * On some machines, the heartbeat will give an error
 		 * and not work unless we re-enable the timer.  So
@@ -1000,6 +1127,7 @@ ipmi_nmi(unsigned int val, struct pt_regs *regs)
 static void nmi_check(struct ipmi_wdt *iwd)
 {
 	int old_state = iwd->state;
+	int old_prestate = iwd->prestate;
 	int old_pretimeout = iwd->wdd.pretimeout;
 	int old_timeout = iwd->wdd.timeout;
 	int rv;
@@ -1026,6 +1154,7 @@ static void nmi_check(struct ipmi_wdt *iwd)
 	 * ourselves 99 seconds to stop the timer.
 	 */
 	iwd->state = WDOG_TIMEOUT_RESET;
+	iwd->prestate = WDOG_PRETIMEOUT_NMI;
 	iwd->wdd.pretimeout = 99;
 	iwd->wdd.timeout = 100;
 
@@ -1063,6 +1192,7 @@ static void nmi_check(struct ipmi_wdt *iwd)
 	mutex_unlock(&ipmi_nmi_test_mutex);
 	iwd->wdd.pretimeout = old_pretimeout;
 	iwd->wdd.timeout = old_timeout;
+	iwd->prestate = old_prestate;
 	iwd->state = old_state;
 	rv = _ipmi_update_timeout(iwd, IPMI_SET_TIMEOUT_FORCE_HB, false);
 	if (rv)
@@ -1079,6 +1209,7 @@ static void ipmi_register_watchdog(int ipmi_intf, struct device *dev)
 {
 	struct ipmi_wdt *iwd = NULL;
 	int rv = -EBUSY;
+	const char *gov;
 
 	mutex_lock(&ipmi_wdt_data_mutex);
 	if ((ifnum_to_use != -1) && (ifnum_to_use != ipmi_intf))
@@ -1126,6 +1257,8 @@ static void ipmi_register_watchdog(int ipmi_intf, struct device *dev)
 	watchdog_init_timeout(&iwd->wdd, timeout, NULL);
 	iwd->wdd.pretimeout = pretimeout;
 	iwd->wdd.parent = dev;
+	iwd->wdd.action = def_action_val;
+	iwd->wdd.preaction = def_preaction_val;
 
 	rv = watchdog_register_device(&iwd->wdd);
 	if (rv) {
@@ -1136,24 +1269,34 @@ static void ipmi_register_watchdog(int ipmi_intf, struct device *dev)
 
 	nmi_check(iwd);
 
-	if (preaction_val == WDOG_PRETIMEOUT_NMI && !iwd->nmi_works) {
+	if (iwd->wdd.preaction == WDIOP_NMI && !iwd->nmi_works) {
 		pr_warn("NMI pretimeout test failed, disabling pretimeout.\n");
-		preaction_val = WDOG_PRETIMEOUT_NONE;
+		iwd->wdd.preaction = WDIOP_NONE;
+	}
+
+	gov = preop_to_governor(def_preop_val);
+	if (!gov) {
+		pr_warn("Invalid preop value: %d\n", def_preop_val);
+	} else {
+		rv = watchdog_pretimeout_governor_set(&iwd->wdd, gov);
+		if (rv)
+			pr_warn("Setting governor to %s failed: %d\n",
+				gov, rv);
 	}
 
 	ipmi_wdt = iwd;
+	rv = 0;
 
  out:
 	mutex_unlock(&ipmi_wdt_data_mutex);
 	if (start_now && rv == 0) {
 		/* Run from startup, so start the timer now. */
 		start_now = 0; /* Disable this function after first startup. */
-		iwd->state = action_val;
-		mutex_lock(&iwd->lock);
-		_ipmi_update_timeout(iwd, IPMI_SET_TIMEOUT_FORCE_HB, false);
-		mutex_unlock(&iwd->lock);
-		pr_info("Starting now!\n");
-	} else if (iwd && rv != 0) {
+		if (_ipmi_wdt_start(iwd))
+			pr_warn("Starting now failed!\n");
+		else
+			pr_info("Starting now!\n");
+	} else if (iwd && rv) {
 		kfree(iwd);
 	}
 }
@@ -1236,22 +1379,6 @@ static int __init ipmi_wdog_init(void)
 {
 	int rv;
 
-	if (action_op(action, NULL)) {
-		pr_info("Unknown action '%s', defaulting to reset\n", action);
-		action_op("reset", NULL);
-	}
-
-	if (preaction_op(preaction, NULL)) {
-		pr_info("Unknown preaction '%s', defaulting to none\n",
-			preaction);
-		preaction_op("pre_none", NULL);
-	}
-
-	if (preop_op(preop, NULL)) {
-		pr_info("Unknown preop '%s', defaulting to none\n", preop);
-		preop_op("preop_none", NULL);
-	}
-
 	register_reboot_notifier(&wdog_reboot_notifier);
 
 	rv = ipmi_smi_watcher_register(&smi_watcher);
-- 
2.17.1

[PATCH 10/12] watchdog: Add a way to set the governor through the ioctl

[minyard]

From: Corey Minyard <cminyard@mvista.com>

The watchdog governor could only be set through the systfs interface
before, make that function available through the ioctl, too.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 Documentation/watchdog/watchdog-api.rst | 31 +++++++++++++++++++++++++
 drivers/watchdog/watchdog_dev.c         | 14 +++++++++++
 drivers/watchdog/watchdog_pretimeout.h  |  2 +-
 include/uapi/linux/watchdog.h           |  8 +++++++
 4 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/Documentation/watchdog/watchdog-api.rst b/Documentation/watchdog/watchdog-api.rst
index 927be9e56b5d..a2a3057e2b5d 100644
--- a/Documentation/watchdog/watchdog-api.rst
+++ b/Documentation/watchdog/watchdog-api.rst
@@ -177,6 +177,37 @@ and queried::
 Note that the pretimeout governor that reads data is not compatible with
 the NMI preaction.  The NMI preaction can only do nothing or panic.
 
+Pretimeout Governors
+====================
+
+When a pretimeout occurs and the pretimeout framework is compiled in
+to the kernel, the pretimeout framework will generally be called.
+(The exception is that NMI pretimeouts do not call the pretimeout
+framework because they need special handling.)  Several pretimeout
+governers can be registered::
+
+    noop - Don't do anything on a pretimeout
+    panic - Issue a panic when a pretimeout occurs.  This is generally the
+            default
+    read_data - Provide one byte of data on the read interface to the
+                watchdog timer.  This way a userland program can handle
+		the pretimeout.
+
+If the CONFING_WATCHDOG_SYSFS is enabled, the pretimeout governor can
+be set by writing the value to the
+/sys/class/watchdog/watchdog<n>/pretimeout_governor sysfs file.
+
+The pretimeout governor can also be set through the ioctl interface with::
+
+    char governor[WATCHDOG_GOV_NAME_MAXLEN] = "panic";
+    ioctl(fd, WDIOC_SETPREGOV, gov);
+
+and can be queried with::
+
+    char governor[WATCHDOG_GOV_NAME_MAXLEN];
+    ioctl(fd, WDIOC_GETPREGOV, gov);
+    printf("The governor is %s\n", gov);
+
 Get the number of seconds before reboot
 =======================================
 
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 0e70f510a491..7e1ad6e303d0 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -762,6 +762,7 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
 	int __user *p = argp;
 	unsigned int val;
 	int err;
+	char gov[WATCHDOG_GOV_NAME_MAXLEN];
 
 	mutex_lock(&wd_data->lock);
 
@@ -866,6 +867,19 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
 	case WDIOC_GETPREACTION:
 		err = put_user(wdd->preaction, p);
 		break;
+	case WDIOC_SETPREGOV:
+		err = strncpy_from_user(gov, argp, sizeof(gov));
+		if (err >= 0)
+			err = watchdog_pretimeout_governor_set(wdd, gov);
+		break;
+	case WDIOC_GETPREGOV:
+		err = 0;
+		val = watchdog_pretimeout_governor_get(wdd, gov);
+		if (val == 0)
+			err = -ENOENT;
+		if (copy_to_user(argp, gov, val + 1))
+			err = -EFAULT;
+		break;
 	default:
 		err = -ENOTTY;
 		break;
diff --git a/drivers/watchdog/watchdog_pretimeout.h b/drivers/watchdog/watchdog_pretimeout.h
index 819517ed0138..e6e191b787bb 100644
--- a/drivers/watchdog/watchdog_pretimeout.h
+++ b/drivers/watchdog/watchdog_pretimeout.h
@@ -2,7 +2,7 @@
 #ifndef __WATCHDOG_PRETIMEOUT_H
 #define __WATCHDOG_PRETIMEOUT_H
 
-#define WATCHDOG_GOV_NAME_MAXLEN	20
+#include <uapi/linux/watchdog.h>
 
 struct watchdog_device;
 
diff --git a/include/uapi/linux/watchdog.h b/include/uapi/linux/watchdog.h
index bf13cf25f9e0..2acbfff6db8c 100644
--- a/include/uapi/linux/watchdog.h
+++ b/include/uapi/linux/watchdog.h
@@ -36,10 +36,18 @@ struct watchdog_info {
 #define	WDIOC_GETACTION		_IOR(WATCHDOG_IOCTL_BASE, 12, int)
 #define	WDIOC_SETPREACTION	_IOWR(WATCHDOG_IOCTL_BASE, 13, int)
 #define	WDIOC_GETPREACTION	_IOR(WATCHDOG_IOCTL_BASE, 14, int)
+#define	WDIOC_SETPREGOV		_IOWR(WATCHDOG_IOCTL_BASE, 15, char)
+#define	WDIOC_GETPREGOV		_IOR(WATCHDOG_IOCTL_BASE, 16, char)
 
 #define	WDIOF_UNKNOWN		-1	/* Unknown flag error */
 #define	WDIOS_UNKNOWN		-1	/* Unknown status error */
 
+/*
+ * Buffer for WDIOC_GETPREGOV must be at least this big.  WDIOC_SETPRGOV
+ * will take at max this many bytes - 1, excess will be ignored.
+ */
+#define WATCHDOG_GOV_NAME_MAXLEN	20
+
 #define	WDIOF_OVERHEAT		0x0001	/* Reset due to CPU overheat */
 #define	WDIOF_FANFAULT		0x0002	/* Fan failed */
 #define	WDIOF_EXTERN1		0x0004	/* External relay 1 */
-- 
2.17.1

[PATCH 11/12] watchdog: Add a sample program that can fully use the watchdog interface

[minyard]

From: Corey Minyard <cminyard@mvista.com>

This is useful for testing.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 samples/watchdog/Makefile       |   2 +-
 samples/watchdog/watchdog-set.c | 580 ++++++++++++++++++++++++++++++++
 2 files changed, 581 insertions(+), 1 deletion(-)
 create mode 100644 samples/watchdog/watchdog-set.c

diff --git a/samples/watchdog/Makefile b/samples/watchdog/Makefile
index a9430fa60253..3cf63106f3ef 100644
--- a/samples/watchdog/Makefile
+++ b/samples/watchdog/Makefile
@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 CC := $(CROSS_COMPILE)gcc
-PROGS := watchdog-simple
+PROGS := watchdog-simple watchdog-set
 
 all: $(PROGS)
 
diff --git a/samples/watchdog/watchdog-set.c b/samples/watchdog/watchdog-set.c
new file mode 100644
index 000000000000..ba0457157a16
--- /dev/null
+++ b/samples/watchdog/watchdog-set.c
@@ -0,0 +1,580 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <fcntl.h>
+#include <string.h>
+#include <errno.h>
+#include <sys/ioctl.h>
+#include <linux/watchdog.h>
+
+/* In case we are compiling with older kernel include files. */
+#ifndef WDIOC_SETACTION
+#define	WDIOC_SETACTION		_IOWR(WATCHDOG_IOCTL_BASE, 11, int)
+#define	WDIOC_GETACTION		_IOR(WATCHDOG_IOCTL_BASE, 12, int)
+#define	WDIOC_SETPREACTION	_IOWR(WATCHDOG_IOCTL_BASE, 13, int)
+#define	WDIOC_GETPREACTION	_IOR(WATCHDOG_IOCTL_BASE, 14, int)
+#define	WDIOC_SETPREGOV		_IOWR(WATCHDOG_IOCTL_BASE, 15, char)
+#define	WDIOC_GETPREGOV		_IOR(WATCHDOG_IOCTL_BASE, 16, char)
+
+/*
+ * Buffer for WDIOC_GETPREGOV must be at least this big.  WDIOC_SETPRGOV
+ * will take at max this many bytes - 1, excess will be ignored.
+ */
+#define WATCHDOG_GOV_NAME_MAXLEN	20
+
+/* Actions for WDIOC_xxxACTION ioctls. */
+#define WDIOA_RESET		0	/* Reset the system. */
+#define WDIOA_POWER_OFF		1	/* Power off the system. */
+#define WDIOA_POWER_CYCLE	2	/* Power cycle the system. */
+
+/* Actions for WDIOC_xxxPREACTION ioctls. */
+#define WDIOP_NONE		0	/* Do nothing. */
+#define WDIOP_NMI		1	/* Issue an NMI. */
+#define WDIOP_SMI		2	/* Issue a system management irq. */
+#define WDIOP_INTERRUPT		3	/* Issue a normal irq. */
+#endif
+
+struct bitflags {
+	int flag;
+	char *name;
+};
+
+static struct bitflags flags[] = {
+	{ WDIOF_OVERHEAT,	"overheat" },
+	{ WDIOF_FANFAULT,	"fanfault" },
+	{ WDIOF_EXTERN1,	"extern1" },
+	{ WDIOF_EXTERN2,	"extern2" },
+	{ WDIOF_POWERUNDER,	"powerunder" },
+	{ WDIOF_CARDRESET,	"cardreset" },
+	{ WDIOF_POWEROVER,	"powerover" },
+	{ WDIOF_SETTIMEOUT,	"settimeout" },
+	{ WDIOF_MAGICCLOSE,	"magicclose" },
+	{ WDIOF_PRETIMEOUT,	"pretimeout" },
+	{ WDIOF_ALARMONLY,	"alarmonly" },
+	{ WDIOF_KEEPALIVEPING,	"keepaliveping" },
+	{ }
+};
+
+static struct bitflags options[] = {
+	{ WDIOS_DISABLECARD,	"disablecard" },
+	{ WDIOS_ENABLECARD,	"enablecard" },
+	{ WDIOS_TEMPPANIC,	"temppanic" },
+	{ }
+};
+
+struct actionvals {
+	int action;
+	char *name;
+};
+
+static struct actionvals actions[] = {
+	{ WDIOA_RESET,		"reset" },
+	{ WDIOA_POWER_OFF,	"poweroff" },
+	{ WDIOA_POWER_CYCLE,	"powercycle" },
+	{ }
+};
+
+static struct actionvals preactions[] = {
+	{ WDIOP_NONE,		"none" },
+	{ WDIOP_NMI,		"nmi" },
+	{ WDIOP_SMI,		"smi" },
+	{ WDIOP_INTERRUPT,	"interrupt" },
+	{ }
+};
+
+static void print_bits(int bitmask, struct bitflags *flags)
+{
+	unsigned int i;
+
+	for (i = 0; flags[i].name; i++) {
+		if (flags[i].flag & bitmask) {
+			bitmask &= ~flags[i].flag;
+			printf(" %s", flags[i].name);
+		}
+	}
+	i = 0;
+	while (bitmask) {
+		while (!(bitmask & (1 << i)))
+			i++;
+		printf(" bit(%d)", i);
+		bitmask &= ~(1 << i);
+	}
+}
+
+static void print_action(int val, struct actionvals *actions)
+{
+	unsigned int i;
+
+	for (i = 0; actions[i].name; i++) {
+		if (val == actions[i].action) {
+			printf("%s\n", actions[i].name);
+			return;
+		}
+	}
+	printf("%d\n", val);
+}
+
+static int action_to_val(char *action, struct actionvals *actions)
+{
+	unsigned int i;
+	int val;
+	char *end;
+
+	for (i = 0; actions[i].name; i++) {
+		if (strcmp(action, actions[i].name) == 0)
+			return actions[i].action;
+	}
+
+	val = strtoul(action, &end, 0);
+	if (end == action || *end != '\0')
+		return -1;
+
+	return val;
+}
+
+static int status(int wdfd, int argc, char *argv[])
+{
+	struct watchdog_info info;
+	int val;
+	int ret;
+	char gov[WATCHDOG_GOV_NAME_MAXLEN];
+
+	printf("info:");
+	ret = ioctl(wdfd, WDIOC_GETSUPPORT, &info);
+	if (ret == -1) {
+		printf(" error:%s\n", strerror(errno));
+	} else {
+		printf("\n  options:");
+		print_bits(info.options, flags);
+		printf("\n  fwver: %d (0x%x)", info.firmware_version,
+		       info.firmware_version);
+		printf("\n  identity: %s\n", info.identity);
+	}
+
+	printf("status:");
+	ret = ioctl(wdfd, WDIOC_GETSTATUS, &val);
+	if (ret == -1) {
+		printf(" error:%s\n", strerror(errno));
+	} else {
+		print_bits(val, flags);
+		printf("\n");
+	}
+
+	printf("bootstatus:");
+	ret = ioctl(wdfd, WDIOC_GETBOOTSTATUS, &val);
+	if (ret == -1) {
+		printf(" error:%s\n", strerror(errno));
+	} else {
+		print_bits(val, flags);
+		printf("\n");
+	}
+
+	ret = ioctl(wdfd, WDIOC_GETTEMP, &val);
+	if (ret != -1) /* Not usually implemented. */
+		printf("temp: %d\n", val);
+
+	ret = ioctl(wdfd, WDIOC_GETTIMEOUT, &val);
+	if (ret == -1)
+		printf("timeout: error:%s\n", strerror(errno));
+	else
+		printf("timeout: %d\n", val);
+
+	ret = ioctl(wdfd, WDIOC_GETPRETIMEOUT, &val);
+	if (ret == -1)
+		printf("pretimeout: error:%s\n", strerror(errno));
+	else
+		printf("pretimeout: %d\n", val);
+
+	ret = ioctl(wdfd, WDIOC_GETACTION, &val);
+	if (ret == -1) {
+		if (errno != ENOTTY) /* If not an older kernel. */
+			printf("action: error:%s\n", strerror(errno));
+	} else {
+		printf("action: ");
+		print_action(val, actions);
+	}
+
+	ret = ioctl(wdfd, WDIOC_GETPREACTION, &val);
+	if (ret == -1) {
+		if (errno != ENOTTY) /* If not an older kernel. */
+			printf("preaction: error:%s\n", strerror(errno));
+	} else {
+		printf("preaction: ");
+		print_action(val, preactions);
+	}
+
+	ret = ioctl(wdfd, WDIOC_GETPREGOV, gov);
+	if (ret == -1) {
+		if (errno != ENOTTY) /* If not an older kernel. */
+			printf("governor: error:%s\n", strerror(errno));
+	} else {
+		printf("governor: %s\n", gov);
+	}
+
+	return 0;
+}
+
+static int setoptions(int wdfd, int argc, char *argv[])
+{
+	int ret, val = 0;
+	int i;
+	unsigned int j;
+	char *end;
+
+	for (i = 0; i < argc; ) {
+		for (j = 0; options[j].name; j++) {
+			if (strcmp(argv[i], options[j].name) == 0) {
+				val |= options[j].flag;
+				goto found;
+			}
+		}
+		val |= strtoul(argv[i], &end, 0);
+		if (end == argv[i] || *end != '\0') {
+			fprintf(stderr, "invalid option: '%s'\n", argv[i]);
+			return 1;
+		}
+found:
+		i++;
+	}
+
+	ret = ioctl(wdfd, WDIOC_SETOPTIONS, &val);
+	if (ret == -1) {
+		fprintf(stderr, "Set options error: %s\n", strerror(errno));
+		return 1;
+	}
+
+	return 0;
+}
+
+static int ping(int wdfd, int argc, char *argv[])
+{
+	int ret;
+
+	ret = ioctl(wdfd, WDIOC_KEEPALIVE);
+	if (ret == -1) {
+		printf("ping error:%s\n", strerror(errno));
+		return 1;
+	}
+
+	return 0;
+}
+
+static int settimeout(int wdfd, int argc, char *argv[])
+{
+	int ret, val;
+	char *end;
+
+	if (argc < 1) {
+		fprintf(stderr, "No value for timeout\n");
+		return 1;
+	}
+
+	val = strtoul(argv[0], &end, 0);
+	if (end == argv[0] || *end != '\0') {
+		fprintf(stderr, "Invalid number for timeout: '%s'\n", argv[0]);
+		return 1;
+	}
+
+	ret = ioctl(wdfd, WDIOC_SETTIMEOUT, &val);
+	if (ret == -1) {
+		fprintf(stderr, "Set timeout error: %s\n", strerror(errno));
+		return 1;
+	}
+
+	return 0;
+}
+
+static int gettimeout(int wdfd, int argc, char *argv[])
+{
+	int ret, val;
+
+	ret = ioctl(wdfd, WDIOC_GETTIMEOUT, &val);
+	if (ret == -1) {
+		fprintf(stderr, "Get timeout error: %s\n", strerror(errno));
+		return 1;
+	}
+
+	printf("%d\n", val);
+	return 0;
+}
+
+static int setpretimeout(int wdfd, int argc, char *argv[])
+{
+	int ret, val;
+	char *end;
+
+	if (argc < 1) {
+		fprintf(stderr, "No value for pretimeout\n");
+		return 1;
+	}
+
+	val = strtoul(argv[0], &end, 0);
+	if (end == argv[0] || *end != '\0') {
+		fprintf(stderr, "Invalid number for pretimeout: '%s'\n",
+			argv[0]);
+		return 1;
+	}
+
+	ret = ioctl(wdfd, WDIOC_SETPRETIMEOUT, &val);
+	if (ret == -1) {
+		fprintf(stderr, "Set pretimeout error: %s\n", strerror(errno));
+		return 1;
+	}
+
+	return 0;
+}
+
+static int getpretimeout(int wdfd, int argc, char *argv[])
+{
+	int ret, val;
+
+	ret = ioctl(wdfd, WDIOC_GETPRETIMEOUT, &val);
+	if (ret == -1) {
+		fprintf(stderr, "Get pretimeout error: %s\n", strerror(errno));
+		return 1;
+	}
+
+	printf("%d\n", val);
+	return 0;
+}
+
+static int gettimeleft(int wdfd, int argc, char *argv[])
+{
+	int ret, val;
+
+	ret = ioctl(wdfd, WDIOC_GETTIMELEFT, &val);
+	if (ret == -1) {
+		fprintf(stderr, "Get time left error: %s\n", strerror(errno));
+		return 1;
+	}
+
+	printf("%d\n", val);
+	return 0;
+}
+
+static int setaction(int wdfd, int argc, char *argv[])
+{
+	int val, ret;
+
+	if (argc < 1) {
+		fprintf(stderr, "No value for action\n");
+		return 1;
+	}
+
+	val = action_to_val(argv[0], actions);
+	if (val == -1) {
+		fprintf(stderr, "Invalid action: '%s'\n", argv[0]);
+		return 1;
+	}
+
+	ret = ioctl(wdfd, WDIOC_SETACTION, &val);
+	if (ret == -1) {
+		fprintf(stderr, "Set action error: %s\n", strerror(errno));
+		return 1;
+	}
+
+	return 0;
+}
+
+static int getaction(int wdfd, int argc, char *argv[])
+{
+	int val, ret;
+
+	ret = ioctl(wdfd, WDIOC_GETACTION, &val);
+	if (ret == -1) {
+		fprintf(stderr, "Get action error: %s\n", strerror(errno));
+		return 1;
+	}
+
+	print_action(val, actions);
+	return 0;
+}
+
+static int setpreaction(int wdfd, int argc, char *argv[])
+{
+	int val, ret;
+
+	if (argc < 1) {
+		fprintf(stderr, "No value for preaction\n");
+		return 1;
+	}
+
+	val = action_to_val(argv[0], preactions);
+	if (val == -1) {
+		fprintf(stderr, "Invalid preaction: '%s'\n", argv[0]);
+		return 1;
+	}
+
+	ret = ioctl(wdfd, WDIOC_SETPREACTION, &val);
+	if (ret == -1) {
+		fprintf(stderr, "Set preaction error: %s\n", strerror(errno));
+		return 1;
+	}
+
+	return 0;
+}
+
+static int getpreaction(int wdfd, int argc, char *argv[])
+{
+	int val, ret;
+
+	ret = ioctl(wdfd, WDIOC_GETPREACTION, &val);
+	if (ret == -1) {
+		fprintf(stderr, "Get preaction error: %s\n", strerror(errno));
+		return 1;
+	}
+
+	print_action(val, preactions);
+	return 0;
+}
+
+static int setpregov(int wdfd, int argc, char *argv[])
+{
+	int ret;
+
+	if (argc < 1) {
+		fprintf(stderr, "No value for pretimeout governor\n");
+		return 1;
+	}
+
+	ret = ioctl(wdfd, WDIOC_SETPREGOV, argv[0]);
+	if (ret == -1) {
+		fprintf(stderr, "Set preaction governor error: %s\n",
+			strerror(errno));
+		return 1;
+	}
+
+	return 0;
+}
+
+static int getpregov(int wdfd, int argc, char *argv[])
+{
+	int ret;
+	char gov[WATCHDOG_GOV_NAME_MAXLEN];
+
+	ret = ioctl(wdfd, WDIOC_GETPREGOV, gov);
+	if (ret == -1) {
+		fprintf(stderr, "Get preaction governor error: %s\n",
+			strerror(errno));
+		return 1;
+	}
+
+	printf("%s\n", gov);
+
+	return 0;
+}
+
+static int waitdata(int wdfd, int argc, char *argv[])
+{
+	char dummy;
+	int ret;
+
+	ret = read(wdfd, &dummy, 1);
+	if (ret == -1)
+		perror("read");
+	else
+		printf("Received data\n");
+	return 0;
+}
+
+static struct {
+	char *name;
+	int (*handler)(int wdfd, int argc, char *argv[]);
+	char *help;
+} handlers[] = {
+	{ "status",		status,
+	  "- print out the current watchdog status" },
+	{ "setoptions",		setoptions,
+	  "[<option> [<option> [...]]] - set options to one or more:\n"
+	  "    disablecard, enabledcard, temppanic" },
+	{ "ping",		ping,
+	  "- reset the watchdog timer's timeout" },
+	{ "settimeout",		settimeout,
+	  "<timeout> - set the value of the timeout, an integer value" },
+	{ "gettimeout",		gettimeout,
+	  "- get the value of the timeout" },
+	{ "setpretimeout",	setpretimeout,
+	  "<timeout> - set the value of the pretimeout, an integer value" },
+	{ "getpretimeout",	getpretimeout,
+	  "- get the value of the pretimeout" },
+	{ "gettimeleft",	gettimeleft,
+	  "- get the time left before the timeout" },
+	{ "setaction",		setaction,
+	  "<action> - set the action on timeout: reset, poweroff, powercycle" },
+	{ "getaction",		getaction,
+	  "- get the action on timeout" },
+	{ "setpreaction",	setpreaction,
+	  "<action> - set the action on pretimeout: none, nmi, smi,\n"
+	  "    interrupt" },
+	{ "getpreaction",	getpreaction,
+	  "- get the action on pretimeout" },
+	{ "setpregov",		setpregov,
+	  "<governor> - Set the pretimeout governor: noop, panic, read_data" },
+	{ "getpregov",		getpregov,
+	  "- get the pretimeout governor" },
+	{ "waitdata",		waitdata,
+	  "- Wait for read data from the watchdog device" },
+	{ }
+};
+
+static void print_help(char *progname)
+{
+	unsigned int i;
+
+	printf("%s [-d devname] [-h] <command>\nCommands are:\n", progname);
+	for (i = 0; handlers[i].name; i++)
+		printf("  %s %s\n", handlers[i].name, handlers[i].help);
+}
+
+int main(int argc, char *argv[])
+{
+	const char *devfile = "/dev/watchdog";
+	int wdfd;
+	int ret = 0;
+	unsigned int i;
+	int carg;
+
+	for (carg = 1; carg < argc && argv[carg][0] == '-'; carg++) {
+		if (strcmp(argv[carg], "-d") == 0) {
+			carg++;
+			if (carg >= argc) {
+				fprintf(stderr, "No value given after -d\n");
+				exit(EXIT_FAILURE);
+			}
+			devfile = argv[carg];
+		} else if (strcmp(argv[carg], "-h") == 0) {
+			print_help(argv[0]);
+			exit(0);
+		} else if (strcmp(argv[carg], "--") == 0) {
+			carg++;
+			break;
+		}
+	}
+
+	wdfd = open(devfile, O_RDWR);
+	if (wdfd == -1) {
+		perror(devfile);
+		exit(EXIT_FAILURE);
+	}
+
+	if (carg >= argc) {
+		status(wdfd, 0, NULL);
+		goto out;
+	}
+
+	for (i = 0; handlers[i].name; i++) {
+		if (strcmp(handlers[i].name, argv[carg]) == 0) {
+			ret = handlers[i].handler(wdfd, argc - carg - 1,
+						  &argv[carg + 1]);
+			break;
+		}
+	}
+	if (!handlers[i].name) {
+		fprintf(stderr, "Unknown operation: %s\n", argv[carg]);
+		ret = 1;
+	}
+out:
+	close(wdfd);
+	return ret;
+}
-- 
2.17.1

[PATCH 12/12] watchdog: Set the preaction fields for drivers supporting pretimeout

[minyard]

From: Corey Minyard <cminyard@mvista.com>

None can be changed, so no need for the set operation.  The only
one not done was kempld_wdt.c; I couldn't figure out how the
pretimeout worked in that one.

Cc: Jerry Hoemann <jerry.hoemann@hpe.com>
Cc: NXP Linux Team <linux-imx@nxp.com>
Cc: Orson Zhai <orsonzhai@gmail.com>
Cc: Baolin Wang <baolin.wang@linaro.org>
Cc: Chunyan Zhang <zhang.lyra@gmail.com>
Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 drivers/watchdog/hpwdt.c      | 3 +++
 drivers/watchdog/imx2_wdt.c   | 1 +
 drivers/watchdog/imx_sc_wdt.c | 1 +
 drivers/watchdog/pm8916_wdt.c | 1 +
 drivers/watchdog/softdog.c    | 1 +
 drivers/watchdog/sprd_wdt.c   | 1 +
 6 files changed, 8 insertions(+)

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 7d34bcf1c45b..4c8875dac5fa 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -228,6 +228,9 @@ static struct watchdog_device hpwdt_dev = {
 	.timeout	= DEFAULT_MARGIN,
 	.pretimeout	= PRETIMEOUT_SEC,
 	.max_hw_heartbeat_ms	= HPWDT_MAX_TIMER * 1000,
+#ifdef CONFIG_HPWDT_NMI_DECODING
+	.preaction	= WDIOP_NMI,
+#endif
 };
 
 
diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
index 32af3974e6bb..14f64523f12b 100644
--- a/drivers/watchdog/imx2_wdt.c
+++ b/drivers/watchdog/imx2_wdt.c
@@ -280,6 +280,7 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
 	wdog->min_timeout	= 1;
 	wdog->timeout		= IMX2_WDT_DEFAULT_TIME;
 	wdog->max_hw_heartbeat_ms = IMX2_WDT_MAX_TIME * 1000;
+	wdog->preaction		= WDIOP_INTERRUPT;
 	wdog->parent		= &pdev->dev;
 
 	ret = platform_get_irq(pdev, 0);
diff --git a/drivers/watchdog/imx_sc_wdt.c b/drivers/watchdog/imx_sc_wdt.c
index 78eaaf75a263..164bde58036a 100644
--- a/drivers/watchdog/imx_sc_wdt.c
+++ b/drivers/watchdog/imx_sc_wdt.c
@@ -169,6 +169,7 @@ static int imx_sc_wdt_probe(struct platform_device *pdev)
 	wdog->max_timeout = MAX_TIMEOUT;
 	wdog->parent = dev;
 	wdog->timeout = DEFAULT_TIMEOUT;
+	wdog->preaction = WDIOP_INTERRUPT;
 
 	watchdog_init_timeout(wdog, 0, dev);
 	watchdog_stop_on_reboot(wdog);
diff --git a/drivers/watchdog/pm8916_wdt.c b/drivers/watchdog/pm8916_wdt.c
index 2d3652004e39..b70cd45067d6 100644
--- a/drivers/watchdog/pm8916_wdt.c
+++ b/drivers/watchdog/pm8916_wdt.c
@@ -184,6 +184,7 @@ static int pm8916_wdt_probe(struct platform_device *pdev)
 	wdt->wdev.max_timeout = PM8916_WDT_MAX_TIMEOUT;
 	wdt->wdev.timeout = PM8916_WDT_DEFAULT_TIMEOUT;
 	wdt->wdev.pretimeout = 0;
+	wdt->wdev.preaction = WDIOP_INTERRUPT;
 	watchdog_set_drvdata(&wdt->wdev, wdt);
 
 	watchdog_init_timeout(&wdt->wdev, 0, dev);
diff --git a/drivers/watchdog/softdog.c b/drivers/watchdog/softdog.c
index 3e4885c1545e..cb3f829d8bcb 100644
--- a/drivers/watchdog/softdog.c
+++ b/drivers/watchdog/softdog.c
@@ -125,6 +125,7 @@ static struct watchdog_device softdog_dev = {
 	.min_timeout = 1,
 	.max_timeout = 65535,
 	.timeout = TIMER_MARGIN,
+	.preaction = WDIOP_INTERRUPT,
 };
 
 static int __init softdog_init(void)
diff --git a/drivers/watchdog/sprd_wdt.c b/drivers/watchdog/sprd_wdt.c
index edba4e278685..d70c738c7356 100644
--- a/drivers/watchdog/sprd_wdt.c
+++ b/drivers/watchdog/sprd_wdt.c
@@ -302,6 +302,7 @@ static int sprd_wdt_probe(struct platform_device *pdev)
 	wdt->wdd.min_timeout = SPRD_WDT_MIN_TIMEOUT;
 	wdt->wdd.max_timeout = SPRD_WDT_MAX_TIMEOUT;
 	wdt->wdd.timeout = SPRD_WDT_MAX_TIMEOUT;
+	wdt->wdd.preaction = WDIOP_INTERRUPT;
 
 	ret = sprd_wdt_enable(wdt);
 	if (ret) {
-- 
2.17.1