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

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

Rebecca Cran via groups.io posted 3 patches 2 years, 2 months ago
There is a newer version of this series
[edk2-devel] [PATCH 3/3] ArmPkg: Disable watchdog interaction after exiting boot services
Posted by Rebecca Cran via groups.io 2 years, 2 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, 9 insertions(+), 5 deletions(-)

diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
index 8f02f38c64e3..912106eb6ad2 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
@@ -36,10 +36,14 @@ STATIC UINTN  mTimerFrequencyHz = 0;
    It is therefore stored here. 0 means the timer is not running. */
 STATIC UINT64  mNumTimerTicks = 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
@@ -91,7 +95,8 @@ WatchdogExitBootServicesEvent (
   )
 {
   WatchdogDisable ();
-  mNumTimerTicks = 0;
+  mNumTimerTicks      = 0;
+  mExitedBootServices = TRUE;
 }
 
 /* This function is called when the watchdog's first signal (WS0) goes high.
@@ -202,8 +207,9 @@ WatchdogSetTimerPeriod (
 {
   UINTN  SystemCount;
 
-  // if TimerPeriod is 0, this is a request to stop the watchdog.
-  if (TimerPeriod == 0) {
+  // if TimerPeriod is 0 or we've exited boot services,
+  // this is a request to stop the watchdog.
+  if (TimerPeriod == 0 || mExitedBootServices) {
     mNumTimerTicks = 0;
     WatchdogDisable ();
     return EFI_SUCCESS;
@@ -303,8 +309,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 (#113107): https://edk2.groups.io/g/devel/message/113107
Mute This Topic: https://groups.io/mt/103510105/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 3/3] ArmPkg: Disable watchdog interaction after exiting boot services
Posted by Sami Mujawar 2 years, 2 months ago 
Hi Rebecca,

Thank you for this patch.

I have some minor suggestions marked inline as [SAMI].

Regards,

Sami Mujawar	

On 03/01/2024, 20:44, "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, 9 insertions(+), 5 deletions(-)


diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
index 8f02f38c64e3..912106eb6ad2 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
@@ -36,10 +36,14 @@ STATIC UINTN mTimerFrequencyHz = 0;
It is therefore stored here. 0 means the timer is not running. */
STATIC UINT64 mNumTimerTicks = 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
@@ -91,7 +95,8 @@ WatchdogExitBootServicesEvent (
)
{
WatchdogDisable ();
- mNumTimerTicks = 0;
+ mNumTimerTicks = 0;
+ mExitedBootServices = TRUE;
}


/* This function is called when the watchdog's first signal (WS0) goes high.
@@ -202,8 +207,9 @@ WatchdogSetTimerPeriod (
{
UINTN SystemCount;


- // if TimerPeriod is 0, this is a request to stop the watchdog.
- if (TimerPeriod == 0) {
+ // if TimerPeriod is 0 or we've exited boot services,
+ // this is a request to stop the watchdog.
+ if (TimerPeriod == 0 || mExitedBootServices) {
[SAMI] I think we need extra brackets here. Also, should EFI_DEVICE_ERROR be returned if ExitBootServices is signalled but TimerPeriod != 0?
 i.e. we stop the watchdog but return an error to the caller so that it knows to fix the broken code.
[/SAMI]
mNumTimerTicks = 0;
WatchdogDisable ();
return EFI_SUCCESS;
@@ -303,8 +309,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 (#113159): https://edk2.groups.io/g/devel/message/113159
Mute This Topic: https://groups.io/mt/103510105/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 3/3] ArmPkg: Disable watchdog interaction after exiting boot services
Posted by Rebecca Cran via groups.io 2 years, 2 months ago 
Thanks, I've incorporated the changes into the v2 patch series.

-- 
Rebecca

On 1/4/2024 3:01 AM, Sami Mujawar wrote:
> Hi Rebecca,
> 
> Thank you for this patch.
> 
> I have some minor suggestions marked inline as [SAMI].
> 
> Regards,
> 
> Sami Mujawar	
> 
> On 03/01/2024, 20:44, "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, 9 insertions(+), 5 deletions(-)
> 
> 
> diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> index 8f02f38c64e3..912106eb6ad2 100644
> --- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> +++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> @@ -36,10 +36,14 @@ STATIC UINTN mTimerFrequencyHz = 0;
> It is therefore stored here. 0 means the timer is not running. */
> STATIC UINT64 mNumTimerTicks = 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
> @@ -91,7 +95,8 @@ WatchdogExitBootServicesEvent (
> )
> {
> WatchdogDisable ();
> - mNumTimerTicks = 0;
> + mNumTimerTicks = 0;
> + mExitedBootServices = TRUE;
> }
> 
> 
> /* This function is called when the watchdog's first signal (WS0) goes high.
> @@ -202,8 +207,9 @@ WatchdogSetTimerPeriod (
> {
> UINTN SystemCount;
> 
> 
> - // if TimerPeriod is 0, this is a request to stop the watchdog.
> - if (TimerPeriod == 0) {
> + // if TimerPeriod is 0 or we've exited boot services,
> + // this is a request to stop the watchdog.
> + if (TimerPeriod == 0 || mExitedBootServices) {
> [SAMI] I think we need extra brackets here. Also, should EFI_DEVICE_ERROR be returned if ExitBootServices is signalled but TimerPeriod != 0?
>   i.e. we stop the watchdog but return an error to the caller so that it knows to fix the broken code.
> [/SAMI]
> mNumTimerTicks = 0;
> WatchdogDisable ();
> return EFI_SUCCESS;
> @@ -303,8 +309,6 @@ STATIC EFI_WATCHDOG_TIMER_ARCH_PROTOCOL mWatchdogTimer = {
> WatchdogGetTimerPeriod
> };
> 
> 
> -STATIC EFI_EVENT mEfiExitBootServicesEvent;
> -
> EFI_STATUS
> EFIAPI
> GenericWatchdogEntry (



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

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.