watchdog: add sun4v_wdt device support
Guenter Roeck
maintainerQuoted 5 lines · On Wed, Jan 20, 2016 at 12:30:44PM -0800, [email protected] wrote:
From: Wim Coekaerts <[email protected]>
This adds a simple watchdog timer for the SPARC sunv4 architecture. Export the
sun4v_mach_set_watchdog()hv call, and add the target.
What is "the target" ?
Quoted 2 lines
The driver is implemented using the new watchdog api framework.
Not really new ...
Quoted 3 lines
Signed-off-by Wim Coekaerts <[email protected]>---
Please version your patches and provide a changelog here.
Quoted 13 lines
Documentation/watchdog/watchdog-parameters.txt | 5 + arch/sparc/kernel/sparc_ksyms_64.c | 1 + drivers/watchdog/Kconfig | 9 + drivers/watchdog/Makefile | 1 + drivers/watchdog/sun4v_wdt.c | 323 ++++++++++++++++++++++++ 5 files changed, 339 insertions(+), 0 deletions(-) create mode 100644 drivers/watchdog/sun4v_wdt.cDocumentation/watchdog/watchdog-parameters.txt +0
-0
Documentation/watchdog/watchdog-parameters.txt wm8350_wdt: · line 407
Quoted 8 lines
Documentation/watchdog/watchdog-parameters.txt +5
-0@@ -400,3 +400,8 @@ wm8350_wdt: nowayout: Watchdog cannot be stopped once started (default=kernel config parameter) ------------------------------------------------- +sun4v_wdt: +timeout: Watchdog timeout in seconds (1..31536000, default`) +nowayout: Watchdog cannot be stopped once started +------------------------------------------------- +
Blank line at end of file causes warning when applying the patch.
Quoted 19 lines
arch/sparc/kernel/sparc_ksyms_64.c +1
-0@@ -37,6 +37,7 @@ EXPORT_SYMBOL(sun4v_niagara_getperf);EXPORT_SYMBOL (sun4v_niagara_setperf);EXPORT_SYMBOL (sun4v_niagara2_getperf);EXPORT_SYMBOL (sun4v_niagara2_setperf); +EXPORT_SYMBOL (sun4v_mach_set_watchdog);/* from hweight.S */ EXPORT_SYMBOL (__arch_hweight8);drivers/watchdog/Kconfig +0
-0@@ -1500,6 +1500,15 @@ config WATCHDOG_RIO machines. The watchdog timeout period is normally one minute but can be changed with a boot-time parameter.
drivers/watchdog/Kconfig config WATCHDOG_RIO · line 1511
Quoted 10 lines
drivers/watchdog/Kconfig +9
-0+config WATCHDOG_SUN4V + tristate "sun4v Watchdog support" + select WATCHDOG_CORE + depends on SPARC64 + help + Say Y/M here to support the hypervisor watchdog capability provided + by Oracle VM for SPARC. The watchdog timeout period is normally one + minute but can be changed with a boot-time parameter. +
Please add customary "This driver can also be built as a module. If so, the module will be called ..."
Quoted 21 lines
drivers/watchdog/Kconfig +0
-0# XTENSA Architecture # Xen Architecturedrivers/watchdog/Makefile +1
-0@@ -175,6 +175,7 @@ obj-$(CONFIG_SH_WDT) += shwdt.o obj-$(CONFIG_WATCHDOG_RIO) += riowd.o obj-$(CONFIG_WATCHDOG_CP1XXX) += cpwd.o +obj-$(CONFIG_WATCHDOG_SUN4V) += sun4v_wdt.o # XTENSA Architecturedrivers/watchdog/sun4v_wdt.c new file +0
-0
drivers/watchdog/sun4v_wdt.c line 10
Quoted 10 lines
drivers/watchdog/sun4v_wdt.c new file +10
-0@@ -0,0 +1,323 @@ +/* +* sun4v watchdog timer +* (c) Copyright 2016 Oracle Corporation +* +* Implement a simple watchdog driver using the sun4v hypervisor +* watchdog support. If time expires, the hypervisor stops or bounces +* the guest domain. +* +* sun4v_mach_set_watchdog() expects time in ms +*
Unusual location for an API definition.
drivers/watchdog/sun4v_wdt.c line 21
Quoted 11 lines
drivers/watchdog/sun4v_wdt.c new file +11
-0+* 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 Free Software Foundation; either version +* 2 of the License, or (at your option) any later version. +*/ + +#define pr_fmt (fmt) KBUILD_MODNAME ": " fmt + +#define DRV_NAME "sun4v_wdt" +#define DRV_VERSION "0.1" +
I would suggest to drop the version number. It doesn't really provide any value, and history suggests that no one will be updating it in the future.
drivers/watchdog/sun4v_wdt.c line 22
Quoted 1 line
drivers/watchdog/sun4v_wdt.c new file +1
-0+#include <linux/bug.h>
Is this needed ? I don't see where.
Quoted 21 lines
drivers/watchdog/sun4v_wdt.c new file +21
-0+#include <linux/errno.h> +#include <linux/hrtimer.h> +#include <linux/init.h> +#include <linux/kernel.h> +#include <linux/ktime.h> +#include <linux/module.h> +#include <linux/moduleparam.h> +#include <linux/platform_device.h> +#include <linux/spinlock.h> +#include <linux/watchdog.h> +#include <asm/hypervisor.h> +#include <asm/mdesc.h> + +#define WATCHDOG_TIMEOUT 60/* in seconds */ +#define WDT_MAX_TIMEOUT 31536000/* in seconds */ +#define WDT_MIN_TIMEOUT 1/* in seconds */ + +static struct platform_device *platform_device; + +struct sun4v_wdt { +spinlock_t lock;
drivers/watchdog/sun4v_wdt.c line 53
Quoted 10 lines
drivers/watchdog/sun4v_wdt.c new file +10
-0+__kernel_time_t expires; +struct watchdog_device wdd; +}; + +static unsigned int max_timeout =WDT_MAX_TIMEOUT ; +static unsigned int timeout =WATCHDOG_TIMEOUT ; +module_param (timeout, uint,S_IRUGO ); +MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds " + "(default=" __MODULE_STRING(WATCHDOG_TIMEOUT) ")"); +
checkpatch warning.
drivers/watchdog/sun4v_wdt.c line 57
Quoted 4 lines
drivers/watchdog/sun4v_wdt.c new file +4
-0+static bool nowayout =WATCHDOG_NOWAYOUT ; +module_param (nowayout, bool,S_IRUGO ); +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started " + "(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
checkpatch warning.
drivers/watchdog/sun4v_wdt.c line 69
Quoted 12 lines
drivers/watchdog/sun4v_wdt.c new file +12
-0+ +static int sun4v_wdt_start (struct watchdog_device *wdd) +{ +struct sun4v_wdt *wdt =watchdog_get_drvdata (wdd); +unsigned long time_remaining; +int err; + +spin_lock (&wdt->lock); + + wdt->expires =ktime_to_timespec (ktime_get ()).tv_sec + wdd->timeout; + err =sun4v_mach_set_watchdog (wdd->timeout *1000 , &time_remaining); +
time_remaining seems to be unused. Can it be dropped ?
drivers/watchdog/sun4v_wdt.c line 71
Quoted 2 lines
drivers/watchdog/sun4v_wdt.c new file +2
-0+spin_unlock (&wdt->lock); +
Why is this extra locking (on top of the locking by the watchdog core) needed ?
drivers/watchdog/sun4v_wdt.c line 72
Quoted 1 line
drivers/watchdog/sun4v_wdt.c new file +1
-0+pr_info ("timer enabled\n" );
Please no such noise. This is normal kernel operation.
drivers/watchdog/sun4v_wdt.c line 85
Quoted 13 lines
drivers/watchdog/sun4v_wdt.c new file +13
-0+return err; +} + +static int sun4v_wdt_stop (struct watchdog_device *wdd) +{ +struct sun4v_wdt *wdt =watchdog_get_drvdata (wdd); +unsigned long time_remaining; +int err; + +spin_lock (&wdt->lock); + + err =sun4v_mach_set_watchdog (0 , &time_remaining); +
Non-standard error code leaking to caller (just an example).
drivers/watchdog/sun4v_wdt.c line 88
Quoted 3 lines
drivers/watchdog/sun4v_wdt.c new file +3
-0+spin_unlock (&wdt->lock); + +pr_info ("timer disabled\n" );
Please drop thie message.
Quoted 9 lines
drivers/watchdog/sun4v_wdt.c new file +9
-0+return err; +} + +static int sun4v_wdt_ping (struct watchdog_device *wdd) +{ +struct sun4v_wdt *wdt =watchdog_get_drvdata (wdd); +int err; +unsigned long time_remaining; +
drivers/watchdog/sun4v_wdt.c line 107
Quoted 10 lines
drivers/watchdog/sun4v_wdt.c new file +10
-0+spin_lock (&wdt->lock); + + wdt->expires =ktime_to_timespec (ktime_get ()).tv_sec + wdd->timeout; + err =sun4v_mach_set_watchdog (wdd->timeout *1000 , &time_remaining); + +spin_unlock (&wdt->lock); + +return err; +} +
Am I missing something, or is the start function identical to the stop function ? If so, why have both ?
drivers/watchdog/sun4v_wdt.c line 115
Quoted 8 lines
drivers/watchdog/sun4v_wdt.c new file +8
-0+static int sun4v_wdt_set_timeout (struct watchdog_device *wdd, +unsigned int timeout) +{ + wdd->timeout = timeout; + +if (watchdog_active (wdd)) { + (void )sun4v_wdt_stop (wdd); +return sun4v_wdt_start (wdd);
Is it really necessary to stop the watchdog before updating the timer ? Can't you just set the new timeout like in the ping function ?
Also, since the calling code executes ping, is this even necessary ?
drivers/watchdog/sun4v_wdt.c line 126
Quoted 11 lines
drivers/watchdog/sun4v_wdt.c new file +11
-0+ } + +return 0 ; +} + +static unsigned int sun4v_wdt_get_timeleft (struct watchdog_device *wdd) +{ +struct sun4v_wdt *wdt =watchdog_get_drvdata (wdd); + +return wdt->expires -ktime_to_timespec (ktime_get ()).tv_sec; +}
get_timeleft() is supposed to return the time left from a hardware register (if available). If we wanted to implement a "soft" version of get_timeleft(), it should be done in the watchdog core, not in individual drivers. Please drop (and with it the 'expires' variable).
Quoted 31 lines
drivers/watchdog/sun4v_wdt.c new file +31
-0+ +static const struct watchdog_info sun4v_wdt_ident = { + .options =WDIOF_SETTIMEOUT |WDIOF_MAGICCLOSE , + .identity ="sun4v watchdog" , + .firmware_version =0 , +}; + +static struct watchdog_ops sun4v_wdt_ops = { + .owner =THIS_MODULE , + .start = sun4v_wdt_start, + .stop = sun4v_wdt_stop, + .ping = sun4v_wdt_ping, + .set_timeout = sun4v_wdt_set_timeout, + .get_timeleft = sun4v_wdt_get_timeleft, +}; + +static int sun4v_wdt_probe (struct platform_device *pdev) +{ +struct watchdog_device *wdd; +struct sun4v_wdt *wdt; +unsigned long time_remaining; +int ret =0 ; + + wdt =devm_kzalloc (&pdev->dev,sizeof (*wdt),GFP_KERNEL ); +if (!wdt) +return -ENOMEM ; + + wdd = &wdt->wdd; + wdd->info = &sun4v_wdt_ident; + wdd->ops = &sun4v_wdt_ops; + wdd->min_timeout =WDT_MIN_TIMEOUT ;
drivers/watchdog/sun4v_wdt.c line 167
Quoted 10 lines
drivers/watchdog/sun4v_wdt.c new file +10
-0+ wdd->max_timeout = max_timeout; + wdd->timeout = timeout; + wdd->parent = &pdev->dev; + +watchdog_set_drvdata (wdd, wdt); + +spin_lock_init (&wdt->lock); + + ret =sun4v_mach_set_watchdog (wdd->timeout, &time_remaining); + (void )sun4v_mach_set_watchdog (0 , &time_remaining);
Why first set (and enable) the watchdog just to disable it immediately afterwards ? Just to check if it can be set ? Is that really necessary ? Can't you just set it to 0 (disable it) and bail out if that does not work ?
drivers/watchdog/sun4v_wdt.c line 168
Quoted 1 line
drivers/watchdog/sun4v_wdt.c new file +1
-0+if (ret !=HV_EOK ) {
Please convert non-standard error numbers to standard error numbers in the low level functions. If you look closely into your code, the non-standard error codes are leaking to the watchdog core and thus to the user.
drivers/watchdog/sun4v_wdt.c line 169
Quoted 1 line
drivers/watchdog/sun4v_wdt.c new file +1
-0+pr_info ("Error setting timer\n" );
If this is an error, it should be an error message.
drivers/watchdog/sun4v_wdt.c line 170
Quoted 1 line
drivers/watchdog/sun4v_wdt.c new file +1
-0+ ret = -ENODEV ;
If it means the device does not exist, there should be no message. We don't want noise for systems which don't support the watchdog but are instantiated for some reason.
drivers/watchdog/sun4v_wdt.c line 178
Quoted 8 lines
drivers/watchdog/sun4v_wdt.c new file +8
-0+ goto out; + } + +watchdog_set_nowayout (wdd, nowayout); + + ret =watchdog_register_device (wdd); +if (ret) { +pr_err ("Failed to register watchdog device (%d)\n" , ret);
Please use dev_info/dev_err where possible.
drivers/watchdog/sun4v_wdt.c line 186
Quoted 8 lines
drivers/watchdog/sun4v_wdt.c new file +8
-0+ goto out; + } + +platform_set_drvdata (pdev, wdt); + +pr_info ("initialized (timeout=%ds, nowayout=%d)\n" , + wdd->timeout, nowayout); +out:
This label, and any jump to it, is unnecessary. Instead of "goto out;", just return the error immediately.
Quoted 9 lines
drivers/watchdog/sun4v_wdt.c new file +9
-0+return ret; +} + +static int sun4v_wdt_remove (struct platform_device *pdev) +{ +struct sun4v_wdt *wdt =platform_get_drvdata (pdev); + + (void )sun4v_wdt_stop (&wdt->wdd); +
drivers/watchdog/sun4v_wdt.c line 205
Quoted 10 lines
drivers/watchdog/sun4v_wdt.c new file +10
-0+watchdog_unregister_device (&wdt->wdd); + +return 0 ; +} + +static void sun4v_wdt_shutdown (struct platform_device *pdev) +{ +struct sun4v_wdt *wdt =platform_get_drvdata (pdev); + + (void )sun4v_wdt_stop (&wdt->wdd);
The (void) is unnecessary here.
drivers/watchdog/sun4v_wdt.c line 212
Quoted 7 lines
drivers/watchdog/sun4v_wdt.c new file +7
-0+} + +static int sun4v_wdt_suspend (struct platform_device *pdev,pm_message_t state) +{ +struct sun4v_wdt *wdt =platform_get_drvdata (pdev); + +return sun4v_wdt_stop (&wdt->wdd);
Even if the watchdog isn't running ?
drivers/watchdog/sun4v_wdt.c line 219
Quoted 7 lines
drivers/watchdog/sun4v_wdt.c new file +7
-0+} + +static int sun4v_wdt_resume (struct platform_device *pdev) +{ +struct sun4v_wdt *wdt =platform_get_drvdata (pdev); + +return sun4v_wdt_start (&wdt->wdd);
Even if the watchdog wasn't running before ?
drivers/watchdog/sun4v_wdt.c line 227
Quoted 8 lines
drivers/watchdog/sun4v_wdt.c new file +8
-0+} + +static struct platform_driver sun4v_wdt_driver = { + .probe = sun4v_wdt_probe, + .remove = sun4v_wdt_remove, + .shutdown = sun4v_wdt_shutdown, + .suspend = sun4v_wdt_suspend, + .resume = sun4v_wdt_resume,
Please use driver pm operations.
Quoted 16 lines
drivers/watchdog/sun4v_wdt.c new file +16
-0+ .driver = { + .name = DRV_NAME, + }, +}; + +static int __init sun4v_wdt_init_module (void ) +{ +int err; +struct mdesc_handle *handle; +u64 node; +const u64 *value; +u64 resolution; + +/* +* There are 2 properties that can be set from the control +* domain for the watchdog.
drivers/watchdog/sun4v_wdt.c line 253
Quoted 10 lines
drivers/watchdog/sun4v_wdt.c new file +10
-0+* watchdog-resolution (in ms defaulting to 1000) +* watchdog-max-timeout (in ms) +* Right now, only support the default 1s (1000ms) resolution +* so just verify against the property, and make sure +* max timeout is taken into account, if set. +*/ + handle =mdesc_grab (); +if (!handle) +return -ENODEV ; +
Is there some means to determine if this is a SUN4V system ? The detections used (like this one, and the attempt to set the watchdog in the probe function) seem to be a bit shaky.
drivers/watchdog/sun4v_wdt.c line 256
Quoted 3 lines
drivers/watchdog/sun4v_wdt.c new file +3
-0+ node =mdesc_node_by_name (handle,MDESC_NODE_NULL ,"platform" ); +if (node =MDESC_NODE_NULL ) { +pr_info ("No platform node\n" );
Is this an error, or does it just indicate that the watchdog is not supported ion this platform ? If it is an error, use pr_err(). If it means the watchdog is not supported, return without message.
drivers/watchdog/sun4v_wdt.c line 267
Quoted 11 lines
drivers/watchdog/sun4v_wdt.c new file +11
-0+ err = -ENODEV ; + goto out; + } + + value =mdesc_get_property (handle, node,"watchdog-resolution" ,NULL ); +if (value) { + resolution = *value; +pr_info ("Platform watchdog-resolution [%llux]\n" , *value); + +if (resolution !=1000 ) { +pr_crit ("Only 1000ms is supported.\n" );
Why is this critical ? Seems to be an implementation problem.
drivers/watchdog/sun4v_wdt.c line 270
Quoted 3 lines
drivers/watchdog/sun4v_wdt.c new file +3
-0+ err = -EINVAL ; + goto out; + }
I don't entirely follow why this restriction is necessary. Why not just support arbitrary resolutions, especially since you consider other resolutions to be a critical problem ?
drivers/watchdog/sun4v_wdt.c line 276
Quoted 6 lines
drivers/watchdog/sun4v_wdt.c new file +6
-0+ } + + value =mdesc_get_property (handle, node,"watchdog-max-timeout" ,NULL ); +if (value) { +/* convert ms to seconds */ + max_timeout = *value /1000 ;
This can overflow.
drivers/watchdog/sun4v_wdt.c line 282
Quoted 6 lines
drivers/watchdog/sun4v_wdt.c new file +6
-0+pr_info ("Platform watchdog-max-timeout [%ds]\n" , max_timeout); + +if (max_timeout <WDT_MIN_TIMEOUT ) { + max_timeout =WDT_MIN_TIMEOUT ; +pr_info ("Setting max timeout to [%ds]\n" , max_timeout); + }
This is kind of odd. If the platform specifies a smaller maximum timeout than the pre-defined minimum, and you can just override that value, why care in the first place ?
Also, WDT_MIN_TIMEOUT is 1 (second), meaning you would set the maximum timeout to 1 second, and the default timeout would end up being invalid. Can you try to define more reasonable acceptable limits ?
drivers/watchdog/sun4v_wdt.c line 286
Quoted 4 lines
drivers/watchdog/sun4v_wdt.c new file +4
-0+ +if (max_timeout >WDT_MAX_TIMEOUT ) { + max_timeout =WDT_MAX_TIMEOUT ; +pr_info ("Setting max timeout to [%ds]\n" , max_timeout);
Is WDT_MAX_TIMEOUT an absolute or an arbitrary limit ?
drivers/watchdog/sun4v_wdt.c line 291
Quoted 5 lines
drivers/watchdog/sun4v_wdt.c new file +5
-0+ } + } + +pr_info ("sun4v watchdog timer v%s\n" ,DRV_VERSION ); +
Please drop this message. The message in the probe function is sufficient (and may already be considered noise).
drivers/watchdog/sun4v_wdt.c line 294
Quoted 3 lines
drivers/watchdog/sun4v_wdt.c new file +3
-0+ err =platform_driver_register (&sun4v_wdt_driver); +if (err) +return err;
no mdesc_release here ?
drivers/watchdog/sun4v_wdt.c line 297
Quoted 3 lines
drivers/watchdog/sun4v_wdt.c new file +3
-0+ + platform_device =platform_device_register_simple (DRV_NAME , +-1 ,NULL ,0 );
Odd alignment. Please run your patch through checkpatch --strict and make sure that alignments are correct.
drivers/watchdog/sun4v_wdt.c line 305
Quoted 8 lines
drivers/watchdog/sun4v_wdt.c new file +8
-0+if (IS_ERR (platform_device)) { + err =PTR_ERR (platform_device); +platform_driver_unregister (&sun4v_wdt_driver); + } + +out: +if (handle) +mdesc_release (handle);
You never get here if handle = NULL.
drivers/watchdog/sun4v_wdt.c line 314
Quoted 9 lines
drivers/watchdog/sun4v_wdt.c new file +9
-0+ +return err; +} + +static void __exit sun4v_wdt_cleanup_module(void) +{ +platform_device_unregister (platform_device); +platform_driver_unregister (&sun4v_wdt_driver); +pr_info ("module unloaded\n" );
Please no such noise.
drivers/watchdog/sun4v_wdt.c line 318
Quoted 4 lines
drivers/watchdog/sun4v_wdt.c new file +4
-0+} + +module_init (sun4v_wdt_init_module); +module_exit (sun4v_wdt_cleanup_module);
Wonder if it would be better to move the initialization into the probe function and use module_patform_driver(), or module_platform_driver_probe(). Any reason for not doing that ?
drivers/watchdog/sun4v_wdt.c line 323
Quoted 5 lines
drivers/watchdog/sun4v_wdt.c new file +5
-0+ +MODULE_AUTHOR ("Wim Coekaerts <[email protected]>" ); +MODULE_DESCRIPTION ("sun4v watchdog timer" ); +MODULE_VERSION (DRV_VERSION ); +MODULE_LICENSE ("GPL" );
No MODULE_ALIAS ?
Quoted 7 lines
Signature · 6 lines
1.7.1 -- To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks for your feedback, I think I am close ;) a few comments/questions
Quoted 13 lines
Quoted 13 lines
(quoted diff) +13
-0+ return err; +} + +static int sun4v_wdt_ping(struct watchdog_device *wdd) +{ + struct sun4v_wdt *wdt = watchdog_get_drvdata(wdd); + int err; + unsigned long time_remaining; + + spin_lock(&wdt->lock); + + wdt->expires = ktime_to_timespec(ktime_get()).tv_sec + wdd->timeout; + err = sun4v_mach_set_watchdog(wdd->timeout * 1000, &time_remaining);
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ + spin_unlock(&wdt->lock); + + return err; +} +Am I missing something, or is the start function identical to the stop function ? If so, why have both ?
fair - I am consolidating ping/start into ping
Quoted 12 lines
Review
Quoted 9 lines
(quoted diff) +8
-0+static int sun4v_wdt_set_timeout(struct watchdog_device *wdd, + unsigned int timeout) +{ + wdd->timeout = timeout; + + if (watchdog_active(wdd)) { + (void) sun4v_wdt_stop(wdd); + return sun4v_wdt_start(wdd);Is it really necessary to stop the watchdog before updating the timer ? Can't you just set the new timeout like in the ping function ?
Quoted 1 line
Also, since the calling code executes ping, is this even necessary ?
Yeah my bad, I should have known this. No need to do this.
Quoted 16 lines
Review
Quoted 12 lines
(quoted diff) +11
-0+ } + + return 0; +} + +static unsigned int sun4v_wdt_get_timeleft(struct watchdog_device *wdd) +{ + struct sun4v_wdt *wdt = watchdog_get_drvdata(wdd); + + return wdt->expires - ktime_to_timespec(ktime_get()).tv_sec; +}
get_timeleft()is supposed to return the time left from a hardware register (if available). If we wanted to implement a "soft" version ofget_timeleft(), it should be done in the watchdog core, not in individual drivers. Please drop (and with it the 'expires' variable).
Ok no problem, I do think it's useful even if the hardware doesn't support it. To have an idea of how much time is left, I guess one could implement it in the code using the watchdog but I think it's reasonable to provide support in the driver or core. Would you implement a new op for this in core?
Anyway, I dropped it from my driver.
Quoted 39 lines
Quoted 39 lines
(quoted diff) +38
-0+ +static const struct watchdog_info sun4v_wdt_ident = { + .options = WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE, + .identity = "sun4v watchdog", + .firmware_version = 0, +}; + +static struct watchdog_ops sun4v_wdt_ops = { + .owner = THIS_MODULE, + .start = sun4v_wdt_start, + .stop = sun4v_wdt_stop, + .ping = sun4v_wdt_ping, + .set_timeout = sun4v_wdt_set_timeout, + .get_timeleft = sun4v_wdt_get_timeleft, +}; + +static int sun4v_wdt_probe(struct platform_device *pdev) +{ + struct watchdog_device *wdd; + struct sun4v_wdt *wdt; + unsigned long time_remaining; + int ret = 0; + + wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); + if (!wdt) + return -ENOMEM; + + wdd = &wdt->wdd; + wdd->info = &sun4v_wdt_ident; + wdd->ops = &sun4v_wdt_ops; + wdd->min_timeout = WDT_MIN_TIMEOUT; + wdd->max_timeout = max_timeout; + wdd->timeout = timeout; + wdd->parent = &pdev->dev; + + watchdog_set_drvdata(wdd, wdt); + + spin_lock_init(&wdt->lock);
Quoted 8 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+ + ret = sun4v_mach_set_watchdog(wdd->timeout, &time_remaining); + (void) sun4v_mach_set_watchdog(0, &time_remaining);Why first set (and enable) the watchdog just to disable it immediately afterwards ? Just to check if it can be set ? Is that really necessary ? Can't you just set it to 0 (disable it) and bail out if that does not work ?
I thought about this a bit more and I removed it.
The point of it was that it tests if timeout is a valid value and if it doesn't return HV_EOK the value is at a minimum wrong. Just a call with 0 wouldn't help, which was why the 2 calls but in the end it's really not the right place to do it. So I just return EINVAL in ping if it's wrong.
Quoted 22 lines
Quoted 22 lines
(quoted diff) +21
-0+ .driver = { + .name = DRV_NAME, + }, +}; + +static int __init sun4v_wdt_init_module(void) +{ + int err; + struct mdesc_handle *handle; + u64 node; + const u64 *value; + u64 resolution; + + /* + * There are 2 properties that can be set from the control + * domain for the watchdog. + * watchdog-resolution (in ms defaulting to 1000) + * watchdog-max-timeout (in ms) + * Right now, only support the default 1s (1000ms) resolution + * so just verify against the property, and make sure + * max timeout is taken into account, if set.
Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ */ + handle = mdesc_grab(); + if (!handle) + return -ENODEV; +Is there some means to determine if this is a SUN4V system ? The detections used (like this one, and the attempt to set the watchdog in the probe function) seem to be a bit shaky.
If this returns NULL, it's not a sun4v platform. This should be very reliable. (unlike the watchdog one)
Quoted 7 lines
Review
Quoted 4 lines
(quoted diff) +3
-0+ node = mdesc_node_by_name(handle, MDESC_NODE_NULL, "platform"); + if (node == MDESC_NODE_NULL) { + pr_info("No platform node\n");Is this an error, or does it just indicate that the watchdog is not supported ion this platform ? If it is an error, use
pr_err(). If it means the watchdog is not supported, return without message.
ok, it should have platform, I think it's fair to assume not supported here as well.
Quoted 13 lines
Review
Quoted 12 lines
(quoted diff) +11
-0+ err = -ENODEV; + goto out; + } + + value = mdesc_get_property(handle, node, "watchdog-resolution", NULL); + if (value) { + resolution = *value; + pr_info("Platform watchdog-resolution [%llux]\n", *value); + + if (resolution != 1000) { + pr_crit("Only 1000ms is supported.\n");Why is this critical ? Seems to be an implementation problem.
yeah ok fine.
Quoted 10 lines
Review
Quoted 7 lines
(quoted diff) +6
-0+ pr_info("Platform watchdog-max-timeout [%ds]\n", max_timeout); + + if (max_timeout < WDT_MIN_TIMEOUT) { + max_timeout = WDT_MIN_TIMEOUT; + pr_info("Setting max timeout to [%ds]\n", max_timeout); + }This is kind of odd. If the platform specifies a smaller maximum timeout than the pre-defined minimum, and you can just override that value, why care in the first place ?
Well -
there's a real max (WDT_MAX_TIMEOUT) for the platform
but as an admin you can specify your own max_timeout as a property of the specific domain and set it smaller.
So if I where to set max_timeout to 500, it would be .5 seconds and that would be messy. So this really just means, if I, as an admin, specify a max timeout that's less than 1 second, then set it to 1 second. That doesn't seem wrong.
Quoted 4 lines
Also,
WDT_MIN_TIMEOUTis 1 (second), meaning you would set the maximum timeout to 1 second, and the default timeout would end up being invalid. Can you try to define more reasonable acceptable limits ?
ok I that is fair, will change.
Quoted 6 lines
Review
Quoted 5 lines
(quoted diff) +4
-0+ + if (max_timeout > WDT_MAX_TIMEOUT) { + max_timeout = WDT_MAX_TIMEOUT; + pr_info("Setting max timeout to [%ds]\n", max_timeout);Is
WDT_MAX_TIMEOUTan absolute or an arbitrary limit ?
absolute - largest value the hv call accepts.
Quoted 8 lines
(quoted diff) +4
-0+} + +module_init(sun4v_wdt_init_module); +module_exit(sun4v_wdt_cleanup_module);Wonder if it would be better to move the initialization into the probe function and use
module_patform_driver(), ormodule_platform_driver_probe(). Any reason for not doing that ?
yeah ok I moved everything to that and it's a lot smaller now.
Will clean up and submit a new version soon - I also cleaned up time_remaining by just passing NULL and modify the hvcall itself in the next rev.
thanks again. sorry for some of the silly mistakes :)