linux-watchdog.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] watchdog: pnx4008: readout watchddog state
@ 2019-04-11 15:36 Alexandre Belloni
  2019-04-11 16:03 ` Guenter Roeck
  0 siblings, 1 reply; 4+ messages in thread
From: Alexandre Belloni @ 2019-04-11 15:36 UTC (permalink / raw)
  To: Guenter Roeck, Vladimir Zapolskiy
  Cc: Wim Van Sebroeck, Sylvain Lemieux, Gregory Clement,
	linux-arm-kernel, linux-watchdog, linux-kernel,
	Alexandre Belloni

Readout the enabled state so it is possible to get the pre-userspace
handler working. Also, avoid disabling the watchdog when nowayout is set to
ensure the watchdog continues working and triggers if there is an issue
later in the boot or if userspace fails to start.

Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 drivers/watchdog/pnx4008_wdt.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/pnx4008_wdt.c b/drivers/watchdog/pnx4008_wdt.c
index 8e261799c84e..9e3714e3814c 100644
--- a/drivers/watchdog/pnx4008_wdt.c
+++ b/drivers/watchdog/pnx4008_wdt.c
@@ -209,7 +209,11 @@ static int pnx4008_wdt_probe(struct platform_device *pdev)
 	watchdog_set_nowayout(&pnx4008_wdd, nowayout);
 	watchdog_set_restart_priority(&pnx4008_wdd, 128);
 
-	pnx4008_wdt_stop(&pnx4008_wdd);	/* disable for now */
+	if (readl(WDTIM_CTRL(wdt_base)) & COUNT_ENAB)
+		set_bit(WDOG_HW_RUNNING, &pnx4008_wdd.status);
+
+	if (!nowayout)
+		pnx4008_wdt_stop(&pnx4008_wdd);	/* disable for now */
 
 	ret = watchdog_register_device(&pnx4008_wdd);
 	if (ret < 0) {
-- 
2.20.1


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

* Re: [PATCH] watchdog: pnx4008: readout watchddog state
  2019-04-11 15:36 [PATCH] watchdog: pnx4008: readout watchddog state Alexandre Belloni
@ 2019-04-11 16:03 ` Guenter Roeck
  2019-04-11 16:13   ` Alexandre Belloni
  0 siblings, 1 reply; 4+ messages in thread
From: Guenter Roeck @ 2019-04-11 16:03 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: Vladimir Zapolskiy, Wim Van Sebroeck, Sylvain Lemieux,
	Gregory Clement, linux-arm-kernel, linux-watchdog, linux-kernel

Hi Alexandre,

On Thu, Apr 11, 2019 at 05:36:46PM +0200, Alexandre Belloni wrote:
> Readout the enabled state so it is possible to get the pre-userspace
> handler working. Also, avoid disabling the watchdog when nowayout is set to
> ensure the watchdog continues working and triggers if there is an issue
> later in the boot or if userspace fails to start.
> 
> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> ---
>  drivers/watchdog/pnx4008_wdt.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/pnx4008_wdt.c b/drivers/watchdog/pnx4008_wdt.c
> index 8e261799c84e..9e3714e3814c 100644
> --- a/drivers/watchdog/pnx4008_wdt.c
> +++ b/drivers/watchdog/pnx4008_wdt.c
> @@ -209,7 +209,11 @@ static int pnx4008_wdt_probe(struct platform_device *pdev)
>  	watchdog_set_nowayout(&pnx4008_wdd, nowayout);
>  	watchdog_set_restart_priority(&pnx4008_wdd, 128);
>  
> -	pnx4008_wdt_stop(&pnx4008_wdd);	/* disable for now */
> +	if (readl(WDTIM_CTRL(wdt_base)) & COUNT_ENAB)
> +		set_bit(WDOG_HW_RUNNING, &pnx4008_wdd.status);
> +
> +	if (!nowayout)
> +		pnx4008_wdt_stop(&pnx4008_wdd);	/* disable for now */

I don't see the point of stopping the watchdog here, even though that is the
old behavior. If it is possible to detect that the watchdog is running, we
can let the core handle pings until the watchdog device is opened. The code
above does that, but only if nowayout is set. That seems inconsistent and
unnecessary. Is there a downside to doing that unconditionally ?

Nitpick: s/watchddog/watchdog/ in subject line.

Thanks,
Guenter

>  
>  	ret = watchdog_register_device(&pnx4008_wdd);
>  	if (ret < 0) {
> -- 
> 2.20.1
> 

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

* Re: [PATCH] watchdog: pnx4008: readout watchddog state
  2019-04-11 16:03 ` Guenter Roeck
@ 2019-04-11 16:13   ` Alexandre Belloni
  2019-04-11 16:37     ` Guenter Roeck
  0 siblings, 1 reply; 4+ messages in thread
From: Alexandre Belloni @ 2019-04-11 16:13 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Vladimir Zapolskiy, Wim Van Sebroeck, Sylvain Lemieux,
	Gregory Clement, linux-arm-kernel, linux-watchdog, linux-kernel

On 11/04/2019 09:03:05-0700, Guenter Roeck wrote:
> Hi Alexandre,
> 
> On Thu, Apr 11, 2019 at 05:36:46PM +0200, Alexandre Belloni wrote:
> > Readout the enabled state so it is possible to get the pre-userspace
> > handler working. Also, avoid disabling the watchdog when nowayout is set to
> > ensure the watchdog continues working and triggers if there is an issue
> > later in the boot or if userspace fails to start.
> > 
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > ---
> >  drivers/watchdog/pnx4008_wdt.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/watchdog/pnx4008_wdt.c b/drivers/watchdog/pnx4008_wdt.c
> > index 8e261799c84e..9e3714e3814c 100644
> > --- a/drivers/watchdog/pnx4008_wdt.c
> > +++ b/drivers/watchdog/pnx4008_wdt.c
> > @@ -209,7 +209,11 @@ static int pnx4008_wdt_probe(struct platform_device *pdev)
> >  	watchdog_set_nowayout(&pnx4008_wdd, nowayout);
> >  	watchdog_set_restart_priority(&pnx4008_wdd, 128);
> >  
> > -	pnx4008_wdt_stop(&pnx4008_wdd);	/* disable for now */
> > +	if (readl(WDTIM_CTRL(wdt_base)) & COUNT_ENAB)
> > +		set_bit(WDOG_HW_RUNNING, &pnx4008_wdd.status);
> > +
> > +	if (!nowayout)
> > +		pnx4008_wdt_stop(&pnx4008_wdd);	/* disable for now */
> 
> I don't see the point of stopping the watchdog here, even though that is the
> old behavior. If it is possible to detect that the watchdog is running, we
> can let the core handle pings until the watchdog device is opened. The code
> above does that, but only if nowayout is set. That seems inconsistent and
> unnecessary. Is there a downside to doing that unconditionally ?
> 

I guess the downside is the change of behaviour. I don't think many will
be affected but they will have to ensure the core pings the watchdog
until the device is open.  WATCHDOG_HANDLE_BOOT_ENABLED is y by default
so that should be fine for most.

Honestly, I would just never disable the watchdog, that is the whole
point of this patch. If you are fine with that, I will just remove the
line.


> Nitpick: s/watchddog/watchdog/ in subject line.
> 

I'll fix it.


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

* Re: [PATCH] watchdog: pnx4008: readout watchddog state
  2019-04-11 16:13   ` Alexandre Belloni
@ 2019-04-11 16:37     ` Guenter Roeck
  0 siblings, 0 replies; 4+ messages in thread
From: Guenter Roeck @ 2019-04-11 16:37 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: Vladimir Zapolskiy, Wim Van Sebroeck, Sylvain Lemieux,
	Gregory Clement, linux-arm-kernel, linux-watchdog, linux-kernel

On Thu, Apr 11, 2019 at 06:13:39PM +0200, Alexandre Belloni wrote:
> On 11/04/2019 09:03:05-0700, Guenter Roeck wrote:
> > Hi Alexandre,
> > 
> > On Thu, Apr 11, 2019 at 05:36:46PM +0200, Alexandre Belloni wrote:
> > > Readout the enabled state so it is possible to get the pre-userspace
> > > handler working. Also, avoid disabling the watchdog when nowayout is set to
> > > ensure the watchdog continues working and triggers if there is an issue
> > > later in the boot or if userspace fails to start.
> > > 
> > > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > > ---
> > >  drivers/watchdog/pnx4008_wdt.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/watchdog/pnx4008_wdt.c b/drivers/watchdog/pnx4008_wdt.c
> > > index 8e261799c84e..9e3714e3814c 100644
> > > --- a/drivers/watchdog/pnx4008_wdt.c
> > > +++ b/drivers/watchdog/pnx4008_wdt.c
> > > @@ -209,7 +209,11 @@ static int pnx4008_wdt_probe(struct platform_device *pdev)
> > >  	watchdog_set_nowayout(&pnx4008_wdd, nowayout);
> > >  	watchdog_set_restart_priority(&pnx4008_wdd, 128);
> > >  
> > > -	pnx4008_wdt_stop(&pnx4008_wdd);	/* disable for now */
> > > +	if (readl(WDTIM_CTRL(wdt_base)) & COUNT_ENAB)
> > > +		set_bit(WDOG_HW_RUNNING, &pnx4008_wdd.status);
> > > +
> > > +	if (!nowayout)
> > > +		pnx4008_wdt_stop(&pnx4008_wdd);	/* disable for now */
> > 
> > I don't see the point of stopping the watchdog here, even though that is the
> > old behavior. If it is possible to detect that the watchdog is running, we
> > can let the core handle pings until the watchdog device is opened. The code
> > above does that, but only if nowayout is set. That seems inconsistent and
> > unnecessary. Is there a downside to doing that unconditionally ?
> > 
> 
> I guess the downside is the change of behaviour. I don't think many will
> be affected but they will have to ensure the core pings the watchdog
> until the device is open.  WATCHDOG_HANDLE_BOOT_ENABLED is y by default
> so that should be fine for most.
> 
Your code already does change behavior if nowayout is set.

> Honestly, I would just never disable the watchdog, that is the whole
> point of this patch. If you are fine with that, I will just remove the
> line.
> 
Please.

Thanks,
Guenter

> 
> > Nitpick: s/watchddog/watchdog/ in subject line.
> > 
> 
> I'll fix it.
> 
> 
> -- 
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

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

end of thread, other threads:[~2019-04-11 16:37 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-11 15:36 [PATCH] watchdog: pnx4008: readout watchddog state Alexandre Belloni
2019-04-11 16:03 ` Guenter Roeck
2019-04-11 16:13   ` Alexandre Belloni
2019-04-11 16:37     ` Guenter Roeck

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