Re: Possible MTD bug in 2.6.15

["Jim Ramsay" <kernel@jimramsay.com>]

On 22/04/06, Thiago Galesi <thiagogalesi@gmail.com> wrote:
> > > > Ok, a couple of comments/questions
> > > >
> > > > 1 - Wouldn't it be better to map all flash, and leave the unneeded
> > > > part as read only?
> >
> > In general, yes.  But this should either be enforced somewhere nicer
> > (ie, die gracefully) so the kernel doesn't panic later, or be allowed
> > as in my patch.
>
> The fundamental problem there seems to be a mismatch between what is
> set by the user and what is read from the flash chip. As you mention
> in your first message, (what it came across is that) you don't have
> (physical / electrical) access to all the flash; not something I would
> recommend (that is, having limited electrical connection to the flash)

Yes, that is exactly what is going on - we have only have electrical
access to 32M of addresses regardless of the size of the actual chip
installed, and unfortunately I can't change that.  However, it's not
really a problem from the electrical side of things - We can still
access the lowest 32M on the chip, just the highest address pin isn't
connected to anything.

> As for the options you propose - enforce and die gracefully (that is,
> if there is a size mismatch, warning and purposely not working) seems
> more correct than the second option.

I don't see why - The size mismatch doesn't prevent the flash chip
from functioning.  Isn't it better to help things work in more
circumstances rather than in less?

And really, it's not a big stretch from what the code currently does
to what my patch changes.  At this point in the code, we know for a
fact that we already have at least one flash chip.  The math that's
going on here with the 'max_chips' variable is to check if there is
actually more than one physical chip implementing the entire reported
size.  The only mistake is that the math goes too far, shifting the
count down to zero if the reported size is too small.  This
'max_chips' should never be allowed to be lower than 1, because we
really do know that there is at least one flash chip.

I agree with you that it's probably "more correct" to actually specify
the real size of the chip in the 'map' struct before the CFI probe,
but I can't think of any reason why specifying a smaller size should
die (even gracefully) when it still works just fine.

Of course, maybe there are some models of flash chips where this
wouldn't work and a graceful prevention of this would be important. 
But with our CFI-compliant chip, we don't see any problem with my
patch applied.

--
Jim Ramsay
"Me fail English?  That's unpossible!"