LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH V2 1/3] watchdog: imgpdc: Allow timeout to be set in device-tree
@ 2015-04-01 17:43 Andrew Bresticker
  2015-04-01 17:43 ` [PATCH V2 2/3] watchdog: imgpdc: Set timeout before starting watchdog Andrew Bresticker
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Andrew Bresticker @ 2015-04-01 17:43 UTC (permalink / raw)
  To: Wim Van Sebroeck, Guenter Roeck
  Cc: linux-watchdog, linux-kernel, Andrew Bresticker, Ezequiel Garcia,
	James Hogan

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.

Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
Cc: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
Cc: James Hogan <james.hogan@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;
 		dev_warn(&pdev->dev,
-			 "Initial timeout out of range! setting max timeout\n");
+			 "Initial timeout out of range! setting default timeout\n");
 	}
 
 	pdc_wdt_stop(&pdc_wdt->wdt_dev);
-- 
2.2.0.rc0.207.ga3a616c


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH V2 2/3] watchdog: imgpdc: Set timeout before starting watchdog
  2015-04-01 17:43 [PATCH V2 1/3] watchdog: imgpdc: Allow timeout to be set in device-tree Andrew Bresticker
@ 2015-04-01 17:43 ` Andrew Bresticker
  2015-04-01 17:43 ` [PATCH V2 3/3] watchdog: imgpdc: Add reboot support Andrew Bresticker
  2015-04-01 22:22 ` [PATCH V2 1/3] watchdog: imgpdc: Allow timeout to be set in device-tree James Hogan
  2 siblings, 0 replies; 12+ messages in thread
From: Andrew Bresticker @ 2015-04-01 17:43 UTC (permalink / raw)
  To: Wim Van Sebroeck, Guenter Roeck
  Cc: linux-watchdog, linux-kernel, Andrew Bresticker, Naidu Tellapati,
	Ezequiel Garcia, James Hogan

Set up the watchdog for the specified timeout before attempting to start it.

Signed-off-by: Naidu Tellapati <naidu.tellapati@imgtec.com>
Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
Cc: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
Cc: James Hogan <james.hogan@imgtec.com>
---
Changes from v1:
 - Moved setting of timeout to a helper as suggested by Guenter.
---
 drivers/watchdog/imgpdc_wdt.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/imgpdc_wdt.c b/drivers/watchdog/imgpdc_wdt.c
index 89b2abc..c58b085 100644
--- a/drivers/watchdog/imgpdc_wdt.c
+++ b/drivers/watchdog/imgpdc_wdt.c
@@ -84,18 +84,24 @@ static int pdc_wdt_stop(struct watchdog_device *wdt_dev)
 	return 0;
 }
 
+static void __pdc_wdt_set_timeout(struct pdc_wdt_dev *wdt)
+{
+	unsigned long clk_rate = clk_get_rate(wdt->wdt_clk);
+	unsigned int val;
+
+	val = readl(wdt->base + PDC_WDT_CONFIG) & ~PDC_WDT_CONFIG_DELAY_MASK;
+	val |= order_base_2(wdt->wdt_dev.timeout * clk_rate) - 1;
+	writel(val, wdt->base + PDC_WDT_CONFIG);
+}
+
 static int pdc_wdt_set_timeout(struct watchdog_device *wdt_dev,
 			       unsigned int new_timeout)
 {
-	unsigned int val;
 	struct pdc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
-	unsigned long clk_rate = clk_get_rate(wdt->wdt_clk);
 
 	wdt->wdt_dev.timeout = new_timeout;
 
-	val = readl(wdt->base + PDC_WDT_CONFIG) & ~PDC_WDT_CONFIG_DELAY_MASK;
-	val |= order_base_2(new_timeout * clk_rate) - 1;
-	writel(val, wdt->base + PDC_WDT_CONFIG);
+	__pdc_wdt_set_timeout(wdt);
 
 	return 0;
 }
@@ -106,6 +112,8 @@ static int pdc_wdt_start(struct watchdog_device *wdt_dev)
 	unsigned int val;
 	struct pdc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
 
+	__pdc_wdt_set_timeout(wdt);
+
 	val = readl(wdt->base + PDC_WDT_CONFIG);
 	val |= PDC_WDT_CONFIG_ENABLE;
 	writel(val, wdt->base + PDC_WDT_CONFIG);
-- 
2.2.0.rc0.207.ga3a616c


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH V2 3/3] watchdog: imgpdc: Add reboot support
  2015-04-01 17:43 [PATCH V2 1/3] watchdog: imgpdc: Allow timeout to be set in device-tree Andrew Bresticker
  2015-04-01 17:43 ` [PATCH V2 2/3] watchdog: imgpdc: Set timeout before starting watchdog Andrew Bresticker
@ 2015-04-01 17:43 ` Andrew Bresticker
  2015-04-01 22:22 ` [PATCH V2 1/3] watchdog: imgpdc: Allow timeout to be set in device-tree James Hogan
  2 siblings, 0 replies; 12+ messages in thread
From: Andrew Bresticker @ 2015-04-01 17:43 UTC (permalink / raw)
  To: Wim Van Sebroeck, Guenter Roeck
  Cc: linux-watchdog, linux-kernel, Andrew Bresticker, Ezequiel Garcia,
	James Hogan

Register a restart handler that will restart the system by writing
to the watchdog's SOFT_RESET register.

Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Cc: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
Cc: James Hogan <james.hogan@imgtec.com>
---
No changes from v1.
---
 drivers/watchdog/imgpdc_wdt.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/watchdog/imgpdc_wdt.c b/drivers/watchdog/imgpdc_wdt.c
index c58b085..99e5ba4 100644
--- a/drivers/watchdog/imgpdc_wdt.c
+++ b/drivers/watchdog/imgpdc_wdt.c
@@ -16,6 +16,7 @@
 #include <linux/log2.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/reboot.h>
 #include <linux/slab.h>
 #include <linux/watchdog.h>
 
@@ -57,6 +58,7 @@ struct pdc_wdt_dev {
 	struct clk *wdt_clk;
 	struct clk *sys_clk;
 	void __iomem *base;
+	struct notifier_block restart_handler;
 };
 
 static int pdc_wdt_keepalive(struct watchdog_device *wdt_dev)
@@ -136,6 +138,18 @@ static const struct watchdog_ops pdc_wdt_ops = {
 	.set_timeout	= pdc_wdt_set_timeout,
 };
 
+static int pdc_wdt_restart(struct notifier_block *this, unsigned long mode,
+			   void *cmd)
+{
+	struct pdc_wdt_dev *wdt = container_of(this, struct pdc_wdt_dev,
+					       restart_handler);
+
+	/* Assert SOFT_RESET */
+	writel(0x1, wdt->base + PDC_WDT_SOFT_RESET);
+
+	return NOTIFY_OK;
+}
+
 static int pdc_wdt_probe(struct platform_device *pdev)
 {
 	int ret, val;
@@ -246,6 +260,13 @@ static int pdc_wdt_probe(struct platform_device *pdev)
 	if (ret)
 		goto disable_wdt_clk;
 
+	pdc_wdt->restart_handler.notifier_call = pdc_wdt_restart;
+	pdc_wdt->restart_handler.priority = 128;
+	ret = register_restart_handler(&pdc_wdt->restart_handler);
+	if (ret)
+		dev_warn(&pdev->dev, "failed to register restart handler: %d\n",
+			 ret);
+
 	return 0;
 
 disable_wdt_clk:
-- 
2.2.0.rc0.207.ga3a616c


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH V2 1/3] watchdog: imgpdc: Allow timeout to be set in device-tree
  2015-04-01 17:43 [PATCH V2 1/3] watchdog: imgpdc: Allow timeout to be set in device-tree Andrew Bresticker
  2015-04-01 17:43 ` [PATCH V2 2/3] watchdog: imgpdc: Set timeout before starting watchdog Andrew Bresticker
  2015-04-01 17:43 ` [PATCH V2 3/3] watchdog: imgpdc: Add reboot support Andrew Bresticker
@ 2015-04-01 22:22 ` James Hogan
  2015-04-01 22:42   ` Andrew Bresticker
                     ` (2 more replies)
  2 siblings, 3 replies; 12+ messages in thread
From: James Hogan @ 2015-04-01 22:22 UTC (permalink / raw)
  To: Andrew Bresticker
  Cc: Wim Van Sebroeck, Guenter Roeck, linux-watchdog, linux-kernel,
	Ezequiel Garcia

[-- Attachment #1: Type: text/plain, Size: 2869 bytes --]

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 <abrestic@chromium.org>
> Cc: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
> Cc: James Hogan <james.hogan@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.

The caller can check (ret < 0 && heartbeat) to tell if heartbeat was
invalid, but unfortunately it can't easily tell if the DT property is
out of range rather than simply absent.

Cheers
James

>  	}
>  
>  	pdc_wdt_stop(&pdc_wdt->wdt_dev);
> -- 
> 2.2.0.rc0.207.ga3a616c
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH V2 1/3] watchdog: imgpdc: Allow timeout to be set in device-tree
  2015-04-01 22:22 ` [PATCH V2 1/3] watchdog: imgpdc: Allow timeout to be set in device-tree James Hogan
@ 2015-04-01 22:42   ` Andrew Bresticker
  2015-04-02  1:08   ` Guenter Roeck
  2015-04-02  1:42   ` Guenter Roeck
  2 siblings, 0 replies; 12+ messages in thread
From: Andrew Bresticker @ 2015-04-01 22:42 UTC (permalink / raw)
  To: James Hogan
  Cc: Wim Van Sebroeck, Guenter Roeck, linux-watchdog, linux-kernel,
	Ezequiel Garcia

Hi James,

On Wed, Apr 1, 2015 at 3:22 PM, James Hogan <james.hogan@imgtec.com> 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 :-(.

I didn't notice it was broken either until Guenter pointed it out :).
Not sure I agree with timeout-sec even being DT property, but many
other drivers already support it so we might as well too.

>> Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
>> Cc: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
>> Cc: James Hogan <james.hogan@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.

Gah, right - will fix.

>>               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.
>
> The caller can check (ret < 0 && heartbeat) to tell if heartbeat was
> invalid, but unfortunately it can't easily tell if the DT property is
> out of range rather than simply absent.

I suppose this is why most watchdog drivers ignore the return value of
watchdog_init_timeout().  Perhaps dev_dbg() or dev_info() and a better
error message would be more appropriate.

Thanks,
Andrew

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH V2 1/3] watchdog: imgpdc: Allow timeout to be set in device-tree
  2015-04-01 22:22 ` [PATCH V2 1/3] watchdog: imgpdc: Allow timeout to be set in device-tree James Hogan
  2015-04-01 22:42   ` Andrew Bresticker
@ 2015-04-02  1:08   ` Guenter Roeck
  2015-04-02 16:46     ` Andrew Bresticker
  2015-04-02  1:42   ` Guenter Roeck
  2 siblings, 1 reply; 12+ messages in thread
From: Guenter Roeck @ 2015-04-02  1:08 UTC (permalink / raw)
  To: James Hogan, Andrew Bresticker
  Cc: Wim Van Sebroeck, linux-watchdog, linux-kernel, Ezequiel Garcia

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 <abrestic@chromium.org>
>> Cc: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
>> Cc: James Hogan <james.hogan@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().

Guenter


> The caller can check (ret < 0 && heartbeat) to tell if heartbeat was
> invalid, but unfortunately it can't easily tell if the DT property is
> out of range rather than simply absent.
>
> Cheers
> James
>
>>   	}
>>
>>   	pdc_wdt_stop(&pdc_wdt->wdt_dev);
>> --
>> 2.2.0.rc0.207.ga3a616c
>>


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH V2 1/3] watchdog: imgpdc: Allow timeout to be set in device-tree
  2015-04-01 22:22 ` [PATCH V2 1/3] watchdog: imgpdc: Allow timeout to be set in device-tree James Hogan
  2015-04-01 22:42   ` Andrew Bresticker
  2015-04-02  1:08   ` Guenter Roeck
@ 2015-04-02  1:42   ` Guenter Roeck
  2 siblings, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2015-04-02  1:42 UTC (permalink / raw)
  To: James Hogan, Andrew Bresticker
  Cc: Wim Van Sebroeck, linux-watchdog, linux-kernel, Ezequiel Garcia

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 :-(.
>

Hi James,

This is a standard watchdog driver attribute. See
Documentation/watchdog/watchdog-kernel-api.txt.

[ Agreed, it would probably make sense to document that in the bindings. ]

Guenter


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH V2 1/3] watchdog: imgpdc: Allow timeout to be set in device-tree
  2015-04-02  1:08   ` Guenter Roeck
@ 2015-04-02 16:46     ` Andrew Bresticker
  2015-04-03  1:52       ` Guenter Roeck
  0 siblings, 1 reply; 12+ messages in thread
From: Andrew Bresticker @ 2015-04-02 16:46 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: James Hogan, Wim Van Sebroeck, linux-watchdog, linux-kernel,
	Ezequiel Garcia

On Wed, Apr 1, 2015 at 6:08 PM, Guenter Roeck <linux@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 <abrestic@chromium.org>
>>> Cc: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
>>> Cc: James Hogan <james.hogan@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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH V2 1/3] watchdog: imgpdc: Allow timeout to be set in device-tree
  2015-04-02 16:46     ` Andrew Bresticker
@ 2015-04-03  1:52       ` Guenter Roeck
  2015-04-03  2:16         ` Andrew Bresticker
  0 siblings, 1 reply; 12+ messages in thread
From: Guenter Roeck @ 2015-04-03  1:52 UTC (permalink / raw)
  To: Andrew Bresticker
  Cc: James Hogan, Wim Van Sebroeck, linux-watchdog, linux-kernel,
	Ezequiel Garcia

On 04/02/2015 09:46 AM, Andrew Bresticker wrote:
> On Wed, Apr 1, 2015 at 6:08 PM, Guenter Roeck <linux@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 <abrestic@chromium.org>
>>>> Cc: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
>>>> Cc: James Hogan <james.hogan@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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH V2 1/3] watchdog: imgpdc: Allow timeout to be set in device-tree
  2015-04-03  1:52       ` Guenter Roeck
@ 2015-04-03  2:16         ` Andrew Bresticker
  2015-04-03  2:35           ` Guenter Roeck
  0 siblings, 1 reply; 12+ messages in thread
From: Andrew Bresticker @ 2015-04-03  2:16 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: James Hogan, Wim Van Sebroeck, linux-watchdog, linux-kernel,
	Ezequiel Garcia

Hi Guenter,

On Thu, Apr 2, 2015 at 6:52 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 04/02/2015 09:46 AM, Andrew Bresticker wrote:
>>
>> On Wed, Apr 1, 2015 at 6:08 PM, Guenter Roeck <linux@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 <abrestic@chromium.org>
>>>>> Cc: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
>>>>> Cc: James Hogan <james.hogan@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 ?

Unless I've gone completely insane, I'm pretty sure this will return
-EINVAL if timeout_parm is 0 and timeout-sec is not present:

int watchdog_init_timeout(struct watchdog_device *wdd,
                                unsigned int timeout_parm, struct device *dev)
{
        unsigned int t = 0;
        int ret = 0;

        watchdog_check_min_max_timeout(wdd);

        /* try to get the timeout module parameter first */
        if (!watchdog_timeout_invalid(wdd, timeout_parm) && timeout_parm) {
                wdd->timeout = timeout_parm;
                return ret;
        }
        if (timeout_parm)
                ret = -EINVAL;

        /* try to get the timeout_sec property */
        if (dev == NULL || dev->of_node == NULL)
                return ret;
        of_property_read_u32(dev->of_node, "timeout-sec", &t);
        if (!watchdog_timeout_invalid(wdd, t) && t)
                wdd->timeout = t;
        else
                ret = -EINVAL;

        return ret;
}

That said, the behavior you describe makes more sense, so perhaps
watchdog_init_timeout() should be updated to match.

Thanks,
Andrew

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH V2 1/3] watchdog: imgpdc: Allow timeout to be set in device-tree
  2015-04-03  2:16         ` Andrew Bresticker
@ 2015-04-03  2:35           ` Guenter Roeck
  2015-04-03  5:38             ` Guenter Roeck
  0 siblings, 1 reply; 12+ messages in thread
From: Guenter Roeck @ 2015-04-03  2:35 UTC (permalink / raw)
  To: Andrew Bresticker
  Cc: James Hogan, Wim Van Sebroeck, linux-watchdog, linux-kernel,
	Ezequiel Garcia

On 04/02/2015 07:16 PM, Andrew Bresticker wrote:
> Hi Guenter,
>
> On Thu, Apr 2, 2015 at 6:52 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 04/02/2015 09:46 AM, Andrew Bresticker wrote:
>>>
>>> On Wed, Apr 1, 2015 at 6:08 PM, Guenter Roeck <linux@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 <abrestic@chromium.org>
>>>>>> Cc: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
>>>>>> Cc: James Hogan <james.hogan@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 ?
>
> Unless I've gone completely insane, I'm pretty sure this will return
> -EINVAL if timeout_parm is 0 and timeout-sec is not present:
>
> int watchdog_init_timeout(struct watchdog_device *wdd,
>                                  unsigned int timeout_parm, struct device *dev)
> {
>          unsigned int t = 0;
>          int ret = 0;
>
>          watchdog_check_min_max_timeout(wdd);
>
>          /* try to get the timeout module parameter first */
>          if (!watchdog_timeout_invalid(wdd, timeout_parm) && timeout_parm) {
>                  wdd->timeout = timeout_parm;
>                  return ret;
>          }
>          if (timeout_parm)
>                  ret = -EINVAL;
>
>          /* try to get the timeout_sec property */
>          if (dev == NULL || dev->of_node == NULL)
>                  return ret;
>          of_property_read_u32(dev->of_node, "timeout-sec", &t);
>          if (!watchdog_timeout_invalid(wdd, t) && t)
>                  wdd->timeout = t;
>          else
>                  ret = -EINVAL;
>
>          return ret;
> }
>
> That said, the behavior you describe makes more sense, so perhaps
> watchdog_init_timeout() should be updated to match.
>

Ah yes, you are right, that last else case. Guess we'll need input from Wim
on how to handle this.

Guenter


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH V2 1/3] watchdog: imgpdc: Allow timeout to be set in device-tree
  2015-04-03  2:35           ` Guenter Roeck
@ 2015-04-03  5:38             ` Guenter Roeck
  0 siblings, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2015-04-03  5:38 UTC (permalink / raw)
  To: Andrew Bresticker
  Cc: James Hogan, Wim Van Sebroeck, linux-watchdog, linux-kernel,
	Ezequiel Garcia

On 04/02/2015 07:35 PM, Guenter Roeck wrote:
> On 04/02/2015 07:16 PM, Andrew Bresticker wrote:
>> Hi Guenter,
>>
>> On Thu, Apr 2, 2015 at 6:52 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>> On 04/02/2015 09:46 AM, Andrew Bresticker wrote:
>>>>
>>>> On Wed, Apr 1, 2015 at 6:08 PM, Guenter Roeck <linux@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 <abrestic@chromium.org>
>>>>>>> Cc: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
>>>>>>> Cc: James Hogan <james.hogan@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 ?
>>
>> Unless I've gone completely insane, I'm pretty sure this will return
>> -EINVAL if timeout_parm is 0 and timeout-sec is not present:
>>
>> int watchdog_init_timeout(struct watchdog_device *wdd,
>>                                  unsigned int timeout_parm, struct device *dev)
>> {
>>          unsigned int t = 0;
>>          int ret = 0;
>>
>>          watchdog_check_min_max_timeout(wdd);
>>
>>          /* try to get the timeout module parameter first */
>>          if (!watchdog_timeout_invalid(wdd, timeout_parm) && timeout_parm) {
>>                  wdd->timeout = timeout_parm;
>>                  return ret;
>>          }
>>          if (timeout_parm)
>>                  ret = -EINVAL;
>>
>>          /* try to get the timeout_sec property */
>>          if (dev == NULL || dev->of_node == NULL)
>>                  return ret;
>>          of_property_read_u32(dev->of_node, "timeout-sec", &t);
>>          if (!watchdog_timeout_invalid(wdd, t) && t)
>>                  wdd->timeout = t;
>>          else
>>                  ret = -EINVAL;
>>
>>          return ret;
>> }
>>
>> That said, the behavior you describe makes more sense, so perhaps
>> watchdog_init_timeout() should be updated to match.
>>
>
> Ah yes, you are right, that last else case. Guess we'll need input from Wim
> on how to handle this.
>

Actually, after looking into how other drivers use it, this is on purpose.
It says "I failed to initialize the timeout" and lets the caller deal with it.
Check other watchdog drivers for examples and possibilities.

Guenter


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2015-04-03  5:38 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-01 17:43 [PATCH V2 1/3] watchdog: imgpdc: Allow timeout to be set in device-tree Andrew Bresticker
2015-04-01 17:43 ` [PATCH V2 2/3] watchdog: imgpdc: Set timeout before starting watchdog Andrew Bresticker
2015-04-01 17:43 ` [PATCH V2 3/3] watchdog: imgpdc: Add reboot support Andrew Bresticker
2015-04-01 22:22 ` [PATCH V2 1/3] watchdog: imgpdc: Allow timeout to be set in device-tree James Hogan
2015-04-01 22:42   ` Andrew Bresticker
2015-04-02  1:08   ` Guenter Roeck
2015-04-02 16:46     ` Andrew Bresticker
2015-04-03  1:52       ` Guenter Roeck
2015-04-03  2:16         ` Andrew Bresticker
2015-04-03  2:35           ` Guenter Roeck
2015-04-03  5:38             ` Guenter Roeck
2015-04-02  1:42   ` Guenter Roeck

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).