Linux-Watchdog Archive on lore.kernel.org
 help / color / Atom feed
From: Corey Minyard <cminyard@mvista.com>
To: Guenter Roeck <linux@roeck-us.net>
Cc: Convert@minyard.net, the@minyard.net, IPMI@minyard.net,
	watchdog@minyard.net, to@minyard.net, standard@minyard.net,
	interface@minyard.net, linux-watchdog@vger.kernel.org,
	Wim Van Sebroeck <wim@linux-watchdog.org>
Subject: Re: [PATCH 02/12] watchdog: Add the ability to provide data to read
Date: Mon, 19 Aug 2019 19:23:09 -0500
Message-ID: <20190820002309.GI445@minyard.net> (raw)
In-Reply-To: <20190819224345.GB8869@roeck-us.net>

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
> > 

  reply index

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20190819203711.32599-1-minyard@acm.org>
2019-08-19 20:37 ` [PATCH 01/12] watchdog: NULL the default governor if it is unregistered minyard
2019-08-19 22:35   ` Guenter Roeck
2019-08-19 20:37 ` [PATCH 02/12] watchdog: Add the ability to provide data to read minyard
2019-08-19 21:50   ` Guenter Roeck
2019-08-19 22:43   ` Guenter Roeck
2019-08-20  0:23     ` Corey Minyard [this message]
2019-08-20  1:09       ` Jerry Hoemann
2019-08-20 12:12         ` Corey Minyard
2019-08-20 13:53           ` Guenter Roeck
2019-08-20 15:58             ` Corey Minyard
2019-08-20 17:14               ` Guenter Roeck
2019-08-20 18:16                 ` Corey Minyard
2019-08-19 20:37 ` [PATCH 03/12] watchdog: Add a pretimeout governor to provide read data minyard
2019-08-19 20:37 ` [PATCH 04/12] watchdog: Allow pretimeout governor setting to be accessed from modules minyard
2019-08-19 21:49   ` Guenter Roeck
2019-08-20  0:24     ` Corey Minyard
2019-08-19 20:37 ` [PATCH 05/12] watchdog:ipmi: Move the IPMI watchdog to drivers/watchdog minyard
2019-08-19 20:37 ` [PATCH 06/12] watchdog:ipmi: Convert over to the standard watchdog infrastructure minyard
2019-08-19 20:37 ` [PATCH 07/12] watchdog:ipmi: Add the ability to fetch the current time left minyard
2019-08-19 20:37 ` [PATCH 08/12] watchdog: Add the ability to set the action of a timeout minyard
2019-08-19 21:58   ` Guenter Roeck
2019-08-20  0:39     ` Corey Minyard
2019-08-20 14:17       ` Guenter Roeck
2019-08-20 19:39         ` Corey Minyard
2019-08-19 20:37 ` [PATCH 09/12] watchdog:ipmi: Implement action and preaction functions minyard
2019-08-19 20:37 ` [PATCH 10/12] watchdog: Add a way to set the governor through the ioctl minyard
2019-08-19 20:37 ` [PATCH 11/12] watchdog: Add a sample program that can fully use the watchdog interface minyard
2019-08-19 20:37 ` [PATCH 12/12] watchdog: Set the preaction fields for drivers supporting pretimeout minyard

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190820002309.GI445@minyard.net \
    --to=cminyard@mvista.com \
    --cc=Convert@minyard.net \
    --cc=IPMI@minyard.net \
    --cc=interface@minyard.net \
    --cc=linux-watchdog@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=standard@minyard.net \
    --cc=the@minyard.net \
    --cc=to@minyard.net \
    --cc=watchdog@minyard.net \
    --cc=wim@linux-watchdog.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Watchdog Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-watchdog/0 linux-watchdog/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-watchdog linux-watchdog/ https://lore.kernel.org/linux-watchdog \
		linux-watchdog@vger.kernel.org
	public-inbox-index linux-watchdog

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-watchdog


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git