Laszlo:
Thanks for the detailed feedback on the changes for this issue. Since we are
not sure if this change are valid / violate some purpose of SNP driver, it
mentioned as Workaround.
@Saloni Kasbekar and @Clark-williams, Zachary can add more on these changes.
As you recommended, we can have PCD which controls these changes till the
changes are addressed in grub.
@Santhosh Kumar V is this issue can be seen only in SLES 15 or it can be found
in any OS having Grub 2.x?
Thanks
Siva
-----Original Message-----
From: Laszlo Ersek <ler...@redhat.com>
Sent: Thursday, February 22, 2024 5:15 AM
To: devel@edk2.groups.io; Santhosh Kumar V <santhoshkum...@ami.com>
Cc: Sivaraman Nainar <sivaram...@ami.com>; Raj V Akilan <ra...@ami.com>;
Soundharia R <soundhar...@ami.com>; Saloni Kasbekar
<saloni.kasbe...@intel.com>; Zachary Clark-williams
<zachary.clark-willi...@intel.com>
Subject: [EXTERNAL] Re: [edk2-devel] [PATCH] NetworkPkg:Resolved Consecutive
Pxe-Http Boot Issue
**CAUTION: The e-mail below is from an external source. Please exercise caution
before opening attachments, clicking links, or following guidance.**
On 2/21/24 18:15, Santhosh Kumar V via groups.io wrote:
> The customer has a server environment where PXE and HTTP service run in same
> Linux Server. In this environment a SUT trying to boot to SLES 15 OS via PXE
> from the Boot Menu. After PXE Boot file downloaded and grub Loaded without
> continuing for installation Exit is pressed and control back to Setup.
> Now the HTTP boot to SLES 15 OS tried in the same environment and failed to
> download the file. If there is a reconnect -r performed before this HTTP Boot
> then boot file download and installation is getting success.
> Root cause of the issue is, when Exit from grub performed, boot Loader Stops
> the SNP Driver and starts the same.
This sentence feels like the key one.
Are you saying that grub calls Snp->Start() just before it exits?
If so, am I right to suspect that that's a grub bug? It sounds like a resource
leak, after all.
Can you perhaps include a grub source code location / pointer in the commit
message?
> During this process SNP is in Initialized State. When HTTP boot is performed
> immediately after PXE Failure, the MNP configure method initiates the SNP
> Start again. Since the SNP already started by grub it returns
> EFI_ALREADY_STARTED and none of the upper Layer drivers are getting started.
> As a work around in MNPConfigure(), if the SNP Start failed with Already
> Started and in Initialized state we can return success so that rest of the
> drivers can be loaded and HTTP boot can work.
>
>
> Cc: Saloni Kasbekar <saloni.kasbe...@intel.com>
> Cc: Zachary Clark-williams <zachary.clark-willi...@intel.com>
>
> Signed-off-by: SanthoshKumar <santhoshkum...@ami.com>
> ---
> NetworkPkg/MnpDxe/MnpConfig.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/NetworkPkg/MnpDxe/MnpConfig.c
> b/NetworkPkg/MnpDxe/MnpConfig.c index 93587d53aa..0f2df28d73 100644
> --- a/NetworkPkg/MnpDxe/MnpConfig.c
> +++ b/NetworkPkg/MnpDxe/MnpConfig.c
> @@ -1120,7 +1120,9 @@ MnpStartSnp (
> // Start the simple network.
>
> //
>
> Status = Snp->Start (Snp);
>
> -
>
> + if ((Status == EFI_ALREADY_STARTED ) && (Snp->Mode->State ==
> + EfiSimpleNetworkInitialized)) {
>
> + return EFI_SUCCESS;
>
> + }
>
> if (!EFI_ERROR (Status)) {
>
> //
>
> // Initialize the simple network.
>
The commit message does say this is a workaround, and I don't immediately any
see why this workaround (in the code) would be problematic in practice, but it
still leaves a bad taste in my mouth.
Consider: the call path is the following:
MnpConfigure() [NetworkPkg/MnpDxe/MnpConfig.c] -- public .Configure()
protocol member function
MnpConfigureInstance() [NetworkPkg/MnpDxe/MnpConfig.c]
MnpStart() [NetworkPkg/MnpDxe/MnpConfig.c]
// see notes!
MnpStartSnp() [NetworkPkg/MnpDxe/MnpConfig.c]
Notes: the MnpStartSnp() call in MnpStart() is conditional on two circumstances
(at the same time):
- "If it's not a configuration update, increase the configured children number."
- "It's the first configured child, start the simple network."
In other words, the MNP driver has just bound SNP "BY_DRIVER" (i.e.,
exclusively), installed the MNP service binding protocol for each vlan (IIUC),
and one of those SB instances is now being used to create the first MNP
instance. I think that under these circumstances, it is reasonable for the MNP
driver to expect that the underlying SNP be in stopped state. :/
How long would NetworkPkg have to carry this workaround? (I.e., how long before
the grub issue is fixed, and the buggy version deprecated?)
I'd prefer at least a comment in the code that the return path is a workaround
for (I feel) an earlier SNP usage violation.
A FeaturePCD to disable the workaround could be reasonable too (but the
NetworkPkg maintainers could disagree about that).
BTW, the commit message should be wrapped at 75 characters. These long lines
(in the body) will pass PatchCheck, but generate warnings. Those warnings are
tolerable for log quotes, URLs, etc, but for normal English text, wrapping is
much preferred.
Another comment on the commit message: the subject line should state something
like
NetworkPkg/MnpDxe: work aroung SNP state leak in grub
Laszlo
-The information contained in this message may be confidential and proprietary
to American Megatrends (AMI). This communication is intended to be read only by
the individual or entity to whom it is addressed or by their designee. If the
reader of this message is not the intended recipient, you are on notice that
any distribution of this message, in any form, is strictly prohibited. Please
promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and
then delete or destroy all copies of the transmission.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115769): https://edk2.groups.io/g/devel/message/115769
Mute This Topic: https://groups.io/mt/104498511/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
