Reviewed-by: Ray Ni <ray.ni@intel.com>
I agree that with “static”, the “mAcpiTimerLib” prefix is not really needed. But having it does no harm😊
I remember that coding style doc says “static” instead of “STATIC”. “STATIC” was the old requirement.
Thanks,
Ray
From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
Sent: Tuesday, December 5, 2023 6:13 AM
To: Pedro Falcato <pedro.falcato@gmail.com>; devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [PATCH v2] PcAtChipsetPkg: Fix AcpiTimerLib incompatibility with XhciDxe
> -----Original Message-----
> From: Pedro Falcato <pedro.falcato@gmail.com>
> Sent: Monday, December 4, 2023 11:59 AM
> To: devel@edk2.groups.io; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>
> Cc: Ni, Ray <ray.ni@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: Re: [edk2-devel] [PATCH v2] PcAtChipsetPkg: Fix AcpiTimerLib
> incompatibility with XhciDxe
>
> On Mon, Dec 4, 2023 at 6:48 PM Nate DeSimone
> <nathaniel.l.desimone@intel.com> wrote:
> >
> > The DXE & MM standalone variant of AcpiTimerLib defines a global named
> > mPerformanceCounterFrequency. A global with an identical name is also
> > present in MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> >
> > Since XhciDxe has a dependency on TimerLib, this can cause link errors
> > due to the same symbol being defined twice if the platform DSC chooses
> > to use AcpiTimerLib as the TimerLib implementation for any given
> > platform.
> >
> > To resolve this, I have changed made the definition of
> > mPerformanceCounterFrequency to static and renamed it to
> > mAcpiTimerLibTscFrequency. Since this variable is not used outside of
> > the DxeStandaloneMmAcpiTimerLib.c compilation unit, there is no reason
> > to have it exported as a global.
> >
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Signed-off-by: Nate DeSimone <nathaniel.l.desimone@intel.com>
> > ---
> > .../AcpiTimerLib/DxeStandaloneMmAcpiTimerLib.c | 18
> > +++++++++---------
> > 1 file changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git
> > a/PcAtChipsetPkg/Library/AcpiTimerLib/DxeStandaloneMmAcpiTimerLib.c
> > b/PcAtChipsetPkg/Library/AcpiTimerLib/DxeStandaloneMmAcpiTimerLib.c
> > index 16ac48938f..ccceb8a649 100644
> > ---
> > a/PcAtChipsetPkg/Library/AcpiTimerLib/DxeStandaloneMmAcpiTimerLib.c
> > +++
> b/PcAtChipsetPkg/Library/AcpiTimerLib/DxeStandaloneMmAcpiTimerLib.
> > +++ c
> > @@ -1,7 +1,7 @@
> > /** @file
> > ACPI Timer implements one instance of Timer Library.
> >
> > - Copyright (c) 2013 - 2018, Intel Corporation. All rights
> > reserved.<BR>
> > + Copyright (c) 2013 - 2023, Intel Corporation. All rights
> > + reserved.<BR>
> > SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > **/
> > @@ -11,6 +11,11 @@
> > #include <Library/BaseLib.h>
> > #include <Library/HobLib.h>
> >
> > +//
> > +// Cached performance counter frequency // static UINT64
> > +mAcpiTimerLibTscFrequency = 0;
>
> I'd say you don't need to rename it if it's a static variable. Now the identifier is
> 2x longer with no additional relevant information.
This is in response to Mike K's feedback:
https://edk2.groups.io/g/devel/message/111966
>
> Aren't we supposed to use STATIC vs static, CONST vs const, etc? Annoyingly
> :/
Apparently not. Honestly, I lose track of what the current coding style is sometimes too.
>
> --
> Pedro