Laszlo, Siyuan, Very good input, thanks. I'll introduce the V2 patch with that fix + control with fixed PCD - it works for me :)
-----Original Message----- From: Laszlo Ersek [mailto:ler...@redhat.com] Sent: Friday, October 11, 2019 11:42 To: Fu, Siyuan <siyuan...@intel.com>; devel@edk2.groups.io; Rabeda, Maciej <maciej.rab...@intel.com> Cc: Wu, Jiaxin <jiaxin...@intel.com> Subject: Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove ExitBootServices event On 10/11/19 02:14, Fu, Siyuan wrote: >> -----Original Message----- >> From: Laszlo Ersek <ler...@redhat.com> >> Sent: 2019年10月11日 0:06 >> To: Fu, Siyuan <siyuan...@intel.com>; devel@edk2.groups.io; Rabeda, >> Maciej <maciej.rab...@intel.com> >> Cc: Wu, Jiaxin <jiaxin...@intel.com> >> Subject: Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove >> ExitBootServices event >> >> On 10/10/19 11:29, Fu, Siyuan wrote: >>>> -----Original Message----- >>>> From: Laszlo Ersek <ler...@redhat.com> >>>> Sent: 2019年10月10日 16:06 >>>> To: Fu, Siyuan <siyuan...@intel.com>; devel@edk2.groups.io; Rabeda, >>>> Maciej <maciej.rab...@intel.com> >>>> Cc: Wu, Jiaxin <jiaxin...@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. >> >> I mainly asked from a consistency point of view. >> >> The inclusion of SnpDxe is already controlled by NETWORK_SNP_ENABLE, >> at least in platforms that utilize the NetworkPkg *.inc files. And >> the PCD in question would control the behavior of SnpDxe at >> ExitBootServices(), if I understand correctly. >> >> This is similar to NETWORK_HTTP_BOOT_ENABLE and >> NETWORK_ALLOW_HTTP_CONNECTIONS: >> - The former includes HttpDxe and HttpBootDxe (and some other drivers). >> - The latter controls PcdAllowHttpConnections, which is consumed by >> HttpDxe and HttpBootDxe. >> >> I don't feel too strongly about it, I just thought it worth raising. > > It's not an existing "consistency" in current network package. The > current macros are mostly used for control more high level feature > enable/disable, not such kind of bug fix, For example, > NETWORK_ISCSI_ENABLE includes IScsiDxe driver, while > PcdIScsiAIPNetworkBootPolicy is consumed by IScsiDriver, but not > controlled by a macro. And also other PXE, DHCP and PXE related PCDs. Good point! So, I'm fine if the new feature PCD is not exposed with a build flag. Thanks Laszlo -------------------------------------------------------------------- Intel Technology Poland sp. z o.o. ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN. Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione. This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48812): https://edk2.groups.io/g/devel/message/48812 Mute This Topic: https://groups.io/mt/34444145/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
