* Patch List for 201911 stable tag @ 2019-11-19 14:25 Liming Gao 2019-11-19 17:50 ` Laszlo Ersek 0 siblings, 1 reply; 9+ messages in thread From: Liming Gao @ 2019-11-19 14:25 UTC (permalink / raw) To: Gao, Liming, Laszlo Ersek (lersek@redhat.com), 'leif.lindholm@linaro.org', Kinney, Michael D, 'afish@apple.com' Cc: devel@edk2.groups.io [-- Attachment #1: Type: text/plain, Size: 1215 bytes --] 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 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 https://edk2.groups.io/g/devel/message/50406 [PATCH 1/1] MdePkg/Include: Add missing definitions of SMBIOS type 42h in SmBios.h https://edk2.groups.io/g/devel/message/50661 [PATCH] UefiCpuPkg: Update the coding styles https://edk2.groups.io/g/devel/message/50662 [PATCH] MdePkg: Update the comments of IsLanguageSupported https://edk2.groups.io/g/devel/message/50663 [PATCH 0/3] Add missing strings for uni files https://edk2.groups.io/g/devel/message/50866 [PATCH V1 0/2] Improve PeiInstallPeiMemory() description https://edk2.groups.io/g/devel/message/50841 [PATCH V2 1/1] MdeModulePkg PeiCore: Fix typos Thanks Liming _._,_._,_ [-- Attachment #2: Type: text/html, Size: 5360 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Patch List for 201911 stable tag 2019-11-19 14:25 Patch List for 201911 stable tag Liming Gao @ 2019-11-19 17:50 ` Laszlo Ersek 2019-11-19 19:01 ` Leif Lindholm 0 siblings, 1 reply; 9+ messages in thread From: Laszlo Ersek @ 2019-11-19 17:50 UTC (permalink / raw) To: Gao, Liming, 'leif.lindholm@linaro.org', Kinney, Michael D, 'afish@apple.com' Cc: devel@edk2.groups.io 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Patch List for 201911 stable tag 2019-11-19 17:50 ` Laszlo Ersek @ 2019-11-19 19:01 ` Leif Lindholm 2019-11-20 1:46 ` [edk2-devel] " Wu, Hao A ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Leif Lindholm @ 2019-11-19 19:01 UTC (permalink / raw) To: Laszlo Ersek Cc: Gao, Liming, Kinney, Michael D, 'afish@apple.com', devel@edk2.groups.io 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] Patch List for 201911 stable tag 2019-11-19 19:01 ` Leif Lindholm @ 2019-11-20 1:46 ` Wu, Hao A 2019-11-20 14:52 ` Liming Gao [not found] ` <15D8E68889A9A669.17632@groups.io> 2 siblings, 0 replies; 9+ messages in thread From: Wu, Hao A @ 2019-11-20 1:46 UTC (permalink / raw) To: devel@edk2.groups.io, leif.lindholm@linaro.org, Laszlo Ersek Cc: Gao, Liming, Kinney, Michael D, 'afish@apple.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 > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] Patch List for 201911 stable tag 2019-11-19 19:01 ` Leif Lindholm 2019-11-20 1:46 ` [edk2-devel] " Wu, Hao A @ 2019-11-20 14:52 ` Liming Gao [not found] ` <15D8E68889A9A669.17632@groups.io> 2 siblings, 0 replies; 9+ messages in thread From: Liming Gao @ 2019-11-20 14:52 UTC (permalink / raw) To: devel@edk2.groups.io, leif.lindholm@linaro.org, Laszlo Ersek Cc: Kinney, Michael D, 'afish@apple.com' 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 > > ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: <15D8E68889A9A669.17632@groups.io>]
* Re: [edk2-devel] Patch List for 201911 stable tag [not found] ` <15D8E68889A9A669.17632@groups.io> @ 2019-11-21 8:57 ` Liming Gao 2019-11-21 10:37 ` Leif Lindholm 0 siblings, 1 reply; 9+ messages in thread From: Liming Gao @ 2019-11-21 8:57 UTC (permalink / raw) To: devel@edk2.groups.io, Gao, Liming, leif.lindholm@linaro.org, Laszlo Ersek, Kinney, Michael D, 'afish@apple.com' Hi Stewards and all: New bugs are for 201911 stable tag. Can you give the comments for them? Bug List (those all have pass code review): https://edk2.groups.io/g/devel/message/50931 [PATCH] MdeModulePkg: LzmaCustomDecompressLib.inf don't support EBC anymore https://edk2.groups.io/g/devel/message/50990 [PATCH V1 1/1] MdeModulePkg/Variable: Initialize local variable https://edk2.groups.io/g/devel/message/50989 [PATCH V2] BaseTools:fix regression issue for platform .map file https://edk2.groups.io/g/devel/message/50936 [PATCH] BaseTools:fixed Build failed issue for Non-English OS Thanks Liming >-----Original Message----- >From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of >Liming Gao >Sent: Wednesday, November 20, 2019 10:52 PM >To: devel@edk2.groups.io; leif.lindholm@linaro.org; Laszlo Ersek ><lersek@redhat.com> >Cc: Kinney, Michael D <michael.d.kinney@intel.com>; 'afish@apple.com' ><afish@apple.com> >Subject: Re: [edk2-devel] Patch List for 201911 stable tag > >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 >> >> > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] Patch List for 201911 stable tag 2019-11-21 8:57 ` Liming Gao @ 2019-11-21 10:37 ` Leif Lindholm 2019-11-21 17:26 ` Laszlo Ersek 0 siblings, 1 reply; 9+ messages in thread From: Leif Lindholm @ 2019-11-21 10:37 UTC (permalink / raw) To: Gao, Liming Cc: devel@edk2.groups.io, Laszlo Ersek, Kinney, Michael D, 'afish@apple.com' On Thu, Nov 21, 2019 at 08:57:12AM +0000, Gao, Liming wrote: > Hi Stewards and all: > New bugs are for 201911 stable tag. Can you give the comments for them? > > Bug List (those all have pass code review): > https://edk2.groups.io/g/devel/message/50931 [PATCH] MdeModulePkg: LzmaCustomDecompressLib.inf don't support EBC anymore This can wait until after the stable tag *unless* that causes serious issues for CI. (And I don't consider non-zero build failures for EBC to be serious.) The fix only addresses building the component standalone as part of MdeModulePkg, so it affects no real platforms. I have some comments with regards to the implementation as well, but I'll give those on the actual patch email. > https://edk2.groups.io/g/devel/message/50990 [PATCH V1 1/1] MdeModulePkg/Variable: Initialize local variable Those two bugs are poster children for why long functions should be avoided. OK for me to include in stable tag. > https://edk2.groups.io/g/devel/message/50989 [PATCH V2] BaseTools:fix regression issue for platform .map file OK for me to include *if* the commit message (and bugzilla) are updated to clarify the issue. "is missing" is passive language, and hence fails to convey what is causing the problem. Does this mean that the 'build' command fails to generate these lines? Also, a comment on why this change (of moving this hunk 18 lines up) resolves the issue should be included. > https://edk2.groups.io/g/devel/message/50936 [PATCH] BaseTools:fixed Build failed issue for Non-English OS I agree that a fix for this bug must be included in the stable tag. But both the BZ and the commit message need improving. And seeing a fix that consists solely of adding "errors='ignore'" worries me somewhat. Please explain *what* goes wrong when using Non-English OS (and which OS), as well as why "errors='ignore'" does not cause further problems. Best Regards, Leif > Thanks > Liming > >-----Original Message----- > >From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > >Liming Gao > >Sent: Wednesday, November 20, 2019 10:52 PM > >To: devel@edk2.groups.io; leif.lindholm@linaro.org; Laszlo Ersek > ><lersek@redhat.com> > >Cc: Kinney, Michael D <michael.d.kinney@intel.com>; 'afish@apple.com' > ><afish@apple.com> > >Subject: Re: [edk2-devel] Patch List for 201911 stable tag > > > >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 > >> > >> > > > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] Patch List for 201911 stable tag 2019-11-21 10:37 ` Leif Lindholm @ 2019-11-21 17:26 ` Laszlo Ersek 2019-11-25 7:36 ` Bob Feng 0 siblings, 1 reply; 9+ messages in thread From: Laszlo Ersek @ 2019-11-21 17:26 UTC (permalink / raw) To: Leif Lindholm, Gao, Liming Cc: devel@edk2.groups.io, Kinney, Michael D, 'afish@apple.com' On 11/21/19 11:37, Leif Lindholm wrote: > On Thu, Nov 21, 2019 at 08:57:12AM +0000, Gao, Liming wrote: >> Hi Stewards and all: >> New bugs are for 201911 stable tag. Can you give the comments for them? >> >> Bug List (those all have pass code review): >> https://edk2.groups.io/g/devel/message/50931 [PATCH] MdeModulePkg: LzmaCustomDecompressLib.inf don't support EBC anymore > > This can wait until after the stable tag *unless* that causes serious > issues for CI. (And I don't consider non-zero build failures for EBC > to be serious.) > The fix only addresses building the component standalone as part of > MdeModulePkg, so it affects no real platforms. I would have beem more permissive with this patch, but I'm OK with the above resolution too. > > I have some comments with regards to the implementation as well, but > I'll give those on the actual patch email. > >> https://edk2.groups.io/g/devel/message/50990 [PATCH V1 1/1] MdeModulePkg/Variable: Initialize local variable > > Those two bugs are poster children for why long functions should be > avoided. > > OK for me to include in stable tag. Agreed. > >> https://edk2.groups.io/g/devel/message/50989 [PATCH V2] BaseTools:fix regression issue for platform .map file > > OK for me to include *if* the commit message (and bugzilla) are > updated to clarify the issue. "is missing" is passive language, and > hence fails to convey what is causing the problem. Does this mean > that the 'build' command fails to generate these lines? > > Also, a comment on why this change (of moving this hunk 18 lines up) > resolves the issue should be included. Agreed; please add a reminder to the commit message (even on push) about what the specific (missing) lines in the map files are good for in the first place. > >> https://edk2.groups.io/g/devel/message/50936 [PATCH] BaseTools:fixed Build failed issue for Non-English OS > > I agree that a fix for this bug must be included in the stable tag. > But both the BZ and the commit message need improving. > And seeing a fix that consists solely of adding "errors='ignore'" > worries me somewhat. > > Please explain *what* goes wrong when using Non-English OS (and which > OS), as well as why "errors='ignore'" does not cause further problems. I think this patch is plain wrong. I'll comment on it in its own thread. Thanks Laszlo ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] Patch List for 201911 stable tag 2019-11-21 17:26 ` Laszlo Ersek @ 2019-11-25 7:36 ` Bob Feng 0 siblings, 0 replies; 9+ messages in thread From: Bob Feng @ 2019-11-25 7:36 UTC (permalink / raw) To: devel@edk2.groups.io, lersek@redhat.com, Leif Lindholm, Gao, Liming Cc: Kinney, Michael D, 'afish@apple.com' Patch "fix regression issue for platform .map file" was pushed. More comments inline. Thanks, Bob -----Original Message----- From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek Sent: Friday, November 22, 2019 1:26 AM To: Leif Lindholm <leif.lindholm@linaro.org>; Gao, Liming <liming.gao@intel.com> Cc: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>; 'afish@apple.com' <afish@apple.com> Subject: Re: [edk2-devel] Patch List for 201911 stable tag On 11/21/19 11:37, Leif Lindholm wrote: > On Thu, Nov 21, 2019 at 08:57:12AM +0000, Gao, Liming wrote: >> Hi Stewards and all: >> New bugs are for 201911 stable tag. Can you give the comments for them? >> >> Bug List (those all have pass code review): >> https://edk2.groups.io/g/devel/message/50931 [PATCH] MdeModulePkg: >> LzmaCustomDecompressLib.inf don't support EBC anymore > > This can wait until after the stable tag *unless* that causes serious > issues for CI. (And I don't consider non-zero build failures for EBC > to be serious.) The fix only addresses building the component > standalone as part of MdeModulePkg, so it affects no real platforms. I would have beem more permissive with this patch, but I'm OK with the above resolution too. > > I have some comments with regards to the implementation as well, but > I'll give those on the actual patch email. > >> https://edk2.groups.io/g/devel/message/50990 [PATCH V1 1/1] >> MdeModulePkg/Variable: Initialize local variable > > Those two bugs are poster children for why long functions should be > avoided. > > OK for me to include in stable tag. Agreed. > >> https://edk2.groups.io/g/devel/message/50989 [PATCH V2] BaseTools:fix >> regression issue for platform .map file > > OK for me to include *if* the commit message (and bugzilla) are > updated to clarify the issue. "is missing" is passive language, and > hence fails to convey what is causing the problem. Does this mean that > the 'build' command fails to generate these lines? > > Also, a comment on why this change (of moving this hunk 18 lines up) > resolves the issue should be included. Agreed; please add a reminder to the commit message (even on push) about what the specific (missing) lines in the map files are good for in the first place. [Bob] The commit message was updated https://edk2.groups.io/g/devel/message/51212 And the patch was pushed. > >> https://edk2.groups.io/g/devel/message/50936 [PATCH] BaseTools:fixed >> Build failed issue for Non-English OS > > I agree that a fix for this bug must be included in the stable tag. > But both the BZ and the commit message need improving. > And seeing a fix that consists solely of adding "errors='ignore'" > worries me somewhat. > > Please explain *what* goes wrong when using Non-English OS (and which > OS), as well as why "errors='ignore'" does not cause further problems. I think this patch is plain wrong. I'll comment on it in its own thread. Thanks Laszlo ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-11-25 7:36 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-11-19 14:25 Patch List for 201911 stable tag Liming Gao 2019-11-19 17:50 ` Laszlo Ersek 2019-11-19 19:01 ` Leif Lindholm 2019-11-20 1:46 ` [edk2-devel] " Wu, Hao A 2019-11-20 14:52 ` Liming Gao [not found] ` <15D8E68889A9A669.17632@groups.io> 2019-11-21 8:57 ` Liming Gao 2019-11-21 10:37 ` Leif Lindholm 2019-11-21 17:26 ` Laszlo Ersek 2019-11-25 7:36 ` Bob Feng
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox