Re: [edk2-devel] [PATCH v1 1/1] MdeModulePkg/Core: Fix the potential hang of calling SetTimer.

["Michael D Kinney" <michael.d.kinney@intel.com>]

Hi,

If a module is has a dependency on the Timer Arch Protocol, that module
should list the Timer Arch Protocol as only of its dependencies in the
dependency expression.

I agree that we should avoid a hang condition, but it is not clear if 
new return statuses need to be added for this case where a module may not
have a correct dependency expression.

An ASSERT() or DEBUG() message may be a better way to inform the 
developer that they may have an incorrect dependency expression.

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ying-Tsun Huang via groups.io
> Sent: Tuesday, September 20, 2022 7:10 PM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>
> Subject: [edk2-devel] [PATCH v1 1/1] MdeModulePkg/Core: Fix the potential hang of calling SetTimer.
> 
> When calling SetTimer with Type is not TimerCancel and TriggerTime is 0,
> gTimer is used to get the timer period. However, gTimer is NULL before
> EFI_TIMER_ARCH_PROTOCOL is installed. Adding the check of gTimer and
> return EFI_NOT_READY to avoid the hang.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Signed-off-by: Ying-Tsun Huang <ying-tsun.huang@amd.com>
> ---
>  MdeModulePkg/Core/Dxe/DxeMain.h     |  2 ++
>  MdeModulePkg/Core/Dxe/Event/Timer.c | 14 ++++++++++----
>  2 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
> index 815a6b4bd844..acf0b244aeda 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain.h
> +++ b/MdeModulePkg/Core/Dxe/DxeMain.h
> @@ -1538,6 +1538,8 @@ CoreCreateEventInternal (
>    @retval EFI_SUCCESS            The event has been set to be signaled at the
>                                   requested time
>    @retval EFI_INVALID_PARAMETER  Event or Type is not valid
> +  @retval EFI_NOT_READY          Type is not TimerCancel, TriggerTime is 0, but
> +                                 EFI_TIMER_ARCH_PROTOCOL is not installed yet
> 
>  **/
>  EFI_STATUS
> diff --git a/MdeModulePkg/Core/Dxe/Event/Timer.c b/MdeModulePkg/Core/Dxe/Event/Timer.c
> index 29e507c67c50..1621814ef3c5 100644
> --- a/MdeModulePkg/Core/Dxe/Event/Timer.c
> +++ b/MdeModulePkg/Core/Dxe/Event/Timer.c
> @@ -229,6 +229,8 @@ CoreTimerTick (
>    @retval EFI_SUCCESS            The event has been set to be signaled at the
>                                   requested time
>    @retval EFI_INVALID_PARAMETER  Event or Type is not valid
> +  @retval EFI_NOT_READY          Type is not TimerCancel, TriggerTime is 0, but
> +                                 EFI_TIMER_ARCH_PROTOCOL is not installed yet
> 
>  **/
>  EFI_STATUS
> @@ -255,6 +257,14 @@ CoreSetTimer (
>      return EFI_INVALID_PARAMETER;
>    }
> 
> +  if ((Type != TimerCancel) && (TriggerTime == 0)) {
> +    if (gTimer) {
> +      gTimer->GetTimerPeriod (gTimer, &TriggerTime);
> +    } else {
> +      return EFI_NOT_READY;
> +    }
> +  }
> +
>    CoreAcquireLock (&mEfiTimerLock);
> 
>    //
> @@ -270,10 +280,6 @@ CoreSetTimer (
> 
>    if (Type != TimerCancel) {
>      if (Type == TimerPeriodic) {
> -      if (TriggerTime == 0) {
> -        gTimer->GetTimerPeriod (gTimer, &TriggerTime);
> -      }
> -
>        Event->Timer.Period = TriggerTime;
>      }
> 
> --
> 2.25.1
> 
> 
> 
> 
>