Re: [PATCHv2] rtc: pcf2127: handle boot-enabled watchdog feature

[Bruno Thomsen <bruno.thomsen@gmail.com>]

Hi Martin,

Den tor. 3. okt. 2019 kl. 15.33 skrev Martin Hundebøll <martin@geanix.com>:
>
> Linux should handle when the pcf2127 watchdog feature is enabled by the
> bootloader. This is done by checking the watchdog timer value during
> init, and set the WDOG_HW_RUNNING flag if the value differs from zero.
>
> Signed-off-by: Martin Hundebøll <martin@geanix.com>
> ---
>
> Change since v1:
>  * remove setting of WDOG_HW_RUNNING in pcf2127_wdt_start()
>
>  drivers/rtc/rtc-pcf2127.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> index cb3472f..4229915 100644
> --- a/drivers/rtc/rtc-pcf2127.c
> +++ b/drivers/rtc/rtc-pcf2127.c
> @@ -420,6 +420,7 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>                         const char *name, bool has_nvmem)
>  {
>         struct pcf2127 *pcf2127;
> +       u32 wdd_timeout;
>         int ret = 0;
>
>         dev_dbg(dev, "%s\n", __func__);
> @@ -462,7 +463,6 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>         /*
>          * Watchdog timer enabled and reset pin /RST activated when timed out.
>          * Select 1Hz clock source for watchdog timer.
> -        * Timer is not started until WD_VAL is loaded with a valid value.

Your patch does not change the fact that the watchdog timer is first
started after loading a
valid value into WD_VAL register. This driver can be used perfectly
fine without enabling the
watchdog feature from userspace. If someone chooses to reboot without
stopping the watchdog
it is of course expected to still run on next boot (e.g. device probe).

> +       /* Test if watchdog timer is started by bootloader */
> +       ret = regmap_read(pcf2127->regmap, PCF2127_REG_WD_VAL, &wdd_timeout);
> +       if (ret) {
> +               dev_err(dev, "%s: watchdog value (wd_wal) failed\n", __func__);
> +               return ret;
> +       }
> +
> +       if (wdd_timeout)
> +               set_bit(WDOG_HW_RUNNING, &pcf2127->wdd.status);
> +

I do not agree that this should be the default setting as
WDOG_HW_RUNNING bit causes
watchdog core to kick watchdog until userland takes over, e.g. you
have just broken the
chain-of-monitoring in the embedded Linux device:

Hardware watchdog -> systemd -> daemon(s) / application(s)

At this point in time you only know that u-boot / barebox can load and
start the kernel with
a device tree blob.

What if mounting of rootfs fails?
What if systemd fails to start?

When doing a reboot due to ex. firmware upgrade, systemd will keep
kicking the watchdog
until the last sec before restart handler is called and the hardware
watchdog should not be
touched before systemd is in control of the system again.

Bruno