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 <ler...@redhat.com>
>> Sent: Wednesday, May 20, 2020 1:18 AM
>> To: Leif Lindholm <l...@nuviainc.com>; Gao, Liming <liming....@intel.com>
>> Cc: Kinney, Michael D <michael.d.kin...@intel.com>; af...@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
>> 20200515073848.13920-1-wei6.xu@intel.com">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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#59991): https://edk2.groups.io/g/devel/message/59991
Mute This Topic: https://groups.io/mt/74322386/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
