diff mbox series

[2/2] watchdog: gpio_wdt: add "start-at-boot" feature

Message ID 20210421162621.24910-3-francesco.zanella@vimar.com
State New
Headers show
Series GPIO WDT "start-at-boot" property | expand

Commit Message

Francesco Zanella April 21, 2021, 4:26 p.m. UTC
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(-)

Comments

Guenter Roeck April 21, 2021, 4:42 p.m. UTC | #1
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
Francesco Zanella April 22, 2021, 4:28 p.m. UTC | #2
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
Guenter Roeck April 22, 2021, 6:05 p.m. UTC | #3
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
Francesco Zanella April 23, 2021, 9:47 a.m. UTC | #4
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
Rasmus Villemoes April 23, 2021, 11:36 a.m. UTC | #5
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
Francesco Zanella April 23, 2021, 2:24 p.m. UTC | #6
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
diff mbox series

Patch

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