From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM12-MW2-obe.outbound.protection.outlook.com (NAM12-MW2-obe.outbound.protection.outlook.com [40.107.244.40]) by mx.groups.io with SMTP id smtpd.web09.12130.1631293518171697151 for ; Fri, 10 Sep 2021 10:05:18 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nvidia.com header.s=selector2 header.b=OahO+S90; spf=permerror, err=parse error for token &{10 18 %{i}._ip.%{h}._ehlo.%{d}._spf.vali.email}: invalid domain name (domain: nvidia.com, ip: 40.107.244.40, mailfrom: jbrasen@nvidia.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=hr+6pkONIjnpMoibG8VGO1j0iWbKqXyG9OZ3JQjjyAkHNnZzACODoHpgVe7ftNX89zLfplV5FJkdPC4YiBKvRE2b+oeZgfzb3ezH8eKslpAKo1135tNU234kuuwpTQwMKvAuMu6VxGGrYUIEbG1fY2VHT/nwZMdv+YkF35+lUBTvGUAfr9pwX1IAR0PIEUaKf5d8D42HaXyl5pWzgST4S+rlnCFUdsJUn2NNQ9KScr/rM0skss4lMcVpNlQ/OvrSzo5JA4pRI2ctt4Ygghu342cX1LX8SvNoW5d99Db30SpXkHVtikudmwU0CT8mwUYWy5mKZiRX/P1dSi1B9F/p/Q== 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; bh=MT+dps/qrsQJyToLUwKkfr2l+kMO40P0Sd1v2AY9K8I=; b=Sq0pKZIMFit7ADU0KFUg1O0biBwkBM+4Nz/WlA5OF5eMg/ecqMwR+X1jErXRV5crBpnWgCNlzyOy+b4IJZctKy0XsElNurv0Me/TS0eItvCi4lSWTCsk9WqCAlb9GGBHJI7wG65xZO5ylWQujS4qV+xNayU/lYlSudy5fd2eNSV42LOiItsmcSauLubgWtfQ14k2PMS6vFPOBqe61/LFF0lDO6+CCWIskhO9KHhYh15WQCQgW1d0efcPTQ3fW5l8p2yh0ZnaXZ0Nr5e8PgOqwjYHhn3g/SMHlKga8UvHwwhS8Px4MBTkFq7alQ9Sskum7b6LCrQEoWB44sSS34D2VA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=nvidia.com; dmarc=pass action=none header.from=nvidia.com; dkim=pass header.d=nvidia.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Nvidia.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=MT+dps/qrsQJyToLUwKkfr2l+kMO40P0Sd1v2AY9K8I=; b=OahO+S90ftZ8V75QL58O2MYrBS3A20zSfebeZXuyepBF7Tz5nTVkWGaKW6zOnfOMnA2PFRnWeX1qCnjJT3U9nmuGsmoSTAr/c5Meqyhqe7wfSEeCo7d7M2anq02a46zLpCCrK73IQtGwahCa2Z+ZZPM/PT3cFTm8ZSABZ6Ks9aUaAFku7ecaaBnQ1SMIzPJ5xSYihFZP1Xwep4c3sax4P+5dLnDrtLOn1KXwTSfCuVvRWNXHHsAZI9rxZZqVeIxGuboCx/iS4si08zOtQg3vXIW5Vgm9hHo+0jdGpybbMap4e7LG7oTlq5g8gIuc8TfWl1RyBXOMxyjqgjpO/lkYyg== Received: from DM6PR12MB3340.namprd12.prod.outlook.com (2603:10b6:5:3d::24) by DM5PR1201MB0156.namprd12.prod.outlook.com (2603:10b6:4:59::9) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4500.14; Fri, 10 Sep 2021 17:05:16 +0000 Received: from DM6PR12MB3340.namprd12.prod.outlook.com ([fe80::68ed:797b:18d4:848c]) by DM6PR12MB3340.namprd12.prod.outlook.com ([fe80::68ed:797b:18d4:848c%3]) with mapi id 15.20.4500.017; Fri, 10 Sep 2021 17:05:16 +0000 From: "Jeff Brasen" To: =?iso-8859-1?Q?Marvin_H=E4user?= , "devel@edk2.groups.io" , "pedro.falcato@gmail.com" Subject: Re: [edk2-devel] [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior Thread-Topic: [edk2-devel] [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior Thread-Index: AQHXplzAmzS2XdAeyEeDaa29U/4086ude7iAgAABG4CAAADk8A== Date: Fri, 10 Sep 2021 17:05:16 +0000 Message-ID: References: <00f58fdf6a9bce566ad5d145b0449ef3e6fdd94e.1631289393.git.jbrasen@nvidia.com> <45555d82-b6ab-da53-7662-bcbeab1ad6ce@posteo.de> In-Reply-To: <45555d82-b6ab-da53-7662-bcbeab1ad6ce@posteo.de> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: suggested_attachment_session_id: afcdf437-afb4-ada5-77d9-2f66be1113be authentication-results: posteo.de; dkim=none (message not signed) header.d=none;posteo.de; dmarc=none action=none header.from=nvidia.com; x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 071c50e4-2355-4895-48ca-08d9747d2964 x-ms-traffictypediagnostic: DM5PR1201MB0156: x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:2733; x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: 8Bnce6Fs/VtWpWH74TZoXHuu4kpOpnHOeNmXN00fc/DGaHkGp7mnokuy1XCzlsyfseIYryKYy+UpLaUKGajy8LCT980U/nZ2IsvcqcG6h8d7+wIc+mU8Sij1EadvyDowUv91++fKkCufRzDL754VeAOZ2eO7KAfE59xIUtTY8fjqInBxTDiv9H1oPLBSjTgUVGNXGVbXw6SpI+WrbL/WHD1MBZ4xIy+CSMZ1GnjfxqHBLUF3MZw3TWOxtDDaDQRVES3UfFtPHyyOnt+k8njVPMD03RpiBQfO6jW84hkXpATKG+aijsI6mQesPa3rBBBF0qz6dUmcRFI0M34JB+E7xujfTj/xHg7JF7RP1bHX7WRsnLJ27f82qO28ETQg/1rEwoJ1NL8SUbWM92124UoBeQw4xxWhFs3S1lBKbW5aMSTnxT3SCJ6OYFgB8fpz3Wu2fv3ts24yv++3lylcCgVuxCRBqjf+cOsTeXpyMrVIzjMOrQb5eAA2Pf7MMWO4yGw5aNS9s3R+TlJl5kfnFvlfssbm5TGF1EZO2q7MCBId/Ck4HgVh5S+QH+BF4zLaH6oxVbv8muGvdSOjgFBwxA+yxGg6TB73FCohLVGwrNZvNcqfokimOMurm4KHJoDO56hX5ZJJqAjsHSN2KBO64Wz5DpkrQ/h4VF+IcSa5H1kypycUIvIFKZh4tFuQiEfJAb/Frae9unzA/J6RubuI8JvTOoFdq6n2VP6KUNTYy0C+Jwc= x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:DM6PR12MB3340.namprd12.prod.outlook.com;PTR:;CAT:NONE;SFS:(6029001)(4636009)(366004)(346002)(376002)(39860400002)(396003)(136003)(9686003)(122000001)(71200400001)(52536014)(66574015)(91956017)(19627405001)(83380400001)(8936002)(86362001)(55016002)(8676002)(7696005)(33656002)(2906002)(186003)(478600001)(53546011)(6506007)(19627235002)(26005)(38070700005)(110136005)(5660300002)(66446008)(76116006)(64756008)(66476007)(66946007)(66556008)(38100700002)(316002)(32563001);DIR:OUT;SFP:1101; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?iso-8859-1?Q?i1RuXb7K6k9YBOX12xVxynIAOd7TLazYqNSzyWL3DTDJBUs2Z3TAkYiRAv?= =?iso-8859-1?Q?0kbx3/Bo2w+nXvFWLE97dWI/vwBTFu9cWWuVP3wgs7cX+29c+aw8VS61nc?= =?iso-8859-1?Q?xp6UmHbIw3UJeOy26THTUwke8LOJ58DISq+n2h8oE6s7HWrj0ct5zofF67?= =?iso-8859-1?Q?bBm9zw2rn5BpxlNkXcAAhjHkWr2652xWESmyKcAHxUGozQBw3TpXtUevau?= =?iso-8859-1?Q?IPsNxf1vWmv5ojWhQ/djPc1y5ACRT6jUEYwB3n4cgxKFZ3iJ5PnAX1wi9J?= =?iso-8859-1?Q?zyXoDzwsAnGJ5oPfiQcwhKdxAPvRLr9T3j8uVjh7X7HHwuvYpUVrb4SgGQ?= =?iso-8859-1?Q?/dZbTXXxwMy5XjqyYoEcTbeu1Jyw8tr5qVupKdTO07V9OocyPQCCgRJzrL?= =?iso-8859-1?Q?L8mpM7PhaeIzfJUFEXBMSf/jdykZqFwHMDJMSwQ42FJP0iGHEdGeGEB7xS?= =?iso-8859-1?Q?OGc0JLJAUqwgOtCpepqL0JopCF39VsiQ+lbVwqyUnJGwypGh6es85BgrBk?= =?iso-8859-1?Q?oU3sHGfJMivsM6g301YwYjpV+cqQvooXY6lMqztMjeE+Qr9kkqov7fSfqu?= =?iso-8859-1?Q?TI8FQiWj3pn+JyiqEugaoHGSzLEtqx9oRXxrwL/0ybOIZ0k6AOja/t86gN?= =?iso-8859-1?Q?qMu4B+8yfTfhgrw4N3PqtT8jafb7ankAaBJsMZjBTJi23GPj95zhITTJJ2?= =?iso-8859-1?Q?b9EawrezjEMfO3pVMuLBCm9PiKzI6gO9IlYnLkC/wkcPTemau5lQ7XjPac?= =?iso-8859-1?Q?wfVR/T3onaiPsZd6Pa6oKKaLeQwaPwwkZXdjM8+OUB3i4CQGMg4GNisxqU?= =?iso-8859-1?Q?81WtYJyHV/v+kGYQKJrszm8+ML5mBQArmgwFcaYUPKSHbSKHzkXLUz5iJV?= =?iso-8859-1?Q?R8kz/pgmG7u58NW6dLtaKvNK5JpJuw2iDI1xuvYDBu9xF5wiwcbxgx9as2?= =?iso-8859-1?Q?tECHN5AufiEAbS32/b8jnt0YsvqAVof+U6+0ZOdm8NnMaBwcdGqFaJecUe?= =?iso-8859-1?Q?I7wMWWuHfFjL9CfC2Y+F5/wr6l+L1a22mo4YdcNlksriRxIXjE7H7R/FQp?= =?iso-8859-1?Q?2hv3TVof24K6dZsTpngxxi45zC4j/3Ju3oKKtdEjVrX9YSCFpVSJQTeQee?= =?iso-8859-1?Q?UVKwZyaW28GAl0lbrICoN2cj5O/WUc8ddDIs6zNEABoZkYgzg9Gvp6Gbcw?= =?iso-8859-1?Q?NPv+2Bc3ZNT9jew+d7L0uNFYIoPPfFHNDNpOcleVsYhn/ny9SJ+GDsWGdI?= =?iso-8859-1?Q?u+EnwnapU5Ef3Z227ofPi4Cwhc9YJPbOZD9ezGq8jOTe0C2DTbmgWa2N8U?= =?iso-8859-1?Q?xg4vEjU/cszlOg2gDFEz6a7nI0t3a9GbFYq2hTJFeNc80FA=3D?= x-ms-exchange-transport-forked: True MIME-Version: 1.0 X-OriginatorOrg: Nvidia.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: DM6PR12MB3340.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 071c50e4-2355-4895-48ca-08d9747d2964 X-MS-Exchange-CrossTenant-originalarrivaltime: 10 Sep 2021 17:05:16.2858 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 43083d15-7273-40c1-b7db-39efd9ccc17a X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: 6f4InqcHJNusfnxn8RwP2f4k4PJDOFTfjSqCEQcZgALCsomnRImRUuQ/XPKTtA0BGaWHbAhboI/t2g5bQxuAEw== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR1201MB0156 Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_DM6PR12MB334088E915AD12547638C38ACBD69DM6PR12MB3340namp_" --_000_DM6PR12MB334088E915AD12547638C38ACBD69DM6PR12MB3340namp_ Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Yeah we need by_driver on at least one protocol to prevent re-binding to th= e controller. It will return ALREADY_STARTED if this driver already has it = open (from in Start) or ACCESS_DENIED if another driver is holding this. I can covert both to by_driver if that is desired. Will make the other little changes as well once I get a desired direction o= n how to open BlockIo. Thanks, Jeff ________________________________ From: Marvin H=E4user Sent: Friday, September 10, 2021 10:56 AM To: devel@edk2.groups.io ; pedro.falcato@gmail.com ; Jeff Brasen Subject: Re: [edk2-devel] [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupp= orted() behavior External email: Use caution opening links or attachments On 10/09/2021 18:52, Pedro Falcato wrote: > Like Marvin raised in v1, it's a good idea to change the first > OpenProtocol's EFI_OPEN_PROTOCOL_BY_DRIVER > into EFI_OPEN_PROTOCOL_GET_PROTOCOL, for consistency's sake. > Since we're not keeping these protocols open, using BY_DRIVER makes no > difference. No, the UEFI spec demands Supported() to use the same Attributes as Start() (likely for these very performance reasons), actually I thought both need BY_DRIVER. Best regards, Marvin > You didn't update Ext4SuperblockCheckMagic's comment in Superblock.c > when you changed it in the header. > Just a nitpick, but if you could quickly align the > Ext4SuperblockCheckMagic's parameters' descriptions like > > @param[in] DiskIo Pointer to the DiskIo. > @param[in] BlockIo Pointer to the BlockIo. > > it would be excellent. > > Apart from that, looks great to me and the patch series will get my RB > after that quick change. > > Best regards, > > Pedro > > On Fri, Sep 10, 2021 at 4:58 PM Jeff Brasen wrote: >> A couple of improvements to improve performance. >> Add check to return ACCESS_DENIED if already connected >> Add check to verify superblock magic during supported to reduce start ca= lls >> >> Signed-off-by: Jeff Brasen >> --- >> Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h | 14 +++++++ >> Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c | 58 +++++++++++++++++++++------ >> Features/Ext4Pkg/Ext4Dxe/Superblock.c | 35 ++++++++++++++++ >> 3 files changed, 95 insertions(+), 12 deletions(-) >> >> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4D= xe/Ext4Dxe.h >> index 64eab455db..5c60860894 100644 >> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h >> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h >> @@ -1117,4 +1117,18 @@ Ext4GetVolumeName ( >> OUT UINTN *VolNameLen >> ); >> >> +/** >> + Checks the superblock's magic value. >> + >> + @param[in] DiskIo Pointer to the DiskIo. >> + @param[in] BlockIo Pointer to the BlockIo. >> + >> + @return TRUE if a valid ext4 superblock, else FALSE. >> +**/ >> +BOOLEAN >> +Ext4SuperblockCheckMagic ( >> + IN EFI_DISK_IO_PROTOCOL *DiskIo, >> + IN EFI_BLOCK_IO_PROTOCOL *BlockIo >> + ); >> + >> #endif >> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c b/Features/Ext4Pkg/Ext4D= xe/Ext4Dxe.c >> index ea2e048d77..5ae93d0484 100644 >> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c >> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c >> @@ -631,7 +631,6 @@ Ext4Unload ( >> @retval EFI_ACCESS_DENIED The device specified by ControllerH= andle and >> RemainingDevicePath is already bein= g managed by a different >> driver or an application that requi= res exclusive access. >> - Currently not implemented. >> @retval EFI_UNSUPPORTED The device specified by ControllerH= andle and >> RemainingDevicePath is not supporte= d by the driver specified by This. >> **/ >> @@ -643,32 +642,67 @@ Ext4IsBindingSupported ( >> IN EFI_DEVICE_PATH *RemainingDevicePath OPTIONAL >> ) >> { >> - // Note to self: EFI_OPEN_PROTOCOL_TEST_PROTOCOL lets us not close th= e >> - // protocol and ignore the output argument entirely >> + EFI_STATUS Status; >> + EFI_DISK_IO_PROTOCOL *DiskIo; >> + EFI_BLOCK_IO_PROTOCOL *BlockIo; >> >> - EFI_STATUS Status; >> + DiskIo =3D NULL; >> + BlockIo =3D NULL; >> >> + // >> + // Open the IO Abstraction(s) needed to perform the supported test >> + // >> Status =3D gBS->OpenProtocol ( >> ControllerHandle, >> &gEfiDiskIoProtocolGuid, >> - NULL, >> - BindingProtocol->ImageHandle, >> + (VOID **) &DiskIo, >> + BindingProtocol->DriverBindingHandle, >> ControllerHandle, >> - EFI_OPEN_PROTOCOL_TEST_PROTOCOL >> + EFI_OPEN_PROTOCOL_BY_DRIVER >> ); >> >> if (EFI_ERROR (Status)) { >> - return Status; >> + goto Exit; >> } >> - >> + // >> + // Open the IO Abstraction(s) needed to perform the supported test >> + // >> Status =3D gBS->OpenProtocol ( >> ControllerHandle, >> &gEfiBlockIoProtocolGuid, >> - NULL, >> - BindingProtocol->ImageHandle, >> + (VOID **) &BlockIo, >> + BindingProtocol->DriverBindingHandle, >> ControllerHandle, >> - EFI_OPEN_PROTOCOL_TEST_PROTOCOL >> + EFI_OPEN_PROTOCOL_GET_PROTOCOL >> ); >> + if (EFI_ERROR (Status)) { >> + goto Exit; >> + } >> + >> + if (!Ext4SuperblockCheckMagic (DiskIo, BlockIo)) { >> + Status =3D EFI_UNSUPPORTED; >> + } >> + >> +Exit: >> + // >> + // Close the I/O Abstraction(s) used to perform the supported test >> + // >> + if (DiskIo !=3D NULL) { >> + gBS->CloseProtocol ( >> + ControllerHandle, >> + &gEfiDiskIoProtocolGuid, >> + BindingProtocol->DriverBindingHandle, >> + ControllerHandle >> + ); >> + } >> + if (BlockIo !=3D NULL) { >> + gBS->CloseProtocol ( >> + ControllerHandle, >> + &gEfiBlockIoProtocolGuid, >> + BindingProtocol->DriverBindingHandle, >> + ControllerHandle >> + ); >> + } >> return Status; >> } >> >> diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/Ex= t4Dxe/Superblock.c >> index c321d8c3d8..1ceb0d5cbb 100644 >> --- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c >> +++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c >> @@ -34,6 +34,41 @@ STATIC CONST UINT32 gSupportedIncompatFeat =3D >> // this is desired, it's fairly trivial to look for EFI_VOLUME_CORRUPT= ED >> // references and add some Ext4SignalCorruption function + function ca= ll. >> >> +/** >> + Checks only superblock magic value. >> + >> + @param[in] DiskIo Pointer to the DiskIo. >> + @param[in] BlockIo Pointer to the BlockIo. >> + >> + @return TRUE if a valid ext4 superblock, else FALSE. >> +**/ >> +BOOLEAN >> +Ext4SuperblockCheckMagic ( >> + IN EFI_DISK_IO_PROTOCOL *DiskIo, >> + IN EFI_BLOCK_IO_PROTOCOL *BlockIo >> + ) >> +{ >> + UINT16 Magic; >> + EFI_STATUS Status; >> + >> + Status =3D DiskIo->ReadDisk ( >> + DiskIo, >> + BlockIo->Media->MediaId, >> + EXT4_SUPERBLOCK_OFFSET + OFFSET_OF (EXT4_SUPERBLOC= K, s_magic), >> + sizeof (Magic), >> + &Magic >> + ); >> + if (EFI_ERROR (Status)) { >> + return FALSE; >> + } >> + >> + if (Magic !=3D EXT4_SIGNATURE) { >> + return FALSE; >> + } >> + >> + return TRUE; >> +} >> + >> /** >> Does brief validation of the ext4 superblock. >> >> -- >> 2.17.1 >> > --_000_DM6PR12MB334088E915AD12547638C38ACBD69DM6PR12MB3340namp_ Content-Type: text/html; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable
Yeah we need by_driver on at least one protocol to prevent re-binding to th= e controller. It will return ALREADY_STARTED if this driver already has it = open (from in Start) or ACCESS_DENIED if another driver is holding this.

I can covert both to by_driver if that is desired.

Will make the other little changes as well once I get a desired direction o= n how to open BlockIo.

Thanks,

Jeff

From: Marvin H=E4user <m= haeuser@posteo.de>
Sent: Friday, September 10, 2021 10:56 AM
To: devel@edk2.groups.io <devel@edk2.groups.io>; pedro.falcato= @gmail.com <pedro.falcato@gmail.com>; Jeff Brasen <jbrasen@nvidia.= com>
Subject: Re: [edk2-devel] [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBind= ingSupported() behavior
 
External email: Use caution opening links or attac= hments


On 10/09/2021 18:52, Pedro Falcato wrote:
> Like Marvin raised in v1, it's a good idea to change the first
> OpenProtocol's EFI_OPEN_PROTOCOL_BY_DRIVER
> into EFI_OPEN_PROTOCOL_GET_PROTOCOL, for consistency's sake.
> Since we're not keeping these protocols open, using BY_DRIVER makes no=
> difference.

No, the UEFI spec demands Supported() to use the same Attributes as
Start() (likely for these very performance reasons), actually I thought
both need BY_DRIVER.

Best regards,
Marvin

> You didn't update Ext4SuperblockCheckMagic's comment in Superblock.c > when you changed it in the header.
> Just a nitpick, but if you could quickly align the
> Ext4SuperblockCheckMagic's parameters' descriptions like
>
>     @param[in] DiskIo     = ; Pointer to the DiskIo.
>     @param[in] BlockIo     Poi= nter to the BlockIo.
>
> it would be excellent.
>
> Apart from that, looks great to me and the patch series will get my RB=
> after that quick change.
>
> Best regards,
>
> Pedro
>
> On Fri, Sep 10, 2021 at 4:58 PM Jeff Brasen <jbrasen@nvidia.com>= wrote:
>> A couple of improvements to improve performance.
>> Add check to return ACCESS_DENIED if already connected
>> Add check to verify superblock magic during supported to reduce st= art calls
>>
>> Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
>> ---
>>   Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h    |= 14 +++++++
>>   Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c    |= 58 +++++++++++++++++++++------
>>   Features/Ext4Pkg/Ext4Dxe/Superblock.c | 35 +++++++++++= +++++
>>   3 files changed, 95 insertions(+), 12 deletions(-)
>>
>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg= /Ext4Dxe/Ext4Dxe.h
>> index 64eab455db..5c60860894 100644
>> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
>> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
>> @@ -1117,4 +1117,18 @@ Ext4GetVolumeName (
>>     OUT UINTN     &nb= sp;    *VolNameLen
>>     );
>>
>> +/**
>> +   Checks the superblock's magic value.
>> +
>> +   @param[in] DiskIo     Pointer to= the DiskIo.
>> +   @param[in] BlockIo     Pointer t= o the BlockIo.
>> +
>> +   @return TRUE if a valid ext4 superblock, else FALSE.=
>> +**/
>> +BOOLEAN
>> +Ext4SuperblockCheckMagic (
>> +  IN EFI_DISK_IO_PROTOCOL   *DiskIo,
>> +  IN EFI_BLOCK_IO_PROTOCOL  *BlockIo
>> +  );
>> +
>>   #endif
>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c b/Features/Ext4Pkg= /Ext4Dxe/Ext4Dxe.c
>> index ea2e048d77..5ae93d0484 100644
>> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
>> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
>> @@ -631,7 +631,6 @@ Ext4Unload (
>>     @retval EFI_ACCESS_DENIED   = ;     The device specified by ControllerHandle and
>>           &= nbsp;           &nbs= p;            &= nbsp; RemainingDevicePath is already being managed by a different
>>           &= nbsp;           &nbs= p;            &= nbsp; driver or an application that requires exclusive access.
>> -           = ;            &n= bsp;           Currently = not implemented.
>>     @retval EFI_UNSUPPORTED   &= nbsp;      The device specified by ControllerHandl= e and
>>           &= nbsp;           &nbs= p;            &= nbsp; RemainingDevicePath is not supported by the driver specified by This.=
>>   **/
>> @@ -643,32 +642,67 @@ Ext4IsBindingSupported (
>>     IN EFI_DEVICE_PATH *RemainingDevicePath&nb= sp; OPTIONAL
>>     )
>>   {
>> -  // Note to self: EFI_OPEN_PROTOCOL_TEST_PROTOCOL lets us n= ot close the
>> -  // protocol and ignore the output argument entirely
>> +  EFI_STATUS        =     Status;
>> +  EFI_DISK_IO_PROTOCOL  *DiskIo;
>> +  EFI_BLOCK_IO_PROTOCOL *BlockIo;
>>
>> -  EFI_STATUS  Status;
>> +  DiskIo =3D NULL;
>> +  BlockIo =3D NULL;
>>
>> +  //
>> +  // Open the IO Abstraction(s) needed to perform the suppor= ted test
>> +  //
>>     Status =3D gBS->OpenProtocol (
>>           &= nbsp;         ControllerHandle,
>>           &= nbsp;         &gEfiDiskIoProtoc= olGuid,
>> -           = ;       NULL,
>> -           = ;       BindingProtocol->ImageHandle,
>> +           = ;       (VOID **) &DiskIo,
>> +           = ;       BindingProtocol->DriverBindingHand= le,
>>           &= nbsp;         ControllerHandle,
>> -           = ;       EFI_OPEN_PROTOCOL_TEST_PROTOCOL
>> +           = ;       EFI_OPEN_PROTOCOL_BY_DRIVER
>>           &= nbsp;         );
>>
>>     if (EFI_ERROR (Status)) {
>> -    return Status;
>> +    goto Exit;
>>     }
>> -
>> +  //
>> +  // Open the IO Abstraction(s) needed to perform the suppor= ted test
>> +  //
>>     Status =3D gBS->OpenProtocol (
>>           &= nbsp;         ControllerHandle,
>>           &= nbsp;         &gEfiBlockIoProto= colGuid,
>> -           = ;       NULL,
>> -           = ;       BindingProtocol->ImageHandle,
>> +           = ;       (VOID **) &BlockIo,
>> +           = ;       BindingProtocol->DriverBindingHand= le,
>>           &= nbsp;         ControllerHandle,
>> -           = ;       EFI_OPEN_PROTOCOL_TEST_PROTOCOL
>> +           = ;       EFI_OPEN_PROTOCOL_GET_PROTOCOL
>>           &= nbsp;         );
>> +  if (EFI_ERROR (Status)) {
>> +    goto Exit;
>> +  }
>> +
>> +  if (!Ext4SuperblockCheckMagic (DiskIo, BlockIo)) {
>> +    Status =3D EFI_UNSUPPORTED;
>> +  }
>> +
>> +Exit:
>> +  //
>> +  // Close the I/O Abstraction(s) used to perform the suppor= ted test
>> +  //
>> +  if (DiskIo !=3D NULL) {
>> +    gBS->CloseProtocol (
>> +          Controller= Handle,
>> +          &gEfiD= iskIoProtocolGuid,
>> +          BindingPro= tocol->DriverBindingHandle,
>> +          Controller= Handle
>> +          );
>> +  }
>> +  if (BlockIo !=3D NULL) {
>> +    gBS->CloseProtocol (
>> +          Controller= Handle,
>> +          &gEfiB= lockIoProtocolGuid,
>> +          BindingPro= tocol->DriverBindingHandle,
>> +          Controller= Handle
>> +          );
>> +  }
>>     return Status;
>>   }
>>
>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4= Pkg/Ext4Dxe/Superblock.c
>> index c321d8c3d8..1ceb0d5cbb 100644
>> --- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
>> +++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
>> @@ -34,6 +34,41 @@ STATIC CONST UINT32  gSupportedIncompatFea= t =3D
>>   // this is desired, it's fairly trivial to look for EF= I_VOLUME_CORRUPTED
>>   // references and add some Ext4SignalCorruption functi= on + function call.
>>
>> +/**
>> +   Checks only superblock magic value.
>> +
>> +   @param[in] DiskIo     Pointer to= the DiskIo.
>> +   @param[in] BlockIo     Pointer t= o the BlockIo.
>> +
>> +   @return TRUE if a valid ext4 superblock, else FALSE.=
>> +**/
>> +BOOLEAN
>> +Ext4SuperblockCheckMagic (
>> +  IN EFI_DISK_IO_PROTOCOL   *DiskIo,
>> +  IN EFI_BLOCK_IO_PROTOCOL  *BlockIo
>> +  )
>> +{
>> +  UINT16      Magic;
>> +  EFI_STATUS  Status;
>> +
>> +  Status =3D DiskIo->ReadDisk (
>> +           = ;          DiskIo,
>> +           = ;          BlockIo->Media-&= gt;MediaId,
>> +           = ;          EXT4_SUPERBLOCK_OFF= SET + OFFSET_OF (EXT4_SUPERBLOCK, s_magic),
>> +           = ;          sizeof (Magic),
>> +           = ;          &Magic
>> +           = ;          );
>> +  if (EFI_ERROR (Status)) {
>> +    return FALSE;
>> +  }
>> +
>> +  if (Magic !=3D EXT4_SIGNATURE) {
>> +    return FALSE;
>> +  }
>> +
>> +  return TRUE;
>> +}
>> +
>>   /**
>>      Does brief validation of the ext4 su= perblock.
>>
>> --
>> 2.17.1
>>
>

--_000_DM6PR12MB334088E915AD12547638C38ACBD69DM6PR12MB3340namp_--