From: Leif Lindholm <leif.lindholm@linaro.org>
To: evan.lloyd@arm.com
Cc: edk2-devel@lists.01.org
Subject: Re: [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add debug asserts
Date: Thu, 12 Oct 2017 20:32:55 +0100 [thread overview]
Message-ID: <20171012193255.kx5w4axzempq66s4@bivouac.eciton.net> (raw)
In-Reply-To: <20170926201529.11644-5-evan.lloyd@arm.com>
On Tue, Sep 26, 2017 at 09:15:14PM +0100, evan.lloyd@arm.com wrote:
> From: Girish Pathak <girish.pathak@arm.com>
>
> 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 <girish.pathak@arm.com>
> Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
> ---
> ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c | 44 ++++++++++++++++++--
> ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.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/HdLcdArmVExpress.c b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
> index b9859a56988f7e5be0adbaa49048a683fe586bfe..58dd9f0c77e1bc9af559a71d0c7cce72d71c6da5 100644
> --- a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
> +++ b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.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;
> + }
> +
The above is not just a debug assertion change, it also changes
behaviour for RELEASE builds.
I have no objection to these changes in the same patch, but it makes
the commit message misleading. Can you update that please, to say
"adds debug assertions and runtime checks"?
> // 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);
> gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES (*VramSize));
> return Status;
> }
> @@ -215,6 +224,7 @@ LcdPlatformSetMode (
> EFI_STATUS Status;
>
> if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> + ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
Is this asserting that LcdPlatformGetMaxMode() returns the same thing
if called twice?
If you just always want to assert when reached, use ASSERT (FALSE).
If you explicitly want the message to tell you which operation went
wrong, please use a temporary variable rather than calling a function twice.
> 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 ());
> + ASSERT (Info != NULL);
> return EFI_INVALID_PARAMETER;
> }
>
> @@ -334,6 +347,28 @@ LcdPlatformGetTimings (
> )
> {
> if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> + ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
ASSERT (FALSE)? Or temporary variable.
> + 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/PL111LcdArmVExpress.c b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
> index 6ae13f06d8b396ea1c67f0bcd735a9d70f476400..5a4abd4c6f9e84d3d690af7233c1cebfe1ad339b 100644
> --- a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
> +++ b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.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);
ASSERT (FALSE)? (You already know Status is an EFI_ERROR, and a
console message saying ASSERT (Status) is not getting you out of
looking at the source code to find out what happened.)
> return Status;
> }
>
> @@ -233,8 +242,8 @@ LcdPlatformGetVram (
> *VramSize,
> EFI_MEMORY_WC
> );
> - ASSERT_EFI_ERROR (Status);
> if (EFI_ERROR (Status)) {
> + ASSERT_EFI_ERROR (Status);
ASSERT (FALSE)?
> gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES (*VramSize));
> return Status;
> }
> @@ -293,6 +302,7 @@ LcdPlatformSetMode (
> UINT32 SysId;
>
> if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> + ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
ASSERT (FALSE)? Or temporary variable?
> 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 ());
Temporary variable?
> + 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..a266671a26f01d31e8ddb0cf7cbfe59d2f4dc49c 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);
ASSERT (FALSE).
> + return Status;
This is a change in behaviour, since the function can now potentially
return other values than it has in the past. Again, it may be fine,
but deserves a mention in the commit message.
> }
>
> Status = LcdPlatformGetBpp (ModeNumber, &LcdBpp);
> - ASSERT_EFI_ERROR (Status);
> if (EFI_ERROR (Status)) {
> - return EFI_DEVICE_ERROR;
> + ASSERT_EFI_ERROR (Status);
> + return Status;
Same as above.
> }
>
> BytesPerPixel = GetBytesPerPixel (LcdBpp);
> diff --git a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> index 386e6140a69b045f77ee7fa60c4587d8bf4e7d54..f432c8d802614e8a0e4ddab3898f6e0dbf9a1572 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;
Same as above.
> }
>
> Status = LcdPlatformGetBpp (ModeNumber, &LcdBpp);
> - ASSERT_EFI_ERROR (Status);
> if (EFI_ERROR (Status)) {
> - return EFI_DEVICE_ERROR;
> + ASSERT_EFI_ERROR (Status);
> + return Status;
Same as above.
/
Leif
> }
>
> // Disable the CLCD_LcdEn bit
> --
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
>
next prev parent reply other threads:[~2017-10-12 19:29 UTC|newest]
Thread overview: 69+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-26 20:15 [PATCH 00/19] ArmPlatformPkg: Update GOP evan.lloyd
2017-09-26 20:15 ` [PATCH 01/19] ArmPlatformPkg: Tidy LcdGraphicsOutputDxe code: Coding standard evan.lloyd
2017-10-12 18:45 ` Leif Lindholm
2017-09-26 20:15 ` [PATCH 02/19] ArmPlatformPkg: Tidy LcdGraphicsOutputDxe code: Added comments evan.lloyd
2017-10-12 19:02 ` Leif Lindholm
2017-12-05 18:55 ` Evan Lloyd
2017-12-05 19:58 ` Leif Lindholm
2017-12-05 22:06 ` Evan Lloyd
2017-09-26 20:15 ` [PATCH 03/19] ArmPlatformPkg: PL111 and HDLCD: add const qualifier evan.lloyd
2017-10-12 19:07 ` Leif Lindholm
2017-10-12 19:47 ` Ard Biesheuvel
2017-12-01 16:17 ` Evan Lloyd
2017-12-01 17:31 ` Ard Biesheuvel
2017-12-05 20:35 ` Evan Lloyd
2017-12-05 20:54 ` Ard Biesheuvel
2017-09-26 20:15 ` [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add debug asserts evan.lloyd
2017-10-12 19:32 ` Leif Lindholm [this message]
2017-10-13 7:33 ` Ard Biesheuvel
2017-12-01 16:33 ` Evan Lloyd
2017-12-01 17:34 ` Ard Biesheuvel
2017-12-01 17:58 ` Leif Lindholm
2017-12-05 20:46 ` Evan Lloyd
2017-12-07 14:55 ` Alexei Fedorov
2017-12-07 15:10 ` Ard Biesheuvel
2017-12-07 16:53 ` Alexei Fedorov
2017-12-08 21:39 ` Ard Biesheuvel
2017-09-26 20:15 ` [PATCH 05/19] ArmPlatformPkg: PL111LcdArmVExpressLib: Minor code cleanup evan.lloyd
2017-10-12 19:33 ` Leif Lindholm
2017-09-26 20:15 ` [PATCH 06/19] ArmPlatformPkg: PL111Lcd: Replace magic number with macro evan.lloyd
2017-10-12 19:34 ` Leif Lindholm
2017-09-26 20:15 ` [PATCH 07/19] ArmPlatformPkg: PL111LcdArmVExpressLib: Use FixedPcdGet32 evan.lloyd
2017-10-12 19:35 ` Leif Lindholm
2017-09-26 20:15 ` [PATCH 08/19] ArmPlatformPkg: PL11LcdArmVExpressLib: Improvement conditional evan.lloyd
2017-10-12 19:36 ` Leif Lindholm
2017-09-26 20:15 ` [PATCH 09/19] ArmPlatformPkg: HdLcdArmVExpressLib: Use FixedPcdGet32 evan.lloyd
2017-10-12 19:38 ` Leif Lindholm
2017-09-26 20:15 ` [PATCH 10/19] ArmPlatformPkg: HdLcdArmVExpressLib: Remove status check EFI_TIMEOUT evan.lloyd
2017-10-12 19:40 ` Leif Lindholm
2017-09-26 20:15 ` [PATCH 11/19] ArmPlatformPkg: Implement LcdIdentify function for HDLCD GOP evan.lloyd
2017-10-12 19:43 ` Leif Lindholm
2017-09-26 20:15 ` [PATCH 12/19] ArmPlatformPkg: Redefine LcdPlatformGetTimings function evan.lloyd
2017-10-13 7:49 ` Leif Lindholm
2017-09-26 20:15 ` [PATCH 13/19] ArmPlatformPkg: HdLcd Remove redundant Bpp evan.lloyd
2017-10-13 7:53 ` Leif Lindholm
2017-10-17 14:32 ` Evan Lloyd
2017-10-17 15:40 ` Leif Lindholm
2017-09-26 20:15 ` [PATCH 14/19] ArmPlatformPkg: Add PCD to select pixel format evan.lloyd
2017-10-25 14:27 ` Leif Lindholm
2017-09-26 20:15 ` [PATCH 15/19] ArmPlatformPkg: PCD to swap red/blue format for HDLCD evan.lloyd
2017-10-25 14:33 ` Leif Lindholm
2017-09-26 20:15 ` [PATCH 16/19] ArmPlatformPkg: Reorganize Lcd Graphics Output evan.lloyd
2017-10-25 14:44 ` Leif Lindholm
2017-09-26 20:15 ` [PATCH 17/19] ArmPlatformPkg: Additional display modes evan.lloyd
2017-10-25 14:45 ` Leif Lindholm
2017-09-26 20:15 ` [PATCH 18/19] ArmPlatformPkg: Reserving framebuffer at build evan.lloyd
2017-10-25 14:51 ` Leif Lindholm
2017-10-25 18:10 ` Ard Biesheuvel
2017-12-01 16:56 ` Evan Lloyd
2017-12-01 17:38 ` Ard Biesheuvel
2017-09-26 20:15 ` [PATCH 19/19] ArmPlatformPkg: New DP500/DP550/DP650 GOP driver evan.lloyd
2017-10-25 15:31 ` Leif Lindholm
2017-11-28 18:17 ` Ard Biesheuvel
2017-12-01 13:12 ` Evan Lloyd
2017-12-01 17:18 ` Ard Biesheuvel
2017-12-05 20:03 ` Evan Lloyd
2017-12-05 21:27 ` Ard Biesheuvel
2017-12-07 20:21 ` Evan Lloyd
2017-12-07 21:10 ` Ard Biesheuvel
2017-12-01 17:29 ` Leif Lindholm
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20171012193255.kx5w4axzempq66s4@bivouac.eciton.net \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox