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