1. Patchew
  2. EDK2
  3. [edk2-devel] [PATCH v3 0/3] ArmPkg: GenericWatchdogDxe fixes and improvements
  4. View patch

[edk2-devel] [PATCH v3 3/3] ArmPkg: Disable watchdog interaction after exiting boot services

Rebecca Cran via groups.io posted 3 patches 1 year, 11 months ago
There is a newer version of this series
[edk2-devel] [PATCH v3 3/3] ArmPkg: Disable watchdog interaction after exiting boot services
Posted by Rebecca Cran via groups.io 1 year, 11 months ago 
Update GenericWatchdogDxe to disable watchdog interaction after exiting
boot services. Also, move the mEfiExitBootServicesEvent event to the top
of the file with the other static variables.

Signed-off-by: Rebecca Cran <rebecca@os.amperecomputing.com>
---
 ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
index 78cee62a19d6..ddf131660f9d 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
@@ -33,10 +33,14 @@
    It is therefore stored here. 0 means the timer is not running. */
 STATIC UINT64  mTimerPeriod = 0;
 
+/* disables watchdog interaction after Exit Boot Services */
+STATIC BOOLEAN  mExitedBootServices = FALSE;
+
 #define MAX_UINT48  0xFFFFFFFFFFFFULL
 
 STATIC EFI_HARDWARE_INTERRUPT2_PROTOCOL  *mInterruptProtocol;
 STATIC EFI_WATCHDOG_TIMER_NOTIFY         mWatchdogNotify;
+STATIC EFI_EVENT                         mEfiExitBootServicesEvent;
 
 STATIC
 VOID
@@ -200,7 +204,13 @@ WatchdogSetTimerPeriod (
   UINT64  TimerFrequencyHz;
   UINT64  NumTimerTicks;
 
-  // if TimerPeriod is 0, this is a request to stop the watchdog.
+  // If we've exited Boot Services but TimerPeriod isn't zero, this
+  // indicates that the caller is doing something wrong.
+  if (mExitedBootServices && (TimerPeriod != 0)) {
+    return EFI_DEVICE_ERROR;
+  }
+
+  // If TimerPeriod is 0 this is a request to stop the watchdog.
   if (TimerPeriod == 0) {
     mTimerPeriod = 0;
     WatchdogDisable ();
@@ -304,8 +314,6 @@ STATIC EFI_WATCHDOG_TIMER_ARCH_PROTOCOL  mWatchdogTimer = {
   WatchdogGetTimerPeriod
 };
 
-STATIC EFI_EVENT  mEfiExitBootServicesEvent;
-
 EFI_STATUS
 EFIAPI
 GenericWatchdogEntry (
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113218): https://edk2.groups.io/g/devel/message/113218
Mute This Topic: https://groups.io/mt/103538118/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v3 3/3] ArmPkg: Disable watchdog interaction after exiting boot services
Posted by Sami Mujawar 1 year, 11 months ago 
Hi Rebecca,

Thank you for this patch. 
Please see my feedback inline marked [SAMI].

Regards,

Sami Mujawar

On 05/01/2024, 05:15, "Rebecca Cran" <rebecca@os.amperecomputing.com <mailto:rebecca@os.amperecomputing.com>> wrote:


Update GenericWatchdogDxe to disable watchdog interaction after exiting
boot services. Also, move the mEfiExitBootServicesEvent event to the top
of the file with the other static variables.


Signed-off-by: Rebecca Cran <rebecca@os.amperecomputing.com <mailto:rebecca@os.amperecomputing.com>>
---
ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)


diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
index 78cee62a19d6..ddf131660f9d 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
@@ -33,10 +33,14 @@
It is therefore stored here. 0 means the timer is not running. */
STATIC UINT64 mTimerPeriod = 0;


+/* disables watchdog interaction after Exit Boot Services */
+STATIC BOOLEAN mExitedBootServices = FALSE;
+
#define MAX_UINT48 0xFFFFFFFFFFFFULL


STATIC EFI_HARDWARE_INTERRUPT2_PROTOCOL *mInterruptProtocol;
STATIC EFI_WATCHDOG_TIMER_NOTIFY mWatchdogNotify;
+STATIC EFI_EVENT mEfiExitBootServicesEvent;


STATIC
VOID
@@ -200,7 +204,13 @@ WatchdogSetTimerPeriod (
UINT64 TimerFrequencyHz;
UINT64 NumTimerTicks;


- // if TimerPeriod is 0, this is a request to stop the watchdog.
+ // If we've exited Boot Services but TimerPeriod isn't zero, this
+ // indicates that the caller is doing something wrong.
+ if (mExitedBootServices && (TimerPeriod != 0)) {
[SAMI] Thanks for updating the code to return the error code. 
However, I see you are not stopping the watchdog timer. Is this because you expect the watchdog period to expire and reset the system? 
Also, did you see an issue that motivated this patch, or this was just a case of hardening the code?
Can you provide more information, please?
[/SAMI]
+ return EFI_DEVICE_ERROR;
+ }
+
+ // If TimerPeriod is 0 this is a request to stop the watchdog.
if (TimerPeriod == 0) {
mTimerPeriod = 0;
WatchdogDisable ();
@@ -304,8 +314,6 @@ STATIC EFI_WATCHDOG_TIMER_ARCH_PROTOCOL mWatchdogTimer = {
WatchdogGetTimerPeriod
};


-STATIC EFI_EVENT mEfiExitBootServicesEvent;
-
EFI_STATUS
EFIAPI
GenericWatchdogEntry (
-- 
2.34.1







-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113274): https://edk2.groups.io/g/devel/message/113274
Mute This Topic: https://groups.io/mt/103538118/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v3 3/3] ArmPkg: Disable watchdog interaction after exiting boot services
Posted by Rebecca Cran via groups.io 1 year, 11 months ago 
On 1/5/2024 4:12 AM, Sami Mujawar wrote:

> - // if TimerPeriod is 0, this is a request to stop the watchdog.
> + // If we've exited Boot Services but TimerPeriod isn't zero, this
> + // indicates that the caller is doing something wrong.
> + if (mExitedBootServices && (TimerPeriod != 0)) {
> [SAMI] Thanks for updating the code to return the error code.
> However, I see you are not stopping the watchdog timer. Is this because you expect the watchdog period to expire and reset the system?

I removed the code to stop the watchdog timer because it's an error 
condition. However, I've updated it so it does also get stopped in this 
case too.

> Also, did you see an issue that motivated this patch, or this was just a case of hardening the code?
> Can you provide more information, please?
> [/SAMI]

This was issues found and improvements made by various people in the 
last couple of years that we're now upstreaming to contribute 
improvements and reduce our diffs against upstream.

-- 
Rebecca Cran


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114018): https://edk2.groups.io/g/devel/message/114018
Mute This Topic: https://groups.io/mt/103538118/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.