[edk2-devel] [PATCH v4 2/2] MdeMoudlePkg/CapsulePei: Substantial change on UefiCapsule.c
Gao, Zhichao
zhichao.gao at intel.com
Mon Jun 10 02:09:37 UTC 2019
> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm at linaro.org]
> Sent: Thursday, June 6, 2019 7:18 PM
> To: Gao, Zhichao <zhichao.gao at intel.com>
> Cc: devel at edk2.groups.io; Wang, Jian J <jian.j.wang at intel.com>; Wu, Hao A
> <hao.a.wu at intel.com>; Ni, Ray <ray.ni at intel.com>; Zeng, Star
> <star.zeng at intel.com>; Gao, Liming <liming.gao at intel.com>; Sean Brogan
> <sean.brogan at microsoft.com>; Michael Turner
> <Michael.Turner at microsoft.com>; Bret Barkelew
> <Bret.Barkelew at 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 at intel.com>
> > Cc: Hao A Wu <hao.a.wu at intel.com>
> > Cc: Ray Ni <ray.ni at intel.com>
> > Cc: Star Zeng <star.zeng at intel.com>
> > Cc: Liming Gao <liming.gao at intel.com>
> > Cc: Sean Brogan <sean.brogan at microsoft.com>
> > Cc: Michael Turner <Michael.Turner at microsoft.com>
> > Cc: Bret Barkelew <Bret.Barkelew at microsoft.com>
> > Cc: Leif Lindholm <leif.lindholm at linaro.org>
> > Signed-off-by: Zhichao gao <zhichao.gao at 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
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#42049): https://edk2.groups.io/g/devel/message/42049
Mute This Topic: https://groups.io/mt/31932897/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list