Re: [edk2-devel] [PATCH v6] MdeModulePkg/Universal/StatusCodeHandler: Fix a bug about log lost

["Tan, Ming" <ming.tan@intel.com>]

Liming & Dandan:
  If there is not new concern, would you like to review and submit this patch?

  Thank you.
  Tan Ming.

-----Original Message-----
From: Tan, Ming 
Sent: Tuesday, December 22, 2020 8:56 AM
To: gaoliming@byosoft.com.cn; Bi, Dandan <dandan.bi@intel.com>
Cc: devel@edk2.groups.io
Subject: RE: [edk2-devel] [PATCH v6] MdeModulePkg/Universal/StatusCodeHandler: Fix a bug about log lost

Liming:
  If remove the UnregisterSerialBootTimeHandlers(), then in SerialStatusCodeWorker.c, need use an external variable mRscHandlerProtocol which is defined in StatusCodeHandlerRuntimeDxe.c, or call gBS->LocateProtocol to get it again.
  Compare with export add an external variable, add an external function seems better.

  BR/Tan Ming.

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of gaoliming
Sent: Tuesday, December 22, 2020 8:36 AM
To: devel@edk2.groups.io; Tan, Ming <ming.tan@intel.com>; Bi, Dandan <dandan.bi@intel.com>
Subject: 回复: [edk2-devel] [PATCH v6] MdeModulePkg/Universal/StatusCodeHandler: Fix a bug about log lost

Ming:
  I have no concern for the updated patch. 

  I just think we can simplify the code logic and remove the function UnregisterSerialBootTimeHandlers(). Unregister logic can be moved to SerialStatusCodeReportWorker(). 

  You can choose your preferred implementation. Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>

Thanks
Liming
> -----邮件原件-----
> 发件人: bounce+27952+69302+4905953+8761045@groups.io
> <bounce+27952+69302+4905953+8761045@groups.io> 代表 Tan, Ming
> 发送时间: 2020年12月21日 15:18
> 收件人: Bi, Dandan <dandan.bi@intel.com>; Liming Gao 
> <gaoliming@byosoft.com.cn>
> 抄送: devel@edk2.groups.io
> 主题: Re: [edk2-devel] [PATCH v6]
> MdeModulePkg/Universal/StatusCodeHandler: Fix a bug about log lost
> 
> Liming:
>   I already submit a new patch for it, is there any new concern for it?
> 
>   Thank you.
>   Tan Ming.
> 
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Tan, 
> Ming
> Sent: Friday, December 18, 2020 12:51 PM
> To: devel@edk2.groups.io
> Cc: Bi, Dandan <dandan.bi@intel.com>; Liming Gao 
> <gaoliming@byosoft.com.cn>
> Subject: [edk2-devel] [PATCH v6]
> MdeModulePkg/Universal/StatusCodeHandler: Fix a bug about log lost
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3126
> 
> 1. If use PeiDxeDebugLibReportStatusCode as DebugLib, then some logs 
> after
> ExitBootService() will be lost.
> 2. The root cause:
> 2.1 The original code will register an unregister function of 
> gEfiEventExitBootServicesGuid, this unregister function will call 
> EFI_RSC_HANDLER_PROTOCOL->Unregister and does not support log through 
> serial port.
> 2.2 And some other drivers also register call back funtions of 
> gEfiEventExitBootServicesGuid.
> 2.3 Then after the unregister function is called, other call back
functions can't
> out log if them use RSC as DebugLib.
> 3. The DxeMain will report status code EFI_SW_BS_PC_EXIT_BOOT_SERVICES 
> after notify all the call back functions of gEfiEventExitBootServicesGuid.
> 4. Solution: the StatusCodeHandlerRuntimeDxe.c will not register an 
> unregister function of gEfiEventExitBootServicesGuid, but unregister 
> it
after
> receive the status code of EFI_SW_BS_PC_EXIT_BOOT_SERVICES.
> 
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Signed-off-by: Ming Tan <ming.tan@intel.com>
> ---
> V6: Change the funcname of UnregisterBootTimeHandlers to 
> UnregisterSerialBootTimeHandlers.
> V5: Fix an unused var bug and compile error using GCC.
> V4: Fix a spell bug in code comment, change 'a' to 'an' before
'unregister'.
> V3: Fix a spell bug in commit message, change 'a' to 'an' before
'unregister'.
> V2: Add the REF link in commit message.
> 
>  .../RuntimeDxe/SerialStatusCodeWorker.c       | 10 ++++++++++
>  .../RuntimeDxe/StatusCodeHandlerRuntimeDxe.c  | 19
> ++-----------------  .../RuntimeDxe/StatusCodeHandlerRuntimeDxe.h  |
> ++11
> +++++++++++
>  3 files changed, 23 insertions(+), 17 deletions(-)
> 
> diff --git
> a/MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/SerialStatusCo
> deWorker.c
> b/MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/SerialStatusCo
> deWorker.c
> index 0b98e7ec63..348f55edba 100644
> ---
> a/MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/SerialStatusCo
> deWorker.c
> +++
> b/MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/SerialStatusCo
> +++ deWorker.c
> @@ -151,6 +151,16 @@ SerialStatusCodeReportWorker (
>    //   SerialPortWrite ((UINT8 *) Buffer, CharCount); +  //+  // If
register
> an unregister function of gEfiEventExitBootServicesGuid,+  // then 
> some
log
> called in ExitBootServices() will be lost,+  // so unregister the 
> handler
after
> receive the value of exit boot service.+  //+  if ((CodeType &
> EFI_STATUS_CODE_TYPE_MASK) == EFI_PROGRESS_CODE &&+      Value
> == (EFI_SOFTWARE_EFI_BOOT_SERVICE |
> EFI_SW_BS_PC_EXIT_BOOT_SERVICES)) {+
> UnregisterSerialBootTimeHandlers();+  }+   return EFI_SUCCESS; } diff
--git
> a/MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHa
> ndlerRuntimeDxe.c
> b/MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHa
> ndlerRuntimeDxe.c
> index a8c0fe5b71..d50335af8a 100644
> ---
> a/MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHa
> ndlerRuntimeDxe.c
> +++
> b/MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHa
> nd
> +++ lerRuntimeDxe.c
> @@ -10,23 +10,17 @@
>  #include "StatusCodeHandlerRuntimeDxe.h"  EFI_EVENT 
> mVirtualAddressChangeEvent = NULL;-static EFI_EVENT
> mExitBootServicesEvent     = NULL; EFI_RSC_HANDLER_PROTOCOL
> *mRscHandlerProtocol       = NULL;  /**   Unregister status code
> callback functions only available at boot time from   report status code
> router when exiting boot services. -  @param  Event         Event
> whose notification function is being invoked.-  @param  Context 
> Pointer to the notification function's context, which is- always zero 
> in current implementation.- **/ VOID
> EFIAPI-UnregisterBootTimeHandlers (-  IN EFI_EVENT        Event,-  IN
> VOID             *Context+UnregisterSerialBootTimeHandlers (+
> VOID   ) {   if (PcdGetBool (PcdStatusCodeUseSerial)) {@@ -178,15 +172,6
> @@ StatusCodeHandlerRuntimeDxeEntry (
>      mRscHandlerProtocol->Register (RtMemoryStatusCodeReportWorker,
> TPL_HIGH_LEVEL);   } -  Status = gBS->CreateEventEx (-
> EVT_NOTIFY_SIGNAL,-                  TPL_NOTIFY,-
> UnregisterBootTimeHandlers,-                  NULL,-
> &gEfiEventExitBootServicesGuid,-
> &mExitBootServicesEvent-                  );-   Status =
> gBS->CreateEventEx (                   EVT_NOTIFY_SIGNAL,
> TPL_NOTIFY,diff --git
> a/MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHa
> ndlerRuntimeDxe.h
> b/MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHa
> ndlerRuntimeDxe.h
> index fd4689c2d7..a2cf2ae0b7 100644
> ---
> a/MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHa
> ndlerRuntimeDxe.h
> +++
> b/MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHa
> nd
> +++ lerRuntimeDxe.h
> @@ -118,4 +118,15 @@ RtMemoryStatusCodeReportWorker (
>    IN EFI_STATUS_CODE_DATA               *Data OPTIONAL   );
> +/**+  Unregister status code callback functions only available at 
> +boot
time
> from+  report status code router when exiting boot
> services.++**/+VOID+EFIAPI+UnregisterSerialBootTimeHandlers (+
> VOID+  );+ #endif--
> 2.24.0.windows.2
> 
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#69173): 
> https://edk2.groups.io/g/devel/message/69173
> Mute This Topic: https://groups.io/mt/79055338/2557430
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ming.tan@intel.com] 
> -=-=-=-=-=-=
> 
> 
> 
> 
> 
>