1. Patchew
  2. EDK2
  3. View series

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

Nate DeSimone posted 1 patch 5 months ago
Failed in applying to current master (apply log)
There is a newer version of this series
.../Library/AcpiTimerLib/DxeStandaloneMmAcpiTimerLib.c        | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
[edk2-devel] [PATCH v1] PcAtChipsetPkg: Fix AcpiTimerLib incompatibility with XhciDxe
Posted by Nate DeSimone 5 months ago 
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 (#111962): https://edk2.groups.io/g/devel/message/111962
Mute This Topic: https://groups.io/mt/102907651/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1] PcAtChipsetPkg: Fix AcpiTimerLib incompatibility with XhciDxe
Posted by Ni, Ray 5 months ago 
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 (#111965): https://edk2.groups.io/g/devel/message/111965
Mute This Topic: https://groups.io/mt/102907651/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1] PcAtChipsetPkg: Fix AcpiTimerLib incompatibility with XhciDxe
Posted by Michael D Kinney 5 months ago 
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1] PcAtChipsetPkg: Fix AcpiTimerLib incompatibility with XhciDxe
Posted by Ni, Ray 5 months ago 
Mike,
Thanks!

Thanks,
Ray
> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Friday, December 1, 2023 12:39 PM
> To: Ni, Ray <ray.ni@intel.com>; 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
> 
> 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 (#111967): https://edk2.groups.io/g/devel/message/111967
Mute This Topic: https://groups.io/mt/102907651/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.