[PATCH] cpu: imx8_cpu: Avoid revision to corrupt device tree
From: Peng Fan peng.fan@nxp.com
U-Boot device tree is padded just after U-Boot proper. After the whole stuff loaded to DRAM space, the device tree area is conflict with BSS region before U-Boot relocation. So any write to BSS area before reloc_fdt will corrupt the device tree. Without the fix, there is issue that “binman_init failed:-2” on i.MX8MP-EVK board.
Move the variable to data section to fix the issue
Signed-off-by: Peng Fan peng.fan@nxp.com --- drivers/cpu/imx8_cpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/cpu/imx8_cpu.c b/drivers/cpu/imx8_cpu.c index 6c0a8c0cbe4..19c65c7ce47 100644 --- a/drivers/cpu/imx8_cpu.c +++ b/drivers/cpu/imx8_cpu.c @@ -71,7 +71,7 @@ static const char *get_imx_type_str(u32 imxtype)
static const char *get_imx_rev_str(u32 rev) { - static char revision[4]; + static char revision[4] __section(".data");
if (IS_ENABLED(CONFIG_IMX8)) { switch (rev) {
On Fri, Oct 11, 2024 at 06:58:27PM +0800, Peng Fan (OSS) wrote:
From: Peng Fan peng.fan@nxp.com
U-Boot device tree is padded just after U-Boot proper. After the whole stuff loaded to DRAM space, the device tree area is conflict with BSS region before U-Boot relocation. So any write to BSS area before reloc_fdt will corrupt the device tree. Without the fix, there is issue that “binman_init failed:-2” on i.MX8MP-EVK board.
Move the variable to data section to fix the issue
Signed-off-by: Peng Fan peng.fan@nxp.com
drivers/cpu/imx8_cpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/cpu/imx8_cpu.c b/drivers/cpu/imx8_cpu.c index 6c0a8c0cbe4..19c65c7ce47 100644 --- a/drivers/cpu/imx8_cpu.c +++ b/drivers/cpu/imx8_cpu.c @@ -71,7 +71,7 @@ static const char *get_imx_type_str(u32 imxtype)
static const char *get_imx_rev_str(u32 rev) {
- static char revision[4];
static char revision[4] __section(".data");
if (IS_ENABLED(CONFIG_IMX8)) { switch (rev) {
Why isn't this padding and alignment with the BSS being taken care of in either the linker script or the binman dts?
Subject: Re: [PATCH] cpu: imx8_cpu: Avoid revision to corrupt device tree
Why isn't this padding and alignment with the BSS being taken care
of
in either the linker script or the binman dts?
Sorry, I am not sure what you mean.
Add a alignment in binman node for FIT dts to avoid dts area conflict with bss?
Peng, in addition to Tom's comment: which commit caused the problem?
Please add a Fixes tag.
It is just this commit makes this issue visible 51e7c1f822fa222d88d1ee06995cf355542427cc mmc: fsl_esdhc_imx: Send 80 clocks before IDLE command
But the real commit should be 38e319782e9c3a1257329b8b7e276d10a30efb73 imx: imx8_cpu: support i.MX9
Regards, Peng.
On Sat, Oct 12, 2024 at 12:16:14AM +0000, Peng Fan wrote:
Subject: Re: [PATCH] cpu: imx8_cpu: Avoid revision to corrupt device tree
Why isn't this padding and alignment with the BSS being taken care
of
in either the linker script or the binman dts?
Sorry, I am not sure what you mean.
I mean, why are things placed in this position to start with? Why is the device tree not already in a place where we aren't smashing it at run time? Is this a problem of the alignment / placement of items in a blob? A runtime placement problem? What?
Subject: Re: [PATCH] cpu: imx8_cpu: Avoid revision to corrupt device tree
On Sat, Oct 12, 2024 at 12:16:14AM +0000, Peng Fan wrote:
Subject: Re: [PATCH] cpu: imx8_cpu: Avoid revision to corrupt
device
tree
Why isn't this padding and alignment with the BSS being taken
care
of
in either the linker script or the binman dts?
Sorry, I am not sure what you mean.
I mean, why are things placed in this position to start with? Why is the device tree not already in a place where we aren't smashing it at run time? Is this a problem of the alignment / placement of items in a blob? A runtime placement problem? What?
Whether binman or not. I think u-boot.dtb is padded just end of u-boot-nodtb.bin. Not alignment or else.
This is a common issue, bss should not be written before reloc_fdt.
Regards, Peng.
-- Tom
On Thu, Oct 17, 2024 at 01:10:02AM +0000, Peng Fan wrote:
Subject: Re: [PATCH] cpu: imx8_cpu: Avoid revision to corrupt device tree
On Sat, Oct 12, 2024 at 12:16:14AM +0000, Peng Fan wrote:
Subject: Re: [PATCH] cpu: imx8_cpu: Avoid revision to corrupt
device
tree
Why isn't this padding and alignment with the BSS being taken
care
of
in either the linker script or the binman dts?
Sorry, I am not sure what you mean.
I mean, why are things placed in this position to start with? Why is the device tree not already in a place where we aren't smashing it at run time? Is this a problem of the alignment / placement of items in a blob? A runtime placement problem? What?
Whether binman or not. I think u-boot.dtb is padded just end of u-boot-nodtb.bin. Not alignment or else.
This is a common issue, bss should not be written before reloc_fdt.
It's possible I'm missing the examples in my quick grep right now but, yes, why are you needing to make this device tree change so early? We should not be using the "put this in the data section" kludge unless strictly necessary. Why is this necessary and cannot wait until, well, further along in the boot? This is supposed to be for pre-DRAM-initialization stuff.
Subject: Re: [PATCH] cpu: imx8_cpu: Avoid revision to corrupt device tree
On Thu, Oct 17, 2024 at 01:10:02AM +0000, Peng Fan wrote:
Subject: Re: [PATCH] cpu: imx8_cpu: Avoid revision to corrupt
device
tree
On Sat, Oct 12, 2024 at 12:16:14AM +0000, Peng Fan wrote:
Subject: Re: [PATCH] cpu: imx8_cpu: Avoid revision to corrupt
device
tree
Why isn't this padding and alignment with the BSS being
taken
care
of
in either the linker script or the binman dts?
Sorry, I am not sure what you mean.
I mean, why are things placed in this position to start with? Why is the device tree not already in a place where we aren't smashing it at run time? Is this a problem of the alignment / placement of items
in a blob?
A runtime placement problem? What?
Whether binman or not. I think u-boot.dtb is padded just end of u-boot-nodtb.bin. Not alignment or else.
This is a common issue, bss should not be written before reloc_fdt.
It's possible I'm missing the examples in my quick grep right now but, yes, why are you needing to make this device tree change so early? We should not be using the "put this in the data section" kludge unless strictly necessary. Why is this necessary and cannot wait until, well, further along in the boot? This is supposed to be for pre-DRAM- initialization stuff.
You may misunderstand. It is imx8_cpu driver probe at pre reloc stage before reloc_fdt, so the written to bss area in imx8_cpu driver will corrupt device tree which padded after u-boot-nodtb.bin.
Then after reloc_fdt, the new dtb is actually a broken one.
Regards, Peng.
-- Tom
Hi Peng,
On Fri, Oct 11, 2024 at 1:56 PM Tom Rini trini@konsulko.com wrote:
Why isn't this padding and alignment with the BSS being taken care of in either the linker script or the binman dts?
I have the same concern as Tom. This fix looks fragile.
Can you propose another solution?
On Fri, Oct 11, 2024 at 06:58:27PM +0800, Peng Fan (OSS) wrote:
From: Peng Fan peng.fan@nxp.com
U-Boot device tree is padded just after U-Boot proper. After the whole stuff loaded to DRAM space, the device tree area is conflict with BSS region before U-Boot relocation. So any write to BSS area before reloc_fdt will corrupt the device tree. Without the fix, there is issue that “binman_init failed:-2” on i.MX8MP-EVK board.
Move the variable to data section to fix the issue
Signed-off-by: Peng Fan peng.fan@nxp.com
drivers/cpu/imx8_cpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/cpu/imx8_cpu.c b/drivers/cpu/imx8_cpu.c index 6c0a8c0cbe4..19c65c7ce47 100644 --- a/drivers/cpu/imx8_cpu.c +++ b/drivers/cpu/imx8_cpu.c @@ -71,7 +71,7 @@ static const char *get_imx_type_str(u32 imxtype)
static const char *get_imx_rev_str(u32 rev) {
- static char revision[4];
- static char revision[4] __section(".data");
After my last mail, I pulled up the patch itself and started looking at the driver. Why is "revision" being done like this? This is not a good common practice.
Subject: Re: [PATCH] cpu: imx8_cpu: Avoid revision to corrupt device tree
On Fri, Oct 11, 2024 at 06:58:27PM +0800, Peng Fan (OSS) wrote:
From: Peng Fan peng.fan@nxp.com
U-Boot device tree is padded just after U-Boot proper. After the
whole
stuff loaded to DRAM space, the device tree area is conflict with BSS region before U-Boot relocation. So any write to BSS area before reloc_fdt will corrupt the device tree. Without the fix, there is issue that “binman_init failed:-2” on i.MX8MP-EVK board.
Move the variable to data section to fix the issue
Signed-off-by: Peng Fan peng.fan@nxp.com
drivers/cpu/imx8_cpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/cpu/imx8_cpu.c b/drivers/cpu/imx8_cpu.c index 6c0a8c0cbe4..19c65c7ce47 100644 --- a/drivers/cpu/imx8_cpu.c +++ b/drivers/cpu/imx8_cpu.c @@ -71,7 +71,7 @@ static const char *get_imx_type_str(u32
imxtype)
static const char *get_imx_rev_str(u32 rev) {
- static char revision[4];
- static char revision[4] __section(".data");
After my last mail, I pulled up the patch itself and started looking at the driver. Why is "revision" being done like this? This is not a good common practice.
Since the function returns a pointer to a string, so ... We could move it to malloc area.
Regards, Peng.
-- Tom
On Thu, Oct 17, 2024 at 03:14:46AM +0000, Peng Fan wrote:
Subject: Re: [PATCH] cpu: imx8_cpu: Avoid revision to corrupt device tree
On Fri, Oct 11, 2024 at 06:58:27PM +0800, Peng Fan (OSS) wrote:
From: Peng Fan peng.fan@nxp.com
U-Boot device tree is padded just after U-Boot proper. After the
whole
stuff loaded to DRAM space, the device tree area is conflict with BSS region before U-Boot relocation. So any write to BSS area before reloc_fdt will corrupt the device tree. Without the fix, there is issue that “binman_init failed:-2” on i.MX8MP-EVK board.
Move the variable to data section to fix the issue
Signed-off-by: Peng Fan peng.fan@nxp.com
drivers/cpu/imx8_cpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/cpu/imx8_cpu.c b/drivers/cpu/imx8_cpu.c index 6c0a8c0cbe4..19c65c7ce47 100644 --- a/drivers/cpu/imx8_cpu.c +++ b/drivers/cpu/imx8_cpu.c @@ -71,7 +71,7 @@ static const char *get_imx_type_str(u32
imxtype)
static const char *get_imx_rev_str(u32 rev) {
- static char revision[4];
- static char revision[4] __section(".data");
After my last mail, I pulled up the patch itself and started looking at the driver. Why is "revision" being done like this? This is not a good common practice.
Since the function returns a pointer to a string, so ... We could move it to malloc area.
Yes, something like that sounds better, thanks.
participants (4)
-
Fabio Estevam -
Peng Fan -
Peng Fan (OSS) -
Tom Rini