hardcoded maximum number of CFI chips - continued

hardcoded maximum number of CFI chips - continued

[andrzej.mialkowski]

Due to e-mail configuration problems I'm not able to continue on previous 
thread.

Jörn,
  You of course right that limit proposed in patch too high. With current 
typical stack size (8K) and structure size (36 bytes - on ARM) it is safe to 
handle this way about 160 chips. This is much more what can be expected in 
resonable designs (and what I need). On the other hand placing kmalloc for 
chips structure effectively leeds to solution with realloc. Drawback of this 
will be creation of heap fragment, it also still has problem with the constant 
limit.
For me attractive are two solutions:
1) Cleaned previous proposal, becuse it is simple and solves problem probably 
fits for all designs.
2) Rewrite genprobe_ident_chips to use realloc emulation to allow expansion 
structure and handling any number of chips.

Andrzej

diff -Nru --minimal prev/drivers/mtd/chips/Config.in 
linux/drivers/mtd/chips/Config.in
--- prev/drivers/mtd/chips/Config.in	Fri Mar 14 17:21:15 2003
+++ linux/drivers/mtd/chips/Config.in	Mon Jul 21 14:23:49 2003
@@ -40,6 +40,7 @@
        bool '    Support 4-chip flash interleave' CONFIG_MTD_CFI_I4
        bool '    Support 8-chip flash interleave' CONFIG_MTD_CFI_I8
      fi
+     int '  Maximum number of flash chips' CONFIG_MTD_CFI_MAX_CHIPS 8 1 32
   fi
 fi
 dep_tristate '  Support for Intel/Sharp flash chips' CONFIG_MTD_CFI_INTELEXT 
$CONFIG_MTD_GEN_PROBE
diff -Nru --minimal prev/include/linux/mtd/cfi.h linux/include/linux/mtd/cfi.h
--- prev/include/linux/mtd/cfi.h	Fri Mar 14 17:05:39 2003
+++ linux/include/linux/mtd/cfi.h	Mon Jul 21 14:22:38 2003
@@ -306,7 +306,11 @@
 	struct flchip chips[0];  /* per-chip data structure for each chip */
 };
 
+#ifndef CONFIG_MTD_CFI_MAX_CHIPS
 #define MAX_CFI_CHIPS 8 /* Entirely arbitrary to avoid realloc() */
+#else
+#define MAX_CFI_CHIPS CONFIG_MTD_CFI_MAX_CHIPS
+#endif
 
 /*
  * Returns the command address according to the given geometry. 

Re: hardcoded maximum number of CFI chips - continued

[Jörn Engel]

On Mon, 21 July 2003 15:04:51 +0200, andrzej.mialkowski@inetia.pl wrote:
> 
> Jörn,
>   You of course right that limit proposed in patch too high. With current 
> typical stack size (8K) and structure size (36 bytes - on ARM) it is safe to 
> handle this way about 160 chips. This is much more what can be expected in 
> resonable designs (and what I need). On the other hand placing kmalloc for 
> chips structure effectively leeds to solution with realloc. Drawback of this 
> will be creation of heap fragment, it also still has problem with the constant 
> limit.

Heap fragmentation should not be a problem in kernel space, but you
have to copy the array around each time you "realloc".

> For me attractive are two solutions:
> 1) Cleaned previous proposal, becuse it is simple and solves problem probably 
> fits for all designs.
> 2) Rewrite genprobe_ident_chips to use realloc emulation to allow expansion 
> structure and handling any number of chips.
3) Rewrite genprobe_ident_chips to use a linked list with struct
list_head.

Usually 3) is a little awkward at first, but when you are done, the
code looks a lot nicer than before.  I'm not sure if it would work
here as well.  It should, but you never know before you actually try
it.

> diff -Nru --minimal prev/drivers/mtd/chips/Config.in 
> linux/drivers/mtd/chips/Config.in
> --- prev/drivers/mtd/chips/Config.in	Fri Mar 14 17:21:15 2003
> +++ linux/drivers/mtd/chips/Config.in	Mon Jul 21 14:23:49 2003
> @@ -40,6 +40,7 @@
>         bool '    Support 4-chip flash interleave' CONFIG_MTD_CFI_I4
>         bool '    Support 8-chip flash interleave' CONFIG_MTD_CFI_I8
>       fi
> +     int '  Maximum number of flash chips' CONFIG_MTD_CFI_MAX_CHIPS 8 1 32
>    fi
>  fi

This should with 4k kernel stack, but it is still an ugly solution,
imo.  The complexety for the problem is all over the place, Makefile
Kconfig, header, source file.  If I only had more time to do a proper
fix myself...

Jörn

-- 
The cost of changing business rules is much more expensive for software
than for a secretaty.
-- unknown

Re: hardcoded maximum number of CFI chips - continued

[andrzej.mialkowski]

Yes, linked list is nice solution; I would also prefer it while writing a new 
module. 
Problem is that module exist and works pretty well. My priorities are time to 
solution and risk of bug avoidance. You must agree that this is big difference 
between changing number 8 to 10 and rewriting even only module initialization 
to linked lists. 
Compromise solution may be leave runtime structures as they are, and change 
chips table to be 'reallocated' for instance in 4-16 chip increments. This 
solution reduces to reasonable amount of copying and memory allocations (still 
not critical during initialization), supports 'unlimited' number of chips and 
may be implemented and tested in period of few hours. I can do this tomorrow.

Andrzej

Re: hardcoded maximum number of CFI chips - continued

[Jörn Engel]

On Mon, 21 July 2003 23:34:05 +0200, andrzej.mialkowski@inetia.pl wrote:
> 
> Yes, linked list is nice solution; I would also prefer it while writing a new 
> module. 
> Problem is that module exist and works pretty well. My priorities are time to 
> solution and risk of bug avoidance. You must agree that this is big difference 
> between changing number 8 to 10 and rewriting even only module initialization 
> to linked lists. 
> Compromise solution may be leave runtime structures as they are, and change 
> chips table to be 'reallocated' for instance in 4-16 chip increments. This 
> solution reduces to reasonable amount of copying and memory allocations (still 
> not critical during initialization), supports 'unlimited' number of chips and 
> may be implemented and tested in period of few hours. I can do this tomorrow.

In that case, why don't you just crank up the number from 8 to 16?  It
is the simpler solution to your problem at hand.

Jörn

-- 
Correctness comes second.
Features come third.
Performance comes last.
Maintainability is needed for all of them.

Re: hardcoded maximum number of CFI chips - continued

[andrzej.mialkowski]

Cytowanie Jörn Engel <joern@wohnheim.fh-wedel.de>:
> In that case, why don't you just crank up the number from 8 to 16?  It
> is the simpler solution to your problem at hand.
> 
> Jörn

Jörn,

Answer is very simple: It is not first my Linux port. In period last two years 
i had to port Linux to about 9 different ARM based systems. In all cases I 
meet several hardcoded limitations. This means so it is also my personal 
business to make Linux easier to port. I'm trying to balace between complexity 
of change an effect. As far as I know friedns from my hadware team, next time 
they will mount me 27.5 chips :).

I looked once again on generic probe code and found following conclusion:
Chips table is used only for alias detection and for building runtime retcfi 
structure. In both cases only information used is chip address. This means 
that table of flchip structures may be replaced by any data structure that is 
able to return this information. Thought about lists, tables and finally 
decided to use bitmap. Bitmap of valid chips can be used effectively if 
assumption that number of possible chips (locations to check!) in map is 
reasonable in comparison to number of chips. Algorithm is not effective only 
if map driver reports millions of unoccupied possible chip locations (if we 
have a lot of chips we must have much more memory for runtime structures).
In my case map driver detects number of writable bits in banking hardware so 
in result of calculations I have 16 possible chip locations.

Below is patch for my work tree 2.4.19-rmk7-ds3, tried to do not affect 
algorithms and large parts of code. Not tested JEDEC part but changes are 
almost identical like for CFI.

Andrzej

diff -Nru --minimal --ignore-all-space prev/drivers/mtd/chips/cfi_probe.c 
linux/drivers/mtd/chips/cfi_probe.c
--- prev/drivers/mtd/chips/cfi_probe.c	Tue Apr 29 13:57:43 2003
+++ linux/drivers/mtd/chips/cfi_probe.c	Thu Jul 24 14:45:06 2003
@@ -25,7 +25,7 @@
 #endif
 
 static int cfi_probe_chip(struct map_info *map, __u32 base,
-			  struct flchip *chips, struct cfi_private *cfi);
+			  struct probe_info *probe, struct cfi_private *cfi);
 static int cfi_chip_setup(struct map_info *map, struct cfi_private *cfi);
 
 struct mtd_info *cfi_probe(struct map_info *map);
@@ -48,7 +48,7 @@
 }
 
 static int cfi_probe_chip(struct map_info *map, __u32 base,
-			  struct flchip *chips, struct cfi_private *cfi)
+			  struct probe_info *probe, struct cfi_private *cfi)
 {
 	int i;
 	
@@ -80,21 +80,26 @@
 		return cfi_chip_setup(map, cfi);
 	}
 
-	/* Check each previous chip to see if it's an alias */
-	for (i=0; i<cfi->numchips; i++) {
+	/* Check each previous chip locations to see if it's an alias */
+	for (i=0; i < (base >> cfi->chipshift); i++) {
+		unsigned long start;
+		if(!test_bit(i, probe->chip_map)) {
+			continue; /* Skip location; no valid chip at this 
address */
+		}
+		start = i << cfi->chipshift;
 		/* This chip should be in read mode if it's one
 		   we've already touched. */
-		if (qry_present(map,chips[i].start,cfi)) {
+		if (qry_present(map, start, cfi)) {
 			/* Eep. This chip also had the QRY marker. 
 			 * Is it an alias for the new one? */
-			cfi_send_gen_cmd(0xF0, 0, chips[i].start, map, cfi, 
cfi->device_type, NULL);
+			cfi_send_gen_cmd(0xF0, 0, start, map, cfi, cfi-
>device_type, NULL);
 			/* some devices don't respond to 0xF0, so send 0xFF to 
be sure */
-			cfi_send_gen_cmd(0xFF, 0, chips[i].start, map, cfi, 
cfi->device_type, NULL);
+			cfi_send_gen_cmd(0xFF, 0, start, map, cfi, cfi-
>device_type, NULL);
 
 			/* If the QRY marker goes away, it's an alias */
-			if (!qry_present(map, chips[i].start, cfi)) {
+			if (!qry_present(map, start, cfi)) {
 				printk(KERN_DEBUG "%s: Found an alias at 0x%x 
for the chip at 0x%lx\n",
-				       map->name, base, chips[i].start);
+				       map->name, base, start);
 				return 0;
 			}
 			/* Yes, it's actually got QRY for data. Most 
@@ -106,7 +111,7 @@
 			cfi_send_gen_cmd(0xFF, 0, base, map, cfi, cfi-
>device_type, NULL);
 			if (qry_present(map, base, cfi)) {
 				printk(KERN_DEBUG "%s: Found an alias at 0x%x 
for the chip at 0x%lx\n",
-				       map->name, base, chips[i].start);
+				       map->name, base, start);
 				return 0;
 			}
 		}
@@ -114,13 +119,7 @@
 	
 	/* OK, if we got to here, then none of the previous chips appear to
 	   be aliases for the current one. */
-	if (cfi->numchips == MAX_CFI_CHIPS) {
-		printk(KERN_WARNING"%s: Too many flash chips detected. 
Increase MAX_CFI_CHIPS from %d.\n", map->name, MAX_CFI_CHIPS);
-		/* Doesn't matter about resetting it to Read Mode - we're not 
going to talk to it anyway */
-		return -1;
-	}
-	chips[cfi->numchips].start = base;
-	chips[cfi->numchips].state = FL_READY;
+	set_bit((base >> cfi->chipshift), probe->chip_map); /* Update chip map 
*/
 	cfi->numchips++;
 	
 	/* Put it back into Read Mode */
diff -Nru --minimal --ignore-all-space prev/drivers/mtd/chips/gen_probe.c 
linux/drivers/mtd/chips/gen_probe.c
--- prev/drivers/mtd/chips/gen_probe.c	Fri Mar 14 17:21:18 2003
+++ linux/drivers/mtd/chips/gen_probe.c	Thu Jul 24 15:47:20 2003
@@ -50,11 +50,11 @@
 
 struct cfi_private *genprobe_ident_chips(struct map_info *map, struct 
chip_probe *cp)
 {
-	unsigned long base=0;
 	struct cfi_private cfi;
 	struct cfi_private *retcfi;
-	struct flchip chip[MAX_CFI_CHIPS];
-	int i;
+	struct probe_info probe;
+	int i, j;
+	int max_chips;
 
 	memset(&cfi, 0, sizeof(cfi));
 
@@ -77,8 +77,6 @@
 		return NULL;
 	}
 #endif
-	chip[0].start = 0;
-	chip[0].state = FL_READY;
 	cfi.chipshift = cfi.cfiq->DevSize;
 
 	switch(cfi.interleave) {
@@ -102,45 +100,57 @@
 		
 	cfi.numchips = 1;
 
+	/* Allocate memory for bitmap of valid chips. Align bitmap storage 
size to full byte. */
+	max_chips = map->size >> cfi.chipshift;
+	probe.chip_map = kmalloc((max_chips + 7) / 8, GFP_KERNEL);
+	if (!probe.chip_map) {
+		printk(KERN_WARNING "%s: kmalloc failed for CFI chip map\n", 
map->name);
+		kfree(cfi.cfiq);
+		return NULL;
+	}
+
+	set_bit(0, probe.chip_map); /* Mark first chip valid */
+
 	/*
 	 * Now probe for other chips, checking sensibly for aliases while
 	 * we're at it. The new_chip probe above should have let the first
 	 * chip in read mode.
-	 *
-	 * NOTE: Here, we're checking if there is room for another chip
-	 *       the same size within the mapping. Therefore, 
-	 *       base + chipsize <= map->size is the correct thing to do, 
-	 *       because, base + chipsize would be the  _first_ byte of the
-	 *       next chip, not the one we're currently pondering.
 	 */
 
-	for (base = (1<<cfi.chipshift); base + (1<<cfi.chipshift) <= map->size;
-	     base += (1<<cfi.chipshift))
-		cp->probe_chip(map, base, &chip[0], &cfi);
+	for (i = 1; i < max_chips; i++) {
+		cp->probe_chip(map, i << cfi.chipshift, &probe, &cfi);
+	}
 
 	/*
 	 * Now allocate the space for the structures we need to return to 
 	 * our caller, and copy the appropriate data into them.
 	 */
 
-	retcfi = kmalloc(sizeof(struct cfi_private) + cfi.numchips * sizeof
(struct flchip), GFP_KERNEL);
+	retcfi = kmalloc(sizeof(struct cfi_private) + (cfi.numchips - 1) * 
sizeof(struct flchip), GFP_KERNEL);
 
 	if (!retcfi) {
 		printk(KERN_WARNING "%s: kmalloc failed for CFI private 
structure\n", map->name);
 		kfree(cfi.cfiq);
+		kfree(probe.chip_map);
 		return NULL;
 	}
 
 	memcpy(retcfi, &cfi, sizeof(cfi));
-	memcpy(&retcfi->chips[0], chip, sizeof(struct flchip) * cfi.numchips);
+	memset(&retcfi->chips[0], 0, sizeof(struct flchip) * cfi.numchips);
 
-	/* Fix up the stuff that breaks when you move it */
-	for (i=0; i< retcfi->numchips; i++) {
-		init_waitqueue_head(&retcfi->chips[i].wq);
-		spin_lock_init(&retcfi->chips[i]._spinlock);
-		retcfi->chips[i].mutex = &retcfi->chips[i]._spinlock;
+	for (i = 0, j = 0; (j < cfi.numchips) && (i < max_chips); i++) {
+		if(test_bit(i, probe.chip_map)) {
+			struct flchip *pchip = &retcfi->chips[j++];
+
+			pchip->start = (i << cfi.chipshift);
+			pchip->state = FL_READY;
+			init_waitqueue_head(&pchip->wq);
+			spin_lock_init(&pchip->_spinlock);
+			pchip->mutex = &pchip->_spinlock;
+		}
 	}
 
+	kfree(probe.chip_map);
 	return retcfi;
 }
 
diff -Nru --minimal --ignore-all-space prev/drivers/mtd/chips/jedec_probe.c 
linux/drivers/mtd/chips/jedec_probe.c
--- prev/drivers/mtd/chips/jedec_probe.c	Fri Mar 14 17:21:19 2003
+++ linux/drivers/mtd/chips/jedec_probe.c	Thu Jul 24 15:11:10 2003
@@ -803,7 +803,7 @@
 static int cfi_jedec_setup(struct cfi_private *p_cfi, int index);
 
 static int jedec_probe_chip(struct map_info *map, __u32 base,
-			    struct flchip *chips, struct cfi_private *cfi);
+			    struct probe_info *probe, struct cfi_private *cfi);
 
 struct mtd_info *jedec_probe(struct map_info *map);
 
@@ -871,7 +871,7 @@
 }
 
 static int jedec_probe_chip(struct map_info *map, __u32 base,
-			      struct flchip *chips, struct cfi_private *cfi)
+			    struct probe_info *probe, struct cfi_private *cfi)
 {
 	int i;
 	int unlockpass = 0;
@@ -977,21 +977,24 @@
 		}
 	}
 	
-	/* Check each previous chip to see if it's an alias */
-	for (i=0; i<cfi->numchips; i++) {
-		/* This chip should be in read mode if it's one
-		   we've already touched. */
-		if (jedec_read_mfr(map, chips[i].start, cfi) == cfi->mfr &&
-		    jedec_read_id(map, chips[i].start, cfi) == cfi->id) {
+	/* Check each previous chip locations to see if it's an alias */
+	for (i=0; i < (base >> cfi->chipshift); i++) {
+		unsigned long start;
+		if(!test_bit(i, probe->chip_map)) {
+			continue; /* Skip location; no valid chip at this 
address */
+		}
+		start = i << cfi->chipshift;
+		if (jedec_read_mfr(map, start, cfi) == cfi->mfr &&
+		    jedec_read_id(map, start, cfi) == cfi->id) {
 			/* Eep. This chip also looks like it's in autoselect 
mode.
 			   Is it an alias for the new one? */
-			jedec_reset(chips[i].start, map, cfi);
+			jedec_reset(start, map, cfi);
 
 			/* If the device IDs go away, it's an alias */
 			if (jedec_read_mfr(map, base, cfi) != cfi->mfr ||
 			    jedec_read_id(map, base, cfi) != cfi->id) {
 				printk(KERN_DEBUG "%s: Found an alias at 0x%x 
for the chip at 0x%lx\n",
-				       map->name, base, chips[i].start);
+				       map->name, base, start);
 				return 0;
 			}
 			
@@ -1003,7 +1006,7 @@
 			if (jedec_read_mfr(map, base, cfi) == cfi->mfr &&
 			    jedec_read_id(map, base, cfi) == cfi->id) {
 				printk(KERN_DEBUG "%s: Found an alias at 0x%x 
for the chip at 0x%lx\n",
-				       map->name, base, chips[i].start);
+				       map->name, base, start);
 				return 0;
 			}
 		}
@@ -1011,13 +1014,7 @@
 		
 	/* OK, if we got to here, then none of the previous chips appear to
 	   be aliases for the current one. */
-	if (cfi->numchips == MAX_CFI_CHIPS) {
-		printk(KERN_WARNING"%s: Too many flash chips detected. 
Increase MAX_CFI_CHIPS from %d.\n", map->name, MAX_CFI_CHIPS);
-		/* Doesn't matter about resetting it to Read Mode - we're not 
going to talk to it anyway */
-		return -1;
-	}
-	chips[cfi->numchips].start = base;
-	chips[cfi->numchips].state = FL_READY;
+	set_bit((base >> cfi->chipshift), probe->chip_map); /* Update chip map 
*/
 	cfi->numchips++;
 		
 ok_out:
diff -Nru --minimal --ignore-all-space prev/include/linux/mtd/cfi.h 
linux/include/linux/mtd/cfi.h
--- prev/include/linux/mtd/cfi.h	Fri Mar 14 17:05:39 2003
+++ linux/include/linux/mtd/cfi.h	Thu Jul 24 13:32:18 2003
@@ -306,8 +306,6 @@
 	struct flchip chips[0];  /* per-chip data structure for each chip */
 };
 
-#define MAX_CFI_CHIPS 8 /* Entirely arbitrary to avoid realloc() */
-
 /*
  * Returns the command address according to the given geometry.
  */
diff -Nru --minimal --ignore-all-space prev/include/linux/mtd/gen_probe.h 
linux/include/linux/mtd/gen_probe.h
--- prev/include/linux/mtd/gen_probe.h	Fri Mar 14 15:49:56 2003
+++ linux/include/linux/mtd/gen_probe.h	Thu Jul 24 14:42:46 2003
@@ -10,12 +10,16 @@
 #include <linux/mtd/flashchip.h>
 #include <linux/mtd/map.h> 
 #include <linux/mtd/cfi.h>
+#include <asm/bitops.h>
+
+struct probe_info {
+	long *chip_map;	/* bitmap of valid chips in mtd map */
+};
 
 struct chip_probe {
 	char *name;
 	int (*probe_chip)(struct map_info *map, __u32 base,
-			  struct flchip *chips, struct cfi_private *cfi);
-
+			  struct probe_info *probe, struct cfi_private *cfi);
 };
 
 struct mtd_info *mtd_do_chip_probe(struct map_info *map, struct chip_probe 
*cp); 

Re: hardcoded maximum number of CFI chips - continued

[Jörn Engel]

On Thu, 24 July 2003 18:28:31 +0200, andrzej.mialkowski@inetia.pl wrote:
> 
> [a lot of code and explanations]

Sorry for the long lag.  Exam period has come up again, the most
irrational timeof the year for me.  But I'm finally reading your code.

Thanks! :)

Jörn

-- 
Premature optimization is the root of all evil.
-- Donald Knuth