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

[Guenter Roeck <linux@roeck-us.net>]

On 10/6/19 8:58 AM, Martin Hundebøll wrote:
> 
> 
> On 6 October 2019 16.29.45 CEST, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 10/6/19 2:07 AM, Bruno Thomsen wrote:
>>> 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
>>>
>>
>> This should not be decided on driver level. The intended means to
>> enforce
>> an initial timeout would be to set CONFIG_WATCHDOG_OPEN_TIMEOUT, or to
>> use
>> the open_timeout kernel parameter.
> 
> That, and WATCHDOG_HANDLE_BOOT_ENABLED
> 

To clarify: If WATCHDOG_HANDLE_BOOT_ENABLED is disabled, the watchdog core
does not ping the watchdog on its own, and Bruno's argument does not apply
in the first place.

Guenter