Usage of MTD_UADDR_UNNECESSARY broken?

[Thayne Harbaugh]

On Thu, 2004-11-18 at 11:50 +0100, Alexander Hoffmann wrote:

> Then I guess that there is really the bug I described in my first mail.
> I would recommend a check for MTD_UADDR_UNNECESSARY in the 
> cfi_jedec_setup(), before
> the unlock_addrs[] array is  referenced:
> 
> if ( uaddr != MTD_UADDR_UNNECESSARY ) {
>                 p_cfi->addr_unlock1 = unlock_addrs[uaddr].addr1 & mask;
>                 p_cfi->addr_unlock2 = unlock_addrs[uaddr].addr2 & mask;
> }
> return 1;

Apparently the "& mask" is not done anymore - you must be using an older
version of jedec_probe.c.

It looks like your suggestion might be appropriate after the window
check in jedec_setup() and before the finfo_uaddr() lookup.

I'm still trying to sort the test at the top of jedec_probe_chip():

 retry:
	if (!cfi->numchips) {
		uaddr_idx++;

		if (MTD_UADDR_UNNECESSARY == uaddr_idx)
			return 0;

		cfi->addr_unlock1 = unlock_addrs[uaddr_idx].addr1;
		cfi->addr_unlock2 = unlock_addrs[uaddr_idx].addr2;
	}


I'm thinking that the MTD_UADDR_UNNECESSARY test (which checks for the
end-of-loop condition) should be *prior* to the uaddr_idx++.  The way it
is, chips that are MTD_UADDR_UNNECESSARY will *never* be tested and will
*always* fail.

> Furthermore I dont understand the following code in the finfo_uaddr() 
> function:
> 
> 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];
>         }

I don't necessarily understand it either.  I have chip documentation
that shows that the unlock addresses of a chip are different for
different modes.  Unfortunately, I don't have a chip to test.

> In my opinion this can't work, because there are a lot of  entries in 
> the jedec_table[] where uaddr[0]
> is not defined ?

C specifications require that structures with static initializers always
have uninitialized members set to 0.  That means that entries in
jedec_table[] that don't have a specific uaddr[0] initializer then it is
0 - which is MTD_UADDR_UNSUPPORTED.  What it buys us is that there's
less typing and any width that isn't explicitly stated as supported is
not supported.

Considering the above WRT the code logic, it means that if a width for a
chip is not supported (MTD_UADDR_NOT_SUPPORTED) then it will return
whatever the unlock address is for X8.  The return value for finfo_uaddr
() is used to not only determine what the unlock addresses should be,
but also if a chip functions in a particular width.  This could cause
chips that only have an X16 and greater modes, but no X8 mode to fail in
jedec_match() and jedec_setup().

Yes, it does look broken.  If the ASSERT comment is *really* true (I am
not convinced) then they should accomplish what they want by using the
same uaddr initializer for all modes of a chip.

-- 
Thayne Harbaugh
Linux Networx
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : http://lists.infradead.org/pipermail/linux-mtd/attachments/20041118/83693a11/attachment.bin