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@xxxxxxx wrote:
From: Corey Minyard <cminyard@xxxxxxxxxx>
This is for the read data pretimeout governor.
Signed-off-by: Corey Minyard <cminyard@xxxxxxxxxx>
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
-----------------------------------------------------------------------------
