* [PATCH BUG 0/2] rpi: Fix PXE issues with grub
@ 2021-04-15 19:22 Jeremy Linton
2021-04-15 19:22 ` [PATCH 1/2] Silicon/Broadcom/BcmGenetDxe: Delay for linkup in transmit Jeremy Linton
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Jeremy Linton @ 2021-04-15 19:22 UTC (permalink / raw)
To: devel
Cc: ard.biesheuvel, leif, pete, samer.el-haj-mahmoud, awarkentin,
Jeremy Linton
When PXE booting with grub the network link
isn't given a chance to resume so grub's transmit
calls fail. This results in failed boots. Similarly
the DMA range for the adapter isn't right since it
doesn't have a 32-bit restriction. Again this keeps
grub from failing on 8G devices,
Jeremy Linton (2):
Silicon/Broadcom/BcmGenetDxe: Delay for linkup in transmit
Platform/RaspberryPi: Increase genet dma window
Platform/RaspberryPi/RPi4/RPi4.dsc | 2 +-
.../Drivers/Net/BcmGenetDxe/SimpleNetwork.c | 24 ++++++++++++++++++++--
2 files changed, 23 insertions(+), 3 deletions(-)
--
2.13.7
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/2] Silicon/Broadcom/BcmGenetDxe: Delay for linkup in transmit
2021-04-15 19:22 [PATCH BUG 0/2] rpi: Fix PXE issues with grub Jeremy Linton
@ 2021-04-15 19:22 ` Jeremy Linton
2021-04-29 20:19 ` Jeremy Linton
` (3 more replies)
2021-04-15 19:22 ` [PATCH 2/2] Platform/RaspberryPi: Increase genet dma window Jeremy Linton
2021-05-10 16:46 ` [PATCH BUG 0/2] rpi: Fix PXE issues with grub Samer El-Haj-Mahmoud
2 siblings, 4 replies; 15+ messages in thread
From: Jeremy Linton @ 2021-04-15 19:22 UTC (permalink / raw)
To: devel
Cc: ard.biesheuvel, leif, pete, samer.el-haj-mahmoud, awarkentin,
Jeremy Linton
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.
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/2] Platform/RaspberryPi: Increase genet dma window
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-15 19:22 ` Jeremy Linton
2021-04-30 18:15 ` Samer El-Haj-Mahmoud
` (2 more replies)
2021-05-10 16:46 ` [PATCH BUG 0/2] rpi: Fix PXE issues with grub Samer El-Haj-Mahmoud
2 siblings, 3 replies; 15+ messages in thread
From: Jeremy Linton @ 2021-04-15 19:22 UTC (permalink / raw)
To: devel
Cc: ard.biesheuvel, leif, pete, samer.el-haj-mahmoud, awarkentin,
Jeremy Linton
The genet is capable of addressing the entire memory space
on the RPI4. Lets allow it to dma into those regions.
This solves intermittent issues with grub/etc being able
to communicate when the 3G limit is lifted on 8G boards.
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
Platform/RaspberryPi/RPi4/RPi4.dsc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc b/Platform/RaspberryPi/RPi4/RPi4.dsc
index ddf4dd6a41..facf34f034 100644
--- a/Platform/RaspberryPi/RPi4/RPi4.dsc
+++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
@@ -698,7 +698,7 @@
Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.inf {
<PcdsFixedAtBuild>
gEmbeddedTokenSpaceGuid.PcdDmaDeviceOffset|0x00000000
- gEmbeddedTokenSpaceGuid.PcdDmaDeviceLimit|0xffffffff
+ gEmbeddedTokenSpaceGuid.PcdDmaDeviceLimit|0xffffffffff
}
#
--
2.13.7
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Silicon/Broadcom/BcmGenetDxe: Delay for linkup in transmit
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-04-29 22:30 ` Andrei Warkentin
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Jeremy Linton @ 2021-04-29 20:19 UTC (permalink / raw)
To: devel
Cc: ard.biesheuvel, leif, pete, samer.el-haj-mahmoud, awarkentin,
Jared McNeill
+Jared McNeill for review.
Thanks,
On 4/15/21 2:22 PM, 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.
>
> 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) {
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Silicon/Broadcom/BcmGenetDxe: Delay for linkup in transmit
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
@ 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
3 siblings, 0 replies; 15+ messages in thread
From: Andrei Warkentin @ 2021-04-29 22:30 UTC (permalink / raw)
To: Jeremy Linton, devel@edk2.groups.io
Cc: ard.biesheuvel@arm.com, leif@nuviainc.com, pete@akeo.ie,
samer.el-haj-mahmoud@arm.com
[-- Attachment #1: Type: text/plain, Size: 4096 bytes --]
Looks fine to me
Reviewed-by: Andrei Warkentin <awarkentin@vmware.com>
________________________________
From: Jeremy Linton <jeremy.linton@arm.com>
Sent: Thursday, April 15, 2021 2:22 PM
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: ard.biesheuvel@arm.com <ard.biesheuvel@arm.com>; leif@nuviainc.com <leif@nuviainc.com>; pete@akeo.ie <pete@akeo.ie>; samer.el-haj-mahmoud@arm.com <samer.el-haj-mahmoud@arm.com>; Andrei Warkentin <awarkentin@vmware.com>; Jeremy Linton <jeremy.linton@arm.com>
Subject: [PATCH 1/2] Silicon/Broadcom/BcmGenetDxe: Delay for linkup in transmit
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.
For reference: https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fpftf%2FRPi4%2Fissues%2F113&data=04%7C01%7Cawarkentin%40vmware.com%7Cadf7e9a53f9b4c6cff9608d90043c562%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637541113365821315%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Gdg67x%2Fv75%2B5r9bWhz8SvB%2B4VBTgZ6sEZ9SeYor%2B02E%3D&reserved=0
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
[-- Attachment #2: Type: text/html, Size: 6480 bytes --]
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] Platform/RaspberryPi: Increase genet dma window
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:45 ` Samer El-Haj-Mahmoud
2021-05-10 16:57 ` [edk2-devel] " Ard Biesheuvel
2 siblings, 1 reply; 15+ messages in thread
From: Samer El-Haj-Mahmoud @ 2021-04-30 18:15 UTC (permalink / raw)
To: Jeremy Linton, devel@edk2.groups.io
Cc: Ard Biesheuvel, leif@nuviainc.com, pete@akeo.ie,
Andrei Warkentin (awarkentin@vmware.com), Jeremy Linton,
Samer El-Haj-Mahmoud, Jared McNeill
+Jared
Reviewed-By: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> -----Original Message-----
> From: Jeremy Linton <jeremy.linton@arm.com>
> Sent: Thursday, April 15, 2021 3:22 PM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>; leif@nuviainc.com;
> pete@akeo.ie; Samer El-Haj-Mahmoud <Samer.El-Haj-
> Mahmoud@arm.com>; Andrei Warkentin (awarkentin@vmware.com)
> <awarkentin@vmware.com>; Jeremy Linton <Jeremy.Linton@arm.com>
> Subject: [PATCH 2/2] Platform/RaspberryPi: Increase genet dma window
>
> The genet is capable of addressing the entire memory space on the RPI4.
> Lets allow it to dma into those regions.
> This solves intermittent issues with grub/etc being able to communicate
> when the 3G limit is lifted on 8G boards.
>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
> Platform/RaspberryPi/RPi4/RPi4.dsc | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc
> b/Platform/RaspberryPi/RPi4/RPi4.dsc
> index ddf4dd6a41..facf34f034 100644
> --- a/Platform/RaspberryPi/RPi4/RPi4.dsc
> +++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
> @@ -698,7 +698,7 @@
> Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.inf {
> <PcdsFixedAtBuild>
> gEmbeddedTokenSpaceGuid.PcdDmaDeviceOffset|0x00000000
> - gEmbeddedTokenSpaceGuid.PcdDmaDeviceLimit|0xffffffff
> + gEmbeddedTokenSpaceGuid.PcdDmaDeviceLimit|0xffffffffff
> }
>
> #
> --
> 2.13.7
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] Platform/RaspberryPi: Increase genet dma window
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
@ 2021-05-10 16:45 ` Samer El-Haj-Mahmoud
2021-05-10 16:57 ` [edk2-devel] " Ard Biesheuvel
2 siblings, 0 replies; 15+ messages in thread
From: Samer El-Haj-Mahmoud @ 2021-05-10 16:45 UTC (permalink / raw)
To: Jeremy Linton, devel@edk2.groups.io
Cc: Ard Biesheuvel, leif@nuviainc.com, pete@akeo.ie,
Andrei Warkentin (awarkentin@vmware.com), Jeremy Linton
+Ard's new e-mail address
> -----Original Message-----
> From: Jeremy Linton <jeremy.linton@arm.com>
> Sent: Thursday, April 15, 2021 3:22 PM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>; leif@nuviainc.com;
> pete@akeo.ie; Samer El-Haj-Mahmoud <Samer.El-Haj-
> Mahmoud@arm.com>; Andrei Warkentin (awarkentin@vmware.com)
> <awarkentin@vmware.com>; Jeremy Linton <Jeremy.Linton@arm.com>
> Subject: [PATCH 2/2] Platform/RaspberryPi: Increase genet dma window
>
> The genet is capable of addressing the entire memory space on the RPI4.
> Lets allow it to dma into those regions.
> This solves intermittent issues with grub/etc being able to communicate
> when the 3G limit is lifted on 8G boards.
>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
> Platform/RaspberryPi/RPi4/RPi4.dsc | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc
> b/Platform/RaspberryPi/RPi4/RPi4.dsc
> index ddf4dd6a41..facf34f034 100644
> --- a/Platform/RaspberryPi/RPi4/RPi4.dsc
> +++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
> @@ -698,7 +698,7 @@
> Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.inf {
> <PcdsFixedAtBuild>
> gEmbeddedTokenSpaceGuid.PcdDmaDeviceOffset|0x00000000
> - gEmbeddedTokenSpaceGuid.PcdDmaDeviceLimit|0xffffffff
> + gEmbeddedTokenSpaceGuid.PcdDmaDeviceLimit|0xffffffffff
> }
>
> #
> --
> 2.13.7
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] Platform/RaspberryPi: Increase genet dma window
[not found] ` <6FD17261-5675-4148-BD06-650AB2177609@invisible.ca>
@ 2021-05-10 16:46 ` Samer El-Haj-Mahmoud
0 siblings, 0 replies; 15+ messages in thread
From: Samer El-Haj-Mahmoud @ 2021-05-10 16:46 UTC (permalink / raw)
To: Jared McNeill
Cc: Jeremy Linton, devel@edk2.groups.io, Ard Biesheuvel,
leif@nuviainc.com, pete@akeo.ie,
Andrei Warkentin (awarkentin@vmware.com)
+Ard's new e-mail address
Jared, I assume your response can be taken as a "Reviewed-By", correct?
> -----Original Message-----
> From: Jared McNeill <jmcneill@invisible.ca>
> Sent: Friday, April 30, 2021 9:08 PM
> To: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> Cc: Jeremy Linton <Jeremy.Linton@arm.com>; devel@edk2.groups.io; Ard
> Biesheuvel <Ard.Biesheuvel@arm.com>; leif@nuviainc.com; pete@akeo.ie;
> Andrei Warkentin (awarkentin@vmware.com) <awarkentin@vmware.com>
> Subject: Re: [PATCH 2/2] Platform/RaspberryPi: Increase genet dma window
>
> LGTM!
>
> Take care,
> Jared
>
>
> > On Apr 30, 2021, at 3:15 PM, Samer El-Haj-Mahmoud <Samer.El-Haj-
> Mahmoud@arm.com> wrote:
> >
> > +Jared
> >
> > Reviewed-By: Samer El-Haj-Mahmoud <Samer.El-Haj-
> Mahmoud@arm.com>
> >
> >> -----Original Message-----
> >> From: Jeremy Linton <jeremy.linton@arm.com>
> >> Sent: Thursday, April 15, 2021 3:22 PM
> >> To: devel@edk2.groups.io
> >> Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>; leif@nuviainc.com;
> >> pete@akeo.ie; Samer El-Haj-Mahmoud <Samer.El-Haj-
> Mahmoud@arm.com>;
> >> Andrei Warkentin (awarkentin@vmware.com)
> <awarkentin@vmware.com>;
> >> Jeremy Linton <Jeremy.Linton@arm.com>
> >> Subject: [PATCH 2/2] Platform/RaspberryPi: Increase genet dma window
> >>
> >> The genet is capable of addressing the entire memory space on the RPI4.
> >> Lets allow it to dma into those regions.
> >> This solves intermittent issues with grub/etc being able to
> >> communicate when the 3G limit is lifted on 8G boards.
> >>
> >> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> >> ---
> >> Platform/RaspberryPi/RPi4/RPi4.dsc | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc
> >> b/Platform/RaspberryPi/RPi4/RPi4.dsc
> >> index ddf4dd6a41..facf34f034 100644
> >> --- a/Platform/RaspberryPi/RPi4/RPi4.dsc
> >> +++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
> >> @@ -698,7 +698,7 @@
> >> Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.inf {
> >> <PcdsFixedAtBuild>
> >> gEmbeddedTokenSpaceGuid.PcdDmaDeviceOffset|0x00000000
> >> - gEmbeddedTokenSpaceGuid.PcdDmaDeviceLimit|0xffffffff
> >> + gEmbeddedTokenSpaceGuid.PcdDmaDeviceLimit|0xffffffffff
> >> }
> >>
> >> #
> >> --
> >> 2.13.7
> >
> > IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended recipient,
> please notify the sender immediately and do not disclose the contents to any
> other person, use it for any purpose, or store or copy the information in any
> medium. Thank you.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH BUG 0/2] rpi: Fix PXE issues with grub
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-15 19:22 ` [PATCH 2/2] Platform/RaspberryPi: Increase genet dma window Jeremy Linton
@ 2021-05-10 16:46 ` Samer El-Haj-Mahmoud
2 siblings, 0 replies; 15+ messages in thread
From: Samer El-Haj-Mahmoud @ 2021-05-10 16:46 UTC (permalink / raw)
To: Jeremy Linton, devel@edk2.groups.io
Cc: Ard Biesheuvel, leif@nuviainc.com, pete@akeo.ie,
Andrei Warkentin (awarkentin@vmware.com), Jeremy Linton
+Ard's new e-mail address
> -----Original Message-----
> From: Jeremy Linton <jeremy.linton@arm.com>
> Sent: Thursday, April 15, 2021 3:22 PM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>; leif@nuviainc.com;
> pete@akeo.ie; Samer El-Haj-Mahmoud <Samer.El-Haj-
> Mahmoud@arm.com>; Andrei Warkentin (awarkentin@vmware.com)
> <awarkentin@vmware.com>; Jeremy Linton <Jeremy.Linton@arm.com>
> Subject: [PATCH BUG 0/2] rpi: Fix PXE issues with grub
>
> When PXE booting with grub the network link isn't given a chance to resume
> so grub's transmit calls fail. This results in failed boots. Similarly the DMA
> range for the adapter isn't right since it doesn't have a 32-bit restriction.
> Again this keeps grub from failing on 8G devices,
>
> Jeremy Linton (2):
> Silicon/Broadcom/BcmGenetDxe: Delay for linkup in transmit
> Platform/RaspberryPi: Increase genet dma window
>
> Platform/RaspberryPi/RPi4/RPi4.dsc | 2 +-
> .../Drivers/Net/BcmGenetDxe/SimpleNetwork.c | 24
> ++++++++++++++++++++--
> 2 files changed, 23 insertions(+), 3 deletions(-)
>
> --
> 2.13.7
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Silicon/Broadcom/BcmGenetDxe: Delay for linkup in transmit
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
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
3 siblings, 0 replies; 15+ messages in thread
From: Samer El-Haj-Mahmoud @ 2021-05-10 16:47 UTC (permalink / raw)
To: Jeremy Linton, devel@edk2.groups.io
Cc: Ard Biesheuvel, leif@nuviainc.com, pete@akeo.ie,
Andrei Warkentin (awarkentin@vmware.com), Jeremy Linton
+Ard's new e-mail address
> -----Original Message-----
> From: Jeremy Linton <jeremy.linton@arm.com>
> Sent: Thursday, April 15, 2021 3:22 PM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>; leif@nuviainc.com;
> pete@akeo.ie; Samer El-Haj-Mahmoud <Samer.El-Haj-
> Mahmoud@arm.com>; Andrei Warkentin (awarkentin@vmware.com)
> <awarkentin@vmware.com>; Jeremy Linton <Jeremy.Linton@arm.com>
> Subject: [PATCH 1/2] Silicon/Broadcom/BcmGenetDxe: Delay for linkup in
> transmit
>
> 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.
>
> 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
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Silicon/Broadcom/BcmGenetDxe: Delay for linkup in transmit
[not found] ` <DC23CCD0-67BF-4643-9FC2-59E630D7D13D@invisible.ca>
@ 2021-05-10 16:47 ` Samer El-Haj-Mahmoud
0 siblings, 0 replies; 15+ messages in thread
From: Samer El-Haj-Mahmoud @ 2021-05-10 16:47 UTC (permalink / raw)
To: Jared McNeill, Jeremy Linton
Cc: devel@edk2.groups.io, Ard Biesheuvel, leif@nuviainc.com,
Pete Batard, Andrei Warkentin (awarkentin@vmware.com)
+Ard's new e-mail address
Jared, please confirm this is a "Reviewed-By"
> -----Original Message-----
> From: Jared McNeill <jmcneill@invisible.ca>
> Sent: Friday, April 30, 2021 6:30 AM
> To: Jeremy Linton <Jeremy.Linton@arm.com>
> Cc: devel@edk2.groups.io; Ard Biesheuvel <Ard.Biesheuvel@arm.com>;
> leif@nuviainc.com; Pete Batard <pete@akeo.ie>; Samer El-Haj-Mahmoud
> <Samer.El-Haj-Mahmoud@arm.com>; Andrei Warkentin
> (awarkentin@vmware.com) <awarkentin@vmware.com>
> Subject: Re: [PATCH 1/2] Silicon/Broadcom/BcmGenetDxe: Delay for linkup in
> transmit
>
> LGTM.
>
> Take care,
> Jared
>
>
> > On Apr 29, 2021, at 5:19 PM, Jeremy Linton <jeremy.linton@arm.com>
> wrote:
> >
> > +Jared McNeill for review.
> >
> > Thanks,
> >
> > On 4/15/21 2:22 PM, 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.
> >> 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) {
> >
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [edk2-devel] [PATCH 1/2] Silicon/Broadcom/BcmGenetDxe: Delay for linkup in transmit
2021-04-15 19:22 ` [PATCH 1/2] Silicon/Broadcom/BcmGenetDxe: Delay for linkup in transmit Jeremy Linton
` (2 preceding siblings ...)
2021-05-10 16:47 ` Samer El-Haj-Mahmoud
@ 2021-05-10 16:56 ` Ard Biesheuvel
2021-05-10 17:26 ` Jeremy Linton
3 siblings, 1 reply; 15+ messages in thread
From: Ard Biesheuvel @ 2021-05-10 16:56 UTC (permalink / raw)
To: edk2-devel-groups-io, Jeremy Linton
Cc: Ard Biesheuvel, Leif Lindholm, Peter Batard, Samer El-Haj-Mahmoud,
Andrei Warkentin
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?
> 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
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [edk2-devel] [PATCH 2/2] Platform/RaspberryPi: Increase genet dma window
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
2021-05-10 16:45 ` Samer El-Haj-Mahmoud
@ 2021-05-10 16:57 ` Ard Biesheuvel
2 siblings, 0 replies; 15+ messages in thread
From: Ard Biesheuvel @ 2021-05-10 16:57 UTC (permalink / raw)
To: edk2-devel-groups-io, Jeremy Linton
Cc: Ard Biesheuvel, Leif Lindholm, Peter Batard, Samer El-Haj-Mahmoud,
Andrei Warkentin
On Thu, 15 Apr 2021 at 21:22, Jeremy Linton <jeremy.linton@arm.com> wrote:
>
> The genet is capable of addressing the entire memory space
> on the RPI4. Lets allow it to dma into those regions.
> This solves intermittent issues with grub/etc being able
> to communicate when the 3G limit is lifted on 8G boards.
>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
Acked-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> Platform/RaspberryPi/RPi4/RPi4.dsc | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc b/Platform/RaspberryPi/RPi4/RPi4.dsc
> index ddf4dd6a41..facf34f034 100644
> --- a/Platform/RaspberryPi/RPi4/RPi4.dsc
> +++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
> @@ -698,7 +698,7 @@
> Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.inf {
> <PcdsFixedAtBuild>
> gEmbeddedTokenSpaceGuid.PcdDmaDeviceOffset|0x00000000
> - gEmbeddedTokenSpaceGuid.PcdDmaDeviceLimit|0xffffffff
> + gEmbeddedTokenSpaceGuid.PcdDmaDeviceLimit|0xffffffffff
> }
>
> #
> --
> 2.13.7
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [edk2-devel] [PATCH 1/2] Silicon/Broadcom/BcmGenetDxe: Delay for linkup in transmit
2021-05-10 16:56 ` [edk2-devel] " Ard Biesheuvel
@ 2021-05-10 17:26 ` Jeremy Linton
2021-05-10 17:32 ` Ard Biesheuvel
0 siblings, 1 reply; 15+ messages in thread
From: Jeremy Linton @ 2021-05-10 17:26 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel-groups-io
Cc: Ard Biesheuvel, Leif Lindholm, Peter Batard, Samer El-Haj-Mahmoud,
Andrei Warkentin
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.
I was actually a bit concerned that the 10s wasn't enough since AFAIK a
large part of the delay is the remote side.
>
>
>
>> 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
>>
>>
>>
>>
>>
>>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [edk2-devel] [PATCH 1/2] Silicon/Broadcom/BcmGenetDxe: Delay for linkup in transmit
2021-05-10 17:26 ` Jeremy Linton
@ 2021-05-10 17:32 ` Ard Biesheuvel
0 siblings, 0 replies; 15+ messages in thread
From: Ard Biesheuvel @ 2021-05-10 17:32 UTC (permalink / raw)
To: Jeremy Linton
Cc: edk2-devel-groups-io, Ard Biesheuvel, Leif Lindholm, Peter Batard,
Samer El-Haj-Mahmoud, Andrei Warkentin
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
> >>
> >>
> >>
> >>
> >>
> >>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-05-10 17:32 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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
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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox