While I'm all for improvements throughout the file, the scope of this change is just to fix the GCC failures and make general style improvements to impacted lines. I'll send a patch V2 with the DEBUG macro updated to DEBUG_ERROR. Anything further peripheral cleanup is a slippery slope for this patch.
Thanks, Michael > -----Original Message----- > From: Chaganty, Rangasai V > Sent: Thursday, September 12, 2019 1:31 AM > To: Kubacki, Michael A <michael.a.kuba...@intel.com>; > devel@edk2.groups.io > Cc: Bi, Dandan <dandan...@intel.com>; Gao, Liming <liming....@intel.com> > Subject: RE: [edk2-platforms][PATCH V1 1/1] AdvancedFeaturePkg/Ipmi: Fix > GCC Build Failures > > Two comments on SolStatus.c: > 1. "SolEnabled" seems more appropriate variable name than just "Enabled. > Should we consider renaming the variable name? > 2. Please replace EFI_D_ERROR with DEBUG_ERROR > > -----Original Message----- > From: Kubacki, Michael A > Sent: Wednesday, September 11, 2019 11:09 PM > To: devel@edk2.groups.io > Cc: Bi, Dandan <dandan...@intel.com>; Chaganty, Rangasai V > <rangasai.v.chaga...@intel.com>; Gao, Liming <liming....@intel.com> > Subject: [edk2-platforms][PATCH V1 1/1] AdvancedFeaturePkg/Ipmi: Fix GCC > Build Failures > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2059 > > These build failures can be reproduced simply by building the > AdvancedFeaturePkg.dsc file in GCC5. To build the whole package DSC (not > pull individual features into other packages), set the WORKSPACE variable to > the edk2 directory in the workspace as is done by executing edksetup.sh > then create the PACKAGES_PATH variable and add the Platform/Intel and > Silicon/Intel directories to the variable value. Then start the build of > AdvancedFeaturePkg.dsc: > 'build -p AdvancedFeaturePkg/AdvancedFeaturePkg.dsc' > > This change corrects the following issues reported by GCC: > * BmcAcpi.c - Cast the pointer actual parameter types passed to > functions to the formal parameter type. > * OsWdt.c - Return the Status variable as the function return value > in DriverInit (). > * PeiIpmiInit.c - Return the Status variable as the function return > value in PeimIpmiInterfaceInit () > * SolStatus.c - Remove the unused variable SOLEnabled. The variable > was not removed after a code refactoring. > > All future contributions to AdvancedFeaturePkg must successfully build in > GCC after this change. > > Cc: Dandan Bi <dandan...@intel.com> > Cc: Sai Chaganty <rangasai.v.chaga...@intel.com> > Cc: Liming Gao <liming....@intel.com> > Signed-off-by: Michael Kubacki <michael.a.kuba...@intel.com> > --- > Platform/Intel/AdvancedFeaturePkg/Ipmi/BmcAcpi/BmcAcpi.c | 16 > ++++++++++++---- > Platform/Intel/AdvancedFeaturePkg/Ipmi/IpmiInit/PeiIpmiInit.c | 15 > ++++++++------- > Platform/Intel/AdvancedFeaturePkg/Ipmi/OsWdt/OsWdt.c | 9 +++++-- > -- > Platform/Intel/AdvancedFeaturePkg/Ipmi/SolStatus/SolStatus.c | 14 > +++++--------- > 4 files changed, 30 insertions(+), 24 deletions(-) > > diff --git a/Platform/Intel/AdvancedFeaturePkg/Ipmi/BmcAcpi/BmcAcpi.c > b/Platform/Intel/AdvancedFeaturePkg/Ipmi/BmcAcpi/BmcAcpi.c > index 3b330da160..990b4b9e83 100644 > --- a/Platform/Intel/AdvancedFeaturePkg/Ipmi/BmcAcpi/BmcAcpi.c > +++ b/Platform/Intel/AdvancedFeaturePkg/Ipmi/BmcAcpi/BmcAcpi.c > @@ -1,7 +1,7 @@ > /** @file > BMC ACPI. > > -Copyright (c) 2018, Intel Corporation. All rights reserved.<BR> > +Copyright (c) 2018 - 2019, Intel Corporation. All rights reserved.<BR> > SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ > @@ -154,7 +154,7 @@ UpdateDeviceSsdtTable ( > IoRsc->BaseAddressMax = PcdGet16(PcdIpmiIoBaseAddress); > } > } > - > + > return EFI_SUCCESS; > } > > @@ -202,7 +202,7 @@ BmcAcpiEntryPoint ( > // > // Locate the firmware volume protocol > // > - Status = LocateSupportProtocol (&gEfiFirmwareVolume2ProtocolGuid, > &FwVol, 1); > + Status = LocateSupportProtocol (&gEfiFirmwareVolume2ProtocolGuid, > + (VOID **) &FwVol, 1); > if (EFI_ERROR (Status)) { > return Status; > } > @@ -216,7 +216,15 @@ BmcAcpiEntryPoint ( > while (!EFI_ERROR (Status)) { > CurrentTable = NULL; > > - Status = FwVol->ReadSection (FwVol, &gEfiCallerIdGuid, > EFI_SECTION_RAW, Instance, &CurrentTable, (UINTN *) &Size, &FvStatus); > + Status = FwVol->ReadSection ( > + FwVol, > + &gEfiCallerIdGuid, > + EFI_SECTION_RAW, > + Instance, > + (VOID **) &CurrentTable, > + (UINTN *) &Size, > + &FvStatus > + ); > if (!EFI_ERROR (Status)) { > // > // Perform any table specific updates. > diff --git a/Platform/Intel/AdvancedFeaturePkg/Ipmi/IpmiInit/PeiIpmiInit.c > b/Platform/Intel/AdvancedFeaturePkg/Ipmi/IpmiInit/PeiIpmiInit.c > index 8245aac8e9..062d20c44e 100644 > --- a/Platform/Intel/AdvancedFeaturePkg/Ipmi/IpmiInit/PeiIpmiInit.c > +++ b/Platform/Intel/AdvancedFeaturePkg/Ipmi/IpmiInit/PeiIpmiInit.c > @@ -1,7 +1,7 @@ > /** @file > - IPMI stack initialization in PEI. > + IPMI stack initialization in PEI. > > -Copyright (c) 2018, Intel Corporation. All rights reserved.<BR> > +Copyright (c) 2018 - 2019, Intel Corporation. All rights reserved.<BR> > SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ > @@ -27,7 +27,7 @@ Routine Description: > > Arguments: > > -Returns: > +Returns: > Status > > --*/ > @@ -35,7 +35,7 @@ Returns: > EFI_STATUS Status; > IPMI_GET_DEVICE_ID_RESPONSE BmcInfo; > UINT32 Retries; > - > + > // > // Set up a loop to retry for up to 30 seconds. Calculate retries not > timeout > // so that in case KCS is not enabled and EfiIpmiSendCommand() returns > @@ -71,9 +71,10 @@ Returns: > The entry point of the Ipmi PEIM. > > @param FileHandle Handle of the file being invoked. > - @param PeiServices Describes the list of possible PEI Services. > + @param PeiServices Describes the list of possible PEI Services. > > @retval EFI_SUCCESS Indicates that Ipmi initialization completed > successfully. > + @retval Others Indicates that Ipmi initialization could not complete > successfully. > **/ > EFI_STATUS > EFIAPI > @@ -85,11 +86,11 @@ PeimIpmiInterfaceInit ( > BOOLEAN UpdateMode; > EFI_STATUS Status; > > - DEBUG((EFI_D_ERROR,"IPMI Peim:Get BMC Device Id\n")); > + DEBUG ((DEBUG_INFO, "IPMI Peim:Get BMC Device Id\n")); > > // > // Get the Device ID and check if the system is in Force Update mode. > // > Status = GetDeviceId (&UpdateMode); > - return EFI_SUCCESS; > + return Status; > } > diff --git a/Platform/Intel/AdvancedFeaturePkg/Ipmi/OsWdt/OsWdt.c > b/Platform/Intel/AdvancedFeaturePkg/Ipmi/OsWdt/OsWdt.c > index c5612d4b6d..25139eadba 100644 > --- a/Platform/Intel/AdvancedFeaturePkg/Ipmi/OsWdt/OsWdt.c > +++ b/Platform/Intel/AdvancedFeaturePkg/Ipmi/OsWdt/OsWdt.c > @@ -1,7 +1,7 @@ > /** @file > IPMI Os watchdog timer Driver. > > -Copyright (c) 2018, Intel Corporation. All rights reserved.<BR> > +Copyright (c) 2018 - 2019, Intel Corporation. All rights reserved.<BR> > SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ > @@ -93,11 +93,12 @@ Arguments: > SystemTable - Pointer to the System Table > > Returns: > - EFI_SUCCESS - Protocol successfully started and installed. > + @retval EFI_SUCCESS Protocol successfully started and installed. > + @retval EFI_OUT_OF_RESOURCES The event could not be allocated. > > --*/ > { > - EFI_STATUS Status; > + EFI_STATUS Status; > > Status = gBS->CreateEvent ( > EVT_SIGNAL_EXIT_BOOT_SERVICES, @@ -107,5 +108,5 @@ > Returns: > &mExitBootServicesEvent > ); > > - return EFI_SUCCESS; > + return Status; > } > diff --git a/Platform/Intel/AdvancedFeaturePkg/Ipmi/SolStatus/SolStatus.c > b/Platform/Intel/AdvancedFeaturePkg/Ipmi/SolStatus/SolStatus.c > index 69479bdbf5..36de0604ee 100644 > --- a/Platform/Intel/AdvancedFeaturePkg/Ipmi/SolStatus/SolStatus.c > +++ b/Platform/Intel/AdvancedFeaturePkg/Ipmi/SolStatus/SolStatus.c > @@ -1,7 +1,7 @@ > /** @file > IPMI Serial Over Lan Driver. > > -Copyright (c) 2018, Intel Corporation. All rights reserved.<BR> > +Copyright (c) 2018 - 2019, Intel Corporation. All rights reserved.<BR> > SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ > @@ -70,7 +70,7 @@ GetSOLStatus ( > if (Status == EFI_SUCCESS) { > *Data = GetConfigurationParametersResponse.ParameterData[0]; > } > - > + > return Status; > } > > @@ -132,11 +132,11 @@ SolStatusEntryPoint ( > IN EFI_SYSTEM_TABLE *SystemTable > ) > /*++ > - > + > Routine Description: > This is the standard EFI driver point. This function intitializes > the private data required for creating SOL Status Driver. > - > + > Arguments: > ImageHandle - Handle for the image of this driver > SystemTable - Pointer to the EFI System Table > @@ -150,14 +150,10 @@ SolStatusEntryPoint ( > EFI_STATUS Status = EFI_SUCCESS; > UINT8 Channel; > BOOLEAN Enabled = FALSE; > - BOOLEAN SOLEnabled = FALSE; > - > + > for (Channel = 1; Channel <= PcdGet8(PcdMaxSOLChannels); Channel++) { > Status = GetSOLStatus (Channel, > IPMI_SOL_CONFIGURATION_PARAMETER_SOL_ENABLE, &Enabled); > if (Status == EFI_SUCCESS) { > - if (Enabled == TRUE) { > - SOLEnabled = TRUE; > - } > DEBUG ((EFI_D_ERROR, "SOL enabling status for channel %x is %x\n", > Channel, Enabled)); > } else { > DEBUG ((EFI_D_ERROR, "Failed to get channel %x SOL status from BMC!, > status is %x\n", Channel, Status)); > -- > 2.16.2.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#47197): https://edk2.groups.io/g/devel/message/47197 Mute This Topic: https://groups.io/mt/34112296/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
