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

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

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.

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
> >
> 
> 
>