Re: [PATCH] selftests: watchdog: Add gettimeout and get|set pretimeout

[Shuah Khan <shuah@kernel.org>]

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.
>>
>>
>>>
>>>> +			else
>>>> +				printf("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);
>>>> +			else
>>>> +				printf("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?
> 
>>
>>>
>>>> +			else
>>>> +				printf("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;
>>>>  		default:
>>>>  			usage(argv[0]);
>>>>  			goto end;
>>>>
>>>
>>> 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