Re: [PATCH 4/4] ArmPkg/GenericWatchdogDxe: implement RegisterHandler() method

[Leif Lindholm <leif.lindholm@linaro.org>]

On Tue, Dec 18, 2018 at 02:10:14PM +0100, Ard Biesheuvel wrote:
> Even though UEFI does not appear to use it, let's implement the
> complete PI watchdog protocol, including handler registration,
> which will be invoked instead of the ResetSystem() runtime service
> when the watchdog timer expires.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
Thanks!

> ---
>  ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 34 ++++++++++++++------
>  1 file changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> index 717a180a64ec..21118a3c88d1 100644
> --- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> +++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> @@ -42,6 +42,7 @@ STATIC UINTN mTimerFrequencyHz = 0;
>  STATIC UINT64 mNumTimerTicks = 0;
>  
>  STATIC EFI_HARDWARE_INTERRUPT2_PROTOCOL *mInterruptProtocol;
> +STATIC EFI_WATCHDOG_TIMER_NOTIFY        mWatchdogNotify;
>  
>  STATIC
>  VOID
> @@ -107,17 +108,25 @@ WatchdogInterruptHandler (
>    )
>  {
>    STATIC CONST CHAR16 ResetString[]= L"The generic watchdog timer ran out.";
> +  UINT64              TimerPeriod;
>  
>    WatchdogDisable ();
>  
>    mInterruptProtocol->EndOfInterrupt (mInterruptProtocol, Source);
>  
> -  gRT->ResetSystem (
> -         EfiResetCold,
> -         EFI_TIMEOUT,
> -         StrSize (ResetString),
> -         (VOID *) &ResetString
> -         );
> +  //
> +  // The notify function should be called with the elapsed number of ticks
> +  // since the watchdog was armed, which should exceed the timer period.
> +  // We don't actually know the elapsed number of ticks, so let's return
> +  // the timer period plus 1.
> +  //
> +  if (mWatchdogNotify != NULL) {
> +    TimerPeriod = ((TIME_UNITS_PER_SECOND / mTimerFrequencyHz) * mNumTimerTicks);
> +    mWatchdogNotify (TimerPeriod + 1);
> +  } else {
> +    gRT->ResetSystem (EfiResetCold, EFI_TIMEOUT, StrSize (ResetString),
> +           (CHAR16 *)ResetString);
> +  }
>  
>    // If we got here then the reset didn't work
>    ASSERT (FALSE);
> @@ -155,9 +164,16 @@ WatchdogRegisterHandler (
>    IN EFI_WATCHDOG_TIMER_NOTIFY                NotifyFunction
>    )
>  {
> -  // ERROR: This function is not supported.
> -  // The watchdog will reset the board
> -  return EFI_UNSUPPORTED;
> +  if (mWatchdogNotify == NULL && NotifyFunction == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (mWatchdogNotify != NULL && NotifyFunction != NULL) {
> +    return EFI_ALREADY_STARTED;
> +  }
> +
> +  mWatchdogNotify = NotifyFunction;
> +  return EFI_SUCCESS;
>  }
>  
>  /**
> -- 
> 2.17.1
>