Message ID | 20241011105827.7729-1-peng.fan@oss.nxp.com |
---|---|
State | Changes Requested |
Delegated to: | Fabio Estevam |
Headers | show |
Series | cpu: imx8_cpu: Avoid revision to corrupt device tree | expand |
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?
> 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.
> 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.
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 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
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.
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) {