On 09/24/2018 02:42 PM, Shuah Khan wrote:
On 09/23/2018 07:47 PM, Jerry Hoemann wrote:
On Fri, Sep 21, 2018 at 05:42:00PM -0600, Shuah Khan wrote:
Hi Jerry,
Thanks for the patch. A few comments below:
Replies inline.
On 09/21/2018 04:55 PM, Jerry Hoemann wrote:
Add command line arguments to call ioctl WDIOC_GETTIMEOUT, WDIOC_GETPRETIMEOUT and WDIOC_SETPRETIMEOUT.
Signed-off-by: Jerry Hoemann jerry.hoemann@hpe.com
tools/testing/selftests/watchdog/watchdog-test.c | 30 +++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/watchdog/watchdog-test.c b/tools/testing/selftests/watchdog/watchdog-test.c index 6e29087..4861e2c 100644 --- a/tools/testing/selftests/watchdog/watchdog-test.c +++ b/tools/testing/selftests/watchdog/watchdog-test.c @@ -19,7 +19,7 @@ int fd; const char v = 'V'; -static const char sopts[] = "bdehp:t:"; +static const char sopts[] = "bdehp:t:Tn:N"; static const struct option lopts[] = { {"bootstatus", no_argument, NULL, 'b'}, {"disable", no_argument, NULL, 'd'}, @@ -27,6 +27,9 @@ {"help", no_argument, NULL, 'h'}, {"pingrate", required_argument, NULL, 'p'}, {"timeout", required_argument, NULL, 't'},
- {"gettimeout", no_argument, NULL, 'T'},
- {"pretimeout", required_argument, NULL, 'n'},
- {"getpretimeout", no_argument, NULL, 'N'}, {NULL, no_argument, NULL, 0x0}
}; @@ -71,6 +74,9 @@ static void usage(char *progname) printf(" -h, --help Print the help message\n"); printf(" -p, --pingrate=P Set ping rate to P seconds (default %d)\n", DEFAULT_PING_RATE); printf(" -t, --timeout=T Set timeout to T seconds\n");
- printf(" -T, --gettimeout Get the timeout\n");
- printf(" -n, --pretimeout Set the pretimeout to T seconds\n");
- printf(" -N, --getpretimeout Get the pretimeout\n");
How are the new arguments used?
I forgot the param. Should be:
+ printf(" -n, --pretimeout=T Set the pretimeout to T seconds\n");I'll update in v2.
Okay.
Is this what you mean? Or did I misunderstand?
printf("\n"); printf("Parameters are parsed left-to-right in real-time.\n"); printf("Example: %s -d -t 10 -p 5 -e\n", progname);
Please add an example usage for each of these new arguments.
Will do.
okay.
@@ -135,6 +141,28 @@ int main(int argc, char *argv[]) else printf("WDIOC_SETTIMEOUT errno '%s'\n", strerror(errno)); break;
case 'T':ret = ioctl(fd, WDIOC_GETTIMEOUT, &flags);if (!ret)printf("Watchdog timeout set to %u seconds.\n", flags);It would good to make this message different from the WDIOC_SETTIMEOUT message. Please update it to reflect that this is the result of a WDIOC_GETTIMEOUT.
Will update message to make distinct.
What would user intend to do with this GETTIMEOUT? Shouldn't this be the case that it prints the current value and exits instead of the same logic as SETTIMEOUT option?
Are you suggesting setting the "oneshot" flag so the test app doesn't actually go into the while(1) keep_alive loop?
Watchdog drivers may adjust the requested value to match hardware constraints. Callers of set timeout (and set pretimeout) should call get timeout to see what value was actually set.
B/c of above, I just got into the habit of specifying both flags: first set, then get to make sure value set was what I intended.
But I can make the "Get" a one shot. Just let me know if this is your preference.
I prefer that both GETs be oneshot. GETs should just print the current value and go follow oneshot path. It doesn't make sense for them to do more.
elseprintf("WDIOC_GETTIMEOUT errno '%s'\n", strerror(errno))Shouldn't this error be an exit condition?
Hmmm, I don't see this error path much different than the error path for the other failing ioctl. Am I missing something?
Yeah that is what I don't understand with the new code as well as the existing. Shouldn't error path be handled differently. What is the point in doing more other than gracefully exit closing the file? I don't think existing error paths are doing this, probably they should.
But, If we make the "GET" a one shot, then we wouldn't really need to special case the failure case as we wouldn't go into the keep_alive loop in either case.
Right.
break;case 'n':flags = strtoul(optarg, NULL, 0);ret = ioctl(fd, WDIOC_SETPRETIMEOUT, &flags);if (!ret)printf("Watchdog pretimeout set to %u seconds.\n", flags);elseprintf("WDIOC_SETPRETIMEOUT errno '%s'\n", strerror(errno));break;case 'N':ret = ioctl(fd, WDIOC_GETPRETIMEOUT, &flags);if (!ret)printf("Watchdog pretimeout set to %u seconds.\n", flags);It would good to make this message different from the WDIOC_GETPRETIMEOUT message. Please update it to reflect that this is the result of a WDIOC_GETPRETIMEOUT
will do.
Okay.
What would user intend to do with this GETTIMEOUT? Shouldn't this be the case that it prints the current value and exits instead of the same logic as WDIOC_SETPRETIMEOUT?
I think you're just asking me to set the "oneshot" flag on this, which I can certainly do.
Correct. For couple of reasons. GET/SET_PRETIMEOUG might not be supported on all platforms/drivers. It would make sense to handle error paths correctly.
But, some background on pretimeout that (I think) is interesting:
The underling HW for the watchdog on proliants allows for the pre-timeout to be enabled or disabled. But if the pretimeout is enabled, the value of the pretimeout is hard coded by HW (9 seconds.)
The hpwdt driver allows for setting pretimeout by passing in a value 0 < pretimeout < timeout to enable a pretimeout. The user then needs to call get pretimeout to determine the actual value.
Failure to take into account the pretimeout when pinging the WD can lead to unexpected system crashes.
I've handled the following issue multiple times:
A user wants to set the timeout to value T and ping the WD every T/2 seconds. He fails to take into account the pretimeout value of P. The system crashes with the pretimeout NMI when (T/2) < P.
The basic misunderstanding is that to prevent the WD from acting, the WD only needs to be pinged at least once every T seconds, when in actuality the WD needs to be pinged at least once every (T-P) seconds.
Specifically for Proliants, I've seen people set the timeout to 10 seconds thinking they had plenty of time to ping the WD only to be surprised when the pretimeout NMI takes the system down 1 second later.
In this case, this patch really doesn't solve the problem. You will still run into this problem if user does a set. You are providing a way to check pretimeout, however that is a separate operation. So I am not clear on how this patch solves the issue of pretimeout NMI takes the system down.
Note: a WD doesn't need to support the pretimeout feature.
It isn't clear what this means?
elseprintf("WDIOC_GETPRETIMEOUT errno '%s'\n", strerror(errno));Shouldn't this error be an exit condition?
Similar to above. I can make GETPRETIMEOUT a "oneshot" to handle both the success/failing case of the ioctl call.
break;Also can you run this test as normal user?
No. Must be run as root to open /dev/watchdog. When /dev/watchdog is opened, the WD is started and if not updated properly, the system will crash.
Hmm. I don't understand why the system would panic if non-root user can't open the device, at least in the context of this test.
fd = open("/dev/watchdog", O_WRONLY); if (fd == -1) { printf("Watchdog device not enabled.\n"); exit(-1); }Shouldn't it just exit based on the code above?
"cat /dev/watchdog" is one of my favorite ways to crash a system. :) :)
That doesn't sound great, if a non-root user can bring the system down!!
This got me concerned enough that I tried this with softdog. It behaved just the way I expected it.
cat /dev/watchdog cat: /dev/watchdog: Permission denied
Running the test as non-root does the following as per the current logic.
watchdog-test -b Watchdog device not enabled.
I think this logic could be improved to detect that a non-root user is running the test and print appropriate message.
However, I am not seeing the behavior you are describing that "cat /dev/watchdog" panics the syste. Did you mean running a root which is expected unless you terminate before the timeout? If you are seeing this as non-root user on you system, the watchdog driver could be suspect.
thanks, -- Shuah