diff mbox series

Makefile: doesn't need check stack size when dtb is not built

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

Commit Message

AKASHI Takahiro March 4, 2020, 6:54 a.m. UTC
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(-)

Comments

Stephen Warren March 4, 2020, 4:21 p.m. UTC | #1
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.
AKASHI Takahiro March 5, 2020, 12:15 a.m. UTC | #2
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
Stephen Warren March 5, 2020, 12:22 a.m. UTC | #3
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.
AKASHI Takahiro March 5, 2020, 12:39 a.m. UTC | #4
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
Stephen Warren March 5, 2020, 4:54 p.m. UTC | #5
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 mbox series

Patch

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