u-boot leaves watchdog enabled by default

[Tom Rini]

On Mon, Sep 21, 2020 at 08:29:00PM +0200, Heinrich Schuchardt wrote:
> On 9/21/20 7:30 PM, Tom Rini wrote:
> > On Mon, Sep 21, 2020 at 11:01:37AM +0200, Stefan Roese wrote:
> >> Hi Michael,
> >> Hi Chris,
> >>
> >> On 15.09.20 12:44, Chris Packham wrote:
> >>>
> >>>
> >>> On Tue, 15 Sep 2020, 7:54 PM Michael Walle, <michael at walle.cc> wrote:
> >>>
> >>>     Am 2020-09-15 09:44, schrieb Rayagonda Kokatanur:
> >>>      > On Tue, Sep 15, 2020 at 12:56 PM Michael Walle <michael at walle.cc>
> >>>      > wrote:
> >>>      >>
> >>>      >> Hi Stefan,
> >>>      >>
> >>>      >> it appears that since commit 06985289d45 ("watchdog: Implement
> >>>     generic
> >>>      >> watchdog_reset() version") - by default - the first watchdog is
> >>>      >> started
> >>>      >> unconditionally if CONFIG_WDT is set but never stopped before
> >>>     booting
> >>>      >> the operating system.
> >>>      >>
> >>>      >> Shouldn't it also be stopped uncondionally? What's worse is that on
> >>>      >> one
> >>>      >> board/arch the watchdog is stopped in arch_preboot_os() which is
> >>>     never
> 
> Which board are you referring to?
> 
> >>>      >> called in the bootefi case. So even if I'd do a workaround and
> >>>     stop it
> >>>      >> manually in my board code, I couldn't do that consistently for
> >>>      >> bootm/bootefi.
> >>>      >>
> >>>      >> Or am I missing something here?
> >>>      >
> >>>      > Define CONFIG_WATCHDOG.
> >>>      > This takes care of resetting wdt.
> >>>
> >>>     Yes as along as you're inside the bootloader, but when u-boot hands
> >>>     control over the OS the watchdog is not serviced anymore; which wouldn't
> >>>     be a problem per se, but it is enabled unconditionally by u-boot.
> >>>
> >>>
> >>> Just to add some data. At $dayjob we use this behaviour as a failsafe to
> >>> make sure our userspace gets to a point where it is servicing the
> >>> watchdog.
> >>
> >> Yes, this is exactly how this is supposed to work AFAIK.
> >>
> >> Michael, are you sure that the watchdog was disabled in U-Boot when
> >> booting into the OS before this patch?
> >>
> >>> That said having a leave-wdt-running environment variable would work for
> >>> our use case.
> >>
> >> I would rather use it the other way around. Something like "wdt-stop-
> >> pre-os" to optionally stop the WDT before booting into the OS.
> >>
> >> Remark:
> >> IMHO, if you don't use the WDT in the OS, it does not make much sense
> >> to enable the WDT in U-Boot.
> >
> > Yes, we need to be very careful about making it so that a watchdog is
> > disabled and not re-enabled before moving on for a whole bunch of
> > reasons.  And the best option would be to just disable the watchdog if
> > it won't be used while the device is running the OS.
> >
> 
> The requirement of the UEFI specification is that if booting fails a
> system should reset after five minutes by default. We ensure this in the
> UEFI sub-system before ExitBootServices() using an EFI timer event.
> 
> In the UEFI sub-system we currently call in ExitBootServices():
> 
>         efi_set_watchdog(0); /* this disables the EFI timer */
>         WATCHDOG_RESET();
> 
> Is there any requirement to do more?

For EFI or ?  What I'm saying is that the watchdog must be left running
and not stopped, if we either:
- Came in to the world with the watchdog running AND were not
  specifically told to disable the watching.
- Came in to the world and were told to enable a watchdog.

It's that first case with the AND I'm concerned with in general and this
thread.

For the EFI case, I assume right now we aren't strictly adhering to the
5 minute rule, but I also assume there's some way for UEFI to tell us to
call WATCHDOG_RESET() as needed.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200921/d3b6efa6/attachment.sig>