[PATCH 1/1] Ext4Pkg: Move unicode collation initialization to Start()

[PATCH 1/1] Ext4Pkg: Move unicode collation initialization to Start()

[Pedro Falcato]

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 <ardb@kernel.org>
Cc: Marvin Häuser <mhaeuser@posteo.de>
Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.")
Signed-off-by: Pedro Falcato <pedro.falcato@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?

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

Re: [PATCH 1/1] Ext4Pkg: Move unicode collation initialization to Start()

[Marvin Häuser]

> On 17. Feb 2023, at 21:14, Pedro Falcato <pedro.falcato@gmail.com> wrote:
> 
> @@ -169,5 +170,20 @@ Ext4StrCmpInsensitive (
>   IN CHAR16  *Str2
>   )
> {
> +  ASSERT (gUnicodeCollationInterface != NULL);
>   return gUnicodeCollationInterface->StriColl (gUnicodeCollationInterface, Str1, Str2);
> }

Off-topic (should merge either way), but is there a reason such a function is not in BaseLib?
@Ard Do you happen to know?

> /**
> @@ -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 ()) {

Why do we need to expose this? I.e., why can't this be an internal decision of Ext4InitialiseUnicodeCollation()?

> +    Status = Ext4InitialiseUnicodeCollation (BindingProtocol->ImageHandle);

Re: [PATCH 1/1] Ext4Pkg: Move unicode collation initialization to Start()

[Pedro Falcato]

On Sat, Feb 18, 2023 at 12:31 PM Marvin Häuser <mhaeuser@posteo.de> wrote:
>
>
> > On 17. Feb 2023, at 21:14, Pedro Falcato <pedro.falcato@gmail.com> wrote:
> >
> > @@ -169,5 +170,20 @@ Ext4StrCmpInsensitive (
> >   IN CHAR16  *Str2
> >   )
> > {
> > +  ASSERT (gUnicodeCollationInterface != NULL);
> >   return gUnicodeCollationInterface->StriColl (gUnicodeCollationInterface, Str1, Str2);
> > }
>
> Off-topic (should merge either way), but is there a reason such a function is not in BaseLib?
> @Ard Do you happen to know?

Proper collation (and proper unicode handling in general) is AIUI
pretty hard and involves a lot of tables, etc.
So it makes total sense to me why one wants to "dynamically link" this
in using protocols instead of statically linking it everywhere.

Should be noted that EnglishDxe makes a mockery out of unicode
collation and does plain ASCII operations only :))

> > /**
> > @@ -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 ()) {
>
> Why do we need to expose this? I.e., why can't this be an internal decision of Ext4InitialiseUnicodeCollation()?
>
> > +    Status = Ext4InitialiseUnicodeCollation (BindingProtocol->ImageHandle);
>

I did think about that, but then decided against it since I felt that
initializing multiple times would make little logical sense.

-- 
Pedro

Re: [PATCH 1/1] Ext4Pkg: Move unicode collation initialization to Start()

[Marvin Häuser]

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


> On 18. Feb 2023, at 16:14, Pedro Falcato <pedro.falcato@gmail.com> wrote:
> 
> Proper collation (and proper unicode handling in general) is AIUI
> pretty hard and involves a lot of tables, etc.
> So it makes total sense to me why one wants to "dynamically link" this
> in using protocols instead of statically linking it everywhere.
> 
> Should be noted that EnglishDxe makes a mockery out of unicode
> collation and does plain ASCII operations only :))

Yes, and so does FatPei [1]. Also, EnglishDxe has a clear (and unvalidated!) assumption that FAT file names are strictly 7-bit ASCII [2]. Without doing any proper research, Wikipedia seems to suggest that even EASCII could be problematic for FAT file names. Under the assumption that FAT will only ever give us 7-bit ASCII file names, it's both unrealistic to assume someone will implement proper Unicode collation, but also to fear any observable deviations from FatDxe even if this was the case and Ext4Dxe used ASCII-only collation (because the extended Unicode collation support would be no-op w.r.t. FAT). The ASCII-only support is pretty tiny, it could live in BaseLib without a problem (and not be duplicated into FatPkg...).

In my opinion, merge this to fix the bug and at some later point change this to static linking (maybe alongside FatDxe, but definitely deduplicate FatPei).

[1] https://github.com/tianocore/edk2/tree/02fcfdce1e5ce86f1951191883e7e30de5aa08be/FatPei/FatLiteLib.c#L341-L365 <https://github.com/tianocore/edk2/blob/master/FatPkg/FatPei/FatLiteLib.c#L341-L365>

[2] https://github.com/tianocore/edk2/tree/02fcfdce1e5ce86f1951191883e7e30de5aa08be/MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/UnicodeCollationEng.c#L386-L406 <https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/UnicodeCollationEng.c#L386-L406>
> I did think about that, but then decided against it since I felt that
> initializing multiple times would make little logical sense.

I get it, but moving it inside would be more consistent with similar code and remove the risk for forgetting the check if it was ever used elsewhere (I know, it probably won't be, but this is a design question).

Best regards,
Marvin

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

Re: [PATCH 1/1] Ext4Pkg: Move unicode collation initialization to Start()

[Ard Biesheuvel]

On Fri, 17 Feb 2023 at 21:14, Pedro Falcato <pedro.falcato@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 <ardb@kernel.org>
> Cc: Marvin Häuser <mhaeuser@posteo.de>
> Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.")
> Signed-off-by: Pedro Falcato <pedro.falcato@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.

Acked-by: Ard Biesheuvel <ardb@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
>

Re: [PATCH 1/1] Ext4Pkg: Move unicode collation initialization to Start()

[Marvin Häuser]

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


> On 20. Feb 2023, at 08:52, Ard Biesheuvel <ardb@kernel.org> wrote:
> 
> On Fri, 17 Feb 2023 at 21:14, Pedro Falcato <pedro.falcato@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 <ardb@kernel.org>
>> Cc: Marvin Häuser <mhaeuser@posteo.de>
>> Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.")
>> Signed-off-by: Pedro Falcato <pedro.falcato@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 <ardb@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
>> 

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

Re: [PATCH 1/1] Ext4Pkg: Move unicode collation initialization to Start()

[Pedro Falcato]

?On Mon, Feb 20, 2023 at 8:00 AM Marvin Häuser <mhaeuser@posteo.de> wrote:
>
>
> On 20. Feb 2023, at 08:52, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Fri, 17 Feb 2023 at 21:14, Pedro Falcato <pedro.falcato@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 <ardb@kernel.org>
>
> Cc: Marvin Häuser <mhaeuser@posteo.de>
>
> Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.")
>
> Signed-off-by: Pedro Falcato <pedro.falcato@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

Marvin,

Do you want me to spin up a quick v2 without the
Ext4IsCollationInitialized? Just doing the check internally in
Ext4InitialiseUnicodeCollation.

-- 
Pedro

Re: [PATCH 1/1] Ext4Pkg: Move unicode collation initialization to Start()

[Marvin Häuser]

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


> On 21. Feb 2023, at 20:25, Pedro Falcato <pedro.falcato@gmail.com> wrote:
> 
> Marvin,
> 
> Do you want me to spin up a quick v2 without the
> Ext4IsCollationInitialized? Just doing the check internally in
> Ext4InitialiseUnicodeCollation.

It'd be my preferred solution, but it's just a detail.

Reviewed-by: Marvin Häuser <mhaeuser@posteo.de <mailto:mhaeuser@posteo.de>> in any case

> 
> -- 
> Pedro


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

Re: [PATCH 1/1] Ext4Pkg: Move unicode collation initialization to Start()

[Pedro Falcato]

On Tue, Feb 21, 2023 at 7:37 PM Marvin Häuser <mhaeuser@posteo.de> wrote:
>
>
> On 21. Feb 2023, at 20:25, Pedro Falcato <pedro.falcato@gmail.com> wrote:
>
> Marvin,
>
> Do you want me to spin up a quick v2 without the
> Ext4IsCollationInitialized? Just doing the check internally in
> Ext4InitialiseUnicodeCollation.
>
>
> It'd be my preferred solution, but it's just a detail.
>
> Reviewed-by: Marvin Häuser <mhaeuser@posteo.de> in any case
>
>
> --
> Pedro
>
>

Fixed it up on my end and removed the unanimously-odd
Ext4IsCollationInitialized.

Pushed as 54306d0.

Thank you both for the bug report and reviews!

-- 
Pedro