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.web12.671.1620667965095914060 for ; Mon, 10 May 2021 10:32:45 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=TZPZg0rB; spf=pass (domain: kernel.org, ip: 198.145.29.99, mailfrom: ardb@kernel.org) Received: by mail.kernel.org (Postfix) with ESMTPSA id 4005A61554 for ; Mon, 10 May 2021 17:32:44 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1620667964; bh=L1Ffnc8+G8rQVMjsk9TUbVl/j4+cOnFCm7As14Mf2bA=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=TZPZg0rBYX7bUYcJwQoS6U+KLqhISZbqNFRQK64zOEq0fWYZ6Inp/z9TqadZSCeZq d+NWNUqVctyE7aU5T8hY74J1BLAQQo8o8gQxwzyPEGoz0mqV7Kcj6/QITXxskPqTej BDldt5qCRy75yE5nR6OqBvoVVWOGG3RMgbz3DzC7NpYudKY+DuwkFr2T/WZGvfjnkx 1hjJi20ZY1Mj5wHL5YWDMV9nx7Rb5aZxQAxuFqjfl8+vxqdBeX9lNCsLutD9FaIyaP N5IES8rpjZw3A56PJeoDIEN/YJucdNTWF/2uPVJUIBZu83XOXhBFNO1EBCZeqDROHt O2G6HQ5wC6PPw== Received: by mail-oi1-f174.google.com with SMTP id v22so11610104oic.2 for ; Mon, 10 May 2021 10:32:44 -0700 (PDT) X-Gm-Message-State: AOAM532ZCGBNdnN4vJ4td+Q+3vqzxQ+q0o5XIIjz+Xf23yHoL/yaSwHH syqIWDDg1gTwnJheFmsZNyWmwyLb7Yyaprndkws= X-Google-Smtp-Source: ABdhPJwERDEPgltvyRabdlvEtl8151dzTu1PZEy5OurJy+sWjD6aKdV1qSsWmDYLKf/Yj8J/OCKOIprjGWa7fXD2b48= X-Received: by 2002:aca:e142:: with SMTP id y63mr18921221oig.33.1620667963466; Mon, 10 May 2021 10:32:43 -0700 (PDT) MIME-Version: 1.0 References: <20210415192207.3257790-1-jeremy.linton@arm.com> <20210415192207.3257790-2-jeremy.linton@arm.com> <431d02c5-1c25-cb0f-3c94-e677c1d29536@arm.com> In-Reply-To: <431d02c5-1c25-cb0f-3c94-e677c1d29536@arm.com> From: "Ard Biesheuvel" Date: Mon, 10 May 2021 19:32:32 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH 1/2] Silicon/Broadcom/BcmGenetDxe: Delay for linkup in transmit To: Jeremy Linton Cc: edk2-devel-groups-io , Ard Biesheuvel , Leif Lindholm , Peter Batard , Samer El-Haj-Mahmoud , Andrei Warkentin Content-Type: text/plain; charset="UTF-8" On Mon, 10 May 2021 at 19:26, Jeremy Linton wrote: > > Hi, > > On 5/10/21 11:56 AM, Ard Biesheuvel wrote: > > 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? > > 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 > >> --- > >> .../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 > >> > >> > >> > >> > >> > >> >