Date
1 - 10 of 10
Soft Feature Freeze starts now for edk2-stable202008
Leif Lindholm <leif@...>
(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
toggle quoted message
Show quoted text
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 witha 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
Laszlo Ersek
On 08/19/20 13:48, Leif Lindholm wrote:
(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
(Slightly trimmed recipient list due to different patch being discussed.)My understanding is:
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
(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 witha 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
Laszlo Ersek
On 08/19/20 15:19, Laszlo Ersek wrote:
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 13:48, Leif Lindholm wrote:Regarding OpenSBI, as of commit 9d56961b2314, the sbi_strncmp() function(Slightly trimmed recipient list due to different patch being discussed.)My understanding is:
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
(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.
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
Laszlo Ersek
On 08/19/20 15:34, Chang, Abner (HPS SW/FW Technologist) wrote:
- 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
Let's move out the dates for the stable tag then, by one week:-----Original Message-----[Chang, Abner] Yes
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 beingMy understanding is:
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
(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, at least so far
(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
(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)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.
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.
- 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
Liming Gao
toggle quoted message
Show quoted text
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.
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
-----Original Message-----Previous stable tags (202005/202002/201905) extended SFF or HFF period to let the critical bug fix catch the stable tag.
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-----Appreciate for this.
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:review it.-----Original Message-----[Chang, Abner] Yes
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 beingMy understanding is:
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
(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, at least so far
(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
(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)
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 totag (if this patch is accepted) for booting RISC-V platform to Linux kernel withThis patch is important because the edk2-stable202008 would be the stable
I think we should postpone the patch until after the stable tag.
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.
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.
I suggest to define the process on delay the release. The initial process is specified here.
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.
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
I replied this in another thread.
BTW what about reverting the OpenSBI change? You could still call
sbi_strncmp() -- rather than strncmp() -- in the "helper" lib.
Thanks
Laszlo
Laszlo Ersek
On 08/19/20 18:20, Gao, Liming wrote:
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.
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
Yes, exactly.-----Original Message-----Previous stable tags (202005/202002/201905) extended SFF or HFF period to let the critical bug fix catch the stable tag.
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-----Appreciate for this.
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:review it.-----Original Message-----[Chang, Abner] Yes
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 beingMy understanding is:
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
(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, at least so far
(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
(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)
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 totag (if this patch is accepted) for booting RISC-V platform to Linux kernel withThis patch is important because the edk2-stable202008 would be the stable
I think we should postpone the patch until after the stable tag.
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.
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.
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.
I'm OK with delaying the release by one or two weeks, for any feature(s)I suggest to define the process on delay the release. The initial process is specified here.
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.
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.
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 callI replied this in another thread.
sbi_strncmp() -- rather than strncmp() -- in the "helper" lib.
Thanks
Laszlo
toggle quoted message
Show quoted text
-----Original Message-----Appreciate for this.
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:review it.-----Original Message-----[Chang, Abner] Yes
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 beingMy understanding is:
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
(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, at least so far
(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
(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)
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 totag (if this patch is accepted) for booting RISC-V platform to Linux kernel withThis patch is important because the edk2-stable202008 would be the stable
I think we should postpone the patch until after the stable tag.
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.
I replied this in another thread.
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
...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 ita disapproving head shake and deep sigh.
instead with
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
toggle quoted message
Show quoted text
-----Original Message-----[Chang, Abner] Yes
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 beingMy understanding is:
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
(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, at least so far
(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
(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)
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.
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
LaszloOn Wed, Aug 19, 2020 at 11:29:52 +0000, Chang, Abner (HPS SW/FWTechnologist) wrote:OnHmm.. 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]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 ita disapproving head shake and deep sigh.
instead with
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
toggle quoted message
Show quoted text
-----Original Message-----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.
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:"EmbeddedPkg/Include/libfdt.h"(Slightly trimmed recipient list due to different patch beingMy understanding is:
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
(1) there is an external project that consumes the FDT library in
EmbeddedPkg, meaning the lib class headerand the lib instance "EmbeddedPkg/Library/FdtLib/FdtLib.inf",Regarding OpenSBI, as of commit 9d56961b2314, the sbi_strncmp() function
(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.
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().
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.
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().
Actually the major motivation is not for the bounded comparison, that is to use C API and leverage edk2 libfdt_env.h.
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