u-boot leaves watchdog enabled by default

[Chris Packham]

On Tue, Sep 22, 2020 at 8:56 AM Michael Walle <michael at walle.cc> wrote:
>
> Hi,
>
> Am 2020-09-21 20:50, schrieb 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?
>
> See the commit above. It is board/alliedtelesis/x530/x530.c. It might
> not use
> EFI, but I tried to use it as a blueprint to disable the watchdog by
> default
> and then noticed it won't work in the bootefi case (and I guess the 'go'
> case).
>

Yes that's the in-tree board we have that uses this although it's for
different reasons (HW related). It doesn't use EFI. We had to add code
for that board to disable the watchdog pre-boot to maintain
compatibility with an old userland. Had we been starting from fresh we
would have just made sure that the userland was able to service the
watchdog (which we are doing for newer boards).

> >>
> >> >>>      >> 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.
>
> My reason to start this thread was the fact that a watchdog is started
> by default in a generic way (i.e. initr_watchdog()) but there is _no_
> way to disable it. I'm having a minimal board configuration and I want
> to be able to boot the debian-installer via EFI -> grub-efi -> d-i.
> The debian installer is not aware of any watchdog. Thus if u-boot
> leave it running, it might bite at very inconvenient times like
> half through the installation.
>
> I'm fine with having a unified way to disable the watchdog per board,
> let it be a CONFIG_WDT_NO_START or a #define ENV "wdt-stop-pre-os", but
> it should work with bootm/booti/go/bootefi.
>
> > 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.
>
> EFI timers seems to be unrelated to the watchdog, right?
>
> -michael