u-boot leaves watchdog enabled by default

[Heinrich Schuchardt]

On 9/21/20 10:56 PM, Michael Walle wrote:
> Hi,
>
> Am 2020-09-21 20:50, schrieb Tom Rini:
>> On Mon, Sep 21, 2020 at 08:29:00PM +0200, Heinrich Schuchardt wrote:
>>> On 9/21/20 7:30 PM, Tom Rini wrote:
>>> > On Mon, Sep 21, 2020 at 11:01:37AM +0200, Stefan Roese wrote:
>>> >> Hi Michael,
>>> >> Hi Chris,
>>> >>
>>> >> On 15.09.20 12:44, Chris Packham wrote:
>>> >>>
>>> >>>
>>> >>> On Tue, 15 Sep 2020, 7:54 PM Michael Walle, <michael at walle.cc>
>>> wrote:
>>> >>>
>>> >>>     Am 2020-09-15 09:44, schrieb Rayagonda Kokatanur:
>>> >>>      > On Tue, Sep 15, 2020 at 12:56 PM Michael Walle
>>> <michael at walle.cc>
>>> >>>      > wrote:
>>> >>>      >>
>>> >>>      >> Hi Stefan,
>>> >>>      >>
>>> >>>      >> it appears that since commit 06985289d45 ("watchdog:
>>> Implement
>>> >>>     generic
>>> >>>      >> watchdog_reset() version") - by default - the first
>>> watchdog is
>>> >>>      >> started
>>> >>>      >> unconditionally if CONFIG_WDT is set but never stopped
>>> before
>>> >>>     booting
>>> >>>      >> the operating system.
>>> >>>      >>
>>> >>>      >> Shouldn't it also be stopped uncondionally? What's worse
>>> is that on
>>> >>>      >> one
>>> >>>      >> board/arch the watchdog is stopped in arch_preboot_os()
>>> which is
>>> >>>     never
>>>
>>> Which board are you referring to?
>
> See the commit above. It is board/alliedtelesis/x530/x530.c. It might
> not use
> EFI, but I tried to use it as a blueprint to disable the watchdog by
> default
> and then noticed it won't work in the bootefi case (and I guess the 'go'
> case).
>
>>>
>>> >>>      >> called in the bootefi case. So even if I'd do a
>>> workaround and
>>> >>>     stop it
>>> >>>      >> manually in my board code, I couldn't do that
>>> consistently for
>>> >>>      >> bootm/bootefi.
>>> >>>      >>
>>> >>>      >> Or am I missing something here?
>>> >>>      >
>>> >>>      > Define CONFIG_WATCHDOG.
>>> >>>      > This takes care of resetting wdt.
>>> >>>
>>> >>>     Yes as along as you're inside the bootloader, but when u-boot
>>> hands
>>> >>>     control over the OS the watchdog is not serviced anymore;
>>> which wouldn't
>>> >>>     be a problem per se, but it is enabled unconditionally by
>>> u-boot.
>>> >>>
>>> >>>
>>> >>> Just to add some data. At $dayjob we use this behaviour as a
>>> failsafe to
>>> >>> make sure our userspace gets to a point where it is servicing the
>>> >>> watchdog.
>>> >>
>>> >> Yes, this is exactly how this is supposed to work AFAIK.
>>> >>
>>> >> Michael, are you sure that the watchdog was disabled in U-Boot when
>>> >> booting into the OS before this patch?
>>> >>
>>> >>> That said having a leave-wdt-running environment variable would
>>> work for
>>> >>> our use case.
>>> >>
>>> >> I would rather use it the other way around. Something like "wdt-stop-
>>> >> pre-os" to optionally stop the WDT before booting into the OS.
>>> >>
>>> >> Remark:
>>> >> IMHO, if you don't use the WDT in the OS, it does not make much sense
>>> >> to enable the WDT in U-Boot.
>>> >
>>> > Yes, we need to be very careful about making it so that a watchdog is
>>> > disabled and not re-enabled before moving on for a whole bunch of
>>> > reasons.  And the best option would be to just disable the watchdog if
>>> > it won't be used while the device is running the OS.
>>> >
>>>
>>> The requirement of the UEFI specification is that if booting fails a
>>> system should reset after five minutes by default. We ensure this in the
>>> UEFI sub-system before ExitBootServices() using an EFI timer event.
>>>
>>> In the UEFI sub-system we currently call in ExitBootServices():
>>>
>>>         efi_set_watchdog(0); /* this disables the EFI timer */
>>>         WATCHDOG_RESET();
>>>
>>> Is there any requirement to do more?
>>
>> For EFI or ?  What I'm saying is that the watchdog must be left running
>> and not stopped, if we either:
>> - Came in to the world with the watchdog running AND were not
>>   specifically told to disable the watching.
>> - Came in to the world and were told to enable a watchdog.
>
> My reason to start this thread was the fact that a watchdog is started
> by default in a generic way (i.e. initr_watchdog()) but there is _no_
> way to disable it. I'm having a minimal board configuration and I want
> to be able to boot the debian-installer via EFI -> grub-efi -> d-i.
> The debian installer is not aware of any watchdog. Thus if u-boot
> leave it running, it might bite at very inconvenient times like
> half through the installation.
>
> I'm fine with having a unified way to disable the watchdog per board,
> let it be a CONFIG_WDT_NO_START or a #define ENV "wdt-stop-pre-os", but
> it should work with bootm/booti/go/bootefi.
>
>> It's that first case with the AND I'm concerned with in general and this
>> thread.
>>
>> For the EFI case, I assume right now we aren't strictly adhering to the
>> 5 minute rule, but I also assume there's some way for UEFI to tell us to
>> call WATCHDOG_RESET() as needed.
>
> EFI timers seems to be unrelated to the watchdog, right?

The UEFI spec requires a watchdog with 5 min default time that can be be
manipulated via the UEFI API. To be independent of what the hardware
offers I implemented it only relying on timer ticks and not actually
using a hardware watchdog. Hence the current implementation does not
interfere with hardware watchdogs.

Best regards

Heinrich