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

[Tom Rini]

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?

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

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200923/d9f53a1a/attachment.sig>