From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=104.47.2.63; helo=eur01-db5-obe.outbound.protection.outlook.com; envelope-from=alexei.fedorov@arm.com; receiver=edk2-devel@lists.01.org Received: from EUR01-DB5-obe.outbound.protection.outlook.com (mail-db5eur01on0063.outbound.protection.outlook.com [104.47.2.63]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 2D7642217CE54 for ; Thu, 7 Dec 2017 06:50:56 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=armh.onmicrosoft.com; s=selector1-arm-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=vX+dHBdl+Rmn/KvST/9XS/EnFD5o6y7eTrtFPjoJwfo=; b=rbejUs9Bn4uwAV3/DECPIxNBe6gEjD00mFyW73Pknh0b3QdqW4Zy8N3mY/6b8TzppVy7jqz2Jm8V8LGhvF5tLb7N2VNKnn1Ci98ml/jCYDlZrxhJsCYSd0UWvK5R/L9yAefBISJncBXAZQMS2HejXdac7EOIXYjKpgxkXbQOLIY= Received: from DB5PR08MB1014.eurprd08.prod.outlook.com (10.166.14.11) by HE1PR0801MB1450.eurprd08.prod.outlook.com (10.167.190.22) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.20.282.5; Thu, 7 Dec 2017 14:55:26 +0000 Received: from DB5PR08MB1014.eurprd08.prod.outlook.com ([fe80::7ca6:8d44:ec42:848d]) by DB5PR08MB1014.eurprd08.prod.outlook.com ([fe80::7ca6:8d44:ec42:848d%13]) with mapi id 15.20.0282.012; Thu, 7 Dec 2017 14:55:26 +0000 From: Alexei Fedorov To: Evan Lloyd , Ard Biesheuvel CC: "edk2-devel@lists.01.org" , "leif.lindholm@linaro.org@arm.com" <"leif.lindholm@linaro.org"@arm.com>, "nd@arm.com@arm.com" <"nd@arm.com"@arm.com>, "ard.biesheuvel@linaro.org@arm.com" <"ard.biesheuvel@linaro.org"@arm.com>, "Matteo.Carlini@arm.com@arm.com" <"Matteo.Carlini@arm.com"@arm.com> Thread-Topic: [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add debug asserts Thread-Index: AQHTbgobZbtsHZjF1UOidimEVhu6BaM31SvD Date: Thu, 7 Dec 2017 14:55:25 +0000 Message-ID: References: <20170926201529.11644-1-evan.lloyd@arm.com> <20170926201529.11644-5-evan.lloyd@arm.com> , In-Reply-To: Accept-Language: en-GB, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=Alexei.Fedorov@arm.com; x-originating-ip: [217.140.96.140] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; HE1PR0801MB1450; 6:3IVtZHDK2wI7oEUDlwpGiJb9yurxF6P5lJwFFMbwtNtk+Ogg2+/Aca51JokcnIdqpebduZvqMxNmVvWbk6+TH3/GjmqQ7H4qTVU8KK53CZ5y7rxAA4szrBy5veUgzL1c3xIpm6KHUabC4unxDm1J0Zn16AMNawGBKF8S0rprOAdcCBe4pDoEsn2xIEBjfw8afhtdUwfZMz/PewgSmCIABj2PbFjh3xbMBRh4kwl45cJQg6xNny9OeqAPkQqkeMYVALI2Y1edYjJDHF/GvyT5z8bD+BAD0ZDlGSUZGUP763SY90iS+ZXjMY6D04Hw4ePmWZQUKeYFc7iYjb4wWFnn18RRaU24S/iXOsOCbKyMia0=; 5:motBE1zrUXI4XVML8eNb4NJQPcamDKHzmmkLGqdrQDyHRyS2ED5Yi5DQB7W4f2zJ9n4LyrJoSN75jBtOk/eLpcMpra+mkMRJLysscC8UL8CV+yIe0jeUojm/VNou+1dIMnDvltMavvf7lXjxJY1XmOzTT/Gfu+8f/OuCd3bgq5k=; 24:y0pxQEmVqpEnS+9KSjTpjQdCNnic/uaKSA+5LXwfPaRFRrFCW4bcAPI0y/5kHyRVk/424yvAqkBb+Qehr+c6KIf+XulbIfb5JqwBEfrjZUc=; 7:UPm0hbQrWoCHmHNq3mJhJqUd03wEnmP9PsdUbBhrgyeULFNvAzqAq0HmsDWvQlKfO6MWBE48awWpektASPef8rTh7u1Lki02ea7QMjCQo0cDDwCFxGc1rVOg1QWk+elikZm+W+FyMlJnZSqlxztV02lUpKuBq5b7ZjbbwyEUVPt7OexB6y/ls1tzM1oulJMp0LCp5Za7ElQIpkygVHQbKqpqULwmh+lK40kER2vgyyCZeb9N9FBG41Ta4qKPyiHp x-ms-exchange-antispam-srfa-diagnostics: SSOS;SSOR; x-ms-office365-filtering-ht: Tenant x-ms-office365-filtering-correlation-id: 16a69e21-3b07-4832-dc18-08d53d828cd3 x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(4534020)(4602075)(4627115)(201703031133081)(201702281549075)(48565401081)(5600026)(4604075)(2017052603286); SRVR:HE1PR0801MB1450; x-ms-traffictypediagnostic: HE1PR0801MB1450: x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(180628864354917)(162533806227266)(17755550239193); x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(6040450)(2401047)(5005006)(8121501046)(3231022)(10201501046)(93006095)(93001095)(3002001)(6055026)(6041248)(20161123560025)(20161123564025)(20161123555025)(20161123562025)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123558100)(6072148)(201708071742011); SRVR:HE1PR0801MB1450; BCL:0; PCL:0; RULEID:(100000803101)(100110400095); SRVR:HE1PR0801MB1450; x-forefront-prvs: 05143A8241 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(39860400002)(376002)(346002)(366004)(13464003)(24454002)(189003)(199004)(40434004)(2906002)(966005)(33656002)(74316002)(101416001)(106356001)(19627405001)(7736002)(105586002)(54896002)(81156014)(3280700002)(6506006)(4326008)(14454004)(606006)(53546010)(2950100002)(3660700001)(5890100001)(5660300001)(55016002)(6436002)(478600001)(93886005)(6606003)(8936002)(97736004)(72206003)(575784001)(316002)(81166006)(66066001)(25786009)(5250100002)(236005)(68736007)(86362001)(3846002)(6116002)(102836003)(9686003)(6306002)(99286004)(53936002)(229853002)(7696005)(76176011)(110136005)(345774005)(2900100001)(6246003)(53946003)(8676002)(579004); DIR:OUT; SFP:1101; SCL:1; SRVR:HE1PR0801MB1450; H:DB5PR08MB1014.eurprd08.prod.outlook.com; FPR:; SPF:None; PTR:InfoNoRecords; MX:1; A:1; LANG:en; received-spf: None (protection.outlook.com: arm.com does not designate permitted sender hosts) spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM MIME-Version: 1.0 X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-Network-Message-Id: 16a69e21-3b07-4832-dc18-08d53d828cd3 X-MS-Exchange-CrossTenant-originalarrivaltime: 07 Dec 2017 14:55:25.9749 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: f34e5979-57d9-4aaa-ad4d-b122a662184d X-MS-Exchange-Transport-CrossTenantHeadersStamped: HE1PR0801MB1450 X-Content-Filtered-By: Mailman/MimeDel 2.1.22 Subject: Re: [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add debug asserts X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 07 Dec 2017 14:50:58 -0000 Content-Language: en-GB Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Hi, I've compiled current HdLcd.c code with different optimisation levels for D= EBUG build using GCC 7.2.1, and the assembler code below was generated for: ASSERT_EFI_ERROR (Status); if (EFI_ERROR( Status )) { return EFI_DEVICE_ERROR; } 1. -O0 (default DEBUG option for AARCH64 before Ard's patch): str x0, [x29, 72] //, Status // r:\edk2\ArmPlatformPkg\Drivers\LcdGraphicsOutputDxe\HdLcd.c:79: ASSERT= _EFI_ERROR (Status); .loc 1 79 0 bl DebugAssertEnabled // and w0, w0, 255 // _1, tmp150 cmp w0, 0 // _1, beq .L4 //, ldr x0, [x29, 72] // Status.9_2, Status cmp x0, 0 // Status.9_2, bge .L4 //, 2. -Os: mov x19, x0 // Status, // r:\edk2\ArmPlatformPkg\Drivers\LcdGraphicsOutputDxe\HdLcd.c:79: ASSERT= _EFI_ERROR (Status); .loc 1 79 0 bl DebugAssertEnabled // .LVL15: tst w0, 255 // beq .L4 //, tbz x19, #63, .L8 // Status, 3. -O3: mov x19, x0 // Status, .LVL14: // r:\edk2\ArmPlatformPkg\Drivers\LcdGraphicsOutputDxe\HdLcd.c:79: ASSERT= _EFI_ERROR (Status); bl DebugAssertEnabled // .LVL15: tst w0, 255 // beq .L5 //, tbnz x19, #63, .L26 // Status, with DebugAssertEnabled() compiled as: DebugAssertEnabled: .LFB4: .loc 1 203 0 .cfi_startproc // r:\edk2\MdePkg\Library\BaseDebugLibSerialPort\DebugLib.c:204: return (= BOOLEAN) ((PcdGet8(PcdDebugPropertyMask) & DEBUG_PROPERTY_DEBUG_ASSERT_ENAB= LED) !=3D 0); .loc 1 204 0 adrp x0, _gPcd_FixedAtBuild_PcdDebugPropertyMask // tmp80, add x0, x0, :lo12:_gPcd_FixedAtBuild_PcdDebugPropertyMask // tmp7= 9, tmp80, ldrb w0, [x0] // _gPcd_FixedAtBuild_PcdDebugPropertyMask.4_1, _gP= cd_FixedAtBuild_PcdDebugPropertyMask and w0, w0, 1 // _3, _2, cmp w0, 0 // _3, cset w0, ne // tmp82, and w0, w0, 255 // _4, tmp81 // r:\edk2\MdePkg\Library\BaseDebugLibSerialPort\DebugLib.c:205: } .loc 1 205 0 ret As you can see each "ASSERT_EFI_ERROR (Status)" macro requires DebugAssertE= nabled() call taking 8 instructions by itself + minimum 3 instructions (for= -Os, -O3) for Status check, which will be performed by "if (EFI_ERROR( Sta= tus ))" anyway. Please correct me if I'm wrong assuming that placing ASSERT_EFI_ERROR (Status) inside if (EFI_ERROR( Status )) { statement is an optimisation improvement. Thank you for your cooperation. Alexei. > >> > - 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 fro= m 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 targe= ts > 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. >[[Evan Lloyd]] I have no objection to the DEBUG build being updated from= -O0. >I strongly suspect that it was set to that because it reduced the incidenc= e of the r18 problem that has now been fixed. >However, I can certainly envisage circumstances when we might need to do a= n emulator run with -O0 used for the build - so the > >reordering would still be beneficial. ________________________________ From: edk2-devel on behalf of Evan Lloyd = Sent: 05 December 2017 20:46 To: Ard Biesheuvel Cc: edk2-devel@lists.01.org; leif.lindholm@linaro.org@arm.com; nd@arm.com@a= rm.com; ard.biesheuvel@linaro.org@arm.com; Matteo.Carlini@arm.com@arm.com Subject: Re: [edk2] [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add= debug asserts > -----Original Message----- > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] > Sent: 01 December 2017 17:35 > To: Evan Lloyd > Cc: edk2-devel@lists.01.org; ard.biesheuvel@linaro.org@arm.com > <"ard.biesheuvel@linaro.org"@arm.com>; > leif.lindholm@linaro.org@arm.com <"leif.lindholm@linaro.org"@arm.com>; > Matteo.Carlini@arm.com@arm.com > <"Matteo.Carlini@arm.com"@arm.com>; nd@arm.com@arm.com > <"nd@arm.com"@arm.com> > Subject: Re: [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add > debug asserts > > On 1 December 2017 at 16:33, Evan Lloyd wrote: > > Responses inline: > > > >> -----Original Message----- > >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] > >> Sent: 13 October 2017 08:33 > >> To: Evan Lloyd > >> Cc: edk2-devel@lists.01.org; "ard.biesheuvel@linaro.org"@arm.com; > >> "leif.lindholm@linaro.org"@arm.com; > >> "Matteo.Carlini@arm.com"@arm.com; "nd@arm.com"@arm.com > >> Subject: Re: [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: > Add > >> debug asserts > >> > >> On 26 September 2017 at 21:15, wrote: > >> > From: Girish Pathak > >> > > >> > 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 > >> > Signed-off-by: Evan Lloyd > >> > --- > >> > > >> > 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 =3D=3D NULL || VramSize =3D=3D NULL) { > >> > + ASSERT (VramBaseAddress !=3D NULL); > >> > + ASSERT (VramSize !=3D NULL); > >> > + return EFI_INVALID_PARAMETER; > >> > + } > >> > + > >> > // Set the vram size > >> > *VramSize =3D 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 fro= m 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 targe= ts > 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. [[Evan Lloyd]] I have no objection to the DEBUG build being updated from -= O0. I strongly suspect that it was set to that because it reduced the incidence= of the r18 problem that has now been fixed. However, I can certainly envisage circumstances when we might need to do an= emulator run with -O0 used for the build - so the reordering would still b= e beneficial. > > >> > >> > gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES > (*VramSize)); > >> > return Status; > >> > } > >> > @@ -215,6 +224,7 @@ LcdPlatformSetMode ( > >> > EFI_STATUS Status; > >> > > >> > if (ModeNumber >=3D LcdPlatformGetMaxMode ()) { > >> > + ASSERT (ModeNumber < LcdPlatformGetMaxMode ()); > >> > return EFI_INVALID_PARAMETER; > >> > } > >> > > >> > @@ -264,6 +274,7 @@ LcdPlatformSetMode ( > >> > * > >> > * @retval EFI_SUCCESS Success if the requested mode i= s > 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 >=3D LcdPlatformGetMaxMode ()) { > >> > + if (ModeNumber >=3D LcdPlatformGetMaxMode () || Info =3D=3D 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 !=3D NULL); > >> > return EFI_INVALID_PARAMETER; > >> > } > >> > > >> > @@ -334,6 +347,28 @@ LcdPlatformGetTimings ( > >> > ) > >> > { > >> > if (ModeNumber >=3D LcdPlatformGetMaxMode ()) { > >> > + ASSERT (ModeNumber < LcdPlatformGetMaxMode ()); > >> > >> same here > > [[Evan Lloyd]] Agreed > >> > >> > + return EFI_INVALID_PARAMETER; > >> > + } > >> > + > >> > + if (HRes =3D=3D NULL > >> > + || HSync =3D=3D NULL > >> > + || HBackPorch =3D=3D NULL > >> > + || HFrontPorch =3D=3D NULL > >> > + || VRes =3D=3D NULL > >> > + || VSync =3D=3D NULL > >> > + || VBackPorch =3D=3D NULL > >> > + || VFrontPorch =3D=3D NULL) > >> > + { > >> > + // One of the pointers is NULL > >> > + ASSERT (HRes !=3D NULL); > >> > + ASSERT (HSync !=3D NULL); > >> > + ASSERT (HBackPorch !=3D NULL); > >> > + ASSERT (HFrontPorch !=3D NULL); > >> > + ASSERT (VRes !=3D NULL); > >> > + ASSERT (VSync !=3D NULL); > >> > + ASSERT (VBackPorch !=3D NULL); > >> > + ASSERT (VFrontPorch !=3D 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 >=3D LcdPlatformGetMaxMode ()) { > >> > + if (ModeNumber >=3D LcdPlatformGetMaxMode () || Bpp =3D=3D NULL) = { > >> > + ASSERT (ModeNumber < LcdPlatformGetMaxMode ()); > >> > + ASSERT (Bpp !=3D 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 =3D EFI_SUCCESS; > >> > > >> > + // Check VramBaseAddress and VramSize are not NULL. > >> > + if (VramBaseAddress =3D=3D NULL || VramSize =3D=3D NULL) { > >> > + ASSERT (VramBaseAddress !=3D NULL); > >> > + ASSERT (VramSize !=3D 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 u= se > 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 >=3D 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 i= s > 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 >=3D LcdPlatformGetMaxMode ()) { > >> > + if (ModeNumber >=3D LcdPlatformGetMaxMode () || Info =3D=3D NULL)= { > >> > + ASSERT (ModeNumber < LcdPlatformGetMaxMode ()); > >> > + ASSERT (Info !=3D NULL); > >> > return EFI_INVALID_PARAMETER; > >> > } > >> > > >> > @@ -415,6 +428,7 @@ LcdPlatformQueryMode ( > >> > * > >> > * @retval EFI_SUCCESS Success if the requested mode i= s > 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 >=3D LcdPlatformGetMaxMode ()) { > >> > + ASSERT (ModeNumber < LcdPlatformGetMaxMode ()); > >> > + return EFI_INVALID_PARAMETER; > >> > + } > >> > + > >> > + if (HRes =3D=3D NULL > >> > + || HSync =3D=3D NULL > >> > + || HBackPorch =3D=3D NULL > >> > + || HFrontPorch =3D=3D NULL > >> > + || VRes =3D=3D NULL > >> > + || VSync =3D=3D NULL > >> > + || VBackPorch =3D=3D NULL > >> > + || VFrontPorch =3D=3D NULL) > >> > + { > >> > + // One of the pointers is NULL > >> > + ASSERT (HRes !=3D NULL); > >> > + ASSERT (HSync !=3D NULL); > >> > + ASSERT (HBackPorch !=3D NULL); > >> > + ASSERT (HFrontPorch !=3D NULL); > >> > + ASSERT (VRes !=3D NULL); > >> > + ASSERT (VSync !=3D NULL); > >> > + ASSERT (VBackPorch !=3D NULL); > >> > + ASSERT (VFrontPorch !=3D 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 >=3D LcdPlatformGetMaxMode ()) { > >> > + ASSERT (ModeNumber < LcdPlatformGetMaxMode ()); > >> > + ASSERT (Bpp !=3D 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 =3D LcdPlatformGetBpp (ModeNumber, &LcdBpp); > >> > - ASSERT_EFI_ERROR (Status); > >> > if (EFI_ERROR (Status)) { > >> > - return EFI_DEVICE_ERROR; > >> > + ASSERT_EFI_ERROR (Status); > >> > + return Status; > >> > } > >> > > >> > BytesPerPixel =3D 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 =3D 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 th= e > information in any medium. Thank you. IMPORTANT NOTICE: The contents of this email and any attachments are confid= ential and may also be privileged. If you are not the intended recipient, p= lease 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 edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel IMPORTANT NOTICE: The contents of this email and any attachments are confid= ential and may also be privileged. If you are not the intended recipient, p= lease 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.