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

["Ni, Ray" <ray.ni@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4424 bytes --]

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<mailto:pedro.falcato@gmail.com>>

> Sent: Monday, December 4, 2023 11:59 AM

> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Desimone, Nathaniel L

> <nathaniel.l.desimone@intel.com<mailto:nathaniel.l.desimone@intel.com>>

> Cc: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Kinney, Michael D

> <michael.d.kinney@intel.com<mailto: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<mailto: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<mailto:ray.ni@intel.com>>

> > Cc: Michael D Kinney <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>

> > Signed-off-by: Nate DeSimone <nathaniel.l.desimone@intel.com<mailto: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


-=-=-=-=-=-=-=-=-=-=-=-
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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 11824 bytes --]