| Message ID | 20210421162621.24910-3-francesco.zanella@vimar.com |
|---|---|
| State | New |
| Headers | show |
| Series | GPIO WDT "start-at-boot" property | expand |
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
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
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
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
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
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&data=04%7C01%7Cfrancesco.zanella%40vimar.com%7Cde549dd02adb45669ff208d9064c0739%7Ca1f008bcd59b4c668f8760fd9af15c7f%7C1%7C0%7C637547745887915290%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=pcqWkd%2B4m6RSS4KwmjgIbLpaa0XSCAOQorwI%2BIle5uY%3D&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 --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);
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(-)