Ah yes, thanks! Although I didn't find the part where they say that
you need to use the same attributes,
after re-reading the spec it seems they recommend using BY_DRIVER.
So, change it to BY_DRIVER. The performance should be the same and
it's more consistent.
On Fri, Sep 10, 2021 at 5:56 PM Marvin Häuser <mhaeu...@posteo.de> wrote:
>
> 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 <jbra...@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 start calls
> >>
> >> Signed-off-by: Jeff Brasen <jbra...@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 *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/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
> >> RemainingDevicePath is already being
> >> managed by a different
> >> driver or an application that
> >> requires exclusive access.
> >> - Currently not implemented.
> >> @retval EFI_UNSUPPORTED The device specified by
> >> ControllerHandle and
> >> RemainingDevicePath is not supported
> >> 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 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 = NULL;
> >> + BlockIo = NULL;
> >>
> >> + //
> >> + // Open the IO Abstraction(s) needed to perform the supported test
> >> + //
> >> Status = 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 = 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 = EFI_UNSUPPORTED;
> >> + }
> >> +
> >> +Exit:
> >> + //
> >> + // Close the I/O Abstraction(s) used to perform the supported test
> >> + //
> >> + if (DiskIo != NULL) {
> >> + gBS->CloseProtocol (
> >> + ControllerHandle,
> >> + &gEfiDiskIoProtocolGuid,
> >> + BindingProtocol->DriverBindingHandle,
> >> + ControllerHandle
> >> + );
> >> + }
> >> + if (BlockIo != NULL) {
> >> + gBS->CloseProtocol (
> >> + ControllerHandle,
> >> + &gEfiBlockIoProtocolGuid,
> >> + BindingProtocol->DriverBindingHandle,
> >> + ControllerHandle
> >> + );
> >> + }
> >> return Status;
> >> }
> >>
> >> diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
> >> b/Features/Ext4Pkg/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 gSupportedIncompatFeat =
> >> // this is desired, it's fairly trivial to look for EFI_VOLUME_CORRUPTED
> >> // references and add some Ext4SignalCorruption function + function call.
> >>
> >> +/**
> >> + 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 = 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 != EXT4_SIGNATURE) {
> >> + return FALSE;
> >> + }
> >> +
> >> + return TRUE;
> >> +}
> >> +
> >> /**
> >> Does brief validation of the ext4 superblock.
> >>
> >> --
> >> 2.17.1
> >>
> >
>
--
Pedro Falcato
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#80502): https://edk2.groups.io/g/devel/message/80502
Mute This Topic: https://groups.io/mt/85513026/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
