public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Rabeda, Maciej" <maciej.rabeda@intel.com>
To: Laszlo Ersek <lersek@redhat.com>,
	"Fu, Siyuan" <siyuan.fu@intel.com>,
	"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Wu, Jiaxin" <jiaxin.wu@intel.com>
Subject: Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove ExitBootServices event
Date: Fri, 11 Oct 2019 10:08:36 +0000	[thread overview]
Message-ID: <40FBBD5C52E8B4429773266A45BDCC174C665ED9@IRSMSX104.ger.corp.intel.com> (raw)
In-Reply-To: <f87842cb-2038-4e48-950f-822e5b0b502e@redhat.com>

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:lersek@redhat.com] 
Sent: Friday, October 11, 2019 11:42
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/11/19 02:14, Fu, Siyuan wrote:
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: 2019年10月11日 0: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 11:29, Fu, Siyuan wrote:
>>>> -----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.
>>
>> 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.

      reply	other threads:[~2019-10-11 10:08 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-08 16:16 [PATCH v1 0/1] Remove UNDI calls from SNP during ExitBootServices Rabeda, Maciej
2019-10-08 16:16 ` [PATCH v1 1/1] NetworkPkg/SnpDxe: Remove ExitBootServices event Rabeda, Maciej
2019-10-09  2:07   ` Siyuan, Fu
2019-10-09  8:50     ` Rabeda, Maciej
2019-10-09  9:38       ` [edk2-devel] " Tomas Pilar (tpilar)
2019-10-09 22:09   ` Laszlo Ersek
2019-10-10  3:32     ` Siyuan, Fu
2019-10-10  8:06       ` Laszlo Ersek
2019-10-10  9:29         ` Siyuan, Fu
2019-10-10 16:06           ` Laszlo Ersek
2019-10-11  0:14             ` Siyuan, Fu
2019-10-11  9:41               ` Laszlo Ersek
2019-10-11 10:08                 ` Rabeda, Maciej [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=40FBBD5C52E8B4429773266A45BDCC174C665ED9@IRSMSX104.ger.corp.intel.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox