[PATCH 0/2] Fix dynamic command cannot start in boot

[PATCH 0/2] Fix dynamic command cannot start in boot

[Ruiyu Ni]

When dynamic command drivers are built into FV and start during
boot, they fails. Because Shell protocol doesn't exist during boot.
The patch changes both ShellLib and dynamic command drivers to fix
the bug.

Ruiyu Ni (2):
  ShellPkg/ShellLib: Fix dynamic command fails to start during boot
  ShellPkg/DynamicCommand: Fix bug that cannot start in boot

 .../DpDynamicCommand/DpDynamicCommand.c            |  1 +
 .../TftpDynamicCommand/TftpDynamicCommand.c        |  1 +
 ShellPkg/Library/UefiShellLib/UefiShellLib.c       | 89 +++++++++++++---------
 ShellPkg/ShellPkg.dsc                              |  7 +-
 4 files changed, 60 insertions(+), 38 deletions(-)

-- 
2.15.0.gvfs.1.preview.4

[PATCH 1/2] ShellPkg/ShellLib: Fix dynamic command fails to start during boot

[Ruiyu Ni]

The previous change in ShellLib: "commit
3d29f8c5e361525a0d2accfa7f5bb0a7210b8927
* ShellPkg/ShellLib: Constructor doesn't depend on ShellParameters"
resolved the issue when loading dynamic command driver from Shell
environment.
But when dynamic command driver is built into FV and started during
boot, the driver still fails to start because Shell protocol doesn't
exist at that time.

The patch changes ShellLib to:
1. Do not look for Shell and ShellParameters protocol when they are
   non-NULL in ShellLibConstructorWorker();
   The two protocols are assumed to be set by DynamicCommand.Handler.
   When ShellInitialize() is called in DynamicCommand.Handler, this
   change can prevent the two protocols to be changed to NULL by
   the locating logic.
2. Do not reset the Shell and ShellParameters protocol to NULL in
   ShellLibDestructor() when CloseProtocol() fails;
   Dynamic command driver needs to set the PcdShellLibAutoInitialize
   to FALSE in order to skip the constructor.
   Current logic calls ShellLibDestructor() when the PCD is FALSE when
   ShellInitialize() is called. The change prevent the two protocols
   to be changed to NULL.

The two changes don't impact existing usage case so they are backward
compatible.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jaben Carsey <jaben.carsey@intel.com>
---
 ShellPkg/Library/UefiShellLib/UefiShellLib.c | 89 ++++++++++++++++------------
 1 file changed, 52 insertions(+), 37 deletions(-)

diff --git a/ShellPkg/Library/UefiShellLib/UefiShellLib.c b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
index 00f58ca0c1..7f6389f655 100644
--- a/ShellPkg/Library/UefiShellLib/UefiShellLib.c
+++ b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
@@ -179,40 +179,45 @@ ShellLibConstructorWorker (
 {
   EFI_STATUS  Status;
 
-  //
-  // UEFI 2.0 shell interfaces (used preferentially)
-  //
-  Status = gBS->OpenProtocol(
-    ImageHandle,
-    &gEfiShellProtocolGuid,
-    (VOID **)&gEfiShellProtocol,
-    ImageHandle,
-    NULL,
-    EFI_OPEN_PROTOCOL_GET_PROTOCOL
-   );
-  if (EFI_ERROR(Status)) {
+  if (gEfiShellProtocol == NULL) {
     //
-    // Search for the shell protocol
+    // UEFI 2.0 shell interfaces (used preferentially)
     //
-    Status = gBS->LocateProtocol(
+    Status = gBS->OpenProtocol (
+      ImageHandle,
       &gEfiShellProtocolGuid,
+      (VOID **)&gEfiShellProtocol,
+      ImageHandle,
       NULL,
-      (VOID **)&gEfiShellProtocol
-     );
-    if (EFI_ERROR(Status)) {
-      gEfiShellProtocol = NULL;
+      EFI_OPEN_PROTOCOL_GET_PROTOCOL
+    );
+    if (EFI_ERROR (Status)) {
+      //
+      // Search for the shell protocol
+      //
+      Status = gBS->LocateProtocol (
+        &gEfiShellProtocolGuid,
+        NULL,
+        (VOID **)&gEfiShellProtocol
+      );
+      if (EFI_ERROR (Status)) {
+        gEfiShellProtocol = NULL;
+      }
     }
   }
-  Status = gBS->OpenProtocol(
-    ImageHandle,
-    &gEfiShellParametersProtocolGuid,
-    (VOID **)&gEfiShellParametersProtocol,
-    ImageHandle,
-    NULL,
-    EFI_OPEN_PROTOCOL_GET_PROTOCOL
-   );
-  if (EFI_ERROR(Status)) {
-    gEfiShellParametersProtocol = NULL;
+
+  if (gEfiShellParametersProtocol == NULL) {
+    Status = gBS->OpenProtocol (
+      ImageHandle,
+      &gEfiShellParametersProtocolGuid,
+      (VOID **)&gEfiShellParametersProtocol,
+      ImageHandle,
+      NULL,
+      EFI_OPEN_PROTOCOL_GET_PROTOCOL
+    );
+    if (EFI_ERROR (Status)) {
+      gEfiShellParametersProtocol = NULL;
+    }
   }
 
   if (gEfiShellProtocol == NULL) {
@@ -324,35 +329,45 @@ ShellLibDestructor (
   IN EFI_SYSTEM_TABLE  *SystemTable
   )
 {
+  EFI_STATUS           Status;
+
   if (mEfiShellEnvironment2 != NULL) {
-    gBS->CloseProtocol(mEfiShellEnvironment2Handle==NULL?ImageHandle:mEfiShellEnvironment2Handle,
+    Status = gBS->CloseProtocol(mEfiShellEnvironment2Handle==NULL?ImageHandle:mEfiShellEnvironment2Handle,
                        &gEfiShellEnvironment2Guid,
                        ImageHandle,
                        NULL);
-    mEfiShellEnvironment2 = NULL;
+    if (!EFI_ERROR (Status)) {
+      mEfiShellEnvironment2       = NULL;
+      mEfiShellEnvironment2Handle = NULL;
+    }
   }
   if (mEfiShellInterface != NULL) {
-    gBS->CloseProtocol(ImageHandle,
+    Status = gBS->CloseProtocol(ImageHandle,
                        &gEfiShellInterfaceGuid,
                        ImageHandle,
                        NULL);
-    mEfiShellInterface = NULL;
+    if (!EFI_ERROR (Status)) {
+      mEfiShellInterface = NULL;
+    }
   }
   if (gEfiShellProtocol != NULL) {
-    gBS->CloseProtocol(ImageHandle,
+    Status = gBS->CloseProtocol(ImageHandle,
                        &gEfiShellProtocolGuid,
                        ImageHandle,
                        NULL);
-    gEfiShellProtocol = NULL;
+    if (!EFI_ERROR (Status)) {
+      gEfiShellProtocol = NULL;
+    }
   }
   if (gEfiShellParametersProtocol != NULL) {
-    gBS->CloseProtocol(ImageHandle,
+    Status = gBS->CloseProtocol(ImageHandle,
                        &gEfiShellParametersProtocolGuid,
                        ImageHandle,
                        NULL);
-    gEfiShellParametersProtocol = NULL;
+    if (!EFI_ERROR (Status)) {
+      gEfiShellParametersProtocol = NULL;
+    }
   }
-  mEfiShellEnvironment2Handle = NULL;
 
   return (EFI_SUCCESS);
 }
-- 
2.15.0.gvfs.1.preview.4

[PATCH 2/2] ShellPkg/DynamicCommand: Fix bug that cannot start in boot

[Ruiyu Ni]

When dynamic command drivers are built into FV and start during
boot, they fails. Because Shell protocol doesn't exist during boot.
The patch sets Shell protocol and also set PcdShellLibAutoInitialize
to FALSE to ensure that
1. Shell protocol check doesn't happen in driver's entry point.
2. Driver can get the Shell protocol in DynamicCommand.Handler().

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jaben Carsey <jaben.carsey@intel.com>
---
 ShellPkg/DynamicCommand/DpDynamicCommand/DpDynamicCommand.c     | 1 +
 ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.c | 1 +
 ShellPkg/ShellPkg.dsc                                           | 7 ++++++-
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/ShellPkg/DynamicCommand/DpDynamicCommand/DpDynamicCommand.c b/ShellPkg/DynamicCommand/DpDynamicCommand/DpDynamicCommand.c
index 6f3997fff4..b10c59f49c 100644
--- a/ShellPkg/DynamicCommand/DpDynamicCommand/DpDynamicCommand.c
+++ b/ShellPkg/DynamicCommand/DpDynamicCommand/DpDynamicCommand.c
@@ -38,6 +38,7 @@ DpCommandHandler (
   )
 {
   gEfiShellParametersProtocol = ShellParameters;
+  gEfiShellProtocol           = Shell;
   return RunDp (gImageHandle, SystemTable);
 }
 
diff --git a/ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.c b/ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.c
index 928ef08468..9e6489dd6f 100644
--- a/ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.c
+++ b/ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.c
@@ -39,6 +39,7 @@ TftpCommandHandler (
   )
 {
   gEfiShellParametersProtocol = ShellParameters;
+  gEfiShellProtocol           = Shell;
   return RunTftp (gImageHandle, SystemTable);
 }
 
diff --git a/ShellPkg/ShellPkg.dsc b/ShellPkg/ShellPkg.dsc
index 65e8959455..86382139a5 100644
--- a/ShellPkg/ShellPkg.dsc
+++ b/ShellPkg/ShellPkg.dsc
@@ -120,9 +120,14 @@ [Components]
 !endif #$(NO_SHELL_PROFILES)
   }
 
-  ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
+  ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf {
+    <PcdsFixedAtBuild>
+      gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
+  }
   ShellPkg/DynamicCommand/TftpDynamicCommand/TftpApp.inf
   ShellPkg/DynamicCommand/DpDynamicCommand/DpDynamicCommand.inf {
+    <PcdsFixedAtBuild>
+      gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
     <LibraryClasses>
       PerformanceLib|MdeModulePkg/Library/DxeSmmPerformanceLib/DxeSmmPerformanceLib.inf
   }
-- 
2.15.0.gvfs.1.preview.4

Re: [PATCH 0/2] Fix dynamic command cannot start in boot

[Carsey, Jaben]

Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Ruiyu Ni
> Sent: Tuesday, November 28, 2017 4:02 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [PATCH 0/2] Fix dynamic command cannot start in boot
> Importance: High
> 
> When dynamic command drivers are built into FV and start during
> boot, they fails. Because Shell protocol doesn't exist during boot.
> The patch changes both ShellLib and dynamic command drivers to fix
> the bug.
> 
> Ruiyu Ni (2):
>   ShellPkg/ShellLib: Fix dynamic command fails to start during boot
>   ShellPkg/DynamicCommand: Fix bug that cannot start in boot
> 
>  .../DpDynamicCommand/DpDynamicCommand.c            |  1 +
>  .../TftpDynamicCommand/TftpDynamicCommand.c        |  1 +
>  ShellPkg/Library/UefiShellLib/UefiShellLib.c       | 89 +++++++++++++---------
>  ShellPkg/ShellPkg.dsc                              |  7 +-
>  4 files changed, 60 insertions(+), 38 deletions(-)
> 
> --
> 2.15.0.gvfs.1.preview.4
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel