On 07/08/15 17:41, Wu, Jiaxin wrote: > Hi Laszlo, > For your mentioned patch: > --[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) > > That was an old version patch used to update Ip4Dxe driver, which was > sent out on Thu 6/18/2015 2:35 PM, but seemed that patch was > *covered* because no one replied me up to Mon 6/29/2015. In the > meantime, I found several issues with that patch including *redundant > subject*, so I created another version2 patch with the new subject > called "MdeModulePkg: Update Ip4Dxe driver" on Mon 6/29/2015 2:46 PM. > After that, I'm glad to receive the reviewing replies from <Qiu, > Shumin> and <Carsey, Jaben> in edk2 community. Since then until Fri > 7/3/2015 1:39 PM, I sent out version3 patch with the same subject to > fix the reviewers' feedback, and <Ye Ting> gave me the Reviewed-by > tag. Detailed you can see attached email. I'm sorry for old > version1's inconsistent subject . But I don't know why you didn't > notice those new patches(version2, version3 and series reviewers' > replies) except for old one. For your inconvenience, please > understanding.
You are perfectly right, I can see the patch now, with the subject you named: MdeModulePkg: Update Ip4Dxe driver While writing my previous email, I could not find this patch on the mailing list for two reasons: - The updated patch subject does not contain the word "Ip4Config2". (In fact the new subject is quite "empty"; it doesn't capture any of the changes made to the driver.) - I also searched the mailing list with the subject that got *actually* committed as SVN r17853. (Because, obviously, I wanted to see the discussion and the reviews that led up to the patch being committed.) The subject line of the committed patch is: MdeModulePkg: Update Ip4Dxe driver to support Ip4Config2 protocol, (note the trailing comma), which yielded zero matches on the mailing list (except the one message that I linked in my email -- ie. your v1.) As to why I did not notice your v2 and v3 patches: it's very simple. The subject did not state OvmfPkg anywhere, and I was not Cc'd personally. Obviously, I'm not saying that I *should* have been Cc'd (or that the patch should have said OvmfPkg -- it was completely unrelated); I'm just saying that sometimes I don't have time to read the *full* traffic of the list, and I can focus only on stuff that directly concerns OvmfPkg / ArmVirtPkg, and/or is Cc'd to me. To summarize: the only *real* problem in this instance is that the final patch that got committed as SVN r17853 has a *different* subject than the final (v3) patch for which you had received all the reviews. (In general, it is helpful if you post the SVN revision to the list (in response to the patch email) after committing the patch to SVN.) > --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- > > For your notes: > -- 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. > > I will double check this item for the protocol GUID, if this is a bug > introduced by that my fix, I will correct it by following our processing. > Thanks for your finding. AFAIR, it has not been introduced by your patch; the problem had been there from earlier, with the version 1 protocol GUID. I just thought that since you were updating that line anyway, the hint could be fixed up as well. > > --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- > For your ordering between the patches: > -- add version 2 of the protocol > -- migrate drivers to version 2 of the protocol > -- remove version 1 of the protocol > > I agree absolutely. Actually, we do indeed. > First, The patch "Update Ip4Dxe driver" is used to add ip4config2 > protocol support; Yes. > Second, The committed patches "ping & ifconfig & UefiHandleParsingLib > & DxeNetLib & IpSecDxe & EfiSocketLib" are used to migrate related > drivers/libs to ip4config2 protocol; Right. > Finally, we sent out the series of patches as [PATCH 0/7] > cover-letter description to continue the related packages migrating > work and remove ip4config protocol. Removing ip4config protocol patch > together with related packages cleanup work was considering to save > time. But, I have realized that is a confused series of patches due > to my less consideration for patch reviewer. I will revise the patch > order and add more inter-patch dependencies descriptions according > your good suggestions. Looking at the SVN commit log, and your explanation, I do understand now (and agree) that the ordering has not been ignored. It's just that the position of 1/7 is not correct in this series (a minor issue, admittedly). I'm sorry I *assumed* that you didn't care about the ordering. Apparently you did care (and 1/7 is just a minor hiccup here), it just wasn't clear from the (very laconic) description. Thanks! Laszlo > --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- > > Thanks for everyone's great comments. > > Best Regards! > Jiaxin > > -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Wednesday, July 8, 2015 6:33 PM > To: Wu, Jiaxin > Cc: edk2-devel@lists.sourceforge.net > Subject: Re: [edk2] [Patch 3/7] OvmfPkg: Remove Ip4ConfigDxe module from > OvmfPkg. > > 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
