| Message ID | 20191022070525.13648-1-judge.packham@gmail.com |
|---|---|
| State | Accepted |
| Commit | 2ec330fcc2d11f8f420ad835c827c6ad29a44a06 |
| Delegated to: | Stefan Roese |
| Headers | show |
| Series | [U-Boot] ARM: mvebu: don't dereference null bd pointer | expand |
Hi Chris, On 22.10.19 09:05, Chris Packham wrote: > As mentioned in doc/README.arm-relocation gd->bd is not available in > dram_init() so we shouldn't attempt to access it. > > Signed-off-by: Chris Packham <judge.packham@gmail.com> > --- > > arch/arm/mach-mvebu/dram.c | 10 ---------- > 1 file changed, 10 deletions(-) > > diff --git a/arch/arm/mach-mvebu/dram.c b/arch/arm/mach-mvebu/dram.c > index fa8c799a462e..ba8ebc62887f 100644 > --- a/arch/arm/mach-mvebu/dram.c > +++ b/arch/arm/mach-mvebu/dram.c > @@ -281,16 +281,6 @@ int dram_init(void) > size = MVEBU_SDRAM_SIZE_MAX; > } > > - for (; i < CONFIG_NR_DRAM_BANKS; i++) { > - /* If above loop terminated prematurely, we need to set > - * remaining banks' start address & size as 0. Otherwise other > - * u-boot functions and Linux kernel gets wrong values which > - * could result in crash */ > - gd->bd->bi_dram[i].start = 0; > - gd->bd->bi_dram[i].size = 0; > - } > - > - How did you spot this issue? Did you see some crash / oops on one of your boards lately? I'm asking since I've never seen any issues with this code. Thanks, Stefan
On Wed, Oct 23, 2019 at 1:15 AM Stefan Roese <sr@denx.de> wrote: > > Hi Chris, > > On 22.10.19 09:05, Chris Packham wrote: > > As mentioned in doc/README.arm-relocation gd->bd is not available in > > dram_init() so we shouldn't attempt to access it. > > > > Signed-off-by: Chris Packham <judge.packham@gmail.com> > > --- > > > > arch/arm/mach-mvebu/dram.c | 10 ---------- > > 1 file changed, 10 deletions(-) > > > > diff --git a/arch/arm/mach-mvebu/dram.c b/arch/arm/mach-mvebu/dram.c > > index fa8c799a462e..ba8ebc62887f 100644 > > --- a/arch/arm/mach-mvebu/dram.c > > +++ b/arch/arm/mach-mvebu/dram.c > > @@ -281,16 +281,6 @@ int dram_init(void) > > size = MVEBU_SDRAM_SIZE_MAX; > > } > > > > - for (; i < CONFIG_NR_DRAM_BANKS; i++) { > > - /* If above loop terminated prematurely, we need to set > > - * remaining banks' start address & size as 0. Otherwise other > > - * u-boot functions and Linux kernel gets wrong values which > > - * could result in crash */ > > - gd->bd->bi_dram[i].start = 0; > > - gd->bd->bi_dram[i].size = 0; > > - } > > - > > - > > How did you spot this issue? Did you see some crash / oops on one of > your boards lately? I'm asking since I've never seen any issues with > this code. > Actually spotted this on another board. Part of the $dayjob customisations include a diagnostic menu used for production testing. For mostly historical reasons we try and run the RAM test from the pre-relocation parts of u-boot. This lets us test all of RAM but as a consequence the bank start/size get overwritten with the test pattern so we hit a bug where the test would work the first time but then run off into the weeds the second time. Aside from the wrongness I'm not aware of any problem this causes.
On 23.10.19 09:48, Chris Packham wrote: > On Wed, Oct 23, 2019 at 1:15 AM Stefan Roese <sr@denx.de> wrote: >> >> Hi Chris, >> >> On 22.10.19 09:05, Chris Packham wrote: >>> As mentioned in doc/README.arm-relocation gd->bd is not available in >>> dram_init() so we shouldn't attempt to access it. >>> >>> Signed-off-by: Chris Packham <judge.packham@gmail.com> >>> --- >>> >>> arch/arm/mach-mvebu/dram.c | 10 ---------- >>> 1 file changed, 10 deletions(-) >>> >>> diff --git a/arch/arm/mach-mvebu/dram.c b/arch/arm/mach-mvebu/dram.c >>> index fa8c799a462e..ba8ebc62887f 100644 >>> --- a/arch/arm/mach-mvebu/dram.c >>> +++ b/arch/arm/mach-mvebu/dram.c >>> @@ -281,16 +281,6 @@ int dram_init(void) >>> size = MVEBU_SDRAM_SIZE_MAX; >>> } >>> >>> - for (; i < CONFIG_NR_DRAM_BANKS; i++) { >>> - /* If above loop terminated prematurely, we need to set >>> - * remaining banks' start address & size as 0. Otherwise other >>> - * u-boot functions and Linux kernel gets wrong values which >>> - * could result in crash */ >>> - gd->bd->bi_dram[i].start = 0; >>> - gd->bd->bi_dram[i].size = 0; >>> - } >>> - >>> - >> >> How did you spot this issue? Did you see some crash / oops on one of >> your boards lately? I'm asking since I've never seen any issues with >> this code. >> > > Actually spotted this on another board. > > Part of the $dayjob customisations include a diagnostic menu used for > production testing. For mostly historical reasons we try and run the > RAM test from the pre-relocation parts of u-boot. This lets us test > all of RAM but as a consequence the bank start/size get overwritten > with the test pattern so we hit a bug where the test would work the > first time but then run off into the weeds the second time. > > Aside from the wrongness I'm not aware of any problem this causes. Thanks for the explanation. Reviewed-by: Stefan Roese <sr@denx.de> Thanks, Stefan
On 22.10.19 09:05, Chris Packham wrote: > As mentioned in doc/README.arm-relocation gd->bd is not available in > dram_init() so we shouldn't attempt to access it. > > Signed-off-by: Chris Packham <judge.packham@gmail.com> Applied to u-boot-marvell/master. Thanks, Stefan
diff --git a/arch/arm/mach-mvebu/dram.c b/arch/arm/mach-mvebu/dram.c index fa8c799a462e..ba8ebc62887f 100644 --- a/arch/arm/mach-mvebu/dram.c +++ b/arch/arm/mach-mvebu/dram.c @@ -281,16 +281,6 @@ int dram_init(void) size = MVEBU_SDRAM_SIZE_MAX; } - for (; i < CONFIG_NR_DRAM_BANKS; i++) { - /* If above loop terminated prematurely, we need to set - * remaining banks' start address & size as 0. Otherwise other - * u-boot functions and Linux kernel gets wrong values which - * could result in crash */ - gd->bd->bi_dram[i].start = 0; - gd->bd->bi_dram[i].size = 0; - } - - if (ecc_enabled()) dram_ecc_scrubbing();
As mentioned in doc/README.arm-relocation gd->bd is not available in dram_init() so we shouldn't attempt to access it. Signed-off-by: Chris Packham <judge.packham@gmail.com> --- arch/arm/mach-mvebu/dram.c | 10 ---------- 1 file changed, 10 deletions(-)