Re: [PATCH 1/7] watchdog: add watchdog pretimeout framework

[Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>]

Hi Wolfram,

On 25.05.2016 16:32, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> The change adds a simple watchdog pretimeout framework infrastructure,
> its purpose is to allow users to select a desired handling of watchdog
> pretimeout events, which may be generated by a watchdog driver.
> 
> By design every watchdog pretimeout governor may be compiled as a
> kernel module, a user selects a default watchdog pretimeout
> governor during compilation stage and can select another governor in
> runtime.
> 
> Watchdogs with WDIOF_PRETIMEOUT capability now have two device
> attributes in sysfs: read/write pretimeout_governor attribute and read
> only pretimeout_available_governors attribute.
> 
> Watchdogs with no WDIOF_PRETIMEOUT capability has no changes in
> sysfs.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> Changes since Vladimir's last version:
> 
> * rebased
>   adapt to the internal data reorganization, especially the now private
>   struct device *dev
> * dropped can_sleep support!
>   The additional lock, list, and workqueue made the code quite complex. The
>   only user was the userspace governor which can be reworked to let the
>   watchdog device code do the bottom half. In addition, I am not fully
>   convinced sending a uevent is the proper thing to do, but this needs to
>   be discussed in another thread. Removing this support makes the code much
>   easier to follow (locking!), saves 30% of LoC + a list + a workqueue.

The thing is that I'm particularly interested in

1) sleeping governors,
2) userspace notification of any appropriate kind, but preferably not by
   adding a clumsy .poll callback, uevent is the best IMHO.

The userspace sleeping governor is the only one proposed for a mainline,
however the whole idea of having a framework is to allow users to write
their own private governors, and that's exactly what we need and use.

So the original complexity has its state-of-the-art grounds, and for
sake of getting a solid picture for reviewers and users it is better to
introduce sleeping functionality right from the beginning. I know it
is quite complex, probably it might be better to add it to the series
as a separate patch?

> * moved pretimeout registration from watchdog_core to watchdog_dev
>   Let's handle it exactly where the device is created, so we have access to
>   the now private device pointer for adding the sysfs files.
> * don't export watchdog_(un)register_pretimeout since they are linked to the
>   core anyhow
> * whitespace cleanups
> 

Thanks for pushing it, but do you think that the authorship of the
code can be preserved?

Feel free to ask me to rebase the change and so on, patch review procedure
is well established and I'm pretty sure I can cope with it.

I believe the main problem with the original code since the time when
rebase was not required is that it didn't receive any formal technical
review from Guenter or Wim, but I'm glad to know that someone else is
interested in it.

Merge window is closing, so it's good time for me to rebase the change
and resend it, do you have any objections?

>  drivers/watchdog/Kconfig               |   8 +
>  drivers/watchdog/Makefile              |   6 +-
>  drivers/watchdog/watchdog_dev.c        |   8 +
>  drivers/watchdog/watchdog_pretimeout.c | 269 +++++++++++++++++++++++++++++++++
>  drivers/watchdog/watchdog_pretimeout.h |  35 +++++
>  include/linux/watchdog.h               |  10 ++
>  6 files changed, 334 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/watchdog/watchdog_pretimeout.c
>  create mode 100644 drivers/watchdog/watchdog_pretimeout.h
> 

--
With best wishes,
Vladimir