Re: [edk2-devel] Patch List for 201911 stable tag

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Leif
> Lindholm
> Sent: Wednesday, November 20, 2019 3:02 AM
> To: Laszlo Ersek
> Cc: Gao, Liming; Kinney, Michael D; '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.


Thanks Leif,

I will create a pull request to push this bugfix ASAP.

Best Regards,
Hao Wu


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