Re: [systemd-devel] [PATCH] watchdog: Don't require WDIOC_SETOPTIONS/WDIOS_ENABLECARD
On Mon, 15.06.15 18:14, Jean Delvare (jdelv...@suse.de) wrote: Not all watchdog drivers implement WDIOC_SETOPTIONS. Drivers which do not implement it have their device always enabled. So it's fine to report an error if WDIOS_DISABLECARD is passed and the ioctl is not implemented, however failing when WDIOS_ENABLECARD is passed and the ioctl is not implemented is not good: if the device was already enabled then WDIOS_ENABLECARD was a no-op and wasn't needed in the first place. So we can just ignore the error and continue. Isn't this something that should be fixed in the drivers? --- src/shared/watchdog.c |3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) --- a/src/shared/watchdog.c +++ b/src/shared/watchdog.c @@ -64,7 +64,8 @@ static int update_timeout(void) { flags = WDIOS_ENABLECARD; r = ioctl(watchdog_fd, WDIOC_SETOPTIONS, flags); -if (r 0) { +/* ENOTTY means the watchdog is always enabled so we're fine */ +if (r 0 errno != ENOTTY) { log_warning(Failed to enable hardware watchdog: %m); return -errno; If this is something to fix in systemd, rather than fix in the drivers: am pretty sure that we should log in all cases, but change the log level to LOG_DEBUG if its ENOTTY. i.e. use log_full(errno == ENOTTY ? LOG_DEBUG : LOG_WARNING... Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH] watchdog: Don't require WDIOC_SETOPTIONS/WDIOS_ENABLECARD
Not all watchdog drivers implement WDIOC_SETOPTIONS. Drivers which do not implement it have their device always enabled. So it's fine to report an error if WDIOS_DISABLECARD is passed and the ioctl is not implemented, however failing when WDIOS_ENABLECARD is passed and the ioctl is not implemented is not good: if the device was already enabled then WDIOS_ENABLECARD was a no-op and wasn't needed in the first place. So we can just ignore the error and continue. --- src/shared/watchdog.c |3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) --- a/src/shared/watchdog.c +++ b/src/shared/watchdog.c @@ -64,7 +64,8 @@ static int update_timeout(void) { flags = WDIOS_ENABLECARD; r = ioctl(watchdog_fd, WDIOC_SETOPTIONS, flags); -if (r 0) { +/* ENOTTY means the watchdog is always enabled so we're fine */ +if (r 0 errno != ENOTTY) { log_warning(Failed to enable hardware watchdog: %m); return -errno; } -- Jean Delvare SUSE L3 Support ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] watchdog: Don't require WDIOC_SETOPTIONS/WDIOS_ENABLECARD
Hi Lennart, Thanks for your quick reply. Le Monday 15 June 2015 à 18:16 +0200, Lennart Poettering a écrit : On Mon, 15.06.15 18:14, Jean Delvare (jdelv...@suse.de) wrote: Not all watchdog drivers implement WDIOC_SETOPTIONS. Drivers which do not implement it have their device always enabled. So it's fine to report an error if WDIOS_DISABLECARD is passed and the ioctl is not implemented, however failing when WDIOS_ENABLECARD is passed and the ioctl is not implemented is not good: if the device was already enabled then WDIOS_ENABLECARD was a no-op and wasn't needed in the first place. So we can just ignore the error and continue. Isn't this something that should be fixed in the drivers? This is a legitimate question and I pondered it myself too. However the fact that over 20 drivers do not implement WDIOC_SETOPTIONS, together with the fact that it's named, well, set options, give me the feeling that not implementing it is legitimate. --- src/shared/watchdog.c |3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) --- a/src/shared/watchdog.c +++ b/src/shared/watchdog.c @@ -64,7 +64,8 @@ static int update_timeout(void) { flags = WDIOS_ENABLECARD; r = ioctl(watchdog_fd, WDIOC_SETOPTIONS, flags); -if (r 0) { +/* ENOTTY means the watchdog is always enabled so we're fine */ +if (r 0 errno != ENOTTY) { log_warning(Failed to enable hardware watchdog: %m); return -errno; If this is something to fix in systemd, rather than fix in the drivers: am pretty sure that we should log in all cases, but change the log level to LOG_DEBUG if its ENOTTY. i.e. use log_full(errno == ENOTTY ? LOG_DEBUG : LOG_WARNING... Sure, I can try that. Updated patch coming (likely tomorrow.) Thanks for the review, -- Jean Delvare SUSE L3 Support ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel