[PATCH] cpu: imx8_cpu: Avoid revision to corrupt device tree

[PATCH] cpu: imx8_cpu: Avoid revision to corrupt device tree

[Peng Fan (OSS)]

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) {
-- 
2.35.3

Re: [PATCH] cpu: imx8_cpu: Avoid revision to corrupt device tree

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1231 bytes --]

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?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] cpu: imx8_cpu: Avoid revision to corrupt device tree

[Fabio Estevam]

> Why isn't this padding and alignment with the BSS being taken care of in
> either the linker script or the binman dts?

Peng, in addition to Tom's comment: which commit caused the problem?

Please add a Fixes tag.

RE: [PATCH] cpu: imx8_cpu: Avoid revision to corrupt device tree

[Peng Fan]

> 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.

Re: [PATCH] cpu: imx8_cpu: Avoid revision to corrupt device tree

[Fabio Estevam]

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?

Re: [PATCH] cpu: imx8_cpu: Avoid revision to corrupt device tree

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 597 bytes --]

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?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

RE: [PATCH] cpu: imx8_cpu: Avoid revision to corrupt device tree

[Peng Fan]

> 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

Re: [PATCH] cpu: imx8_cpu: Avoid revision to corrupt device tree

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1374 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

RE: [PATCH] cpu: imx8_cpu: Avoid revision to corrupt device tree

[Peng Fan]

> 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

Re: [PATCH] cpu: imx8_cpu: Avoid revision to corrupt device tree

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1213 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

RE: [PATCH] cpu: imx8_cpu: Avoid revision to corrupt device tree

[Peng Fan]

> 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

Re: [PATCH] cpu: imx8_cpu: Avoid revision to corrupt device tree

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1634 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]