Hi Ard, > -----Original Message----- > From: edk2-devel [mailto:[email protected]] On Behalf Of > Ard Biesheuvel > Sent: 23 December 2017 14:12 > To: Evan Lloyd <[email protected]> > Cc: "[email protected]"@arm.com; > "[email protected]"@arm.com; "[email protected]"@arm.com; edk2- > [email protected]; Thomas Abraham <[email protected]>; Arvind > Chauhan <[email protected]>; > "[email protected]"@arm.com > Subject: Re: [edk2] [PATCH edk2-platforms v2 06/18] ARM/VExpressPkg: Add > and update debug ASSERTS > > On 22 December 2017 at 19:08, <[email protected]> wrote: > > From: Girish Pathak <girish.pathak at arm.com> > > > > This change adds some debug assertions e.g to catch NULL pointer > > errors missing in PL11Lcd and HdLcd platform libraries. > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Girish Pathak <[email protected]> > > Signed-off-by: Evan Lloyd <[email protected]> > > --- > > > Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpr > ess.c | 22 +++++++++++++++++- > > > > > Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArm > VEx > > press.c | 24 +++++++++++++++++++- > > 2 files changed, 44 insertions(+), 2 deletions(-) > > > > diff --git > > > a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVEx > pres > > s.c > > > b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVEx > pres > > s.c index > > > 6afd764897f49c64490ce891682f99bb0f5d993b..a8fe8696da0653017ce9fa6e4a > 86 > > caf283bc04c9 100644 > > --- > > > a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVEx > pres > > s.c > > +++ > b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVEx > > +++ press.c > > @@ -153,6 +153,9 @@ LcdPlatformGetVram ( > > EFI_STATUS Status; > > EFI_ALLOCATE_TYPE AllocationType; > > > > + ASSERT (VramBaseAddress != NULL); > > + ASSERT (VramSize != NULL); > > + > > // Set the vram size > > *VramSize = LCD_VRAM_SIZE; > > > > @@ -171,6 +174,7 @@ LcdPlatformGetVram ( > > VramBaseAddress > > ); > > if (EFI_ERROR (Status)) { > > + ASSERT (FALSE); > > return Status; > > } > > > > @@ -181,8 +185,8 @@ LcdPlatformGetVram ( > > *VramSize, > > EFI_MEMORY_WC > > ); > > - ASSERT_EFI_ERROR (Status); > > if (EFI_ERROR (Status)) { > > + ASSERT (FALSE); > > As in the sibling patch against EDK2, this patch makes it more difficult to > figure out what went wrong when you hit the ASSERT. > ASSERT_EFI_ERROR prints the value of Status, ASSERT(FALSE) only prints > '0 != 1' >
This change(and other similar changes) is in response to review comments on patch v1 https://lists.01.org/pipermail/edk2-devel/2017-October/015995.html with above reference, Can you please confirm if we should revert to the patch v1 version ? > > gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES (*VramSize)); > > return Status; > > } > > @@ -221,6 +225,7 @@ LcdPlatformSetMode ( > > EFI_STATUS Status; > > > > if (ModeNumber >= LcdPlatformGetMaxMode ()) { > > + ASSERT (FALSE); > > These are fine: the code itself explains adequately which condition triggered > the ASSERT to fire. > > > return EFI_INVALID_PARAMETER; > > } > > > > @@ -279,7 +284,10 @@ LcdPlatformQueryMode ( > > OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION * CONST Info > > ) > > { > > + ASSERT (Info != NULL); > > + > > if (ModeNumber >= LcdPlatformGetMaxMode ()) { > > + ASSERT (FALSE); > > return EFI_INVALID_PARAMETER; > > } > > > > @@ -343,7 +351,18 @@ LcdPlatformGetTimings ( > > OUT UINT32 * CONST VFrontPorch > > ) > > { > > + // One of the pointers is NULL > > + ASSERT (HRes != NULL); > > + ASSERT (HSync != NULL); > > + ASSERT (HBackPorch != NULL); > > + ASSERT (HFrontPorch != NULL); > > + ASSERT (VRes != NULL); > > + ASSERT (VSync != NULL); > > + ASSERT (VBackPorch != NULL); > > + ASSERT (VFrontPorch != NULL); > > + > > if (ModeNumber >= LcdPlatformGetMaxMode ()) { > > + ASSERT (FALSE); > > return EFI_INVALID_PARAMETER; > > } > > > > @@ -376,6 +395,7 @@ LcdPlatformGetBpp ( > > ) > > { > > if (ModeNumber >= LcdPlatformGetMaxMode ()) { > > + ASSERT (FALSE); > > return EFI_INVALID_PARAMETER; > > } > > > > diff --git > > > a/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdAr > mV > > Express.c > > > b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdAr > mV > > Express.c index > > > 799fb3fc781ce04bb64cb1fa0b87f262a670ed78..fd4eea8f8e2397bc7d4ddf4cfe > 3d > > cc97a5109edb 100644 > > --- > > > a/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdAr > mV > > Express.c > > +++ > b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111Lcd > > +++ ArmVExpress.c > > @@ -205,6 +205,9 @@ LcdPlatformGetVram ( > > > > Status = EFI_SUCCESS; > > > > + ASSERT (VramBaseAddress != NULL); > > + ASSERT (VramSize != NULL); > > + > > // Is it on the motherboard or on the daughterboard? > > switch (PL111_CLCD_SITE) { > > > > @@ -225,6 +228,7 @@ LcdPlatformGetVram ( > > VramBaseAddress > > ); > > if (EFI_ERROR (Status)) { > > + ASSERT (FALSE); > > return Status; > > } > > > > @@ -235,8 +239,8 @@ LcdPlatformGetVram ( > > *VramSize, > > EFI_MEMORY_WC > > ); > > - ASSERT_EFI_ERROR (Status); > > if (EFI_ERROR (Status)) { > > + ASSERT (FALSE); > > gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES > (*VramSize)); > > return Status; > > } > > @@ -294,6 +298,7 @@ LcdPlatformSetMode ( > > UINT32 SysId; > > > > if (ModeNumber >= LcdPlatformGetMaxMode ()) { > > + ASSERT (FALSE); > > return EFI_INVALID_PARAMETER; > > } > > > > @@ -369,7 +374,10 @@ LcdPlatformQueryMode ( > > OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION * CONST Info > > ) > > { > > + ASSERT (Info != NULL); > > + > > if (ModeNumber >= LcdPlatformGetMaxMode ()) { > > + ASSERT (FALSE); > > return EFI_INVALID_PARAMETER; > > } > > > > @@ -433,7 +441,18 @@ LcdPlatformGetTimings ( > > OUT UINT32 * CONST VFrontPorch > > ) > > { > > + // One of the pointers is NULL > > + ASSERT (HRes != NULL); > > + ASSERT (HSync != NULL); > > + ASSERT (HBackPorch != NULL); > > + ASSERT (HFrontPorch != NULL); > > + ASSERT (VRes != NULL); > > + ASSERT (VSync != NULL); > > + ASSERT (VBackPorch != NULL); > > + ASSERT (VFrontPorch != NULL); > > + > > if (ModeNumber >= LcdPlatformGetMaxMode ()) { > > + ASSERT (FALSE); > > return EFI_INVALID_PARAMETER; > > } > > > > @@ -465,7 +484,10 @@ LcdPlatformGetBpp ( > > OUT LCD_BPP * CONST Bpp > > ) > > { > > + ASSERT (Bpp != NULL); > > + > > if (ModeNumber >= LcdPlatformGetMaxMode ()) { > > + ASSERT (FALSE); > > return EFI_INVALID_PARAMETER; > > } > > > > -- > > Guid("CE165669-3EF3-493F-B85D-6190EE5B9759") > > > > _______________________________________________ > > edk2-devel mailing list > > [email protected] > > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ > edk2-devel mailing list > [email protected] > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
