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

[Mark Kettenis]

> Content-Type: text/plain; charset=US-ASCII;
>  format=flowed
> Date: Thu, 24 Sep 2020 22:30:24 +0200
> From: Michael Walle <michael at walle.cc>
> Cc: Heinrich Schuchardt <xypron.glpk at gmx.de>,
>         Mark Kettenis
>  <mark.kettenis at xs4all.nl>, 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 15:19, schrieb Tom Rini:
> > On Thu, Sep 24, 2020 at 09:33:50AM +0200, Michael Walle wrote:
> >> Am 2020-09-23 19:35, schrieb Tom Rini:
> [..]
> 
> >> > > Not printf(), maybe log_info().
> >> > >
> >> > > The disabling has to occur in ExitBootServices() (aka.
> >> > > efi_exit_boot_services()). Here we are in the middle of an executing
> >> > > UEFI application. Printing anything on the screen may mess up the
> >> > > output
> >> > > of the UEFI application.
> >> > >
> >> > > So, please, don't output anything.
> >> >
> >> > We need to find a good way to inform the user we're disabling their
> >> > watchdog.  Maybe before we fully jump in to UEFI note that it will be
> >> > disabled before entering the OS?  Or something a bit more generally
> >> > understood than ExitBootServices() having been called.  I don't know
> >> > _where_ the best place is, but I think it's important to inform the
> >> > user.
> >> 
> >> The watchdog is only disabled in the "supervise u-boot" mode, why
> >> would we need to inform the user? It was the users choice to have
> >> the timer only enabled in u-boot.
> >> 
> >> Or do you mean if for example the vendor chooses that option and
> >> in this case the user doesn't know anything about it? The mode
> >> is indicated in the "WDT:" output.
> > 
> > I'm talking about the case where we say we've enabled the WDT to
> > supervise OS, but then bootefi something and have disabled the watchdog
> > (to meet UEFI requirements) but didn't tell the user we've turned off
> > the WDT that we had told them is on.
> 
> Ah, do you really want to have a different behavior between bootm and
> bootefi? Thats even more suprising IMHO.

I fear it is the only way to support both user communities.

> I had the following in mind:
> 
> +config WATCHDOG_SUPERVISE_OS
> +       bool "Supervise U-boot and operating system"
> +       help
> +         Upon U-Boot startup the first watchdog will be started 
> automatically
> +         and kept running even after booting the operating system.
> +         Be aware, that the operating system needs to service the 
> watchdog!
> +
> +         Additionally, this is not UEFI compliant because:
> +          - the timeout won't be set to 5 minutes before starting the 
> OS and
> +          - the watchdog timer isn't stopped after the OS calls
> +            ExitBootServices().

That would be highly problematic:

* It is unfeasable for an EFI OS bootloader to include drivers for the
  hardware watchdog.  This means that the time available for the user
  to interact with the bootloader is limited by the timeout of the
  hardware watchdog.  Consider for example the case of full disk
  encryption where the user is supposed to enter a password before the
  OS can be loaded.  The hardware watchdog would limit the amount of
  time available to enter that password.  If the timeout is seconds
  rather than minutes the system becomes unusable.

* This requires the OS to have support for the hardware watchdog.
  Consider the case where a U-Boot with a default configuration has
  been programmed in SPI flash.  Without the hardware watchdog running
  you can install an arbitrary OS using an EFI bootable image.  With
  the hardware watchdog running an OS lacking support for the hardware
  watchdog would probably reboot in the middle of an install.  Even
  when the kernel that will be ultimately installed supports the
  hardware watchdog, the kernel used by the OS installer may not.  For
  example a Linux kernel where the watchdog timer driver is built as a
  module and that module isn't present on the install image.