From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Leif Lindholm <leif.lindholm@linaro.org>
Cc: "edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
Sami Mujawar <sami.mujawar@arm.com>,
Thomas Panakamattam Abraham <thomas.abraham@arm.com>,
Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>,
Udit Kumar <udit.kumar@nxp.com>,
Matteo Carlini <Matteo.Carlini@arm.com>,
Nariman Poushin <nariman.poushin@linaro.org>
Subject: Re: [PATCH 3/4] ArmPkg/GenericWatchdogDxe: clean up the code
Date: Tue, 18 Dec 2018 16:19:20 +0100 [thread overview]
Message-ID: <CAKv+Gu_R8p6DqjnF92Hnj9Dt5ZqrRTAUO8mVea-WWVS-0QxMTw@mail.gmail.com> (raw)
In-Reply-To: <20181218134339.jorvyyciarq3lb3d@bivouac.eciton.net>
On Tue, 18 Dec 2018 at 14:43, Leif Lindholm <leif.lindholm@linaro.org> 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 <ard.biesheuvel@linaro.org>
> > ---
> > 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 <leif.lindholm@linaro.org>
>
> 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
> >
next prev parent reply other threads:[~2018-12-18 15:19 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-18 13:10 [PATCH 0/4] ArmPkg, ArmPlatformPkg: watchdog driver cleanup Ard Biesheuvel
2018-12-18 13:10 ` [PATCH 1/4] ArmPlatformPkg/SP805WatchdogDxe: cosmetic cleanup Ard Biesheuvel
2018-12-18 13:35 ` Leif Lindholm
2018-12-18 13:10 ` [PATCH 2/4] ArmPlatformPkg/SP805WatchdogDxe: switch to interrupt mode Ard Biesheuvel
2018-12-18 13:39 ` Leif Lindholm
2018-12-18 16:29 ` Ard Biesheuvel
2018-12-19 9:03 ` Thomas Abraham
2018-12-18 15:58 ` Udit Kumar
2018-12-18 16:23 ` Ard Biesheuvel
2018-12-18 13:10 ` [PATCH 3/4] ArmPkg/GenericWatchdogDxe: clean up the code Ard Biesheuvel
2018-12-18 13:43 ` Leif Lindholm
2018-12-18 15:19 ` Ard Biesheuvel [this message]
2018-12-18 15:24 ` Leif Lindholm
2018-12-18 13:10 ` [PATCH 4/4] ArmPkg/GenericWatchdogDxe: implement RegisterHandler() method Ard Biesheuvel
2018-12-18 13:44 ` Leif Lindholm
2018-12-18 16:00 ` Udit Kumar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAKv+Gu_R8p6DqjnF92Hnj9Dt5ZqrRTAUO8mVea-WWVS-0QxMTw@mail.gmail.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox