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

["Laszlo Ersek" <lersek@redhat.com>]

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

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