Re: Patch List for 202005 stable tag

["Laszlo Ersek" <lersek@redhat.com>]

On 05/20/20 16:43, Gao, Liming wrote:
> Leif and Laszlo:
>   I add my comments for previous three. Besides, I have got several changes for edk2 202005 stable tag. 
> 
> Bug List (pass review)
> 2. https://edk2.groups.io/g/devel/message/59921 [PATCH] IntelFsp2Pkg: Add FunctionParametePtr to FspGlobalData
> [Chiu, Chasel] We missed something in previous FSP enhancement commit (https://edk2.groups.io/g/devel/message/59438) and this patch fixed the bug.

See my response in that thread ("I guess it qualifies").

> 
> 3. https://edk2.groups.io/g/devel/message/59914 [PATCH v2] NetworkPkg/DxeNetLib: Change the order of conditions in IF statement
> [Liming] This is a bug. It has been merged in https://github.com/tianocore/edk2/pull/634 by Package maintainer.
> 
> 4. https://edk2.groups.io/g/devel/message/59937 [PATCH] MdePkg: add definitions for ACPI NVDIMM Device Path
> [Liming] This is one missing definition in MdePkg. I think it is fine to catch this stable tag.

I agree; minimally for consistency with how we've handled the first few
patches of the series "[PATCH V7 0/6] Add definitions introduced in UEFI
2.8a" (which also only add definitions).

> 
> 5. https://edk2.groups.io/g/devel/message/56943 [PATCH v1 1/1] BaseTools: Remove deprecated Visual Studio Option 
> [Liming] This is the change in VS2017 and VS2019 tool chain. 

I agree this is a bugfix (and therefore it would qualify even during the
HFF).

Thanks,
Laszlo

> 
> Thanks
> Liming
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Wednesday, May 20, 2020 1:18 AM
>> To: Leif Lindholm <leif@nuviainc.com>; Gao, Liming <liming.gao@intel.com>
>> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; afish@apple.com; devel@edk2.groups.io
>> Subject: Re: Patch List for 202005 stable tag
>>
>> On 05/19/20 14:04, Leif Lindholm wrote:
>>> On Tue, May 19, 2020 at 11:09:42 +0000, Gao, Liming wrote:
>>>> Hi Stewards and all:
>>>>   I collect current patch lists in devel mail list. Those patch contributors request to add them for 200205 stable tag. Because we
>> have enter into Soft Feature Freeze, I want to collect your feedback for them. If any patches are missing, please reply this mail to add
>> them.
>>>>
>>>> Feature List (pass review):
>>>> https://edk2.groups.io/g/devel/message/59578 [PATCH V7 0/1] Disable safe string constraint assertions
>>>
>>> This one I might even be persuaded to wave through in hard
>>> freeze. Yes, please include.
>>
>> Agreed.
>>
> 
>>>> https://edk2.groups.io/g/devel/message/59589 [PATCH V7 0/6] Add definitions introduced in UEFI 2.8a
>>>
>>> 1,3-4 only adds definitions - I'm fine with those.
>>>
>>> 2/6 looks out of place compared with the set description, but as it
>>> falls within the scope of "align codebase with current spec", and it
>>> is a correction, it can stay. (It should also not break anything, and
>>> if it does, that's automatically a bug isn't it?)
>>
>> Patches v7 #1 through #4 should be merged, because they were posted
>> before the SFF, and they already carried an R-b from Liming (MdePkg
>> co-maintainer).
>>
>>> 5-6, I have no comment on. Someone else will have to make that call.
>>
>> Patches #5 and #6 seem to have been superseded by the following separate
>> series:
>>
>>   [edk2-devel] [PATCH 0/2] Add FMP Capsule Image Header extension
>>   https://edk2.groups.io/g/devel/message/59652
>>   http://mid.mail-archive.com/20200515073848.13920-1-wei6.xu@intel.com
>>
>> Now, this separate series was also posted (even if barely: 22 minutes)
>> before the SFF. The patches also contained some R-b tags upon posting.
>>
>> Patch #1 had an R-b from Liming, upon posting. The files it modifies are:
>> - MdeModulePkg/Application/CapsuleApp/CapsuleDump.c
>> - MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c
>>
>> I've run "BaseTools/Scripts/GetMaintainer.py" with the "-l" option on
>> both files. Both invocations list Liming as a designated reviewer. (From
>> section "MdeModulePkg: Firmware Update modules", as far as I can tell.)
>>
>> Patch #2 had an R-b from Liming and Chao Zhang, on posting. The patch
>> modifies the following file:
>> - SignedCapsulePkg/Universal/RecoveryModuleLoadPei/RecoveryModuleLoadPei.c
>>
>> "GetMaintainer.py" does list "Chao Zhang" with the "-l" option. (See the
>> "SignedCapsulePkg" section in Maintainers.txt".)
>>
>> So, assuming both patches included those R-b's *justifiedly* when there
>> were posted (that is, the R-b's were given earlier on the list), I think
>> this 2-part series too should be merged. Technically speaking, it was
>> fully reviewed as soon as it was posted, and it was posted in time.
>>
> [Liming] This patch set have been merged in https://github.com/tianocore/edk2/pull/635. 
> 
>>>
>>>> Bug List (pass review):
>>>> https://edk2.groups.io/g/devel/message/59735 [PATCH v3] SecurityPkg: Change default value source
>>>
>>> Patch is pretty garbled in groups.io. Thankfully, it looks better in
>>> gmail.
>>>
>>> I have a minor concern in that neither the comments on v2 nor the
>>> changelog in v3 indicates the (useful) addition of the bit-value
>>> lookup table for PcdTcg2PhysicalPresenceFlags in SecurityPkg.dec
>>> and its associated dropping of the line
>>> "# Default setting is TCG2_BIOS_TPM_MANAGEMENT_FLAG_DEFAULT | TCG2_BIOS_STORAGE_MANAGEMENT_FLAG_DEFAULT".
>>>
>>> But if SecurityPkg maintainers are happy they haven't missed any other
>>> changes in v3, I'm OK with this going in.
>>
>> I think even a v4 would be eligible for merging (with the commit message
>> improvements that you are suggesting). Even during the hard feature
>> freeze. (Possibly justifying an extension to the hard feature freeze.)
> 
> [Liming] I see SecurityPkg maintainters have given reviewed-by. So, I think it is fine to merge this change. 
>>
>> Here's why I think that:
>> <https://bugzilla.tianocore.org/show_bug.cgi?id=2713#c0> is a good issue
>> report. It explains that this patch is a bugfix. Namely, commit
>> 710174e01174 ("SecurityPkg: Tcg2PhysicalPresence: Define TCG2 PP Flags
>> Initial Pcd", 2016-12-29) was incomplete; it created an inconsistency in
>> physical presence flag defaults, between different code paths. And the
>> new patch fixes that.
>>
>> (On the negative side, three versions of the patch have been posted to
>> the list, and the bugzilla ticket has pointers to... none of those. The
>> last comment remains, at this time, "Still investigate the solution to
>> fix it". I find that really frustrating and sad.)
>>
>> Thanks,
>> Laszlo
>