Re: [edk2-devel] [PATCH v2] PcAtChipsetPkg: Fix AcpiTimerLib incompatibility with XhciDxe
Pushed as df2ec2a -Original Message- From: devel@edk2.groups.io On Behalf Of Nate DeSimone Sent: Monday, December 4, 2023 10:48 AM To: devel@edk2.groups.io Cc: Ni, Ray ; Kinney, Michael D Subject: [edk2-devel] [PATCH v2] 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 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 Cc: Michael D Kinney Signed-off-by: Nate DeSimone --- .../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. + Copyright (c) 2013 - 2023, Intel Corporation. All rights + reserved. SPDX-License-Identifier: BSD-2-Clause-Patent **/ @@ -11,6 +11,11 @@ #include #include +// +// Cached performance counter frequency // static UINT64 +mAcpiTimerLibTscFrequency = 0; + extern GUID mFrequencyHobGuid; /** @@ -48,11 +53,6 @@ InternalCalculateTscFrequency ( VOID ); -// -// Cached performance counter frequency -// -UINT64 mPerformanceCounterFrequency = 0; - /** Internal function to retrieves the 64-bit frequency in Hz. @@ -66,7 +66,7 @@ InternalGetPerformanceCounterFrequency ( VOID ) { - return mPerformanceCounterFrequency; + return mAcpiTimerLibTscFrequency; } /** @@ -92,9 +92,9 @@ CommonAcpiTimerLibConstructor ( // GuidHob = GetFirstGuidHob (); if (GuidHob != NULL) { -mPerformanceCounterFrequency = *(UINT64 *)GET_GUID_HOB_DATA (GuidHob); +mAcpiTimerLibTscFrequency = *(UINT64 *)GET_GUID_HOB_DATA (GuidHob); } else { -mPerformanceCounterFrequency = InternalCalculateTscFrequency (); +mAcpiTimerLibTscFrequency = InternalCalculateTscFrequency (); } return EFI_SUCCESS; -- 2.39.2.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112141): https://edk2.groups.io/g/devel/message/112141 Mute This Topic: https://groups.io/mt/103024047/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2] PcAtChipsetPkg: Fix AcpiTimerLib incompatibility with XhciDxe
Reviewed-by: Ray Ni 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 Sent: Tuesday, December 5, 2023 6:13 AM To: Pedro Falcato ; devel@edk2.groups.io Cc: Ni, Ray ; Kinney, Michael D Subject: RE: [edk2-devel] [PATCH v2] PcAtChipsetPkg: Fix AcpiTimerLib incompatibility with XhciDxe > -Original Message- > From: Pedro Falcato mailto:pedro.falc...@gmail.com>> > Sent: Monday, December 4, 2023 11:59 AM > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Desimone, Nathaniel L > mailto:nathaniel.l.desim...@intel.com>> > Cc: Ni, Ray mailto:ray...@intel.com>>; Kinney, Michael D > mailto:michael.d.kin...@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 > mailto:nathaniel.l.desim...@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 mailto:ray...@intel.com>> > > Cc: Michael D Kinney > > mailto:michael.d.kin...@intel.com>> > > Signed-off-by: Nate DeSimone > > mailto:nathaniel.l.desim...@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. > > + Copyright (c) 2013 - 2023, Intel Corporation. All rights > > + reserved. > >SPDX-License-Identifier: BSD-2-Clause-Patent > > > > **/ > > @@ -11,6 +11,11 @@ > > #include > > #include > > > > +// > > +// 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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112070): https://edk2.groups.io/g/devel/message/112070 Mute This Topic: https://groups.io/mt/102976788/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2] PcAtChipsetPkg: Fix AcpiTimerLib incompatibility with XhciDxe
> -Original Message- > From: Pedro Falcato > Sent: Monday, December 4, 2023 11:59 AM > To: devel@edk2.groups.io; Desimone, Nathaniel L > > Cc: Ni, Ray ; Kinney, Michael D > > Subject: Re: [edk2-devel] [PATCH v2] PcAtChipsetPkg: Fix AcpiTimerLib > incompatibility with XhciDxe > > On Mon, Dec 4, 2023 at 6:48 PM Nate DeSimone > mailto:nathaniel.l.desim...@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 mailto:ray...@intel.com>> > > Cc: Michael D Kinney > > mailto:michael.d.kin...@intel.com>> > > Signed-off-by: Nate DeSimone > > mailto:nathaniel.l.desim...@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. > > + Copyright (c) 2013 - 2023, Intel Corporation. All rights > > + reserved. > >SPDX-License-Identifier: BSD-2-Clause-Patent > > > > **/ > > @@ -11,6 +11,11 @@ > > #include > > #include > > > > +// > > +// 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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112052): https://edk2.groups.io/g/devel/message/112052 Mute This Topic: https://groups.io/mt/102976788/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2] PcAtChipsetPkg: Fix AcpiTimerLib incompatibility with XhciDxe
On Mon, Dec 4, 2023 at 6:48 PM Nate DeSimone 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 > Cc: Michael D Kinney > Signed-off-by: Nate DeSimone > --- > .../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. > + Copyright (c) 2013 - 2023, Intel Corporation. All rights reserved. >SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ > @@ -11,6 +11,11 @@ > #include > #include > > +// > +// 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. Aren't we supposed to use STATIC vs static, CONST vs const, etc? Annoyingly :/ -- Pedro -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112051): https://edk2.groups.io/g/devel/message/112051 Mute This Topic: https://groups.io/mt/102976788/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH v2] 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 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 Cc: Michael D Kinney Signed-off-by: Nate DeSimone --- .../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. + Copyright (c) 2013 - 2023, Intel Corporation. All rights reserved. SPDX-License-Identifier: BSD-2-Clause-Patent **/ @@ -11,6 +11,11 @@ #include #include +// +// Cached performance counter frequency +// +static UINT64 mAcpiTimerLibTscFrequency = 0; + extern GUID mFrequencyHobGuid; /** @@ -48,11 +53,6 @@ InternalCalculateTscFrequency ( VOID ); -// -// Cached performance counter frequency -// -UINT64 mPerformanceCounterFrequency = 0; - /** Internal function to retrieves the 64-bit frequency in Hz. @@ -66,7 +66,7 @@ InternalGetPerformanceCounterFrequency ( VOID ) { - return mPerformanceCounterFrequency; + return mAcpiTimerLibTscFrequency; } /** @@ -92,9 +92,9 @@ CommonAcpiTimerLibConstructor ( // GuidHob = GetFirstGuidHob (); if (GuidHob != NULL) { -mPerformanceCounterFrequency = *(UINT64 *)GET_GUID_HOB_DATA (GuidHob); +mAcpiTimerLibTscFrequency = *(UINT64 *)GET_GUID_HOB_DATA (GuidHob); } else { -mPerformanceCounterFrequency = InternalCalculateTscFrequency (); +mAcpiTimerLibTscFrequency = InternalCalculateTscFrequency (); } return EFI_SUCCESS; -- 2.39.2.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112050): https://edk2.groups.io/g/devel/message/112050 Mute This Topic: https://groups.io/mt/102976788/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-