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

[Heinrich Schuchardt]

On 9/23/20 10:23 PM, Tom Rini wrote:
> On Wed, Sep 23, 2020 at 10:01:29PM +0200, Heinrich Schuchardt wrote:
>> On 9/23/20 9:02 PM, Tom Rini wrote:
>>> On Wed, Sep 23, 2020 at 08:51:53PM +0200, Heinrich Schuchardt wrote:
>>>> On 9/23/20 7:53 PM, Mark Kettenis wrote:
>>>>>> Date: Wed, 23 Sep 2020 13:14:09 -0400
>>>>>> From: Tom Rini <trini at konsulko.com>
>>>>>>
>>>>>> 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.
>>>>>>>
>>>>
>>>> So 3) must not be allowed for CONFIG_EFI_LOADER=y which we have enabled
>>>> on most boards.
>>>
>>> No, EFI is going to need to figure out how to deal with the watchdog.
>>> See below...
>>
>> Any active watchdog after ExitBootServices() is forbidden by the UEFI spec.
>>
>> 3) does not describe disabling at ExitBootServices().
>
> Well, that's why I go back and forth on having the EFI loader say it's
> changing the current watchdog behavior.
>
> But just because EFI loader is enabled by default doesn't mean it gets
> to set the overall default behavior for watchdogs.  It's an option in a
> lot of cases where it's not actually used and a traditional method is
> instead used.
>
>>>>>>> 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
>>>>>> a surprise to the user.  I say this because it's a surprise to me and I
>>>>>> guess answers the question of "how does x86 handle this?" I had the
>>>>>> other day.
>>>>>
>>>>> So the UEFI requirement actually is:
>>>>>
>>>>> * Before starting an EFI payload a watchdog timer is started to reset
>>>>>   the system after 5 minutes.
>>>>>
>>>>> * This watchdog timer is cancelled as soon as the EFI payload calls
>>>>>   ExitBootServices().
>>>>
>>>> This requirement is currently fulfilled by a software watchdog in
>>>> lib/efi_loader/efi_watchdog.c. We need an emulation because many boards
>>>> don't offer a hardware watchdog in U-Boot.
>>>
>>> OK.
>>>
>>>>> The OpenBSD kernel in general does not supervise the watchdog unless
>>>>> explicitly requested to do so by the user.  What may happen is that
>>>>> the driver for the hardware stops the watchdog timer when it attaches,
>>>>> but (a) that is just a side-effect and (b) watchdog timer support
>>>>> isn't implemented for all supported SoCs.
>>>>>
>>>>> The OpenBSD EFI bootloader does explicitly disable the watchdog timer
>>>>> though by calling SetWatchdogTime() as soon as it starts.  This is to
>>>>> prevent an automatic reboot if you leave it sitting at the boot prompt
>>>>> for more than 5 minutes.  This is done across all our architectures
>>>>> that support EFI, including amd64.  So maybe that hides any
>>>>> non-conforming behaviour.
>>>>>
>>>>
>>>> SetWatchdogTime() resets the software watchdog implemented in
>>>> lib/efi_loader/efi_watchdog.c.
>>>>
>>>> When an UEFI payload reads a key via the simple text input protocol or
>>>> the extended simple text input protocol U-Boot calls efi_timer_check()
>>>> which invokes WATCHDOG_RESET(). This is why hardware watchdogs don't
>>>> kill an UEFI application waiting for keyboard input.
>>>
>>> I think we need to make things such that if there's a HW watchdog, our
>>> EFI loader/services make use of it.  And maybe just the Kconfig help
>>> text ends up spelling out what the UEFI spec says happens with
>>> watchdogs, so it's documented and expected behavior.
>>
>> There is a symbol CONFIG_WATCHDOG where mentioning UEFI might make
>> sense. But I could not find any software watchdog implementation
>> matching this symbol. Does it exist at all?
>
> Do we have a pure software watchdog?  I'm not 100% sure off-hand.
>
>>> My concern is that
>>> it's not obvious to the average user/developer what the behavior is
>>> going to be in this case and that they'll expect a watchdog to fire in a
>>> case where we've turned it off, and that's an important detail that's
>>> just within the spec.
>>>
>>
>> git grep -n hw_watchdog_reset
>> tells us that most boards don't have a hardware watchdog.
>
> That's the non-DM function.  And looking at where we have drivers for
> SoC watchdogs under drivers/watchdog/ I disagree with "most".  We keep
> adding more, even.
>
>> There is no maintainer for drivers/watchdog/wdt-uclass.c.
>
> Yes, just general "Is DM, check with Simon".
>
>> doc/README.watchdog provides no description on the usage.
>
> Given the amount of docs you've been writing of late (thanks!) yes,
> neither of us are surprised docs aren't in sync with implementation
> right now.
>
>> We do not even have a routine to set a time out period in struct wdt_ops.
>
> Changing the timeout at run time?

That is what EFI_BOOT_SERVICES.SetWatchdogTimer() is about.

The UEFI application can set the watchdog timeout to a multiple of 1
second (without upper limit) or disable it completely.

But this must not interfere with our serial drivers which are using
hardware watchdogs. And now look at

include/configs/M5373EVB.h:27:
#define CONFIG_WATCHDOG_TIMEOUT   3360
/* timeout in ms, max is 3.36 sec */

The current use of hardware watchdog does not fit well to UEFI.

A possible design would be a 100 Hz watchdog, to which U-Boot drivers
can add listeners.

Best regards

Heinrich

>
>> Hardware watchdogs are already used by some serial drivers. So we cannot
>> independently use them for UEFI.
>
> Yes, that's where we typically service the watchdog.
>
>> This does not look enticing.
>
> OK.  So long as you don't break the non-EFI case when EFI is enabled,
> whatever needs doing to meet the spec, until someone has time to poke
> this area a bit harder still.
>