All of lore.kernel.org
 help / color / mirror / Atom feed
* u-boot leaves watchdog enabled by default
@ 2020-09-15  7:26 Michael Walle
  2020-09-15  7:44 ` Rayagonda Kokatanur
  0 siblings, 1 reply; 19+ messages in thread
From: Michael Walle @ 2020-09-15  7:26 UTC (permalink / raw)
  To: u-boot

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

The SoC on my board has a built-in watchdog and I've noticed this 
behaviour when I was trying to install debian via its stock installer 
which doesn't have any driver support for it.

-michael

^ permalink raw reply	[flat|nested] 19+ messages in thread

* u-boot leaves watchdog enabled by default
  2020-09-15  7:26 u-boot leaves watchdog enabled by default Michael Walle
@ 2020-09-15  7:44 ` Rayagonda Kokatanur
  2020-09-15  7:54   ` Michael Walle
  0 siblings, 1 reply; 19+ messages in thread
From: Rayagonda Kokatanur @ 2020-09-15  7:44 UTC (permalink / raw)
  To: u-boot

On Tue, Sep 15, 2020 at 12:56 PM Michael Walle <michael@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
> 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.

Best regards,
Rayagonda
>
> The SoC on my board has a built-in watchdog and I've noticed this
> behaviour when I was trying to install debian via its stock installer
> which doesn't have any driver support for it.
>
> -michael

^ permalink raw reply	[flat|nested] 19+ messages in thread

* u-boot leaves watchdog enabled by default
  2020-09-15  7:44 ` Rayagonda Kokatanur
@ 2020-09-15  7:54   ` Michael Walle
  2020-09-15 10:44     ` Chris Packham
  0 siblings, 1 reply; 19+ messages in thread
From: Michael Walle @ 2020-09-15  7:54 UTC (permalink / raw)
  To: u-boot

Am 2020-09-15 09:44, schrieb Rayagonda Kokatanur:
> On Tue, Sep 15, 2020 at 12:56 PM Michael Walle <michael@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
>> 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.

-michael

^ permalink raw reply	[flat|nested] 19+ messages in thread

* u-boot leaves watchdog enabled by default
  2020-09-15  7:54   ` Michael Walle
@ 2020-09-15 10:44     ` Chris Packham
  2020-09-21  9:01       ` Stefan Roese
  0 siblings, 1 reply; 19+ messages in thread
From: Chris Packham @ 2020-09-15 10:44 UTC (permalink / raw)
  To: u-boot

On Tue, 15 Sep 2020, 7:54 PM Michael Walle, <michael@walle.cc> wrote:

> Am 2020-09-15 09:44, schrieb Rayagonda Kokatanur:
> > On Tue, Sep 15, 2020 at 12:56 PM Michael Walle <michael@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
> >> 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.
That said having a leave-wdt-running environment variable would work for
our use case.

>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* u-boot leaves watchdog enabled by default
  2020-09-15 10:44     ` Chris Packham
@ 2020-09-21  9:01       ` Stefan Roese
  2020-09-21 17:30         ` Tom Rini
  2020-09-21 20:41         ` Michael Walle
  0 siblings, 2 replies; 19+ messages in thread
From: Stefan Roese @ 2020-09-21  9:01 UTC (permalink / raw)
  To: u-boot

Hi Michael,
Hi Chris,

On 15.09.20 12:44, Chris Packham wrote:
> 
> 
> On Tue, 15 Sep 2020, 7:54 PM Michael Walle, <michael@walle.cc> wrote:
> 
>     Am 2020-09-15 09:44, schrieb Rayagonda Kokatanur:
>      > On Tue, Sep 15, 2020 at 12:56 PM Michael Walle <michael@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
>      >> 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.

Thanks,
Stefan

^ permalink raw reply	[flat|nested] 19+ messages in thread

* u-boot leaves watchdog enabled by default
  2020-09-21  9:01       ` Stefan Roese
@ 2020-09-21 17:30         ` Tom Rini
  2020-09-21 18:29           ` Heinrich Schuchardt
  2020-09-21 20:41         ` Michael Walle
  1 sibling, 1 reply; 19+ messages in thread
From: Tom Rini @ 2020-09-21 17:30 UTC (permalink / raw)
  To: u-boot

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@walle.cc> wrote:
> > 
> >     Am 2020-09-15 09:44, schrieb Rayagonda Kokatanur:
> >      > On Tue, Sep 15, 2020 at 12:56 PM Michael Walle <michael@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
> >      >> 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.

-- 
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/20200921/9a31b769/attachment.sig>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* u-boot leaves watchdog enabled by default
  2020-09-21 17:30         ` Tom Rini
@ 2020-09-21 18:29           ` Heinrich Schuchardt
  2020-09-21 18:50             ` Tom Rini
  0 siblings, 1 reply; 19+ messages in thread
From: Heinrich Schuchardt @ 2020-09-21 18:29 UTC (permalink / raw)
  To: u-boot

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@walle.cc> wrote:
>>>
>>>     Am 2020-09-15 09:44, schrieb Rayagonda Kokatanur:
>>>      > On Tue, Sep 15, 2020 at 12:56 PM Michael Walle <michael@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?

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

Best regards

Heinrich

^ permalink raw reply	[flat|nested] 19+ messages in thread

* u-boot leaves watchdog enabled by default
  2020-09-21 18:29           ` Heinrich Schuchardt
@ 2020-09-21 18:50             ` Tom Rini
  2020-09-21 20:56               ` Michael Walle
  0 siblings, 1 reply; 19+ messages in thread
From: Tom Rini @ 2020-09-21 18:50 UTC (permalink / raw)
  To: u-boot

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@walle.cc> wrote:
> >>>
> >>>     Am 2020-09-15 09:44, schrieb Rayagonda Kokatanur:
> >>>      > On Tue, Sep 15, 2020 at 12:56 PM Michael Walle <michael@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?
> 
> >>>      >> 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.

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.

-- 
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/20200921/d3b6efa6/attachment.sig>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* u-boot leaves watchdog enabled by default
  2020-09-21  9:01       ` Stefan Roese
  2020-09-21 17:30         ` Tom Rini
@ 2020-09-21 20:41         ` Michael Walle
  1 sibling, 0 replies; 19+ messages in thread
From: Michael Walle @ 2020-09-21 20:41 UTC (permalink / raw)
  To: u-boot

Hi Stefan,

Am 2020-09-21 11:01, schrieb Stefan Roese:
> Hi Michael,
> Hi Chris,
> 
> On 15.09.20 12:44, Chris Packham wrote:
>> 
>> 
>> On Tue, 15 Sep 2020, 7:54 PM Michael Walle, <michael@walle.cc> wrote:
>> 
>>     Am 2020-09-15 09:44, schrieb Rayagonda Kokatanur:
>>      > On Tue, Sep 15, 2020 at 12:56 PM Michael Walle 
>> <michael@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
>>      >> 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?

If I read the patch correctly, it was per board, wasn't it?

At the moment you end up with a watchdog enabled if you have
  (a) CONFIG_WDT set,
  (b) A watchdog defined in the device tree

And no way to stop it reliably (well you could use some kind of bootcmd,
but I don't think that is the way to go).

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

I'm fine with either one. At least I can stop the watchdog reliably.

OTOH a configuration option to not start a watchdog in the first place,
should still be a valid choice.

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

Keep in mind that my board is a "generic" module. I.e. there might be
customers which may want to start a watchdog and there might be 
customers
(that is also the debian-installer, for example) which doesn't want it
or have no driver for it. This makes even more sense with EFI in place,
which provides a universal boot method for any distribution/os (that
might not be aware that a watchdog is running).

-michael

^ permalink raw reply	[flat|nested] 19+ messages in thread

* u-boot leaves watchdog enabled by default
  2020-09-21 18:50             ` Tom Rini
@ 2020-09-21 20:56               ` Michael Walle
  2020-09-21 21:10                 ` Chris Packham
                                   ` (2 more replies)
  0 siblings, 3 replies; 19+ messages in thread
From: Michael Walle @ 2020-09-21 20:56 UTC (permalink / raw)
  To: u-boot

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@walle.cc> wrote:
>> >>>
>> >>>     Am 2020-09-15 09:44, schrieb Rayagonda Kokatanur:
>> >>>      > On Tue, Sep 15, 2020 at 12:56 PM Michael Walle <michael@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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* u-boot leaves watchdog enabled by default
  2020-09-21 20:56               ` Michael Walle
@ 2020-09-21 21:10                 ` Chris Packham
  2020-09-22  0:52                 ` Heinrich Schuchardt
  2020-09-22  1:18                 ` Tom Rini
  2 siblings, 0 replies; 19+ messages in thread
From: Chris Packham @ 2020-09-21 21:10 UTC (permalink / raw)
  To: u-boot

On Tue, Sep 22, 2020 at 8:56 AM Michael Walle <michael@walle.cc> 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@walle.cc> wrote:
> >> >>>
> >> >>>     Am 2020-09-15 09:44, schrieb Rayagonda Kokatanur:
> >> >>>      > On Tue, Sep 15, 2020 at 12:56 PM Michael Walle <michael@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).
>

Yes that's the in-tree board we have that uses this although it's for
different reasons (HW related). It doesn't use EFI. We had to add code
for that board to disable the watchdog pre-boot to maintain
compatibility with an old userland. Had we been starting from fresh we
would have just made sure that the userland was able to service the
watchdog (which we are doing for newer boards).

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* u-boot leaves watchdog enabled by default
  2020-09-21 20:56               ` Michael Walle
  2020-09-21 21:10                 ` Chris Packham
@ 2020-09-22  0:52                 ` Heinrich Schuchardt
  2020-09-22  1:18                 ` Tom Rini
  2 siblings, 0 replies; 19+ messages in thread
From: Heinrich Schuchardt @ 2020-09-22  0:52 UTC (permalink / raw)
  To: u-boot

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@walle.cc>
>>> wrote:
>>> >>>
>>> >>>???? Am 2020-09-15 09:44, schrieb Rayagonda Kokatanur:
>>> >>>????? > On Tue, Sep 15, 2020 at 12:56 PM Michael Walle
>>> <michael@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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* u-boot leaves watchdog enabled by default
  2020-09-21 20:56               ` Michael Walle
  2020-09-21 21:10                 ` Chris Packham
  2020-09-22  0:52                 ` Heinrich Schuchardt
@ 2020-09-22  1:18                 ` Tom Rini
  2020-09-22  6:59                   ` Michael Walle
  2 siblings, 1 reply; 19+ messages in thread
From: Tom Rini @ 2020-09-22  1:18 UTC (permalink / raw)
  To: u-boot

On Mon, Sep 21, 2020 at 10:56:14PM +0200, 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@walle.cc> wrote:
> > > >>>
> > > >>>     Am 2020-09-15 09:44, schrieb Rayagonda Kokatanur:
> > > >>>      > On Tue, Sep 15, 2020 at 12:56 PM Michael Walle <michael@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

OK, but why is CONFIG_WDT enabled if you don't want to use the watchdog?

-- 
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/20200921/e4407325/attachment.sig>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* u-boot leaves watchdog enabled by default
  2020-09-22  1:18                 ` Tom Rini
@ 2020-09-22  6:59                   ` Michael Walle
  2020-09-22 12:36                     ` Tom Rini
  0 siblings, 1 reply; 19+ messages in thread
From: Michael Walle @ 2020-09-22  6:59 UTC (permalink / raw)
  To: u-boot

Hi,

Am 2020-09-22 03:18, schrieb Tom Rini:
> On Mon, Sep 21, 2020 at 10:56:14PM +0200, Michael Walle wrote:
>> Hi,
>> 
[..]
>> > > >>>      >> 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
> 
> OK, but why is CONFIG_WDT enabled if you don't want to use the 
> watchdog?

I guess we agree, that there are good reasons to have watchdog support 
in
the bootloader (and even to keep in on before starting an OS). Think of
tailored embedded operating systems for a specifc use case.
In fact, for my board, the initial watchdog might even be enabled before
u-boot and supervises the bootloader startup and switches to a failsafe
image in case of an error. Thus, there is also a handy command "wdt
expire 1" to restart into that image manually.

OTOH, I really want to support generic distributions which doesn't know
anything about an already running watchdog.

Oh and I want the user to be able to install and boot a distribution
without any change to the bootloader environment. Therefore, the
default for this board has to be "watchdog disabled before booting
OS". Like I said, I'm fine with having a
   #define ENV "disable-wdt-pre-os"
in the board configuration.

-michael

^ permalink raw reply	[flat|nested] 19+ messages in thread

* u-boot leaves watchdog enabled by default
  2020-09-22  6:59                   ` Michael Walle
@ 2020-09-22 12:36                     ` Tom Rini
  2020-09-22 13:18                       ` Michael Walle
  0 siblings, 1 reply; 19+ messages in thread
From: Tom Rini @ 2020-09-22 12:36 UTC (permalink / raw)
  To: u-boot

On Tue, Sep 22, 2020 at 08:59:00AM +0200, Michael Walle wrote:
> Hi,
> 
> Am 2020-09-22 03:18, schrieb Tom Rini:
> > On Mon, Sep 21, 2020 at 10:56:14PM +0200, Michael Walle wrote:
> > > Hi,
> > > 
> [..]
> > > > > >>>      >> 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
> > 
> > OK, but why is CONFIG_WDT enabled if you don't want to use the watchdog?
> 
> I guess we agree, that there are good reasons to have watchdog support in
> the bootloader (and even to keep in on before starting an OS). Think of
> tailored embedded operating systems for a specifc use case.
> In fact, for my board, the initial watchdog might even be enabled before
> u-boot and supervises the bootloader startup and switches to a failsafe
> image in case of an error. Thus, there is also a handy command "wdt
> expire 1" to restart into that image manually.
> 
> OTOH, I really want to support generic distributions which doesn't know
> anything about an already running watchdog.
> 
> Oh and I want the user to be able to install and boot a distribution
> without any change to the bootloader environment. Therefore, the
> default for this board has to be "watchdog disabled before booting
> OS". Like I said, I'm fine with having a
>   #define ENV "disable-wdt-pre-os"
> in the board configuration.

The next question I have, and I didn't see a good answer to yet in a
quick search is, how does this work out on x86 server hardware?  Are you
supposed to disable the watchdog before installing there too?

But all that said, since we have "wdt stop", perhaps you can find a
place to put that in the boot script?  Or just declare that if we get
far enough to run preboot cmd then it's good enough, and update your
call to "wdt expire 1" to be "wdt start && wdt expire 1" ?

-- 
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/20200922/cdd51a75/attachment.sig>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* u-boot leaves watchdog enabled by default
  2020-09-22 12:36                     ` Tom Rini
@ 2020-09-22 13:18                       ` Michael Walle
  2020-09-22 14:41                         ` Tom Rini
  0 siblings, 1 reply; 19+ messages in thread
From: Michael Walle @ 2020-09-22 13:18 UTC (permalink / raw)
  To: u-boot

Hi,

Am 2020-09-22 14:36, schrieb Tom Rini:
> On Tue, Sep 22, 2020 at 08:59:00AM +0200, Michael Walle wrote:
>> Hi,
>> 
>> Am 2020-09-22 03:18, schrieb Tom Rini:
>> > On Mon, Sep 21, 2020 at 10:56:14PM +0200, Michael Walle wrote:
>> > > Hi,
>> > >
>> [..]
>> > > > > >>>      >> 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
>> >
>> > OK, but why is CONFIG_WDT enabled if you don't want to use the watchdog?
>> 
>> I guess we agree, that there are good reasons to have watchdog support 
>> in
>> the bootloader (and even to keep in on before starting an OS). Think 
>> of
>> tailored embedded operating systems for a specifc use case.
>> In fact, for my board, the initial watchdog might even be enabled 
>> before
>> u-boot and supervises the bootloader startup and switches to a 
>> failsafe
>> image in case of an error. Thus, there is also a handy command "wdt
>> expire 1" to restart into that image manually.
>> 
>> OTOH, I really want to support generic distributions which doesn't 
>> know
>> anything about an already running watchdog.
>> 
>> Oh and I want the user to be able to install and boot a distribution
>> without any change to the bootloader environment. Therefore, the
>> default for this board has to be "watchdog disabled before booting
>> OS". Like I said, I'm fine with having a
>>   #define ENV "disable-wdt-pre-os"
>> in the board configuration.
> 
> The next question I have, and I didn't see a good answer to yet in a
> quick search is, how does this work out on x86 server hardware?  Are 
> you
> supposed to disable the watchdog before installing there too?

I can only guess here, but there seems to be no watchdog driver support
at all (i.e. there are no wdt modules available).

> But all that said, since we have "wdt stop", perhaps you can find a
> place to put that in the boot script?  Or just declare that if we get
> far enough to run preboot cmd then it's good enough, and update your
> call to "wdt expire 1" to be "wdt start && wdt expire 1" ?

"wdt expire 1" will automatically start the watchdog, won't it? Anyway,
it was just an example why I need the CONFIG_WDT.

Just to get your opinion correct on this topic: you say as soon as the
CONFIG_WDT is enabled u-boot will start it and never stop it, although
CONFIG_WDT is just "Enable driver model watchdog timer drivers" and the
help text is just about that, too.

IMHO it is wrong to enable the watchdog together with that option. There
should be another one (even defaulting to 'yes') which tells u-boot 
whether
it should be enabled by default.

config WDT_AUTOSTART
    boot "Start the (first) watchdog by default"
    default y
    help
      Upon u-boot startup the first watchdog will be started 
automatically.
      Be aware, that it will also kept enabled after the bootloader 
starts
      the operation system!

-michael

^ permalink raw reply	[flat|nested] 19+ messages in thread

* u-boot leaves watchdog enabled by default
  2020-09-22 13:18                       ` Michael Walle
@ 2020-09-22 14:41                         ` Tom Rini
  2020-09-22 15:41                           ` Michael Walle
  0 siblings, 1 reply; 19+ messages in thread
From: Tom Rini @ 2020-09-22 14:41 UTC (permalink / raw)
  To: u-boot

On Tue, Sep 22, 2020 at 03:18:58PM +0200, Michael Walle wrote:
> Hi,
> 
> Am 2020-09-22 14:36, schrieb Tom Rini:
> > On Tue, Sep 22, 2020 at 08:59:00AM +0200, Michael Walle wrote:
> > > Hi,
> > > 
> > > Am 2020-09-22 03:18, schrieb Tom Rini:
> > > > On Mon, Sep 21, 2020 at 10:56:14PM +0200, Michael Walle wrote:
> > > > > Hi,
> > > > >
> > > [..]
> > > > > > > >>>      >> 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
> > > >
> > > > OK, but why is CONFIG_WDT enabled if you don't want to use the watchdog?
> > > 
> > > I guess we agree, that there are good reasons to have watchdog
> > > support in
> > > the bootloader (and even to keep in on before starting an OS). Think
> > > of
> > > tailored embedded operating systems for a specifc use case.
> > > In fact, for my board, the initial watchdog might even be enabled
> > > before
> > > u-boot and supervises the bootloader startup and switches to a
> > > failsafe
> > > image in case of an error. Thus, there is also a handy command "wdt
> > > expire 1" to restart into that image manually.
> > > 
> > > OTOH, I really want to support generic distributions which doesn't
> > > know
> > > anything about an already running watchdog.
> > > 
> > > Oh and I want the user to be able to install and boot a distribution
> > > without any change to the bootloader environment. Therefore, the
> > > default for this board has to be "watchdog disabled before booting
> > > OS". Like I said, I'm fine with having a
> > >   #define ENV "disable-wdt-pre-os"
> > > in the board configuration.
> > 
> > The next question I have, and I didn't see a good answer to yet in a
> > quick search is, how does this work out on x86 server hardware?  Are you
> > supposed to disable the watchdog before installing there too?
> 
> I can only guess here, but there seems to be no watchdog driver support
> at all (i.e. there are no wdt modules available).

I really do wonder.  Perhaps it's just some EFI service that's generally
handled?

> > But all that said, since we have "wdt stop", perhaps you can find a
> > place to put that in the boot script?  Or just declare that if we get
> > far enough to run preboot cmd then it's good enough, and update your
> > call to "wdt expire 1" to be "wdt start && wdt expire 1" ?
> 
> "wdt expire 1" will automatically start the watchdog, won't it? Anyway,
> it was just an example why I need the CONFIG_WDT.

Right, OK.  I'm just wondering if we can use the existing "wdt stop"
functionality to cover what you're aiming for.

> Just to get your opinion correct on this topic: you say as soon as the
> CONFIG_WDT is enabled u-boot will start it and never stop it, although
> CONFIG_WDT is just "Enable driver model watchdog timer drivers" and the
> help text is just about that, too.

I will agree that the help text and symbols can use further cleaning up
still.  CONFIG_WDT implies in CONFIG_WATCHDOG which says:

          This option enables U-Boot watchdog support where U-Boot is using
          watchdog_reset function to service watchdog device in U-Boot. Enable
          this option if you want to service enabled watchdog by U-Boot. Disable
          this option if you want U-Boot to start watchdog but never service it.

Which is what we've done (to the best of my knowledge) "forever".

> IMHO it is wrong to enable the watchdog together with that option. There
> should be another one (even defaulting to 'yes') which tells u-boot whether
> it should be enabled by default.
> 
> config WDT_AUTOSTART
>    boot "Start the (first) watchdog by default"
>    default y
>    help
>      Upon u-boot startup the first watchdog will be started automatically.
>      Be aware, that it will also kept enabled after the bootloader starts
>      the operation system!

Now, given what I said above looking at commit 06985289d452 ("watchdog:
Implement generic watchdog_reset() version") is where we get the current
behavior, symbol-wise.  At this point, I'm not quite sure how best to do
what you're looking for, or if we just have a bug in terms of which
symbols are used.  It sounds like you just want to stop the watchdog in
U-Boot (outside of a specific start-and-trigger case) and let the OS
decide if it's going to enable it.  And if the OS is going to enable it
and you want the watchdog started before OS boot, preboot could be set
in the environment to "wdt start" to get it going again (and you enable
CONFIG_USE_PREBOOT and set CONFIG_PREBOOT to an empty string, or wdt
stop?).  Or does that still not cover what you're trying to do?

-- 
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/20200922/a31244b3/attachment.sig>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* u-boot leaves watchdog enabled by default
  2020-09-22 14:41                         ` Tom Rini
@ 2020-09-22 15:41                           ` Michael Walle
  2020-09-22 15:55                             ` Tom Rini
  0 siblings, 1 reply; 19+ messages in thread
From: Michael Walle @ 2020-09-22 15:41 UTC (permalink / raw)
  To: u-boot

Hi Tom,

Am 2020-09-22 16:41, schrieb Tom Rini:
> On Tue, Sep 22, 2020 at 03:18:58PM +0200, Michael Walle wrote:
[..]
>> > But all that said, since we have "wdt stop", perhaps you can find a
>> > place to put that in the boot script?  Or just declare that if we get
>> > far enough to run preboot cmd then it's good enough, and update your
>> > call to "wdt expire 1" to be "wdt start && wdt expire 1" ?
>> 
>> "wdt expire 1" will automatically start the watchdog, won't it? 
>> Anyway,
>> it was just an example why I need the CONFIG_WDT.
> 
> Right, OK.  I'm just wondering if we can use the existing "wdt stop"
> functionality to cover what you're aiming for.

See below.

>> Just to get your opinion correct on this topic: you say as soon as the
>> CONFIG_WDT is enabled u-boot will start it and never stop it, although
>> CONFIG_WDT is just "Enable driver model watchdog timer drivers" and 
>> the
>> help text is just about that, too.
> 
> I will agree that the help text and symbols can use further cleaning up
> still.  CONFIG_WDT implies in CONFIG_WATCHDOG which says:
> 
>           This option enables U-Boot watchdog support where U-Boot is 
> using
>           watchdog_reset function to service watchdog device in U-Boot. 
> Enable
>           this option if you want to service enabled watchdog by 
> U-Boot. Disable
>           this option if you want U-Boot to start watchdog but never 
> service it.
> 
> Which is what we've done (to the best of my knowledge) "forever".

Yes CONFIG_WATCHDOG is implied, but if you disable CONFIG_WATCHDOG it 
will
still be started. Just not serviced.

>> IMHO it is wrong to enable the watchdog together with that option. 
>> There
>> should be another one (even defaulting to 'yes') which tells u-boot 
>> whether
>> it should be enabled by default.
>> 
>> config WDT_AUTOSTART
>>    boot "Start the (first) watchdog by default"
>>    default y
>>    help
>>      Upon u-boot startup the first watchdog will be started 
>> automatically.
>>      Be aware, that it will also kept enabled after the bootloader 
>> starts
>>      the operation system!
> 
> Now, given what I said above looking at commit 06985289d452 ("watchdog:
> Implement generic watchdog_reset() version") is where we get the 
> current
> behavior, symbol-wise.  At this point, I'm not quite sure how best to 
> do
> what you're looking for, or if we just have a bug in terms of which
> symbols are used.  It sounds like you just want to stop the watchdog in
> U-Boot (outside of a specific start-and-trigger case) and let the OS
> decide if it's going to enable it.  And if the OS is going to enable it
> and you want the watchdog started before OS boot, preboot could be setf
> in the environment to "wdt start" to get it going again (and you enable
> CONFIG_USE_PREBOOT and set CONFIG_PREBOOT to an empty string, or wdt
> stop?).  Or does that still not cover what you're trying to do?

There are two things I want(ed) to achieve:
  (1) While booting the d-i I noticed my board does watchdog resets. So
      I digged into this and noticed that since the commit in question,
      any watchdog will be started unconditionally, which looked wrong
      to me and is a bit of a suprising behaviour. Esp. when you inherit
      the device trees from linux where the SoC watchdogs are usually
      enabled.
      Also I looked into how you could disable this behavior per-board.
      I didn't find a reliable method that worked for any boot command.
      Therefore, I've started this discussion, to find out if this
      is the intended behavior.

  (2) To be able to boot an operating system with the boards default
      environment, that isn't aware of any watchdog.

For my specific use-case there are the following solutions so far:
  (a) stop the watchdog via "wdt stop" sometime
  (b) disable CONFIG_WDT
  (c) don't start the watchdog at all
  (d) have a runtime switch in the environment to stop it
      before booting the OS.

(a) and (b) is currently possible, (c) and (d) would need a patch.
(b) is out of question for me, because I need the u-boot wdt commands.
(a) sounds like a hack to me, why would you stop it even if I don't
want it to be started in the first place. So I'd prefer (c).

I see that someone might prefer (d) as it gives the user the choice
without having to recompile u-boot.

But apart from my use case, I could think of others and IMHO we
should leave the choice up to the board user (and making it as easy
as possible to configure it). So what do about the following:

choice
   prompt "Watchdog behaviour"
   default WDT_SUPERVISE_OS

config WDT_SUPERVISE_NOTHING
    boot "Supervise nothing"
    help
      No watchdog will be started.

config WDT_SUPERVISE_U_BOOT
    boot "Supervise u-boot"
    help
      Upon u-boot startup the first watchdog will be started 
automatically
      and stopped as soon as an operating system is booted.

config WDT_SUPERVISE_OS
    boot "Supervise u-boot and operating system"
    help
      Upon u-boot startup the first watchdog will be started 
automatically
      and kept running even after booting the operating system.
      Be aware, that the operating system needs to service the watchdog!

endchoice

-michael

^ permalink raw reply	[flat|nested] 19+ messages in thread

* u-boot leaves watchdog enabled by default
  2020-09-22 15:41                           ` Michael Walle
@ 2020-09-22 15:55                             ` Tom Rini
  0 siblings, 0 replies; 19+ messages in thread
From: Tom Rini @ 2020-09-22 15:55 UTC (permalink / raw)
  To: u-boot

On Tue, Sep 22, 2020 at 05:41:47PM +0200, Michael Walle wrote:
> Hi Tom,
> 
> Am 2020-09-22 16:41, schrieb Tom Rini:
> > On Tue, Sep 22, 2020 at 03:18:58PM +0200, Michael Walle wrote:
> [..]
> > > > But all that said, since we have "wdt stop", perhaps you can find a
> > > > place to put that in the boot script?  Or just declare that if we get
> > > > far enough to run preboot cmd then it's good enough, and update your
> > > > call to "wdt expire 1" to be "wdt start && wdt expire 1" ?
> > > 
> > > "wdt expire 1" will automatically start the watchdog, won't it?
> > > Anyway,
> > > it was just an example why I need the CONFIG_WDT.
> > 
> > Right, OK.  I'm just wondering if we can use the existing "wdt stop"
> > functionality to cover what you're aiming for.
> 
> See below.
> 
> > > Just to get your opinion correct on this topic: you say as soon as the
> > > CONFIG_WDT is enabled u-boot will start it and never stop it, although
> > > CONFIG_WDT is just "Enable driver model watchdog timer drivers" and
> > > the
> > > help text is just about that, too.
> > 
> > I will agree that the help text and symbols can use further cleaning up
> > still.  CONFIG_WDT implies in CONFIG_WATCHDOG which says:
> > 
> >           This option enables U-Boot watchdog support where U-Boot is
> > using
> >           watchdog_reset function to service watchdog device in U-Boot.
> > Enable
> >           this option if you want to service enabled watchdog by U-Boot.
> > Disable
> >           this option if you want U-Boot to start watchdog but never
> > service it.
> > 
> > Which is what we've done (to the best of my knowledge) "forever".
> 
> Yes CONFIG_WATCHDOG is implied, but if you disable CONFIG_WATCHDOG it will
> still be started. Just not serviced.
> 
> > > IMHO it is wrong to enable the watchdog together with that option.
> > > There
> > > should be another one (even defaulting to 'yes') which tells u-boot
> > > whether
> > > it should be enabled by default.
> > > 
> > > config WDT_AUTOSTART
> > >    boot "Start the (first) watchdog by default"
> > >    default y
> > >    help
> > >      Upon u-boot startup the first watchdog will be started
> > > automatically.
> > >      Be aware, that it will also kept enabled after the bootloader
> > > starts
> > >      the operation system!
> > 
> > Now, given what I said above looking at commit 06985289d452 ("watchdog:
> > Implement generic watchdog_reset() version") is where we get the current
> > behavior, symbol-wise.  At this point, I'm not quite sure how best to do
> > what you're looking for, or if we just have a bug in terms of which
> > symbols are used.  It sounds like you just want to stop the watchdog in
> > U-Boot (outside of a specific start-and-trigger case) and let the OS
> > decide if it's going to enable it.  And if the OS is going to enable it
> > and you want the watchdog started before OS boot, preboot could be setf
> > in the environment to "wdt start" to get it going again (and you enable
> > CONFIG_USE_PREBOOT and set CONFIG_PREBOOT to an empty string, or wdt
> > stop?).  Or does that still not cover what you're trying to do?
> 
> There are two things I want(ed) to achieve:
>  (1) While booting the d-i I noticed my board does watchdog resets. So
>      I digged into this and noticed that since the commit in question,
>      any watchdog will be started unconditionally, which looked wrong
>      to me and is a bit of a suprising behaviour. Esp. when you inherit
>      the device trees from linux where the SoC watchdogs are usually
>      enabled.
>      Also I looked into how you could disable this behavior per-board.
>      I didn't find a reliable method that worked for any boot command.
>      Therefore, I've started this discussion, to find out if this
>      is the intended behavior.
> 
>  (2) To be able to boot an operating system with the boards default
>      environment, that isn't aware of any watchdog.
> 
> For my specific use-case there are the following solutions so far:
>  (a) stop the watchdog via "wdt stop" sometime
>  (b) disable CONFIG_WDT
>  (c) don't start the watchdog at all
>  (d) have a runtime switch in the environment to stop it
>      before booting the OS.
> 
> (a) and (b) is currently possible, (c) and (d) would need a patch.
> (b) is out of question for me, because I need the u-boot wdt commands.
> (a) sounds like a hack to me, why would you stop it even if I don't
> want it to be started in the first place. So I'd prefer (c).
> 
> I see that someone might prefer (d) as it gives the user the choice
> without having to recompile u-boot.
> 
> But apart from my use case, I could think of others and IMHO we
> should leave the choice up to the board user (and making it as easy
> as possible to configure it). So what do about the following:
> 
> choice
>   prompt "Watchdog behaviour"
>   default WDT_SUPERVISE_OS
> 
> config WDT_SUPERVISE_NOTHING
>    boot "Supervise nothing"
>    help
>      No watchdog will be started.
> 
> config WDT_SUPERVISE_U_BOOT
>    boot "Supervise u-boot"
>    help
>      Upon u-boot startup the first watchdog will be started automatically
>      and stopped as soon as an operating system is booted.
> 
> config WDT_SUPERVISE_OS
>    boot "Supervise u-boot and operating system"
>    help
>      Upon u-boot startup the first watchdog will be started automatically
>      and kept running even after booting the operating system.
>      Be aware, that the operating system needs to service the watchdog!
> 
> endchoice

Can you code this up as a patch please?  I think that's likely the best
path forward to covering all cases.  Thanks.

-- 
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/20200922/3fccda2a/attachment.sig>

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2020-09-22 15:55 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-15  7:26 u-boot leaves watchdog enabled by default Michael Walle
2020-09-15  7:44 ` Rayagonda Kokatanur
2020-09-15  7:54   ` Michael Walle
2020-09-15 10:44     ` Chris Packham
2020-09-21  9:01       ` Stefan Roese
2020-09-21 17:30         ` Tom Rini
2020-09-21 18:29           ` Heinrich Schuchardt
2020-09-21 18:50             ` Tom Rini
2020-09-21 20:56               ` Michael Walle
2020-09-21 21:10                 ` Chris Packham
2020-09-22  0:52                 ` Heinrich Schuchardt
2020-09-22  1:18                 ` Tom Rini
2020-09-22  6:59                   ` Michael Walle
2020-09-22 12:36                     ` Tom Rini
2020-09-22 13:18                       ` Michael Walle
2020-09-22 14:41                         ` Tom Rini
2020-09-22 15:41                           ` Michael Walle
2020-09-22 15:55                             ` Tom Rini
2020-09-21 20:41         ` Michael Walle

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.