From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-vk1-f177.google.com (mail-vk1-f177.google.com [209.85.221.177]) by mx.groups.io with SMTP id smtpd.web08.12009.1631292753269734117 for ; Fri, 10 Sep 2021 09:52:33 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20210112 header.b=MaVaqhgH; spf=pass (domain: gmail.com, ip: 209.85.221.177, mailfrom: pedro.falcato@gmail.com) Received: by mail-vk1-f177.google.com with SMTP id g34so853582vkd.11 for ; Fri, 10 Sep 2021 09:52:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=cwLBYMAeO8II3sLIuw6TTcAS7Rb9pMK+VYrH+saKrL0=; b=MaVaqhgHbWQgp+vylBokHfHlPxuFZbfUBDRQGPQ3Ccj4K7qBSKErw1EEdwspd+neny 7Hgvo9+xKKxtdSTXTgsQIWZhzzqy1h7ibwhFhOq5UUvCbF3J7YI1vlXERLrVdSjqTXlC WqPq/CuVGMdRk03lYCX+X9BDSdt4KIDmg3LGdiwYH7jNUuQMFQuGFuCgTVdHhCFjU7/4 TSJpTtGvu15Pf42BHi01CXxvhmNjgXJnE4BVgGmnJ3u2owuq8P15OL3Es33fdR9/FyAX 5k0Ke73hhOQ7DMdk5AV4+sTzQDDY8/iWf7F1cG3yfCCaZ843rl5/sTmQB3WwSRbwLGng 8G/g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=cwLBYMAeO8II3sLIuw6TTcAS7Rb9pMK+VYrH+saKrL0=; b=Dy/A2yOMR4/bqugRjYVSoB1i7cidQPIY9PA7A7lfLoONR2GzyoFH4dh/KEaSMoGs8+ 5txSFYyuPNQR/6avfACxrhivXFWfCz0PlqLBP+rbn+cSZQpFZ+qlNbdTU3pX0bdoR4nA cGKLEJcZO8zm1bnG0SiWNsF1TZCOH0wryqXTc/oP+R8gx4sE98r3TjbAMtua8srHLCLa bmuTOXU8K6touVzRIo3UuSn3dUmBQ3yse7TDpA5NUzqk0rGIkgJEN0MuAyr2l9ewgzN+ Ugqj/nsAr5Vh/unMiQpyOH6l4ttHQEubIEfqKTeplGZU3YskYYHZJL/hK2GbgulHhmbA sWgw== X-Gm-Message-State: AOAM530p3EtGVkSmKAnl+nEV9/yywiPwKz2CplyT2raogCczmojnt3wD T5B+n2bUkBTFRh1N+RI/tywe2BgV1gybIdhD5P4= X-Google-Smtp-Source: ABdhPJzjaJoj3yeGSks5alnj8a81C8YRP85BZVc9PqSltitNc985HZu8Cv5wknElhZuDWihKZU5z/13+0W+ECSyY/bA= X-Received: by 2002:a1f:9815:: with SMTP id a21mr5351300vke.25.1631292752319; Fri, 10 Sep 2021 09:52:32 -0700 (PDT) MIME-Version: 1.0 References: <00f58fdf6a9bce566ad5d145b0449ef3e6fdd94e.1631289393.git.jbrasen@nvidia.com> In-Reply-To: <00f58fdf6a9bce566ad5d145b0449ef3e6fdd94e.1631289393.git.jbrasen@nvidia.com> From: "Pedro Falcato" Date: Fri, 10 Sep 2021 17:52:21 +0100 Message-ID: Subject: Re: [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior To: Jeff Brasen Cc: edk2-devel-groups-io Content-Type: text/plain; charset="UTF-8" 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. 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 > -- Pedro Falcato