Add path configuration for hardware watchdog device #7352

eddiejames · 2017-11-15T20:07:42Z

The hardware watchdog is currently hard-coded to /dev/watchdog0. This can create problems for systems with multiple watchdogs tied to different system behaviours.

This patch set modifies the hardware watchdog code to allow an index to be specified when opening, pinging, and setting the timeout of a hardware watchdog device. This index for the device number is configurable with the "WatchdogDeviceNum" systemd configuration option. The default device number is 0, so there should be no change of behaviour unless that option is specified. This takes effect for the runtime and shutdown watchdogs.

I tested both runtime and shutdown watchdogs on an AST2500 (small ARM core with three watchdogs, used as a baseboard management controller).

boucman · 2017-11-15T20:39:43Z

could you please document the new option ?

poettering · 2017-11-16T10:01:02Z

Hmpf, I am not sure I like this approach. Device indexes are essentially non-deterministic, they depend on the order of driver initialization, and in a world of USB and whatnot that order is very much undefined.

There has been a long-term TODO list item to rework the watchdog logic to consult udev for watchdog devices, add support for pinging multiple devices in parallel, all controlled by looking for devices tagged with some special udev tag. Of course this would mean that we wouldn't ping watchdog devices before udev probed the devices, but that's OK I think.

Altogether I think doing the udev thing would make this a lot more powerful, and people could choose explicitly which watchdogs they want systemd to ping and which ones not, simply by writing the appropriate udev rule that attaches the tag to right set of devices. By default we'd place the tag on all devices however.

Of course, doing such a rework is more work, but if we touch this code, we might really want to do this properly right-away instead of adding new configuration interfaces now that are likely going away again if we do the proper rework.

Any chance you could rework this based on udev device subscriptions?

g0tar · 2017-11-20T04:54:11Z

Of course this would mean that we wouldn't ping watchdog devices before udev probed the devices, but that's OK I think.

I don't think so. In a perfect world you could rely on udev, but in perfect world you wouldn't need any watchdog in the first place. This particular device is responsible for last-resort dealing with "any remaining bugs".

Consider scenarios:

systemd crashes early (due to anything, internal bugs, kernel bugs, failing hardware, cosmic rays) - no watchdog opened means no reboot,
BIOS has set OS boot timeout on watchdog, udev hangs on some device for a few minutes (let's say this is hw fault) - no watchdog opened means reboot despite the system could be started.

Watchdog is one of the very first things to be handled for better reliability. Putting a dependency on anything here is bad.

As for the changing device numbers - this in turn can be handled by system administrator well enough. Consider:

kernel with softdog build-it (always watchdog0) and any hardware watchdog handled by kernel module (always watchdog1),
multiple watchdog devices behind single module - they would be probably initialized in constant order until one mangles the hardware setup.

Anyway, watchdogs do not usually fall into "USB world" class with asynchronous initialization.

Moreover, even switching watchdogs after udev settle might be hard/impossible (nowayout? magic close?) so for this case there should be an exception allowing to bypass udev (module might be loaded manually or from modules.d, if not built-in); this device is much closer to the bare metal and deserves to be treated differently (just like boot loaders, no udev available yet).

And BTW, is there any way to query systemd about current watchdog status? As wdctl cannot open the device, there should be another interface provided.

poettering · 2017-11-27T12:01:17Z

I don't think so. In a perfect world you could rely on udev, but in perfect world you wouldn't need any watchdog in the first place. This particular device is responsible for last-resort dealing with "any remaining bugs".

Hmm, OK, I figure early watchdog is relevant for stability testing...

Hmm, I figure it would be OK to say that — as soon as the udev-based watchdog pinging is implemented – the set of watchdogs to ping is the combination of whatever is specified on the kernel cmdline and what carries the right udev tag.

Hence, I figure a patch your like yours can go in, however, semantically I think we should not base any of this on "device indexes", but at least device names. i.e. make the full path of the device node configurable, not the index only. That way the specific drivers can avoid the "unstable probing order problem", by picking more descriptive device names. i.e. the soft watchdog could use "/dev/softdog" or something like that, and things wouldn't be as undeterministic.

Could you please rework the patch accordingly?

poettering · 2017-11-27T12:04:12Z

src/shared/watchdog.c

+        if (asprintf(&watchdog_file, "/dev/watchdog%d", wd_number) < 0)
+                return -errno;
+
+        watchdog_fd = open(watchdog_file, O_WRONLY|O_CLOEXEC);


hmm, we currently maintain the watchdog_fd as a static variable. I figure the watchdog path should be maintained the same way, and be settable by a new call watchdog_set_path() or so, which is fed from the kernel cmdline and the config file.

poettering · 2017-11-27T12:05:29Z

also, this needs a rebase, currently does not apply to git

eddiejames · 2017-11-28T17:43:00Z

Thanks for the suggestions, I have rebased and updated my commits. Much cleaner now.

poettering · 2017-11-28T17:46:32Z

src/core/main.c

@@ -461,6 +462,15 @@ static int parse_proc_cmdline_item(const char *key, const char *value, void *dat
                if (arg_default_timeout_start_usec <= 0)
                        arg_default_timeout_start_usec = USEC_INFINITY;

+        } else if (proc_cmdline_key_streq(key, "systemd.watchdog_dev_path")) {


no unnecessary abbreviations. I think "systemd.watchdog_device" is a better name for this

poettering · 2017-11-28T17:50:32Z

src/core/main.c

+                if (proc_cmdline_value_missing(key, value))
+                        return 0;
+
+                r = parse_path(value, &arg_watchdog_device_path);


parse_path() does not exist?

Please test your stuff before pushing things. This can't possibly have compiled for you

My mistake here. I did test this, and fixed this in latest commits. My problem is I write/test with systemd 2.23 so I can test on hardware. I then have to rebase on master, I think I got the wrong branch with this bad code.

poettering · 2017-11-28T17:50:55Z

src/core/main.c

@@ -751,6 +761,7 @@ static int parse_config_file(void) {
                { "Manager", "JoinControllers",           config_parse_join_controllers, 0, &arg_join_controllers                  },
                { "Manager", "RuntimeWatchdogSec",        config_parse_sec,              0, &arg_runtime_watchdog                  },
                { "Manager", "ShutdownWatchdogSec",       config_parse_sec,              0, &arg_shutdown_watchdog                 },
+                { "Manager", "WatchdogDevicePath",        config_parse_path,             0, &arg_watchdog_device_path              },


just "WatchdogDevice" should be fine

poettering · 2017-11-28T17:51:12Z

src/core/main.c

@@ -1519,6 +1530,11 @@ static int become_shutdown(
                /* Tell the binary how often to ping, ignore failure */
                if (asprintf(&e, "WATCHDOG_USEC="USEC_FMT, arg_shutdown_watchdog) > 0)
                        (void) strv_push(&env_block, e);
+
+                if (arg_watchdog_device_path) {
+                        if (asprintf(&e, "WATCHDOG_DEV_PATH=%s", arg_watchdog_device_path) > 0)


WATCHDOG_DEVICE

poettering · 2017-11-28T17:52:39Z

src/core/main.c

@@ -120,6 +120,7 @@ static usec_t arg_default_start_limit_interval = DEFAULT_START_LIMIT_INTERVAL;
 static unsigned arg_default_start_limit_burst = DEFAULT_START_LIMIT_BURST;
 static usec_t arg_runtime_watchdog = 0;
 static usec_t arg_shutdown_watchdog = 10 * USEC_PER_MINUTE;
+static const char *arg_watchdog_device_path = NULL;


memory allocation is weird here... strings we allocate/free cannot be "const".

Please make this a normal "char*", and make sure we always allocate strings properly when assigning this variables. Also, please free this when shutting down. Not that freeing would strictly be necessary, but it's very helpful for valgrinding stuff.

Yes, latest commits I changed it to char *. I will add a call to free, thanks.

poettering · 2017-11-30T12:39:59Z

src/shared/watchdog.c

@@ -31,6 +31,7 @@
 #include "watchdog.h"

 static int watchdog_fd = -1;
+static const char *watchdog_dev = "/dev/watchdog";


uh, i really don't like the memory management for this. This should either always point to static memory, or always to dynamic memory. But right now it initially points to static memory, except when the user overrides it, in which case it points to externally owned dynamic memory, which might go away any time.

Please let's clean up memory management here, let's stick one kind of memory: dynamic. Specifically, change "const char*" to just "char*", and initialize this to NULL. When open_watchdog() sees this set to NULL it should use "/dev/watchdog" instead, i.e. the fallbcak should happen at the moment we read the variable, not already at initialization.

Then, make sure that watchdog_set_device() allocates a new string, and make sure there's watchdog_free_device() or so, that frees the string, and that is called at PID 1 exit, so that we are valgrind clean. Yes, this means we'll keep the string in memory twice (once in main.c's arg_watchdog_device, and once in this copy), but that's fine. I very much prefer clean memory owenership rules about minimizing copies.

Makes sense, will change, thanks.

poettering · 2017-11-30T12:40:36Z

src/shared/watchdog.c

@@ -96,6 +97,10 @@ static int open_watchdog(void) {
        return update_timeout();
 }

+void watchdog_set_path(const char *path) {
+	watchdog_dev = path;


as discussed above, this should do strdup(). And hence also be able to return an error...

And I figure this should be called watchdog_set_device() or so

poettering · 2017-11-30T12:41:04Z

src/core/main.c

@@ -120,6 +120,7 @@ static usec_t arg_default_start_limit_interval = DEFAULT_START_LIMIT_INTERVAL;
 static unsigned arg_default_start_limit_burst = DEFAULT_START_LIMIT_BURST;
 static usec_t arg_runtime_watchdog = 0;
 static usec_t arg_shutdown_watchdog = 10 * USEC_PER_MINUTE;
+static char *arg_watchdog_device_path = NULL;


just call this arg_watchdog_device so that the internal variable name and the externally visible config/kernel cmdline options match

poettering · 2017-11-30T12:42:43Z

src/core/main.c

+
+                r = parse_path_argument_and_warn(value, false, &arg_watchdog_device_path);
+                if (r < 0)
+                        log_warning_errno(r, "Failed to parse watchdog device path: %s, ignoring.", value);


parse_path_argument_and_warn() logs on its own (that's what the _and_warn suffix is supposed to say), no need to log here a second time, that'd just be confusing for the user

poettering · 2017-11-30T12:43:58Z

man/kernel-command-line.xml

@@ -382,6 +382,15 @@
        </listitem>
      </varlistentry>

+      <varlistentry>


Signed-off-by is a kernel thing btw, we don't use that in systemd, please drop

keszybz · 2017-12-01T11:13:21Z

src/shared/watchdog.c

@@ -84,7 +85,8 @@ static int open_watchdog(void) {
        if (watchdog_fd >= 0)
                return 0;

-        watchdog_fd = open("/dev/watchdog", O_WRONLY|O_CLOEXEC);
+        watchdog_fd = open(watchdog_dev ? watchdog_dev : "/dev/watchdog",


Please use watchdog_dev ?: "/dev/watchdog".

keszybz · 2017-12-01T11:14:04Z

src/shared/watchdog.c

@@ -96,6 +98,16 @@ static int open_watchdog(void) {
        return update_timeout();
 }

+int watchdog_set_device(char *path) {
+        watchdog_dev = strdup(path);


return free_and_strdup(&watchdog_dev, path);.

keszybz · 2017-12-01T11:14:27Z

src/shared/watchdog.c

+}
+
+void watchdog_free_device(void) {
+        free(watchdog_dev);


watchdog_dev = mfree(watchdog_dev).

Or even better, just remove this function and use watchdog_set_device(NULL) instead.

maybe make watchdog_free_device() an inline static wrapper in the header file, that internally just calls watchdog_set_device(NULL)

keszybz · 2017-12-01T11:20:50Z

src/core/main.c

+
+                if (arg_watchdog_device) {
+                        if (asprintf(&e, "WATCHDOG_DEVICE=%s", arg_watchdog_device) > 0)
+                                (void) strv_push(&env_block, e);


This code was already buggy, so not really your fault, but please fix it if you're touching it. This should be

if (asprintf(&e, ...) > 0) (void) strv_consume(&env_block, e); if (arg_watchdog_device && asprintf(&e, ...) > 0) (void) strv_consume(&env_block, e);

keszybz · 2017-12-01T11:21:23Z

src/core/main.c

@@ -1944,6 +1958,11 @@ int main(int argc, char *argv[]) {
                        test_usr();
                }

+                if (arg_system && arg_watchdog_device) {
+                        if (watchdog_set_device(arg_watchdog_device) < 0)
+                                log_error("Failed to set watchdog device %s.", arg_watchdog_device);


"set watchdog device to %s"?

please use:

if (…) { r = watchdog_set_device(…); if (r < 0) log_warning_errno(r, "Failed to set watchdog device to %s, ignoring: %m", …); }

keszybz · 2017-12-01T11:22:51Z

src/core/shutdown.c

+        watchdog_device = getenv("WATCHDOG_DEVICE");
+        if (watchdog_device) {
+                if (watchdog_set_device(watchdog_device) < 0)
+                        log_error("Failed to set watchdog device %s.", watchdog_device);


keszybz · 2017-12-01T11:25:03Z

man/kernel-command-line.xml

+        <term><varname>systemd.watchdog_device=</varname></term>
+
+        <listitem>
+          <para>Overwrites the watchdog device path <varname>WatchdogDevice=</varname> at boot. For details, see


I'd drop "at boot" here. We have various options which apply only at boot, and this one applies even after boot, and in particular during shutdown.

keszybz · 2017-12-01T11:26:50Z

man/systemd-system.conf.xml

@@ -161,8 +161,9 @@
        <literal>d</literal>, <literal>w</literal>). If
        <varname>RuntimeWatchdogSec=</varname> is set to a non-zero
        value, the watchdog hardware
-        (<filename>/dev/watchdog</filename>) will be programmed to
-        automatically reboot the system if it is not contacted within
+        (<filename>/dev/watchdog</filename> or the path to which


"<filename>/dev/watchdog</filename> or the path specified with <varname>WatchdogDevice=</varname> or the kernel option <varname>systemd.watchdog-device=</varname>"

poettering · 2017-12-01T11:34:26Z

src/shared/watchdog.c

@@ -31,6 +31,7 @@
 #include "watchdog.h"

 static int watchdog_fd = -1;
+static char *watchdog_dev = NULL;


I'd not abbreviate this either, just call it watchdog_device, so that the same name is used everywhere: externally, internally, in functions, in global variables, and so on.

poettering · 2017-12-01T11:35:16Z

src/shared/watchdog.c

+}
+
+void watchdog_free_device(void) {
+        free(watchdog_dev);


maybe make watchdog_free_device() an inline static wrapper in the header file, that internally just calls watchdog_set_device(NULL)

poettering · 2017-12-01T11:37:41Z

src/core/main.c

@@ -1944,6 +1958,11 @@ int main(int argc, char *argv[]) {
                        test_usr();
                }

+                if (arg_system && arg_watchdog_device) {
+                        if (watchdog_set_device(arg_watchdog_device) < 0)
+                                log_error("Failed to set watchdog device %s.", arg_watchdog_device);


please use:

if (…) { r = watchdog_set_device(…); if (r < 0) log_warning_errno(r, "Failed to set watchdog device to %s, ignoring: %m", …); }

eddiejames · 2017-12-07T20:29:03Z

Any more updates requested on this one?

poettering · 2017-12-08T16:42:47Z

ah, we don't get notified in some cases if you push a new version. Please always write a comment as well, otherwise we might not notice, like in this case.

poettering · 2017-12-08T16:44:31Z

patch looks excellent now, but needs to be rebased, main.c has been rearranged quite a bit...

Currently systemd hardcodes the use of /dev/watchdog. This is a legacy chardev that points to watchdog0 in the system. Modify the watchdog API to allow a different device path to be passed and stored. Opening the watchdog defaults to /dev/watchdog, maintaining existing behavior.

This option allows a device path to be specified for the systemd watchdog (both runtime and shutdown). If a system requires a watchdog other than /dev/watchdog (pointing to /dev/watchdog0) to be used to reboot the system, this setting should be changed to the relevant watchdog device path (e.g. /dev/watchdog1).

Document the command line parameter and the system configuration file setting.

eddiejames · 2017-12-08T17:43:48Z

Great, thanks, I just rebased.

poettering · 2017-12-08T18:26:44Z

lgtm

upstream: systemd/systemd#7352 Change-Id: I62df65eeec786890c0a7b923fd0455f7869f38b9 Signed-off-by: Edward A. James <eajames@us.ibm.com>

poettering added needs-discussion 🤔 pid1 labels Nov 16, 2017

amboar mentioned this pull request Nov 19, 2017

Enable use of watchdog within systemd in userspace openbmc/openbmc#2427

Closed

poettering added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed needs-discussion 🤔 labels Nov 27, 2017

poettering requested changes Nov 27, 2017

View reviewed changes

eddiejames force-pushed the master branch from 3148764 to 56a3968 Compare November 28, 2017 17:40

eddiejames changed the title ~~Add index for hardware watchdog device~~ Add path configuration for hardware watchdog device Nov 28, 2017

eddiejames force-pushed the master branch from 56a3968 to 231a114 Compare November 28, 2017 20:29

poettering requested changes Nov 30, 2017

View reviewed changes

eddiejames force-pushed the master branch from 231a114 to a524ffc Compare November 30, 2017 22:49

keszybz requested changes Dec 1, 2017

View reviewed changes

poettering requested changes Dec 1, 2017

View reviewed changes

eddiejames force-pushed the master branch from a524ffc to 20cf30d Compare December 1, 2017 17:11

poettering added needs-rebase and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Dec 8, 2017

eddiejames added 3 commits December 8, 2017 11:26

documentation: add description for watchdog device path

c75396c

Document the command line parameter and the system configuration file setting.

eddiejames force-pushed the master branch from 20cf30d to c75396c Compare December 8, 2017 17:27

poettering added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed needs-rebase labels Dec 8, 2017

poettering merged commit f7757a4 into systemd:master Dec 8, 2017

geissonator pushed a commit to openbmc/openbmc that referenced this pull request Apr 10, 2018

systemd: patch in watchdog device configuration change

aa111ab

upstream: systemd/systemd#7352 Change-Id: I62df65eeec786890c0a7b923fd0455f7869f38b9 Signed-off-by: Edward A. James <eajames@us.ibm.com>

Add path configuration for hardware watchdog device #7352

Add path configuration for hardware watchdog device #7352

Conversation

eddiejames commented Nov 15, 2017

boucman commented Nov 15, 2017

poettering commented Nov 16, 2017

g0tar commented Nov 20, 2017

poettering commented Nov 27, 2017

Choose a reason for hiding this comment

poettering commented Nov 27, 2017

eddiejames commented Nov 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddiejames commented Dec 7, 2017

poettering commented Dec 8, 2017

poettering commented Dec 8, 2017

eddiejames commented Dec 8, 2017

poettering commented Dec 8, 2017