| Message ID | 20190415105201.2078-1-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [RFC] watchdog: renesas_wdt: support handover from bootloader | expand |
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 >
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 :)
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
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?
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
> > 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!
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;
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(-)