From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout01.posteo.de (mout01.posteo.de [185.67.36.65]) by mx.groups.io with SMTP id smtpd.web11.8302.1676880049444015206 for ; Mon, 20 Feb 2023 00:00:50 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@posteo.de header.s=2017 header.b=mrXZQqXc; spf=pass (domain: posteo.de, ip: 185.67.36.65, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [185.67.36.169]) by mout01.posteo.de (Postfix) with ESMTPS id C1E9724002F for ; Mon, 20 Feb 2023 09:00:47 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1676880047; bh=WVUG4cJWskXKXL9qmQ7LyZzARyG+f7i857dZp6DjQvw=; h=From:Subject:Date:Cc:To:From; b=mrXZQqXcK2U2NfmVzSl1JTSg4IiER/1AJrSsQirySm0CtHH0I8zCLsLvyjxJpUqku KB49CAdOJdLG7ygDxTAk4KGLrg/amiwF7VcxBV/Re2FeagfJBW+k4w5x5CxHXSNgzK bB18if+TpbTnk4WCMt+tk8disEgIE5tWacsgOggcnpdb/0oD5Fhmeze9GeCZUy6SW2 A4dZPhKcYn56g4SKUiitHnod023zb44BxgsUEofO476UEE0zwp1hECNZfSysy5bBbe tcJd0YydLtBY8a0OzHO1EM6HIUlRRNbmcfHEdEUgqMyXX/L5B89SbkFoE0AoLxZxud wb4nC1SwJGxyg== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4PKvxB5nKMz6tmq; Mon, 20 Feb 2023 09:00:46 +0100 (CET) From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= Mime-Version: 1.0 (1.0) Subject: Re: [PATCH 1/1] Ext4Pkg: Move unicode collation initialization to Start() Date: Mon, 20 Feb 2023 08:00:46 +0000 Message-Id: <549E455D-0A81-40BB-A944-9F79EE17248F@posteo.de> References: Cc: Pedro Falcato , devel@edk2.groups.io In-Reply-To: To: Ard Biesheuvel Content-Type: multipart/alternative; boundary=Apple-Mail-529698DC-3589-425B-8CAF-D20BB26100DF Content-Transfer-Encoding: 7bit --Apple-Mail-529698DC-3589-425B-8CAF-D20BB26100DF Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable > On 20. Feb 2023, at 08:52, Ard Biesheuvel wrote: >=20 > =EF=BB=BFOn Fri, 17 Feb 2023 at 21:14, Pedro Falcato wrote: >>=20 >> 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. >>=20 >> [1]: https://edk2.groups.io/g/devel/message/100312 >>=20 >> Cc: Ard Biesheuvel >> Cc: Marvin H=C3=A4user >> Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.") >> Signed-off-by: Pedro Falcato >> --- >> RFC-ish patch that implements one of the possibilities discussed in [1]. >> Please test and give feedback. >>=20 >> Odd that this issue was never caught in any other platform. Do the RPi pl= atforms have >> something funky going on? >>=20 >=20 > It could be something as simple as the order the drivers appear in the > .FDF file. >=20 >> 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(-) >>=20 >> diff --git a/Features/Ext4Pkg/Ext4Dxe/Collation.c b/Features/Ext4Pkg/Ext4= Dxe/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 >>=20 >> - 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. >>=20 >> SPDX-License-Identifier: BSD-2-Clause-Patent >> @@ -9,6 +9,7 @@ >>=20 >> #include >>=20 >> +#include >> #include >> #include >> #include >> @@ -169,5 +170,20 @@ Ext4StrCmpInsensitive ( >> IN CHAR16 *Str2 >> ) >> { >> + ASSERT (gUnicodeCollationInterface !=3D NULL); >> return gUnicodeCollationInterface->StriColl (gUnicodeCollationInterface= , Str1, Str2); >> } >> + >> +/** >> + Check if unicode collation is initialized >> + >> + @retval TRUE if Ext4InitialiseUnicodeCollation() was already called s= uccessfully >> + @retval FALSE if Ext4InitialiseUnicodeCollation() was not yet called s= uccessfully >> +**/ >> +BOOLEAN >> +Ext4IsCollationInitialized ( >> + VOID >> + ) >> +{ >> + return gUnicodeCollationInterface !=3D NULL; >> +} >> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c b/Features/Ext4Pkg/Ext4Dx= e/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 >>=20 >> - Copyright (c) 2021 Pedro Falcato All rights reserved. >> + Copyright (c) 2021 - 2023 Pedro Falcato All rights reserved. >> SPDX-License-Identifier: BSD-2-Clause-Patent >> **/ >>=20 >> @@ -513,26 +513,18 @@ Ext4EntryPoint ( >> IN EFI_SYSTEM_TABLE *SystemTable >> ) >> { >> - EFI_STATUS Status; >> - >> - Status =3D 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 >> + ); >> } >>=20 >> /** >> @@ -761,6 +753,19 @@ Ext4Bind ( >> BlockIo =3D NULL; >> DiskIo =3D NULL; >>=20 >> + // Note: We initialize collation here since this is called in BDS, whe= n we are likely >> + // to have the Unicode Collation protocols available. >> + if (!Ext4IsCollationInitialized ()) { >> + Status =3D Ext4InitialiseUnicodeCollation (BindingProtocol->ImageHan= dle); >> + if (EFI_ERROR (Status)) { >> + // Lets throw a loud error into the log >> + // It is very unlikely something like this may fire out of the blu= e. Chances are either >> + // the platform configuration is wrong, or we are. >> + DEBUG ((DEBUG_ERROR, "[ext4] Error: Unicode Collation not availabl= e - failure to Start() - error %r\n", Status)); >> + goto Error; >> + } >> + } >> + >=20 > This should work, although I'm not sure I see the point of the extra check= . Thanks for checking! Which extra check, IsInit? It=E2=80=99s just a weirdly a= bstract shortcut to not go through protocol lookup again when it already suc= ceeded 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 s= tatic linking)? Thanks! Best regards, Marvin >=20 > Acked-by: Ard Biesheuvel >=20 >> Status =3D gBS->OpenProtocol ( >> ControllerHandle, >> &gEfiDiskIoProtocolGuid, >> @@ -774,7 +779,7 @@ Ext4Bind ( >> goto Error; >> } >>=20 >> - DEBUG ((DEBUG_INFO, "[Ext4] Controller supports DISK_IO\n")); >> + DEBUG ((DEBUG_INFO, "[ext4] Controller supports DISK_IO\n")); >>=20 >> Status =3D gBS->OpenProtocol ( >> ControllerHandle, >> @@ -787,7 +792,7 @@ Ext4Bind ( >> // It's okay to not support DISK_IO2 >>=20 >> if (DiskIo2 !=3D NULL) { >> - DEBUG ((DEBUG_INFO, "[Ext4] Controller supports DISK_IO2\n")); >> + DEBUG ((DEBUG_INFO, "[ext4] Controller supports DISK_IO2\n")); >> } >>=20 >> Status =3D gBS->OpenProtocol ( >> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dx= e/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 >> ); >>=20 >> +/** >> + Check if unicode collation is initialized >> + >> + @retval TRUE if Ext4InitialiseUnicodeCollation() was already called s= uccessfully >> + @retval FALSE if Ext4InitialiseUnicodeCollation() was not yet called s= uccessfully >> +**/ >> +BOOLEAN >> +Ext4IsCollationInitialized ( >> + VOID >> + ); >> + >> /** >> Does a case-insensitive string comparison. Refer to >> EFI_UNICODE_COLLATION_PROTOCOL's StriColl for more details. >> -- >> 2.39.2 >>=20 --Apple-Mail-529698DC-3589-425B-8CAF-D20BB26100DF Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: quoted-printable
On 20. Feb 2023, at 08:52,= Ard Biesheuvel <ardb@kernel.org> wrote:

=EF=BB=BFOn Fri, 17 Feb 2023 a= t 21:14, Pedro Falcato <pedro.falcato@gmail.com> wrote:

There have been reports[1] of failures to boot due to unicode collat= ion
protocols not bei= ng available at Ext4Dxe load time. Therefore, attempt
to initialize unicode collation at Start() t= ime, like done previously in
FatPkg/EnhancedFatDxe. By doing so, we move collation initialization=
to BDS, where the mo= dule 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=C3=A4user= <mhaeuser@posteo.de>
Fixes: d9ceedca6c8f ("Ext4Pkg: Add Ext4Dxe driver.")
Signed-off-by: Pedro Falcato <pedr= o.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 o= ther platform. Do the RPi platforms have
something funky going on?


It c= ould 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, 5= 6 insertions(+), 24 deletions(-)

diff --g= it a/Features/Ext4Pkg/Ext4Dxe/Collation.c b/Features/Ext4Pkg/Ext4Dxe/Collati= on.c
index 91d172b1cb= 89..65c34c33ea93 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Collation.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Collation.c
@@ -1,7 +1,7 @@
/** @file
=
  Unicode collation routines

<= blockquote type=3D"cite">-  Copyright (c) 2021 Pedro Falcato All r= ights reserved.
+ &nb= sp;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/UefiBootServicesTable= Lib.h>
#include &= lt;Library/MemoryAllocationLib.h>
@@ -169,5 +170,20 @@ Ext4StrCmpInsensitive (
  IN CHAR16  *Str2
  )<= br>
{
+  ASSERT (gUnicodeCollationInterface !=3D= NULL);
  = return gUnicodeCollationInterface->StriColl (gUnicodeCollationInterface, S= tr1, Str2);
}=
+
+/**
+   Check if unicode collation is initialized<= br>
+
+   @retval TRUE if Ext4InitialiseUni= codeCollation() was already called successfully
+   @retval FALSE if Ext4InitialiseUnic= odeCollation() was not yet called successfully
+**/
+BOOLEAN
+E= xt4IsCollationInitialized (
+  VOID
+=  )
+{
+  return gUnicodeCollati= onInterface !=3D NULL;
+}
diff --git a/Fea= tures/Ext4Pkg/Ext4Dxe/Ext4Dxe.c b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c<= br>
index 2a4f5a7bd0ef..de0d633f= ebfb 100644
--- a/Fea= tures/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
<= blockquote type=3D"cite">@@ -1,7 +1,7 @@
/** @file
  Driver entry point

-  Copyright (c) 2021 Pedro Falcato All rights reserved.<= br>
+  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
 &= nbsp;)
{
<= /blockquote>
-  EFI_STATUS  Status;=
-
-  Status =3D EfiLibInstallAllDriv= erProtocols2 (
- &nbs= p;           ImageHan= dle,
-   &n= bsp;         SystemTable,
-     =         &gExt4BindingProtocol,
-    &n= bsp;        ImageHandle,
<= /blockquote>
-      =        &gExt4ComponentName,
-     &nbs= p;       &gExt4ComponentName2,=
-     &= nbsp;       NULL,
=
-        =      NULL,
-           &= nbsp; NULL,
- &n= bsp;           NULL
-    &n= bsp;        );
-
-  if (EFI_ERROR (Status)) {
-    return Status;
-  }
-
-  return Ext4InitialiseUnicodeCollation (ImageHandle);
+  return EfiLibInsta= llAllDriverProtocols2 (
+           ImageHandle= ,
+    = ;       SystemTable,
+       &nb= sp;   &gExt4BindingProtocol,
+         = ;  ImageHandle,
<= span>+           &gExt= 4ComponentName,
+ &nb= sp;         &gExt4Component= Name2,
+   =         NULL,
+        = ;   NULL,
+           NULL,
+     = ;      NULL
+          = ; );
}

/**
@@ -761,6 +753,19 @@ Ext4Bind (
  BlockIo =3D NULL;
=
  DiskIo  =3D NULL;

+  // Note: We initialize collation here sin= ce this is called in BDS, when we are likely
+  // to have the Unicode Collation protocols a= vailable.
+  if (= !Ext4IsCollationInitialized ()) {
+    Status =3D Ext4InitialiseUnicodeCollation (= BindingProtocol->ImageHandle);
+    if (EFI_ERROR (Status)) {
+      // Let= s throw a loud error into the log
+      // It is very unlikely somethin= g like this may fire out of the blue. Chances are either
+      // the p= latform configuration is wrong, or we are.
+      DEBUG ((DEBUG_ERROR, "= [ext4] Error: Unicode Collation not available - failure to Start() - error %= r\n", Status));
+ &nb= sp;    goto Error;
+    }
+  }
+

This should work, al= though I'm not sure I see the point of the extra check.

Thanks for checking! Which extra check, IsInit?= It=E2=80=99s just a weirdly abstract shortcut to not go through protocol lo= okup 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 lin= king)? Thanks!

Best regards,
Marvin
=

Acked-= by: Ard Biesheuvel <ardb@kernel.org>

  Status =3D gBS->OpenProtocol (
    = ;            &nb= sp; ControllerHandle,
=            &nb= sp;      &gEfiDiskIoProtocolGuid,
@@ -774,7 +779,7 @@ Ext4Bind (=
   &= nbsp;goto Error;
&nb= sp; }
-  DEBUG ((DEBUG_INFO, "[= Ext4] Controller supports DISK_IO\n"));
+  DEBUG ((DEBUG_INFO, "[ext4] Controller supports D= ISK_IO\n"));
<= br>
  Status =3D gBS-= >OpenProtocol (
&= nbsp;            = ;     ControllerHandle,
@@ -787,7 +792,7 @@ Ext4Bind (
  // It's okay to not suppo= rt DISK_IO2
  if (DiskIo2 !=3D= NULL) {
-  &nbs= p; DEBUG ((DEBUG_INFO, "[Ext4] Controller supports DISK_IO2\n"));
+    DEBUG (= (DEBUG_INFO, "[ext4] Controller supports DISK_IO2\n"));
  }

  Status =3D gBS->OpenProtocol (
diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext= 4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
index 786e20f49fe1..93f6cf01fe06 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext= 4Dxe.h
+++ b/Features= /Ext4Pkg/Ext4Dxe/Ext4Dxe.h
= @@ -947,6 +947,17 @@ Ext4InitialiseUnicodeCollation (
  EFI_HANDLE  DriverH= andle
  );=

+/**
+   Check if unicode collation is initialized
+
+   @retval TRUE if Ext4Initial= iseUnicodeCollation() was already called successfully
+   @retval FALSE if Ext4Initiali= seUnicodeCollation() was not yet called successfully
=
+**/
+BOOLEAN
+Ext4IsCollationInitialized (
+  VOID
<= span>+  );
+
/**
   Does a case-insensiti= ve string comparison. Refer to
EFI_UNICODE_COLLATION_PROTOCOL's StriColl for more details.
--
=
2.39.2

= --Apple-Mail-529698DC-3589-425B-8CAF-D20BB26100DF--