Re: [edk2-platforms][PATCH V1 1/1] AdvancedFeaturePkg/Ipmi: Fix GCC Build Failures

["Kubacki, Michael A" <michael.a.kubacki@intel.com>]

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.kubacki@intel.com>;
> devel@edk2.groups.io
> Cc: Bi, Dandan <dandan.bi@intel.com>; Gao, Liming <liming.gao@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.bi@intel.com>; Chaganty, Rangasai V
> <rangasai.v.chaganty@intel.com>; Gao, Liming <liming.gao@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.bi@intel.com>
> Cc: Sai Chaganty <rangasai.v.chaganty@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Michael Kubacki <michael.a.kubacki@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