MTD concat layer

MTD concat layer

[Robert Kaiser]

[-- Attachment #1: Type: text/plain, Size: 517 bytes --]

Hi David,

Here is the third cut of my MTD concat layer. The previous version had a bug 
in concat_erase() that I have been able to fix with the help of Jonas. It now 
works nice for him as well as for me.

Can this go into the CVS, please ?

Rob

----------------------------------------------------------------
Robert Kaiser                         email: rkaiser@sysgo.de
SYSGO RTS GmbH
Am Pfaffenstein 14                    phone: (49) 6136 9948-762
D-55270 Klein-Winternheim / Germany   fax:   (49) 6136 9948-10

[-- Attachment #2: patch-mtd-concat.gz --]
[-- Type: application/x-gzip, Size: 5898 bytes --]

[-- Attachment #3: patch-mtd-concat-part.gz --]
[-- Type: application/x-gzip, Size: 2949 bytes --]

Re: MTD concat layer

[David Woodhouse]

rob@sysgo.de said:
>  Here is the third cut of my MTD concat layer. The previous version
> had a bug  in concat_erase() that I have been able to fix with the
> help of Jonas. It now  works nice for him as well as for me.

> Can this go into the CVS, please ? 

Go for it.

--
dwmw2

RE: MTD concat layer

[Jonas Holmberg]

>> You could hack up a map driver that appears to have the two CFI chips 
>> contiguously, and then the probe code would find them both and make a 
>> single MTD device from them -  but there was a reason why he wasn't
doing 
>> this. Perhaps the 'BIOS' bit is in fact the beginning of the second
chip, 
>> not actually a separate chip at all? I don't remember.
>
>No, the BIOS is in the middle of the chip's addressing space. All this
>_could_ conceivably be solved with a hacked-up map driver, but then
>again,
>one might as well do the partitioning stuff in a hacked-up map driver.
>The
>idea was to have this functionality available in a generic form, so it
>it is accessible as a service from the MTD core layer.

I started to work on a different solution; probe functions that could be called several times with the same map. This would also make it possible to mix CFI and JEDEC chips. And you could mix chips of different geometry. I did , however, caught a nasty cold and haven't been working on it for a week now. If you are interested in this solution I will continue working on it when I get well, otherwise I'll use your solution.

/Jonas

MTD concat layer

[Robert Kaiser]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4813 bytes --]

Hi,

attached is my first version of the MTD "concatenation layer".
This basically allows to build virtual MTD devices by concatenating
existing ones, so it is -in a way- the opposite of partitioning.
I would like to add this to the MTD CVS and so I'm asking people
to have a look at it.

I see two possible uses for this:

1) Having flash filesystem volumes span multiple physical chips
   I have recently seen several people ask for this on the mailing list.

2) Allow re-ordering of blocks in flash chips.
   This is my personal reason for writing this code.  It requires
   some explanation:

   I have this cute DIL/NetPC module that -for technical reasons-
   has it's BIOS right in the middle of the 2MB flash. The mapping
   driver creates partitions reflecting this hardware-imposed map.
   However, this map is very unfortunate in many cases. Specifically,
   it would be nice if one could move the BIOS block out of the way
   and treat the flash portions below and above that BIOS block as
   one contiguous device that can then be partitioned arbitrarily.
   The concat layer allows just that: I can concatenate the physical
   partitions into a single device, specifying the sequence such
   that the BIOS block appears at the end and then re-partition
   that new (virtual) device as to my heart's desire :-)


Q: Why not do this in the mtdpart layer ?

David suggested at some point that this should be done in the
partitioning layer:

dwmw2 said:
> It doesn't need to be an extra layer. Splitting a single device into 
> multiple 'partitions' and building a big 'partition' from multiple devices 
> are just two cases of the same thing - munging the offsets and passing the 
> request off to a 'master' device. 

I tried to hack up the mtdpart.c accordingly, but came to the conclusion
that it is impossible to implement this without causing significant
overhead for parties that use only partitioning or only concatenating,
so it seemed cleaner to do it in a seperate layer. Moreover, the current
partitionining layer is scheduled to be replaced in the near future by
Jörn Engel's code, so I thought it best to stay clear of that layer of
code.

There are two patches attached to this mail:

1) patch-mtd-concat

This contains the actual concat layer. It also modifies the sc520cdp.c
mapping driver to demonstrate how use it by concatenating
the sc520cdp's two flash banks into one MTD device.

The concat code is in drivers/mtd/mtdconcat.c
This code defines two extenal functions:

-mtd_concat_create() to build a virtual MTD device by concatenating
 existing MTD devices.

-mtd_concat_destroy() to destroy an MTD object obtained from
 mtd_concat_create()

There is a header file (incluce/linux/mtd/concat.h) defining the
prototypes of these functions.

2) patch-mtd-concat-part

This patch modifies the DIL/NetPC mapping driver to re-order flash
blocks as described above. It also modifies the partitioning layer
(which is why I've made this a seperate patch): The current partitioning
layer has the habit of always registering the partitions as MTD devices
and not giving the caller access to the device objects it creates. I've
changed this (in a backward compatible way) because I do not want the
intermediate layers (i.e. the physical partitioning) to appear
as MTD devices. (I believe JÖrn's new partitioning code deals with this
too, though probably in a different way.)

Shortcomings I'm aware of so far (suggestions welcome):

* Currently, all sub-devices are expected to be of the same type.
  It might be reasonable to allow concatenating certain combinations
  of different types, where the super device would be assigned a
  "least common denominator" type (i.e. if combining ROMs with flashes,
  would yield a ROM, etc.).

* I'm not sure how to assign flags to the concatenated device. Right now,
  all but the writeablility flag have to be the same.

* Currently, all sub-devices are expected to have the same OOB and
  ECC info. I must admit that I don't have enough clue about these
  things to tell wether it might make sense to concatenate devices
  with differring OOB and/or ECC info. So by default, I'm not allowing it.

* The readv()/writev() functions are not implemented. The reason is that
  so far I could not conceive of a way to do this that would not result
  in terribly messy code. Also, it is my impression that these functions
  are not very important anyway (I might be wrong, here..).


Thanks for any suggestions

Rob

----------------------------------------------------------------
Robert Kaiser                          email: rkaiser@sysgo.de
SYSGO RTS GmbH
Am Pfaffenstein 14
D-55270 Klein-Winternheim / Germany    fax:   (49) 6136 9948-10

[-- Attachment #2: Type: APPLICATION/x-gunzip, Size: 4752 bytes --]

[-- Attachment #3: Type: APPLICATION/x-gunzip, Size: 3034 bytes --]

RE: MTD concat layer

[Joakim Tjernlund]

> Hi,
> 
> 
> I see two possible uses for this:
> 
> 1) Having flash filesystem volumes span multiple physical chips
>    I have recently seen several people ask for this on the mailing list.

I thougth this was already supported? Just create a mtd partition that will
span two or more banks and mount a JFFS2 FS on top of that partition.

 Jocke 

Re: MTD concat layer

[David Woodhouse]

joakim.tjernlund@lumentis.se said:
>  I thougth this was already supported? Just create a mtd partition
> that will span two or more banks and mount a JFFS2 FS on top of that
> partition. 

Doesn't work if the banks aren't made to appear contiguous by the map 
driver. The probe code can't handle it. 

The case in question has something like:

 --------------
 |  CFI chip  |
 --------------
 |  BIOS chip |
 --------------
 |  CFI chip  |
 --------------

You could hack up a map driver that appears to have the two CFI chips 
contiguously, and then the probe code would find them both and make a 
single MTD device from them -  but there was a reason why he wasn't doing 
this. Perhaps the 'BIOS' bit is in fact the beginning of the second chip, 
not actually a separate chip at all? I don't remember.

--
dwmw2

Re: MTD concat layer

[Robert Kaiser]

On Wed, 13 Feb 2002, David Woodhouse wrote:

> 
> joakim.tjernlund@lumentis.se said:
> >  I thougth this was already supported? Just create a mtd partition
> > that will span two or more banks and mount a JFFS2 FS on top of that
> > partition. 
> 
> Doesn't work if the banks aren't made to appear contiguous by the map 
> driver. The probe code can't handle it. 

Yes. My initial plan _was_ to hide that stuff in the mapping driver
but you (David) persuaded me to look for a more general solution.

BTW, the concat layer even allows to concatenate different kinds of
chips (as long as they are of the same type, e.g. Flash).

> 
> The case in question has something like:
> 
>  --------------
>  |  CFI chip  |
>  --------------
>  |  BIOS chip |
>  --------------
>  |  CFI chip  |
>  --------------
> 
> You could hack up a map driver that appears to have the two CFI chips 
> contiguously, and then the probe code would find them both and make a 
> single MTD device from them -  but there was a reason why he wasn't doing 
> this. Perhaps the 'BIOS' bit is in fact the beginning of the second chip, 
> not actually a separate chip at all? I don't remember.

No, the BIOS is in the middle of the chip's addressing space. All this
_could_ conceivably be solved with a hacked-up map driver, but then again,
one might as well do the partitioning stuff in a hacked-up map driver. The
idea was to have this functionality available in a generic form, so it
it is accessible as a service from the MTD core layer.

Rob

----------------------------------------------------------------
Robert Kaiser                          email: rkaiser@sysgo.de
SYSGO RTS GmbH
Am Pfaffenstein 14
D-55270 Klein-Winternheim / Germany    fax:   (49) 6136 9948-10

Re: MTD concat layer

[Robert Schwebel]

On Wed, 13 Feb 2002, David Woodhouse wrote:
> Perhaps the 'BIOS' bit is in fact the beginning of the second chip,
> not actually a separate chip at all? I don't remember.

The DNP has only one Intel 28F016F3 flash chip. Does anybody know
something about the reason why this strange layout for the BIOS was
chosen?

Robert
--
 +--------------------------------------------------------+
 | Dipl.-Ing. Robert Schwebel | http://www.pengutronix.de |
 | Pengutronix - Linux Solutions for Science and Industry |
 |   Braunschweiger Str. 79,  31134 Hildesheim, Germany   |
 |    Phone: +49-5121-28619-0 |  Fax: +49-5121-28619-4    |
 +--------------------------------------------------------+

Re: MTD concat layer

[Daniel Engström]

On 2002.02.13 12:37 Robert Schwebel wrote:
> The DNP has only one Intel 28F016F3 flash chip. Does anybody know
> something about the reason why this strange layout for the BIOS was
> chosen?

The realmode BIOS must reside at 0xf0000-0xfffff in system memory. 
Depending
on which options the chipset offers to map the flash, the BIOS may have to
be located in the middle of the flash device.

/Daniel

Re: MTD concat layer

[Eric W. Biederman]

Daniel Engström <daniel@omicron.se> writes:

> On 2002.02.13 12:37 Robert Schwebel wrote:
> > The DNP has only one Intel 28F016F3 flash chip. Does anybody know
> > something about the reason why this strange layout for the BIOS was
> > chosen?
> 
> The realmode BIOS must reside at 0xf0000-0xfffff in system
> memory. Depending
> on which options the chipset offers to map the flash, the BIOS may have to
> be located in the middle of the flash device.

Hmm.  Except the BIOS starts executing at 0xfffffff0 in system memory,
so ROMS chips usually reside at 0xfff00000 - 0xffffffff.  Usually an
alias exists at 0xf0000-0xfffff is handy but not required.  (I
don't use it when I port LinuxBIOS to a new board).  But usually
0xf0000-0xfffff on x86 is surrounded by RAM so this does not appear to
be the reason.

Eric

Re: MTD concat layer

[David Woodhouse]

rob@sysgo.de said:
>  attached is my first version of the MTD "concatenation layer". This
> basically allows to build virtual MTD devices by concatenating
> existing ones, so it is -in a way- the opposite of partitioning. I
> would like to add this to the MTD CVS and so I'm asking people to have
> a look at it. 

> +#if defined(CONFIG_MTD_CONCAT) || defined(CONFIG_MTD_CONCAT_MODULE) 

Don't ever do that. Modules should be, well, modular - don't make other 
stuff depend on whether you happened to build a particular module today or 
not.

It doesn't look like you have the concat_erase code right. An erase which 
covers more than one subdev will only get partially done.

I think the best way to do this would be to merge it with the existing
partition code. The question of how to actually _register_ these remains
open, but I think the core code ought to be something like this...

--- mtdpart.c	2001/11/27 14:55:11	1.25
+++ mtdpart.c	2002/02/15 15:02:51
@@ -21,13 +21,19 @@
 /* Our partition linked list */
 static LIST_HEAD(mtd_partitions);
 
+struct mtd_part_section {
+	struct mtd_info *master;
+	uint32_t logical_offset; /* In the 'partition' */
+	uint32_t phys_offset; /* In the master */
+	uint32_t len;
+};
+
 /* Our partition node structure */
 struct mtd_part {
 	struct mtd_info mtd;
-	struct mtd_info *master;
-	u_int32_t offset;
-	int index;
 	struct list_head list;
+	int nr_sects;
+	struct mtd_part_section sections[0];
 };
 
 /*
@@ -46,12 +52,41 @@ static int part_read (struct mtd_info *m
 			size_t *retlen, u_char *buf)
 {
 	struct mtd_part *part = PART(mtd);
+	size_t thisretlen;
+	size_t totretlen = 0;
+	int sect;
+	int ret = 0;
+
 	if (from >= mtd->size)
-		len = 0;
-	else if (from + len > mtd->size)
-		len = mtd->size - from;
-	return part->master->read (part->master, from + part->offset, 
-					len, retlen, buf);
+		return -EINVAL;
+
+	for (sect = 0; sect < part->nr_sects; sect_ofs += part->sections[sect].len, sect++) {
+		struct mtd_part_section *thissect = &part->sections[sect];
+		uint32_t thisofs, thislen;
+
+		if (from >= thissect->logical_offset + thissect->len)
+			continue;
+		
+		/* Offset within this section */
+		thisofs = from - thissect->logical_offset;
+		/* Trim length */
+		thislen = min(len, thissect->len - thisofs);
+		thisofs += thissect->phys_offset;
+
+		ret = part->master->read (part->master, from + part->offset, 
+					len, &thisretlen, buf);
+
+		if (ret)
+			goto out;
+
+		totretlen += thisretlen;
+		buf += thisretlen;
+		from += thisretlen;
+	}
+ out:
+	*retlen = totretlen;
+	return ret;
+
 }
 
 static int part_write (struct mtd_info *mtd, loff_t to, size_t len,
@@ -224,7 +259,6 @@ int add_mtd_partitions(struct mtd_info *
 		slave->mtd.erase = part_erase;
 		slave->master = master;
 		slave->offset = parts[i].offset;
-		slave->index = i;
 
 		if (slave->offset == MTDPART_OFS_APPEND)
 			slave->offset = cur_offset;


--
dwmw2

Re: MTD concat layer

[Robert Kaiser]

On Fri, 15 Feb 2002, David Woodhouse wrote:

> 
> > +#if defined(CONFIG_MTD_CONCAT) || defined(CONFIG_MTD_CONCAT_MODULE) 
> 
> Don't ever do that. Modules should be, well, modular - don't make other 
> stuff depend on whether you happened to build a particular module today or 
> not.
> 

OK, I'll change it.

> It doesn't look like you have the concat_erase code right. An erase which 
> covers more than one subdev will only get partially done.

OK, will look into this. I stole that part from Aleksander Sanochkin's
patch. I did test it, BTW (with erase/eraseall), and it worked.
Apparently, these tools erase the device block by block. Is it
legal/supported at all to make erase calls covering multiple blocks or
partial blocks ?

> 
> I think the best way to do this would be to merge it with the existing
> partition code.


Hmm, I agreed with you about this before I actually attempted coding it
this way, but the code turned out to be a mess. IMHO, it is _much_ cleaner
as a seperate layer. What would be the advantage of merging it into the
partition code ?

Disadvantages are:

- more overhead for parties requiring only partitioning or only
  concatenating.
- significant effort, specifically since the current partitioning
  code is scheduled for replacement by Jörn's code, so I'd have to
  do it twice.

Cheers

Rob

----------------------------------------------------------------
Robert Kaiser                          email: rkaiser@sysgo.de
SYSGO RTS GmbH
Am Pfaffenstein 14
D-55270 Klein-Winternheim / Germany    fax:   (49) 6136 9948-10

Re: MTD concat layer

[David Woodhouse]

rob@sysgo.de said:
>  OK, will look into this. I stole that part from Aleksander
> Sanochkin's patch. I did test it, BTW (with erase/eraseall), and it
> worked. Apparently, these tools erase the device block by block. Is it
> legal/supported at all to make erase calls covering multiple blocks or
> partial blocks ? 

Partial blocks is impossible, so not supported. Multiple blocks is 
permitted.

>  Hmm, I agreed with you about this before I actually attempted coding
> it this way, but the code turned out to be a mess. IMHO, it is _much_
> cleaner as a seperate layer. What would be the advantage of merging it
> into the partition code ?

Fewer wrapper layers to go through. Look at the part_read() call I 
implemented - is that really so bad for the case where people don't have 
multiple 'sections' in each partition?

> - significant effort, specifically since the current partitioning
>   code is scheduled for replacement by Jörn's code, so I'd have to
>   do it twice. 

Jörn's code needs to provide this functionality to be merged - making sure 
there's no regression is part of the work necessary for merging the new 
stuff, and wouldn't necessarily fall to you.

--
dwmw2

Re: MTD concat layer

[Jörn Engel]

Hi!

> >  Hmm, I agreed with you about this before I actually attempted coding
> > it this way, but the code turned out to be a mess. IMHO, it is _much_
> > cleaner as a seperate layer. What would be the advantage of merging it
> > into the partition code ?
> 
> Fewer wrapper layers to go through. Look at the part_read() call I 
> implemented - is that really so bad for the case where people don't have 
> multiple 'sections' in each partition?

Are fewer wrapper layers really an advantage? They don't improve
maintainability or correctness, nor do they give new features.
An additional wrapper only needs one additional function call, so
execution time should not degrade much. But it will actually improve,
whenever you don't need the additional layer.

> > - significant effort, specifically since the current partitioning
> >   code is scheduled for replacement by Jörn's code, so I'd have to
> >   do it twice. 
> 
> Jörn's code needs to provide this functionality to be merged - making sure 
> there's no regression is part of the work necessary for merging the new 
> stuff, and wouldn't necessarily fall to you.

Ack.
Apart from that, it is often quite a good idea to write the same code
twice. First implementations usually suck, especially mine.

Jörn

-- 
You can't tell where a program is going to spend its time. Bottlenecks
occur in surprising places, so don't try to second guess and put in a
speed hack until you've proven that's where the bottleneck is.
-- Rob Pike

Re: MTD concat layer

[Robert Kaiser]

On Fri, 15 Feb 2002, Jörn Engel wrote:

> dwmw2 wrote: 
> > Fewer wrapper layers to go through. Look at the part_read() call I 
> > implemented - is that really so bad for the case where people don't have 
> > multiple 'sections' in each partition?
> 
> Are fewer wrapper layers really an advantage? They don't improve
> maintainability or correctness, nor do they give new features.
> An additional wrapper only needs one additional function call, so
> execution time should not degrade much. But it will actually improve,
> whenever you don't need the additional layer.

I fully agree! Nevertheless I am considering to make another attempt to
code it according to David's suggestions. That would give us some
hard data for comparison. Problem is I can't afford to spend much
time on this, so it may take a while.

Could the current code go into CVS in the meantime as a provisional
solution ? (I keep seeing people ask for this functionality)

> Apart from that, it is often quite a good idea to write the same code
> twice. First implementations usually suck, especially mine.

:-)

Actually, for a "first implementation", I'm quite happy with the concat
layer (apart from the erase function bug that David spotted -- I'll
provide a fix for it next week). But then again, this isn't really the
first implementation, only the first one I dared to post :-)

Rob

----------------------------------------------------------------
Robert Kaiser                          email: rkaiser@sysgo.de
SYSGO RTS GmbH
Am Pfaffenstein 14
D-55270 Klein-Winternheim / Germany    fax:   (49) 6136 9948-10

Re: MTD concat layer

[Robert Kaiser]

On Fri, 15 Feb 2002, David Woodhouse wrote:

> Partial blocks is impossible, so not supported. Multiple blocks is 
> permitted.

OK, just to be clear about this: If my erase function is asked
to erase -say-  1.5 blocks, what should it do:

- erase one block and silently ignore the .5 part ?
- erase one block and return an error ?
- erase two blocks ?
- do nothing and return an error ?

Rob

----------------------------------------------------------------
Robert Kaiser                          email: rkaiser@sysgo.de
SYSGO RTS GmbH
Am Pfaffenstein 14
D-55270 Klein-Winternheim / Germany    fax:   (49) 6136 9948-10

Re: MTD concat layer

[David Woodhouse]

rob@sysgo.de said:
> OK, just to be clear about this: If my erase function is asked to
> erase -say-  1.5 blocks, what should it do: 

-EINVAL.

--
dwmw2

Re: MTD concat layer

[Robert Kaiser]

On Sat, 16 Feb 2002, David Woodhouse wrote:

> 
> rob@sysgo.de said:
> > OK, just to be clear about this: If my erase function is asked to
> > erase -say-  1.5 blocks, what should it do: 
> 
> -EINVAL.

OK, but is the function allowed to erase blocks up to the point
where it hits the partial block request ? That would make it simpler,
especially in the presence of variable block sizes.

Rob

----------------------------------------------------------------
Robert Kaiser                          email: rkaiser@sysgo.de
SYSGO RTS GmbH
Am Pfaffenstein 14
D-55270 Klein-Winternheim / Germany    fax:   (49) 6136 9948-10

Re: MTD concat layer

[David Woodhouse]

rob@sysgo.de said:
>  OK, but is the function allowed to erase blocks up to the point where
> it hits the partial block request ? That would make it simpler,
> especially in the presence of variable block sizes.

It doesn't matter. The caller _knows_ the blocksize setup, and must not
_ever_ ask for an erase that doesn't both start and end on block boundaries.

I hereby declare that the behaviour of the driver if this happens is
undefined. You can do what ever you like, including erasing the whole of 
the flash, all hard drives, and blowing up the machine.

I probably won't actually accept a patch which implements that. I'd take one
which has a BUG() though ;)

--
dwmw2

Re: MTD concat layer

[Brian J. Fox]

   From: Robert Kaiser <rob@sysgo.de>
   Date: Sat, 16 Feb 2002 12:03:27 +0100 (MET)

   On Sat, 16 Feb 2002, David Woodhouse wrote:

   > 
   > rob@sysgo.de said:
   > > OK, just to be clear about this: If my erase function is asked to
   > > erase -say-  1.5 blocks, what should it do: 
   > 
   > -EINVAL.

   OK, but is the function allowed to erase blocks up to the point
   where it hits the partial block request ? That would make it simpler,
   especially in the presence of variable block sizes.

I was lurking, but I guess I have to post an opinion on this.

*No*, you should *not* erase some blocks and return an error.

Brian
== The Difference Between Cultures: ==
    Einigkeit und Recht und Freiheit
    Liberte', E'galite', Fraternite'
    Sex, drugs and rock'n'roll

Re: MTD concat layer

[Robert Kaiser]

On Sat, 16 Feb 2002, Brian J. Fox wrote:

> 
>    From: Robert Kaiser <rob@sysgo.de>
>    Date: Sat, 16 Feb 2002 12:03:27 +0100 (MET)
> 
>    On Sat, 16 Feb 2002, David Woodhouse wrote:
> 
>    > 
>    > rob@sysgo.de said:
>    > > OK, just to be clear about this: If my erase function is asked to
>    > > erase -say-  1.5 blocks, what should it do: 
>    > 
>    > -EINVAL.
> 
>    OK, but is the function allowed to erase blocks up to the point
>    where it hits the partial block request ? That would make it simpler,
>    especially in the presence of variable block sizes.
> 
> *No*, you should *not* erase some blocks and return an error.
> 

Hmm, would you care to elaborate why ?

Rob

----------------------------------------------------------------
Robert Kaiser                          email: rkaiser@sysgo.de
SYSGO RTS GmbH
Am Pfaffenstein 14
D-55270 Klein-Winternheim / Germany    fax:   (49) 6136 9948-10

Re: MTD concat layer

[Brian J. Fox]

   Date: Sun, 17 Feb 2002 11:36:30 +0100 (MET)
   From: Robert Kaiser <rob@sysgo.de>
   X-Sender: rob@dagobert.svc.sysgo.de
   cc: dwmw2@infradead.org, linux-mtd@lists.infradead.org,
      joern@wohnheim.fh-wedel.de

   On Sat, 16 Feb 2002, Brian J. Fox wrote:

   > 
   >    From: Robert Kaiser <rob@sysgo.de>
   >    Date: Sat, 16 Feb 2002 12:03:27 +0100 (MET)
   > 
   >    On Sat, 16 Feb 2002, David Woodhouse wrote:
   > 
   >    > 
   >    > rob@sysgo.de said:
   >    > > OK, just to be clear about this: If my erase function is asked to
   >    > > erase -say-  1.5 blocks, what should it do: 
   >    > 
   >    > -EINVAL.
   > 
   >    OK, but is the function allowed to erase blocks up to the point
   >    where it hits the partial block request ? That would make it simpler,
   >    especially in the presence of variable block sizes.
   > 
   > *No*, you should *not* erase some blocks and return an error.
   > 

   Hmm, would you care to elaborate why ?

Just to be clear, the words that are important to me in my above
statement are the combination of "erase" and "and error" -- i.e., a
better printing of my above statement is:

   *No*, you should *not* erase some blocks *and* return an error.

For each function that you call (or write) in your program, there are
a myriad of possibilities for how bad inputs are handled.  Three of
these are common.  One may ignore bad inputs, and just keep chugging,
*perhaps* causing a crash.  One may massage the bad inputs into "good"
inputs, and pretend that the caller supplied those inputs.  Or, one
may refuse to operate utilizing the bad inputs.

Of the three options that I gave above, only one of them results in
predictable behavior -- the last one.

In order to achieve predictable behavior, and fewer bugs in the driver
overall, functions should simply refuse to operate on parameters that
fall out-of-bounds, and should return an error which specifies why
nothing happened.

If the erase function could only be called from within the driver code
proper (i.e., the only way bad inputs could be generated would be
purely from other bugs in the driver code), it would be acceptable to
*deliberately* crash at the point that the bad input was seen, so as
to speed the debugging of the driver code.

Low-level system software should never try to DWIM -- it is guaranteed
to fail.

Hab' ich meine Gedanken gut eklaren?

Brian
== The Difference Between Cultures: ==
    Einigkeit und Recht und Freiheit
    Liberte', E'galite', Fraternite'
    Sex, drugs and rock'n'roll

Re: MTD concat layer

[Robert Kaiser]

On Sun, 17 Feb 2002, Brian J. Fox wrote:

> 
>    Date: Sun, 17 Feb 2002 11:36:30 +0100 (MET)
>    From: Robert Kaiser <rob@sysgo.de>
>    X-Sender: rob@dagobert.svc.sysgo.de
>    cc: dwmw2@infradead.org, linux-mtd@lists.infradead.org,
>       joern@wohnheim.fh-wedel.de
> 
>    On Sat, 16 Feb 2002, Brian J. Fox wrote:
> 
>    > 
>    >    From: Robert Kaiser <rob@sysgo.de>
>    >    Date: Sat, 16 Feb 2002 12:03:27 +0100 (MET)
>    > 
>    >    On Sat, 16 Feb 2002, David Woodhouse wrote:
>    > 
>    >    > 
>    >    > rob@sysgo.de said:
>    >    > > OK, just to be clear about this: If my erase function is asked to
>    >    > > erase -say-  1.5 blocks, what should it do: 
>    >    > 
>    >    > -EINVAL.
>    > 
>    >    OK, but is the function allowed to erase blocks up to the point
>    >    where it hits the partial block request ? That would make it simpler,
>    >    especially in the presence of variable block sizes.
>    > 
>    > *No*, you should *not* erase some blocks and return an error.
>    > 
> 
>    Hmm, would you care to elaborate why ?
> 
> Just to be clear, the words that are important to me in my above
> statement are the combination of "erase" and "and error" -- i.e., a
> better printing of my above statement is:
> 
>    *No*, you should *not* erase some blocks *and* return an error.
> 
> For each function that you call (or write) in your program, there are
> a myriad of possibilities for how bad inputs are handled.  Three of
> these are common.  One may ignore bad inputs, and just keep chugging,
> *perhaps* causing a crash.

That would be unacceptable (of course), if the parameters to the function
are defined by userland programs.

>  One may massage the bad inputs into "good"
> inputs, and pretend that the caller supplied those inputs.

Also unacceptable for a device driver.

>  Or, one
> may refuse to operate utilizing the bad inputs.
> 
> Of the three options that I gave above, only one of them results in
> predictable behavior -- the last one.

Fully agreed.

> 
> In order to achieve predictable behavior, and fewer bugs in the driver
> overall, functions should simply refuse to operate on parameters that
> fall out-of-bounds, and should return an error which specifies why
> nothing happened.

Now, here comes the problem. In the context of an erase function,
what does "predictable behavior" mean ? If it returns EINVAL, the caller
can see that they passed a faulty parameter, so does it make any
difference wether some of the specified flash range have been erased in
the process ? By specifying a certain area of flash to be erased, the user
has by definition given up any hope of seeing the data in that area again.

I think one can compare this to a read() function: if read() returns
an error, would you expect that the buffer that you passed to it
still contains all of its previous data ?


> 
> If the erase function could only be called from within the driver code
> proper (i.e., the only way bad inputs could be generated would be
> purely from other bugs in the driver code), it would be acceptable to
> *deliberately* crash at the point that the bad input was seen, so as
> to speed the debugging of the driver code.
> 
> Low-level system software should never try to DWIM -- it is guaranteed
> to fail.

If, by DWIM, you mean something like "do what is meant", I couldn't agree
more.

> 
> Hab' ich meine Gedanken gut eklaren?
> 

Ja. Ich meine auch ?

Cheers

Rob

----------------------------------------------------------------
Robert Kaiser                          email: rkaiser@sysgo.de
SYSGO RTS GmbH
Am Pfaffenstein 14
D-55270 Klein-Winternheim / Germany    fax:   (49) 6136 9948-10

Re: MTD concat layer

[David Woodhouse]

rob@sysgo.de said:
>  Now, here comes the problem. In the context of an erase function,
> what does "predictable behavior" mean ? If it returns EINVAL, the
> caller can see that they passed a faulty parameter, so does it make
> any difference wether some of the specified flash range have been
> erased in the process ? By specifying a certain area of flash to be
> erased, the user has by definition given up any hope of seeing the
> data in that area again.

There is never any excuse for passing invalid offset/length to the erase
function - even the ioctl code can do a sanity check before passing through
the values provided by the user.

For errors which can understandably occur, your concerns are valid and we 
should ensure sanity by failing the request as safely as possible, doing 
nothing else.

This is not such an error. As long as the ioctl() has an appropriate sanity 
check, the occurrence of such an error indicates that the kernel code is 
completely broken. Better to BUG() than return an error, in that case - 
that way, there's no chance that the broken code will try again, and happen 
to pick a range which _does_ look valid, but which it still shouldn't be 
erasing.


--
dwmw2

Re: MTD concat layer

[Brian J. Fox]

   From: David Woodhouse <dwmw2@infradead.org>
   Cc: "Brian J. Fox" <bfox@ua.com>, linux-mtd@lists.infradead.org,
       J?rn Engel <joern@wohnheim.fh-wedel.de>
   Date: Mon, 18 Feb 2002 09:05:55 +0000
   Sender: David Woodhouse <dwmw2@redhat.com>

   There is never any excuse for passing invalid offset/length to the
   erase function - even the ioctl code can do a sanity check before
   passing through the values provided by the user.

That's right.

   For errors which can understandably occur, your concerns are valid
   and we should ensure sanity by failing the request as safely as
   possible, doing nothing else.

   This is not such an error. As long as the ioctl() has an
   appropriate sanity check, the occurrence of such an error indicates
   that the kernel code is completely broken. Better to BUG()

BUG() is completely acceptable.  It allows the driver to be fixed.
It's also orthogonal to the original question.  Erasing blocks is
simply a way to guarantee that data has been irrevocably lost.

Brian
== The Difference Between Cultures: ==
    Einigkeit und Recht und Freiheit
    Liberte', E'galite', Fraternite'
    Sex, drugs and rock'n'roll

Re: MTD concat layer

[Robert Kaiser]

On Mon, 18 Feb 2002, Brian J. Fox wrote:
> 
> BUG() is completely acceptable.  It allows the driver to be fixed.
> It's also orthogonal to the original question.  Erasing blocks is
> simply a way to guarantee that data has been irrevocably lost.

OK, so we all finally agree that BUG() is the way to go (rather than
returning -EINVAL. Right ?

Rob

----------------------------------------------------------------
Robert Kaiser                          email: rkaiser@sysgo.de
SYSGO RTS GmbH
Am Pfaffenstein 14
D-55270 Klein-Winternheim / Germany    fax:   (49) 6136 9948-10

Re: MTD concat layer

[David Woodhouse]

rob@sysgo.de said:
>  OK, so we all finally agree that BUG() is the way to go (rather than
> returning -EINVAL. Right ?

As long as you make sure you can't trigger the BUG() from the mtdchar 
ioctl() routine, yes.



--
dwmw2

Re: MTD concat layer

[Brian J. Fox]

   Date: Mon, 18 Feb 2002 09:48:51 +0100 (MET)
   From: Robert Kaiser <rob@sysgo.de>

   > In order to achieve predictable behavior, and fewer bugs in the driver
   > overall, functions should simply refuse to operate on parameters that
   > fall out-of-bounds, and should return an error which specifies why
   > nothing happened.

   Now, here comes the problem. In the context of an erase function,
   what does "predictable behavior" mean ? If it returns EINVAL, the caller
   can see that they passed a faulty parameter, so does it make any
   difference wether some of the specified flash range have been erased in
   the process?

Sure.  Erasing the blocks *changes* the state of the world from a
predictable one to an unpredictable one.

   By specifying a certain area of flash to be erased, the user has by
   definition given up any hope of seeing the data in that area again.

If the arguments are invalid, the code that calculated them is
probably invalid.  Erasing the blocks is simply mean, and irreversible.

   I think one can compare this to a read() function: if read() returns
   an error, would you expect that the buffer that you passed to it
   still contains all of its previous data ?

It wouldn't matter if the buffer contained partial data, or no data,
or the previous data.  You haven't done something that is user visibly
irreversible!

   > Hab' ich meine Gedanken gut eklaren?
   > 

   Ja. Ich meine auch ?

Ja, aber "read" ist nicht "delete"!

MBD,

Brian
== The Difference Between Cultures: ==
    Einigkeit und Recht und Freiheit
    Liberte', E'galite', Fraternite'
    Sex, drugs and rock'n'roll

Re: MTD concat layer

[Jonas Holmberg]

On Tue, 2002-02-12 at 19:40, Robert Kaiser wrote:
> Hi,
> 
> attached is my first version of the MTD "concatenation layer".
> This basically allows to build virtual MTD devices by concatenating
> existing ones, so it is -in a way- the opposite of partitioning.
> I would like to add this to the MTD CVS and so I'm asking people
> to have a look at it.

Hi Robert,
I'm ready to do some serious testing now. Should I use these patches or
do you have newer versions? When can we expect to see this in CVS?

/Jonas

Re: MTD concat layer

[Robert Kaiser]

Hi Jonas,

On 20 Feb 2002, Jonas Holmberg wrote:

> I'm ready to do some serious testing now.

Good!

> Should I use these patches or
> do you have newer versions?

The erase function has a bug for which I'm currently developing
a fix. I hope to get it done by tomorrow. The bug does not show
up if you use erase/eraseall, but will probably manifest itself
when using JFFS/JFFS2 on a concatenated device.

> When can we expect to see this in CVS?

Good question :-). Apparently David is not yet completely convinced
that it should be implemented the way I did. If you do some serious
testing, your test results might help to persuade him (or prove to him
that his scepticism was right). In any case, please share your
results.

Cheers

Rob
----------------------------------------------------------------
Robert Kaiser                          email: rkaiser@sysgo.de
SYSGO RTS GmbH
Am Pfaffenstein 14
D-55270 Klein-Winternheim / Germany    fax:   (49) 6136 9948-10

Re: MTD concat layer

[Jonas Holmberg]

On Wed, 2002-02-20 at 16:35, Robert Kaiser wrote:
> Hi Jonas,
> 
> On 20 Feb 2002, Jonas Holmberg wrote:
> 
> > I'm ready to do some serious testing now.
> 
> Good!
> 
> > Should I use these patches or
> > do you have newer versions?
> 
> The erase function has a bug for which I'm currently developing
> a fix. I hope to get it done by tomorrow. The bug does not show
> up if you use erase/eraseall, but will probably manifest itself
> when using JFFS/JFFS2 on a concatenated device.

Yep, I get the following when erasing across chip boundry:

JFFS: jffs_write_node: Failed to write, requested 8260, wrote 1
Last[3] is c6a6, datum is 3931
Didn't write all bytes in flash_safe_writev(). Returned -5
JFFS: jffs_write_node: Failed to write, requested 8260, wrote 1
Last[3] is c6a6, datum is 3931
Didn't write all bytes in flash_safe_writev(). Returned -5
JFFS: jffs_write_node: Failed to write, requested 8260, wrote 1
Last[3] is c6a6, datum is 3931
Didn't write all bytes in flash_safe_writev(). Returned -5
JFFS: jffs_write_node: Failed to write, requested 8260, wrote 1
Last[3] is c6a6, datum is 3931

Other than that it seems fine. I have tried mixing CFI and JEDEC chips
as well. I'm awaiting your erase-fix.

It seems like JFFS uses writev. Are you planning to implement the
v-functions?

> 
> > When can we expect to see this in CVS?
> 
> Good question :-). Apparently David is not yet completely convinced
> that it should be implemented the way I did. If you do some serious
> testing, your test results might help to persuade him (or prove to him
> that his scepticism was right). In any case, please share your
> results.

Is it much work implementing it his way instead? I have no opinion of
which is better. Your concat layer looks very clean, but David use to be
right :-)

/Jonas

Re: MTD concat layer

[Robert Kaiser]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2518 bytes --]

Hi,

here is my second attempt of an MTD concat layer. The issues that
David found with the first one have (hopefully) been fixed. I have also
made some benchmarks comparing MTD performance on the platforms I have
at my disposal (DIL/NetPC and SC520CDP) with vs. without concat layer.

Judging by these benchmarks it seems to me that the performance impact of
the concat layer is almost negligible.

On 21 Feb 2002, Jonas Holmberg wrote:

> On Wed, 2002-02-20 at 16:35, Robert Kaiser wrote:
> > The erase function has a bug for which I'm currently developing
> > a fix. I hope to get it done by tomorrow.
> I'm awaiting your erase-fix.

As usual, it took a little longer than expected, but finally here it is :)

> It seems like JFFS uses writev. Are you planning to implement the
> v-functions?

Yes, Both jffs and jffs2 use it, but both also have a substitute routine
in case the driver doesn't provide one. Of course I could hack up
what would basically be a copy of mtd_fake_writev() from jffs2/wbuf.c
and add that to mtdconcat.c, but I think that would be wrong (see
the comment in jffs2/wbuf.c, BTW).

> 
> > 
> > > When can we expect to see this in CVS?
> > 
> > Good question :-). Apparently David is not yet completely convinced
> > that it should be implemented the way I did. If you do some serious
> > testing, your test results might help to persuade him (or prove to him
> > that his scepticism was right). In any case, please share your
> > results.
> 
> Is it much work implementing it his way instead?

The simple cases like read/write, etc. will probably not be hard
to do in the partition layer. However, if you look at the new concat_erase
function, I do not think it has much in common with the one in
in mtdpart.c. Also the actual merging of MTD devices (i.e. generating
a singe device object that reflects the properties of multiple underlying
device objects) seems fundamentally different to me from what the
partitioning layer does, but maybe I'm already so focused in my idea
that I can't see the alternatives...

> Your concat layer looks very clean, but David use to be
> right :-)

I believe David's biggest objection was the potential loss of performance
due to the stacking of layers. I hope the benchmarks I made show that this
is not a problem.

Cheers

Rob

----------------------------------------------------------------
Robert Kaiser                          email: rkaiser@sysgo.de
SYSGO RTS GmbH
Am Pfaffenstein 14
D-55270 Klein-Winternheim / Germany    fax:   (49) 6136 9948-10

[-- Attachment #2: New concat layer --]
[-- Type: APPLICATION/x-gunzip, Size: 5759 bytes --]

[-- Attachment #3: Additional concat stuf w/ slight modification to partitioni --]
[-- Type: APPLICATION/x-gunzip, Size: 2949 bytes --]

[-- Attachment #4: concat benchmark results --]
[-- Type: TEXT/plain, Size: 1230 bytes --]

Platform                             DIL/NetPC with concat layer DIL/NetPC without concat layer  AMD SC520CDP with concat layer AMD SC520CDP without concat layer
Layers traversed                     3 (2 x part. 1 x concat)    1                               2 (1 x concat, 1 x part.)      0
eraseall /dev/mtd/xx                 45.7                        45.8                            28.7                           28.2
dd if=/dev/mtd/xx of=/dev/null       1.6                         1.6                             1.3                            1.3
dd if=/dev/zero of=/dev/mtd/xx       187.5                       187.3                           66.7                           65.7
mount -t jffs2 /dev/mtdblock/xx /tmp 1.3                         1.3                             1.3                            1.3
cd /tmp; tar xvf /test.tar           227.1                       222.3                           44.6                           44.5
cd /tmp; tar cvf /test1.tar .        30.4                        30.9                            8.4                            8.5
cd /tmp; rm -rf *                    2.8                         2.7                             0.6                            0.5

Re: MTD concat layer

[Jonas Holmberg]

On Tue, 2002-02-26 at 12:32, Robert Kaiser wrote:
> Hi,
> 
> here is my second attempt of an MTD concat layer. The issues that
> David found with the first one have (hopefully) been fixed. I have also
> made some benchmarks comparing MTD performance on the platforms I have
> at my disposal (DIL/NetPC and SC520CDP) with vs. without concat layer.

I get an OOPS when erasing (jffs gc). I'm not 100% sure that it's
something wrong with mtdconcat.c but the same thing works if I use the
amd_flash driver instead of cfi_cmdset_0002 + mtdconcat. It looks lika a
busfault in concat_erase. I have disassembled vmlinux and it is
"subdev->size" on line 663 in mtdconcat.c that gives the busfault, maybe
because the variable i has been assigned a bad value before entering the
for loop?

(The ksymoops warning is because my target system is a cris and I run
ksymoops on an i686))

/Jonas


ksymoops 2.4.1 on i686 2.4.9-13.  Options used
     -v os/linux/vmlinux (specified)
     -K (specified)
     -L (specified)
     -O (specified)
     -m os/linux/System.map (specified)

Reading Oops report from the terminal
Oops: 0000
IRP: c006996c SRP: c00697ec DCCR: 000004a0 USP: affff884 MOF: 00000000
 r0: c0f3b008  r1: 00000002   r2: c0f3b004  r3: 00050000
 r4: 00000000  r5: c0f3b000   r6: c01d82a0  r7: c0f3a5c0
 r8: 000f0000  r9: 00200000  r10: ffffffff r11: c0f89ec0
r12: 001b0000 r13: c0f89ecc oR10: ffffffff
R_MMU_CAUSE: 00001001
Process jffs_gcd (pid: 17, stackpage=c0f88000)

Stack from affff884:
       0c0b8fae 3aabf7c8 3aabf92a affff920 3aabf7d8 3aad1d51 00000000
00000000
       3aab8c3a affff884 affffa84 3aab5348 00000000 affff8a4 00000001
affffaa0
       affff9d0 00000000 3aabf7d8 00000000 00000000 3aad30b1 3aac96b4
3aabf7d8
Call Trace:
Stack from c0f89d80:
       c0006ac0 c0f89ec0 c0053226 c005338a c0f89ec0 00000000 00000000
00000000
       c00c4b6c 00000000 c0f89e7c c005344e c0f89e7c c018a000 c0006ac0
c00554f8
       000f0000 c0f3a5c0 c01d82a0 c0f3b000 00000000 00050000 c0f89e7c
c018a000
Call Trace: [<c0006ac0>] [<c0053226>] [<c005338a>] [<c005344e>]
[<c0006ac0>] [<c00554f8>] [<c00b2c48>]
       [<c0069746>] [<c0066706>] [<c00552f4>] [<c0053046>] [<c00697ec>]
[<c006996c>] [<da6d0012>] [<c006a26e>]
       [<c004dde6>] [<c005002a>] [<c00502b8>] [<c0052ee4>] [<c005015e>]
[<c005043e>] [<c0053186>] [<c0053190>]
Code: 69 aa 62 ca 6c 96 23 96 05 a1 6d da (ed) 96 06 80 ac d6 04 e0 e0
db e0 3b Oops: 0000
 r0: c0f3b008  r1: 00000002   r2: c0f3b004  r3: 00050000
 r4: 00000000  r5: c0f3b000   r6: c01d82a0  r7: c0f3a5c0
 r8: 000f0000  r9: 00200000  r10: ffffffff r11: c0f89ec0
r12: 001b0000 r13: c0f89ecc oR10: ffffffff
Warning (Oops_set_i370_regs): garbage 'oR10: ffffffff' at end of i370
register line ignored
Process jffs_gcd (pid: 17, stackpage=c0f88000)
Stack from affff884:
       0c0b8fae 3aabf7c8 3aabf92a affff920 3aabf7d8 3aad1d51 00000000
00000000
       3aab8c3a affff884 affffa84 3aab5348 00000000 affff8a4 00000001
affffaa0
       affff9d0 00000000 3aabf7d8 00000000 00000000 3aad30b1 3aac96b4
3aabf7d8
Stack from c0f89d80:
       c0006ac0 c0f89ec0 c0053226 c005338a c0f89ec0 00000000 00000000
00000000
       c00c4b6c 00000000 c0f89e7c c005344e c0f89e7c c018a000 c0006ac0
c00554f8
       000f0000 c0f3a5c0 c01d82a0 c0f3b000 00000000 00050000 c0f89e7c
c018a000
Call Trace: [<c0006ac0>] [<c0053226>] [<c005338a>] [<c005344e>]
[<c0006ac0>] [<c00554f8>] [<c00b2c48>]
       [<c0069746>] [<c0066706>] [<c00552f4>] [<c0053046>] [<c00697ec>]
[<c006996c>] [<da6d0012>] [<c006a26e>]
       [<c004dde6>] [<c005002a>] [<c00502b8>] [<c0052ee4>] [<c005015e>]
[<c005043e>] [<c0053186>] [<c0053190>]
Code: 69 aa 62 ca 6c 96 23 96 05 a1 6d da (ed) 96 06 80 ac d6 04 e0 e0
db e0 3b Using defaults from ksymoops -t elf32-i386 -a i386

>>r0 ; c0f3b008 <_end+e126c8/ed76c0>
>>r2 ; c0f3b004 <_end+e126c4/ed76c0>
>>r3 ; 00050000 Before first symbol
>>r5 ; c0f3b000 <_end+e126c0/ed76c0>
>>r6 ; c01d82a0 <_end+af960/ed76c0>
>>r7 ; c0f3a5c0 <_end+e11c80/ed76c0>
>>r8 ; 000f0000 Before first symbol
>>r9 ; 00200000 Before first symbol
>>r10; ffffffff <END_OF_CODE+3effffff/????>
>>r11; c0f89ec0 <_end+e61580/ed76c0>
>>r12; 001b0000 Before first symbol
>>r13; c0f89ecc <_end+e6158c/ed76c0>
Trace; c0006ac0 <printk+0/14c>
Trace; c0053226 <show_stack+0/90>
Trace; c005338a <show_registers+d4/146>
Trace; c005344e <die_if_kernel+34/46>
Trace; c0006ac0 <printk+0/14c>
Trace; c00554f8 <do_page_fault+200/2a8>
Trace; c00b2c48 <__Umod+0/147c>
Trace; c0069746 <concat_erase_callback+12/14>
Trace; c0066706 <cfi_amdstd_erase_varsize+c02/c0c>
Trace; c00552f4 <handle_mmu_bus_fault+b4/b8>
Trace; c0053046 <mmu_bus_fault+28/30>
Trace; c00697ec <concat_dev_erase+a4/bc>
Trace; c006996c <concat_erase+168/1d0>
Trace; da6d0012 <END_OF_CODE+196d0012/????>
Trace; c006a26e <part_erase+36/38>
Trace; c004dde6 <flash_erase_region+9e/ee>
Trace; c005002a <jffs_try_to_erase+66/90>
Trace; c00502b8 <jffs_garbage_collect_thread+15a/1d2>
Trace; c0052ee4 <_reschedule+8/c>
Trace; c005015e <jffs_garbage_collect_thread+0/1d2>
Trace; c005043e <jffs_read_super+10e/142>
Trace; c0053186 <kernel_thread+12/28>
Trace; c0053190 <kernel_thread+1c/28>
Code;  fffffff4 <END_OF_CODE+3efffff4/????>
00000000 <_EIP>:
Code;  fffffff4 <END_OF_CODE+3efffff4/????>
   0:   69 aa 62 ca 6c 96 23      imul  
$0xa1059623,0x966cca62(%edx),%ebp
Code;  fffffffb <END_OF_CODE+3efffffb/????>
   7:   96 05 a1
Code;  fffffffe <END_OF_CODE+3efffffe/????>
   a:   6d                        insl   (%dx),%es:(%edi)
Code;  ffffffff <END_OF_CODE+3effffff/????>
   b:   da ed                     (bad)
Code;  00000001 Before first symbol
   d:   96                        xchg   %eax,%esi
Code;  00000002 Before first symbol
   e:   06                        push   %es
Code;  00000003 Before first symbol
   f:   80 ac d6 04 e0 e0 db      subb   $0xe0,0xdbe0e004(%esi,%edx,8)
Code;  0000000a Before first symbol
  16:   e0
Code;  0000000b Before first symbol
  17:   3b 00                     cmp    (%eax),%eax


1 warning issued.  Results may not be reliable.

Re: MTD concat layer

[Robert Kaiser]

Hi Jonas,

Am Mittwoch, 6. März 2002 14:37 schrieben Sie:
> I get an OOPS when erasing (jffs gc). I'm not 100% sure that it's
> something wrong with mtdconcat.c but the same thing works if I use the
> amd_flash driver instead of cfi_cmdset_0002 + mtdconcat. It looks lika a
> busfault in concat_erase. I have disassembled vmlinux and it is
> "subdev->size" on line 663 in mtdconcat.c that gives the busfault, maybe
> because the variable i has been assigned a bad value before entering the
> for loop?
>

Hmm, line 663 is the very last line in mtdconcat.c and it doesn't contain
a reference to subdev->size. Could it be line 279 instead ?

I guess I need to set up an environment here and try to reproduce the problem.
Obviously I do not have access to a cris board, so I'll try with my SC520CDP
instead (I have used it for testing before, but I have so far tested with 
JFFS2 only).

In the meantime, could you verify that the bad pointer is really due to i
going beyond bounds, i.e. add something like:

    if(i >= concat->num_subdev)
        BUG();

before line 276. (actually you can copy-paste it from a few lines above).


Cheers

Rob

----------------------------------------------------------------
Robert Kaiser                         email: rkaiser@sysgo.de
SYSGO RTS GmbH
Am Pfaffenstein 14                    phone: (49) 6136 9948-762
D-55270 Klein-Winternheim / Germany   fax:   (49) 6136 9948-10