On 07/08/15 07:18, Wu, Jiaxin wrote:
> Hi Laszlo,
> Thanks for your comments first.
> Ip4Dxe driver update patch has been sent out and reviewed, we have
> submitted to edk2 already. So, that means the packages I listed will
> consume Ip4Config2 protocol integrated in Ip4Dxe driver directly.
What patch are you *specifically* referring to?
Are you referring to the email
[edk2] [Patch] MdeModulePkg: Update Ip4Dxe driver to support
Ip4Config2 protocol, and also add UI support in Ip4Dxe
driver.
http://thread.gmane.org/gmane.comp.bios.tianocore.devel/15823
If that is the patch that you have in mind, I have a couple of notes:
- Yes, that patch seems to install gEfiIp4Config2ProtocolGuid, in the
Ip4DriverBindingStart() function.
- However, the INF file change (for
"MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Dxe.inf") is not correct.
It uses the TO_START comment for the protocol GUID, while it should
by BY_START.
- You are saying that the patch has been reviewed. I cannot see any
discussion in that thread (or any other thread with the same subject).
Your initial posting also doesn't carry any Reviewed-by tags (from
internal reviewers at Intel).
> For patch series, those patches used to update related pkgs and
> remove Ip4ConfigDxe driver. I will submit related pkgs to edk2
> first, and then remove Ip4ConfigDxe driver. I don't think the patch
> series has any issue.
The issue here is that your current patchset (that removes version 1 of
the protocol from a number of packages) *depends* on your patch that
adds version 2 of the protocol. The ordering between the patches must be:
- add version 2 of the protocol
- migrate drivers to version 2 of the protocol
- remove version 1 of the protocol
*Unless* you spell out this dependency in your patch submission
explicitly, there is no guarantee that the independently submitted
patches will not be committed in reverse order. If you commit them in
reverse order, then there will exist a state in the edk2 tree where
those platforms are broken, because they have *no* version of the ip4
config protocol.
And yes, that is bad. When someone looks for an independent bug with
git-bisect, they might end up at that exact broken state in the tree.
So, this patch set is not broken *per se*. The breakage is that you did
not spell out either the justification for this set, or the strict
dependency this patch has on other patches of yours.
When I reviewed this series of yours, I realized the dependency myself.
Not knowing about your other patches, I went to the edk2 tree and
verified the dependency manually. The dependency was not satisfied at
that point, inside the tree, which is why I rejected your patch. (If you
patch had been committed at that exact moment, it would have broken OVMF.)
*Now* we have a different situation of course. I can see that your patch
that I referenced above, providing gEfiIp4Config2ProtocolGuid, has been
committed to the tree:
commit 1f6729ffe98095107ce82e67a4a0209674601a90
Author: jiaxinwu <jiaxin...@intel.com>
Date: Tue Jul 7 08:19:55 2015 +0000
MdeModulePkg: Update Ip4Dxe driver to support Ip4Config2 protocol,
and also add new UI configuration support in Ip4Dxe driver.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: jiaxinwu <jiaxin...@intel.com>
Reviewed-by: Ye Ting <ting...@intel.com>
git-svn-id: https://svn.code.sf.net/p/edk2/code/trunk/edk2@17853
6f19259b-4bc3-4df7-8a09-765794883524
I have a couple notes for this as well:
- This patch was never submitted to the list for review. Minimally, the
subject line has been changed relative to the patch I linked above.
- The patch carries the Reviewed-by tag of Ye Ting. Where was that
Reviewed-by tag posted to the public mailing list? If you got it in
private, that's useless.
- The patch with Ip4Config2 protocol support was committed on
2015-07-07, at 10:19:55 (in my time zone, UTC+2).
However, you submitted your patch that removed version 1 of the
protocol *earlier* than that, on the same day, but about 7 hours
earlier. I may not have reviewed your patch against a fully
up-to-date tree, but when you submitted the version 1 removal patch,
without any hints at dependencies, it was *out of order*.
So, *assuming* that all version 2 protocol dependencies have been
provided / updated in the edk2 tree by now, this OvmfPkg patch is correct.
Please be much more explicit about inter-patch dependencies the next
time. One way to do that is to organize all related patches in a single
patch series. (Unless it becomes too big.) Another way is to reference
the prerequisite patches / series, by subject and URL, in the cover
letter (eg. 0/7) of the dependent patch set.
Finally, the commit message of this specific patch should state *why* it
is safe to remove the Ip4ConfigDxe module from OvmfPkg -- because Ip4Dxe
provides the Ip4Config2 protocol, and because all network drivers built
into OVMF that used to depend on Ip4Config have been migrated to Ip4Config2.
Thanks
Laszlo
>
> Thanks.
> Jiaxin
>
> -----Original Message-----
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Tuesday, July 7, 2015 8:56 PM
> To: Wu, Jiaxin
> Cc: edk2-devel@lists.sourceforge.net
> Subject: Re: [edk2] [Patch 3/7] OvmfPkg: Remove Ip4ConfigDxe module from
> OvmfPkg.
>
> On 07/07/15 03:27, jiaxinwu wrote:
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: jiaxinwu <jiaxin...@intel.com>
>> ---
>> OvmfPkg/OvmfPkgIa32.dsc | 1 -
>> OvmfPkg/OvmfPkgIa32.fdf | 1 -
>> OvmfPkg/OvmfPkgIa32X64.dsc | 1 -
>> OvmfPkg/OvmfPkgIa32X64.fdf | 1 -
>> OvmfPkg/OvmfPkgX64.dsc | 1 -
>> OvmfPkg/OvmfPkgX64.fdf | 1 -
>> 6 files changed, 6 deletions(-)
>>
>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc index
>> bb008f6..c3508e6 100644
>> --- a/OvmfPkg/OvmfPkgIa32.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32.dsc
>> @@ -524,11 +524,10 @@
>> MdeModulePkg/Universal/Network/DpcDxe/DpcDxe.inf
>> MdeModulePkg/Universal/Network/MnpDxe/MnpDxe.inf
>> MdeModulePkg/Universal/Network/VlanConfigDxe/VlanConfigDxe.inf
>> MdeModulePkg/Universal/Network/ArpDxe/ArpDxe.inf
>> MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Dxe.inf
>> - MdeModulePkg/Universal/Network/Ip4ConfigDxe/Ip4ConfigDxe.inf
>> MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Dxe.inf
>> MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Dxe.inf
>> MdeModulePkg/Universal/Network/Udp4Dxe/Udp4Dxe.inf
>> !if $(NETWORK_IP6_ENABLE) == TRUE
>> NetworkPkg/Ip6Dxe/Ip6Dxe.inf
>> diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf index
>> 24ad0bf..aadb099 100644
>> --- a/OvmfPkg/OvmfPkgIa32.fdf
>> +++ b/OvmfPkg/OvmfPkgIa32.fdf
>> @@ -302,11 +302,10 @@ FILE FREEFORM =
>> PCD(gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdLogoFile) {
>> INF MdeModulePkg/Universal/Network/DpcDxe/DpcDxe.inf
>> INF MdeModulePkg/Universal/Network/MnpDxe/MnpDxe.inf
>> INF MdeModulePkg/Universal/Network/VlanConfigDxe/VlanConfigDxe.inf
>> INF MdeModulePkg/Universal/Network/ArpDxe/ArpDxe.inf
>> INF MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Dxe.inf
>> - INF MdeModulePkg/Universal/Network/Ip4ConfigDxe/Ip4ConfigDxe.inf
>> INF MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Dxe.inf
>> INF MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Dxe.inf
>> INF MdeModulePkg/Universal/Network/Udp4Dxe/Udp4Dxe.inf
>> !if $(NETWORK_IP6_ENABLE) == TRUE
>> INF NetworkPkg/Ip6Dxe/Ip6Dxe.inf
>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
>> index fed584f..40a1cf1 100644
>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>> @@ -531,11 +531,10 @@
>> MdeModulePkg/Universal/Network/DpcDxe/DpcDxe.inf
>> MdeModulePkg/Universal/Network/MnpDxe/MnpDxe.inf
>> MdeModulePkg/Universal/Network/VlanConfigDxe/VlanConfigDxe.inf
>> MdeModulePkg/Universal/Network/ArpDxe/ArpDxe.inf
>> MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Dxe.inf
>> - MdeModulePkg/Universal/Network/Ip4ConfigDxe/Ip4ConfigDxe.inf
>> MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Dxe.inf
>> MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Dxe.inf
>> MdeModulePkg/Universal/Network/Udp4Dxe/Udp4Dxe.inf
>> !if $(NETWORK_IP6_ENABLE) == TRUE
>> NetworkPkg/Ip6Dxe/Ip6Dxe.inf
>> diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
>> index 5f98be8..bc42a2b 100644
>> --- a/OvmfPkg/OvmfPkgIa32X64.fdf
>> +++ b/OvmfPkg/OvmfPkgIa32X64.fdf
>> @@ -302,11 +302,10 @@ FILE FREEFORM =
>> PCD(gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdLogoFile) {
>> INF MdeModulePkg/Universal/Network/DpcDxe/DpcDxe.inf
>> INF MdeModulePkg/Universal/Network/MnpDxe/MnpDxe.inf
>> INF MdeModulePkg/Universal/Network/VlanConfigDxe/VlanConfigDxe.inf
>> INF MdeModulePkg/Universal/Network/ArpDxe/ArpDxe.inf
>> INF MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Dxe.inf
>> - INF MdeModulePkg/Universal/Network/Ip4ConfigDxe/Ip4ConfigDxe.inf
>> INF MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Dxe.inf
>> INF MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Dxe.inf
>> INF MdeModulePkg/Universal/Network/Udp4Dxe/Udp4Dxe.inf
>> !if $(NETWORK_IP6_ENABLE) == TRUE
>> INF NetworkPkg/Ip6Dxe/Ip6Dxe.inf
>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc index
>> 92af10f..ac4e731 100644
>> --- a/OvmfPkg/OvmfPkgX64.dsc
>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>> @@ -529,11 +529,10 @@
>> MdeModulePkg/Universal/Network/DpcDxe/DpcDxe.inf
>> MdeModulePkg/Universal/Network/MnpDxe/MnpDxe.inf
>> MdeModulePkg/Universal/Network/VlanConfigDxe/VlanConfigDxe.inf
>> MdeModulePkg/Universal/Network/ArpDxe/ArpDxe.inf
>> MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Dxe.inf
>> - MdeModulePkg/Universal/Network/Ip4ConfigDxe/Ip4ConfigDxe.inf
>> MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Dxe.inf
>> MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Dxe.inf
>> MdeModulePkg/Universal/Network/Udp4Dxe/Udp4Dxe.inf
>> !if $(NETWORK_IP6_ENABLE) == TRUE
>> NetworkPkg/Ip6Dxe/Ip6Dxe.inf
>> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf index
>> 11e74e7..b6992bd 100644
>> --- a/OvmfPkg/OvmfPkgX64.fdf
>> +++ b/OvmfPkg/OvmfPkgX64.fdf
>> @@ -302,11 +302,10 @@ FILE FREEFORM =
>> PCD(gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdLogoFile) {
>> INF MdeModulePkg/Universal/Network/DpcDxe/DpcDxe.inf
>> INF MdeModulePkg/Universal/Network/MnpDxe/MnpDxe.inf
>> INF MdeModulePkg/Universal/Network/VlanConfigDxe/VlanConfigDxe.inf
>> INF MdeModulePkg/Universal/Network/ArpDxe/ArpDxe.inf
>> INF MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Dxe.inf
>> - INF MdeModulePkg/Universal/Network/Ip4ConfigDxe/Ip4ConfigDxe.inf
>> INF MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Dxe.inf
>> INF MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Dxe.inf
>> INF MdeModulePkg/Universal/Network/Udp4Dxe/Udp4Dxe.inf
>> !if $(NETWORK_IP6_ENABLE) == TRUE
>> INF NetworkPkg/Ip6Dxe/Ip6Dxe.inf
>>
>
> I don't see any driver in the edk2 tree that installs
> gEfiIp4Config2ProtocolGuid, at the moment.
>
> In your other email you wrote:
>
>> Ip4ConfigDxe driver is deprecated in UEFI 2.5, so we will not support
>> original Ip4Config Protocol, which is replace by Ip4Config2 Protocol
>> integrated in Ip4Dxe driver.
>
>> Related pkgs(MdeModulePkg, Nt32Pkg, ArmPlatformPkg, ArmVirtPkg,
>> EmulatorPkg, OvmfPkg, Vlv2TbltDevicePkg) need to remove Ip4ConfigDxe
>> module and related guid definitions, otherwise they will meet build
>> failure issue after we update Ip4Dxe driver and deprecate Ip4ConfigDxe
>> driver.
>
> However, if these patches were committed first, then the packages you listed
> would have no Ip4Config protocol *at all*, either version 1 or version 2.
> Because, as you said, Ip4Config2 will only be implemented in the future (in
> Ip4Dxe).
>
> That would break IPv4 networking for all of these packages: Ip4Dxe.inf lists
> "gEfiIp4ConfigProtocolGuid" (that is, version 1) as TO_START.
>
> The right way to implement such changes is the following:
> - First implement the new (version 2) protocol in the edk2 tree. This
> can be a separate, initial patch set, or else the first segment of a
> larger patch series.
> - Then flip client packages over to the new protocol version. This is
> where the old driver INF can be removed from the DSC and FDF files.
> - Finally, you can delete Ip4ConfigDxe from the tree completely.
>
> In other words, the tree must build, and preferably even work, at *any* stage
> within your patch series. The patches must preserve bisectability.
>
> Therefore, I don't think this patch series is good, in its current form.
> Please consider the pattern I described above.
>
> Thanks
> Laszlo
>
------------------------------------------------------------------------------
Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
