Re: Patch List for 202005 stable tag

["Liming Gao" <liming.gao@intel.com>]

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.

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.

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. 

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