Re: [edk2-devel] [PATCH v1] PcAtChipsetPkg: Fix AcpiTimerLib incompatibility with XhciDxe

["Michael D Kinney" <michael.d.kinney@intel.com>]

Hi Ray,

'static' is allowed and actually preferred for module globals 
that start with 'm' that are scoped to a single C file.

Globals that start with 'g' that need to be access by multiple
C files can not be static.

Since 'm' globals could be changed to 'g' globals due to
maintenance, we want to avoid symbol collisions between
'g' globals.  Prefixing all globals with a module name helps
prevent symbol collisions.

Summary
========
* Use lower case 'static'
* Use 'static' for 'm' globals
* Do not use 'static' for 'g' globals
* Add module/lib specific prefix to 'm' and 'g' global

Mike

> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Thursday, November 30, 2023 7:13 PM
> To: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>;
> devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: RE: [PATCH v1] PcAtChipsetPkg: Fix AcpiTimerLib incompatibility
> with XhciDxe
> 
> Mike,
> Does today's EDK2 C coding style spec allow using "STATIC" for global
> variables?
> Or lower case "static"?
> Or changing the variable to a name with lib name prefix, e.g.: "
> mTimerLibPerformanceCounterFrequency"?
> 
> 
> Thanks,
> Ray
> > -----Original Message-----
> > From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
> > Sent: Friday, December 1, 2023 9:56 AM
> > To: devel@edk2.groups.io
> > Cc: Ni, Ray <ray.ni@intel.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>
> > Subject: [PATCH v1] PcAtChipsetPkg: Fix AcpiTimerLib incompatibility with
> > XhciDxe
> >
> > 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. 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>
> > ---
> >  .../Library/AcpiTimerLib/DxeStandaloneMmAcpiTimerLib.c        | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git
> > a/PcAtChipsetPkg/Library/AcpiTimerLib/DxeStandaloneMmAcpiTimerLib.c
> > b/PcAtChipsetPkg/Library/AcpiTimerLib/DxeStandaloneMmAcpiTimerLib.c
> > index 16ac48938f..41d2af7d55 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
> >
> >  **/
> > @@ -51,7 +51,7 @@ InternalCalculateTscFrequency (
> >  //
> >  // Cached performance counter frequency
> >  //
> > -UINT64  mPerformanceCounterFrequency = 0;
> > +STATIC UINT64 mPerformanceCounterFrequency = 0;
> >
> >  /**
> >    Internal function to retrieves the 64-bit frequency in Hz.
> > --
> > 2.39.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111966): https://edk2.groups.io/g/devel/message/111966
Mute This Topic: https://groups.io/mt/102907651/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-