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;

Well, I committed a change, even though I originally wasn't going to do
it this way:

--- drivers/mtd/chips/jedec_probe.c	18 Nov 2004 14:13:09 -0000	1.60
+++ drivers/mtd/chips/jedec_probe.c	19 Nov 2004 20:51:07 -0000
@@ -227,6 +227,11 @@
 	[MTD_UADDR_DONT_CARE] = {
 		.addr1 = 0x0000,      /* Doesn't matter which address */
 		.addr2 = 0x0000       /* is used - must be last entry */
+	},
+
+	[MTD_UADDR_UNNECESSARY] = {
+		.addr1 = 0x0000,
+		.addr2 = 0x0000
 	}
 };

I figure that simply adding a dummy entry will prevent having to always
branch on MTD_UADDR_UNNECESSARRY when the remaining logic can be
identical.

My apologies for taking so long - thanks for your patience.

BTW, I haven't seen any comments regarding this:

On Thu, 2004-11-18 at 07:44 -0700, Thayne Harbaugh wrote: 
> On Thu, 2004-11-18 at 11:50 +0100, Alexander Hoffmann wrote:
> > 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/20041119/a195f8a8/attachment.bin