[PATCH 2/2] watchdog: add watchdog behavior configuration

[Mark Kettenis]

> Content-Type: text/plain; charset=US-ASCII;
>  format=flowed
> Date: Thu, 24 Sep 2020 10:20:30 +0200
> From: Michael Walle <michael at walle.cc>
> Cc: trini at konsulko.com, xypron.glpk at gmx.de, u-boot at lists.denx.de,
>         agraf at csgraf.de, marex at denx.de, judge.packham at gmail.com, sr at denx.de,
>         rayagonda.kokatanur at broadcom.com
> 
> Am 2020-09-24 10:10, schrieb Mark Kettenis:
> >> Date: Thu, 24 Sep 2020 09:33:50 +0200
> >> From: Michael Walle <michael at walle.cc>
> >> 
> >> Am 2020-09-23 19:35, schrieb Tom Rini:
> >> > On Wed, Sep 23, 2020 at 07:31:00PM +0200, Heinrich Schuchardt wrote:
> >> >> On 9/23/20 7:14 PM, Tom Rini wrote:
> >> >> > On Wed, Sep 23, 2020 at 07:01:54PM +0200, Mark Kettenis wrote:
> >> >> >>> From: Michael Walle <michael at walle.cc>
> >> >> >>> Date: Wed, 23 Sep 2020 18:45:27 +0200
> >> >> >>>
> >> >> >>> Let the user choose between three different behaviours of the watchdog:
> >> >> >>>  (1) Keep the watchdog disabled
> >> >> >>>  (2) Supervise u-boot
> >> >> >>>  (3) Supervise u-boot and the operating systen (default)
> >> >> >>>
> >> >> >>> Option (2) will disable the watchdog right before handing control to the
> >> >> >>> operating system. This is useful when the OS is not aware of the
> >> >> >>> watchdog. Option (3) doesn't disable the watchdog and assumes the OS
> >> >> >>> will continue servicing.
> >> >> >>
> >> >> >> (3) can't be the default, at least for EFI
> >> >> >>
> >> >> >> The UEFI standard explicitly says that upon calling
> >> >> >> ExitBootServices(), the watchdog timer is disabled.
> >> >> >>
> >> >> >> In general, you can't expect an OS to have support for a particular
> >> >> >> watchdog timer.  So (3) only makes sense in cases where U-Boot is
> >> >> >> bundled with an OS image.
> >> >> >
> >> >> > We need to be careful here then.  The current and historical / generally
> >> >> > expected behavior is if we've enabled the watchdog we supervise it and
> >> >> > leave it enabled for the OS.  Given what UEFI requires I'd like to see
> >> >> > that case handled with a print about disabling the watchdog so it's not
> >> 
> >> I agree with "current and historical behavior" but not with "expected
> >> behavior".
> >> 
> >> I was thinking about something like
> >> 
> >> +choice
> >> +       prompt "Watchdog behavior"
> >> +       default WATCHDOG_SUPERVISE_U_BOOT if EFI_LOADER
> >> +       default WATCHDOG_SUPERVISE_OS if !EFI_LOADER
> >> +       depends on WDT
> >> 
> >> Unfortunately, EFI_LOADER is default y for any architecture != ARM.
> >> Therefore, it is likely we are changing the behavior of some boards
> >> and I agree this isn't what we want.
> > 
> > I think you are misreading that.  The following stanza:
> > 
> >         depends on OF_LIBFDT && ( \
> >                 ARM && (SYS_CPU = arm1136 || \
> >                         SYS_CPU = arm1176 || \
> >                         SYS_CPU = armv7   || \
> >                         SYS_CPU = armv8)  || \
> >                 X86 || RISCV || SANDBOX)
> > 
> > means that EFI_LOADER is onlu ever defined on (newish) ARM, X86, RISCV
> > and SANDBOX.  Which makes sense since there is no EFI calling
> > convention defined for other architectures like MIPS and PPC.
> 
> I've missed that, but that will just limit the search space.
> Like how can we figure out what board has both EFI_LOADER=y and
> WDT=y set? If there is no such board, then we can use the
> defaults above. If there are such boards we will have to put
> CONFIG_SUPERVISE_OS=y in their defconfig.
> 
> If EFI_LOADER would have had no default y, then a simple
> grep for EFI_LOADER=y (and WDT=y) would have been sufficient.

And this is where the conflict of interest surfaces.

>From a "running a generic OS" standpoint of view we really want to
have EFI_LOADER enabled on as many ARM/X86/RISCV boards as possible
and want to discourage the sometimes heavy customization that some of
the board vendors have done historically.  Such customizations often
break booting a generic OS or at least create inconsistencies tat are
confusing to users of such a generic OS. And WDT=y pretty is such a
customization.

At the same time there obviously is the desire from some vendors to
integrate U-Boot with an OS (which in practice probably always is a
customized Linux kernel).  This scenario typically uses "bootm"
instead of the EFI loader and I believe that only in this context the
historic watchdog behaviour makes sense.

Therefore I think it makes sense to always disable any running
hardware watchdog timer when starting an EFI payload, and reenable it
if that payload returns to the bootloader.  I don't think printing a
message when doing so makes sense as users of the EFI loader expect
any watchdog timer to be disabled, but printing a message that a
hardware watchdog is being disabled before starting the EFI payload
may be acceptable.

Please note that very few ARM board configurations actually set WDT=y,
so generic OS users may simply not have noticed issues with the
current "policy" of leaving a hardware watchdog running when booting
a generic OS.