Re: [PATCH v4 2/2] MdeMoudlePkg/CapsulePei: Substantial change on UefiCapsule.c

["Gao, Zhichao" <zhichao.gao@intel.com>]

> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: Thursday, June 6, 2019 7:18 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>
> Cc: devel@edk2.groups.io; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Gao, Liming <liming.gao@intel.com>; Sean Brogan
> <sean.brogan@microsoft.com>; Michael Turner
> <Michael.Turner@microsoft.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>
> Subject: Re: [PATCH v4 2/2] MdeMoudlePkg/CapsulePei: Substantial change
> on UefiCapsule.c
> 
> Zhichao,
> 
> Thank you for splitting the patches up. This makes for much better history.
> However,
> 
> On Wed, Jun 05, 2019 at 09:15:45AM +0800, Zhichao Gao wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1853
> >
> > AreCapsulesStaged do not need to return the status, only boolean
> > result is useful. So directly return a boolean value.
> > Cannot initialize the variable at its definition.
> >
> > GetScatterGatherHeadEntries: use allocated buffer instead of fixed
> > array to handle the condition which the SG list is larger then the
> > array size.
> >
> > Remove API specifier AreCapsulesStaged and
> GetScatterGatherHeadEntries
> > because they are internal used.
> >
> > Fix some coding style issues.
> 
> The above are three or four (is the EFIAPI change a coding style issue or not?)
> unrelated changes. Could you please break this up further?

EFIAPI is mentioned in CCS_2_1 spec. So I think it is a coding style issue too. The other coding style issue is lacking space between the function name and bracket.
I would separate 2/2 base on the purpose to make the history clear.

Thanks,
Zhichao

> 
> Best Regards,
> 
> Leif
> 
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > Cc: Michael Turner <Michael.Turner@microsoft.com>
> > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > Signed-off-by: Zhichao gao <zhichao.gao@intel.com>
> > ---
> >  .../Universal/CapsulePei/UefiCapsule.c        | 103 +++++++++---------
> >  1 file changed, 54 insertions(+), 49 deletions(-)
> >
> > diff --git a/MdeModulePkg/Universal/CapsulePei/UefiCapsule.c
> > b/MdeModulePkg/Universal/CapsulePei/UefiCapsule.c
> > index 7c8c7a0f45..fabf30926c 100644
> > --- a/MdeModulePkg/Universal/CapsulePei/UefiCapsule.c
> > +++ b/MdeModulePkg/Universal/CapsulePei/UefiCapsule.c
> > @@ -1,7 +1,7 @@
> >  /** @file
> >    Capsule update PEIM for UEFI2.0
> >
> > -Copyright (c) 2006 - 2018, Intel Corporation. All rights
> > reserved.<BR>
> > +Copyright (c) 2006 - 2019, Intel Corporation. All rights
> > +reserved.<BR>
> >  Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
> >
> >  SPDX-License-Identifier: BSD-2-Clause-Patent @@ -10,6 +10,8 @@
> > SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> >  #include "Capsule.h"
> >
> > +#define DEFAULT_SG_LIST_HEADS       (20)
> > +
> >  #ifdef MDE_CPU_IA32
> >  //
> >  // Global Descriptor Table (GDT)
> > @@ -793,30 +795,21 @@ BuildMemoryResourceDescriptor (
> >  /**
> >    Check if the capsules are staged.
> >
> > -  @param UpdateCapsules   A pointer to return the check result.
> > -
> > -  @retval EFI_INVALID_PARAMETER   The parameter is null.
> > -  @retval EFI_SUCCESS             The Capsules are staged.
> > +  @retval TRUE              The capsules are staged.
> > +  @retval FALSE             The capsules are not staged.
> >
> >  **/
> > -EFI_STATUS
> > -EFIAPI
> > -AreCapsulesStaged(
> > -  OUT BOOLEAN     *UpdateCapsules
> > +BOOLEAN
> > +AreCapsulesStaged (
> > +  VOID
> >    )
> >  {
> >    EFI_STATUS                        Status;
> >    UINTN                             Size;
> >    EFI_PEI_READ_ONLY_VARIABLE2_PPI   *PPIVariableServices;
> > -  EFI_PHYSICAL_ADDRESS              CapsuleDataPtr64 = 0;
> > -
> > -  if (UpdateCapsules == NULL) {
> > -    DEBUG ((DEBUG_ERROR, "%a Invalid parameters.  Inputs can't be
> NULL\n", __FUNCTION__));
> > -    ASSERT (UpdateCapsules != NULL);
> > -    return EFI_INVALID_PARAMETER;
> > -  }
> > +  EFI_PHYSICAL_ADDRESS              CapsuleDataPtr64;
> >
> > -  *UpdateCapsules = FALSE;
> > +  CapsuleDataPtr64 = 0;
> >
> >    Status = PeiServicesLocatePpi(
> >                &gEfiPeiReadOnlyVariable2PpiGuid, @@ -827,7 +820,7 @@
> > AreCapsulesStaged(
> >
> >    if (EFI_ERROR (Status)) {
> >      DEBUG ((DEBUG_ERROR, "Failed to find ReadOnlyVariable2PPI\n"));
> > -    return Status;
> > +    return FALSE;
> >    }
> >
> >    //
> > @@ -844,14 +837,12 @@ AreCapsulesStaged(
> >                                    );
> >
> >    if (!EFI_ERROR (Status)) {
> > -    *UpdateCapsules = TRUE;
> > +    return TRUE;
> >    }
> >
> > -  return EFI_SUCCESS;
> > +  return FALSE;
> >  }
> >
> > -#define MAX_SG_LIST_HEADS (20)
> > -
> >  /**
> >    Check all the variables for SG list heads and get the count and addresses.
> >
> > @@ -865,23 +856,24 @@ AreCapsulesStaged(
> >
> >  **/
> >  EFI_STATUS
> > -EFIAPI
> > -GetScatterGatherHeadEntries(
> > +GetScatterGatherHeadEntries (
> >    OUT UINTN *ListLength,
> >    OUT EFI_PHYSICAL_ADDRESS **HeadList
> >    )
> >  {
> > -  EFI_STATUS                       Status;
> > -  UINTN                            Size;
> > -  UINTN                            Index;
> > -  UINTN                            TempIndex;
> > -  UINTN                            ValidIndex;
> > -  BOOLEAN                          Flag;
> > -  CHAR16                           CapsuleVarName[30];
> > -  CHAR16                           *TempVarName;
> > -  EFI_PHYSICAL_ADDRESS             CapsuleDataPtr64;
> > -  EFI_PEI_READ_ONLY_VARIABLE2_PPI  *PPIVariableServices;
> > -  EFI_PHYSICAL_ADDRESS             TempList[MAX_SG_LIST_HEADS];
> > +  EFI_STATUS                        Status;
> > +  UINTN                             Size;
> > +  UINTN                             Index;
> > +  UINTN                             TempIndex;
> > +  UINTN                             ValidIndex;
> > +  BOOLEAN                           Flag;
> > +  CHAR16                            CapsuleVarName[30];
> > +  CHAR16                            *TempVarName;
> > +  EFI_PHYSICAL_ADDRESS              CapsuleDataPtr64;
> > +  EFI_PEI_READ_ONLY_VARIABLE2_PPI   *PPIVariableServices;
> > +  EFI_PHYSICAL_ADDRESS              *TempList;
> > +  EFI_PHYSICAL_ADDRESS              *EnlargedTempList;
> > +  UINTN                             TempListLength;
> >
> >    Index             = 0;
> >    TempVarName       = NULL;
> > @@ -911,16 +903,26 @@ GetScatterGatherHeadEntries(
> >      return Status;
> >    }
> >
> > +  //
> > +  // Allocate memory for sg list head  //  TempListLength =
> > + DEFAULT_SG_LIST_HEADS * sizeof (EFI_PHYSICAL_ADDRESS);  TempList =
> > + AllocateZeroPool (TempListLength);  if (TempList == NULL) {
> > +    DEBUG((DEBUG_ERROR, "Failed to allocate memory\n"));
> > +    return EFI_OUT_OF_RESOURCES;
> > +  }
> > +
> >    //
> >    // setup var name buffer for update capsules
> >    //
> >    StrCpyS (CapsuleVarName, sizeof (CapsuleVarName) / sizeof (CHAR16),
> EFI_CAPSULE_VARIABLE_NAME);
> >    TempVarName = CapsuleVarName + StrLen (CapsuleVarName);
> > -  while (ValidIndex < MAX_SG_LIST_HEADS) {
> > +  while (TRUE) {
> >      if (Index != 0) {
> >        UnicodeValueToStringS (
> >          TempVarName,
> > -        (sizeof (CapsuleVarName) - ((StrLen (CapsuleVarName) + 1) * sizeof
> (CHAR16))),
> > +        (sizeof(CapsuleVarName) - ((UINTN)TempVarName -
> > + (UINTN)CapsuleVarName)),
> >          0,
> >          Index,
> >          0
> > @@ -958,6 +960,17 @@ GetScatterGatherHeadEntries(
> >        continue;
> >      }
> >
> > +    //
> > +    // The TempList is full, enlarge it
> > +    //
> > +    if ((ValidIndex + 1) >= TempListLength) {
> > +      EnlargedTempList = AllocateZeroPool (TempListLength * 2);
> > +      CopyMem (EnlargedTempList, TempList, TempListLength);
> > +      FreePool (TempList);
> > +      TempList = EnlargedTempList;
> > +      TempListLength *= 2;
> > +    }
> > +
> >      //
> >      // add it to the cached list
> >      //
> > @@ -1122,19 +1135,11 @@ CheckCapsuleUpdate (
> >    IN EFI_PEI_SERVICES           **PeiServices
> >    )
> >  {
> > -  EFI_STATUS  Status;
> > -  BOOLEAN     Update;
> > -
> > -  Status = AreCapsulesStaged (&Update);
> > -
> > -  if (!EFI_ERROR (Status)) {
> > -    if (Update) {
> > -      Status = EFI_SUCCESS;
> > -    } else {
> > -      Status = EFI_NOT_FOUND;
> > -    }
> > +  if (AreCapsulesStaged ()) {
> > +    return EFI_SUCCESS;
> > +  } else {
> > +    return EFI_NOT_FOUND;
> >    }
> > -  return Status;
> >  }
> >  /**
> >    This function will look at a capsule and determine if it's a test pattern.
> > --
> > 2.21.0.windows.1
> >