Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove ExitBootServices event

["Siyuan, Fu" <siyuan.fu@intel.com>]

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: 2019年10月10日 16:06
> To: Fu, Siyuan <siyuan.fu@intel.com>; devel@edk2.groups.io; Rabeda,
> Maciej <maciej.rabeda@intel.com>
> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>
> Subject: Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove
> ExitBootServices event
> 
> On 10/10/19 05:32, Fu, Siyuan wrote:
> > Hi, Maciej
> >
> > Considering that this patch has to co-work with corresponding UNDI device
> driver
> > bug fix, in order to avoid potential compatibility problem, please add a PCD
> to
> > NetworkPkg for this fix, and set the default value to disable state (no
> behavior
> >  change). The platform which need this fix could set the PCD to enable in
> its
> > platform DSC.
> 
> Should the new PCD go into "NetworkPkg/NetworkPcds.dsc.inc", and be
> gated with a new build flag defined in "NetworkPkg/NetworkDefines.dsc.inc"?

Currently not all the network package PCDs are listed in the NetworkPcds.dsc.inc,
But I do not oppose it if you think this PCD should be controlled by a build flag.

Thanks,
Siyuan

> 
> Thanks
> Laszlo
> 
> >
> > Please add clear description for the problem, new PCD, the SNP fix, and
> also the
> >  expected UNDI driver fix in Bugzilla 1974. So 3rd party UNDI device vendor
> could
> >  know how to fix the problem if they meet same issue.
> >
> > And please correct the Bugzilla number in patch description as Laszlo
> pointed out.
> >
> > Thanks.
> >
> > Best Regards
> > Siyuan
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> >> Ersek
> >> Sent: 2019年10月10日 6:10
> >> To: devel@edk2.groups.io; Rabeda, Maciej <maciej.rabeda@intel.com>
> >> Cc: Fu, Siyuan <siyuan.fu@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
> >> Subject: Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove
> >> ExitBootServices event
> >>
> >> On 10/08/19 18:16, Rabeda, Maciej wrote:
> >>> Patch addresses Bugzilla #1972.
> >>
> >> I think the BZ reference should be
> >> <https://bugzilla.tianocore.org/show_bug.cgi?id=1974>. (The cover letter
> >> has it right.)
> >>
> >> Thanks
> >> Laszlo
> >>
> >>> During ExitBootServices stage, drivers should not call any
> >>> functions known to use Memory Allocation Services. One of such
> >>> functions (as per UEFI spec) is UNDI->Shutdown().
> >>>
> >>> Since UNDI drivers during ExitBootServices phase are expected
> >>> to put the adapter to such a state that it will not perform any DMA
> >>> operations, there is no need to interface UNDI by SNP driver during
> >>> that phase.
> >>>
> >>> Finally, since ExitBootServices event notification function in SNP
> >>> only calls UNDI->Shutdown() and Stop() functions, there is no need
> >>> to create this event at all.
> >>>
> >>> Signed-off-by: Maciej Rabeda <maciej.rabeda@intel.com>
> >>> Cc: Siyuan Fu <siyuan.fu@intel.com>
> >>> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> >>> ---
> >>>  NetworkPkg/SnpDxe/Snp.c      | 45 --------------------
> >>>  NetworkPkg/SnpDxe/Snp.h      |  2 -
> >>>  NetworkPkg/SnpDxe/SnpDxe.inf |  3 --
> >>>  3 files changed, 50 deletions(-)
> >>>
> >>> diff --git a/NetworkPkg/SnpDxe/Snp.c b/NetworkPkg/SnpDxe/Snp.c
> >>> index a23af05078bc..7646a3ce0293 100644
> >>> --- a/NetworkPkg/SnpDxe/Snp.c
> >>> +++ b/NetworkPkg/SnpDxe/Snp.c
> >>> @@ -8,31 +8,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> >>>
> >>>  #include "Snp.h"
> >>>
> >>> -/**
> >>> -  One notified function to stop UNDI device when gBS-
> >ExitBootServices()
> >> called.
> >>> -
> >>> -  @param  Event                   Pointer to this event
> >>> -  @param  Context                 Event handler private data
> >>> -
> >>> -**/
> >>> -VOID
> >>> -EFIAPI
> >>> -SnpNotifyExitBootServices (
> >>> -  EFI_EVENT Event,
> >>> -  VOID      *Context
> >>> -  )
> >>> -{
> >>> -  SNP_DRIVER             *Snp;
> >>> -
> >>> -  Snp  = (SNP_DRIVER *)Context;
> >>> -
> >>> -  //
> >>> -  // Shutdown and stop UNDI driver
> >>> -  //
> >>> -  PxeShutdown (Snp);
> >>> -  PxeStop (Snp);
> >>> -}
> >>> -
> >>>  /**
> >>>    Send command to UNDI. It does nothing currently.
> >>>
> >>> @@ -647,21 +622,6 @@ SimpleNetworkDriverStart (
> >>>    PxeShutdown (Snp);
> >>>    PxeStop (Snp);
> >>>
> >>> -  //
> >>> -  // Create EXIT_BOOT_SERIVES Event
> >>> -  //
> >>> -  Status = gBS->CreateEventEx (
> >>> -                  EVT_NOTIFY_SIGNAL,
> >>> -                  TPL_NOTIFY,
> >>> -                  SnpNotifyExitBootServices,
> >>> -                  Snp,
> >>> -                  &gEfiEventExitBootServicesGuid,
> >>> -                  &Snp->ExitBootServicesEvent
> >>> -                  );
> >>> -  if (EFI_ERROR (Status)) {
> >>> -    goto Error_DeleteSNP;
> >>> -  }
> >>> -
> >>>    //
> >>>    //  add SNP to the undi handle
> >>>    //
> >>> @@ -778,11 +738,6 @@ SimpleNetworkDriverStop (
> >>>      return Status;
> >>>    }
> >>>
> >>> -  //
> >>> -  // Close EXIT_BOOT_SERIVES Event
> >>> -  //
> >>> -  gBS->CloseEvent (Snp->ExitBootServicesEvent);
> >>> -
> >>>    Status = gBS->CloseProtocol (
> >>>                    Controller,
> >>>                    &gEfiNetworkInterfaceIdentifierProtocolGuid_31,
> >>> diff --git a/NetworkPkg/SnpDxe/Snp.h b/NetworkPkg/SnpDxe/Snp.h
> >>> index e6b62930397d..f83a4f075adc 100644
> >>> --- a/NetworkPkg/SnpDxe/Snp.h
> >>> +++ b/NetworkPkg/SnpDxe/Snp.h
> >>> @@ -120,8 +120,6 @@ typedef struct {
> >>>      VOID                  *MapCookie;
> >>>    } MapList[MAX_MAP_LENGTH];
> >>>
> >>> -  EFI_EVENT              ExitBootServicesEvent;
> >>> -
> >>>    //
> >>>    // Whether UNDI support reporting media status from GET_STATUS
> >> command,
> >>>    // i.e. PXE_STATFLAGS_GET_STATUS_NO_MEDIA_SUPPORTED or
> >>> diff --git a/NetworkPkg/SnpDxe/SnpDxe.inf
> >> b/NetworkPkg/SnpDxe/SnpDxe.inf
> >>> index afeb1526cc10..8d045cfcf4ca 100644
> >>> --- a/NetworkPkg/SnpDxe/SnpDxe.inf
> >>> +++ b/NetworkPkg/SnpDxe/SnpDxe.inf
> >>> @@ -64,9 +64,6 @@
> >>>    DebugLib
> >>>    NetLib
> >>>
> >>> -[Guids]
> >>> -  gEfiEventExitBootServicesGuid                 ## SOMETIMES_CONSUMES
> ##
> >> Event
> >>> -
> >>>  [Protocols]
> >>>    gEfiSimpleNetworkProtocolGuid                 ## BY_START
> >>>    gEfiDevicePathProtocolGuid                    ## TO_START
> >>>
> >>
> >>
> >> 
> >