Re: [PATCH 02/12] watchdog: Add the ability to provide data to read

From: Guenter Roeck <linux@roeck-us.net>
To: cminyard@mvista.com, Jerry Hoemann <jerry.hoemann@hpe.com>
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 02/12] watchdog: Add the ability to provide data to read
Date: Tue, 20 Aug 2019 06:53:40 -0700	[thread overview]
Message-ID: <fa50d921-8d40-6bf8-00f3-b46d9468829a@roeck-us.net> (raw)
In-Reply-To: <20190820121243.GO445@minyard.net>

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.

On a side note, a standard watchdog driver can implement its own ioctl functions.

Guenter

> -corey
> 
>>
>>
>>
>>
>> -- 
>>
>> -----------------------------------------------------------------------------
>> Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
>> -----------------------------------------------------------------------------
> 