On 04/02/2015 09:46 AM, Andrew Bresticker wrote:
On Wed, Apr 1, 2015 at 6:08 PM, Guenter Roeck <li...@roeck-us.net> wrote:
On 04/01/2015 03:22 PM, James Hogan wrote:
Hi Andrew,
On Wed, Apr 01, 2015 at 10:43:14AM -0700, Andrew Bresticker wrote:
Since the heartbeat is statically initialized to its default value,
watchdog_init_timeout() will never look in the device-tree for a
timeout-sec value. Instead of statically initializing heartbeat,
fall back to the default timeout value if watchdog_init_timeout()
fails.
Whoops. Sorry about that. I wasn't aware that a timeout-sec value was
expected. It isn't mentioned in the DT binding documentation for this
device :-(.
Signed-off-by: Andrew Bresticker <abres...@chromium.org>
Cc: Ezequiel Garcia <ezequiel.gar...@imgtec.com>
Cc: James Hogan <james.ho...@imgtec.com>
---
New for v2.
---
drivers/watchdog/imgpdc_wdt.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/watchdog/imgpdc_wdt.c
b/drivers/watchdog/imgpdc_wdt.c
index 0deaa4f..89b2abc 100644
--- a/drivers/watchdog/imgpdc_wdt.c
+++ b/drivers/watchdog/imgpdc_wdt.c
@@ -42,7 +42,7 @@
#define PDC_WDT_MIN_TIMEOUT 1
#define PDC_WDT_DEF_TIMEOUT 64
-static int heartbeat = PDC_WDT_DEF_TIMEOUT;
+static int heartbeat;
module_param(heartbeat, int, 0);
MODULE_PARM_DESC(heartbeat, "Watchdog heartbeats in seconds "
"(default=" __MODULE_STRING(PDC_WDT_DEF_TIMEOUT) ")");
@@ -195,9 +195,9 @@ static int pdc_wdt_probe(struct platform_device
*pdev)
ret = watchdog_init_timeout(&pdc_wdt->wdt_dev, heartbeat,
&pdev->dev);
if (ret < 0) {
- pdc_wdt->wdt_dev.timeout = pdc_wdt->wdt_dev.max_timeout;
+ pdc_wdt->wdt_dev.timeout = PDC_WDT_DEF_TIMEOUT;
The watchdog_init_timeout kerneldoc comment suggests that the old value
should be the default timeout, i.e. that timeout should be set to
PDC_WDT_DEF_TIMEOUT before calling watchdog_init_timeout, rather than
whenever ret < 0.
Indeed, if heartbeat is set to an invalid non-zero value,
watchdog_init_timeout will still try and set timeout from DT, but also
still returns -EINVAL regardless of whether that succeeds, and this
would incorrectly override the timeout from DT with the hardcoded
default.
dev_warn(&pdev->dev,
- "Initial timeout out of range! setting max
timeout\n");
+ "Initial timeout out of range! setting default
timeout\n");
It feels wrong for a presumably safe & normal situation (i.e. no default
in DT, which arguably shouldn't contain policy anyway) to show a
warning, but it can also show due to an invalid module parameter (or
invalid DT property) which is most definitely justified.
Agreed. I would suggest to leave that part alone and set the default prior
to calling watchdog_init_timeout().
Yes, but I think James' concern here was that we'd now get a
dev_warn() in the normal case where no timeout is specified via module
parameter or DT.
My understanding is that watchdog_init_timeout only returns an error if
the second parameter is not 0 and invalid, or if the timeout-sec property
has been provided and is invalid. I am not entirely sure I understand
why you think this is a problem. Can you please explain ?
Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
