u-boot leaves watchdog enabled by default

[Tom Rini]

On Tue, Sep 22, 2020 at 03:18:58PM +0200, Michael Walle wrote:
> Hi,
> 
> Am 2020-09-22 14:36, schrieb Tom Rini:
> > On Tue, Sep 22, 2020 at 08:59:00AM +0200, Michael Walle wrote:
> > > Hi,
> > > 
> > > Am 2020-09-22 03:18, schrieb Tom Rini:
> > > > On Mon, Sep 21, 2020 at 10:56:14PM +0200, Michael Walle wrote:
> > > > > Hi,
> > > > >
> > > [..]
> > > > > > > >>>      >> 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
> > > >
> > > > OK, but why is CONFIG_WDT enabled if you don't want to use the watchdog?
> > > 
> > > I guess we agree, that there are good reasons to have watchdog
> > > support in
> > > the bootloader (and even to keep in on before starting an OS). Think
> > > of
> > > tailored embedded operating systems for a specifc use case.
> > > In fact, for my board, the initial watchdog might even be enabled
> > > before
> > > u-boot and supervises the bootloader startup and switches to a
> > > failsafe
> > > image in case of an error. Thus, there is also a handy command "wdt
> > > expire 1" to restart into that image manually.
> > > 
> > > OTOH, I really want to support generic distributions which doesn't
> > > know
> > > anything about an already running watchdog.
> > > 
> > > Oh and I want the user to be able to install and boot a distribution
> > > without any change to the bootloader environment. Therefore, the
> > > default for this board has to be "watchdog disabled before booting
> > > OS". Like I said, I'm fine with having a
> > >   #define ENV "disable-wdt-pre-os"
> > > in the board configuration.
> > 
> > The next question I have, and I didn't see a good answer to yet in a
> > quick search is, how does this work out on x86 server hardware?  Are you
> > supposed to disable the watchdog before installing there too?
> 
> I can only guess here, but there seems to be no watchdog driver support
> at all (i.e. there are no wdt modules available).

I really do wonder.  Perhaps it's just some EFI service that's generally
handled?

> > But all that said, since we have "wdt stop", perhaps you can find a
> > place to put that in the boot script?  Or just declare that if we get
> > far enough to run preboot cmd then it's good enough, and update your
> > call to "wdt expire 1" to be "wdt start && wdt expire 1" ?
> 
> "wdt expire 1" will automatically start the watchdog, won't it? Anyway,
> it was just an example why I need the CONFIG_WDT.

Right, OK.  I'm just wondering if we can use the existing "wdt stop"
functionality to cover what you're aiming for.

> Just to get your opinion correct on this topic: you say as soon as the
> CONFIG_WDT is enabled u-boot will start it and never stop it, although
> CONFIG_WDT is just "Enable driver model watchdog timer drivers" and the
> help text is just about that, too.

I will agree that the help text and symbols can use further cleaning up
still.  CONFIG_WDT implies in CONFIG_WATCHDOG which says:

          This option enables U-Boot watchdog support where U-Boot is using
          watchdog_reset function to service watchdog device in U-Boot. Enable
          this option if you want to service enabled watchdog by U-Boot. Disable
          this option if you want U-Boot to start watchdog but never service it.

Which is what we've done (to the best of my knowledge) "forever".

> IMHO it is wrong to enable the watchdog together with that option. There
> should be another one (even defaulting to 'yes') which tells u-boot whether
> it should be enabled by default.
> 
> config WDT_AUTOSTART
>    boot "Start the (first) watchdog by default"
>    default y
>    help
>      Upon u-boot startup the first watchdog will be started automatically.
>      Be aware, that it will also kept enabled after the bootloader starts
>      the operation system!

Now, given what I said above looking at commit 06985289d452 ("watchdog:
Implement generic watchdog_reset() version") is where we get the current
behavior, symbol-wise.  At this point, I'm not quite sure how best to do
what you're looking for, or if we just have a bug in terms of which
symbols are used.  It sounds like you just want to stop the watchdog in
U-Boot (outside of a specific start-and-trigger case) and let the OS
decide if it's going to enable it.  And if the OS is going to enable it
and you want the watchdog started before OS boot, preboot could be set
in the environment to "wdt start" to get it going again (and you enable
CONFIG_USE_PREBOOT and set CONFIG_PREBOOT to an empty string, or wdt
stop?).  Or does that still not cover what you're trying to do?

-- 
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/20200922/a31244b3/attachment.sig>