Re: [PATCH v3 1/1] ArmVirtPkg: Replace obsoleted network drivers from platform DSC/FDF.

[Laszlo Ersek <lersek@redhat.com>]

On 11/06/18 13:32, Ard Biesheuvel wrote:
> On 6 November 2018 at 02:24, Fu Siyuan <siyuan.fu@intel.com> wrote:
>> V3:
>> Remove duplicate library added in v2 patch, since ArmVirtPkg.dsc.inc
>> already have them. Just remove the if...end there is enough.
>>
>> V2:
>> Add missing library instance for NetworkPkg iSCSI driver.
>>
> 
> Please don't put the patch revision history in the commit log. Put it
> below the ---
> 
>> This patch replaces the MdeModulePkg TCP, PXE and iSCSI driver with those
>> ones in NetworkPkg. These 3 drivers in MdeModulePkg are not being actively
>> maintained and will be removed from edk2 master soon.
>>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Julien Grall <julien.grall@linaro.org>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Fu Siyuan <siyuan.fu@intel.com>
>> ---
> 
> ... here ...
> 
> The patch looks fine to me
> 
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
> but please don't merge it until after the next stable tag has been created

This is not a bad idea (see also your discussion with Leif); however it
does create a bit of inconsistency with how the other platform DSC/FDF
files have been handled. (The changes have been pushed for those.)

Again, I don't disagree, and I don't mind if ArmVirt is handled
differently. It's just that we should have handled this more uniformly,
I believe.


In retrospect, I would have also appreciated if the patches had
referenced <https://bugzilla.tianocore.org/show_bug.cgi?id=1278>, even
though they only implement "prep" work for now, on the platform DSC/FDF
level, and not the actual driver removal.

For example, the important explanation about MdeModulePkg's iSCSI driver
implementing its own MD5 algo cannot be connected to the OVMF commit now
(d2f1f6423bd1). I have copied the most relevant passage from the cover
letter of this series into TianoCore BZ#1278, but the commit in question
doesn't reference any BZ, so the link cannot be established.

Thanks
Laszlo