[PATCH V2 1/3] watchdog: imgpdc: Allow timeout to be set in device-tree

[PATCH V2 1/3] watchdog: imgpdc: Allow timeout to be set in device-tree

[Andrew Bresticker]

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

[PATCH V2 2/3] watchdog: imgpdc: Set timeout before starting watchdog

[Andrew Bresticker]

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

[PATCH V2 3/3] watchdog: imgpdc: Add reboot support

[Andrew Bresticker]

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

Re: [PATCH V2 1/3] watchdog: imgpdc: Allow timeout to be set in device-tree

[James Hogan]

[-- 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 --]

Re: [PATCH V2 1/3] watchdog: imgpdc: Allow timeout to be set in device-tree

[Andrew Bresticker]

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

Re: [PATCH V2 1/3] watchdog: imgpdc: Allow timeout to be set in device-tree

[Guenter Roeck]

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
>>

Re: [PATCH V2 1/3] watchdog: imgpdc: Allow timeout to be set in device-tree

[Andrew Bresticker]

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.

Re: [PATCH V2 1/3] watchdog: imgpdc: Allow timeout to be set in device-tree

[Guenter Roeck]

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

Re: [PATCH V2 1/3] watchdog: imgpdc: Allow timeout to be set in device-tree

[Andrew Bresticker]

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

Re: [PATCH V2 1/3] watchdog: imgpdc: Allow timeout to be set in device-tree

[Guenter Roeck]

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

Re: [PATCH V2 1/3] watchdog: imgpdc: Allow timeout to be set in device-tree

[Guenter Roeck]

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

Re: [PATCH V2 1/3] watchdog: imgpdc: Allow timeout to be set in device-tree

[Guenter Roeck]

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