[U-Boot] [PATCH] ARM: mvebu: don't dereference null bd pointer

[U-Boot] [PATCH] ARM: mvebu: don't dereference null bd pointer

[Chris Packham]

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;
-	}
-
-
 	if (ecc_enabled())
 		dram_ecc_scrubbing();
 
-- 
2.23.0

[U-Boot] [PATCH] ARM: mvebu: don't dereference null bd pointer

[Stefan Roese]

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

[U-Boot] [PATCH] ARM: mvebu: don't dereference null bd pointer

[Chris Packham]

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.

[U-Boot] [PATCH] ARM: mvebu: don't dereference null bd pointer

[Stefan Roese]

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

[U-Boot] [PATCH] ARM: mvebu: don't dereference null bd pointer

[Stefan Roese]

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