public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Jeff Brasen" <jbrasen@nvidia.com>
To: Pedro Falcato <pedro.falcato@gmail.com>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>
Subject: Re: [PATCH 1/2] Ext4Pkg: Improve Binding support behavior
Date: Fri, 10 Sep 2021 04:34:00 +0000	[thread overview]
Message-ID: <DM6PR12MB334061623D9143117346AC1ACBD69@DM6PR12MB3340.namprd12.prod.outlook.com> (raw)
In-Reply-To: <CAKbZUD0q2ib71ATwWnagcB=gbGjaSgGdvO_MOg7t3XrLK-2d9A@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 7952 bytes --]

Sounds good will update that in a v2 patch tomorrow

Get Outlook for Android<https://aka.ms/ghei36>
________________________________
From: Pedro Falcato <pedro.falcato@gmail.com>
Sent: Thursday, September 9, 2021 10:22:51 PM
To: Jeff Brasen <jbrasen@nvidia.com>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>
Subject: Re: [PATCH 1/2] Ext4Pkg: Improve Binding support behavior

External email: Use caution opening links or attachments


Hi Jeff,

Comments below.

The patch itself looks good.

Nitpick on the commit message: I'd reword the commit message to
something like "Improve Ext4IsBindingSupported() behavior"
or "Improve IsBindingSupported behavior", since this patch doesn't
change Ext4Bind(), which does the actual bind.

On Thu, Sep 9, 2021 at 9:41 PM Jeff Brasen <jbrasen@nvidia.com> wrote:
>
> A couple improvements to improve performance.
> Add check to return ACCESS_DENIED if already connected
> Add check to verify superblock magic during supported to deduce start calls
>
> Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> ---
>  Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h    | 14 +++++++
>  Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c    | 57 +++++++++++++++++++++------
>  Features/Ext4Pkg/Ext4Dxe/Superblock.c | 34 ++++++++++++++++
>  3 files changed, 92 insertions(+), 13 deletions(-)
>
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> index 64eab455db..9a3938e671 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> @@ -1117,4 +1117,18 @@ Ext4GetVolumeName (
>    OUT UINTN          *VolNameLen
>    );
>
> +/**
> +   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
> +  );

I'd reword the first part of the comment into something like "Checks
the superblock's magic value.".
Aligning the two "Pointer to the ...Io" would be neat :)
Types and parameter names need to be separated by at least two
columns. So, something like:

 IN EFI_DISK_IO_PROTOCOL   *DiskIo,
 IN EFI_BLOCK_IO_PROTOCOL  *BlockIo

The feedback above also applies to the function's definition below, in
Superblock.c.

> +
>  #endif
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
> index ea2e048d77..cb1e6d532a 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,64 @@ 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_STATUS            Status;
> +  EFI_DISK_IO_PROTOCOL  *DiskIo = NULL;
> +  EFI_BLOCK_IO_PROTOCOL *BlockIo = NULL;
>
Initialization of variables are always separate from the declaration. Example:
EFI_DISK_IO_PROTOCOL  *DiskIo;
...

DiskIo = 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..1f8cdd3705 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
> @@ -34,6 +34,40 @@ 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
> +  )

Same as above.

> +{
> +  UINT16      Magic;
> +  EFI_STATUS Status;
> +
> +  Status = DiskIo->ReadDisk (DiskIo,
> +                             BlockIo->Media->MediaId,
> +                             EXT4_SUPERBLOCK_OFFSET + OFFSET_OF (EXT4_SUPERBLOCK, s_magic),
> +                             sizeof (Magic),
> +                             &Magic
> +                             );
Splitting a function call in multiple lines should be done like in
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2-docs.gitbook.io%2Fedk-ii-c-coding-standards-specification%2F5_source_files%2F52_spacing%235-2-2-4-subsequent-lines-of-multi-line-function-calls-should-line-up-two-spaces-from-the-beginning-of-the-function-name&amp;data=04%7C01%7Cjbrasen%40nvidia.com%7C6e1a40882279499fbb0008d97412af02%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637668446481896854%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=WZjbNs2Q3eU6QH%2F1RZbxakJnhVYOE4%2Bt%2BWF68JragVg%3D&amp;reserved=0.

> +  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

[-- Attachment #2: Type: text/html, Size: 15286 bytes --]

  reply	other threads:[~2021-09-10  4:34 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-09 20:40 [PATCH 0/2] ExtPkg Updates Jeff Brasen
2021-09-09 20:41 ` [PATCH 1/2] Ext4Pkg: Improve Binding support behavior Jeff Brasen
2021-09-10  4:22   ` Pedro Falcato
2021-09-10  4:34     ` Jeff Brasen [this message]
2021-09-10 16:24   ` [edk2-devel] " Marvin Häuser
2021-09-09 20:41 ` [PATCH 2/2] Ext4Pkg: Support non-cleanlty unmounted filesystems Jeff Brasen
2021-09-10  4:35   ` Pedro Falcato
2021-09-10  4:40 ` [PATCH 0/2] ExtPkg Updates Pedro Falcato

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DM6PR12MB334061623D9143117346AC1ACBD69@DM6PR12MB3340.namprd12.prod.outlook.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox