|
(Slightly trimmed recipient list due to different patch being discussed.) So, I can't make this call, because I'm the one who messed up. This patch does exactly what I had requested Abner to do some time back (off-list, unfortunately), and I was *convinced* I gave it an R-b as soon as it hit my inbox - until Abner nudged me about it yesterday. The patch in question is https://edk2.groups.io/g/devel/topic/76021725/ Leif
On Wed, Aug 19, 2020 at 11:29:52 +0000, Chang, Abner (HPS SW/FW Technologist) wrote: Hmm.. I had a one commit (which is not the feature) just reviewed by
Leif but not pushing yet. Is that possible to push before the Hard
Feature freeze and also be included in 202008 stable tag? I guess it
gets the chance according to the article of SoftFeatureFreeze on
Wiki page.
Patch attached FYR
Abner
-----Original Message-----
From: announce@edk2.groups.io [mailto:announce@edk2.groups.io] On
Behalf Of Laszlo Ersek
Sent: Wednesday, August 19, 2020 5:59 PM
To: Bret Barkelew <Bret.Barkelew@...>; devel@edk2.groups.io;
liming.gao <liming.gao@...>; announce@edk2.groups.io
Cc: Leif Lindholm <leif@...>; afish@...; Kinney, Michael D
<michael.d.kinney@...>; Guptha, Soumya K
<soumya.k.guptha@...>
Subject: Re: [edk2-announce] [EXTERNAL] Re: Soft Feature Freeze starts
now for edk2-stable202008
On 08/18/20 17:10, Bret Barkelew wrote:
I agree with the process and withdraw my request, replacing it instead with a disapproving head shake and deep sigh.
We as a community definitely deserve your disapproval, as our review
response times have been abysmal. :(
I’ll go back to pushing on this after the tag. Thanks for your persistence!
Laszlo
|
|
|
On 08/19/20 13:48, Leif Lindholm wrote: (Slightly trimmed recipient list due to different patch being discussed.)
So, I can't make this call, because I'm the one who messed up.
This patch does exactly what I had requested Abner to do some time
back (off-list, unfortunately), and I was *convinced* I gave it an R-b
as soon as it hit my inbox - until Abner nudged me about it yesterday.
The patch in question is
https://edk2.groups.io/g/devel/topic/76021725 My understanding is: (1) there is an external project that consumes the FDT library in EmbeddedPkg, meaning the lib class header "EmbeddedPkg/Include/libfdt.h" and the lib instance "EmbeddedPkg/Library/FdtLib/FdtLib.inf", (2) the lib class header pulls in "fdt.h" and "libfdt_env.h", (3) the external project is not edk2-platforms, (4) the external project wants -- for some strange reason -- edk2's "libfdt_env.h" to provide an strncmp() function (or function-like macro), with that particular stncmp() implementation not being needed in either edk2-platforms or edk2 itself, (5) the patch for adding said strncmp() was posted on Aug 6th (at least when viewed from my time zone), i.e., before the SFF, (6) it was reviewed 12 days later (within the SFF) If my understanding is correct, then I don't see how this patch could be considered a bugfix -- even as a feature addition, it seems hardly justified to me --, and there would have been ~8 days before the SFF to review it. I think we should postpone the patch until after the stable tag. Thanks Laszlo On Wed, Aug 19, 2020 at 11:29:52 +0000, Chang, Abner (HPS SW/FW Technologist) wrote:
Hmm.. I had a one commit (which is not the feature) just reviewed by
Leif but not pushing yet. Is that possible to push before the Hard
Feature freeze and also be included in 202008 stable tag? I guess it
gets the chance according to the article of SoftFeatureFreeze on
Wiki page.
Patch attached FYR
Abner
-----Original Message-----
From: announce@edk2.groups.io [mailto:announce@edk2.groups.io] On
Behalf Of Laszlo Ersek
Sent: Wednesday, August 19, 2020 5:59 PM
To: Bret Barkelew <Bret.Barkelew@...>; devel@edk2.groups.io;
liming.gao <liming.gao@...>; announce@edk2.groups.io
Cc: Leif Lindholm <leif@...>; afish@...; Kinney, Michael D
<michael.d.kinney@...>; Guptha, Soumya K
<soumya.k.guptha@...>
Subject: Re: [edk2-announce] [EXTERNAL] Re: Soft Feature Freeze starts
now for edk2-stable202008
On 08/18/20 17:10, Bret Barkelew wrote:
I agree with the process and withdraw my request, replacing it instead with a disapproving head shake and deep sigh.
We as a community definitely deserve your disapproval, as our review
response times have been abysmal. :(
I’ll go back to pushing on this after the tag. Thanks for your persistence!
Laszlo
|
|
|
On 08/19/20 15:19, Laszlo Ersek wrote: On 08/19/20 13:48, Leif Lindholm wrote:
(Slightly trimmed recipient list due to different patch being discussed.)
So, I can't make this call, because I'm the one who messed up.
This patch does exactly what I had requested Abner to do some time
back (off-list, unfortunately), and I was *convinced* I gave it an R-b
as soon as it hit my inbox - until Abner nudged me about it yesterday.
The patch in question is
https://edk2.groups.io/g/devel/topic/76021725 My understanding is:
(1) there is an external project that consumes the FDT library in
EmbeddedPkg, meaning the lib class header "EmbeddedPkg/Include/libfdt.h"
and the lib instance "EmbeddedPkg/Library/FdtLib/FdtLib.inf",
(2) the lib class header pulls in "fdt.h" and "libfdt_env.h",
(3) the external project is not edk2-platforms,
(4) the external project wants -- for some strange reason -- edk2's
"libfdt_env.h" to provide an strncmp() function (or function-like
macro), with that particular stncmp() implementation not being needed in
either edk2-platforms or edk2 itself,
(5) the patch for adding said strncmp() was posted on Aug 6th (at least
when viewed from my time zone), i.e., before the SFF,
(6) it was reviewed 12 days later (within the SFF)
If my understanding is correct, then I don't see how this patch could be
considered a bugfix -- even as a feature addition, it seems hardly
justified to me --, and there would have been ~8 days before the SFF to
review it.
I think we should postpone the patch until after the stable tag. Regarding OpenSBI, as of commit 9d56961b2314, the sbi_strncmp() function has not been removed, and it seems that sbi_strncmp() is still used / called in at least some build configurations (where the lib/utils/libfdt/libfdt_env.h:#define strncmp sbi_strncmp definition is supposed to be in effect). This means that the codebase can not rid itself of sbi_strncmp(). To me this indicates that OpenSBI commit 2cfd2fc90488 ("lib: utils: Use strncmp in fdt_parse_hart_id()", 2020-07-29) was wrong. It should have replaced sbi_strcmp() with sbi_strncmp(), not strncmp(). After all, the size limit seems to be the motivation for the entire change -- put a size limit on the string comparison in fdt_parse_hart_id(). Commit 2cfd2fc90488 did two things at the same time: replace the size-unbounded comparison with the size-bounded one, *and* switch to the standard C function name from the self-contained function. The former goal looks justifiable, the latter I don't understand. Now: I realize that, going back to edk2 commit fa4a70633868 ("EmbeddedPkg: Added support to libfdt", 2012-09-27), we already have a bunch of wrappers in "EmbeddedPkg/Include/libfdt_env.h". So I guess adding "one more" is not inconsistent with those -- even though the lib instance within edk2 doesn't need the new function. But it's still a micro-feature whose review should have completed before the SFF. Thanks Laszlo
|
|
|
On 08/19/20 15:34, Chang, Abner (HPS SW/FW Technologist) wrote:
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@...]
Sent: Wednesday, August 19, 2020 9:19 PM
To: Leif Lindholm <leif@...>; Chang, Abner (HPS SW/FW
Technologist) <abner.chang@...>
Cc: devel@edk2.groups.io; liming.gao <liming.gao@...>;
announce@edk2.groups.io; afish@...; Kinney, Michael D
<michael.d.kinney@...>
Subject: Re: [edk2-announce] Re: Soft Feature Freeze starts now for edk2-
stable202008
On 08/19/20 13:48, Leif Lindholm wrote:
(Slightly trimmed recipient list due to different patch being
discussed.)
So, I can't make this call, because I'm the one who messed up.
This patch does exactly what I had requested Abner to do some time
back (off-list, unfortunately), and I was *convinced* I gave it an R-b
as soon as it hit my inbox - until Abner nudged me about it yesterday.
The patch in question is
https://edk2.groups.io/g/devel/topic/76021725 My understanding is:
(1) there is an external project that consumes the FDT library in
EmbeddedPkg, meaning the lib class header
"EmbeddedPkg/Include/libfdt.h"
and the lib instance "EmbeddedPkg/Library/FdtLib/FdtLib.inf", [Chang, Abner] Yes
(2) the lib class header pulls in "fdt.h" and "libfdt_env.h",
[Chang, Abner] yes
(3) the external project is not edk2-platforms,
[Chang, Abner] yes
(4) the external project wants -- for some strange reason -- edk2's
"libfdt_env.h" to provide an strncmp() function (or function-like macro), with
that particular stncmp() implementation not being needed in either edk2-
platforms or edk2 itself,
[Chang, Abner] yes, at least so far
(5) the patch for adding said strncmp() was posted on Aug 6th (at least when
viewed from my time zone), i.e., before the SFF,
[Chang, Abner] Yes
(6) it was reviewed 12 days later (within the SFF)
[Chang, Abner] yes.
If my understanding is correct, then I don't see how this patch could be
considered a bugfix -- even as a feature addition, it seems hardly justified to
me --, and there would have been ~8 days before the SFF to review it.
I think we should postpone the patch until after the stable tag.
This patch is important because the edk2-stable202008 would be the stable tag (if this patch is accepted) for booting RISC-V platform to Linux kernel with EFI Runtime service on either real platform and QEMU. We can publish this information in RISC-V community which is considered as a valuable milestone for RISC-V edk2 port.
Let's move out the dates for the stable tag then, by one week: - let the SFF start on 2020-08-21 - let the HFF start on 2020-08-28 - let's release edk2-stable202008 on 2020-09-04 Release slips are permitted and there have been examples. What doesn't make sense is making rules and then breaking them opportunistically, whenever they're uncomfortable. If that's a frequent occurrence, we should pick different rules, or -- again -- if this is a very important patch, we should delay the release for it. BTW what about reverting the OpenSBI change? You could still call sbi_strncmp() -- rather than strncmp() -- in the "helper" lib. Thanks Laszlo
|
|
|
toggle quoted message
Show quoted text
-----Original Message-----
From: Chang, Abner (HPS SW/FW Technologist) <abner.chang@...>
Sent: Wednesday, August 19, 2020 10:59 PM
To: Laszlo Ersek <lersek@...>; Leif Lindholm <leif@...>
Cc: devel@edk2.groups.io; Gao, Liming <liming.gao@...>; announce@edk2.groups.io; afish@...; Kinney, Michael D
<michael.d.kinney@...>
Subject: RE: [edk2-announce] Re: Soft Feature Freeze starts now for edk2-stable202008
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@...]
Sent: Wednesday, August 19, 2020 10:30 PM
To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@...>; Leif
Lindholm <leif@...>
Cc: devel@edk2.groups.io; liming.gao <liming.gao@...>;
announce@edk2.groups.io; afish@...; Kinney, Michael D
<michael.d.kinney@...>
Subject: Re: [edk2-announce] Re: Soft Feature Freeze starts now for edk2-
stable202008
On 08/19/20 15:34, Chang, Abner (HPS SW/FW Technologist) wrote:
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@...]
Sent: Wednesday, August 19, 2020 9:19 PM
To: Leif Lindholm <leif@...>; Chang, Abner (HPS SW/FW
Technologist) <abner.chang@...>
Cc: devel@edk2.groups.io; liming.gao <liming.gao@...>;
announce@edk2.groups.io; afish@...; Kinney, Michael D
<michael.d.kinney@...>
Subject: Re: [edk2-announce] Re: Soft Feature Freeze starts now for
edk2-
stable202008
On 08/19/20 13:48, Leif Lindholm wrote:
(Slightly trimmed recipient list due to different patch being
discussed.)
So, I can't make this call, because I'm the one who messed up.
This patch does exactly what I had requested Abner to do some time
back (off-list, unfortunately), and I was *convinced* I gave it an
R-b as soon as it hit my inbox - until Abner nudged me about it yesterday.
The patch in question is
https://edk2.groups.io/g/devel/topic/76021725 My understanding is:
(1) there is an external project that consumes the FDT library in
EmbeddedPkg, meaning the lib class header
"EmbeddedPkg/Include/libfdt.h"
and the lib instance "EmbeddedPkg/Library/FdtLib/FdtLib.inf", [Chang, Abner] Yes
(2) the lib class header pulls in "fdt.h" and "libfdt_env.h",
[Chang, Abner] yes
(3) the external project is not edk2-platforms,
[Chang, Abner] yes
(4) the external project wants -- for some strange reason -- edk2's
"libfdt_env.h" to provide an strncmp() function (or function-like
macro), with that particular stncmp() implementation not being needed
in either edk2- platforms or edk2 itself,
[Chang, Abner] yes, at least so far
(5) the patch for adding said strncmp() was posted on Aug 6th (at
least when viewed from my time zone), i.e., before the SFF,
[Chang, Abner] Yes
(6) it was reviewed 12 days later (within the SFF)
[Chang, Abner] yes.
If my understanding is correct, then I don't see how this patch could
be considered a bugfix -- even as a feature addition, it seems hardly
justified to me --, and there would have been ~8 days before the SFF to
review it.
I think we should postpone the patch until after the stable tag.
This patch is important because the edk2-stable202008 would be the stable
tag (if this patch is accepted) for booting RISC-V platform to Linux kernel with
EFI Runtime service on either real platform and QEMU. We can publish this
information in RISC-V community which is considered as a valuable milestone
for RISC-V edk2 port.
Let's move out the dates for the stable tag then, by one week:
- let the SFF start on 2020-08-21
- let the HFF start on 2020-08-28
- let's release edk2-stable202008 on 2020-09-04
Release slips are permitted and there have been examples. Appreciate for this. Previous stable tags (202005/202002/201905) extended SFF or HFF period to let the critical bug fix catch the stable tag. Today case is like a new feature. According to Abner comment, it is important to RISC-V community. If SFF start time is changed, more features may catch this stable, such as VariableLock feature.
What doesn't make sense is making rules and then breaking them
opportunistically, whenever they're uncomfortable. If that's a frequent
occurrence, we should pick different rules, or -- again -- if this is a very
important patch, we should delay the release for it.
I suggest to define the process on delay the release. The initial process is specified here. The requestor provides the justification why his patch needs to catch the stable tag after SFF phase. All stewards make the decision whether delay the stable tag to include his patch. If stewards agree to delay the stable tag, and there is no objection from the community, the release maintainer will update edk2 release planning with new date. If stewards rejects to this request, there is no change in stable tag release. Thanks Liming
BTW what about reverting the OpenSBI change? You could still call
sbi_strncmp() -- rather than strncmp() -- in the "helper" lib.
I replied this in another thread.
Thanks
Laszlo
|
|
|
On 08/19/20 18:20, Gao, Liming wrote:
-----Original Message-----
From: Chang, Abner (HPS SW/FW Technologist) <abner.chang@...>
Sent: Wednesday, August 19, 2020 10:59 PM
To: Laszlo Ersek <lersek@...>; Leif Lindholm <leif@...>
Cc: devel@edk2.groups.io; Gao, Liming <liming.gao@...>; announce@edk2.groups.io; afish@...; Kinney, Michael D
<michael.d.kinney@...>
Subject: RE: [edk2-announce] Re: Soft Feature Freeze starts now for edk2-stable202008
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@...]
Sent: Wednesday, August 19, 2020 10:30 PM
To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@...>; Leif
Lindholm <leif@...>
Cc: devel@edk2.groups.io; liming.gao <liming.gao@...>;
announce@edk2.groups.io; afish@...; Kinney, Michael D
<michael.d.kinney@...>
Subject: Re: [edk2-announce] Re: Soft Feature Freeze starts now for edk2-
stable202008
On 08/19/20 15:34, Chang, Abner (HPS SW/FW Technologist) wrote:
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@...]
Sent: Wednesday, August 19, 2020 9:19 PM
To: Leif Lindholm <leif@...>; Chang, Abner (HPS SW/FW
Technologist) <abner.chang@...>
Cc: devel@edk2.groups.io; liming.gao <liming.gao@...>;
announce@edk2.groups.io; afish@...; Kinney, Michael D
<michael.d.kinney@...>
Subject: Re: [edk2-announce] Re: Soft Feature Freeze starts now for
edk2-
stable202008
On 08/19/20 13:48, Leif Lindholm wrote:
(Slightly trimmed recipient list due to different patch being
discussed.)
So, I can't make this call, because I'm the one who messed up.
This patch does exactly what I had requested Abner to do some time
back (off-list, unfortunately), and I was *convinced* I gave it an
R-b as soon as it hit my inbox - until Abner nudged me about it yesterday.
The patch in question is
https://edk2.groups.io/g/devel/topic/76021725 My understanding is:
(1) there is an external project that consumes the FDT library in
EmbeddedPkg, meaning the lib class header
"EmbeddedPkg/Include/libfdt.h"
and the lib instance "EmbeddedPkg/Library/FdtLib/FdtLib.inf", [Chang, Abner] Yes
(2) the lib class header pulls in "fdt.h" and "libfdt_env.h",
[Chang, Abner] yes
(3) the external project is not edk2-platforms,
[Chang, Abner] yes
(4) the external project wants -- for some strange reason -- edk2's
"libfdt_env.h" to provide an strncmp() function (or function-like
macro), with that particular stncmp() implementation not being needed
in either edk2- platforms or edk2 itself,
[Chang, Abner] yes, at least so far
(5) the patch for adding said strncmp() was posted on Aug 6th (at
least when viewed from my time zone), i.e., before the SFF,
[Chang, Abner] Yes
(6) it was reviewed 12 days later (within the SFF)
[Chang, Abner] yes.
If my understanding is correct, then I don't see how this patch could
be considered a bugfix -- even as a feature addition, it seems hardly
justified to me --, and there would have been ~8 days before the SFF to
review it.
I think we should postpone the patch until after the stable tag.
This patch is important because the edk2-stable202008 would be the stable
tag (if this patch is accepted) for booting RISC-V platform to Linux kernel with
EFI Runtime service on either real platform and QEMU. We can publish this
information in RISC-V community which is considered as a valuable milestone
for RISC-V edk2 port.
Let's move out the dates for the stable tag then, by one week:
- let the SFF start on 2020-08-21
- let the HFF start on 2020-08-28
- let's release edk2-stable202008 on 2020-09-04
Release slips are permitted and there have been examples. Appreciate for this. Previous stable tags (202005/202002/201905) extended SFF or HFF period to let the critical bug fix catch the stable tag.
Today case is like a new feature. According to Abner comment, it is important to RISC-V community.
If SFF start time is changed, more features may catch this stable, such as VariableLock feature.
Yes, exactly. We must apply the same deadline to all features. If one feature is important enough for delaying the SFF, then patch review for other pending features is resumed as well.
What doesn't make sense is making rules and then breaking them
opportunistically, whenever they're uncomfortable. If that's a frequent
occurrence, we should pick different rules, or -- again -- if this is a very
important patch, we should delay the release for it.
I suggest to define the process on delay the release. The initial process is specified here.
The requestor provides the justification why his patch needs to catch the stable tag after SFF phase.
All stewards make the decision whether delay the stable tag to include his patch.
If stewards agree to delay the stable tag, and there is no objection from the community,
the release maintainer will update edk2 release planning with new date.
If stewards rejects to this request, there is no change in stable tag release.
I'm OK with delaying the release by one or two weeks, for any feature(s) currently pending review on the list. (If we go for two weeks, we should likely change the name of the upcoming stable tag.) (Some projects don't even propose cut-off dates, they say "we'll make a release when we have enough stuff to release, and when all of those things are complete". That's another approach to consider.) Thanks Laszlo BTW what about reverting the OpenSBI change? You could still call
sbi_strncmp() -- rather than strncmp() -- in the "helper" lib. I replied this in another thread.
Thanks
Laszlo
|
|
|
Abner Chang
toggle quoted message
Show quoted text
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@...]
Sent: Wednesday, August 19, 2020 10:30 PM
To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@...>; Leif
Lindholm <leif@...>
Cc: devel@edk2.groups.io; liming.gao <liming.gao@...>;
announce@edk2.groups.io; afish@...; Kinney, Michael D
<michael.d.kinney@...>
Subject: Re: [edk2-announce] Re: Soft Feature Freeze starts now for edk2-
stable202008
On 08/19/20 15:34, Chang, Abner (HPS SW/FW Technologist) wrote:
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@...]
Sent: Wednesday, August 19, 2020 9:19 PM
To: Leif Lindholm <leif@...>; Chang, Abner (HPS SW/FW
Technologist) <abner.chang@...>
Cc: devel@edk2.groups.io; liming.gao <liming.gao@...>;
announce@edk2.groups.io; afish@...; Kinney, Michael D
<michael.d.kinney@...>
Subject: Re: [edk2-announce] Re: Soft Feature Freeze starts now for
edk2-
stable202008
On 08/19/20 13:48, Leif Lindholm wrote:
(Slightly trimmed recipient list due to different patch being
discussed.)
So, I can't make this call, because I'm the one who messed up.
This patch does exactly what I had requested Abner to do some time
back (off-list, unfortunately), and I was *convinced* I gave it an
R-b as soon as it hit my inbox - until Abner nudged me about it yesterday.
The patch in question is
https://edk2.groups.io/g/devel/topic/76021725 My understanding is:
(1) there is an external project that consumes the FDT library in
EmbeddedPkg, meaning the lib class header
"EmbeddedPkg/Include/libfdt.h"
and the lib instance "EmbeddedPkg/Library/FdtLib/FdtLib.inf", [Chang, Abner] Yes
(2) the lib class header pulls in "fdt.h" and "libfdt_env.h",
[Chang, Abner] yes
(3) the external project is not edk2-platforms,
[Chang, Abner] yes
(4) the external project wants -- for some strange reason -- edk2's
"libfdt_env.h" to provide an strncmp() function (or function-like
macro), with that particular stncmp() implementation not being needed
in either edk2- platforms or edk2 itself,
[Chang, Abner] yes, at least so far
(5) the patch for adding said strncmp() was posted on Aug 6th (at
least when viewed from my time zone), i.e., before the SFF,
[Chang, Abner] Yes
(6) it was reviewed 12 days later (within the SFF)
[Chang, Abner] yes.
If my understanding is correct, then I don't see how this patch could
be considered a bugfix -- even as a feature addition, it seems hardly
justified to me --, and there would have been ~8 days before the SFF to
review it.
I think we should postpone the patch until after the stable tag.
This patch is important because the edk2-stable202008 would be the stable
tag (if this patch is accepted) for booting RISC-V platform to Linux kernel with
EFI Runtime service on either real platform and QEMU. We can publish this
information in RISC-V community which is considered as a valuable milestone
for RISC-V edk2 port.
Let's move out the dates for the stable tag then, by one week:
- let the SFF start on 2020-08-21
- let the HFF start on 2020-08-28
- let's release edk2-stable202008 on 2020-09-04
Release slips are permitted and there have been examples. Appreciate for this.
What doesn't make sense is making rules and then breaking them
opportunistically, whenever they're uncomfortable. If that's a frequent
occurrence, we should pick different rules, or -- again -- if this is a very
important patch, we should delay the release for it.
BTW what about reverting the OpenSBI change? You could still call
sbi_strncmp() -- rather than strncmp() -- in the "helper" lib.
I replied this in another thread.
Thanks
Laszlo
|
|
|
Abner Chang
...and I didn’t aware that announcement of Soft Feature freeze because [edk2-annnounce] never went to my inbox and end up with I missed it, I should set the rule for it. :(
toggle quoted message
Show quoted text
-----Original Message-----
From: Leif Lindholm [mailto:leif@...]
Sent: Wednesday, August 19, 2020 7:49 PM
To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@...>
Cc: Laszlo Ersek <lersek@...>; devel@edk2.groups.io; liming.gao
<liming.gao@...>; announce@edk2.groups.io; afish@...;
Kinney, Michael D <michael.d.kinney@...>
Subject: Re: [edk2-announce] Re: Soft Feature Freeze starts now for edk2-
stable202008
(Slightly trimmed recipient list due to different patch being discussed.)
So, I can't make this call, because I'm the one who messed up.
This patch does exactly what I had requested Abner to do some time back
(off-list, unfortunately), and I was *convinced* I gave it an R-b as soon as it
hit my inbox - until Abner nudged me about it yesterday.
The patch in question is
INVALID URI REMOVED
3A__edk2.groups.io_g_devel_topic_76021725&d=DwIDaQ&c=C5b8zRQO1mi
GmBeVZ2LFWg&r=_SN6FZBN4Vgi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3E&m=
aIPVzq25kdyKl8jdl_OJcSuym6rNT7xSGICexWHc8UY&s=vVeUUfgQDUnjWzqh
YpCiNqNHDScByiwJzFiUZ4kx4TU&e=
/
Leif
On Wed, Aug 19, 2020 at 11:29:52 +0000, Chang, Abner (HPS SW/FW
Technologist) wrote:
Hmm.. I had a one commit (which is not the feature) just reviewed by
Leif but not pushing yet. Is that possible to push before the Hard
Feature freeze and also be included in 202008 stable tag? I guess it
gets the chance according to the article of SoftFeatureFreeze on Wiki
page.
Patch attached FYR
Abner
-----Original Message-----
From: announce@edk2.groups.io [mailto:announce@edk2.groups.io] On
Behalf Of Laszlo Ersek
Sent: Wednesday, August 19, 2020 5:59 PM
To: Bret Barkelew <Bret.Barkelew@...>;
devel@edk2.groups.io; liming.gao <liming.gao@...>;
announce@edk2.groups.io
Cc: Leif Lindholm <leif@...>; afish@...; Kinney,
Michael D <michael.d.kinney@...>; Guptha, Soumya K
<soumya.k.guptha@...>
Subject: Re: [edk2-announce] [EXTERNAL] Re: Soft Feature Freeze
starts now for edk2-stable202008
On 08/18/20 17:10, Bret Barkelew wrote:
I agree with the process and withdraw my request, replacing it
instead with a disapproving head shake and deep sigh.
We as a community definitely deserve your disapproval, as our review
response times have been abysmal. :(
I’ll go back to pushing on this after the tag. Thanks for your persistence!
Laszlo
|
|
|
Abner Chang
toggle quoted message
Show quoted text
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@...]
Sent: Wednesday, August 19, 2020 9:19 PM
To: Leif Lindholm <leif@...>; Chang, Abner (HPS SW/FW
Technologist) <abner.chang@...>
Cc: devel@edk2.groups.io; liming.gao <liming.gao@...>;
announce@edk2.groups.io; afish@...; Kinney, Michael D
<michael.d.kinney@...>
Subject: Re: [edk2-announce] Re: Soft Feature Freeze starts now for edk2-
stable202008
On 08/19/20 13:48, Leif Lindholm wrote:
(Slightly trimmed recipient list due to different patch being
discussed.)
So, I can't make this call, because I'm the one who messed up.
This patch does exactly what I had requested Abner to do some time
back (off-list, unfortunately), and I was *convinced* I gave it an R-b
as soon as it hit my inbox - until Abner nudged me about it yesterday.
The patch in question is
https://edk2.groups.io/g/devel/topic/76021725 My understanding is:
(1) there is an external project that consumes the FDT library in
EmbeddedPkg, meaning the lib class header
"EmbeddedPkg/Include/libfdt.h"
and the lib instance "EmbeddedPkg/Library/FdtLib/FdtLib.inf", [Chang, Abner] Yes
(2) the lib class header pulls in "fdt.h" and "libfdt_env.h",
[Chang, Abner] yes
(3) the external project is not edk2-platforms,
[Chang, Abner] yes
(4) the external project wants -- for some strange reason -- edk2's
"libfdt_env.h" to provide an strncmp() function (or function-like macro), with
that particular stncmp() implementation not being needed in either edk2-
platforms or edk2 itself,
[Chang, Abner] yes, at least so far
(5) the patch for adding said strncmp() was posted on Aug 6th (at least when
viewed from my time zone), i.e., before the SFF,
[Chang, Abner] Yes
(6) it was reviewed 12 days later (within the SFF)
[Chang, Abner] yes.
If my understanding is correct, then I don't see how this patch could be
considered a bugfix -- even as a feature addition, it seems hardly justified to
me --, and there would have been ~8 days before the SFF to review it.
I think we should postpone the patch until after the stable tag.
This patch is important because the edk2-stable202008 would be the stable tag (if this patch is accepted) for booting RISC-V platform to Linux kernel with EFI Runtime service on either real platform and QEMU. We can publish this information in RISC-V community which is considered as a valuable milestone for RISC-V edk2 port.
Thanks
Laszlo
On Wed, Aug 19, 2020 at 11:29:52 +0000, Chang, Abner (HPS SW/FW Technologist) wrote:
Hmm.. I had a one commit (which is not the feature) just reviewed by
Leif but not pushing yet. Is that possible to push before the Hard
Feature freeze and also be included in 202008 stable tag? I guess it
gets the chance according to the article of SoftFeatureFreeze on Wiki
page.
Patch attached FYR
Abner
-----Original Message-----
From: announce@edk2.groups.io [mailto:announce@edk2.groups.io]
On
Behalf Of Laszlo Ersek
Sent: Wednesday, August 19, 2020 5:59 PM
To: Bret Barkelew <Bret.Barkelew@...>;
devel@edk2.groups.io; liming.gao <liming.gao@...>;
announce@edk2.groups.io
Cc: Leif Lindholm <leif@...>; afish@...; Kinney,
Michael D <michael.d.kinney@...>; Guptha, Soumya K
<soumya.k.guptha@...>
Subject: Re: [edk2-announce] [EXTERNAL] Re: Soft Feature Freeze
starts now for edk2-stable202008
On 08/18/20 17:10, Bret Barkelew wrote:
I agree with the process and withdraw my request, replacing it
instead with a disapproving head shake and deep sigh.
We as a community definitely deserve your disapproval, as our review
response times have been abysmal. :(
I’ll go back to pushing on this after the tag. Thanks for your persistence!
Laszlo
|
|
|
Abner Chang
toggle quoted message
Show quoted text
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@...]
Sent: Wednesday, August 19, 2020 9:37 PM
To: Leif Lindholm <leif@...>; Chang, Abner (HPS SW/FW
Technologist) <abner.chang@...>
Cc: devel@edk2.groups.io; liming.gao <liming.gao@...>;
announce@edk2.groups.io; afish@...; Kinney, Michael D
<michael.d.kinney@...>
Subject: Re: [edk2-announce] Re: Soft Feature Freeze starts now for edk2-
stable202008
On 08/19/20 15:19, Laszlo Ersek wrote:
On 08/19/20 13:48, Leif Lindholm wrote:
(Slightly trimmed recipient list due to different patch being
discussed.)
So, I can't make this call, because I'm the one who messed up.
This patch does exactly what I had requested Abner to do some time
back (off-list, unfortunately), and I was *convinced* I gave it an
R-b as soon as it hit my inbox - until Abner nudged me about it yesterday.
The patch in question is
https://edk2.groups.io/g/devel/topic/76021725 My understanding is:
(1) there is an external project that consumes the FDT library in
EmbeddedPkg, meaning the lib class header "EmbeddedPkg/Include/libfdt.h"
and the lib instance "EmbeddedPkg/Library/FdtLib/FdtLib.inf",
(2) the lib class header pulls in "fdt.h" and "libfdt_env.h",
(3) the external project is not edk2-platforms,
(4) the external project wants -- for some strange reason -- edk2's
"libfdt_env.h" to provide an strncmp() function (or function-like
macro), with that particular stncmp() implementation not being needed
in either edk2-platforms or edk2 itself,
(5) the patch for adding said strncmp() was posted on Aug 6th (at
least when viewed from my time zone), i.e., before the SFF,
(6) it was reviewed 12 days later (within the SFF)
If my understanding is correct, then I don't see how this patch could
be considered a bugfix -- even as a feature addition, it seems hardly
justified to me --, and there would have been ~8 days before the SFF
to review it.
I think we should postpone the patch until after the stable tag. Regarding OpenSBI, as of commit 9d56961b2314, the sbi_strncmp() function
has not been removed, and it seems that sbi_strncmp() is still used / called in
at least some build configurations (where the
lib/utils/libfdt/libfdt_env.h:#define strncmp sbi_strncmp
definition is supposed to be in effect).
This means that the codebase can not rid itself of sbi_strncmp(). The code base doesn't have to get rid of using sbi_strncmp function (the code base defines bunch of sbi_strxxx functions) which may be used in other non-fdt related OpenSBI code and it is out of fdt library scope.
To me this indicates that OpenSBI commit 2cfd2fc90488 ("lib: utils: Use
strncmp in fdt_parse_hart_id()", 2020-07-29) was wrong. It should have
replaced sbi_strcmp() with sbi_strncmp(), not strncmp().
We don’t use libfdt_env.h from OpenSBI, we use libfdt_env.h from EmbeddedPkg which is impossible to define a macro to replace "sbi_strncmp" with AsciiStrnCmp in Edk2 libfdt_env.h. We definitely can include sbi_string.h somewhere in EDK2 RISC-V header file (this is the temp solution I use now before edk2 upstream has this fix), so it won't pop up build error. But this temp solution looks ugly and specifically. Furthermore, OpenSBI fdt helper lib is sort of a partial fdtlib code, use C API keeps the consistency with fdtlib makes sense to me.
After all, the size limit seems to be the motivation for the entire change --
put a size limit on the string comparison in fdt_parse_hart_id(). Commit
2cfd2fc90488 did two things at the same
time: replace the size-unbounded comparison with the size-bounded one,
*and* switch to the standard C function name from the self-contained
function. The former goal looks justifiable, the latter I don't understand
Actually the major motivation is not for the bounded comparison, that is to use C API and leverage edk2 libfdt_env.h.
Now: I realize that, going back to edk2 commit fa4a70633868
("EmbeddedPkg: Added support to libfdt", 2012-09-27), we already have a
bunch of wrappers in "EmbeddedPkg/Include/libfdt_env.h". So I guess
adding "one more" is not inconsistent with those -- even though the lib
instance within edk2 doesn't need the new function.
But it's still a micro-feature whose review should have completed before the
SFF.
Thanks
Laszlo
|
|
|
