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: Jerry Hoemann <jerry.hoemann@hpe.com>,
	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: Tue, 20 Aug 2019 13:16:37 -0500
Message-ID: <20190820181637.GR445@minyard.net> (raw)
In-Reply-To: <f4a6d027-c491-5e81-777e-ac2ffaecd3dc@roeck-us.net>

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

  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 [this message]
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=20190820181637.GR445@minyard.net \
    --to=cminyard@mvista.com \
    --cc=Convert@minyard.net \
    --cc=IPMI@minyard.net \
    --cc=interface@minyard.net \
    --cc=jerry.hoemann@hpe.com \
    --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