Re: [PATCH edk2-platforms v2 06/18] ARM/VExpressPkg: Add and update debug ASSERTS

[Girish Pathak <Girish.Pathak@arm.com>]

Hi Ard,

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Ard Biesheuvel
> Sent: 23 December 2017 14:12
> To: Evan Lloyd <Evan.Lloyd@arm.com>
> Cc: "Matteo.Carlini@arm.com"@arm.com;
> "leif.lindholm@linaro.org"@arm.com; "nd@arm.com"@arm.com; edk2-
> devel@lists.01.org; Thomas Abraham <thomas.abraham@arm.com>; Arvind
> Chauhan <Arvind.Chauhan@arm.com>;
> "ard.biesheuvel@linaro.org"@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,  <evan.lloyd@arm.com> 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 <girish.pathak@arm.com>
> > Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
> > ---
> >
> 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
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel