Re: Patch List for 201911 stable tag


Liming Gao
 

Laszlo and Leif:
Thanks for your detail review. I will continue to monitor the coming changes for 201911 stable tag.

Thanks
Liming

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Leif Lindholm
Sent: Wednesday, November 20, 2019 3:02 AM
To: Laszlo Ersek <lersek@redhat.com>
Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; 'afish@apple.com' <afish@apple.com>;
devel@edk2.groups.io
Subject: Re: [edk2-devel] Patch List for 201911 stable tag

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

Join devel@edk2.groups.io to automatically receive all group messages.