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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#50917): https://edk2.groups.io/g/devel/message/50917 Mute This Topic: https://groups.io/mt/60556595/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
