I'd like some feedback on this approach before I send this to the EDK2 list.
Networking initialization broke in 2014.01 due reliance on a specific ordering of driver initialization that is not enforced. In particular, networking only worked if the Ip4ConfigDriverBinding protocol was started after the ArpDriverBinding protocol. This was due to the Arp driver startup causing the initialization of the LAN9xxx driver. These two binding protocols have equal "versions", which control the ordering, so it seems that the ordering between the two is arbitrary. (I have not tracked down exactly why the ordering is different in the two releases.)
There were two different problems in the failing case:
1) The MnpGetModeData() function would not update the SnpMode structure it was passed if the SNP driver was not started, even though the Ip4Config caller did not treat EFI_NOT_STARTED as an error. Ip4ConfigDriverBindingStart() would then proceed to use a SnpMode structure with unitialized stack data. The first patch in the series updates MnpGetModeData() to not treat EFI_NOT_STARTED as an error and to update the SnpMode structure in this case. With this fix, networking initialization is still broken, but at least no unitialized data is used.
2) The LAN9xxx drivers did not update the SnpMode MAC addresses until the driver was started or initialized (they each did it differently.) The next 2 patches update the driver to set the SnpMode MAC addresses at driver entry, so they are available at the time the Ip4Config driver binds with the LAN9xxx driver.
Roy Franz (3): Mnp Error handling LAN9118: set SNP MAC in entry function LAN91x: Set SNP current MAC address in entry function
ArmPlatformPkg/Drivers/LAN9118Dxe/LAN9118Dxe.c | 9 +++------ ArmPlatformPkg/Drivers/LAN91xDxe/LAN91xDxe.c | 3 +-- MdeModulePkg/Universal/Network/MnpDxe/MnpMain.c | 2 +- 3 files changed, 5 insertions(+), 9 deletions(-)
Change MnpGetModeData() handling of return status from SnpStatus to not consider EFI_NOT_STARTED an error, similar to how Ip4ConfigDriverBindingStart() handles it.
Without this change, Ip4ConfigDriverBindingStart() considers the Mnp->GetModeData successful even though the SnpMode parameter is unchanged, and is full of unitialized stack data.
If this change is not correct, then other error handling changes need to be made in Ip4ConfigDriverBindingStart() so that uninitialized data from the stack is not used.
Signed-off-by: Roy Franz roy.franz@linaro.org Contributed-under: TianoCore Contribution Agreement 1.0 --- MdeModulePkg/Universal/Network/MnpDxe/MnpMain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/MdeModulePkg/Universal/Network/MnpDxe/MnpMain.c b/MdeModulePkg/Universal/Network/MnpDxe/MnpMain.c index 795f063..c628991 100644 --- a/MdeModulePkg/Universal/Network/MnpDxe/MnpMain.c +++ b/MdeModulePkg/Universal/Network/MnpDxe/MnpMain.c @@ -80,7 +80,7 @@ MnpGetModeData ( // will be updated to reflect any change of media status // Status = Snp->GetStatus (Snp, &InterruptStatus, NULL); - if (!EFI_ERROR (Status)) { + if (!EFI_ERROR (Status) || (Status == EFI_NOT_STARTED)) { CopyMem (SnpModeData, Snp->Mode, sizeof (*SnpModeData)); } }
Set the MAC addresses in the SNP function in the entry point, rather than the initialization function. This allows the driver to be bound to before being initialized. Without this change there is the unhandled requirement that the LAN9118 driver be initialized before the ip4 driver binds to it.
Signed-off-by: Roy Franz roy.franz@linaro.org Contributed-under: TianoCore Contribution Agreement 1.0 --- ArmPlatformPkg/Drivers/LAN9118Dxe/LAN9118Dxe.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/ArmPlatformPkg/Drivers/LAN9118Dxe/LAN9118Dxe.c b/ArmPlatformPkg/Drivers/LAN9118Dxe/LAN9118Dxe.c index b05fac2..ab9fa38 100644 --- a/ArmPlatformPkg/Drivers/LAN9118Dxe/LAN9118Dxe.c +++ b/ArmPlatformPkg/Drivers/LAN9118Dxe/LAN9118Dxe.c @@ -1381,11 +1381,6 @@ DEBUG((EFI_D_ERROR, "Here: %a, line: %d\n", __func__, __LINE__)); gRxDataSize = RxBufferSize; }
- // Set the current MAC Address - Snp->Mode->CurrentAddress = GetCurrentMacAddress (); - - // Set the permanent MAC Address - Snp->Mode->PermanentAddress = Snp->Mode->CurrentAddress;
// Do auto-negotiation if supported Status = AutoNegotiate (Snp); @@ -2572,9 +2567,11 @@ DEBUG((EFI_D_ERROR, "Here: %a, line: %d, snpMode: %p\n", __func__, __LINE__, Snp
// Set broadcast address SetMem (&SnpMode->BroadcastAddress, sizeof (EFI_MAC_ADDRESS), 0xFF); + SnpMode->PermanentAddress = GetCurrentMacAddress (); + SnpMode->CurrentAddress = SnpMode->PermanentAddress;
// Assign fields for device path - Lan9118Path->Lan9118.MacAddress = GetCurrentMacAddress (); + Lan9118Path->Lan9118.MacAddress = SnpMode->PermanentAddress; Lan9118Path->Lan9118.IfType = Snp->Mode->IfType;
// Initialise the protocol
Set the current (permanent was already being set) in the driver entry function. This allows the driver to be connected to before it is initialized.
Signed-off-by: Roy Franz roy.franz@linaro.org Contributed-under: TianoCore Contribution Agreement 1.0 --- ArmPlatformPkg/Drivers/LAN91xDxe/LAN91xDxe.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/ArmPlatformPkg/Drivers/LAN91xDxe/LAN91xDxe.c b/ArmPlatformPkg/Drivers/LAN91xDxe/LAN91xDxe.c index f82bb33..0a78e4d 100644 --- a/ArmPlatformPkg/Drivers/LAN91xDxe/LAN91xDxe.c +++ b/ArmPlatformPkg/Drivers/LAN91xDxe/LAN91xDxe.c @@ -986,6 +986,7 @@ Probe (
// Read and save the Permanent MAC Address LanDriver->SnpMode.PermanentAddress = GetCurrentMacAddress (LanDriver); + LanDriver->SnpMode.CurrentAddress = LanDriver->SnpMode.PermanentAddress; DEBUG((EFI_D_ERROR, //EFI_D_NET | EFI_D_INFO, "LAN91x: HW MAC Address: %02x-%02x-%02x-%02x-%02x-%02x\n", LanDriver->SnpMode.PermanentAddress.Addr[0], @@ -1057,8 +1058,6 @@ SnpStart ( ReturnUnlock (EFI_DEVICE_ERROR); }
- // Set the Current MAC address - Mode->CurrentAddress = Mode->PermanentAddress;
// Change state Mode->State = EfiSimpleNetworkStarted;
Hi Olivier,
I dug through the edk2 lists to find the discussion of the disputed networking patch, and I think that this problem/fix are related to that. I don't think I was able to find the complete threads, so I am likely missing some background. I wrote the patches in the previous email based on examining the code, and before finding the discussion on the mailing list. After spending some time reading the specs, I think that initializing the Snp.Mode structure in the LAN9xxx driver entry points as my patches do is the proper way to handle this. I have confirmed that your patch to ArpDriver.c is not needed with the changes in this series.
On p. 944 of the UEFI specification (version 2.4, June 2013) the description of the EFI_SIMPLE_NETWORK_MODE structure says that all the fields must be discovered during driver initialization. What is confusing here is that the SNP protocol has a method called "Initialize", but I don't think that method does "driver initialization."
From Section 4.1, page 82:
"If the driver does not follow the UEFI Driver Model, then it performs any required initialization and installs its protocol services before returning."
Since the LAN9xxx drivers do not use the EFI_DRIVER_BINDING_PROTOCOL, they do not follow the UEFI driver model, and the above line applies to them. All of the prohibitions of drivers not touching hardware in their entry function are referring to ones that do follow the UEFI driver model, and use the EFI_DRIVER_BINDING_PROTOCOL.
In Section 4.7.1, p95 "Image Entry Point Examples", the examples of of drivers not following the the UEFI driver model show "driver initialization" being implemented in the entry point.
Based on the above, I think that the patch series is consistent with the specification, and based on what I saw of the previous discussion should be acceptable upstream.
Thanks, Roy
On Thu, Feb 6, 2014 at 8:53 PM, Roy Franz roy.franz@linaro.org wrote:
I'd like some feedback on this approach before I send this to the EDK2 list.
Networking initialization broke in 2014.01 due reliance on a specific ordering of driver initialization that is not enforced. In particular, networking only worked if the Ip4ConfigDriverBinding protocol was started after the ArpDriverBinding protocol. This was due to the Arp driver startup causing the initialization of the LAN9xxx driver. These two binding protocols have equal "versions", which control the ordering, so it seems that the ordering between the two is arbitrary. (I have not tracked down exactly why the ordering is different in the two releases.)
There were two different problems in the failing case:
- The MnpGetModeData() function would not update the SnpMode structure it
was passed if the SNP driver was not started, even though the Ip4Config caller did not treat EFI_NOT_STARTED as an error. Ip4ConfigDriverBindingStart() would then proceed to use a SnpMode structure with unitialized stack data. The first patch in the series updates MnpGetModeData() to not treat EFI_NOT_STARTED as an error and to update the SnpMode structure in this case. With this fix, networking initialization is still broken, but at least no unitialized data is used.
- The LAN9xxx drivers did not update the SnpMode MAC addresses until the driver
was started or initialized (they each did it differently.) The next 2 patches update the driver to set the SnpMode MAC addresses at driver entry, so they are available at the time the Ip4Config driver binds with the LAN9xxx driver.
Roy Franz (3): Mnp Error handling LAN9118: set SNP MAC in entry function LAN91x: Set SNP current MAC address in entry function
ArmPlatformPkg/Drivers/LAN9118Dxe/LAN9118Dxe.c | 9 +++------ ArmPlatformPkg/Drivers/LAN91xDxe/LAN91xDxe.c | 3 +-- MdeModulePkg/Universal/Network/MnpDxe/MnpMain.c | 2 +- 3 files changed, 5 insertions(+), 9 deletions(-)
-- 1.7.10.4
Hi Roy, I have just pinged the MdeModulePkg maintainer for the MnpDxe patch I sent 7 months ago. If I am lucky my patch might get some consideration from the maintainer and get merged...
Have a look at the EDK2 mailing-list, you might have a chance to get your change after mine. Your patch looks correct, but I am not the maintainer of this package.
Thanks, Olivier
-----Original Message----- From: Roy Franz [mailto:roy.franz@linaro.org] Sent: 07 February 2014 04:53 To: linaro-uefi@lists.linaro.org; Olivier Martin Cc: Roy Franz Subject: [PATCH 1/3] Mnp Error handling
Change MnpGetModeData() handling of return status from SnpStatus to not consider EFI_NOT_STARTED an error, similar to how Ip4ConfigDriverBindingStart() handles it.
Without this change, Ip4ConfigDriverBindingStart() considers the Mnp->GetModeData successful even though the SnpMode parameter is unchanged, and is full of unitialized stack data.
If this change is not correct, then other error handling changes need to be made in Ip4ConfigDriverBindingStart() so that uninitialized data from the stack is not used.
Signed-off-by: Roy Franz roy.franz@linaro.org Contributed-under: TianoCore Contribution Agreement 1.0
MdeModulePkg/Universal/Network/MnpDxe/MnpMain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/MdeModulePkg/Universal/Network/MnpDxe/MnpMain.c b/MdeModulePkg/Universal/Network/MnpDxe/MnpMain.c index 795f063..c628991 100644 --- a/MdeModulePkg/Universal/Network/MnpDxe/MnpMain.c +++ b/MdeModulePkg/Universal/Network/MnpDxe/MnpMain.c @@ -80,7 +80,7 @@ MnpGetModeData ( // will be updated to reflect any change of media status // Status = Snp->GetStatus (Snp, &InterruptStatus, NULL);
- if (!EFI_ERROR (Status)) {
- if (!EFI_ERROR (Status) || (Status == EFI_NOT_STARTED)) { CopyMem (SnpModeData, Snp->Mode, sizeof (*SnpModeData)); } }
-- 1.7.10.4
-
Olivier Martin -
Roy Franz