On Tue, Sep 26, 2017 at 09:15:14PM +0100, [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/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")
>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
