From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM11-DM6-obe.outbound.protection.outlook.com (NAM11-DM6-obe.outbound.protection.outlook.com [40.107.223.67]) by mx.groups.io with SMTP id smtpd.web12.14571.1588352178757270718 for ; Fri, 01 May 2020 09:56:19 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@vmware.com header.s=selector2 header.b=poySoQSf; spf=pass (domain: vmware.com, ip: 40.107.223.67, mailfrom: awarkentin@vmware.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=V6lR4E27WtSujU6d9z3h/YuU5Ytet6SARCHTXO3IH03LUATa/Texb+PazHfmgDqck6zguO3D9rRg/lyCG1SbJkdZCoFuLStyyL4KBqpfeQHjuIclHkwLZB89EWrVGSJhp/AWiZK9vJOaBTJjSAZDzGD0o3XV5S42+7yGQeXU/6HqBZFiiew+7vEnSQMnm/0KjCOsvNZywNCZSxDkjl1221PqcyV1RK5eI5gJtQbrkZfhStQnfXK86uipPR+dgOC9NXSC863NMYT+Tzi4pQr6cAnA2PzhEIPOMrH8skMKPkKfnH+IcAdfXjFqzApLs7lUPbKtOYQwmkaI2MJc6yPdjw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=jYdS8oMlQgfp0/dRRAEPb+KPUsLji1dcR2bv0AsmThw=; b=a9U9iRev4QQf0KdQG+8lnsgA9wjWTLIWG+783dajm7aJZbrzAGMcwHe/FY4mAeS4tkXVYBC9YBF53LhELosqSwmq1ZZ9ei6AWFwS39N+6M9nByP/bTCrgmefLv7r+neVp1rWAd5kFC70tb6Z6OMmkD4/LMF8AP4VmvvA524AKPGs3CwEeUIvpzz0LofnAo/UXwww2lAQwT5w1WZlpaE5VW9/eGdDz5aZyp+RbkKCDWW3PsKCnNT9xcpjZpGC9TjWya6WWNzuSQNSaQFqSYs+sU/36TP+Sc8dPq3GhlQTuTif4sH/bIJnTVIFTJWw/mhNwj+P9+UtvLZYdIheXEUlIg== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=vmware.com; dmarc=pass action=none header.from=vmware.com; dkim=pass header.d=vmware.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=vmware.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=jYdS8oMlQgfp0/dRRAEPb+KPUsLji1dcR2bv0AsmThw=; b=poySoQSfllUMfOjvzwyB30ac56gJqjiM/SWv6ewlk8iTvqhnHjMulrUj6pp5+4u5mZH1avBYjrLH4z69oPxdVyYpz4SGS5teaDvWTJ1sbl0cEVs8V8PjH9cZdi7mwUzPdnrOkpxDpKQkFAalzA8f8pqGCdgaMbDbC/sohV7NMmE= Received: from BN6PR05MB3411.namprd05.prod.outlook.com (2603:10b6:405:43::23) by BN6PR05MB3235.namprd05.prod.outlook.com (2603:10b6:404:bb::22) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2979.16; Fri, 1 May 2020 16:56:14 +0000 Received: from BN6PR05MB3411.namprd05.prod.outlook.com ([fe80::f463:db64:43d8:5a0f]) by BN6PR05MB3411.namprd05.prod.outlook.com ([fe80::f463:db64:43d8:5a0f%3]) with mapi id 15.20.2979.017; Fri, 1 May 2020 16:56:14 +0000 From: "Andrei Warkentin" To: Ard Biesheuvel , Andrei Warkentin , "devel@edk2.groups.io" , "pete@akeo.ie" CC: "leif@nuviainc.com" , "philmd@redhat.com" Subject: Re: [edk2-devel] [edk2-platforms][PATCH 1/2] RPi: rip out FdtDxe logic to use internal DTB Thread-Topic: [edk2-devel] [edk2-platforms][PATCH 1/2] RPi: rip out FdtDxe logic to use internal DTB Thread-Index: AQHWHzSfInOzpwsV50WMnyNC6daDbKiTN/IAgAAfNwCAAAB6AIAAAOiAgAAbWLM= Date: Fri, 1 May 2020 16:56:13 +0000 Message-ID: References: <20200430211617.120926-1-andrey.warkentin@gmail.com> <20200430211617.120926-2-andrey.warkentin@gmail.com> <77f961d0-55a2-4e85-f8e6-478c6499cd21@arm.com> <4538b8ec-dc41-ec16-2ef5-011be3e90849@akeo.ie> <621362ba-ab8d-9acf-5a02-74b9e7ca80af@arm.com>, In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: arm.com; dkim=none (message not signed) header.d=none;arm.com; dmarc=none action=none header.from=vmware.com; x-originating-ip: [98.214.99.181] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 653e630f-11ae-4a47-9d1c-08d7edf08ed0 x-ms-traffictypediagnostic: BN6PR05MB3235: x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:7219; x-forefront-prvs: 0390DB4BDA x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: 7bi5d0xcV8ZqHHMl1s8Rpgh2tdR5eIMRYeyuRw5tornJ/pK7D91M729E0N+pRFIqYYXnePVTbAnXXTEdgPrSzJkYSs4yckEtCGd3FV13qavS2J7UxpYSE5GY/hRZlKnL4yjy65zWDUlkMEyo2MGBTFC9kJmGcXMeqRqUWh662jRwZnPgTyWOKMYvjgGgpoHXeTYUIEzkG03qYIfKLRExMx5EjVs/5qek8vLmhmPtjV7QmdMrdWvu82QABGtXtQ0QW2AO/urvbIP6e+OD5yFk2KYLTKCXiHM/nXSM0wO+Fig2eu3A+nwesTgnzyNvkk0hVO8TXriEPAQxzzm5J2uaEij/EuZyuJ72NRBnjJ/ynjiEQavWvE3RmOrL+ux7PqsGCulZuz/zyBzczWz2qWrgEfkq+zBayKgQm0r4pCst1ie8lE3fQA/X4QUbGzKT5CArT5mYP3WDQYhQZyUH5DunJy8fdldTgAjYIE0skRKDpbHqroNPseopCIWKT9PtIBxefgenkTMXI8JLxtE6O5OtiA== x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:BN6PR05MB3411.namprd05.prod.outlook.com;PTR:;CAT:NONE;SFTY:;SFS:(4636009)(396003)(376002)(346002)(366004)(136003)(39860400002)(53546011)(8936002)(8676002)(6506007)(55016002)(186003)(2906002)(9686003)(26005)(316002)(19627405001)(33656002)(110136005)(7696005)(86362001)(54906003)(4326008)(478600001)(45080400002)(5660300002)(52536014)(66946007)(71200400001)(66476007)(966005)(64756008)(66556008)(76116006)(66446008);DIR:OUT;SFP:1101; x-ms-exchange-antispam-messagedata: MYBouDRCxy8BOArJ/s7kKtZdroxVXFfcMR1mVot4XNmy/LYcDyx39vMuy2ffsVKAF58EUUi6PprmhMukQuXLRak00aDraC7+x6lJWQu6KjeI8Y600FAQmKMZ3gcRg6Gi6IXNtr+cIocY2yUYOd+MgjZOSB6gaZy6qtnYiIaGH4NVS66cRc1V45KjRFdbIHVb7VCyWTiz6DX3H158RVtC1oYc1/gvW0zI9D6JXcl+4ckK63e0ulzl601fu8E39k3UKiVZgIk0UiwdQK73dxvc6dRBxAqZokBrjrHgLT97lSYqp2ghVIwvssjqzccCWBxmeIe/PTrpl0uNVlXY+VTj2BIbkJE0PgBatVuPrZRq7XbrkFcZg6DLsuQiLq9YXEDFDNuRE/XLNgyzIUoegxsV5U8fJ+bZ+kXa5iZMwoCTixpdZmNAFkpM8KHKcwPvEhiMlIDGEuNvtzJSoGol3a8ZvQzrk7sWenU5mEwcy1Gfe2j/HUUI2VWF3Y9taBMSMeEcBSHbGfm9nWm1nu5PEz3AD4wMExVXeRRz1Ri5rrYsdyyVUKJsE+WhMN79C0FLLBMvJQPMk/yQ7/Bny82/ZcPsE2V+1tccc6Ikpe4RhAioeolwep0+RnkgOoJt6jgL5c5Yqpv+Mji910U/7zel/PTaXqcNs44iBEl3pEApYDG3qjiEVxBvzpNxt1+cWsj06/kjB/yKdPVFvyucPMzI007ez7Mwnotfr74nU7qdiKSUlQElOl5rpGX7D9j6R2Fpp2PansI7cK8C1pVNXo+M15kwj9nYm2qMwGv8Q7R0U0oFzK0= x-ms-exchange-transport-forked: True MIME-Version: 1.0 X-OriginatorOrg: vmware.com X-MS-Exchange-CrossTenant-Network-Message-Id: 653e630f-11ae-4a47-9d1c-08d7edf08ed0 X-MS-Exchange-CrossTenant-originalarrivaltime: 01 May 2020 16:56:13.9765 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: b39138ca-3cee-4b4a-a4d6-cd83d9dd62f0 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: HhMrZM3MbAyDv0O1xlmqNc023em7723Md+xJiHyo1HJkWFY9HEZl40uVM2dlvdAay2fu2+iOWdfh+eRUQO/OOA== X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR05MB3235 Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_BN6PR05MB34110B58136D06E18E7AED7AB9AB0BN6PR05MB3411namp_" --_000_BN6PR05MB34110B58136D06E18E7AED7AB9AB0BN6PR05MB3411namp_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi folks, I added that specific line more as a safeguard in case the depex ever chan= ges, as I wasn't comfortable with code that relied on an external factor to= have defined behavior. I don't have a strong position on it. If you want it to go, I'll send an u= pdate. A ________________________________ From: devel@edk2.groups.io on behalf of Pete Batard= via groups.io Sent: Friday, May 1, 2020 10:16 AM To: Ard Biesheuvel ; Andrei Warkentin ; devel@edk2.groups.io Cc: leif@nuviainc.com ; philmd@redhat.com Subject: Re: [edk2-devel] [edk2-platforms][PATCH 1/2] RPi: rip out FdtDxe = logic to use internal DTB On 2020.05.01 16:13, Ard Biesheuvel wrote: > On 5/1/20 5:11 PM, Pete Batard wrote: >> On 2020.05.01 14:19, Ard Biesheuvel wrote: >>> On 4/30/20 11:16 PM, Andrei Warkentin wrote: >>>> Initially, FdtDxe used an internal (embedded in UEFI) FDT, >>>> because it was neither understood how to consume the >>>> one loaded by the VideoCore firmware, nor understood just >>>> how important it is to use the DTB provided by config.txt. >>>> >>>> Embedding the DT was a bad idea, because: >>>> - Permanently stale >>>> - No overlays >>>> >>>> Also, on devices like the Pi 4 you _have_ to have a DT >>>> around for the start4 VPU firmware to pick up, otherwise >>>> the board is left in an inconsistent state. So we're being >>>> proscriptive now about DT use with config.txt, which means >>>> this internal DT logic is deadc code. >>>> >>>> Further FdtDxe cleanups are possible and will be handled >>>> separately, specifically: >>>> - probably no need to use a separate allocation for patched DT >>>> (optimize memory used) >>>> - suspicious use of EfiBootServicesData (I filed >>>> https://nam04.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fg= ithub.com%2FARM-software%2Febbr%2Fissues%2F45&data=3D02%7C01%7Cawarkent= in%40vmware.com%7Ccd65ce9968f54b569f8208d7ede2a025%7Cb39138ca3cee4b4aa4d6cd= 83d9dd62f0%7C0%7C0%7C637239429916614640&sdata=3DpJ3Wu%2BGJhj%2FqfbBEFMQ= 9nQ%2B1Pgc%2Bo4Xw0fer2h9ZYvQ%3D&reserved=3D0 to sort out the real >>>> requirements) >>>> >>>> Testing: Booted Ubuntu 18.04 on Pi 2B (1.2). >>>> >>>> Signed-off-by: Andrei Warkentin >>>> --- >>>> Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c | 206 >>>> ++++---------------- >>>> Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf | 4 - >>>> Platform/RaspberryPi/RPi3/RPi3.fdf | 11 -- >>>> Platform/RaspberryPi/RPi4/RPi4.fdf | 8 - >>>> Platform/RaspberryPi/RaspberryPi.dec | 7 - >>>> 5 files changed, 38 insertions(+), 198 deletions(-) >>>> >>>> diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c >>>> b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c >>>> index e7143f57..187b9566 100644 >>>> --- a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c >>>> +++ b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c >>>> @@ -335,90 +335,6 @@ CleanSimpleFramebuffer ( >>>> return EFI_SUCCESS; >>>> } >>>> -#define MAX_CMDLINE_SIZE 512 >>>> - >>>> -STATIC >>>> -EFI_STATUS >>>> -UpdateBootArgs ( >>>> - VOID >>>> - ) >>>> -{ >>>> - INTN Node; >>>> - INTN Retval; >>>> - EFI_STATUS Status; >>>> - CHAR8 *CommandLine; >>>> - >>>> - // >>>> - // Locate the /chosen node >>>> - // >>>> - Node =3D fdt_path_offset (mFdtImage, "/chosen"); >>>> - if (Node < 0) { >>>> - DEBUG ((DEBUG_ERROR, "%a: failed to locate /chosen node\n", >>>> __FUNCTION__)); >>>> - return EFI_NOT_FOUND; >>>> - } >>>> - >>>> - // >>>> - // If /chosen/bootargs already exists, we want to add a space >>>> character >>>> - // before adding the firmware supplied arguments. However, the >>>> RpiFirmware >>>> - // protocol expects a 32-bit aligned buffer. So let's allocate 4 >>>> bytes of >>>> - // slack, and skip the first 3 when passing this buffer into libfd= t. >>>> - // >>>> - CommandLine =3D AllocatePool (MAX_CMDLINE_SIZE) + 4; >>>> - if (!CommandLine) { >>>> - DEBUG ((DEBUG_ERROR, "%a: failed to allocate memory\n", >>>> __FUNCTION__)); >>>> - return EFI_OUT_OF_RESOURCES; >>>> - } >>>> - >>>> - // >>>> - // Get the command line from the firmware >>>> - // >>>> - Status =3D mFwProtocol->GetCommandLine (MAX_CMDLINE_SIZE, >>>> CommandLine + 4); >>>> - if (EFI_ERROR (Status)) { >>>> - DEBUG ((DEBUG_ERROR, "%a: failed to retrieve command line\n", >>>> __FUNCTION__)); >>>> - return Status; >>>> - } >>>> - >>>> - if (AsciiStrLen (CommandLine + 4) =3D=3D 0) { >>>> - DEBUG ((DEBUG_INFO, "%a: empty command line received\n", >>>> __FUNCTION__)); >>>> - return EFI_SUCCESS; >>>> - } >>>> - >>>> - CommandLine[3] =3D ' '; >>>> - >>>> - Retval =3D fdt_appendprop_string (mFdtImage, Node, "bootargs", >>>> &CommandLine[3]); >>>> - if (Retval !=3D 0) { >>>> - DEBUG ((DEBUG_ERROR, "%a: failed to set /chosen/bootargs >>>> property (%d)\n", >>>> - __FUNCTION__, Retval)); >>>> - } >>>> - >>>> - DEBUG_CODE_BEGIN (); >>>> - CONST CHAR8 *Prop; >>>> - INT32 Length; >>>> - INT32 Index; >>>> - >>>> - Node =3D fdt_path_offset (mFdtImage, "/chosen"); >>>> - ASSERT (Node >=3D 0); >>>> - >>>> - Prop =3D fdt_getprop (mFdtImage, Node, "bootargs", &Length); >>>> - ASSERT (Prop !=3D NULL); >>>> - >>>> - DEBUG ((DEBUG_INFO, "Command line set from firmware (length >>>> %d):\n'", Length)); >>>> - >>>> - for (Index =3D 0; Index < Length; Index++, Prop++) { >>>> - if (*Prop =3D=3D '\0') { >>>> - continue; >>>> - } >>>> - DEBUG ((DEBUG_INFO, "%c", *Prop)); >>>> - } >>>> - >>>> - DEBUG ((DEBUG_INFO, "'\n")); >>>> - DEBUG_CODE_END (); >>>> - >>>> - FreePool (CommandLine - 4); >>>> - return EFI_SUCCESS; >>>> -} >>>> - >>>> - >>>> /** >>>> @param ImageHandle of the loaded driver >>>> @param SystemTable Pointer to the System Table >>>> @@ -435,13 +351,10 @@ FdtDxeInitialize ( >>>> IN EFI_SYSTEM_TABLE *SystemTable >>>> ) >>>> { >>>> + INT32 Retval; >>>> EFI_STATUS Status; >>>> - EFI_GUID *FdtGuid; >>>> - VOID *FdtImage; >>>> UINTN FdtSize; >>>> - INT32 Retval; >>>> - UINT32 BoardRevision; >>>> - BOOLEAN Internal; >>>> + VOID *FdtImage =3D NULL; >>>> if (PcdGet32 (PcdOptDeviceTree) =3D=3D 0) { >>>> DEBUG ((DEBUG_INFO, "Device Tree disabled per user >>>> configuration\n")); >>>> @@ -450,77 +363,28 @@ FdtDxeInitialize ( >>>> Status =3D gBS->LocateProtocol (&gRaspberryPiFirmwareProtocolGuid= , >>>> NULL, >>>> (VOID**)&mFwProtocol); >>>> - ASSERT_EFI_ERROR (Status); >>>> + if (mFwProtocol =3D=3D NULL) { >>>> + /* >>>> + * Impossible because of the depex... >>>> + */ >>>> + ASSERT_EFI_ERROR (Status); >>>> + return EFI_NOT_FOUND; >>>> + } >>> >>> Do we need this change? >> >> Looking at what we are doing elsewhere, I thing we should go with: >> >> ASSERT_EFI_ERROR (Status); >> if (EFI_ERROR (Status)) { >> return Status; >> } >> >> The ASSERT_EFI_ERROR () on its own was not enough for RELEASE, so we >> definitely want to return an error code here if needed, and I guess >> that, technically, we could consider that LocateProtocol () may >> succeed and return a NULL value for mFwProtocol, but I doubt that's a >> valid consideration. >> > > But the DEPEX guarantees that the module will only be dispatched at a > time when LocateProtocol() will succeed. This is a common pattern, so I > wonder why it is being silently modified here. > Fair enough. Let's wait for Andrei to reply on this. /Pete --_000_BN6PR05MB34110B58136D06E18E7AED7AB9AB0BN6PR05MB3411namp_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable
Hi folks,

I added that specific line more as a safeguard in case the depex ever chan= ges, as I wasn't comfortable with code that relied on an external factor to= have defined behavior.

I don't have a strong position on it. If you want it to go, I'll send an u= pdate.

A
From: devel@edk2.groups.io= <devel@edk2.groups.io> on behalf of Pete Batard via groups.io <pe= te=3Dakeo.ie@groups.io>
Sent: Friday, May 1, 2020 10:16 AM
To: Ard Biesheuvel <ard.biesheuvel@arm.com>; Andrei Warkentin= <andrey.warkentin@gmail.com>; devel@edk2.groups.io <devel@edk2.gr= oups.io>
Cc: leif@nuviainc.com <leif@nuviainc.com>; philmd@redhat.com = <philmd@redhat.com>
Subject: Re: [edk2-devel] [edk2-platforms][PATCH 1/2] RPi: rip out = FdtDxe logic to use internal DTB
 
On 2020.05.01 16:13, Ard Biesheuvel wrote:
> On 5/1/20 5:11 PM, Pete Batard wrote:
>> On 2020.05.01 14:19, Ard Biesheuvel wrote:
>>> On 4/30/20 11:16 PM, Andrei Warkentin wrote:
>>>> Initially, FdtDxe used an internal (embedded in UEFI) FDT= ,
>>>> because it was neither understood how to consume the
>>>> one loaded by the VideoCore firmware, nor understood just=
>>>> how important it is to use the DTB provided by config.txt= .
>>>>
>>>> Embedding the DT was a bad idea, because:
>>>> - Permanently stale
>>>> - No overlays
>>>>
>>>> Also, on devices like the Pi 4 you _have_ to have a DT >>>> around for the start4 VPU firmware to pick up, otherwise<= br> >>>> the board is left in an inconsistent state. So we're bein= g
>>>> proscriptive now about DT use with config.txt, which mean= s
>>>> this internal DT logic is deadc code.
>>>>
>>>> Further FdtDxe cleanups are possible and will be handled<= br> >>>> separately, specifically:
>>>> - probably no need to use a separate allocation for patch= ed DT
>>>> (optimize memory used)
>>>> - suspicious use of EfiBootServicesData (I filed
>>>> https://nam04.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fgithub= .com%2FARM-software%2Febbr%2Fissues%2F45&amp;data=3D02%7C01%7Cawarkenti= n%40vmware.com%7Ccd65ce9968f54b569f8208d7ede2a025%7Cb39138ca3cee4b4aa4d6cd8= 3d9dd62f0%7C0%7C0%7C637239429916614640&amp;sdata=3DpJ3Wu%2BGJhj%2FqfbBE= FMQ9nQ%2B1Pgc%2Bo4Xw0fer2h9ZYvQ%3D&amp;reserved=3D0 to sort out the real
>>>> requirements)
>>>>
>>>> Testing: Booted Ubuntu 18.04 on Pi 2B (1.2).
>>>>
>>>> Signed-off-by: Andrei Warkentin <andrey.warkentin@gmai= l.com>
>>>> ---
>>>>   Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c =   | 206
>>>> ++++----------------
>>>>   Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.inf |&n= bsp;  4 -
>>>>   Platform/RaspberryPi/RPi3/RPi3.fdf  &nbs= p;          |  11 --
>>>>   Platform/RaspberryPi/RPi4/RPi4.fdf  &nbs= p;          |   8 -<= br> >>>>   Platform/RaspberryPi/RaspberryPi.dec  &n= bsp;        |   7 -
>>>>   5 files changed, 38 insertions(+), 198 deletio= ns(-)
>>>>
>>>> diff --git a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c=
>>>> b/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
>>>> index e7143f57..187b9566 100644
>>>> --- a/Platform/RaspberryPi/Drivers/FdtDxe/FdtDxe.c
>>>> +++ b/Platform/RaspberryPi/Drivers/FdtDxe/Fdt= Dxe.c
>>>> @@ -335,90 +335,6 @@ CleanSimpleFramebuffer (
>>>>     return EFI_SUCCESS;
>>>>   }
>>>> -#define MAX_CMDLINE_SIZE    512
>>>> -
>>>> -STATIC
>>>> -EFI_STATUS
>>>> -UpdateBootArgs (
>>>> -  VOID
>>>> -  )
>>>> -{
>>>> -  INTN       &nb= sp;  Node;
>>>> -  INTN       &nb= sp;  Retval;
>>>> -  EFI_STATUS    Status;
>>>> -  CHAR8       &n= bsp; *CommandLine;
>>>> -
>>>> -  //
>>>> -  // Locate the /chosen node
>>>> -  //
>>>> -  Node =3D fdt_path_offset (mFdtImage, "/chose= n");
>>>> -  if (Node < 0) {
>>>> -    DEBUG ((DEBUG_ERROR, "%a: failed= to locate /chosen node\n",
>>>> __FUNCTION__));
>>>> -    return EFI_NOT_FOUND;
>>>> -  }
>>>> -
>>>> -  //
>>>> -  // If /chosen/bootargs already exists, we want to= add a space
>>>> character
>>>> -  // before adding the firmware supplied arguments.= However, the
>>>> RpiFirmware
>>>> -  // protocol expects a 32-bit aligned buffer. So l= et's allocate 4
>>>> bytes of
>>>> -  // slack, and skip the first 3 when passing this = buffer into libfdt.
>>>> -  //
>>>> -  CommandLine =3D AllocatePool (MAX_CMDLINE_SIZE) &= #43; 4;
>>>> -  if (!CommandLine) {
>>>> -    DEBUG ((DEBUG_ERROR, "%a: failed= to allocate memory\n",
>>>> __FUNCTION__));
>>>> -    return EFI_OUT_OF_RESOURCES;
>>>> -  }
>>>> -
>>>> -  //
>>>> -  // Get the command line from the firmware
>>>> -  //
>>>> -  Status =3D mFwProtocol->GetCommandLine (MAX_CM= DLINE_SIZE,
>>>> CommandLine + 4);
>>>> -  if (EFI_ERROR (Status)) {
>>>> -    DEBUG ((DEBUG_ERROR, "%a: failed= to retrieve command line\n",
>>>> __FUNCTION__));
>>>> -    return Status;
>>>> -  }
>>>> -
>>>> -  if (AsciiStrLen (CommandLine + 4) =3D=3D 0) {=
>>>> -    DEBUG ((DEBUG_INFO, "%a: empty c= ommand line received\n",
>>>> __FUNCTION__));
>>>> -    return EFI_SUCCESS;
>>>> -  }
>>>> -
>>>> -  CommandLine[3] =3D ' ';
>>>> -
>>>> -  Retval =3D fdt_appendprop_string (mFdtImage, Node= , "bootargs",
>>>> &CommandLine[3]);
>>>> -  if (Retval !=3D 0) {
>>>> -    DEBUG ((DEBUG_ERROR, "%a: failed= to set /chosen/bootargs
>>>> property (%d)\n",
>>>> -      __FUNCTION__, Retval)); >>>> -  }
>>>> -
>>>> -  DEBUG_CODE_BEGIN ();
>>>> -    CONST CHAR8    *Prop;<= br> >>>> -    INT32     &n= bsp;   Length;
>>>> -    INT32     &n= bsp;   Index;
>>>> -
>>>> -    Node =3D fdt_path_offset (mFdtImage, = "/chosen");
>>>> -    ASSERT (Node >=3D 0);
>>>> -
>>>> -    Prop =3D fdt_getprop (mFdtImage, Node= , "bootargs", &Length);
>>>> -    ASSERT (Prop !=3D NULL);
>>>> -
>>>> -    DEBUG ((DEBUG_INFO, "Command lin= e set from firmware (length
>>>> %d):\n'", Length));
>>>> -
>>>> -    for (Index =3D 0; Index < Length; = Index++, Prop++) {
>>>> -      if (*Prop =3D=3D '\0') {<= br> >>>> -        continue;
>>>> -      }
>>>> -      DEBUG ((DEBUG_INFO, "= ;%c", *Prop));
>>>> -    }
>>>> -
>>>> -    DEBUG ((DEBUG_INFO, "'\n"))= ;
>>>> -  DEBUG_CODE_END ();
>>>> -
>>>> -  FreePool (CommandLine - 4);
>>>> -  return EFI_SUCCESS;
>>>> -}
>>>> -
>>>> -
>>>>   /**
>>>>     @param  ImageHandle   o= f the loaded driver
>>>>     @param  SystemTable   P= ointer to the System Table
>>>> @@ -435,13 +351,10 @@ FdtDxeInitialize (
>>>>     IN EFI_SYSTEM_TABLE   *Syste= mTable
>>>>     )
>>>>   {
>>>> +  INT32      Retval; >>>>     EFI_STATUS Status;
>>>> -  EFI_GUID   *FdtGuid;
>>>> -  VOID       *FdtImag= e;
>>>>     UINTN      Fd= tSize;
>>>> -  INT32      Retval;
>>>> -  UINT32     BoardRevision;
>>>> -  BOOLEAN    Internal;
>>>> +  VOID       *Fdt= Image =3D NULL;
>>>>     if (PcdGet32 (PcdOptDeviceTree) =3D=3D= 0) {
>>>>       DEBUG ((DEBUG_INFO, "= Device Tree disabled per user
>>>> configuration\n"));
>>>> @@ -450,77 +363,28 @@ FdtDxeInitialize (
>>>>     Status =3D gBS->LocateProtocol (&am= p;gRaspberryPiFirmwareProtocolGuid,
>>>> NULL,
>>>>          &nb= sp;          (VOID**)&mFwP= rotocol);
>>>> -  ASSERT_EFI_ERROR (Status);
>>>> +  if (mFwProtocol =3D=3D NULL) {
>>>> +    /*
>>>> +     * Impossible because of the= depex...
>>>> +     */
>>>> +    ASSERT_EFI_ERROR (Status);
>>>> +    return EFI_NOT_FOUND;
>>>> +  }
>>>
>>> Do we need this change?
>>
>> Looking at what we are doing elsewhere, I thing we should go with= :
>>
>>    ASSERT_EFI_ERROR (Status);
>>    if (EFI_ERROR (Status)) {
>>      return Status;
>>    }
>>
>> The ASSERT_EFI_ERROR () on its own was not enough for RELEASE, so= we
>> definitely want to return an error code here if needed, and I gue= ss
>> that, technically, we could consider that LocateProtocol () may <= br> >> succeed and return a NULL value for mFwProtocol, but I doubt that= 's a
>> valid consideration.
>>
>
> But the DEPEX guarantees that the module will only be dispatched at a=
> time when LocateProtocol() will succeed. This is a common pattern, so= I
> wonder why it is being silently modified here.
>

Fair enough. Let's wait for Andrei to reply on this.

/Pete



--_000_BN6PR05MB34110B58136D06E18E7AED7AB9AB0BN6PR05MB3411namp_--