From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout02.posteo.de (mout02.posteo.de [185.67.36.66]) by mx.groups.io with SMTP id smtpd.web11.11471.1631291102196609887 for ; Fri, 10 Sep 2021 09:25:03 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@posteo.de header.s=2017 header.b=nC2U+NIz; spf=pass (domain: posteo.de, ip: 185.67.36.66, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [89.146.220.130]) by mout02.posteo.de (Postfix) with ESMTPS id D8A65240105 for ; Fri, 10 Sep 2021 18:24:58 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1631291098; bh=37YxrkhYVkK4OVfdx1+h849j9c0a6qA5CeKoLdM9q2U=; h=Subject:To:Cc:From:Date:From; b=nC2U+NIzmK5ga0zkwruK0RlKXgJyFJ1kyCnW3YQsBKEnZAPvUKcYNGruWLTYZ9fVk 5M5IHPOm3EhSuJKAaFe0SfdNItxB3bstwBstGkgfDx8Ikmqxu33r72lywiA5AD6nAU PzVAdqS3e8n3h9nREQnT1tLnkA+eAMTgg5E3RdEaUskNEvnyPHXCWg3nxG1nAMH7Bq EtpikIKdwmsThLWlld3A+uYTf6qCH51zqTBE9XrAf3kavvBmcTDPb3FLjgdUHJg2sE LkrlbwID1K3ON2Pw5uRrtX9eCDGb1e3C89KU18xdyCUyZiUFB6TsdwdkB9UFaTp6sF R7bNEcH74yZLg== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4H5h6d5MgTz9rxD; Fri, 10 Sep 2021 18:24:57 +0200 (CEST) Subject: Re: [edk2-devel] [PATCH 1/2] Ext4Pkg: Improve Binding support behavior To: devel@edk2.groups.io, jbrasen@nvidia.com Cc: pedro.falcato@gmail.com References: <9c6ac428e55f3584295bd490502532ba456b4d3f.1631219610.git.jbrasen@nvidia.com> From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= Message-ID: Date: Fri, 10 Sep 2021 16:24:57 +0000 MIME-Version: 1.0 In-Reply-To: <9c6ac428e55f3584295bd490502532ba456b4d3f.1631219610.git.jbrasen@nvidia.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: quoted-printable Good day, On 09/09/2021 22:41, Jeff Brasen via groups.io wrote: > A couple improvements to improve performance. > Add check to return ACCESS_DENIED if already connected This "performance improvement" actually aligns the load behaviour with=20 the UEFI spec, maybe it should be mentioned? > Add check to verify superblock magic during supported to deduce start c= alls > > Signed-off-by: Jeff Brasen > --- > Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h | 14 +++++++ > Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c | 57 +++++++++++++++++++++-----= - > Features/Ext4Pkg/Ext4Dxe/Superblock.c | 34 ++++++++++++++++ > 3 files changed, 92 insertions(+), 13 deletions(-) > > diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4= Dxe/Ext4Dxe.h > index 64eab455db..9a3938e671 100644 > --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h > +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h > @@ -1117,4 +1117,18 @@ Ext4GetVolumeName ( > OUT UINTN *VolNameLen > ); > =20 > +/** > + 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 > + ); > + > #endif > diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c b/Features/Ext4Pkg/Ext4= Dxe/Ext4Dxe.c > index ea2e048d77..cb1e6d532a 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 Controller= Handle and > RemainingDevicePath is already bei= ng managed by a different > driver or an application that requ= ires exclusive access. > - Currently not implemented. > @retval EFI_UNSUPPORTED The device specified by Controller= Handle and > RemainingDevicePath is not support= ed by the driver specified by This. > **/ > @@ -643,32 +642,64 @@ Ext4IsBindingSupported ( > IN EFI_DEVICE_PATH *RemainingDevicePath OPTIONAL > ) > { > - // Note to self: EFI_OPEN_PROTOCOL_TEST_PROTOCOL lets us not close t= he > - // protocol and ignore the output argument entirely > - > - EFI_STATUS Status; > + EFI_STATUS Status; > + EFI_DISK_IO_PROTOCOL *DiskIo =3D NULL; > + EFI_BLOCK_IO_PROTOCOL *BlockIo =3D NULL; > =20 > + // > + // 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 > ); > =20 > if (EFI_ERROR (Status)) { > - return Status; > + goto Exit; This change is not required as nothing was opened on failure; see below. > } > - > + // > + // 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 Why do the open modes mismatch if both protocols are equally stored in=20 the private partition data? Is the owner of Block I/O a different one=20 than the one of Disk I/O? > ); > + if (EFI_ERROR (Status)) { > + goto Exit; > + } > + > + if (!Ext4SuperblockCheckMagic (DiskIo, BlockIo)) { > + Status =3D EFI_UNSUPPORTED; > + } This can easily be rewritten to not require "goto": =C2=A0 if (!EFI_ERROR (Status) && !Ext4SuperblockCheckMagic (DiskIo, Blo= ckIo)) { =C2=A0=C2=A0=C2=A0 Status =3D EFI_UNSUPPORTED; =C2=A0 } With the change above this allows to drop the label and any "goto" code. > + > +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 > + ); > + } Protocols retrieved by GET_PROTOCOL are not closed. > return Status; > } > =20 > diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/E= xt4Dxe/Superblock.c > index c321d8c3d8..1f8cdd3705 100644 > --- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c > +++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c > @@ -34,6 +34,40 @@ STATIC CONST UINT32 gSupportedIncompatFeat =3D > // this is desired, it's fairly trivial to look for EFI_VOLUME_CORRUP= TED > // references and add some Ext4SignalCorruption function + function c= all. > =20 > +/** > + Checks only superblock magic value. Whether it "only" checks the magic is more of a functional detail than=20 an actual description. > + > + @param[in] DiskIo Pointer to the DiskIo. > + @param[in] BlockIo Pointer to the BlockIo. > + > + @return TRUE if a valid ext4 superblock, else FALSE. Maybe use "@returns=C2=A0 Whether the partition has a valid EXT4 superblo= ck=20 magic" for readability? Best regards, Marvin > +**/ > +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_= 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 superblock. > =20