[PATCH] Fix the unlock addr lookup BUG in MTD JEDEC probe

[PATCH] Fix the unlock addr lookup BUG in MTD JEDEC probe

[Mark Zhan]

Hi All,

This patch fixes a BUG in the function finfo_uaddr() in driver/mtd/chips/jedec_probe.c.
This function will fetch the unlock addr type from the pre-defined flash chip info.

static inline __u8 finfo_uaddr(const struct amd_flash_info *finfo, int device_type)
{
	int uaddr_idx;
	__u8 uaddr = MTD_UADDR_NOT_SUPPORTED;

	switch ( device_type ) {
	case CFI_DEVICETYPE_X8:  uaddr_idx = 0; break;
	case CFI_DEVICETYPE_X16: uaddr_idx = 1; break;
	case CFI_DEVICETYPE_X32: uaddr_idx = 2; break;
	default:
		printk(KERN_NOTICE "MTD: %s(): unknown device_type %d\n",
		       __func__, device_type);
		goto uaddr_done;
	}

	uaddr = finfo->uaddr[uaddr_idx];

	if (uaddr != MTD_UADDR_NOT_SUPPORTED ) {
		/* ASSERT("The unlock addresses for non-8-bit mode
		   are bollocks. We don't really need an array."); */
		uaddr = finfo->uaddr[0];
	}

  uaddr_done:
	return uaddr;
}

However, the later 'if' sentence will force that the first unlock addr type is always returned.
If a chip has two device types (x8 x16) and the chip works in x16 mode, this bug will result in
that the chip can't be probed correctly because the unlock addr doesn't match.

This patch fixes this bug.

Signed-off-by: Rongkai.Zhan <rongkai.zhan@windriver.com>

----
diff --git a/drivers/mtd/chips/jedec_probe.c b/drivers/mtd/chips/jedec_probe.c
index 8f39d0a..a0ab0df 100644
--- a/drivers/mtd/chips/jedec_probe.c
+++ b/drivers/mtd/chips/jedec_probe.c
@@ -1804,7 +1804,7 @@ static inline __u8 finfo_uaddr(const str

  	uaddr = finfo->uaddr[uaddr_idx];

-	if (uaddr != MTD_UADDR_NOT_SUPPORTED ) {
+	if (uaddr == MTD_UADDR_NOT_SUPPORTED ) {
  		/* ASSERT("The unlock addresses for non-8-bit mode
  		   are bollocks. We don't really need an array."); */
  		uaddr = finfo->uaddr[0];

Re: [PATCH] Fix the unlock addr lookup BUG in MTD JEDEC probe

[Mark Zhan]

cc to linux-mtd

Best Regards,
Mark. Zhan
-----Original Message-----
From: Zhan, Rongkai
Sent: Thursday, July 27, 2006 7:39 PM
To: 'David Woodhouse'
Cc: akpm@osdl.org; 'linux-mtd@lists.infradead.org'
Subject: RE: + fix-the-unlock-addr-lookup-bug-in-mtd-jedec-probe.patchadded to -mm tree

Hi David,

I use AMD AM29LV800BT chips (4 x16 in a 64-bit bank) in my MIPS32 4kc board.

The following are the kernel boot logs before and after applying my patch.
BTW, I turn on MTD DEBUG Level to 3, and add more DEBUG prinks in jedec_probe.c :-)

===================Kernel boot log before patched =====================
physmap flash device: 400000 at 1fc00000
MTD jedec_probe_chip() enter: map phys_mapped_flash, base 0x0, chip_map 0x0, cfi interleave x4, device type 2-bit
reset unlock called 555 2aa
Search for id (01 22da): CFI interleave x4, device type (16-bit), unlock addr (0555 02aa)
MTD jedec_match(): Check fit 0x00000000 + 0x00100000 = 0x00100000
MTD finfo_uaddr(): uaddr_idx 1, uaddr type 1
MTD finfo_uaddr(): uaddr type changes from 1 to 4
MTD jedec_match(): check CFI unlock addrs (0x0555 0x02aa) dev_type 2
MTD jedec_match(): pre-defined unlock addr (0x0aaa 0x0555) (idx: 4) did not match
reset unlock called 555 aaa
Search for id (01 22da): CFI interleave x4, device type (16-bit), unlock addr (0555 0aaa)
MTD jedec_match(): Check fit 0x00000000 + 0x00100000 = 0x00100000
MTD finfo_uaddr(): uaddr_idx 1, uaddr type 1
MTD finfo_uaddr(): uaddr type changes from 1 to 4
MTD jedec_match(): check CFI unlock addrs (0x0555 0x0aaa) dev_type 2
MTD jedec_match(): pre-defined unlock addr (0x0aaa 0x0555) (idx: 4) did not match
reset unlock called 5555 2aaa
Search for id (01 22da): CFI interleave x4, device type (16-bit), unlock addr (5555 2aaa)
MTD jedec_match(): Check fit 0x00000000 + 0x00100000 = 0x00100000
MTD finfo_uaddr(): uaddr_idx 1, uaddr type 1
MTD finfo_uaddr(): uaddr type changes from 1 to 4
MTD jedec_match(): check CFI unlock addrs (0x5555 0x2aaa) dev_type 2
MTD jedec_match(): pre-defined unlock addr (0x0aaa 0x0555) (idx: 4) did not match
reset unlock called aaa 555

===================Kernel boot log After patched =====================
physmap flash device: 400000 at 1fc00000
MTD jedec_probe_chip() enter: map phys_mapped_flash, base 0x0, chip_map 0x0, cfi interleave x4, device type 2-bit
reset unlock called 555 2aa
Search for id (01 22da): CFI interleave x4, device type (16-bit), unlock addr (0555 02aa)
MTD jedec_match(): Check fit 0x00000000 + 0x00100000 = 0x00100000
MTD finfo_uaddr(): uaddr_idx 1, uaddr type 1
MTD jedec_match(): check CFI unlock addrs (0x0555 0x02aa) dev_type 2
MTD jedec_match(): check ID's disappear when not in ID mode
reset unlock called 555 2aa
MTD jedec_match(): return to ID mode
MTD jedec_probe_chip(): matched device 0x1,0x22da unlock_addrs: 0x0555 0x02aa
Found: AMD AM29LV800BT
=====================================================================

Best Regards,
Mark. Zhan

-----Original Message-----
From: David Woodhouse [mailto:dwmw2@infradead.org]
Sent: Wednesday, July 26, 2006 3:25 PM
To: akpm@osdl.org
Cc: Zhan, Rongkai
Subject: Re: + fix-the-unlock-addr-lookup-bug-in-mtd-jedec-probe.patchadded to -mm tree

On Wed, 2006-07-26 at 00:08 -0700, akpm@osdl.org wrote:
 > However, the later 'if' sentence will force that the first unlock addr type
 > is always returned.  If a chip has two device types (x8 x16) and the chip
 > works in x16 mode, this bug will result in that the chip can't be probed
 > correctly because the unlock addr doesn't match.

Chip probe code makes my head hurt. I have vague memories that this was
done on purpose -- we want to drop the magic unlock addresses for
different modes and _calculate_ them instead. The difference between x8
and x16 mode is just a simple shift. I think the best answer is probably
to fix your chip's entry in the table instead.

Please post to the linux-mtd list, giving precise details of the chip
you're using.

--
dwmw2

Mark Zhan wrote:
> Hi All,
> 
> This patch fixes a BUG in the function finfo_uaddr() in 
> driver/mtd/chips/jedec_probe.c.
> This function will fetch the unlock addr type from the pre-defined flash 
> chip info.
> 
> static inline __u8 finfo_uaddr(const struct amd_flash_info *finfo, int 
> device_type)
> {
>     int uaddr_idx;
>     __u8 uaddr = MTD_UADDR_NOT_SUPPORTED;
> 
>     switch ( device_type ) {
>     case CFI_DEVICETYPE_X8:  uaddr_idx = 0; break;
>     case CFI_DEVICETYPE_X16: uaddr_idx = 1; break;
>     case CFI_DEVICETYPE_X32: uaddr_idx = 2; break;
>     default:
>         printk(KERN_NOTICE "MTD: %s(): unknown device_type %d\n",
>                __func__, device_type);
>         goto uaddr_done;
>     }
> 
>     uaddr = finfo->uaddr[uaddr_idx];
> 
>     if (uaddr != MTD_UADDR_NOT_SUPPORTED ) {
>         /* ASSERT("The unlock addresses for non-8-bit mode
>            are bollocks. We don't really need an array."); */
>         uaddr = finfo->uaddr[0];
>     }
> 
>  uaddr_done:
>     return uaddr;
> }
> 
> However, the later 'if' sentence will force that the first unlock addr 
> type is always returned.
> If a chip has two device types (x8 x16) and the chip works in x16 mode, 
> this bug will result in
> that the chip can't be probed correctly because the unlock addr doesn't 
> match.
> 
> This patch fixes this bug.
> 
> Signed-off-by: Rongkai.Zhan <rongkai.zhan@windriver.com>
> 
> ----
> diff --git a/drivers/mtd/chips/jedec_probe.c 
> b/drivers/mtd/chips/jedec_probe.c
> index 8f39d0a..a0ab0df 100644
> --- a/drivers/mtd/chips/jedec_probe.c
> +++ b/drivers/mtd/chips/jedec_probe.c
> @@ -1804,7 +1804,7 @@ static inline __u8 finfo_uaddr(const str
> 
>      uaddr = finfo->uaddr[uaddr_idx];
> 
> -    if (uaddr != MTD_UADDR_NOT_SUPPORTED ) {
> +    if (uaddr == MTD_UADDR_NOT_SUPPORTED ) {
>          /* ASSERT("The unlock addresses for non-8-bit mode
>             are bollocks. We don't really need an array."); */
>          uaddr = finfo->uaddr[0];
>