Re: Patch List for 201911 stable tag

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

On 11/19/19 15:25, Gao, Liming wrote:
> Hi Stewards and all:
>   I collect current patch lists in devel mail list. Those patch
>   contributors request to add them for 201911 stable tag. Because the
>   time is close to Hard 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/50602 [PATCH V2] BaseTools: Add [packages] section in dsc file

This patch can be merged during the Soft Feature Freeze. It was posted
before the Soft Feature Freeze, and also reviewed (by Bob, i.e. a
BaseTools Maintainer) before the Soft Feature Freeze.

As far as I can see, there is still an outstanding question from you, to
Zhiju ("Can you show what test are done for this new support?"), so I
think we should await the response to that.

Note that the patch should not be merged once the Hard Feature Freeze
starts, so there are ~3 days for Zhiju to answer the question about
testing (and for you to acknowledge that you are OK with the reply).

> Bug List (those all have pass code review):
> https://edk2.groups.io/g/devel/message/50625 [PATCH v1] MdeModulePkg/NvmExpressDxe: Fix wrong queue size for async IO queues

Looks very much like a bugfix to me, so it's suitable for merging even
during the Hard Feature Freeze.

> https://edk2.groups.io/g/devel/message/50406 [PATCH 1/1] MdePkg/Include: Add missing definitions of SMBIOS type 42h in SmBios.h

Based on Abner's response in the thread, this change does not appear
necessary for fixing actual functionality bugs; it rather completes a
previously incomplete feature addition. And Abner is not in a rush to
catch the upcoming stable tag with the patch. I suggest to delay it.

If others disagree, I won't insist; the above is just my preference.

> https://edk2.groups.io/g/devel/message/50661 [PATCH] UefiCpuPkg: Update the coding styles

Hmmm, quite undecided on this one. Does not fix a functionality bug
either, but what it fixes *are* a coding style bugs, and the patch is
low risk. I'm leaning towards merging it.

> https://edk2.groups.io/g/devel/message/50662 [PATCH] MdePkg: Update the comments of IsLanguageSupported

This was even reviewed by a package maintainer (= you) before the SFF,
so it can definitely go in.

> https://edk2.groups.io/g/devel/message/50663 [PATCH 0/3] Add missing strings for uni files

First of all, the structure of this series is wrong; please see my
feedback here:

  https://edk2.groups.io/g/devel/message/50666

(The two patches discussed just above were incorrectly included in the
same posting.)

Second, the three patches for the UNI files add too much brand new text
for my taste, for them to be considered bugfixes. The patches were
posted in time for the SFF, but the maintainer reviews came too late:

  https://edk2.groups.io/g/devel/message/50872
  https://edk2.groups.io/g/devel/message/50869
  https://edk2.groups.io/g/devel/message/50870

I suggest postponing.

> https://edk2.groups.io/g/devel/message/50866 [PATCH V1 0/2] Improve PeiInstallPeiMemory() description

I'm seriously confused by the subject prefixes in this patch thread.
What's going on with the version numbers?

  [edk2-devel] [PATCH V1 0/2] Improve PeiInstallPeiMemory() description
  [edk2-devel] [PATCH V3 1/2] MdeModulePkg PeiCore: Improve PeiInstallPeiMemory() description
  [edk2-devel] [PATCH V1 2/2] MdePkg PiPeiCis.h: Improve PeiInstallPeiMemory() description

Other than that... I'm torn. I guess I could be convinced that these
patches are indeed bugfixes, so I'm leaning towards merging them.

> https://edk2.groups.io/g/devel/message/50841 [PATCH V2 1/1] MdeModulePkg PeiCore: Fix typos

Personally I'm not happy about this patch. It's way too large for my taste:

 MdeModulePkg/Core/Pei/PeiMain.inf             | 10 ++--
 MdeModulePkg/Core/Pei/FwVol/FwVol.h           | 20 +++----
 MdeModulePkg/Core/Pei/PeiMain.h               | 52 ++++++++--------
 MdeModulePkg/Core/Pei/Dependency/Dependency.c | 12 ++--
 MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 51 ++++++++--------
 MdeModulePkg/Core/Pei/FwVol/FwVol.c           | 63 ++++++++++----------
 MdeModulePkg/Core/Pei/Hob/Hob.c               |  4 +-
 MdeModulePkg/Core/Pei/Image/Image.c           | 10 ++--
 MdeModulePkg/Core/Pei/Memory/MemoryServices.c | 18 +++---
 MdeModulePkg/Core/Pei/PeiMain/PeiMain.c       |  2 +-
 MdeModulePkg/Core/Pei/Ppi/Ppi.c               |  4 +-
 MdeModulePkg/Core/Pei/Security/Security.c     | 12 ++--
 12 files changed, 129 insertions(+), 129 deletions(-)

and it mixes multiple kinds of changes:

"Fixes typos and clarifies some wording throughout PeiCore."

When reviewing such a patch, the reviewer has a difficult time telling
apart purely syntactic (typo) fixes from semantic (wording) fixes. As a
reviewer I would suggest splitting this patch at least in two (typos vs.
semantics). Then I could be convinced such a set of two patches is
purely a bugfix.

I'm leaning towards "postpone" on this one, but I can see why people
would think "that's arbitrary". I guess I'll have to defer to others in
this instance.

Thanks
Laszlo