> Date: Thu, 24 Sep 2020 09:33:50 +0200
> From: Michael Walle <mich...@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 <mich...@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.
> >> 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.
>
> -michael
>
