> On 20. Feb 2023, at 08:52, Ard Biesheuvel <a...@kernel.org> wrote: > > On Fri, 17 Feb 2023 at 21:14, Pedro Falcato <pedro.falc...@gmail.com> wrote: >> >> There have been reports[1] of failures to boot due to unicode collation >> protocols not being available at Ext4Dxe load time. Therefore, attempt >> to initialize unicode collation at Start() time, like done previously in >> FatPkg/EnhancedFatDxe. By doing so, we move collation initialization >> to BDS, where the module responsible for protocol installation should >> have already been loaded and ran. >> >> [1]: https://edk2.groups.io/g/devel/message/100312 >> >> Cc: Ard Biesheuvel <a...@kernel.org> >> Cc: Marvin Häuser <mhaeu...@posteo.de> >> Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.") >> Signed-off-by: Pedro Falcato <pedro.falc...@gmail.com> >> --- >> RFC-ish patch that implements one of the possibilities discussed in [1]. >> Please test and give feedback. >> >> Odd that this issue was never caught in any other platform. Do the RPi >> platforms have >> something funky going on? >> > > It could be something as simple as the order the drivers appear in the > .FDF file. > >> Features/Ext4Pkg/Ext4Dxe/Collation.c | 18 +++++++++- >> Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c | 51 +++++++++++++++------------- >> Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h | 11 ++++++ >> 3 files changed, 56 insertions(+), 24 deletions(-) >> >> diff --git a/Features/Ext4Pkg/Ext4Dxe/Collation.c >> b/Features/Ext4Pkg/Ext4Dxe/Collation.c >> index 91d172b1cb89..65c34c33ea93 100644 >> --- a/Features/Ext4Pkg/Ext4Dxe/Collation.c >> +++ b/Features/Ext4Pkg/Ext4Dxe/Collation.c >> @@ -1,7 +1,7 @@ >> /** @file >> Unicode collation routines >> >> - Copyright (c) 2021 Pedro Falcato All rights reserved. >> + Copyright (c) 2021 - 2023 Pedro Falcato All rights reserved. >> Copyright (c) 2005 - 2017, Intel Corporation. All rights reserved. >> >> SPDX-License-Identifier: BSD-2-Clause-Patent >> @@ -9,6 +9,7 @@ >> >> #include <Uefi.h> >> >> +#include <Library/DebugLib.h> >> #include <Library/UefiLib.h> >> #include <Library/UefiBootServicesTableLib.h> >> #include <Library/MemoryAllocationLib.h> >> @@ -169,5 +170,20 @@ Ext4StrCmpInsensitive ( >> IN CHAR16 *Str2 >> ) >> { >> + ASSERT (gUnicodeCollationInterface != NULL); >> return gUnicodeCollationInterface->StriColl (gUnicodeCollationInterface, >> Str1, Str2); >> } >> + >> +/** >> + Check if unicode collation is initialized >> + >> + @retval TRUE if Ext4InitialiseUnicodeCollation() was already called >> successfully >> + @retval FALSE if Ext4InitialiseUnicodeCollation() was not yet called >> successfully >> +**/ >> +BOOLEAN >> +Ext4IsCollationInitialized ( >> + VOID >> + ) >> +{ >> + return gUnicodeCollationInterface != NULL; >> +} >> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c >> b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c >> index 2a4f5a7bd0ef..de0d633febfb 100644 >> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c >> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c >> @@ -1,7 +1,7 @@ >> /** @file >> Driver entry point >> >> - Copyright (c) 2021 Pedro Falcato All rights reserved. >> + Copyright (c) 2021 - 2023 Pedro Falcato All rights reserved. >> SPDX-License-Identifier: BSD-2-Clause-Patent >> **/ >> >> @@ -513,26 +513,18 @@ Ext4EntryPoint ( >> IN EFI_SYSTEM_TABLE *SystemTable >> ) >> { >> - EFI_STATUS Status; >> - >> - Status = EfiLibInstallAllDriverProtocols2 ( >> - ImageHandle, >> - SystemTable, >> - &gExt4BindingProtocol, >> - ImageHandle, >> - &gExt4ComponentName, >> - &gExt4ComponentName2, >> - NULL, >> - NULL, >> - NULL, >> - NULL >> - ); >> - >> - if (EFI_ERROR (Status)) { >> - return Status; >> - } >> - >> - return Ext4InitialiseUnicodeCollation (ImageHandle); >> + return EfiLibInstallAllDriverProtocols2 ( >> + ImageHandle, >> + SystemTable, >> + &gExt4BindingProtocol, >> + ImageHandle, >> + &gExt4ComponentName, >> + &gExt4ComponentName2, >> + NULL, >> + NULL, >> + NULL, >> + NULL >> + ); >> } >> >> /** >> @@ -761,6 +753,19 @@ Ext4Bind ( >> BlockIo = NULL; >> DiskIo = NULL; >> >> + // Note: We initialize collation here since this is called in BDS, when >> we are likely >> + // to have the Unicode Collation protocols available. >> + if (!Ext4IsCollationInitialized ()) { >> + Status = Ext4InitialiseUnicodeCollation (BindingProtocol->ImageHandle); >> + if (EFI_ERROR (Status)) { >> + // Lets throw a loud error into the log >> + // It is very unlikely something like this may fire out of the blue. >> Chances are either >> + // the platform configuration is wrong, or we are. >> + DEBUG ((DEBUG_ERROR, "[ext4] Error: Unicode Collation not available - >> failure to Start() - error %r\n", Status)); >> + goto Error; >> + } >> + } >> + > > This should work, although I'm not sure I see the point of the extra check.
Thanks for checking! Which extra check, IsInit? It’s just a weirdly abstract shortcut to not go through protocol lookup again when it already succeeded before. We can probably merge it as-is for now. Though, would you mind commenting on this: https://edk2.groups.io/g/devel/message/100348 (the first part, about static linking)? Thanks! Best regards, Marvin > > Acked-by: Ard Biesheuvel <a...@kernel.org> > >> Status = gBS->OpenProtocol ( >> ControllerHandle, >> &gEfiDiskIoProtocolGuid, >> @@ -774,7 +779,7 @@ Ext4Bind ( >> goto Error; >> } >> >> - DEBUG ((DEBUG_INFO, "[Ext4] Controller supports DISK_IO\n")); >> + DEBUG ((DEBUG_INFO, "[ext4] Controller supports DISK_IO\n")); >> >> Status = gBS->OpenProtocol ( >> ControllerHandle, >> @@ -787,7 +792,7 @@ Ext4Bind ( >> // It's okay to not support DISK_IO2 >> >> if (DiskIo2 != NULL) { >> - DEBUG ((DEBUG_INFO, "[Ext4] Controller supports DISK_IO2\n")); >> + DEBUG ((DEBUG_INFO, "[ext4] Controller supports DISK_IO2\n")); >> } >> >> Status = gBS->OpenProtocol ( >> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h >> b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h >> index 786e20f49fe1..93f6cf01fe06 100644 >> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h >> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h >> @@ -947,6 +947,17 @@ Ext4InitialiseUnicodeCollation ( >> EFI_HANDLE DriverHandle >> ); >> >> +/** >> + Check if unicode collation is initialized >> + >> + @retval TRUE if Ext4InitialiseUnicodeCollation() was already called >> successfully >> + @retval FALSE if Ext4InitialiseUnicodeCollation() was not yet called >> successfully >> +**/ >> +BOOLEAN >> +Ext4IsCollationInitialized ( >> + VOID >> + ); >> + >> /** >> Does a case-insensitive string comparison. Refer to >> EFI_UNICODE_COLLATION_PROTOCOL's StriColl for more details. >> -- >> 2.39.2 >> -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#100361): https://edk2.groups.io/g/devel/message/100361 Mute This Topic: https://groups.io/mt/97037199/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
