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

[Michael Walle]

Am 2020-09-24 12:22, schrieb Mark Kettenis:
>> 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.

Mh, so I did the following:
  - applied Tom's "Convert CONFIG_WATCHDOG et al to Kconfig"
  - for c in configs/*_defconfig; do
      BOARD=$(echo $c | sed -e 's#configs/\(.*\)_defconfig#\1#')
      make ${BOARD}_defconfig
      mv .config ${BOARD}.config
    done
    grep -l CONFIG_WDT=y $(grep -l CONFIG_EFI_LOADER=y *.config)

This will list 50 boards, which has both CONFIG_WDT and
CONFIG_EFI_LOADER set.

-michael