u-boot leaves watchdog enabled by default

[Michael Walle]

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?

-michael