Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1646980 > unrolled thread

[PATCH v5 0/3] watchdog: allow setting deadline for opening /dev/watchdogN

Back to article view | Back to linux.kernel

Contents

  [PATCH v5 0/3] watchdog: allow setting deadline for opening /dev/watchdogN Rasmus Villemoes <rasmus.villemoes@prevas.dk> - 2017-05-22 16:30 +0200
    Re: [PATCH v5 0/3] watchdog: allow setting deadline for opening  /dev/watchdogN Alan Cox <gnomes@lxorguk.ukuu.org.uk> - 2017-05-22 20:10 +0200
      Re: [PATCH v5 0/3] watchdog: allow setting deadline for opening  /dev/watchdogN Guenter Roeck <linux@roeck-us.net> - 2017-05-22 21:50 +0200
      Re: [PATCH v5 0/3] watchdog: allow setting deadline for opening  /dev/watchdogN Rasmus Villemoes <rasmus.villemoes@prevas.dk> - 2017-05-23 09:30 +0200
    Re: [PATCH v5 0/3] watchdog: allow setting deadline for opening /dev/watchdogN Esben Haabendal <esben.haabendal@gmail.com> - 2017-05-24 16:30 +0200

#1646980 — [PATCH v5 0/3] watchdog: allow setting deadline for opening /dev/watchdogN

If a watchdog driver tells the framework that the device is running,
the framework takes care of feeding the watchdog until userspace opens
the device. If the userspace application which is supposed to do that
never comes up properly, the watchdog is fed indefinitely by the
kernel. This can be especially problematic for embedded devices.

These patches allow one to set a maximum time for which the kernel
will feed the watchdog, thus ensuring that either userspace has come
up, or the board gets reset. This allows fallback logic in the
bootloader to attempt some recovery (for example, if an automatic
update is in progress, it could roll back to the previous version).

The patches have been tested on a Raspberry Pi 2 and a Wandboard.

v5 is identical to v4 posted in January, just rebased to current
master (v4.12-rc2).

v4 is mostly identical to v1. The differences are that the ability to
compile out this feature is removed, and the ability to set the
default value for the watchdog.open_timeout command line parameter via
Kconfig is split into a separate patch.

Compared to v2/v3, this drops the ability to set the open_timeout via
a device property; I'll leave implementing that to those who actually
need it.

Rasmus Villemoes (3):
  watchdog: introduce watchdog_worker_should_ping helper
  watchdog: introduce watchdog.open_timeout commandline parameter
  watchdog: introduce CONFIG_WATCHDOG_OPEN_TIMEOUT

 Documentation/watchdog/watchdog-parameters.txt | 10 +++++++
 drivers/watchdog/Kconfig                       |  9 +++++++
 drivers/watchdog/watchdog_dev.c                | 37 +++++++++++++++++++++++---
 3 files changed, 52 insertions(+), 4 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1647231 — Re: [PATCH v5 0/3] watchdog: allow setting deadline for opening /dev/watchdogN

On Mon, 22 May 2017 16:06:36 +0200
Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote:

> If a watchdog driver tells the framework that the device is running,
> the framework takes care of feeding the watchdog until userspace opens
> the device. If the userspace application which is supposed to do that
> never comes up properly, the watchdog is fed indefinitely by the
> kernel. This can be especially problematic for embedded devices.
> 
> These patches allow one to set a maximum time for which the kernel
> will feed the watchdog, thus ensuring that either userspace has come
> up, or the board gets reset. This allows fallback logic in the
> bootloader to attempt some recovery (for example, if an automatic
> update is in progress, it could roll back to the previous version).


This makes sense except for being a CONFIG_ option not a boot parameter.
If it's a boot parameter then the same kernel works for multiple systems
and is general. If it's compile time then you have to build a custom
kernel.

For some embedded stuff that might not matter (although I bet they'd
prefer it command line/device tree too) but for something like an x86
platform where you are deploying a standard vendor supplied kernel it's
bad to do it that way IMHO.

In other words I think you should drop patch 3 but the rest is good.

Alan

[toc] | [prev] | [next] | [standalone]

#1647311 — Re: [PATCH v5 0/3] watchdog: allow setting deadline for opening /dev/watchdogN

On Mon, May 22, 2017 at 07:07:51PM +0100, Alan Cox wrote:
> On Mon, 22 May 2017 16:06:36 +0200
> Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote:
> 
> > If a watchdog driver tells the framework that the device is running,
> > the framework takes care of feeding the watchdog until userspace opens
> > the device. If the userspace application which is supposed to do that
> > never comes up properly, the watchdog is fed indefinitely by the
> > kernel. This can be especially problematic for embedded devices.
> > 
> > These patches allow one to set a maximum time for which the kernel
> > will feed the watchdog, thus ensuring that either userspace has come
> > up, or the board gets reset. This allows fallback logic in the
> > bootloader to attempt some recovery (for example, if an automatic
> > update is in progress, it could roll back to the previous version).
> 
> 
> This makes sense except for being a CONFIG_ option not a boot parameter.
> If it's a boot parameter then the same kernel works for multiple systems
> and is general. If it's compile time then you have to build a custom
> kernel.
> 
> For some embedded stuff that might not matter (although I bet they'd
> prefer it command line/device tree too) but for something like an x86
> platform where you are deploying a standard vendor supplied kernel it's
> bad to do it that way IMHO.
> 
> In other words I think you should drop patch 3 but the rest is good.
> 

Same here. Can we assume a formal Reviewed-by: from you for the first two
patches ?

Thanks,
Guenter

> Alan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1647724 — Re: [PATCH v5 0/3] watchdog: allow setting deadline for opening /dev/watchdogN

On 2017-05-22 20:07, Alan Cox wrote:
> On Mon, 22 May 2017 16:06:36 +0200
> Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote:
> 
>> If a watchdog driver tells the framework that the device is running,
>> the framework takes care of feeding the watchdog until userspace opens
>> the device. If the userspace application which is supposed to do that
>> never comes up properly, the watchdog is fed indefinitely by the
>> kernel. This can be especially problematic for embedded devices.
>>
>> These patches allow one to set a maximum time for which the kernel
>> will feed the watchdog, thus ensuring that either userspace has come
>> up, or the board gets reset. This allows fallback logic in the
>> bootloader to attempt some recovery (for example, if an automatic
>> update is in progress, it could roll back to the previous version).
> 
> 
> This makes sense except for being a CONFIG_ option not a boot parameter.
> If it's a boot parameter then the same kernel works for multiple systems
> and is general. If it's compile time then you have to build a custom
> kernel.

I don't understand. Patch 3 simply allows the default to be set in the
config, but it is very much still possible to set it via the boot
command line, overriding the default (whether that was a CONFIG or a
hardcoded 0).

> For some embedded stuff that might not matter (although I bet they'd
> prefer it command line/device tree too)

When building a full BSP, yes, one can cook it into the boot command
line in u-boot, but that means that to update it one has to edit one's
u-boot recipe. It also often makes testing-turnaround times longer,
since updating the kernel can often be done by just scp'ing a new image
to the board, while updating the bootloader is sometimes a much more
involved procedure (not to mention that that's sometimes not even
possible to do remotely). There's also the issue of the command line
getting rather crowded if every aspect of the kernel has to be
configured at boot time.

As an embedded developer, I'm all for being able to override this
timeout as well lots of other kernel behavior via the command line, but
at the same time, I also prefer being able to set the default values in
the kernel's .config.

 but for something like an x86
> platform where you are deploying a standard vendor supplied kernel it's
> bad to do it that way IMHO.

A "standard vendor supplied kernel" will never set that CONFIG to
anything but its default - how could it possibly decide how long
userspace should have to open /dev/watchdog0, or even if userspace will
ever do that? Again, patch 3 doesn't in any way remove the possiblity of
setting this on the command line, and the end user is no worse off just
because the vendor had to keep a default setting.

> In other words I think you should drop patch 3 but the rest is good.

For the life of me, I cannot see the downside of patch 3, or how it's
any different from CONSOLE_LOGLEVEL_DEFAULT, BLK_DEV_RAM_SIZE, or any
number of other parameters-with-defaults-from-Kconfig.

Rasmus

[toc] | [prev] | [next] | [standalone]

#1649664

On 22 May 2017 at 16:06, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote:
>
> Rasmus Villemoes (3):
>   watchdog: introduce watchdog_worker_should_ping helper
>   watchdog: introduce watchdog.open_timeout commandline parameter
>   watchdog: introduce CONFIG_WATCHDOG_OPEN_TIMEOUT
>
>  Documentation/watchdog/watchdog-parameters.txt | 10 +++++++
>  drivers/watchdog/Kconfig                       |  9 +++++++
>  drivers/watchdog/watchdog_dev.c                | 37 +++++++++++++++++++++++---
>  3 files changed, 52 insertions(+), 4 deletions(-)


Thanks. This is definitely useful for embedded applications.

If this get merged, it should be able to replace several out-of-tree
hacks to accomplish the same thing.

For all 3 patches:
Reviewed-by: Esben Haabendal <esben@haabendal.dk>

/Esben

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web