Re: Patch List for 201908 stable tag

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

On 08/16/19 10:00, Gao, Liming wrote:
> 
> From: Gao, Liming [mailto:liming.gao@intel.com]
> Sent: Friday, August 16, 2019 3:59 PM
> To: Laszlo Ersek (lersek@redhat.com) <lersek@redhat.com>; leif.lindholm@linaro.org; Kinney, Michael D <michael.d.kinney@intel.com>; afish@apple.com; Cetola, Stephano <stephano.cetola@intel.com>
> Cc: edk2-devel@lists.01.org
> Subject: Patch List for 201908 stable tag
> 
> Hi Stewards and all:
>   I collect current patch lists in devel mail list. Those patch contributors request to add them for 201908 stable tag. Because the time is very close to Soft Feature Freeze, I want to collect your feedback for below patches.
> 
> Feature List (those all have pass code review):
> https://edk2.groups.io/g/devel/message/45734 [PATCH v6 0/5] Build cache enhancement

Seems to be well documented (both in commit messages and in the BZ), and
the series was approved by a BaseTools maintainer [1] before the soft
feature freeze [2].

[1] https://edk2.groups.io/g/devel/message/45783
[2] https://edk2.groups.io/g/devel/message/45806

The series can be pushed during the soft feature freeze, according to
the current SFF definition.

Some requests regarding the bugzilla:

- Comment 0 in the bugzilla says "redesign the platform hash algorithm".
I feel that would be better reflected if we changed the Product field to
TianoCore Feature Requests.

- Please capture the v1..v6 cover letters, with mailing list archive
links, in the bugzilla.

> https://edk2.groups.io/g/devel/message/45707 [PATCH] UefiCpuPkg: Adding a new TSC library by using CPUID(0x15) TSC leaf

Accepted by a UefiCpuPkg maintainer [3] before the SFF [2].

[3] https://edk2.groups.io/g/devel/message/45787

Can be pushed during the soft feature freeze, according to the current
SFF definition.

According to my comments sent earlier today for this series, the
documentation and the bugzilla status are extremely lacking. I'm OK with
pushing the series, but those aspects should be fixed in the bugzilla.
Please see <https://edk2.groups.io/g/devel/message/45826>.

In addition, it appears multiple versions of the patch have been sent,
without using "vN" identifiers in the subject prefix. Please collect all
versions of the patch series from the mailing list archive, in
chronological order, and link them all into the bugzilla ticket.

> https://edk2.groups.io/g/devel/message/45503 [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro

I commented on this today, as well:
<https://edk2.groups.io/g/devel/message/45827>

I disagree with the maintainer approval for this patch, from a reviewer
perspective. I think the submission isn't complete enough / mature
enough at this stage. The first version of the patch was posted on 12
Aug <https://edk2.groups.io/g/devel/message/45451>, as far as I can see.
Why are we in a mortal hurry to add a central macro without any call
sites in edk2 proper?

Anyway I'm not going to NACK the patch. If Mike is fine with the patch
as-is, I'll live with that, with my disagreement noted.

> Bug List:
> https://edk2.groups.io/g/devel/message/45794  [PATCH 1/1] CryptoPkg: Fix coding style

Purely from the subject line, looks like a bugfix, so no conflict with
the *soft* feature freeze.

> https://edk2.groups.io/g/devel/message/45791 [PATCH v2 1/1] ShellPkg/UefiShellAcpiViewCommandLib: Replace shift logical left

Clearly a bugfix, so ditto.

> https://edk2.groups.io/g/devel/message/45793 [Patch 1/1] BaseTools: Fixed issue of incorrect Module Unique Name

Ditto.

> https://edk2.groups.io/g/devel/message/45773 [Patch v4 0/6] Add "test then write" mechanism

Sigh, this is a messy question.

To my eyes, this introduces a feature. The cover letter says "add ...
mechanism". Even though it is used to fix a bug, it still introduces a
new facility, in my opinion. So here I disagree with
<https://edk2.groups.io/g/devel/message/45590>.

For UefiCpuPkg, Ray and Eric are Maintainers, and I'm a reviewer. The
submitter is Eric, and Ray is away. Eric asked me to review
<https://edk2.groups.io/g/devel/message/45489>.

I reviewed v2. Then v3 was posted (not sure in response to what, as I
can't see any comments under v2 that requested the v3 changes). V3
received a bunch of comments, so v4 was posted quickly afterwards.

The posting of v4 was still in time for the SFF, but I only arrived at
it after the SFF. (And Ray is still away.) Technically speaking, in my
book, this series has missed the stable tag. But that's not the
submitter's fault, arguably.

On this, I am undecided. I will defer to the other stewards. If they are
fine with the series going in, I'm OK too.

> https://edk2.groups.io/g/devel/message/45317 [Patch] MdeModulePkg DxeCore: Fix for missing MAT update

This is a bugfix and therefore there is no conflict with the soft
feature freeze. Jian provided a maintainer R-b, and I asked for a
comment style cleanup and some documentation (commit message)
improvements. In addition, perhaps we should conditionalize the logic on
the gEfiMemoryTypeInformationGuid GUID HOB. (The discussion between
Liming and myself on where the HOB can come from is irrelevant in that
regard -- wherever the HOB comes from, it is suitable for checking in
the DXE core, I think.)

I think it's fine to submit a v2 and merge it during the soft feature
freeze (again, bugfix, so no conflict). I may not be around to review
v2, so don't wait for my feedback.

Thanks
Laszlo