Re: [edk2-devel] [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior

["Pedro Falcato" <pedro.falcato@gmail.com>]

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 <mhaeuser@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 <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 start 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          *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