Responses inline:
> -----Original Message-----
> From: Ard Biesheuvel [mailto:[email protected]]
> Sent: 13 October 2017 08:33
> To: Evan Lloyd <[email protected]>
> Cc: [email protected]; "[email protected]"@arm.com;
> "[email protected]"@arm.com;
> "[email protected]"@arm.com; "[email protected]"@arm.com
> Subject: Re: [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add
> debug asserts
>
> On 26 September 2017 at 21:15, <[email protected]> wrote:
> > From: Girish Pathak <[email protected]>
> >
> > This change adds some debug assertions e.g to catch NULL pointer
> > errors missing in PL11Lcd and HdLcd modules.
> >
> > This change also improves related error handling code.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Girish Pathak <[email protected]>
> > Signed-off-by: Evan Lloyd <[email protected]>
> > ---
> >
> ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdAr
> mVExpress.c | 44 ++++++++++++++++++--
> >
> ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL11
> 1LcdArmVExpress.c | 43 ++++++++++++++++++-
> > ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> | 8 ++--
> > ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> | 8 ++--
> > 4 files changed, 90 insertions(+), 13 deletions(-)
> >
> > diff --git
> >
> a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> ArmVE
> > xpress.c
> >
> b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> ArmVE
> > xpress.c index
> >
> b9859a56988f7e5be0adbaa49048a683fe586bfe..58dd9f0c77e1bc9af559a
> 71d0c7c
> > ce72d71c6da5 100644
> > ---
> >
> a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> ArmVE
> > xpress.c
> > +++
> b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> A
> > +++ rmVExpress.c
> > @@ -140,6 +140,7 @@ LcdPlatformInitializeDisplay (
> > * buffer in bytes
> > *
> > * @retval EFI_SUCCESS Frame buffer memory allocation success.
> > + * @retval EFI_INVALID_PARAMETER VramBaseAddress or VramSize
> are NULL.
> > * @retval !(EFI_SUCCESS) Other errors.
> > **/
> > EFI_STATUS
> > @@ -151,6 +152,13 @@ LcdPlatformGetVram (
> > EFI_STATUS Status;
> > EFI_ALLOCATE_TYPE AllocationType;
> >
> > + // Check VramBaseAddress and VramSize are not NULL.
> > + if (VramBaseAddress == NULL || VramSize == NULL) {
> > + ASSERT (VramBaseAddress != NULL);
> > + ASSERT (VramSize != NULL);
> > + return EFI_INVALID_PARAMETER;
> > + }
> > +
> > // Set the vram size
> > *VramSize = LCD_VRAM_SIZE;
> >
> > @@ -169,6 +177,7 @@ LcdPlatformGetVram (
> > VramBaseAddress
> > );
> > if (EFI_ERROR (Status)) {
> > + ASSERT_EFI_ERROR (Status);
> > return Status;
> > }
> >
> > @@ -179,8 +188,8 @@ LcdPlatformGetVram (
> > *VramSize,
> > EFI_MEMORY_WC
> > );
> > - ASSERT_EFI_ERROR (Status);
> > if (EFI_ERROR (Status)) {
> > + ASSERT_EFI_ERROR (Status);
>
> What is the point of this change?
[[Evan Lloyd]] It is a minor efficiency improvement. Since the ASSERT can only
fire when the if condition is true, it removes a duplicated test from the main
(non-error) code flow. This is irrelevant on hardware, but actually
significant when running debug builds on an emulator environment.
>
> > gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES (*VramSize));
> > return Status;
> > }
> > @@ -215,6 +224,7 @@ LcdPlatformSetMode (
> > EFI_STATUS Status;
> >
> > if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> > + ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> > return EFI_INVALID_PARAMETER;
> > }
> >
> > @@ -264,6 +274,7 @@ LcdPlatformSetMode (
> > *
> > * @retval EFI_SUCCESS Success if the requested mode is found.
> > * @retval EFI_INVALID_PARAMETER Requested mode not found.
> > + * @retval EFI_INVALID_PARAMETER Info is NULL.
> > **/
> > EFI_STATUS
> > LcdPlatformQueryMode (
> > @@ -271,7 +282,9 @@ LcdPlatformQueryMode (
> > OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION * CONST Info
> > )
> > {
> > - if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> > + if (ModeNumber >= LcdPlatformGetMaxMode () || Info == NULL) {
> > + ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
>
> Please don't put anything that may have a side effect inside ASSERT (), since
> they are dropped from RELEASE builds. You can just use ASSERT (FALSE)
> here, or use a temp variable.
[[Evan Lloyd]] Agreed
>
>
> > + ASSERT (Info != NULL);
> > return EFI_INVALID_PARAMETER;
> > }
> >
> > @@ -334,6 +347,28 @@ LcdPlatformGetTimings (
> > )
> > {
> > if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> > + ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
>
> same here
[[Evan Lloyd]] Agreed
>
> > + return EFI_INVALID_PARAMETER;
> > + }
> > +
> > + if (HRes == NULL
> > + || HSync == NULL
> > + || HBackPorch == NULL
> > + || HFrontPorch == NULL
> > + || VRes == NULL
> > + || VSync == NULL
> > + || VBackPorch == NULL
> > + || VFrontPorch == NULL)
> > + {
> > + // 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);
> > return EFI_INVALID_PARAMETER;
> > }
> >
> > @@ -356,6 +391,7 @@ LcdPlatformGetTimings (
> > *
> > * @retval EFI_SUCCESS The requested mode is found.
> > * @retval EFI_INVALID_PARAMETER Requested mode not found.
> > + * @retval EFI_INVALID_PARAMETER Bpp is NULL.
> > **/
> > EFI_STATUS
> > LcdPlatformGetBpp (
> > @@ -363,7 +399,9 @@ LcdPlatformGetBpp (
> > OUT LCD_BPP * CONST Bpp
> > )
> > {
> > - if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> > + if (ModeNumber >= LcdPlatformGetMaxMode () || Bpp == NULL) {
> > + ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> > + ASSERT (Bpp != NULL);
> > return EFI_INVALID_PARAMETER;
> > }
> >
> > diff --git
> >
> a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL1
> 11Lc
> > dArmVExpress.c
> >
> b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL1
> 11Lc
> > dArmVExpress.c index
> >
> 6ae13f06d8b396ea1c67f0bcd735a9d70f476400..5a4abd4c6f9e84d3d690a
> f7233c1
> > cebfe1ad339b 100644
> > ---
> >
> a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL1
> 11Lc
> > dArmVExpress.c
> > +++
> b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL1
> > +++ 11LcdArmVExpress.c
> > @@ -191,6 +191,7 @@ LcdPlatformInitializeDisplay (
> > * buffer in bytes
> > *
> > * @retval EFI_SUCCESS Frame buffer memory allocation success.
> > + * @retval EFI_INVALID_PARAMETER VramBaseAddress or VramSize is
> NULL.
> > * @retval !(EFI_SUCCESS) Other errors.
> > **/
> > EFI_STATUS
> > @@ -203,6 +204,13 @@ LcdPlatformGetVram (
> >
> > Status = EFI_SUCCESS;
> >
> > + // Check VramBaseAddress and VramSize are not NULL.
> > + if (VramBaseAddress == NULL || VramSize == NULL) {
> > + ASSERT (VramBaseAddress != NULL);
> > + ASSERT (VramSize != NULL);
> > + return EFI_INVALID_PARAMETER;
> > + }
> > +
> > // Is it on the motherboard or on the daughterboard?
> > switch (PL111_CLCD_SITE) {
> >
> > @@ -223,6 +231,7 @@ LcdPlatformGetVram (
> > VramBaseAddress
> > );
> > if (EFI_ERROR (Status)) {
> > + ASSERT_EFI_ERROR (Status);
> > return Status;
> > }
> >
> > @@ -233,8 +242,8 @@ LcdPlatformGetVram (
> > *VramSize,
> > EFI_MEMORY_WC
> > );
> > - ASSERT_EFI_ERROR (Status);
> > if (EFI_ERROR (Status)) {
> > + ASSERT_EFI_ERROR (Status);
>
> Please drop this change
[[Evan Lloyd]] As stated above, this is not inconsequential for some use
cases. The ASSERT can only fire when the if is satisfied, so moving the ASSERT
reduces the number of tests executed in normal execution (depending on
optimisation level).
>
> > gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES
> (*VramSize));
> > return Status;
> > }
> > @@ -293,6 +302,7 @@ LcdPlatformSetMode (
> > UINT32 SysId;
> >
> > if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> > + ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
>
> No function calls inside ASSERT () please
[[Evan Lloyd]] Agreed
>
> > return EFI_INVALID_PARAMETER;
> > }
> >
> > @@ -359,6 +369,7 @@ LcdPlatformSetMode (
> > * (on success).
> > *
> > * @retval EFI_SUCCESS Success if the requested mode is found.
> > + * @retval EFI_INVALID_PARAMETER Info is NULL.
> > * @retval EFI_INVALID_PARAMETER Requested mode not found.
> > **/
> > EFI_STATUS
> > @@ -367,7 +378,9 @@ LcdPlatformQueryMode (
> > OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION * CONST Info
> > )
> > {
> > - if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> > + if (ModeNumber >= LcdPlatformGetMaxMode () || Info == NULL) {
> > + ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> > + ASSERT (Info != NULL);
> > return EFI_INVALID_PARAMETER;
> > }
> >
> > @@ -415,6 +428,7 @@ LcdPlatformQueryMode (
> > *
> > * @retval EFI_SUCCESS Success if the requested mode is found.
> > * @retval EFI_INVALID_PARAMETER Requested mode not found.
> > + * @retval EFI_INVALID_PARAMETER One of the OUT parameters is
> NULL.
> > **/
> > EFI_STATUS
> > LcdPlatformGetTimings (
> > @@ -430,6 +444,28 @@ LcdPlatformGetTimings (
> > )
> > {
> > if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> > + ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> > + return EFI_INVALID_PARAMETER;
> > + }
> > +
> > + if (HRes == NULL
> > + || HSync == NULL
> > + || HBackPorch == NULL
> > + || HFrontPorch == NULL
> > + || VRes == NULL
> > + || VSync == NULL
> > + || VBackPorch == NULL
> > + || VFrontPorch == NULL)
> > + {
> > + // 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);
> > return EFI_INVALID_PARAMETER;
> > }
> >
> > @@ -452,6 +488,7 @@ LcdPlatformGetTimings (
> > *
> > * @retval EFI_SUCCESS The requested mode is found.
> > * @retval EFI_INVALID_PARAMETER Requested mode not found.
> > + * @retval EFI_INVALID_PARAMETER Bpp is NULL.
> > **/
> > EFI_STATUS
> > LcdPlatformGetBpp (
> > @@ -460,6 +497,8 @@ LcdPlatformGetBpp (
> > )
> > {
> > if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> > + ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> > + ASSERT (Bpp != NULL);
> > return EFI_INVALID_PARAMETER;
> > }
> >
> > diff --git a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> > b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> > index
> >
> 5f950579720fb69e0a481f697a5cc4038158b409..a266671a26f01d31e8dd
> b0cf7cbf
> > e59d2f4dc49c 100644
> > --- a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> > +++ b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> > @@ -109,15 +109,15 @@ LcdSetMode (
> > &VBackPorch,
> > &VFrontPorch
> > );
> > - ASSERT_EFI_ERROR (Status);
> > if (EFI_ERROR (Status)) {
> > - return EFI_DEVICE_ERROR;
> > + ASSERT_EFI_ERROR (Status);
> > + return Status;
> > }
> >
> > Status = LcdPlatformGetBpp (ModeNumber, &LcdBpp);
> > - ASSERT_EFI_ERROR (Status);
> > if (EFI_ERROR (Status)) {
> > - return EFI_DEVICE_ERROR;
> > + ASSERT_EFI_ERROR (Status);
> > + return Status;
> > }
> >
> > BytesPerPixel = GetBytesPerPixel (LcdBpp); diff --git
> > a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> > b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> > index
> >
> 386e6140a69b045f77ee7fa60c4587d8bf4e7d54..f432c8d802614e8a0e4dd
> ab3898f
> > 6e0dbf9a1572 100644
> > --- a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> > +++ b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> > @@ -110,15 +110,15 @@ LcdSetMode (
> > &VBackPorch,
> > &VFrontPorch
> > );
> > - ASSERT_EFI_ERROR (Status);
> > if (EFI_ERROR (Status)) {
> > - return EFI_DEVICE_ERROR;
> > + ASSERT_EFI_ERROR (Status);
> > + return Status;
> > }
> >
> > Status = LcdPlatformGetBpp (ModeNumber, &LcdBpp);
> > - ASSERT_EFI_ERROR (Status);
> > if (EFI_ERROR (Status)) {
> > - return EFI_DEVICE_ERROR;
> > + ASSERT_EFI_ERROR (Status);
> > + return Status;
> > }
> >
> > // Disable the CLCD_LcdEn bit
> > --
> > Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
> >
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
