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

Michael Walle
2020-09-24T07:33:50Z

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.

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