On 1 December 2017 at 16:33, Evan Lloyd <[email protected]> wrote:
> 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.
>
Fair enough. But I'd prefer to finally fix the DEBUG vs NOOPT build
targets for ARM/AARCH64. DEBUG was never intended to be -O0 (and it is
not on X86 either).
Code is complex enough as it is, and how to move trivial tests like
this around for 'performance' reasons is a dimension I would like to
avoid.
>>
>> > 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).
>
As stated above, let's fix DEBUG vs NOOPT instead.
>>
>> > 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
