From: "Ard Biesheuvel" <ardb@kernel.org>
To: Jeremy Linton <jeremy.linton@arm.com>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>,
Ard Biesheuvel <ard.biesheuvel@arm.com>,
Leif Lindholm <leif@nuviainc.com>, Peter Batard <pete@akeo.ie>,
Samer El-Haj-Mahmoud <samer.el-haj-mahmoud@arm.com>,
Andrei Warkentin <awarkentin@vmware.com>
Subject: Re: [edk2-devel] [PATCH 1/2] Silicon/Broadcom/BcmGenetDxe: Delay for linkup in transmit
Date: Mon, 10 May 2021 19:32:32 +0200 [thread overview]
Message-ID: <CAMj1kXHTD9tUd1aADiZCB+8+J9MD4Hw8QjGeYPQUeeZTV1ptSQ@mail.gmail.com> (raw)
In-Reply-To: <431d02c5-1c25-cb0f-3c94-e677c1d29536@arm.com>
On Mon, 10 May 2021 at 19:26, Jeremy Linton <jeremy.linton@arm.com> wrote:
>
> Hi,
>
> On 5/10/21 11:56 AM, Ard Biesheuvel wrote:
> > On Thu, 15 Apr 2021 at 21:22, Jeremy Linton <jeremy.linton@arm.com> 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?
>
> Isn't that what GenericPhyUpdateConfig is doing? Checking for linkup,
> and updating the phy state? So once the link goes up, it return
> EFI_SUCCESS and the loop terminates.
>
OK. Could you dedicate some prose to this logic in the commit log please?
> I was actually a bit concerned that the 10s wasn't enough since AFAIK a
> large part of the delay is the remote side.
>
The remote side of what? The Ethernet cable?
>
>
> >
> >
> >
> >> For reference: https://github.com/pftf/RPi4/issues/113
> >>
> >> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> >> ---
> >> .../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 <Library/DebugLib.h>
> >> #include <Library/DmaLib.h>
> >> #include <Library/NetLib.h>
> >> +#include <Library/UefiBootServicesTableLib.h>
> >> #include <Protocol/SimpleNetwork.h>
> >>
> >> #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
> >>
> >>
> >>
> >>
> >>
> >>
>
next prev parent reply other threads:[~2021-05-10 17:32 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-15 19:22 [PATCH BUG 0/2] rpi: Fix PXE issues with grub Jeremy Linton
2021-04-15 19:22 ` [PATCH 1/2] Silicon/Broadcom/BcmGenetDxe: Delay for linkup in transmit Jeremy Linton
2021-04-29 20:19 ` Jeremy Linton
[not found] ` <DC23CCD0-67BF-4643-9FC2-59E630D7D13D@invisible.ca>
2021-05-10 16:47 ` Samer El-Haj-Mahmoud
2021-04-29 22:30 ` Andrei Warkentin
2021-05-10 16:47 ` Samer El-Haj-Mahmoud
2021-05-10 16:56 ` [edk2-devel] " Ard Biesheuvel
2021-05-10 17:26 ` Jeremy Linton
2021-05-10 17:32 ` Ard Biesheuvel [this message]
2021-04-15 19:22 ` [PATCH 2/2] Platform/RaspberryPi: Increase genet dma window Jeremy Linton
2021-04-30 18:15 ` Samer El-Haj-Mahmoud
[not found] ` <6FD17261-5675-4148-BD06-650AB2177609@invisible.ca>
2021-05-10 16:46 ` Samer El-Haj-Mahmoud
2021-05-10 16:45 ` Samer El-Haj-Mahmoud
2021-05-10 16:57 ` [edk2-devel] " Ard Biesheuvel
2021-05-10 16:46 ` [PATCH BUG 0/2] rpi: Fix PXE issues with grub Samer El-Haj-Mahmoud
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=CAMj1kXHTD9tUd1aADiZCB+8+J9MD4Hw8QjGeYPQUeeZTV1ptSQ@mail.gmail.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