From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4864:20::143; helo=mail-it1-x143.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it1-x143.google.com (mail-it1-x143.google.com [IPv6:2607:f8b0:4864:20::143]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id C61E92119EF33 for ; Tue, 18 Dec 2018 07:19:32 -0800 (PST) Received: by mail-it1-x143.google.com with SMTP id w18so4345968ite.1 for ; Tue, 18 Dec 2018 07:19:32 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=vf9XaLTm6Lr2BcVAzfWZ6SsZQPJAlZBZRYF9W0gf5GU=; b=E6H0nxbPyQbpEqXzcGS0DTdOLSwRERu+5U25vSlNRMGNezzhwczV+oOPtfB7g2ERmM uLCjr2hQQz3ailAQsssgpINAD+whbbPS24pq7YfGvztSPsw3xu2/2qyKc3uX752m5zw3 rjnlmFc9bQTLYGwKQ1WWy8bteZQZgSBgzj08I= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=vf9XaLTm6Lr2BcVAzfWZ6SsZQPJAlZBZRYF9W0gf5GU=; b=cMBpeEtbC4OerglB+Ipt/V3q+LYb6SUqTJpgN1A/XD6TtnTEnP60mrE44llbM18qgi 7KrO3KsX/H2/CcvuF/DB9fdlBhlvrEZ89UuGrzRIEWAe9M99iQ0mPkIViFnspA9A5VWV auDtM6xsjhkQuzLWsowBEEQGRxFJ8UDAB4phj5FYJarxmUzOCpK5EwCDge/X7aOjjeSJ eXcF8BD04MoSt1YpKVkRitlKGgj9kQIF6YMQD2WVwymYQi+tsZ7vVMR+1xYvDM/a12LR kl5QU6ddzcSvgqQOdEwBJdCmtbgtSvGK8+AVqPRS2TYt8xD9Xm+XpKoYj7Usoz2S59KS TTVA== X-Gm-Message-State: AA+aEWZdxxjWuPbWH86+UxLSSZgHM9g0Ff4B7cgJ8MbaeC4ZbQuAZ/U8 ykqEPyqWDHNIac/I25vA0zeOYRgOI/12Be8q9ofLHg== X-Google-Smtp-Source: AFSGD/UuMJvnEKP8Nglfdeu7nR4QTYKsg2f5T6QSFqEn1R1VrRH46pUwMifrFrr0Vir7JKW+HjscdrlFYkOhb6TuVFA= X-Received: by 2002:a02:183:: with SMTP id 3mr16288094jak.130.1545146371806; Tue, 18 Dec 2018 07:19:31 -0800 (PST) MIME-Version: 1.0 References: <20181218131015.20062-1-ard.biesheuvel@linaro.org> <20181218131015.20062-4-ard.biesheuvel@linaro.org> <20181218134339.jorvyyciarq3lb3d@bivouac.eciton.net> In-Reply-To: <20181218134339.jorvyyciarq3lb3d@bivouac.eciton.net> From: Ard Biesheuvel Date: Tue, 18 Dec 2018 16:19:20 +0100 Message-ID: To: Leif Lindholm Cc: "edk2-devel@lists.01.org" , Sami Mujawar , Thomas Panakamattam Abraham , Meenakshi Aggarwal , Udit Kumar , Matteo Carlini , Nariman Poushin Subject: Re: [PATCH 3/4] ArmPkg/GenericWatchdogDxe: clean up the code X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 18 Dec 2018 15:19:33 -0000 Content-Type: text/plain; charset="UTF-8" On Tue, 18 Dec 2018 at 14:43, Leif Lindholm wrote: > > On Tue, Dec 18, 2018 at 02:10:13PM +0100, Ard Biesheuvel wrote: > > Clean up the code, by adding missing STATIC modifiers, drop > > redundant casts, and get rid of the 'success handling' anti > > pattern in the entry point code. > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Ard Biesheuvel > > --- > > ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 111 +++++++++++--------- > > ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf | 11 +- > > 2 files changed, 64 insertions(+), 58 deletions(-) > > > > diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c > > index 8ccf15366dfa..717a180a64ec 100644 > > --- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c > > +++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c > > @@ -34,15 +34,16 @@ > > #define TIME_UNITS_PER_SECOND 10000000 > > > > // Tick frequency of the generic timer basis of the generic watchdog. > > -UINTN mTimerFrequencyHz = 0; > > +STATIC UINTN mTimerFrequencyHz = 0; > > > > /* In cases where the compare register was set manually, information about > > how long the watchdog was asked to wait cannot be retrieved from hardware. > > It is therefore stored here. 0 means the timer is not running. */ > > -UINT64 mNumTimerTicks = 0; > > +STATIC UINT64 mNumTimerTicks = 0; > > > > -EFI_HARDWARE_INTERRUPT2_PROTOCOL *mInterruptProtocol; > > +STATIC EFI_HARDWARE_INTERRUPT2_PROTOCOL *mInterruptProtocol; > > > > +STATIC > > VOID > > WatchdogWriteOffsetRegister ( > > UINT32 Value > > @@ -51,6 +52,7 @@ WatchdogWriteOffsetRegister ( > > MmioWrite32 (GENERIC_WDOG_OFFSET_REG, Value); > > } > > > > +STATIC > > VOID > > WatchdogWriteCompareRegister ( > > UINT64 Value > > @@ -60,6 +62,7 @@ WatchdogWriteCompareRegister ( > > MmioWrite32 (GENERIC_WDOG_COMPARE_VALUE_REG_HIGH, (Value >> 32) & MAX_UINT32); > > } > > > > +STATIC > > VOID > > WatchdogEnable ( > > VOID > > @@ -68,6 +71,7 @@ WatchdogEnable ( > > MmioWrite32 (GENERIC_WDOG_CONTROL_STATUS_REG, GENERIC_WDOG_ENABLED); > > } > > > > +STATIC > > VOID > > WatchdogDisable ( > > VOID > > @@ -79,6 +83,7 @@ WatchdogDisable ( > > /** On exiting boot services we must make sure the Watchdog Timer > > is stopped. > > **/ > > +STATIC > > VOID > > EFIAPI > > WatchdogExitBootServicesEvent ( > > @@ -93,6 +98,7 @@ WatchdogExitBootServicesEvent ( > > /* This function is called when the watchdog's first signal (WS0) goes high. > > It uses the ResetSystem Runtime Service to reset the board. > > */ > > +STATIC > > VOID > > EFIAPI > > WatchdogInterruptHandler ( > > @@ -141,10 +147,11 @@ WatchdogInterruptHandler ( > > @retval EFI_UNSUPPORTED The code does not support NotifyFunction. > > > > **/ > > +STATIC > > EFI_STATUS > > EFIAPI > > WatchdogRegisterHandler ( > > - IN CONST EFI_WATCHDOG_TIMER_ARCH_PROTOCOL *This, > > + IN EFI_WATCHDOG_TIMER_ARCH_PROTOCOL *This, > > IN EFI_WATCHDOG_TIMER_NOTIFY NotifyFunction > > ) > > { > > @@ -167,10 +174,11 @@ WatchdogRegisterHandler ( > > in TimerPeriod 100ns units. > > > > **/ > > +STATIC > > EFI_STATUS > > EFIAPI > > WatchdogSetTimerPeriod ( > > - IN CONST EFI_WATCHDOG_TIMER_ARCH_PROTOCOL *This, > > + IN EFI_WATCHDOG_TIMER_ARCH_PROTOCOL *This, > > IN UINT64 TimerPeriod // In 100ns units > > ) > > { > > @@ -178,8 +186,8 @@ WatchdogSetTimerPeriod ( > > > > // if TimerPeriod is 0, this is a request to stop the watchdog. > > if (TimerPeriod == 0) { > > - mNumTimerTicks = 0; > > - WatchdogDisable (); > > + //mNumTimerTicks = 0; > > + //WatchdogDisable (); > > Should these be deletions? > Nope. I commented them out to test whether the watchdog actually fires, since most shell tools (and GRUB) disable it while running in interactive mode. So this whole hunk should be dropped. > > return EFI_SUCCESS; > > } > > > > @@ -222,10 +230,11 @@ WatchdogSetTimerPeriod ( > > @retval EFI_INVALID_PARAMETER TimerPeriod is NULL. > > > > **/ > > +STATIC > > EFI_STATUS > > EFIAPI > > WatchdogGetTimerPeriod ( > > - IN CONST EFI_WATCHDOG_TIMER_ARCH_PROTOCOL *This, > > + IN EFI_WATCHDOG_TIMER_ARCH_PROTOCOL *This, > > OUT UINT64 *TimerPeriod > > ) > > { > > @@ -270,13 +279,13 @@ WatchdogGetTimerPeriod ( > > Retrieves the period of the timer interrupt in 100ns units. > > > > **/ > > -EFI_WATCHDOG_TIMER_ARCH_PROTOCOL gWatchdogTimer = { > > - (EFI_WATCHDOG_TIMER_REGISTER_HANDLER)WatchdogRegisterHandler, > > - (EFI_WATCHDOG_TIMER_SET_TIMER_PERIOD)WatchdogSetTimerPeriod, > > - (EFI_WATCHDOG_TIMER_GET_TIMER_PERIOD)WatchdogGetTimerPeriod > > +STATIC EFI_WATCHDOG_TIMER_ARCH_PROTOCOL mWatchdogTimer = { > > + WatchdogRegisterHandler, > > + WatchdogSetTimerPeriod, > > + WatchdogGetTimerPeriod > > }; > > > > -EFI_EVENT EfiExitBootServicesEvent = (EFI_EVENT)NULL; > > +STATIC EFI_EVENT mEfiExitBootServicesEvent; > > > > EFI_STATUS > > EFIAPI > > @@ -288,6 +297,10 @@ GenericWatchdogEntry ( > > EFI_STATUS Status; > > EFI_HANDLE Handle; > > > > + Status = gBS->LocateProtocol (&gHardwareInterrupt2ProtocolGuid, NULL, > > + (VOID **)&mInterruptProtocol); > > + ASSERT_EFI_ERROR (Status); > > + > > /* Make sure the Watchdog Timer Architectural Protocol has not been installed > > in the system yet. > > This will avoid conflicts with the universal watchdog */ > > @@ -296,51 +309,45 @@ GenericWatchdogEntry ( > > mTimerFrequencyHz = ArmGenericTimerGetTimerFreq (); > > ASSERT (mTimerFrequencyHz != 0); > > > > - // Register for an ExitBootServicesEvent > > - Status = gBS->CreateEvent ( > > - EVT_SIGNAL_EXIT_BOOT_SERVICES, > > - TPL_NOTIFY, > > - WatchdogExitBootServicesEvent, > > - NULL, > > - &EfiExitBootServicesEvent > > - ); > > - if (!EFI_ERROR (Status)) { > > - // Install interrupt handler > > - Status = gBS->LocateProtocol ( > > - &gHardwareInterrupt2ProtocolGuid, > > - NULL, > > - (VOID **)&mInterruptProtocol > > - ); > > - if (!EFI_ERROR (Status)) { > > - Status = mInterruptProtocol->RegisterInterruptSource ( > > - mInterruptProtocol, > > - FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum), > > - WatchdogInterruptHandler > > - ); > > - if (!EFI_ERROR (Status)) { > > - Status = mInterruptProtocol->SetTriggerType ( > > - mInterruptProtocol, > > - FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum), > > - EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING > > - ); > > - if (!EFI_ERROR (Status)) { > > - // Install the Timer Architectural Protocol onto a new handle > > - Handle = NULL; > > - Status = gBS->InstallMultipleProtocolInterfaces ( > > - &Handle, > > - &gEfiWatchdogTimerArchProtocolGuid, > > - &gWatchdogTimer, > > - NULL > > - ); > > - } > > - } > > - } > > + // Install interrupt handler > > + Status = mInterruptProtocol->RegisterInterruptSource (mInterruptProtocol, > > + FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum), > > + WatchdogInterruptHandler); > > + if (EFI_ERROR (Status)) { > > + return Status; > > + } > > + > > + Status = mInterruptProtocol->SetTriggerType (mInterruptProtocol, > > + FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum), > > + EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING); > > + if (EFI_ERROR (Status)) { > > + goto UnregisterHandler; > > } > > > > + // Install the Timer Architectural Protocol onto a new handle > > + Handle = NULL; > > + Status = gBS->InstallMultipleProtocolInterfaces (&Handle, > > + &gEfiWatchdogTimerArchProtocolGuid, &mWatchdogTimer, > > + NULL); > > + if (EFI_ERROR (Status)) { > > + goto UnregisterHandler; > > + } > > + > > + // Register for an ExitBootServicesEvent > > + Status = gBS->CreateEvent (EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_NOTIFY, > > + WatchdogExitBootServicesEvent, NULL, > > + &mEfiExitBootServicesEvent); > > ASSERT_EFI_ERROR (Status); > > > > mNumTimerTicks = 0; > > WatchdogDisable (); > > > > + return EFI_SUCCESS; > > + > > +UnregisterHandler: > > + // Unregister the handler > > + mInterruptProtocol->RegisterInterruptSource (mInterruptProtocol, > > + FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum), > > + NULL); > > return Status; > > } > > diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf > > index ba0403d7fdc3..7992f3ac78bb 100644 > > --- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf > > +++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf > > @@ -16,17 +16,16 @@ > > FILE_GUID = 0619f5c2-4858-4caa-a86a-73a21a18df6b > > MODULE_TYPE = DXE_DRIVER > > VERSION_STRING = 1.0 > > - > > ENTRY_POINT = GenericWatchdogEntry > > > > [Sources.common] > > GenericWatchdogDxe.c > > > > [Packages] > > - MdePkg/MdePkg.dec > > - EmbeddedPkg/EmbeddedPkg.dec > > - ArmPkg/ArmPkg.dec > > ArmPlatformPkg/ArmPlatformPkg.dec > > + ArmPkg/ArmPkg.dec > > Same as before. > > If you fix that, and the above deletions rather than comment-outs: > Reviewed-by: Leif Lindholm > > Nice work! > > > + EmbeddedPkg/EmbeddedPkg.dec > > + MdePkg/MdePkg.dec > > > > [LibraryClasses] > > ArmGenericTimerCounterLib > > @@ -46,8 +45,8 @@ > > gArmTokenSpaceGuid.PcdGenericWatchdogEl2IntrNum > > > > [Protocols] > > - gEfiWatchdogTimerArchProtocolGuid > > - gHardwareInterrupt2ProtocolGuid > > + gEfiWatchdogTimerArchProtocolGuid ## ALWAYS_PRODUCES > > + gHardwareInterrupt2ProtocolGuid ## ALWAYS_CONSUMES > > > > [Depex] > > gHardwareInterrupt2ProtocolGuid > > -- > > 2.17.1 > >