Patchwork [U-Boot] cfi_flash: fix bug introduced while recent change to flash_get_size()

login
register
mail settings
Submitter Anatolij Gustschin
Date Nov. 28, 2010, 1:13 a.m.
Message ID <1290906813-12017-1-git-send-email-agust@denx.de>
Download mbox | patch
Permalink /patch/73311/
State Accepted
Commit 34bbb8fb467a00aebc3e631e71888c5b00875a68
Headers show

Comments

Anatolij Gustschin - Nov. 28, 2010, 1:13 a.m.
commit ec50a8e389863ac35bfd9d9a2e8b30187318e59e
"cfi_flash: handle 'chip size exceeds address window' situation"
added 3rd argument to flash_get_size() but didn't fix all the
function calls from the board specific code. Many boards have
their own flash_get_size() definitions in the board code and
use them there, but some boards (e.g. tqm834x, tqm85xx, pdm360ng)
use flash_get_size() from the cfi_flash.c driver.

The bug shows up if the value of the "max_size" argument (which
is not defined when calling the function with two arguments)
happens to be less than "info->size". In this case on the
affected boards we end up with a bank of reduced size and
in the worst case might even be not able to update U-Boot or
to boot the kernel from flash:

=> fli

Bank # 1: CFI conformant FLASH (32 x 16)  Size: 0 kB in 1 Sectors
  AMD Standard command set, Manufacturer ID: 0x01, Device ID: 0x227E
  Erase timeout: 4096 ms, write timeout: 1 ms
  Buffer write timeout: 3 ms, buffer size: 64 bytes

  Sector Start Addresses:
  F0000000   RO

Bank # 2: CFI conformant FLASH (32 x 16)  Size: 128 MB in 512 Sectors
  AMD Standard command set, Manufacturer ID: 0x01, Device ID: 0x227E
  Erase timeout: 4096 ms, write timeout: 1 ms
  Buffer write timeout: 3 ms, buffer size: 64 bytes

  Sector Start Addresses:
  F8000000        F8040000        F8080000        F80C0000        F8100000
  F8140000        F8180000        F81C0000        F8200000        F8240000
  ...

E.g., updating U-Boot is not possible now:

=> protect off ${u-boot_addr} +${u-boot_size}
Error: end address (0xf007ffff) not in flash!
Bad address format
=> era ${u-boot_addr} +${u-boot_size}
Error: end address (0xf007ffff) not in flash!
Bad address format

This patch removes the 3rd argument of flash_get_size() again
and sets "max_size" in the function itself instead of passing
it as a function argument.

Signed-off-by: Anatolij Gustschin <agust@denx.de>
---
Probably we should put the prototype of flash_get_size() into
include/flash.h and not use prototype definitions in the
board code. This could prevent such a nasty bug (compile time
errors would show it). But I'm not sure here, need to look
more at the code.

 drivers/mtd/cfi_flash.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)
Wolfgang Denk - Nov. 28, 2010, 6:43 p.m.
Dear Anatolij Gustschin,

In message <1290906813-12017-1-git-send-email-agust@denx.de> you wrote:
> commit ec50a8e389863ac35bfd9d9a2e8b30187318e59e
> "cfi_flash: handle 'chip size exceeds address window' situation"
> added 3rd argument to flash_get_size() but didn't fix all the
> function calls from the board specific code. Many boards have
> their own flash_get_size() definitions in the board code and
> use them there, but some boards (e.g. tqm834x, tqm85xx, pdm360ng)
> use flash_get_size() from the cfi_flash.c driver.
> 
> The bug shows up if the value of the "max_size" argument (which
> is not defined when calling the function with two arguments)
> happens to be less than "info->size". In this case on the
> affected boards we end up with a bank of reduced size and
> in the worst case might even be not able to update U-Boot or
> to boot the kernel from flash:
> 
> => fli
> 
> Bank # 1: CFI conformant FLASH (32 x 16)  Size: 0 kB in 1 Sectors
>   AMD Standard command set, Manufacturer ID: 0x01, Device ID: 0x227E
>   Erase timeout: 4096 ms, write timeout: 1 ms
>   Buffer write timeout: 3 ms, buffer size: 64 bytes
> 
>   Sector Start Addresses:
>   F0000000   RO
> 
> Bank # 2: CFI conformant FLASH (32 x 16)  Size: 128 MB in 512 Sectors
>   AMD Standard command set, Manufacturer ID: 0x01, Device ID: 0x227E
>   Erase timeout: 4096 ms, write timeout: 1 ms
>   Buffer write timeout: 3 ms, buffer size: 64 bytes
> 
>   Sector Start Addresses:
>   F8000000        F8040000        F8080000        F80C0000        F8100000
>   F8140000        F8180000        F81C0000        F8200000        F8240000
>   ...
> 
> E.g., updating U-Boot is not possible now:
> 
> => protect off ${u-boot_addr} +${u-boot_size}
> Error: end address (0xf007ffff) not in flash!
> Bad address format
> => era ${u-boot_addr} +${u-boot_size}
> Error: end address (0xf007ffff) not in flash!
> Bad address format
> 
> This patch removes the 3rd argument of flash_get_size() again
> and sets "max_size" in the function itself instead of passing
> it as a function argument.
> 
> Signed-off-by: Anatolij Gustschin <agust@denx.de>
> ---
> Probably we should put the prototype of flash_get_size() into
> include/flash.h and not use prototype definitions in the
> board code. This could prevent such a nasty bug (compile time
> errors would show it). But I'm not sure here, need to look
> more at the code.
> 
>  drivers/mtd/cfi_flash.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)

Applied, thanks.

Hope this is ok with your, Stefan - I just wanted to have this in
-rc2.

Best regards,

Wolfgang Denk

Patch

diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
index c92c7a7..39c235e 100644
--- a/drivers/mtd/cfi_flash.c
+++ b/drivers/mtd/cfi_flash.c
@@ -1837,7 +1837,7 @@  static void flash_fixup_stm(flash_info_t *info, struct cfi_qry *qry)
  * The following code cannot be run from FLASH!
  *
  */
-ulong flash_get_size (phys_addr_t base, int banknum, unsigned long max_size)
+ulong flash_get_size (phys_addr_t base, int banknum)
 {
 	flash_info_t *info = &flash_info[banknum];
 	int i, j;
@@ -1849,6 +1849,7 @@  ulong flash_get_size (phys_addr_t base, int banknum, unsigned long max_size)
 	int erase_region_size;
 	int erase_region_count;
 	struct cfi_qry qry;
+	unsigned long max_size;
 
 	memset(&qry, 0, sizeof(qry));
 
@@ -1929,6 +1930,7 @@  ulong flash_get_size (phys_addr_t base, int banknum, unsigned long max_size)
 		info->size = 1 << qry.dev_size;
 		/* multiply the size by the number of chips */
 		info->size *= size_ratio;
+		max_size = cfi_flash_bank_size(banknum);
 		if (max_size && (info->size > max_size)) {
 			debug("[truncated from %ldMiB]", info->size >> 20);
 			info->size = max_size;
@@ -2043,8 +2045,7 @@  unsigned long flash_init (void)
 		flash_info[i].flash_id = FLASH_UNKNOWN;
 
 		if (!flash_detect_legacy(cfi_flash_bank_addr(i), i))
-			flash_get_size(cfi_flash_bank_addr(i), i,
-					cfi_flash_bank_size(i));
+			flash_get_size(cfi_flash_bank_addr(i), i);
 		size += flash_info[i].size;
 		if (flash_info[i].flash_id == FLASH_UNKNOWN) {
 #ifndef CONFIG_SYS_FLASH_QUIET_TEST