From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ua1-f49.google.com (mail-ua1-f49.google.com [209.85.222.49]) by mx.groups.io with SMTP id smtpd.web12.32523.1631550962627231356 for ; Mon, 13 Sep 2021 09:36:02 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20210112 header.b=pBiXO+eo; spf=pass (domain: gmail.com, ip: 209.85.222.49, mailfrom: pedro.falcato@gmail.com) Received: by mail-ua1-f49.google.com with SMTP id s4so6621940uar.5 for ; Mon, 13 Sep 2021 09:36:02 -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:content-transfer-encoding; bh=LjJ5GJX++rCaUS1+6aIf2mIWC30zI/Ot2zYt+31ay5g=; b=pBiXO+eoQs279/HBalXL2E3mIGhuzKokMNWxsFLobTSru16NPSxtGM1lrQJlIksU8T CMiTzzW2ns+fVVWqqqnWE2v5C0vys4AnqdRW07WjQagLhzz8HXJyJyx34rS3RFZgnIjS tWLKLeVeA//egrkOThKMD24VYjY1pd/AdJpIWj8sOX9DFAYrfUZRcKMSSH6a4IYwCv8t +3ZnNEBy1gvuPXtRB8FwEQB2ivgpSrBDamfn/2P+2h+YyPFo3beGSu3b9l3XBtX2S5oh A5jcEXNUvHfUEkVvkqro6QQirrQ80nR1CKVleTxlyo6Or7jD6LkErkBJNEWMAap1H4Ql SVkw== 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:content-transfer-encoding; bh=LjJ5GJX++rCaUS1+6aIf2mIWC30zI/Ot2zYt+31ay5g=; b=ZKH03nfQfsKvK9X6DdM8iTLnNXeGKwnLHxe8OnvVB89iybGE/vZ7g8DhEeaKKBotO/ zxcu2nvZW0V+duyTm9ooRzWrETX2+MoDiRxqmgPkfm/D0gYc/JKI6OdU3dh3Ou0vCuiN k5RBgbKkUfarez21bfyOXyW56n0LGu8d/XWtbAufZUMq0+RaMKzvmbwus6mpV8xb72fU TCj2jr/mrIPTFNs9o+4yXZlFR5bmv02DVeiTniU1ZFy6SPEMJ1PDLxY3kxKDfju4giqg H2hTVsHzm9LhJnpssqUNUP8wM/CZIlRqSbh14rcaLgHHksEL0EyrK4tA0hmXr/inOVrj xd3Q== X-Gm-Message-State: AOAM531/DLpB1sXIkBwP3zgeak4LMOvnLKVyMiWsyAxLNLxdmDmeHZ1B YVwH823CrloJFuxyhglJnhulolJOx3APQHnIdz8= X-Google-Smtp-Source: ABdhPJxgH7rZEgaWldK4CP45j8hqI5uNywpbO4CwfLmKML2a8ppEpwt2zC0LXrm1vCQMzR+4Txw8DZ7zKxHpMyv7ZiY= X-Received: by 2002:a9f:3210:: with SMTP id x16mr472217uad.135.1631550961658; Mon, 13 Sep 2021 09:36:01 -0700 (PDT) MIME-Version: 1.0 References: <7631e4dcf3c01613ca24d3acd9b4bd5d09fa8126.1631311760.git.jbrasen@nvidia.com> <23908cee-adf2-bee3-3bf9-d65f245070b8@posteo.de> In-Reply-To: <23908cee-adf2-bee3-3bf9-d65f245070b8@posteo.de> From: "Pedro Falcato" Date: Mon, 13 Sep 2021 17:35:51 +0100 Message-ID: Subject: Re: [edk2-devel] [PATCH v3 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior To: =?UTF-8?Q?Marvin_H=C3=A4user?= Cc: edk2-devel-groups-io , Jeff Brasen Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On BY_DRIVER, Jeff reached the conclusion that we can't open the BLOCK_IO protocol with BY_DRIVER since it's already owned by DiskIoDxe. The finding makes sense and is consistent with FatPkg's behaviour. As for the GET_PROTOCOL issue, feel free to send a patch ;) On Sun, Sep 12, 2021 at 11:40 AM Marvin H=C3=A4user wr= ote: > > On 11/09/2021 00:11, Jeff Brasen via groups.io 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 c= alls > > > > Signed-off-by: Jeff Brasen > > --- > > Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h | 14 +++++++ > > Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c | 54 +++++++++++++++++++++-----= - > > Features/Ext4Pkg/Ext4Dxe/Superblock.c | 35 +++++++++++++++++ > > 3 files changed, 92 insertions(+), 11 deletions(-) > > > > diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4= Dxe/Ext4Dxe.h > > index 64eab455db..a9b932ed52 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. > > + > > + @returns Whether the partition has a valid EXT4 superblock magic va= lue. > > +**/ > > +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..d9fbe9ea78 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,65 @@ 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_DISK_IO_PROTOCOL *DiskIo; > > + EFI_BLOCK_IO_PROTOCOL *BlockIo; > > > > - EFI_STATUS Status; > > + DiskIo =3D NULL; > > + BlockIo =3D NULL; > > > > + // > > + // 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 > > ); > > > > if (EFI_ERROR (Status)) { > > return Status; > > } > > - > > + // > > + // 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 > > ); > > + > > + if (!EFI_ERROR (Status)) { > > + if (!Ext4SuperblockCheckMagic (DiskIo, BlockIo)) { > > + Status =3D EFI_UNSUPPORTED; > > + } > > + } > > + > > + // > > + // 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 > > + ); > > + } > > GET_PROTOCOL protocols are not to be closed, I guess this was missed > when there was still the question whether to use BY_DRIVER for BlockIo. > > Best regards, > Marvin > > > return Status; > > } > > > > diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/E= xt4Dxe/Superblock.c > > index c321d8c3d8..0c965415c5 100644 > > --- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c > > +++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c > > @@ -34,6 +34,41 @@ 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. > > > > +/** > > + Checks the superblock's magic value. > > + > > + @param[in] DiskIo Pointer to the DiskIo. > > + @param[in] BlockIo Pointer to the BlockIo. > > + > > + @returns Whether the partition has a valid EXT4 superblock magic va= lue. > > +**/ > > +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_SUPERBLO= CK, 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 Pedro Falcato