From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:4864:20::344; helo=mail-wm1-x344.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm1-x344.google.com (mail-wm1-x344.google.com [IPv6:2a00:1450:4864:20::344]) (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 B98B82119EF4C for ; Tue, 18 Dec 2018 05:43:43 -0800 (PST) Received: by mail-wm1-x344.google.com with SMTP id g67so2860001wmd.2 for ; Tue, 18 Dec 2018 05:43:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=+zXbyfZIoZA5DzMXbQST+NLxtD5dliYbGXVNBLnaccY=; b=NvilK95HoiB9lBt6GHCFJL+w1p8lLTOX0MgBI/B0jUVh6uCnszRD7ABWGBoCX8wwMh TBRMIny/bPPrAPGCfzXIX8EDcAxW7a83XpXGG+e2AWxP63GYLqHoa0qaNzff8T4ZbaBd bWTOqhWb3BbJAyt78oolYFB1xCBJEDIJFs/MY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=+zXbyfZIoZA5DzMXbQST+NLxtD5dliYbGXVNBLnaccY=; b=NFeVJTnjUDJ0QkEUwrYq7s9F474CTqxmb3VmdptWT9N1ES6Oz16EaNZWgEXmOPK/sL vYNxdwQ9wfHZUB6Rw+tK1DwacuhKRzjZ5frcoSsbCgpqy54H3TYsNmKlfgcM7AtEIi6B qAtdAw0zV9yQSN9tqkYYW6nlcm+WD5uCfjqZ4ZPKRjdadlKZ2RGUV505AQruu7v/h+4W dzIeyFaI3SZbbpqm8g9PxvUVhQHM8jbSscbQNTlnMxA3V5R9pQ7t2K6sVoIVi2J8gLJZ Pjf/VcJUuVxz8/3la9MxQa+5q7WGhx8FZcue+sNafjWwtqxP1GFG+tS/eXz0y6lHLxA3 jFDw== X-Gm-Message-State: AA+aEWbI/QvxyB3iK6kqhlwaU05+RGU60W6iVKlJLj2U8TwGsIbKDD1x 3nJy0DAa+Ws0vFkeC0p0uTYpaP6VCWY= X-Google-Smtp-Source: AFSGD/XBbqM9AzA9qVvN0wuUheiYo3AsLk/RD2cwVmB6Tr12cZX8gjl4ARpVTAGXN/0uNeb9wbyWeA== X-Received: by 2002:a1c:c181:: with SMTP id r123mr2768288wmf.8.1545140621924; Tue, 18 Dec 2018 05:43:41 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id h10sm1851250wmf.44.2018.12.18.05.43.40 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 18 Dec 2018 05:43:41 -0800 (PST) Date: Tue, 18 Dec 2018 13:43:39 +0000 From: Leif Lindholm To: Ard Biesheuvel Cc: edk2-devel@lists.01.org, Sami Mujawar , Thomas Panakamattam Abraham , Meenakshi Aggarwal , Udit Kumar , Matteo Carlini , Nariman Poushin Message-ID: <20181218134339.jorvyyciarq3lb3d@bivouac.eciton.net> References: <20181218131015.20062-1-ard.biesheuvel@linaro.org> <20181218131015.20062-4-ard.biesheuvel@linaro.org> MIME-Version: 1.0 In-Reply-To: <20181218131015.20062-4-ard.biesheuvel@linaro.org> User-Agent: NeoMutt/20170113 (1.7.2) 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 13:43:44 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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? > 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 >