linux-watchdog.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] GPIO WDT "start-at-boot" property
@ 2021-04-21 16:26 Francesco Zanella
  2021-04-21 16:26 ` [PATCH 1/2] dt-bindings: watchdog: gpio-wdt: add "start-at-boot" Francesco Zanella
  2021-04-21 16:26 ` [PATCH 2/2] watchdog: gpio_wdt: add "start-at-boot" feature Francesco Zanella
  0 siblings, 2 replies; 11+ messages in thread
From: Francesco Zanella @ 2021-04-21 16:26 UTC (permalink / raw)
  To: linux-watchdog, wim, linux
  Cc: devicetree, robh+dt, linux-kernel, Francesco Zanella

I propose to add a new property to GPIO WDT device tree binding
"start-at-boot".
If this property is present, start pinging hw watchdog at probe, in order
to take advantage of kernel configs:
- WATCHDOG_HANDLE_BOOT_ENABLED: Avoid possible reboot if hw watchdog was
  been enabled before the kernel (by uboot for example) and userspace
  doesn't take control of /dev/watchdog in time;
- WATCHDOG_OPEN_TIMEOUT: Reboot if userspace doesn't take control of
  /dev/watchdog within the timeout.

Francesco Zanella (2):
  dt-bindings: watchdog: gpio-wdt: add "start-at-boot"
  watchdog: gpio_wdt: add "start-at-boot" feature

 Documentation/devicetree/bindings/watchdog/gpio-wdt.txt | 7 +++++++
 drivers/watchdog/gpio_wdt.c                             | 6 +++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

-- 
2.17.1


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

* [PATCH 1/2] dt-bindings: watchdog: gpio-wdt: add "start-at-boot"
  2021-04-21 16:26 [PATCH 0/2] GPIO WDT "start-at-boot" property Francesco Zanella
@ 2021-04-21 16:26 ` Francesco Zanella
  2021-04-21 16:37   ` Guenter Roeck
  2021-04-21 16:26 ` [PATCH 2/2] watchdog: gpio_wdt: add "start-at-boot" feature Francesco Zanella
  1 sibling, 1 reply; 11+ messages in thread
From: Francesco Zanella @ 2021-04-21 16:26 UTC (permalink / raw)
  To: linux-watchdog, wim, linux
  Cc: devicetree, robh+dt, linux-kernel, Francesco Zanella

Documentation for new device tree property "start-at-boot".

Signed-off-by: Francesco Zanella <francesco.zanella@vimar.com>
---
 Documentation/devicetree/bindings/watchdog/gpio-wdt.txt | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
index 198794963786..cdaf7f0602e8 100644
--- a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
@@ -17,6 +17,13 @@ Optional Properties:
 - always-running: If the watchdog timer cannot be disabled, add this flag to
   have the driver keep toggling the signal without a client. It will only cease
   to toggle the signal when the device is open and the timeout elapsed.
+- start-at-boot: Start pinging hw watchdog at probe, in order to take advantage
+  of kernel configs:
+  - WATCHDOG_HANDLE_BOOT_ENABLED: Avoid possible reboot if hw watchdog was been
+    enabled before the kernel (by uboot for example) and userspace doesn't take
+    control of /dev/watchdog in time;
+  - WATCHDOG_OPEN_TIMEOUT: Reboot if userspace doesn't take control of
+    /dev/watchdog within the timeout.
 
 Example:
 	watchdog: watchdog {
-- 
2.17.1


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

* [PATCH 2/2] watchdog: gpio_wdt: add "start-at-boot" feature
  2021-04-21 16:26 [PATCH 0/2] GPIO WDT "start-at-boot" property Francesco Zanella
  2021-04-21 16:26 ` [PATCH 1/2] dt-bindings: watchdog: gpio-wdt: add "start-at-boot" Francesco Zanella
@ 2021-04-21 16:26 ` Francesco Zanella
  2021-04-21 16:42   ` Guenter Roeck
  2021-04-23 11:36   ` Rasmus Villemoes
  1 sibling, 2 replies; 11+ messages in thread
From: Francesco Zanella @ 2021-04-21 16:26 UTC (permalink / raw)
  To: linux-watchdog, wim, linux
  Cc: devicetree, robh+dt, linux-kernel, Francesco Zanella

If "start-at-boot" property is present in the device tree, start pinging
hw watchdog at probe, in order to take advantage of kernel configs:
- WATCHDOG_HANDLE_BOOT_ENABLED: Avoid possible reboot if hw watchdog was
  been enabled before the kernel (by uboot for example) and userspace
  doesn't take control of /dev/watchdog in time;
- WATCHDOG_OPEN_TIMEOUT: Reboot if userspace doesn't take control of
  /dev/watchdog within the timeout.

Signed-off-by: Francesco Zanella <francesco.zanella@vimar.com>
---
 drivers/watchdog/gpio_wdt.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
index 0923201ce874..1e6f0322ab7a 100644
--- a/drivers/watchdog/gpio_wdt.c
+++ b/drivers/watchdog/gpio_wdt.c
@@ -31,6 +31,7 @@ struct gpio_wdt_priv {
 	struct gpio_desc	*gpiod;
 	bool			state;
 	bool			always_running;
+	bool			start_at_boot;
 	unsigned int		hw_algo;
 	struct watchdog_device	wdd;
 };
@@ -147,6 +148,9 @@ static int gpio_wdt_probe(struct platform_device *pdev)
 	priv->always_running = of_property_read_bool(np,
 						     "always-running");
 
+	priv->start_at_boot = of_property_read_bool(np,
+						    "start-at-boot");
+
 	watchdog_set_drvdata(&priv->wdd, priv);
 
 	priv->wdd.info		= &gpio_wdt_ident;
@@ -161,7 +165,7 @@ static int gpio_wdt_probe(struct platform_device *pdev)
 
 	watchdog_stop_on_reboot(&priv->wdd);
 
-	if (priv->always_running)
+	if (priv->always_running || priv->start_at_boot)
 		gpio_wdt_start(&priv->wdd);
 
 	return devm_watchdog_register_device(dev, &priv->wdd);
-- 
2.17.1


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

* Re: [PATCH 1/2] dt-bindings: watchdog: gpio-wdt: add "start-at-boot"
  2021-04-21 16:26 ` [PATCH 1/2] dt-bindings: watchdog: gpio-wdt: add "start-at-boot" Francesco Zanella
@ 2021-04-21 16:37   ` Guenter Roeck
  2021-04-22 16:28     ` Francesco Zanella
  0 siblings, 1 reply; 11+ messages in thread
From: Guenter Roeck @ 2021-04-21 16:37 UTC (permalink / raw)
  To: Francesco Zanella; +Cc: linux-watchdog, wim, devicetree, robh+dt, linux-kernel

On Wed, Apr 21, 2021 at 06:26:20PM +0200, Francesco Zanella wrote:
> Documentation for new device tree property "start-at-boot".
> 
> Signed-off-by: Francesco Zanella <francesco.zanella@vimar.com>
> ---
>  Documentation/devicetree/bindings/watchdog/gpio-wdt.txt | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
> index 198794963786..cdaf7f0602e8 100644
> --- a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
> @@ -17,6 +17,13 @@ Optional Properties:
>  - always-running: If the watchdog timer cannot be disabled, add this flag to
>    have the driver keep toggling the signal without a client. It will only cease
>    to toggle the signal when the device is open and the timeout elapsed.
> +- start-at-boot: Start pinging hw watchdog at probe, in order to take advantage
> +  of kernel configs:
> +  - WATCHDOG_HANDLE_BOOT_ENABLED: Avoid possible reboot if hw watchdog was been
> +    enabled before the kernel (by uboot for example) and userspace doesn't take
> +    control of /dev/watchdog in time;
> +  - WATCHDOG_OPEN_TIMEOUT: Reboot if userspace doesn't take control of
> +    /dev/watchdog within the timeout.

You are not supposed to refer to Linux kernel details in devicetree
bindings documents.

Guenter

>  
>  Example:
>  	watchdog: watchdog {
> -- 
> 2.17.1
> 

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

* Re: [PATCH 2/2] watchdog: gpio_wdt: add "start-at-boot" feature
  2021-04-21 16:26 ` [PATCH 2/2] watchdog: gpio_wdt: add "start-at-boot" feature Francesco Zanella
@ 2021-04-21 16:42   ` Guenter Roeck
  2021-04-22 16:28     ` Francesco Zanella
  2021-04-23 11:36   ` Rasmus Villemoes
  1 sibling, 1 reply; 11+ messages in thread
From: Guenter Roeck @ 2021-04-21 16:42 UTC (permalink / raw)
  To: Francesco Zanella; +Cc: linux-watchdog, wim, devicetree, robh+dt, linux-kernel

On Wed, Apr 21, 2021 at 06:26:21PM +0200, Francesco Zanella wrote:
> If "start-at-boot" property is present in the device tree, start pinging
> hw watchdog at probe, in order to take advantage of kernel configs:
> - WATCHDOG_HANDLE_BOOT_ENABLED: Avoid possible reboot if hw watchdog was
>   been enabled before the kernel (by uboot for example) and userspace
>   doesn't take control of /dev/watchdog in time;
> - WATCHDOG_OPEN_TIMEOUT: Reboot if userspace doesn't take control of
>   /dev/watchdog within the timeout.
> 
> Signed-off-by: Francesco Zanella <francesco.zanella@vimar.com>
> ---
>  drivers/watchdog/gpio_wdt.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
> index 0923201ce874..1e6f0322ab7a 100644
> --- a/drivers/watchdog/gpio_wdt.c
> +++ b/drivers/watchdog/gpio_wdt.c
> @@ -31,6 +31,7 @@ struct gpio_wdt_priv {
>  	struct gpio_desc	*gpiod;
>  	bool			state;
>  	bool			always_running;
> +	bool			start_at_boot;
>  	unsigned int		hw_algo;
>  	struct watchdog_device	wdd;
>  };
> @@ -147,6 +148,9 @@ static int gpio_wdt_probe(struct platform_device *pdev)
>  	priv->always_running = of_property_read_bool(np,
>  						     "always-running");
>  
> +	priv->start_at_boot = of_property_read_bool(np,
> +						    "start-at-boot");
> +
>  	watchdog_set_drvdata(&priv->wdd, priv);
>  
>  	priv->wdd.info		= &gpio_wdt_ident;
> @@ -161,7 +165,7 @@ static int gpio_wdt_probe(struct platform_device *pdev)
>  
>  	watchdog_stop_on_reboot(&priv->wdd);
>  
> -	if (priv->always_running)
> +	if (priv->always_running || priv->start_at_boot)
>  		gpio_wdt_start(&priv->wdd);

So the only real difference to always_running is that always_running
doesn't stop the watchdog on close but keeps it running.

Does that really warrant another property ? Why not just use
always-running ?

The special use case of being able to stop the watchdog doesn't seem
to be worth the trouble. Please explain your use case.

Guenter

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

* Re: [PATCH 1/2] dt-bindings: watchdog: gpio-wdt: add "start-at-boot"
  2021-04-21 16:37   ` Guenter Roeck
@ 2021-04-22 16:28     ` Francesco Zanella
  0 siblings, 0 replies; 11+ messages in thread
From: Francesco Zanella @ 2021-04-22 16:28 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: linux-watchdog, wim, devicetree, robh+dt, linux-kernel



On 21/04/21 18:37, Guenter Roeck wrote:
> On Wed, Apr 21, 2021 at 06:26:20PM +0200, Francesco Zanella wrote:
>> Documentation for new device tree property "start-at-boot".
>>
>> Signed-off-by: Francesco Zanella <francesco.zanella@vimar.com>
>> ---
>>  Documentation/devicetree/bindings/watchdog/gpio-wdt.txt | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
>> index 198794963786..cdaf7f0602e8 100644
>> --- a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
>> +++ b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
>> @@ -17,6 +17,13 @@ Optional Properties:
>>  - always-running: If the watchdog timer cannot be disabled, add this flag to
>>    have the driver keep toggling the signal without a client. It will only cease
>>    to toggle the signal when the device is open and the timeout elapsed.
>> +- start-at-boot: Start pinging hw watchdog at probe, in order to take advantage
>> +  of kernel configs:
>> +  - WATCHDOG_HANDLE_BOOT_ENABLED: Avoid possible reboot if hw watchdog was been
>> +    enabled before the kernel (by uboot for example) and userspace doesn't take
>> +    control of /dev/watchdog in time;
>> +  - WATCHDOG_OPEN_TIMEOUT: Reboot if userspace doesn't take control of
>> +    /dev/watchdog within the timeout.
> 
> You are not supposed to refer to Linux kernel details in devicetree
> bindings documents.
> 
> Guenter
> 
>>  
>>  Example:
>>  	watchdog: watchdog {
>> -- 
>> 2.17.1
>>

OK, I'm sorry. I will resend the patch series without kernel configs
references in documents.

Francesco

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

* Re: [PATCH 2/2] watchdog: gpio_wdt: add "start-at-boot" feature
  2021-04-21 16:42   ` Guenter Roeck
@ 2021-04-22 16:28     ` Francesco Zanella
  2021-04-22 18:05       ` Guenter Roeck
  0 siblings, 1 reply; 11+ messages in thread
From: Francesco Zanella @ 2021-04-22 16:28 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: linux-watchdog, wim, devicetree, robh+dt, linux-kernel



On 21/04/21 18:42, Guenter Roeck wrote:
> On Wed, Apr 21, 2021 at 06:26:21PM +0200, Francesco Zanella wrote:
>> If "start-at-boot" property is present in the device tree, start pinging
>> hw watchdog at probe, in order to take advantage of kernel configs:
>> - WATCHDOG_HANDLE_BOOT_ENABLED: Avoid possible reboot if hw watchdog was
>>   been enabled before the kernel (by uboot for example) and userspace
>>   doesn't take control of /dev/watchdog in time;
>> - WATCHDOG_OPEN_TIMEOUT: Reboot if userspace doesn't take control of
>>   /dev/watchdog within the timeout.
>>
>> Signed-off-by: Francesco Zanella <francesco.zanella@vimar.com>
>> ---
>>  drivers/watchdog/gpio_wdt.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
>> index 0923201ce874..1e6f0322ab7a 100644
>> --- a/drivers/watchdog/gpio_wdt.c
>> +++ b/drivers/watchdog/gpio_wdt.c
>> @@ -31,6 +31,7 @@ struct gpio_wdt_priv {
>>  	struct gpio_desc	*gpiod;
>>  	bool			state;
>>  	bool			always_running;
>> +	bool			start_at_boot;
>>  	unsigned int		hw_algo;
>>  	struct watchdog_device	wdd;
>>  };
>> @@ -147,6 +148,9 @@ static int gpio_wdt_probe(struct platform_device *pdev)
>>  	priv->always_running = of_property_read_bool(np,
>>  						     "always-running");
>>  
>> +	priv->start_at_boot = of_property_read_bool(np,
>> +						    "start-at-boot");
>> +
>>  	watchdog_set_drvdata(&priv->wdd, priv);
>>  
>>  	priv->wdd.info		= &gpio_wdt_ident;
>> @@ -161,7 +165,7 @@ static int gpio_wdt_probe(struct platform_device *pdev)
>>  
>>  	watchdog_stop_on_reboot(&priv->wdd);
>>  
>> -	if (priv->always_running)
>> +	if (priv->always_running || priv->start_at_boot)
>>  		gpio_wdt_start(&priv->wdd);
> 
> So the only real difference to always_running is that always_running
> doesn't stop the watchdog on close but keeps it running.
> 
> Does that really warrant another property ? Why not just use
> always-running ?
> 
> The special use case of being able to stop the watchdog doesn't seem
> to be worth the trouble. Please explain your use case.
> 
> Guenter
> 

You got the point.
I would like to be able to stop the watchdog when I want.
From my point of view always_running is used in the very special
case when the wdt chip can't be stopped.
I want a normal wdt that can be stopped (for example during a system
update), but I want it to start at boot for the 2 reasons that I
explained before:
- I need continuity in hw wdt pinging because I start in uboot,
  so I take advantage of WATCHDOG_HANDLE_BOOT_ENABLED that makes
  the kernel to do that job; but it is triggered only if it is
  started at probe, if I'm not wrong.
- I would like to monitor with the wdt also the kernel at boot,
  and more importantly handle the case when the userspace app
  doesn't take control of /dev/watchdog for whatever reason
  within the timeout set with WATCHDOG_OPEN_TIMEOUT.

Hope that this explain my target.
Thanks for your attention,

Francesco

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

* Re: [PATCH 2/2] watchdog: gpio_wdt: add "start-at-boot" feature
  2021-04-22 16:28     ` Francesco Zanella
@ 2021-04-22 18:05       ` Guenter Roeck
  2021-04-23  9:47         ` Francesco Zanella
  0 siblings, 1 reply; 11+ messages in thread
From: Guenter Roeck @ 2021-04-22 18:05 UTC (permalink / raw)
  To: Francesco Zanella; +Cc: linux-watchdog, wim, devicetree, robh+dt, linux-kernel

On Thu, Apr 22, 2021 at 06:28:40PM +0200, Francesco Zanella wrote:
> 
> 
> On 21/04/21 18:42, Guenter Roeck wrote:
> > On Wed, Apr 21, 2021 at 06:26:21PM +0200, Francesco Zanella wrote:
> >> If "start-at-boot" property is present in the device tree, start pinging
> >> hw watchdog at probe, in order to take advantage of kernel configs:
> >> - WATCHDOG_HANDLE_BOOT_ENABLED: Avoid possible reboot if hw watchdog was
> >>   been enabled before the kernel (by uboot for example) and userspace
> >>   doesn't take control of /dev/watchdog in time;
> >> - WATCHDOG_OPEN_TIMEOUT: Reboot if userspace doesn't take control of
> >>   /dev/watchdog within the timeout.
> >>
> >> Signed-off-by: Francesco Zanella <francesco.zanella@vimar.com>
> >> ---
> >>  drivers/watchdog/gpio_wdt.c | 6 +++++-
> >>  1 file changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
> >> index 0923201ce874..1e6f0322ab7a 100644
> >> --- a/drivers/watchdog/gpio_wdt.c
> >> +++ b/drivers/watchdog/gpio_wdt.c
> >> @@ -31,6 +31,7 @@ struct gpio_wdt_priv {
> >>  	struct gpio_desc	*gpiod;
> >>  	bool			state;
> >>  	bool			always_running;
> >> +	bool			start_at_boot;
> >>  	unsigned int		hw_algo;
> >>  	struct watchdog_device	wdd;
> >>  };
> >> @@ -147,6 +148,9 @@ static int gpio_wdt_probe(struct platform_device *pdev)
> >>  	priv->always_running = of_property_read_bool(np,
> >>  						     "always-running");
> >>  
> >> +	priv->start_at_boot = of_property_read_bool(np,
> >> +						    "start-at-boot");
> >> +
> >>  	watchdog_set_drvdata(&priv->wdd, priv);
> >>  
> >>  	priv->wdd.info		= &gpio_wdt_ident;
> >> @@ -161,7 +165,7 @@ static int gpio_wdt_probe(struct platform_device *pdev)
> >>  
> >>  	watchdog_stop_on_reboot(&priv->wdd);
> >>  
> >> -	if (priv->always_running)
> >> +	if (priv->always_running || priv->start_at_boot)
> >>  		gpio_wdt_start(&priv->wdd);
> > 
> > So the only real difference to always_running is that always_running
> > doesn't stop the watchdog on close but keeps it running.
> > 
> > Does that really warrant another property ? Why not just use
> > always-running ?
> > 
> > The special use case of being able to stop the watchdog doesn't seem
> > to be worth the trouble. Please explain your use case.
> > 
> > Guenter
> > 
> 
> You got the point.
> I would like to be able to stop the watchdog when I want.
> From my point of view always_running is used in the very special
> case when the wdt chip can't be stopped.
> I want a normal wdt that can be stopped (for example during a system
> update), but I want it to start at boot for the 2 reasons that I
> explained before:
> - I need continuity in hw wdt pinging because I start in uboot,
>   so I take advantage of WATCHDOG_HANDLE_BOOT_ENABLED that makes
>   the kernel to do that job; but it is triggered only if it is
>   started at probe, if I'm not wrong.

That depends. If the driver can read the current state (ie if
it can detect if the watchdog is running) it can report it
to the watchdog core accordingly. That would be the preferred
mechanism. Everything else is just a workaround for bad hardware
(bad as in "it doesn't report its state").

Guenter

> - I would like to monitor with the wdt also the kernel at boot,
>   and more importantly handle the case when the userspace app
>   doesn't take control of /dev/watchdog for whatever reason
>   within the timeout set with WATCHDOG_OPEN_TIMEOUT.
> 
> Hope that this explain my target.
> Thanks for your attention,
> 
> Francesco

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

* Re: [PATCH 2/2] watchdog: gpio_wdt: add "start-at-boot" feature
  2021-04-22 18:05       ` Guenter Roeck
@ 2021-04-23  9:47         ` Francesco Zanella
  0 siblings, 0 replies; 11+ messages in thread
From: Francesco Zanella @ 2021-04-23  9:47 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: linux-watchdog, wim, devicetree, robh+dt, linux-kernel,
	francesco.zanella



On 22/04/21 20:05, Guenter Roeck wrote:
> On Thu, Apr 22, 2021 at 06:28:40PM +0200, Francesco Zanella wrote:
>>
>>
>> On 21/04/21 18:42, Guenter Roeck wrote:
>>> On Wed, Apr 21, 2021 at 06:26:21PM +0200, Francesco Zanella wrote:
>>>> If "start-at-boot" property is present in the device tree, start pinging
>>>> hw watchdog at probe, in order to take advantage of kernel configs:
>>>> - WATCHDOG_HANDLE_BOOT_ENABLED: Avoid possible reboot if hw watchdog was
>>>>   been enabled before the kernel (by uboot for example) and userspace
>>>>   doesn't take control of /dev/watchdog in time;
>>>> - WATCHDOG_OPEN_TIMEOUT: Reboot if userspace doesn't take control of
>>>>   /dev/watchdog within the timeout.
>>>>
>>>> Signed-off-by: Francesco Zanella <francesco.zanella@vimar.com>
>>>> ---
>>>>  drivers/watchdog/gpio_wdt.c | 6 +++++-
>>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
>>>> index 0923201ce874..1e6f0322ab7a 100644
>>>> --- a/drivers/watchdog/gpio_wdt.c
>>>> +++ b/drivers/watchdog/gpio_wdt.c
>>>> @@ -31,6 +31,7 @@ struct gpio_wdt_priv {
>>>>  	struct gpio_desc	*gpiod;
>>>>  	bool			state;
>>>>  	bool			always_running;
>>>> +	bool			start_at_boot;
>>>>  	unsigned int		hw_algo;
>>>>  	struct watchdog_device	wdd;
>>>>  };
>>>> @@ -147,6 +148,9 @@ static int gpio_wdt_probe(struct platform_device *pdev)
>>>>  	priv->always_running = of_property_read_bool(np,
>>>>  						     "always-running");
>>>>  
>>>> +	priv->start_at_boot = of_property_read_bool(np,
>>>> +						    "start-at-boot");
>>>> +
>>>>  	watchdog_set_drvdata(&priv->wdd, priv);
>>>>  
>>>>  	priv->wdd.info		= &gpio_wdt_ident;
>>>> @@ -161,7 +165,7 @@ static int gpio_wdt_probe(struct platform_device *pdev)
>>>>  
>>>>  	watchdog_stop_on_reboot(&priv->wdd);
>>>>  
>>>> -	if (priv->always_running)
>>>> +	if (priv->always_running || priv->start_at_boot)
>>>>  		gpio_wdt_start(&priv->wdd);
>>>
>>> So the only real difference to always_running is that always_running
>>> doesn't stop the watchdog on close but keeps it running.
>>>
>>> Does that really warrant another property ? Why not just use
>>> always-running ?
>>>
>>> The special use case of being able to stop the watchdog doesn't seem
>>> to be worth the trouble. Please explain your use case.
>>>
>>> Guenter
>>>
>>
>> You got the point.
>> I would like to be able to stop the watchdog when I want.
>> From my point of view always_running is used in the very special
>> case when the wdt chip can't be stopped.
>> I want a normal wdt that can be stopped (for example during a system
>> update), but I want it to start at boot for the 2 reasons that I
>> explained before:
>> - I need continuity in hw wdt pinging because I start in uboot,
>>   so I take advantage of WATCHDOG_HANDLE_BOOT_ENABLED that makes
>>   the kernel to do that job; but it is triggered only if it is
>>   started at probe, if I'm not wrong.
> 
> That depends. If the driver can read the current state (ie if
> it can detect if the watchdog is running) it can report it
> to the watchdog core accordingly. That would be the preferred
> mechanism. Everything else is just a workaround for bad hardware
> (bad as in "it doesn't report its state").
> 
> Guenter
> 
>> - I would like to monitor with the wdt also the kernel at boot,
>>   and more importantly handle the case when the userspace app
>>   doesn't take control of /dev/watchdog for whatever reason
>>   within the timeout set with WATCHDOG_OPEN_TIMEOUT.
>>
>> Hope that this explain my target.
>> Thanks for your attention,
>>
>> Francesco

Right, I agree with you that's the optimal way, but we are talking
about a low cost, simple GPIO WDT, that has only an input GPIO as
an interface with the system... how can it report its state if the
only way to enable/disable it is a particular state of the GPIO
that we are going to pilot?
I think that this driver was born for that kind of low cost chips
(I'm using a SGM706 for example), why can't we let it take
advantage of a useful kernel mechanism?

Thank you,

Francesco

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

* Re: [PATCH 2/2] watchdog: gpio_wdt: add "start-at-boot" feature
  2021-04-21 16:26 ` [PATCH 2/2] watchdog: gpio_wdt: add "start-at-boot" feature Francesco Zanella
  2021-04-21 16:42   ` Guenter Roeck
@ 2021-04-23 11:36   ` Rasmus Villemoes
  2021-04-23 14:24     ` Francesco Zanella
  1 sibling, 1 reply; 11+ messages in thread
From: Rasmus Villemoes @ 2021-04-23 11:36 UTC (permalink / raw)
  To: Francesco Zanella, linux-watchdog, wim, linux
  Cc: devicetree, robh+dt, linux-kernel

On 21/04/2021 18.26, Francesco Zanella wrote:
> If "start-at-boot" property is present in the device tree, start pinging
> hw watchdog at probe, in order to take advantage of kernel configs:

(1) Are you aware of the recent proposal to add a similar feature on
watchdog core level:

https://lore.kernel.org/lkml/?q=start_enable

(2) If you set always-running but not nowayout you essentially have what
you want now: If userspace opens the device [within the limit set by
OPEN_TIMEOUT if that is in effect], but then does a graceful close (i.e.
writes 'V' immediately before close()), the kernel will assume
responsibility for pinging the device. So the device isn't stopped as
such, but if you can't trust the kernel thread/timer to keep it alive,
the system is already mostly unusable. [Also, how reliable is that 'the
timer is stopped if the gpio is set to be an input' anyway].

Rasmus

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

* Re: [PATCH 2/2] watchdog: gpio_wdt: add "start-at-boot" feature
  2021-04-23 11:36   ` Rasmus Villemoes
@ 2021-04-23 14:24     ` Francesco Zanella
  0 siblings, 0 replies; 11+ messages in thread
From: Francesco Zanella @ 2021-04-23 14:24 UTC (permalink / raw)
  To: Rasmus Villemoes, linux-watchdog, wim, linux
  Cc: devicetree, robh+dt, linux-kernel



On 23/04/21 13:36, Rasmus Villemoes wrote:
> On 21/04/2021 18.26, Francesco Zanella wrote:
>> If "start-at-boot" property is present in the device tree, start pinging
>> hw watchdog at probe, in order to take advantage of kernel configs:
> 
> (1) Are you aware of the recent proposal to add a similar feature on
> watchdog core level:
> 
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F%3Fq%3Dstart_enable&amp;data=04%7C01%7Cfrancesco.zanella%40vimar.com%7Cde549dd02adb45669ff208d9064c0739%7Ca1f008bcd59b4c668f8760fd9af15c7f%7C1%7C0%7C637547745887915290%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=pcqWkd%2B4m6RSS4KwmjgIbLpaa0XSCAOQorwI%2BIle5uY%3D&amp;reserved=0
> 

Oh good! Happy to know that, I missed it, sorry, it's quite new.
That kind of work would have been my next proposal if this had been accepted.
Hope that it will be mainlined.

> (2) If you set always-running but not nowayout you essentially have what
> you want now: If userspace opens the device [within the limit set by
> OPEN_TIMEOUT if that is in effect], but then does a graceful close (i.e.
> writes 'V' immediately before close()), the kernel will assume
> responsibility for pinging the device. So the device isn't stopped as
> such, but if you can't trust the kernel thread/timer to keep it alive,
> the system is already mostly unusable. [Also, how reliable is that 'the
> timer is stopped if the gpio is set to be an input' anyway].
> 
> Rasmus
> 

No I would like to be able to totally disable it with stop, not that the kernel
will keep it pinged.

However, glad to know the news, I will follow the evolution.

Thanks, regards,

Francesco Zanella

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

end of thread, other threads:[~2021-04-23 14:24 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-21 16:26 [PATCH 0/2] GPIO WDT "start-at-boot" property Francesco Zanella
2021-04-21 16:26 ` [PATCH 1/2] dt-bindings: watchdog: gpio-wdt: add "start-at-boot" Francesco Zanella
2021-04-21 16:37   ` Guenter Roeck
2021-04-22 16:28     ` Francesco Zanella
2021-04-21 16:26 ` [PATCH 2/2] watchdog: gpio_wdt: add "start-at-boot" feature Francesco Zanella
2021-04-21 16:42   ` Guenter Roeck
2021-04-22 16:28     ` Francesco Zanella
2021-04-22 18:05       ` Guenter Roeck
2021-04-23  9:47         ` Francesco Zanella
2021-04-23 11:36   ` Rasmus Villemoes
2021-04-23 14:24     ` Francesco Zanella

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