From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by mx.groups.io with SMTP id smtpd.web08.182.1620665784910523844 for ; Mon, 10 May 2021 09:56:25 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=aAoYWYQb; spf=pass (domain: kernel.org, ip: 198.145.29.99, mailfrom: ardb@kernel.org) Received: by mail.kernel.org (Postfix) with ESMTPSA id 21BEC61421 for ; Mon, 10 May 2021 16:56:24 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1620665784; bh=2lad4DLN7S50x5DF3IC5PT1lo5VXvP+nkcZUYbObjGM=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=aAoYWYQb0sJAv04kQyeDJRFSHveL61pEcXLOw0RnfKkMyvnP4PjMpUu3dw5TfrAaq Bf44USEGlZyYtPT/uzvWJnbNGf5MKW0on/mpbcnSVF0uAghQIQx7VSzEhd1bKTZACH Iq8YxioywKcfcim62LaM1Kko3MswJRSCyWj4Gji7Aju+JhnRMuES52PQ+GV6tZevNo VtVlkPWd6v+eWP00yx5eleOtt6ZPa7skToPCicuL7mE2T8nOcpfasd0Q8ga/OL57o7 S8936ZcKeE+1VLLSWJslqEIx1dxFUMiuZ/XXlQV3GFPvo8ddAUHJS/Gh2rUlLCI/gg uUkOJs/r0Xt6A== Received: by mail-ot1-f47.google.com with SMTP id r26-20020a056830121ab02902a5ff1c9b81so15025731otp.11 for ; Mon, 10 May 2021 09:56:24 -0700 (PDT) X-Gm-Message-State: AOAM533x40TpmeuT6+5nasYUd80DAUe2tpswgMBx6Al5yfIN2GFJHGF9 inkMXUsODGKF7S4i3yyrho/BMAQ5ngo3FpBkGBM= X-Google-Smtp-Source: ABdhPJy95ZCkSBLpP9ZO/OkBT1Zj4xNovdfwIwALX6aSJMdAks1hoALjQwuEIerEE8uepxrjeHPx7JEdxhMFxOuPmuc= X-Received: by 2002:a9d:7cd8:: with SMTP id r24mr10165333otn.90.1620665783425; Mon, 10 May 2021 09:56:23 -0700 (PDT) MIME-Version: 1.0 References: <20210415192207.3257790-1-jeremy.linton@arm.com> <20210415192207.3257790-2-jeremy.linton@arm.com> In-Reply-To: <20210415192207.3257790-2-jeremy.linton@arm.com> From: "Ard Biesheuvel" Date: Mon, 10 May 2021 18:56:12 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH 1/2] Silicon/Broadcom/BcmGenetDxe: Delay for linkup in transmit To: edk2-devel-groups-io , Jeremy Linton Cc: Ard Biesheuvel , Leif Lindholm , Peter Batard , Samer El-Haj-Mahmoud , Andrei Warkentin Content-Type: text/plain; charset="UTF-8" On Thu, 15 Apr 2021 at 21:22, Jeremy Linton wrote: > > Under normal circumstances GenetSimpleNetworkTransmit won't be > called unless the rest of the network stack detects the link is > up. So, during normal operation when the adapter is initialized > the link naturally transitions to link up, and then is ready for > activity later in the boot sequence. If that hasn't happened by > the time PXE runs then it will itself wait for the link. > > OTOH, the normal distro PXE sequence involves PXE loading shim > which in turn loads grub, which tries to read machine specific > configs, modules, and grub.cfg in order to prepare the boot menu. > Then, once a grub selection is picked, it might try to load the > kernel+initrd. > > In this sequence the network stack is shutdown and restarted > multiple times. Grub though, starts up, notices its been network > booted, reads saved network parameters and immediately tries to > transmit data assuming the link is still up. > > When that happens grub will print "couldn't send network packet" > and if that lasts long enough it fails to load grub.cfg and the > user gets dropped to the grub prompt because no one in the path > bothers to assure the link state has transitioned back up. > This is an excellent problem description. Could we dedicate a couple of paragraphs to the solution as well? Especially given that 'wait for random # of seconds' seems to be the taken approach here, which seems a bit sloppy to me, given that we should be able to detect whether the link is up, no? > For reference: https://github.com/pftf/RPi4/issues/113 > > Signed-off-by: Jeremy Linton > --- > .../Drivers/Net/BcmGenetDxe/SimpleNetwork.c | 24 ++++++++++++++++++++-- > 1 file changed, 22 insertions(+), 2 deletions(-) > > diff --git a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/SimpleNetwork.c b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/SimpleNetwork.c > index 1bda18f157..29c76d8495 100644 > --- a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/SimpleNetwork.c > +++ b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/SimpleNetwork.c > @@ -13,6 +13,7 @@ > #include > #include > #include > +#include > #include > > #include "BcmGenetDxe.h" > @@ -590,9 +591,28 @@ GenetSimpleNetworkTransmit ( > > if (!Genet->SnpMode.MediaPresent) { > // > - // Don't bother transmitting if there's no link. > + // We should only really get here if the link was up > + // and is now down due to a stop/shutdown sequence, and > + // the app (grub) doesn't bother to check link state > + // because it was up a moment before. > + // Lets wait a bit for the link to resume, rather than > + // failing to send. In the case of grub it works either way > + // but we can't be sure that is universally true, and > + // hanging for a couple seconds is nicer than a screen of > + // grub send failure messages. > // > - return EFI_NOT_READY; > + int retries = 1000; > + DEBUG ((DEBUG_INFO, "%a: Waiting 10s for link\n", __FUNCTION__)); > + do { > + gBS->Stall (10000); > + Status = GenericPhyUpdateConfig (&Genet->Phy); > + } while (EFI_ERROR (Status) && retries--); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "%a: no link\n", __FUNCTION__)); > + return EFI_NOT_READY; > + } else { > + Genet->SnpMode.MediaPresent = TRUE; > + } > } > > if (HeaderSize != 0) { > -- > 2.13.7 > > > > > >