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

[Mark Zhan <rongkai.zhan@windriver.com>]

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];
>