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

Michael Walle
2020-09-24T21:05:25Z

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@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@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