Re: watchdog: WatchDog Timer Driver Core: ping a hardware watchdog in kernel's space

[Guenter Roeck <linux@roeck-us.net>]

On 09/04/2014 08:47 AM, Janusz Użycki wrote:
> Hi,
>
> Some applications require to start watchdog before userspace software. This patch enables such feature. Only WATCHDOG_KERNEL_PING flag is necessary to enable it (attached example for stmp3xxx_rtc_wdt.c). Moreover kernel's ping is re-enabled when userspace software closed watchdog using the magic character. The features improves kernel's reliable if hardware watchdog is available.
> * Can you comment the proposed patch?
> * Shoud dynamic or static timer_list be used (small structure...)?
> * I also added wdd->ops->ref/unref calls but I'm afraid that even original code is buggy in watchdog_dev.c. Is any driver that uses the pointers? In my opinion watchdog_open() should call wdd->ops->ref() before watchdog_start() and watchdog_release() should call wdd->ops->unref() before module_put(). Otherwise fault is possible if watchdog module is unloaded.
> * I noticed that current watchdog core does not support suspend/resume case. If you want to use suspend without the patch you need to close a watchdog in userspace using the magic character before suspend command. With the patch you must to use WDIOC_SETOPTIONS ioctl and WDIOS_DISABLECARD/WDIOS_ENABLECARD. Is there any other method to suspend with watchdog?
>
> best regards
> Janusz Uzycki
>
I like the basic idea. Couple of comments.

Please read and follow Documentation/SubmittingPatches.
Long lines as above are discouraged, as is sending patches
as attachment. As you can see, the patch disappears in the
reply, making it very hard to comment on it.

I would suggest to just modify the timer to half the timeout value,
and then just ping the watchdog unconditionally whenever the callback
runs. The timer should stop when the watchdog is opened, so there
should not be a need to check its open status in the callback.

I don't think there is a need to manipulate the driver reference
count when the kernel timer starts and stops. You already run
module_get and module_put during registration / unregistration.

I would not call this "kernel ping". We'll need to find a better
name. Proposals welcome. Something indicating the status, ie something
indicating that the wdt is always running and can not be stopped.

You would not update the status in the affected driver(s) by
setting the flag in the probe function. You can use the status
flag initialization for that purpose. Also, we'll need to know if
the driver you changed is always affected or only in your system.
Either case, driver patches need to be submitted separately.

We'll need to tie this functionality with parallel efforts
to add similar code into individual drivers. There is one patch
along that line pending right now [1], and I think there is similar
support in other drivers. dw_wdt is one example where the wdt can
not be stopped after it was started, of_xilinx_wdt is another.

Given that, there are two use states to consider: WDT is always running,
and WDT can not be stopped after it was started once. We should cover
both cases.

The feature should have DT support from the beginning if possible,
though it should be added as a separate patch in case there is
a hiccup with the DT folks.

It might be worthwhile exploring if the same mechanism can be used
to augment user space pinging with kernel ping if the maximum timeout
is too short to be handled by user space alone. That should be a
separate patch, but we need to keep this use case in mind.

Guenter

---
[1] http://patchwork.roeck-us.net/patch/1890/