Re: [edk2-devel] [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior
We actually can't open the BlockIo protocol as BY_DRIVER as it is already owned by the DiskIoDxe driver. Will upload a v3 with the other feedback though. Thanks, Jeff From: Marvin Häuser Sent: Friday, September 10, 2021 12:09 PM To: Pedro Falcato Cc: edk2-devel-groups-io ; 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 19:08, Pedro Falcato wrote: > 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. UEFI 2.9, 11.1, "Device Driver", 2.: "It must use the same Attribute value that was used in Supported()." > So, change it to BY_DRIVER. The performance should be the same and > it's more consistent. Should be better because all handles opened by BY_DRIVER already will immediately fail. Best regards, Marvin > > On Fri, Sep 10, 2021 at 5:56 PM Marvin Häuser 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 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_DENIEDThe device specified by >>>> ControllerHandle and >>>> RemainingDevicePath is already being >>>> managed by a different >>>> driver or an application that >>>> requires exclusive access. >>>> - Currently not implemented. >&g
Re: [edk2-devel] [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior
On 10/09/2021 19:08, Pedro Falcato wrote:
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.
UEFI 2.9, 11.1, "Device Driver", 2.: "It must use the same Attribute
value that was used in Supported()."
So, change it to BY_DRIVER. The performance should be the same and
it's more consistent.
Should be better because all handles opened by BY_DRIVER already will
immediately fail.
Best regards,
Marvin
On Fri, Sep 10, 2021 at 5:56 PM Marvin Häuser 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 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_DENIEDThe 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_STATUSStatus;
+ 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,
,
- NULL,
- BindingProtocol->ImageHandle,
+ (VOID **) ,
+ 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,
,
- NULL,
- BindingProtocol->ImageHandle,
+ (VOID **) ,
+ BindingProtocol->DriverBindingHandle,
ControllerHandle,
- EFI_OPEN_PROTOCOL_TEST_PROTOCOL
+ EFI_OPEN_PROTOCOL_GET_PROTOCOL
);
+ if (EFI_ERROR (Status)) {
+goto Exit;
+ }
+
+
Re: [edk2-devel] [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior
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 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 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_DENIEDThe 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_STATUSStatus;
> >> + 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,
> >> ,
> >> - NULL,
> >> - BindingProtocol->ImageHandle,
> >> + (VOID **) ,
> >> + 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,
> >> ,
> >> - NULL,
> >> - BindingProtocol->ImageHandle,
> >> + (VOID **) ,
>
Re: [edk2-devel] [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior
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.falc...@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_DENIEDThe 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_STATUSStatus;
>> + 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,
>> ,
>> - NULL,
>> - BindingProtocol->ImageHandle
Re: [edk2-devel] [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior
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_DENIEDThe 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_STATUSStatus;
+ 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,
,
- NULL,
- BindingProtocol->ImageHandle,
+ (VOID **) ,
+ 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,
,
- NULL,
- BindingProtocol->ImageHandle,
+ (VOID **) ,
+ 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,
+ ,
+ BindingProtocol->DriverBindingHandle,
+ ControllerHandle
+ );
+ }
+ if (BlockIo != NULL) {
+gBS->CloseProtocol (
+ ControllerHandle,
+ ,
+ BindingProtocol->DriverBindingHandle,
+ ControllerHandle
+ );
+ }
return Status;
}
diff --git
Re: [edk2-devel] [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior
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_DENIEDThe 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_STATUSStatus;
> + 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,
>,
> - NULL,
> - BindingProtocol->ImageHandle,
> + (VOID **) ,
> + 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,
>,
> - NULL,
> - BindingProtocol->ImageHandle,
> + (VOID **) ,
> + 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,
> + ,
> + BindingProtocol->DriverBindingHandle,
> + ControllerHandle
> + );
> + }
> + if (BlockIo != NULL) {
> +gBS->CloseProtocol (
> + ControllerHandle,
> + ,
> + BindingProtocol->DriverBindingHandle,
> + ControllerHandle
> + );
> + }
>return Status;
> }
>
> diff --git
[edk2-devel] [PATCH v2 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior
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_DENIEDThe 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_STATUSStatus;
+ 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,
,
- NULL,
- BindingProtocol->ImageHandle,
+ (VOID **) ,
+ 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,
,
- NULL,
- BindingProtocol->ImageHandle,
+ (VOID **) ,
+ 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,
+ ,
+ BindingProtocol->DriverBindingHandle,
+ ControllerHandle
+ );
+ }
+ if (BlockIo != NULL) {
+gBS->CloseProtocol (
+ ControllerHandle,
+ ,
+ 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),
+
