Re: [edk2-devel] [PATCH] NetworkPkg:Resolved Consecutive Pxe-Http Boot Issue

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

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.kasbekar@intel.com>
> Cc: Zachary Clark-williams <zachary.clark-williams@intel.com>
>
> Signed-off-by: SanthoshKumar <santhoshkumarv@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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115755): https://edk2.groups.io/g/devel/message/115755
Mute This Topic: https://groups.io/mt/104498511/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-