Re: Patch List for 201911 stable tag

["Leif Lindholm" <leif.lindholm@linaro.org>]

On Tue, Nov 19, 2019 at 06:50:19PM +0100, Laszlo Ersek wrote:
> 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).

Agreed.

> > 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.

I agree. But I am still slightly nervous about changing such a
fundamental part of such a fundamental driver. Certainly if it is
going in, I want it in ASAP, not just at the end of soft freeze - to
give us as much time as possible to revert it if the fix exposes
latent errors in previously working systems.

> > 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.

I'm OK either way.

> > 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.

I am against merging this, even though it's low-risk.

The process says:
"By the date of the soft feature freeze, developers must have sent
their patches to the mailing list and received positive maintainer
reviews (Reviewed-by or Acked-by tags)."
This received Acks 4 days late.

If it came with a commit message indicating the incorrect comment
syntax caused problems with document generation, then maybe it could
be considered from a bugfix standpoint. But it didn't and it's too
late to re-scope the change at this point.

I also dislike the mixing of doxygen formating changes and plain
whitespace changes. Even though trivial, it ought to be split up.

> > 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.

Agree (if cutting it close).

> > 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.

Agree.

> > 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.

Non-functional change submitted after start of soft-freeze?
I don't see why it should be considered.

> > 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.

Non-functional change submitted after start of soft-freeze?
I don't see why it should be considered.

I also agree on the needs splitting up bit.

Best Regards,

Leif