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 08/12] watchdog: Add the ability to set the action of a timeout
Date: Mon, 19 Aug 2019 19:39:28 -0500
Message-ID: <20190820003928.GK445@minyard.net> (raw)
In-Reply-To: <20190819215858.GB7517@roeck-us.net>

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

  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
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 [this message]
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=20190820003928.GK445@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