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

["Dandan Bi" <dandan.bi@intel.com>]

Reviewed-by: Dandan Bi <dandan.bi@intel.com> for V6, will create a pull request for it.


Thanks,
Dandan
> -----Original Message-----
> From: Tan, Ming <ming.tan@intel.com>
> Sent: Tuesday, December 22, 2020 9:26 AM
> To: gaoliming@byosoft.com.cn; Bi, Dandan <dandan.bi@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> devel@edk2.groups.io
> Subject: RE: [edk2-devel] [PATCH v6]
> MdeModulePkg/Universal/StatusCodeHandler: Fix a bug about log lost
> 
> 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/SerialStatusC
> o
> > deWorker.c
> >
> b/MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/SerialStatusC
> o
> > deWorker.c
> > index 0b98e7ec63..348f55edba 100644
> > ---
> >
> a/MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/SerialStatusC
> o
> > deWorker.c
> > +++
> >
> b/MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/SerialStatusC
> o
> > +++ 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/StatusCodeH
> a
> > ndlerRuntimeDxe.c
> >
> b/MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeH
> a
> > ndlerRuntimeDxe.c
> > index a8c0fe5b71..d50335af8a 100644
> > ---
> >
> a/MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeH
> a
> > ndlerRuntimeDxe.c
> > +++
> >
> b/MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeH
> a
> > 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/StatusCodeH
> a
> > ndlerRuntimeDxe.h
> >
> b/MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeH
> a
> > ndlerRuntimeDxe.h
> > index fd4689c2d7..a2cf2ae0b7 100644
> > ---
> >
> a/MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeH
> a
> > ndlerRuntimeDxe.h
> > +++
> >
> b/MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeH
> a
> > 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]
> > -=-=-=-=-=-=
> >
> >
> >
> >
> >
> >
> 
> 
> 
> 
> 
> 
>