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

[Vladimir Zapolskiy]

Hi Wolfram,

On 05.06.2016 12:48, Wolfram Sang wrote:
> On Thu, May 26, 2016 at 03:37:17PM -0700, Guenter Roeck wrote:
>> On Thu, May 26, 2016 at 06:41:36PM +0200, Wolfram Sang wrote:
>> [ ... ]
>>>
>>> No doubt about that. I had some ideas and thought it is easier to talk
>>> over code. If you want to rebase it, too, I'd be happy to check what you
>>> came up with to solve the problems. I might still argue that I prefer
>>> the less-code approach, but it will be Guenter's / Wim's decision, of
>>> course.
>>>
>> I have a large back-log of patches to review. Simple patches with less code
>> will get preferential treating. The more complex, the higher the likelyhood
>> that the patches get pushed to the end of the queue.
>>
>> Giving a quick glance, I liked Wolfram's patches because they seemed to
>> be simple and straightforward. I hope the next version won't add too much
>> additional complexity.
> 
> Vladimir, did you have time to look into this?

yes, I managed to rebase and split my change to smaller pieces to encourage
reviewers.

My plan is to do thorough testing of the change and send it for review
tomorrow or on Tuesday, please participate in review.

> I can offer to resend this series on-top of v4.7-rc1 with the core patch
> re-attributed to Vladimir. Then we can review the basic stuff now, and
> can discuss/deal with the bottom half handling incrementally.
> 
> Sounds good?
> 

--
Best wishes,
Vladimir

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

[Wolfram Sang]

On Thu, May 26, 2016 at 03:37:17PM -0700, Guenter Roeck wrote:
> On Thu, May 26, 2016 at 06:41:36PM +0200, Wolfram Sang wrote:
> [ ... ]
> > 
> > No doubt about that. I had some ideas and thought it is easier to talk
> > over code. If you want to rebase it, too, I'd be happy to check what you
> > came up with to solve the problems. I might still argue that I prefer
> > the less-code approach, but it will be Guenter's / Wim's decision, of
> > course.
> > 
> I have a large back-log of patches to review. Simple patches with less code
> will get preferential treating. The more complex, the higher the likelyhood
> that the patches get pushed to the end of the queue.
> 
> Giving a quick glance, I liked Wolfram's patches because they seemed to
> be simple and straightforward. I hope the next version won't add too much
> additional complexity.

Vladimir, did you have time to look into this?

I can offer to resend this series on-top of v4.7-rc1 with the core patch
re-attributed to Vladimir. Then we can review the basic stuff now, and
can discuss/deal with the bottom half handling incrementally.

Sounds good?



signature.asc
Description: PGP signature

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

[Guenter Roeck]

On Thu, May 26, 2016 at 06:41:36PM +0200, Wolfram Sang wrote:
[ ... ]
> 
> No doubt about that. I had some ideas and thought it is easier to talk
> over code. If you want to rebase it, too, I'd be happy to check what you
> came up with to solve the problems. I might still argue that I prefer
> the less-code approach, but it will be Guenter's / Wim's decision, of
> course.
> 
I have a large back-log of patches to review. Simple patches with less code
will get preferential treating. The more complex, the higher the likelyhood
that the patches get pushed to the end of the queue.

Giving a quick glance, I liked Wolfram's patches because they seemed to
be simple and straightforward. I hope the next version won't add too much
additional complexity.

Guenter

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

[Wolfram Sang]

Hi Vladimir,

great to see you still have capacity for this series :)

> 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.

I am totally open that poll might not be a good idea, but why do you
think uevent is best? (Disclaimer: I don't do much userspace code)

> 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.

One reason I decided to drop 'can_sleep' is that I guessed 98% of users
will be happy with the panic, noop, and userspace governers. 2% might
need custom governors from which maybe not even all need to sleep.
Chances are high IMO that these govenors will be out-of-tree code, so
having all this additional complexity for some out-of-tree govenors was
questionable to me. I wondered if it would make sense to let those
govenors do the bottom half handling themselves.

There was also a technical reason: The dev pointer was first moved to
watchdog_device private data before it was ultimately removed. So, while
trying to fix this, the code got more and more complicated which led me
to the decision to go the other way around: make the code simpler so it
will be easier maintainable in the future.

> 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 still wonder if bottom half handling shouldn't be put to the governors
which need that.

> I know it is quite complex, probably it might be better to add it to
> the series as a separate patch?

That might help the initial review.

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

I changed the authorship because I did one fundamental change to your
original design. Not knowing if you'd approve of that, I didn't want to
put your sticker on something you might not even like.

> 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.

No doubt about that. I had some ideas and thought it is easier to talk
over code. If you want to rebase it, too, I'd be happy to check what you
came up with to solve the problems. I might still argue that I prefer
the less-code approach, but it will be Guenter's / Wim's decision, of
course.

And I apologize for not contacting you beforehand which would have been
friendly. I got a rush on hacking it and wanted to show what I came up
with. No offence, sorry!

Thanks,

   Wolfram



signature.asc
Description: PGP signature

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

[Vladimir Zapolskiy]

Hi Wolfram,

On 25.05.2016 16:32, Wolfram Sang wrote:
> From: Wolfram Sang 
> 
> 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 
> Signed-off-by: Wolfram Sang 
> ---
> 
> 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

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

[Wolfram Sang]

From: Wolfram Sang 

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 
Signed-off-by: Wolfram Sang 
---

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.
* 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

 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

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 3902c9ca7f099d..909d1021de5cbc 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1774,4 +1774,12 @@ config USBPCWATCHDOG
 
  Most people will say N.
 
+comment "Watchdog Pretimeout Governors"
+
+config WATCHDOG_PRETIMEOUT_GOV
+   bool "Enable watchdog pretimeout governors"
+   default n
+   help
+ The option allows to select watchdog pretimeout governors.
+
 endif # WATCHDOG
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 2cbc9709852d0e..820860cf3e8d62 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -3,8 +3,10 @@
 #
 
 # The WatchDog Timer Driver Core.
-watchdog-objs  += watchdog_core.o watchdog_dev.o
-obj-$(CONFIG_WATCHDOG_CORE)+= watchdog.o
+obj-$(CONFIG_WATCHDOG_CORE) += watchdog.o
+
+watchdog-y += watchdog_core.o watchdog_dev.o
+watchdog-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV) += watchdog_pretimeout.o
 
 # Only one watchdog can succeed. We probe the ISA/PCI/USB based
 # watchdog-cards first, then the architecture specific watchdog
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 3595cffa24ea49..5d028f94a90743 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -49,6 +49,7 @@
 #include  /* For copy_to_user/put_user/... */
 
 #include "watchdog_core.h"
+#include "watchdog_pretimeout.h"
 
 /*
  * struct watchdog_core_data - watchdog core internal data
@@ -911,6 +912,12 @@ int watchdog_dev_register(struct watchdog_device *wdd)
return PTR_ERR(dev);
}
 
+   ret = watchdog_register_pretimeout(wdd, dev);
+   if (ret) {
+   device_destroy(_class, devno);
+   watchdog_cdev_unregister(wdd);
+   }
+
return ret;
 }
 
@@ -924,6 +931,7 @@ int watchdog_dev_register(struct watchdog_device *wdd)
 
 void watchdog_dev_unregister(struct watchdog_device *wdd)
 {
+   watchdog_unregister_pretimeout(wdd);
device_destroy(_class, wdd->wd_data->cdev.dev);
watchdog_cdev_unregister(wdd);
 }
diff --git a/drivers/watchdog/watchdog_pretimeout.c 
b/drivers/watchdog/watchdog_pretimeout.c
new file mode 100644
index 00..87a10ebeaacc7e
--- /dev/null
+++ b/drivers/watchdog/watchdog_pretimeout.c
@@ -0,0 +1,269 @@
+/*
+ * Watchdog pretimout governor framework
+ *
+ * Copyright (C) 2015 Mentor Graphics
+ * Copyright (C) 2016 Renesas Electronics Corporation
+ * Copyright (C) 2016 Sang Engineering, Wolfram Sang
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the