[RFC PATCH] watchdog: renesas_wdt: support handover from bootloader

[RFC PATCH] watchdog: renesas_wdt: support handover from bootloader

[Wolfram Sang]

Support an already running watchdog by checking its enable bit and set
up the status accordingly before registering the device.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

This patch was tested using a Renesas Salvator XS board (R-Car M3N). It works.
However, there is a small window where the watchdog clock is disabled, namely
after the MSSR clock driver initializes it until RuntimePM of the watchdog
driver takes over. If the system hangs in this window, bad luck. So, I'd think
it makes sense to have this clock either always-on or to keep the state which
came from the firmware. Geert, what do you think?

 drivers/watchdog/renesas_wdt.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
index 565dbc1ec638..37d757288b22 100644
--- a/drivers/watchdog/renesas_wdt.c
+++ b/drivers/watchdog/renesas_wdt.c
@@ -179,6 +179,7 @@ static int rwdt_probe(struct platform_device *pdev)
 	struct clk *clk;
 	unsigned long clks_per_sec;
 	int ret, i;
+	u8 csra;
 
 	if (rwdt_blacklisted(&pdev->dev))
 		return -ENODEV;
@@ -198,8 +199,8 @@ static int rwdt_probe(struct platform_device *pdev)
 	pm_runtime_enable(&pdev->dev);
 	pm_runtime_get_sync(&pdev->dev);
 	priv->clk_rate = clk_get_rate(clk);
-	priv->wdev.bootstatus = (readb_relaxed(priv->base + RWTCSRA) &
-				RWTCSRA_WOVF) ? WDIOF_CARDRESET : 0;
+	csra = readb_relaxed(priv->base + RWTCSRA);
+	priv->wdev.bootstatus = csra & RWTCSRA_WOVF ? WDIOF_CARDRESET : 0;
 	pm_runtime_put(&pdev->dev);
 
 	if (!priv->clk_rate) {
@@ -237,6 +238,16 @@ static int rwdt_probe(struct platform_device *pdev)
 	/* This overrides the default timeout only if DT configuration was found */
 	watchdog_init_timeout(&priv->wdev, 0, &pdev->dev);
 
+	if (csra & RWTCSRA_TME) {
+		/* Ensure properly initialized dividers */
+		rwdt_start(&priv->wdev);
+		set_bit(WDOG_HW_RUNNING, &priv->wdev.status);
+		//FIXME: We are missing pm_runtime_put in some code paths to
+		// to balance PM calls. We first need to decide if we maybe
+		// should have the RWDT clock always-on or if using RPM for
+		// clock management is OK.
+	}
+
 	ret = watchdog_register_device(&priv->wdev);
 	if (ret < 0)
 		goto out_pm_disable;
-- 
2.11.0

Re: [RFC PATCH] watchdog: renesas_wdt: support handover from bootloader

[Guenter Roeck]

On Mon, Apr 15, 2019 at 12:52:01PM +0200, Wolfram Sang wrote:
> Support an already running watchdog by checking its enable bit and set
> up the status accordingly before registering the device.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> This patch was tested using a Renesas Salvator XS board (R-Car M3N). It works.
> However, there is a small window where the watchdog clock is disabled, namely
> after the MSSR clock driver initializes it until RuntimePM of the watchdog
> driver takes over. If the system hangs in this window, bad luck. So, I'd think
> it makes sense to have this clock either always-on or to keep the state which
> came from the firmware. Geert, what do you think?
> 
>  drivers/watchdog/renesas_wdt.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> index 565dbc1ec638..37d757288b22 100644
> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -179,6 +179,7 @@ static int rwdt_probe(struct platform_device *pdev)
>  	struct clk *clk;
>  	unsigned long clks_per_sec;
>  	int ret, i;
> +	u8 csra;
>  
>  	if (rwdt_blacklisted(&pdev->dev))
>  		return -ENODEV;
> @@ -198,8 +199,8 @@ static int rwdt_probe(struct platform_device *pdev)
>  	pm_runtime_enable(&pdev->dev);
>  	pm_runtime_get_sync(&pdev->dev);
>  	priv->clk_rate = clk_get_rate(clk);
> -	priv->wdev.bootstatus = (readb_relaxed(priv->base + RWTCSRA) &
> -				RWTCSRA_WOVF) ? WDIOF_CARDRESET : 0;
> +	csra = readb_relaxed(priv->base + RWTCSRA);
> +	priv->wdev.bootstatus = csra & RWTCSRA_WOVF ? WDIOF_CARDRESET : 0;
>  	pm_runtime_put(&pdev->dev);
>  
>  	if (!priv->clk_rate) {
> @@ -237,6 +238,16 @@ static int rwdt_probe(struct platform_device *pdev)
>  	/* This overrides the default timeout only if DT configuration was found */
>  	watchdog_init_timeout(&priv->wdev, 0, &pdev->dev);
>  
> +	if (csra & RWTCSRA_TME) {
> +		/* Ensure properly initialized dividers */
> +		rwdt_start(&priv->wdev);
> +		set_bit(WDOG_HW_RUNNING, &priv->wdev.status);
> +		//FIXME: We are missing pm_runtime_put in some code paths to
> +		// to balance PM calls. We first need to decide if we maybe
> +		// should have the RWDT clock always-on or if using RPM for
> +		// clock management is OK.

Maybe I am missing something, but ..

Is handover even possible if the clock is controlled by clock management ?
Seems to me the clock would then be turned off through pm, which effectively
turns off the watchdog. So it will be off between clock/pm initialization
and the above code, meaning wdt handover from the boot loader is for all
practical purposes useless if the kernel gets stuck in between.

Thanks,
Guenter

> +	}
> +
>  	ret = watchdog_register_device(&priv->wdev);
>  	if (ret < 0)
>  		goto out_pm_disable;
> -- 
> 2.11.0
> 

Re: [RFC PATCH] watchdog: renesas_wdt: support handover from bootloader

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 647 bytes --]

Hi Guenter,

> > driver takes over. If the system hangs in this window, bad luck. So, I'd think
> > it makes sense to have this clock either always-on or to keep the state which
> > came from the firmware.

I wrote this paragraph...

> Is handover even possible if the clock is controlled by clock management ?
> Seems to me the clock would then be turned off through pm, which effectively
> turns off the watchdog. So it will be off between clock/pm initialization
> and the above code, meaning wdt handover from the boot loader is for all
> practical purposes useless if the kernel gets stuck in between.

... because I fully agree with you :)


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH] watchdog: renesas_wdt: support handover from bootloader

[Geert Uytterhoeven]

Hi Wolfram,

On Mon, Apr 15, 2019 at 12:52 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> Support an already running watchdog by checking its enable bit and set
> up the status accordingly before registering the device.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>
> This patch was tested using a Renesas Salvator XS board (R-Car M3N). It works.
> However, there is a small window where the watchdog clock is disabled, namely
> after the MSSR clock driver initializes it until RuntimePM of the watchdog
> driver takes over. If the system hangs in this window, bad luck. So, I'd think
> it makes sense to have this clock either always-on or to keep the state which
> came from the firmware. Geert, what do you think?

The MSSR clock driver does not disable the clock. The clock's core
clk_disable_unused() does, which is a late initcall.
So if the handover code calls rwdt_start() before that (i.e. no deferred
probing happens), the clock would never be disabled.

Note that pm_runtime_put() in rwdt_probe() queues a power down request,
but as it is not the _sync variant, it is delayed by some time, so
probably it would never happen if rwdt_start() is called by the handover
code in probe.

Now, if we would mark the clock always-on (CLK_IS_CRITICAL),
we can never disable it, even if the wdt is not used or the driver is
not compiled-in.

I don't think there's a way to mark a clock as "keep the state which
came from the firmware", CLK_IS_CRITICAL enables the clock in
__clk_core_init().

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC PATCH] watchdog: renesas_wdt: support handover from bootloader

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 946 bytes --]

On Mon, Apr 15, 2019 at 12:52:01PM +0200, Wolfram Sang wrote:
> Support an already running watchdog by checking its enable bit and set
> up the status accordingly before registering the device.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

After second thought, I am getting confused a little. If the WDT is
already running then

a) before this patch: after successful probe, RPM will disable the
clock until userspace opens the watchdog device

b) after this patch: during probe, our default timeout will be
programmed and because of WDOG_HW_RUNNING, the core will generate pings
until userspace opens the watchdog device.

So, b) will protect from a crashing kernel (no pings anymore) but not
from something like missing rootfs, or?

The usecase I had in mind ("give the kernel <x> seconds to boot into
working userspace") seems to be achieved by loading the WDT driver as a
module then, I guess?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH] watchdog: renesas_wdt: support handover from bootloader

[Guenter Roeck]

On Fri, May 24, 2019 at 03:52:37PM +0200, Wolfram Sang wrote:
> On Mon, Apr 15, 2019 at 12:52:01PM +0200, Wolfram Sang wrote:
> > Support an already running watchdog by checking its enable bit and set
> > up the status accordingly before registering the device.
> > 
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> After second thought, I am getting confused a little. If the WDT is
> already running then
> 
> a) before this patch: after successful probe, RPM will disable the
> clock until userspace opens the watchdog device
> 
> b) after this patch: during probe, our default timeout will be
> programmed and because of WDOG_HW_RUNNING, the core will generate pings
> until userspace opens the watchdog device.
> 
> So, b) will protect from a crashing kernel (no pings anymore) but not
> from something like missing rootfs, or?
> 
> The usecase I had in mind ("give the kernel <x> seconds to boot into
> working userspace") seems to be achieved by loading the WDT driver as a
> module then, I guess?
> 

Would
https://lore.kernel.org/linux-watchdog/20190605140628.618-1-rasmus.villemoes@prevas.dk/

solve your use case ?

Guenter

Re: [RFC PATCH] watchdog: renesas_wdt: support handover from bootloader

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 704 bytes --]


> > The usecase I had in mind ("give the kernel <x> seconds to boot into
> > working userspace") seems to be achieved by loading the WDT driver as a
> > module then, I guess?
> > 
> 
> Would
> https://lore.kernel.org/linux-watchdog/20190605140628.618-1-rasmus.villemoes@prevas.dk/
> 
> solve your use case ?

Yes, it would. Thanks for the pointer! I missed the
"handle_boot_enabled" parameter, too, for some reason. I think this
could also be enough for some scenarios.

As a result, it seems it makes sense to respin my patch, and test it
with "handle_boot_enabled" and the patch series you were pointing out.

I'll try to get this done within the next two weeks.

Thanks again!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]