| Message ID | 20200304065450.1068-1-takahiro.akashi@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Makefile: doesn't need check stack size when dtb is not built | expand |
On 3/3/20 11:54 PM, AKASHI Takahiro wrote: > The commit 5fed97af20da ("Makefile: ensure DTB doesn't overflow into > initial stack") adds an extra check for stack size in BSS if > CONFIG_SYS_INIT_SP_BSS_OFFSET is enabled. > This check, however, doesn't make sense under the configuration where > control dtb won't be built in and it should be void in such cases. Don't you want to edit the following hunk from the original patch instead or as well? +ifneq ($(CONFIG_SYS_INIT_SP_BSS_OFFSET),) +ALL-y += init_sp_bss_offset_check +endif That's why there's no rule for init_sp_bss_offset_check in the else case.
On Wed, Mar 04, 2020 at 09:21:29AM -0700, Stephen Warren wrote: > On 3/3/20 11:54 PM, AKASHI Takahiro wrote: > > The commit 5fed97af20da ("Makefile: ensure DTB doesn't overflow into > > initial stack") adds an extra check for stack size in BSS if > > CONFIG_SYS_INIT_SP_BSS_OFFSET is enabled. > > This check, however, doesn't make sense under the configuration where > > control dtb won't be built in and it should be void in such cases. > > Don't you want to edit the following hunk from the original patch instead or > as well? > > +ifneq ($(CONFIG_SYS_INIT_SP_BSS_OFFSET),) > +ALL-y += init_sp_bss_offset_check > +endif > > That's why there's no rule for init_sp_bss_offset_check in the else case. I intentionally left it as it is because someone may in the future want to add other *sanity checks* whether dtb is used or not. Rather, my concern is: Is "ifneq ($(CONFIG_OF_SEPARATE)$(CONFIG_OF_EMBED)$(CONFIG_OF_HOSTFILE),)" sufficient and appropriate to guard the check? Thanks, -Takahiro Akashi
On 3/4/20 5:15 PM, AKASHI Takahiro wrote: > On Wed, Mar 04, 2020 at 09:21:29AM -0700, Stephen Warren wrote: >> On 3/3/20 11:54 PM, AKASHI Takahiro wrote: >>> The commit 5fed97af20da ("Makefile: ensure DTB doesn't overflow into >>> initial stack") adds an extra check for stack size in BSS if >>> CONFIG_SYS_INIT_SP_BSS_OFFSET is enabled. >>> This check, however, doesn't make sense under the configuration where >>> control dtb won't be built in and it should be void in such cases. >> >> Don't you want to edit the following hunk from the original patch instead or >> as well? >> >> +ifneq ($(CONFIG_SYS_INIT_SP_BSS_OFFSET),) >> +ALL-y += init_sp_bss_offset_check >> +endif >> >> That's why there's no rule for init_sp_bss_offset_check in the else case. > > I intentionally left it as it is because someone may in the future > want to add other *sanity checks* whether dtb is used or not. I'd probably expect the following in that case: ifneq ($(CONFIG_SYS_INIT_SP_BSS_OFFSET),) ifneq ($(CONFIG_OF_SEPARATE)$(CONFIG_OF_EMBED)$(CONFIG_OF_HOSTFILE),) ALL-y += init_sp_bss_offset_check endif ALL-y += some_other_check endif > Rather, my concern is: > Is "ifneq ($(CONFIG_OF_SEPARATE)$(CONFIG_OF_EMBED)$(CONFIG_OF_HOSTFILE),)" > sufficient and appropriate to guard the check? I think OF_SEPARATE is the only case this check makes sense. OF_EMBED sounds like the build process puts the DTB into .data or similar, so the issue doesn't apply. Since OF_HOSTFILE is a sandbox thing where the DT is read from a file, the check definitely doesn't apply.
On Wed, Mar 04, 2020 at 05:22:25PM -0700, Stephen Warren wrote: > On 3/4/20 5:15 PM, AKASHI Takahiro wrote: > > On Wed, Mar 04, 2020 at 09:21:29AM -0700, Stephen Warren wrote: > > > On 3/3/20 11:54 PM, AKASHI Takahiro wrote: > > > > The commit 5fed97af20da ("Makefile: ensure DTB doesn't overflow into > > > > initial stack") adds an extra check for stack size in BSS if > > > > CONFIG_SYS_INIT_SP_BSS_OFFSET is enabled. > > > > This check, however, doesn't make sense under the configuration where > > > > control dtb won't be built in and it should be void in such cases. > > > > > > Don't you want to edit the following hunk from the original patch instead or > > > as well? > > > > > > +ifneq ($(CONFIG_SYS_INIT_SP_BSS_OFFSET),) > > > +ALL-y += init_sp_bss_offset_check > > > +endif > > > > > > That's why there's no rule for init_sp_bss_offset_check in the else case. > > > > I intentionally left it as it is because someone may in the future > > want to add other *sanity checks* whether dtb is used or not. > > I'd probably expect the following in that case: > > ifneq ($(CONFIG_SYS_INIT_SP_BSS_OFFSET),) > ifneq ($(CONFIG_OF_SEPARATE)$(CONFIG_OF_EMBED)$(CONFIG_OF_HOSTFILE),) > ALL-y += init_sp_bss_offset_check > endif > ALL-y += some_other_check > endif > > > Rather, my concern is: > > Is "ifneq ($(CONFIG_OF_SEPARATE)$(CONFIG_OF_EMBED)$(CONFIG_OF_HOSTFILE),)" > > sufficient and appropriate to guard the check? > > I think OF_SEPARATE is the only case this check makes sense. OF_EMBED sounds > like the build process puts the DTB into .data or similar, so the issue > doesn't apply. Since OF_HOSTFILE is a sandbox thing where the DT is read > from a file, the check definitely doesn't apply. Okay, sounds reasonable. (and the target should be dts/dt.dtb rather than u-boot.dtb for clarity?) Thanks, -Takahiro Akashi
On 3/4/20 5:39 PM, AKASHI Takahiro wrote: > On Wed, Mar 04, 2020 at 05:22:25PM -0700, Stephen Warren wrote: >> On 3/4/20 5:15 PM, AKASHI Takahiro wrote: >>> On Wed, Mar 04, 2020 at 09:21:29AM -0700, Stephen Warren wrote: >>>> On 3/3/20 11:54 PM, AKASHI Takahiro wrote: >>>>> The commit 5fed97af20da ("Makefile: ensure DTB doesn't overflow into >>>>> initial stack") adds an extra check for stack size in BSS if >>>>> CONFIG_SYS_INIT_SP_BSS_OFFSET is enabled. >>>>> This check, however, doesn't make sense under the configuration where >>>>> control dtb won't be built in and it should be void in such cases. >>>> >>>> Don't you want to edit the following hunk from the original patch instead or >>>> as well? >>>> >>>> +ifneq ($(CONFIG_SYS_INIT_SP_BSS_OFFSET),) >>>> +ALL-y += init_sp_bss_offset_check >>>> +endif >>>> >>>> That's why there's no rule for init_sp_bss_offset_check in the else case. >>> >>> I intentionally left it as it is because someone may in the future >>> want to add other *sanity checks* whether dtb is used or not. >> >> I'd probably expect the following in that case: >> >> ifneq ($(CONFIG_SYS_INIT_SP_BSS_OFFSET),) >> ifneq ($(CONFIG_OF_SEPARATE)$(CONFIG_OF_EMBED)$(CONFIG_OF_HOSTFILE),) >> ALL-y += init_sp_bss_offset_check >> endif >> ALL-y += some_other_check >> endif >> >>> Rather, my concern is: >>> Is "ifneq ($(CONFIG_OF_SEPARATE)$(CONFIG_OF_EMBED)$(CONFIG_OF_HOSTFILE),)" >>> sufficient and appropriate to guard the check? >> >> I think OF_SEPARATE is the only case this check makes sense. OF_EMBED sounds >> like the build process puts the DTB into .data or similar, so the issue >> doesn't apply. Since OF_HOSTFILE is a sandbox thing where the DT is read >> from a file, the check definitely doesn't apply. > > Okay, sounds reasonable. > (and the target should be dts/dt.dtb rather than u-boot.dtb for clarity?) By "target" I assume you mean the DTB filename in the following? @dtb_size=$(shell wc -c u-boot.dtb | awk '{print $$1}') ; \ If so, it doesn't make any difference; u-boot.dtb is just a copy of dts/dt.dtb: u-boot.dtb: dts/dt.dtb $(call cmd,copy)
diff --git a/Makefile b/Makefile index 0af89e0a7881..78efd7e9e250 100644 --- a/Makefile +++ b/Makefile @@ -1208,7 +1208,10 @@ binary_size_check: u-boot-nodtb.bin FORCE fi \ fi -ifdef CONFIG_INIT_SP_RELATIVE +ifeq ($(CONFIG_INIT_SP_RELATIVE),y) +ifneq ($(CONFIG_OF_SEPARATE)$(CONFIG_OF_EMBED)$(CONFIG_OF_HOSTFILE),) +# u-boot.dtb will be built only if one of those options is enabled + ifneq ($(CONFIG_SYS_MALLOC_F_LEN),) subtract_sys_malloc_f_len = space=$$(($${space} - $(CONFIG_SYS_MALLOC_F_LEN))) else @@ -1233,6 +1236,10 @@ init_sp_bss_offset_check: u-boot.dtb FORCE echo "(CONFIG_SYS_INIT_SP_BSS_OFFSET - CONFIG_SYS_MALLOC_F_LEN)" >&2 ; \ exit 1 ; \ fi +else +init_sp_bss_offset_check: + +endif endif u-boot-nodtb.bin: u-boot FORCE
The commit 5fed97af20da ("Makefile: ensure DTB doesn't overflow into initial stack") adds an extra check for stack size in BSS if CONFIG_SYS_INIT_SP_BSS_OFFSET is enabled. This check, however, doesn't make sense under the configuration where control dtb won't be built in and it should be void in such cases. Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org> Fixes: 5fed97af20da ("Makefile: ensure DTB doesn't overflow into initial stack") --- Makefile | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)