Yeah we need by_driver on at least one protocol to prevent re-binding to the 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 on how to open BlockIo. Thanks, Jeff ________________________________ From: Marvin Häuser 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 Ext4IsBindingSupported() 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 calls >> >> 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/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 >> >