[PATCH 1/2] mtdpart: Avoid divide-by-zero on out-of-reach path

[PATCH 1/2] mtdpart: Avoid divide-by-zero on out-of-reach path

[Atsushi Nemoto]

On "partition is out of reach" path, i.e. slave's offset was bigger
than the master's size, slave's erasesize will not be calculated and
it leads division by zero on following boundary checking.  This patch
makes calculation of the slave's erasesize more robust.

Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
---
This is broken-out patch from "[PATCH RESEND] mtdpart: Avoid
divide-by-zero on out-of-reach path" on Sat, 14 Jun 2008 23:45:40

 drivers/mtd/mtdpart.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 07c7011..a7a82c7 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -452,7 +452,9 @@ int add_mtd_partitions(struct mtd_info *master,
 			for (i=0; i < master->numeraseregions && slave->offset >= regions[i].offset; i++)
 				;
 
-			for (i--; i < master->numeraseregions && slave->offset + slave->mtd.size > regions[i].offset; i++) {
+			i--;
+			slave->mtd.erasesize = regions[i].erasesize;
+			for (; i < master->numeraseregions && slave->offset + slave->mtd.size > regions[i].offset; i++) {
 				if (slave->mtd.erasesize < regions[i].erasesize) {
 					slave->mtd.erasesize = regions[i].erasesize;
 				}

Re: [PATCH 1/2] mtdpart: Avoid divide-by-zero on out-of-reach path

[Jörn Engel]

On Mon, 16 June 2008 23:32:22 +0900, Atsushi Nemoto wrote:
> 
>  
> -			for (i--; i < master->numeraseregions && slave->offset + slave->mtd.size > regions[i].offset; i++) {
> +			i--;
> +			slave->mtd.erasesize = regions[i].erasesize;
> +			for (; i < master->numeraseregions && slave->offset + slave->mtd.size > regions[i].offset; i++) {
>  				if (slave->mtd.erasesize < regions[i].erasesize) {
>  					slave->mtd.erasesize = regions[i].erasesize;
>  				}

While this patch appears to work, I still don't like it.  Before the
patch, the whole function is simply a mess.  After your patch, it looks
even worse and becomes almost impossible to understand.  So while you
are fixing a bug today, the very next change may introduce a new bug
simply because whoever makes the change doesn't understand the code.

At least I have a hard enough time understanding it today.  The first
loop seems to look for the last eraseregion that is part of the current
partition.  Why then it should check for
	slave->offset + slave->mtd.size > regions[i].offset
instead of 
	slave->offset >= regions[i].offset

Odd.  And the second loop should go backwards as long as the
eraseregions are part of the current partition.  Which means that
	i < master->numeraseregions
doesn't make sense at all and
	slave->offset + slave->mtd.size > regions[i].offset
would imply that eraseregions go backwards.

In other words, I am tempted to replace all that with a single line:
	BUG();

At least that line is short and descriptive.  Otherwise it seems to be
roughly equivalent of what we had before.

Jörn

-- 
Mundie uses a textbook tactic of manipulation: start with some
reasonable talk, and lead the audience to an unreasonable conclusion.
-- Bruce Perens

Re: [PATCH 1/2] mtdpart: Avoid divide-by-zero on out-of-reach path

[Jörn Engel]

On Tue, 17 June 2008 17:29:32 +0200, Jörn Engel wrote:
> 
> In other words, I am tempted to replace all that with a single line:
> 	BUG();

Or maybe combine it with the else path:
	slave->mtd.erasesize = master->erasesize;
	if (master->numeraseregions > 1)
		printk(KERN_ERR"mtdpart can not handle multiple eraseregions correctly.\n");

Nemoto-san, would this be good enough for you?  It fixes the bug you
noticed, it removes most long lines and it removes the sparse warning.
It also removes functionality that may have been useful to some person -
detecting a smaller erasesize.  And with some luck and a warm easterly
wind, that functionality may even have worked.

Jörn

-- 
Maintenance in other professions and of other articles is concerned with
the return of the item to its original state; in Software, maintenance
is concerned with moving an item away from its original state.
-- Les Belady

Re: [PATCH 1/2] mtdpart: Avoid divide-by-zero on out-of-reach path

[Atsushi Nemoto]

On Tue, 17 Jun 2008 17:39:43 +0200, Jörn Engel <joern@logfs.org> wrote:
> > In other words, I am tempted to replace all that with a single line:
> > 	BUG();
> 
> Or maybe combine it with the else path:
> 	slave->mtd.erasesize = master->erasesize;
> 	if (master->numeraseregions > 1)
> 		printk(KERN_ERR"mtdpart can not handle multiple eraseregions correctly.\n");
> 
> Nemoto-san, would this be good enough for you?  It fixes the bug you
> noticed, it removes most long lines and it removes the sparse warning.
> It also removes functionality that may have been useful to some person -
> detecting a smaller erasesize.  And with some luck and a warm easterly
> wind, that functionality may even have worked.

No, unfortunately.  For example, if you split a 2MB chip with
16KB+8KB+8KB+32KB+64KBx31 eraseregions into two 1MB partitions, both
partition should have 64KB erase size.

---
Atsushi Nemoto

Re: [PATCH 1/2] mtdpart: Avoid divide-by-zero on out-of-reach path

[Atsushi Nemoto]

On Tue, 17 Jun 2008 17:29:32 +0200, Jörn Engel <joern@logfs.org> wrote:
> At least I have a hard enough time understanding it today.  The first
> loop seems to look for the last eraseregion that is part of the current
> partition.  Why then it should check for
> 	slave->offset + slave->mtd.size > regions[i].offset
> instead of 
> 	slave->offset >= regions[i].offset

The first loop searches an erase region which the start of the
partition belongs to.  So slave->mtd.size should be irrelevant.

> Odd.  And the second loop should go backwards as long as the
> eraseregions are part of the current partition.  Which means that
> 	i < master->numeraseregions
> doesn't make sense at all and
> 	slave->offset + slave->mtd.size > regions[i].offset
> would imply that eraseregions go backwards.

No, the second loop go forwards.  It searches maximum erasesize of the
partition.

> In other words, I am tempted to replace all that with a single line:
> 	BUG();
> 
> At least that line is short and descriptive.  Otherwise it seems to be
> roughly equivalent of what we had before.

Oh please don't.  Creating partitions on NOR flash chip with small
boot sectors is common usage.  These codes are surely required.

---
Atsushi Nemoto

Re: [PATCH 1/2] mtdpart: Avoid divide-by-zero on out-of-reach path

[Jörn Engel]

On Wed, 18 June 2008 00:57:35 +0900, Atsushi Nemoto wrote:
> 
> > Odd.  And the second loop should go backwards as long as the
> > eraseregions are part of the current partition.  Which means that
> > 	i < master->numeraseregions
> > doesn't make sense at all and
> > 	slave->offset + slave->mtd.size > regions[i].offset
> > would imply that eraseregions go backwards.
> 
> No, the second loop go forwards.  It searches maximum erasesize of the
> partition.

Indeed, it does.  And if I change the order in the conditions, those
make sense even to me.  Hm.

But what purpose does the 'i--' serve?  Why would we want to check an
eraseregion _before_ the start of the partition?

Next, you could simply add code like this below the second loop:
	/* In case no eraseregion matched. */
	if (slave->mtd.erasesize == 0)
		slave->mtd.erasesize = master->erasesize;

But under which conditions would we ever run into this?  It appears as
if those conditions would be better served with either 'BUG();' or
'return -EINVAL;'.

I am still rather confused by all this.

Jörn

-- 
...one more straw can't possibly matter...
-- Kirby Bakken

Re: [PATCH 1/2] mtdpart: Avoid divide-by-zero on out-of-reach path

[Atsushi Nemoto]

On Tue, 17 Jun 2008 18:46:29 +0200, Jörn Engel <joern@logfs.org> wrote:
> > No, the second loop go forwards.  It searches maximum erasesize of the
> > partition.
> 
> Indeed, it does.  And if I change the order in the conditions, those
> make sense even to me.  Hm.
> 
> But what purpose does the 'i--' serve?  Why would we want to check an
> eraseregion _before_ the start of the partition?

The decrement is there because when the first loop stoped 'i' points
the second region in the partition.

> Next, you could simply add code like this below the second loop:
> 	/* In case no eraseregion matched. */
> 	if (slave->mtd.erasesize == 0)
> 		slave->mtd.erasesize = master->erasesize;

Yes, it should work.

> But under which conditions would we ever run into this?  It appears as
> if those conditions would be better served with either 'BUG();' or
> 'return -EINVAL;'.
> 
> I am still rather confused by all this.

The keyword is 'out-of-reach path'.  I found this when I expected 8MB
flash and try to split into two 4MB chip, but 2MB chip was actually
mounted on the board socket.

The mtdpart detect this condition and disable the partition by setting
zero to mtd.size.  And fall through to code we are talking about.

	if (slave->offset >= master->size) {
			/* let's register it anyway to preserve ordering */
		slave->offset = 0;
		slave->mtd.size = 0;
		printk ("mtd: partition \"%s\" is out of reach -- disabled\n",
			parts[i].name);
	}

Then, the body of the second loop we are talking about will never
executed.  And finally 'slave->offset % slave->mtd.erasesize' will
cause division-by-zero.

Putting 'goto somewhere' or 'slave->mtd.erasesize = master->erasesize'
in the above 'if' block can be an alternative fix.

---
Atsushi Nemoto

Re: [PATCH 1/2] mtdpart: Avoid divide-by-zero on out-of-reach path

[Jörn Engel]

On Wed, 18 June 2008 11:19:07 +0900, Atsushi Nemoto wrote:
> 
> The decrement is there because when the first loop stoped 'i' points
> the second region in the partition.

Finally even I understand that code.  Thank you for your patience.

> Putting 'goto somewhere' or 'slave->mtd.erasesize = master->erasesize'
> in the above 'if' block can be an alternative fix.

That seems a better idea.

Jörn

-- 
A defeated army first battles and then seeks victory.
-- Sun Tzu

Re: [PATCH 1/2] mtdpart: Avoid divide-by-zero on out-of-reach path

[Jörn Engel]

On Wed, 18 June 2008 19:40:34 +0200, Jörn Engel wrote:
> On Wed, 18 June 2008 11:19:07 +0900, Atsushi Nemoto wrote:
> 
> > Putting 'goto somewhere' or 'slave->mtd.erasesize = master->erasesize'
> > in the above 'if' block can be an alternative fix.
> 
> That seems a better idea.

I've had a quick go at it.  Three cleanup patches first, then the goto.
Patches have been compile-tested, but not more.  Would these work?

Jörn

-- 
I can say that I spend most of my time fixing bugs even if I have lots
of new features to implement in mind, but I give bugs more priority.
-- Andrea Arcangeli, 2000

[PATCH 1/4] [MTD][MTDPART] Seperate main loop from per-partition code in add_mtd_partition

add_mtd_partition was a 150+ line monster consisting mostly of a single
loop.  Seperate the loop from most of the body.  Now it should be
obvious which variables are carried around from iteration to iteration.

Signed-off-by: Joern Engel <joern@logfs.org>
---
 drivers/mtd/mtdpart.c |  327 +++++++++++++++++++++++++------------------------
 1 files changed, 168 insertions(+), 159 deletions(-)

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 07c7011..f0d0430 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -322,184 +322,193 @@ int del_mtd_partitions(struct mtd_info *master)
 	return 0;
 }
 
-/*
- * This function, given a master MTD object and a partition table, creates
- * and registers slave MTD objects which are bound to the master according to
- * the partition definitions.
- * (Q: should we register the master MTD object as well?)
- */
-
-int add_mtd_partitions(struct mtd_info *master,
-		       const struct mtd_partition *parts,
-		       int nbparts)
+static int add_one_partition(struct mtd_info *master,
+		const struct mtd_partition *part, int partno,
+		u_int32_t cur_offset)
 {
 	struct mtd_part *slave;
-	u_int32_t cur_offset = 0;
-	int i;
-
-	printk (KERN_NOTICE "Creating %d MTD partitions on \"%s\":\n", nbparts, master->name);
-
-	for (i = 0; i < nbparts; i++) {
 
-		/* allocate the partition structure */
-		slave = kzalloc (sizeof(*slave), GFP_KERNEL);
-		if (!slave) {
-			printk ("memory allocation error while creating partitions for \"%s\"\n",
-				master->name);
-			del_mtd_partitions(master);
-			return -ENOMEM;
-		}
-		list_add(&slave->list, &mtd_partitions);
+	/* allocate the partition structure */
+	slave = kzalloc (sizeof(*slave), GFP_KERNEL);
+	if (!slave) {
+		printk("memory allocation error while creating partitions for \"%s\"\n",
+			master->name);
+		del_mtd_partitions(master);
+		return -ENOMEM;
+	}
+	list_add(&slave->list, &mtd_partitions);
 
-		/* set up the MTD object for this partition */
-		slave->mtd.type = master->type;
-		slave->mtd.flags = master->flags & ~parts[i].mask_flags;
-		slave->mtd.size = parts[i].size;
-		slave->mtd.writesize = master->writesize;
-		slave->mtd.oobsize = master->oobsize;
-		slave->mtd.oobavail = master->oobavail;
-		slave->mtd.subpage_sft = master->subpage_sft;
+	/* set up the MTD object for this partition */
+	slave->mtd.type = master->type;
+	slave->mtd.flags = master->flags & ~part->mask_flags;
+	slave->mtd.size = part->size;
+	slave->mtd.writesize = master->writesize;
+	slave->mtd.oobsize = master->oobsize;
+	slave->mtd.oobavail = master->oobavail;
+	slave->mtd.subpage_sft = master->subpage_sft;
 
-		slave->mtd.name = parts[i].name;
-		slave->mtd.owner = master->owner;
+	slave->mtd.name = part->name;
+	slave->mtd.owner = master->owner;
 
-		slave->mtd.read = part_read;
-		slave->mtd.write = part_write;
+	slave->mtd.read = part_read;
+	slave->mtd.write = part_write;
 
-		if (master->panic_write)
-			slave->mtd.panic_write = part_panic_write;
+	if (master->panic_write)
+		slave->mtd.panic_write = part_panic_write;
 
-		if(master->point && master->unpoint){
-			slave->mtd.point = part_point;
-			slave->mtd.unpoint = part_unpoint;
-		}
+	if(master->point && master->unpoint){
+		slave->mtd.point = part_point;
+		slave->mtd.unpoint = part_unpoint;
+	}
 
-		if (master->read_oob)
-			slave->mtd.read_oob = part_read_oob;
-		if (master->write_oob)
-			slave->mtd.write_oob = part_write_oob;
-		if(master->read_user_prot_reg)
-			slave->mtd.read_user_prot_reg = part_read_user_prot_reg;
-		if(master->read_fact_prot_reg)
-			slave->mtd.read_fact_prot_reg = part_read_fact_prot_reg;
-		if(master->write_user_prot_reg)
-			slave->mtd.write_user_prot_reg = part_write_user_prot_reg;
-		if(master->lock_user_prot_reg)
-			slave->mtd.lock_user_prot_reg = part_lock_user_prot_reg;
-		if(master->get_user_prot_info)
-			slave->mtd.get_user_prot_info = part_get_user_prot_info;
-		if(master->get_fact_prot_info)
-			slave->mtd.get_fact_prot_info = part_get_fact_prot_info;
-		if (master->sync)
-			slave->mtd.sync = part_sync;
-		if (!i && master->suspend && master->resume) {
-				slave->mtd.suspend = part_suspend;
-				slave->mtd.resume = part_resume;
+	if (master->read_oob)
+		slave->mtd.read_oob = part_read_oob;
+	if (master->write_oob)
+		slave->mtd.write_oob = part_write_oob;
+	if(master->read_user_prot_reg)
+		slave->mtd.read_user_prot_reg = part_read_user_prot_reg;
+	if(master->read_fact_prot_reg)
+		slave->mtd.read_fact_prot_reg = part_read_fact_prot_reg;
+	if(master->write_user_prot_reg)
+		slave->mtd.write_user_prot_reg = part_write_user_prot_reg;
+	if(master->lock_user_prot_reg)
+		slave->mtd.lock_user_prot_reg = part_lock_user_prot_reg;
+	if(master->get_user_prot_info)
+		slave->mtd.get_user_prot_info = part_get_user_prot_info;
+	if(master->get_fact_prot_info)
+		slave->mtd.get_fact_prot_info = part_get_fact_prot_info;
+	if (master->sync)
+		slave->mtd.sync = part_sync;
+	if (!partno && master->suspend && master->resume) {
+			slave->mtd.suspend = part_suspend;
+			slave->mtd.resume = part_resume;
+	}
+	if (master->writev)
+		slave->mtd.writev = part_writev;
+	if (master->lock)
+		slave->mtd.lock = part_lock;
+	if (master->unlock)
+		slave->mtd.unlock = part_unlock;
+	if (master->block_isbad)
+		slave->mtd.block_isbad = part_block_isbad;
+	if (master->block_markbad)
+		slave->mtd.block_markbad = part_block_markbad;
+	slave->mtd.erase = part_erase;
+	slave->master = master;
+	slave->offset = part->offset;
+	slave->index = partno;
+
+	if (slave->offset == MTDPART_OFS_APPEND)
+		slave->offset = cur_offset;
+	if (slave->offset == MTDPART_OFS_NXTBLK) {
+		slave->offset = cur_offset;
+		if ((cur_offset % master->erasesize) != 0) {
+			/* Round up to next erasesize */
+			slave->offset = ((cur_offset / master->erasesize) + 1) * master->erasesize;
+			printk(KERN_NOTICE "Moving partition %d: "
+			       "0x%08x -> 0x%08x\n", partno,
+			       cur_offset, slave->offset);
 		}
-		if (master->writev)
-			slave->mtd.writev = part_writev;
-		if (master->lock)
-			slave->mtd.lock = part_lock;
-		if (master->unlock)
-			slave->mtd.unlock = part_unlock;
-		if (master->block_isbad)
-			slave->mtd.block_isbad = part_block_isbad;
-		if (master->block_markbad)
-			slave->mtd.block_markbad = part_block_markbad;
-		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;
-		if (slave->offset == MTDPART_OFS_NXTBLK) {
-			slave->offset = cur_offset;
-			if ((cur_offset % master->erasesize) != 0) {
-				/* Round up to next erasesize */
-				slave->offset = ((cur_offset / master->erasesize) + 1) * master->erasesize;
-				printk(KERN_NOTICE "Moving partition %d: "
-				       "0x%08x -> 0x%08x\n", i,
-				       cur_offset, slave->offset);
+	}
+	if (slave->mtd.size == MTDPART_SIZ_FULL)
+		slave->mtd.size = master->size - slave->offset;
+
+	printk (KERN_NOTICE "0x%08x-0x%08x : \"%s\"\n", slave->offset,
+		slave->offset + slave->mtd.size, slave->mtd.name);
+
+	/* let's do some sanity checks */
+	if (slave->offset >= master->size) {
+			/* let's register it anyway to preserve ordering */
+		slave->offset = 0;
+		slave->mtd.size = 0;
+		printk ("mtd: partition \"%s\" is out of reach -- disabled\n",
+			part->name);
+	}
+	if (slave->offset + slave->mtd.size > master->size) {
+		slave->mtd.size = master->size - slave->offset;
+		printk ("mtd: partition \"%s\" extends beyond the end of device \"%s\" -- size truncated to %#x\n",
+			part->name, master->name, slave->mtd.size);
+	}
+	if (master->numeraseregions>1) {
+		/* Deal with variable erase size stuff */
+		int i;
+		struct mtd_erase_region_info *regions = master->eraseregions;
+
+		/* Find the first erase regions which is part of this partition. */
+		for (i=0; i < master->numeraseregions && regions[i].offset <= slave->offset; i++)
+			;
+
+		for (i--; i < master->numeraseregions && regions[i].offset < slave->offset + slave->mtd.size; i++) {
+			if (slave->mtd.erasesize < regions[i].erasesize) {
+				slave->mtd.erasesize = regions[i].erasesize;
 			}
 		}
-		if (slave->mtd.size == MTDPART_SIZ_FULL)
-			slave->mtd.size = master->size - slave->offset;
-		cur_offset = slave->offset + slave->mtd.size;
+	} else {
+		/* Single erase size */
+		slave->mtd.erasesize = master->erasesize;
+	}
 
-		printk (KERN_NOTICE "0x%08x-0x%08x : \"%s\"\n", slave->offset,
-			slave->offset + slave->mtd.size, slave->mtd.name);
+	if ((slave->mtd.flags & MTD_WRITEABLE) &&
+	    (slave->offset % slave->mtd.erasesize)) {
+		/* Doesn't start on a boundary of major erase size */
+		/* FIXME: Let it be writable if it is on a boundary of _minor_ erase size though */
+		slave->mtd.flags &= ~MTD_WRITEABLE;
+		printk ("mtd: partition \"%s\" doesn't start on an erase block boundary -- force read-only\n",
+			part->name);
+	}
+	if ((slave->mtd.flags & MTD_WRITEABLE) &&
+	    (slave->mtd.size % slave->mtd.erasesize)) {
+		slave->mtd.flags &= ~MTD_WRITEABLE;
+		printk ("mtd: partition \"%s\" doesn't end on an erase block -- force read-only\n",
+			part->name);
+	}
 
-		/* let's do some sanity checks */
-		if (slave->offset >= master->size) {
-				/* let's register it anyway to preserve ordering */
-			slave->offset = 0;
-			slave->mtd.size = 0;
-			printk ("mtd: partition \"%s\" is out of reach -- disabled\n",
-				parts[i].name);
-		}
-		if (slave->offset + slave->mtd.size > master->size) {
-			slave->mtd.size = master->size - slave->offset;
-			printk ("mtd: partition \"%s\" extends beyond the end of device \"%s\" -- size truncated to %#x\n",
-				parts[i].name, master->name, slave->mtd.size);
-		}
-		if (master->numeraseregions>1) {
-			/* Deal with variable erase size stuff */
-			int i;
-			struct mtd_erase_region_info *regions = master->eraseregions;
-
-			/* Find the first erase regions which is part of this partition. */
-			for (i=0; i < master->numeraseregions && slave->offset >= regions[i].offset; i++)
-				;
-
-			for (i--; i < master->numeraseregions && slave->offset + slave->mtd.size > regions[i].offset; i++) {
-				if (slave->mtd.erasesize < regions[i].erasesize) {
-					slave->mtd.erasesize = regions[i].erasesize;
-				}
-			}
-		} else {
-			/* Single erase size */
-			slave->mtd.erasesize = master->erasesize;
-		}
+	slave->mtd.ecclayout = master->ecclayout;
+	if (master->block_isbad) {
+		uint32_t offs = 0;
 
-		if ((slave->mtd.flags & MTD_WRITEABLE) &&
-		    (slave->offset % slave->mtd.erasesize)) {
-			/* Doesn't start on a boundary of major erase size */
-			/* FIXME: Let it be writable if it is on a boundary of _minor_ erase size though */
-			slave->mtd.flags &= ~MTD_WRITEABLE;
-			printk ("mtd: partition \"%s\" doesn't start on an erase block boundary -- force read-only\n",
-				parts[i].name);
-		}
-		if ((slave->mtd.flags & MTD_WRITEABLE) &&
-		    (slave->mtd.size % slave->mtd.erasesize)) {
-			slave->mtd.flags &= ~MTD_WRITEABLE;
-			printk ("mtd: partition \"%s\" doesn't end on an erase block -- force read-only\n",
-				parts[i].name);
+		while(offs < slave->mtd.size) {
+			if (master->block_isbad(master,
+						offs + slave->offset))
+				slave->mtd.ecc_stats.badblocks++;
+			offs += slave->mtd.erasesize;
 		}
+	}
 
-		slave->mtd.ecclayout = master->ecclayout;
-		if (master->block_isbad) {
-			uint32_t offs = 0;
+	if(part->mtdp) {	/* store the object pointer (caller may or may not register it */
+		*part->mtdp = &slave->mtd;
+		slave->registered = 0;
+	} else {
+		/* register our partition */
+		add_mtd_device(&slave->mtd);
+		slave->registered = 1;
+	}
+	return 0;
+}
 
-			while(offs < slave->mtd.size) {
-				if (master->block_isbad(master,
-							offs + slave->offset))
-					slave->mtd.ecc_stats.badblocks++;
-				offs += slave->mtd.erasesize;
-			}
-		}
+/*
+ * This function, given a master MTD object and a partition table, creates
+ * and registers slave MTD objects which are bound to the master according to
+ * the partition definitions.
+ * (Q: should we register the master MTD object as well?)
+ */
 
-		if(parts[i].mtdp)
-		{	/* store the object pointer (caller may or may not register it */
-			*parts[i].mtdp = &slave->mtd;
-			slave->registered = 0;
-		}
-		else
-		{
-			/* register our partition */
-			add_mtd_device(&slave->mtd);
-			slave->registered = 1;
-		}
+int add_mtd_partitions(struct mtd_info *master,
+		       const struct mtd_partition *parts,
+		       int nbparts)
+{
+	struct mtd_part *slave;
+	u_int32_t cur_offset = 0;
+	int i, err;
+
+	printk (KERN_NOTICE "Creating %d MTD partitions on \"%s\":\n", nbparts, master->name);
+
+	for (i = 0; i < nbparts; i++) {
+		err = add_one_partition(master, parts + i, i, cur_offset);
+		if (err)
+			return err;
+		slave = PART(parts[i].mtdp);
+		cur_offset = slave->offset + slave->mtd.size;
 	}
 
 	return 0;
-- 
1.5.3.5

Re: [PATCH 1/2] mtdpart: Avoid divide-by-zero on out-of-reach path

[Jörn Engel]

[PATCH 2/4] [MTD][MTDPART] Handle most checkpatch findings

Remaining are 13 warnings about long lines, 1 about a CVS marker already
removed by Adrian and 1 about braces that could be argued about.

Signed-off-by: Joern Engel <joern@logfs.org>
---
 drivers/mtd/mtdpart.c |  129 +++++++++++++++++++++++++------------------------
 1 files changed, 65 insertions(+), 64 deletions(-)

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index f0d0430..e759ee6 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -46,8 +46,8 @@ struct mtd_part {
  * to the _real_ device.
  */
 
-static int part_read (struct mtd_info *mtd, loff_t from, size_t len,
-			size_t *retlen, u_char *buf)
+static int part_read(struct mtd_info *mtd, loff_t from, size_t len,
+		size_t *retlen, u_char *buf)
 {
 	struct mtd_part *part = PART(mtd);
 	int res;
@@ -56,7 +56,7 @@ static int part_read (struct mtd_info *mtd, loff_t from, size_t len,
 		len = 0;
 	else if (from + len > mtd->size)
 		len = mtd->size - from;
-	res = part->master->read (part->master, from + part->offset,
+	res = part->master->read(part->master, from + part->offset,
 				   len, retlen, buf);
 	if (unlikely(res)) {
 		if (res == -EUCLEAN)
@@ -67,8 +67,8 @@ static int part_read (struct mtd_info *mtd, loff_t from, size_t len,
 	return res;
 }
 
-static int part_point (struct mtd_info *mtd, loff_t from, size_t len,
-			size_t *retlen, void **virt, resource_size_t *phys)
+static int part_point(struct mtd_info *mtd, loff_t from, size_t len,
+		size_t *retlen, void **virt, resource_size_t *phys)
 {
 	struct mtd_part *part = PART(mtd);
 	if (from >= mtd->size)
@@ -87,7 +87,7 @@ static void part_unpoint(struct mtd_info *mtd, loff_t from, size_t len)
 }
 
 static int part_read_oob(struct mtd_info *mtd, loff_t from,
-			 struct mtd_oob_ops *ops)
+		struct mtd_oob_ops *ops)
 {
 	struct mtd_part *part = PART(mtd);
 	int res;
@@ -107,38 +107,38 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from,
 	return res;
 }
 
-static int part_read_user_prot_reg (struct mtd_info *mtd, loff_t from, size_t len,
-			size_t *retlen, u_char *buf)
+static int part_read_user_prot_reg(struct mtd_info *mtd, loff_t from,
+		size_t len, size_t *retlen, u_char *buf)
 {
 	struct mtd_part *part = PART(mtd);
-	return part->master->read_user_prot_reg (part->master, from,
+	return part->master->read_user_prot_reg(part->master, from,
 					len, retlen, buf);
 }
 
-static int part_get_user_prot_info (struct mtd_info *mtd,
-				    struct otp_info *buf, size_t len)
+static int part_get_user_prot_info(struct mtd_info *mtd,
+		struct otp_info *buf, size_t len)
 {
 	struct mtd_part *part = PART(mtd);
-	return part->master->get_user_prot_info (part->master, buf, len);
+	return part->master->get_user_prot_info(part->master, buf, len);
 }
 
-static int part_read_fact_prot_reg (struct mtd_info *mtd, loff_t from, size_t len,
-			size_t *retlen, u_char *buf)
+static int part_read_fact_prot_reg(struct mtd_info *mtd, loff_t from,
+		size_t len, size_t *retlen, u_char *buf)
 {
 	struct mtd_part *part = PART(mtd);
-	return part->master->read_fact_prot_reg (part->master, from,
+	return part->master->read_fact_prot_reg(part->master, from,
 					len, retlen, buf);
 }
 
-static int part_get_fact_prot_info (struct mtd_info *mtd,
-				    struct otp_info *buf, size_t len)
+static int part_get_fact_prot_info(struct mtd_info *mtd, struct otp_info *buf,
+		size_t len)
 {
 	struct mtd_part *part = PART(mtd);
-	return part->master->get_fact_prot_info (part->master, buf, len);
+	return part->master->get_fact_prot_info(part->master, buf, len);
 }
 
-static int part_write (struct mtd_info *mtd, loff_t to, size_t len,
-			size_t *retlen, const u_char *buf)
+static int part_write(struct mtd_info *mtd, loff_t to, size_t len,
+		size_t *retlen, const u_char *buf)
 {
 	struct mtd_part *part = PART(mtd);
 	if (!(mtd->flags & MTD_WRITEABLE))
@@ -147,12 +147,12 @@ static int part_write (struct mtd_info *mtd, loff_t to, size_t len,
 		len = 0;
 	else if (to + len > mtd->size)
 		len = mtd->size - to;
-	return part->master->write (part->master, to + part->offset,
+	return part->master->write(part->master, to + part->offset,
 				    len, retlen, buf);
 }
 
-static int part_panic_write (struct mtd_info *mtd, loff_t to, size_t len,
-			size_t *retlen, const u_char *buf)
+static int part_panic_write(struct mtd_info *mtd, loff_t to, size_t len,
+		size_t *retlen, const u_char *buf)
 {
 	struct mtd_part *part = PART(mtd);
 	if (!(mtd->flags & MTD_WRITEABLE))
@@ -161,12 +161,12 @@ static int part_panic_write (struct mtd_info *mtd, loff_t to, size_t len,
 		len = 0;
 	else if (to + len > mtd->size)
 		len = mtd->size - to;
-	return part->master->panic_write (part->master, to + part->offset,
+	return part->master->panic_write(part->master, to + part->offset,
 				    len, retlen, buf);
 }
 
 static int part_write_oob(struct mtd_info *mtd, loff_t to,
-			 struct mtd_oob_ops *ops)
+		struct mtd_oob_ops *ops)
 {
 	struct mtd_part *part = PART(mtd);
 
@@ -180,31 +180,32 @@ static int part_write_oob(struct mtd_info *mtd, loff_t to,
 	return part->master->write_oob(part->master, to + part->offset, ops);
 }
 
-static int part_write_user_prot_reg (struct mtd_info *mtd, loff_t from, size_t len,
-			size_t *retlen, u_char *buf)
+static int part_write_user_prot_reg(struct mtd_info *mtd, loff_t from,
+		size_t len, size_t *retlen, u_char *buf)
 {
 	struct mtd_part *part = PART(mtd);
-	return part->master->write_user_prot_reg (part->master, from,
+	return part->master->write_user_prot_reg(part->master, from,
 					len, retlen, buf);
 }
 
-static int part_lock_user_prot_reg (struct mtd_info *mtd, loff_t from, size_t len)
+static int part_lock_user_prot_reg(struct mtd_info *mtd, loff_t from,
+		size_t len)
 {
 	struct mtd_part *part = PART(mtd);
-	return part->master->lock_user_prot_reg (part->master, from, len);
+	return part->master->lock_user_prot_reg(part->master, from, len);
 }
 
-static int part_writev (struct mtd_info *mtd,  const struct kvec *vecs,
-			 unsigned long count, loff_t to, size_t *retlen)
+static int part_writev(struct mtd_info *mtd, const struct kvec *vecs,
+		unsigned long count, loff_t to, size_t *retlen)
 {
 	struct mtd_part *part = PART(mtd);
 	if (!(mtd->flags & MTD_WRITEABLE))
 		return -EROFS;
-	return part->master->writev (part->master, vecs, count,
+	return part->master->writev(part->master, vecs, count,
 					to + part->offset, retlen);
 }
 
-static int part_erase (struct mtd_info *mtd, struct erase_info *instr)
+static int part_erase(struct mtd_info *mtd, struct erase_info *instr)
 {
 	struct mtd_part *part = PART(mtd);
 	int ret;
@@ -236,7 +237,7 @@ void mtd_erase_callback(struct erase_info *instr)
 }
 EXPORT_SYMBOL_GPL(mtd_erase_callback);
 
-static int part_lock (struct mtd_info *mtd, loff_t ofs, size_t len)
+static int part_lock(struct mtd_info *mtd, loff_t ofs, size_t len)
 {
 	struct mtd_part *part = PART(mtd);
 	if ((len + ofs) > mtd->size)
@@ -244,7 +245,7 @@ static int part_lock (struct mtd_info *mtd, loff_t ofs, size_t len)
 	return part->master->lock(part->master, ofs + part->offset, len);
 }
 
-static int part_unlock (struct mtd_info *mtd, loff_t ofs, size_t len)
+static int part_unlock(struct mtd_info *mtd, loff_t ofs, size_t len)
 {
 	struct mtd_part *part = PART(mtd);
 	if ((len + ofs) > mtd->size)
@@ -270,7 +271,7 @@ static void part_resume(struct mtd_info *mtd)
 	part->master->resume(part->master);
 }
 
-static int part_block_isbad (struct mtd_info *mtd, loff_t ofs)
+static int part_block_isbad(struct mtd_info *mtd, loff_t ofs)
 {
 	struct mtd_part *part = PART(mtd);
 	if (ofs >= mtd->size)
@@ -279,7 +280,7 @@ static int part_block_isbad (struct mtd_info *mtd, loff_t ofs)
 	return part->master->block_isbad(part->master, ofs);
 }
 
-static int part_block_markbad (struct mtd_info *mtd, loff_t ofs)
+static int part_block_markbad(struct mtd_info *mtd, loff_t ofs)
 {
 	struct mtd_part *part = PART(mtd);
 	int res;
@@ -312,7 +313,7 @@ int del_mtd_partitions(struct mtd_info *master)
 		if (slave->master == master) {
 			struct list_head *prev = node->prev;
 			__list_del(prev, node->next);
-			if(slave->registered)
+			if (slave->registered)
 				del_mtd_device(&slave->mtd);
 			kfree(slave);
 			node = prev;
@@ -321,6 +322,7 @@ int del_mtd_partitions(struct mtd_info *master)
 
 	return 0;
 }
+EXPORT_SYMBOL(del_mtd_partitions);
 
 static int add_one_partition(struct mtd_info *master,
 		const struct mtd_partition *part, int partno,
@@ -329,9 +331,9 @@ static int add_one_partition(struct mtd_info *master,
 	struct mtd_part *slave;
 
 	/* allocate the partition structure */
-	slave = kzalloc (sizeof(*slave), GFP_KERNEL);
+	slave = kzalloc(sizeof(*slave), GFP_KERNEL);
 	if (!slave) {
-		printk("memory allocation error while creating partitions for \"%s\"\n",
+		printk(KERN_ERR"memory allocation error while creating partitions for \"%s\"\n",
 			master->name);
 		del_mtd_partitions(master);
 		return -ENOMEM;
@@ -356,7 +358,7 @@ static int add_one_partition(struct mtd_info *master,
 	if (master->panic_write)
 		slave->mtd.panic_write = part_panic_write;
 
-	if(master->point && master->unpoint){
+	if (master->point && master->unpoint) {
 		slave->mtd.point = part_point;
 		slave->mtd.unpoint = part_unpoint;
 	}
@@ -365,17 +367,17 @@ static int add_one_partition(struct mtd_info *master,
 		slave->mtd.read_oob = part_read_oob;
 	if (master->write_oob)
 		slave->mtd.write_oob = part_write_oob;
-	if(master->read_user_prot_reg)
+	if (master->read_user_prot_reg)
 		slave->mtd.read_user_prot_reg = part_read_user_prot_reg;
-	if(master->read_fact_prot_reg)
+	if (master->read_fact_prot_reg)
 		slave->mtd.read_fact_prot_reg = part_read_fact_prot_reg;
-	if(master->write_user_prot_reg)
+	if (master->write_user_prot_reg)
 		slave->mtd.write_user_prot_reg = part_write_user_prot_reg;
-	if(master->lock_user_prot_reg)
+	if (master->lock_user_prot_reg)
 		slave->mtd.lock_user_prot_reg = part_lock_user_prot_reg;
-	if(master->get_user_prot_info)
+	if (master->get_user_prot_info)
 		slave->mtd.get_user_prot_info = part_get_user_prot_info;
-	if(master->get_fact_prot_info)
+	if (master->get_fact_prot_info)
 		slave->mtd.get_fact_prot_info = part_get_fact_prot_info;
 	if (master->sync)
 		slave->mtd.sync = part_sync;
@@ -413,7 +415,7 @@ static int add_one_partition(struct mtd_info *master,
 	if (slave->mtd.size == MTDPART_SIZ_FULL)
 		slave->mtd.size = master->size - slave->offset;
 
-	printk (KERN_NOTICE "0x%08x-0x%08x : \"%s\"\n", slave->offset,
+	printk(KERN_NOTICE "0x%08x-0x%08x : \"%s\"\n", slave->offset,
 		slave->offset + slave->mtd.size, slave->mtd.name);
 
 	/* let's do some sanity checks */
@@ -421,21 +423,21 @@ static int add_one_partition(struct mtd_info *master,
 			/* let's register it anyway to preserve ordering */
 		slave->offset = 0;
 		slave->mtd.size = 0;
-		printk ("mtd: partition \"%s\" is out of reach -- disabled\n",
+		printk(KERN_ERR"mtd: partition \"%s\" is out of reach -- disabled\n",
 			part->name);
 	}
 	if (slave->offset + slave->mtd.size > master->size) {
 		slave->mtd.size = master->size - slave->offset;
-		printk ("mtd: partition \"%s\" extends beyond the end of device \"%s\" -- size truncated to %#x\n",
+		printk(KERN_WARNING"mtd: partition \"%s\" extends beyond the end of device \"%s\" -- size truncated to %#x\n",
 			part->name, master->name, slave->mtd.size);
 	}
-	if (master->numeraseregions>1) {
+	if (master->numeraseregions > 1) {
 		/* Deal with variable erase size stuff */
 		int i;
 		struct mtd_erase_region_info *regions = master->eraseregions;
 
 		/* Find the first erase regions which is part of this partition. */
-		for (i=0; i < master->numeraseregions && regions[i].offset <= slave->offset; i++)
+		for (i = 0; i < master->numeraseregions && regions[i].offset <= slave->offset; i++)
 			;
 
 		for (i--; i < master->numeraseregions && regions[i].offset < slave->offset + slave->mtd.size; i++) {
@@ -451,15 +453,16 @@ static int add_one_partition(struct mtd_info *master,
 	if ((slave->mtd.flags & MTD_WRITEABLE) &&
 	    (slave->offset % slave->mtd.erasesize)) {
 		/* Doesn't start on a boundary of major erase size */
-		/* FIXME: Let it be writable if it is on a boundary of _minor_ erase size though */
+		/* FIXME: Let it be writable if it is on a boundary of
+		 * _minor_ erase size though */
 		slave->mtd.flags &= ~MTD_WRITEABLE;
-		printk ("mtd: partition \"%s\" doesn't start on an erase block boundary -- force read-only\n",
+		printk(KERN_WARNING"mtd: partition \"%s\" doesn't start on an erase block boundary -- force read-only\n",
 			part->name);
 	}
 	if ((slave->mtd.flags & MTD_WRITEABLE) &&
 	    (slave->mtd.size % slave->mtd.erasesize)) {
 		slave->mtd.flags &= ~MTD_WRITEABLE;
-		printk ("mtd: partition \"%s\" doesn't end on an erase block -- force read-only\n",
+		printk(KERN_WARNING"mtd: partition \"%s\" doesn't end on an erase block -- force read-only\n",
 			part->name);
 	}
 
@@ -467,7 +470,7 @@ static int add_one_partition(struct mtd_info *master,
 	if (master->block_isbad) {
 		uint32_t offs = 0;
 
-		while(offs < slave->mtd.size) {
+		while (offs < slave->mtd.size) {
 			if (master->block_isbad(master,
 						offs + slave->offset))
 				slave->mtd.ecc_stats.badblocks++;
@@ -475,7 +478,8 @@ static int add_one_partition(struct mtd_info *master,
 		}
 	}
 
-	if(part->mtdp) {	/* store the object pointer (caller may or may not register it */
+	if (part->mtdp) {
+		/* store the object pointer (caller may or may not register it*/
 		*part->mtdp = &slave->mtd;
 		slave->registered = 0;
 	} else {
@@ -501,7 +505,7 @@ int add_mtd_partitions(struct mtd_info *master,
 	u_int32_t cur_offset = 0;
 	int i, err;
 
-	printk (KERN_NOTICE "Creating %d MTD partitions on \"%s\":\n", nbparts, master->name);
+	printk(KERN_NOTICE "Creating %d MTD partitions on \"%s\":\n", nbparts, master->name);
 
 	for (i = 0; i < nbparts; i++) {
 		err = add_one_partition(master, parts + i, i, cur_offset);
@@ -513,9 +517,7 @@ int add_mtd_partitions(struct mtd_info *master,
 
 	return 0;
 }
-
 EXPORT_SYMBOL(add_mtd_partitions);
-EXPORT_SYMBOL(del_mtd_partitions);
 
 static DEFINE_SPINLOCK(part_parser_lock);
 static LIST_HEAD(part_parsers);
@@ -547,6 +549,7 @@ int register_mtd_parser(struct mtd_part_parser *p)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(register_mtd_parser);
 
 int deregister_mtd_parser(struct mtd_part_parser *p)
 {
@@ -555,6 +558,7 @@ int deregister_mtd_parser(struct mtd_part_parser *p)
 	spin_unlock(&part_parser_lock);
 	return 0;
 }
+EXPORT_SYMBOL_GPL(deregister_mtd_parser);
 
 int parse_mtd_partitions(struct mtd_info *master, const char **types,
 			 struct mtd_partition **pparts, unsigned long origin)
@@ -582,7 +586,4 @@ int parse_mtd_partitions(struct mtd_info *master, const char **types,
 	}
 	return ret;
 }
-
 EXPORT_SYMBOL_GPL(parse_mtd_partitions);
-EXPORT_SYMBOL_GPL(register_mtd_parser);
-EXPORT_SYMBOL_GPL(deregister_mtd_parser);
-- 
1.5.3.5

Re: [PATCH 1/2] mtdpart: Avoid divide-by-zero on out-of-reach path

[Jörn Engel]

[PATCH 3/4] [MTD][MTDPART] Cleanup and document the erase region handling

Mostly simplifying the loops.  Now everything fits into 80 columns,
is easier to read and the finer details have extra comments.

Signed-off-by: Joern Engel <joern@logfs.org>
---
 drivers/mtd/mtdpart.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index e759ee6..c1a8916 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -433,18 +433,24 @@ static int add_one_partition(struct mtd_info *master,
 	}
 	if (master->numeraseregions > 1) {
 		/* Deal with variable erase size stuff */
-		int i;
+		int i, max = master->numeraseregions;
+		u32 end = slave->offset + slave->mtd.size;
 		struct mtd_erase_region_info *regions = master->eraseregions;
 
-		/* Find the first erase regions which is part of this partition. */
-		for (i = 0; i < master->numeraseregions && regions[i].offset <= slave->offset; i++)
+		/* Find the first erase regions which is part of this
+		 * partition. */
+		for (i = 0; i < max && regions[i].offset <= slave->offset; i++)
 			;
+		/* The loop searched for the region _behind_ the first one */
+		i--;
 
-		for (i--; i < master->numeraseregions && regions[i].offset < slave->offset + slave->mtd.size; i++) {
+		/* Pick biggest erasesize */
+		for (; i < max && regions[i].offset < end; i++) {
 			if (slave->mtd.erasesize < regions[i].erasesize) {
 				slave->mtd.erasesize = regions[i].erasesize;
 			}
 		}
+		BUG_ON(slave->mtd.erasesize == 0);
 	} else {
 		/* Single erase size */
 		slave->mtd.erasesize = master->erasesize;
-- 
1.5.3.5

Re: [PATCH 1/2] mtdpart: Avoid divide-by-zero on out-of-reach path

[Jörn Engel]

[PATCH 0/4] [MTD][MTDPART] Fix a division by zero bug

Spotted and debugged by Atsushi Nemoto <anemo@mba.ocn.ne.jp>

When detecting a partition beyond the end of the device, skip most of
the initialisation, in particular those bits causing a division by zero.

Signed-off-by: Joern Engel <joern@logfs.org>
---
 drivers/mtd/mtdpart.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index c1a8916..bd51dba 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -420,11 +420,12 @@ static int add_one_partition(struct mtd_info *master,
 
 	/* let's do some sanity checks */
 	if (slave->offset >= master->size) {
-			/* let's register it anyway to preserve ordering */
+		/* let's register it anyway to preserve ordering */
 		slave->offset = 0;
 		slave->mtd.size = 0;
 		printk(KERN_ERR"mtd: partition \"%s\" is out of reach -- disabled\n",
 			part->name);
+		goto out_register;
 	}
 	if (slave->offset + slave->mtd.size > master->size) {
 		slave->mtd.size = master->size - slave->offset;
@@ -484,6 +485,7 @@ static int add_one_partition(struct mtd_info *master,
 		}
 	}
 
+out_register:
 	if (part->mtdp) {
 		/* store the object pointer (caller may or may not register it*/
 		*part->mtdp = &slave->mtd;
-- 
1.5.3.5

Re: [PATCH 1/2] mtdpart: Avoid divide-by-zero on out-of-reach path

[Atsushi Nemoto]

On Wed, 18 Jun 2008 19:52:53 +0200, Jörn Engel <joern@logfs.org> wrote:
> I've had a quick go at it.  Three cleanup patches first, then the goto.
> Patches have been compile-tested, but not more.  Would these work?

Unfortunately, no.

> +		err = add_one_partition(master, parts + i, i, cur_offset);
> +		if (err)
> +			return err;
> +		slave = PART(parts[i].mtdp);
> +		cur_offset = slave->offset + slave->mtd.size;

This should be 'slave = PART(*parts[i].mtdp)', but anyway you cannot
dereference mtdp while it can be NULL.

Other parts of your patchset are very good for me.  Thanks.

This is my quick fix on top of your patchset.  Please fold this (or
your own fix) into first patch.

--- linux/drivers/mtd/mtdpart.c	2008-06-19 15:35:28.000000000 +0900
+++ linux/drivers/mtd/mtdpart.c	2008-06-19 15:36:43.000000000 +0900
@@ -324,7 +324,7 @@ int del_mtd_partitions(struct mtd_info *
 }
 EXPORT_SYMBOL(del_mtd_partitions);
 
-static int add_one_partition(struct mtd_info *master,
+static struct mtd_part *add_one_partition(struct mtd_info *master,
 		const struct mtd_partition *part, int partno,
 		u_int32_t cur_offset)
 {
@@ -336,7 +336,7 @@ static int add_one_partition(struct mtd_
 		printk(KERN_ERR"memory allocation error while creating partitions for \"%s\"\n",
 			master->name);
 		del_mtd_partitions(master);
-		return -ENOMEM;
+		return NULL;
 	}
 	list_add(&slave->list, &mtd_partitions);
 
@@ -495,7 +495,7 @@ out_register:
 		add_mtd_device(&slave->mtd);
 		slave->registered = 1;
 	}
-	return 0;
+	return slave;
 }
 
 /*
@@ -511,15 +511,14 @@ int add_mtd_partitions(struct mtd_info *
 {
 	struct mtd_part *slave;
 	u_int32_t cur_offset = 0;
-	int i, err;
+	int i;
 
 	printk(KERN_NOTICE "Creating %d MTD partitions on \"%s\":\n", nbparts, master->name);
 
 	for (i = 0; i < nbparts; i++) {
-		err = add_one_partition(master, parts + i, i, cur_offset);
-		if (err)
-			return err;
-		slave = PART(parts[i].mtdp);
+		slave = add_one_partition(master, parts + i, i, cur_offset);
+		if (!slave)
+			return -ENOMEM;
 		cur_offset = slave->offset + slave->mtd.size;
 	}
 

---
Atsushi Nemoto

Re: [PATCH 1/2] mtdpart: Avoid divide-by-zero on out-of-reach path

[Jörn Engel]

On Thu, 19 June 2008 16:09:38 +0900, Atsushi Nemoto wrote:
> On Wed, 18 Jun 2008 19:52:53 +0200, Jörn Engel <joern@logfs.org> wrote:
> 
> > +		err = add_one_partition(master, parts + i, i, cur_offset);
> > +		if (err)
> > +			return err;
> > +		slave = PART(parts[i].mtdp);
> > +		cur_offset = slave->offset + slave->mtd.size;
> 
> This should be 'slave = PART(*parts[i].mtdp)', but anyway you cannot
> dereference mtdp while it can be NULL.
> 
> Other parts of your patchset are very good for me.  Thanks.
> 
> This is my quick fix on top of your patchset.  Please fold this (or
> your own fix) into first patch.

Much better.  I was thinking about the same thing while doing the
patches, but for some reason forgot about it again.

Do you want me to push the patchset?  And would you be willing to add a
tested-by: or so?

Jörn

-- 
The key to performance is elegance, not battalions of special cases.
-- Jon Bentley and Doug McIlroy

Re: [PATCH 1/2] mtdpart: Avoid divide-by-zero on out-of-reach path

[Atsushi Nemoto]

On Thu, 19 Jun 2008 10:24:54 +0200, Jörn Engel <joern@logfs.org> wrote:
> > This is my quick fix on top of your patchset.  Please fold this (or
> > your own fix) into first patch.
> 
> Much better.  I was thinking about the same thing while doing the
> patches, but for some reason forgot about it again.
> 
> Do you want me to push the patchset?  And would you be willing to add a
> tested-by: or so?

Yes, please.

Tested-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>

---
Atsushi Nemoto

Re: [PATCH 1/2] mtdpart: Avoid divide-by-zero on out-of-reach path

[Atsushi Nemoto]

On Thu, 19 Jun 2008 17:34:58 +0900 (JST), Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:
> > Much better.  I was thinking about the same thing while doing the
> > patches, but for some reason forgot about it again.
> > 
> > Do you want me to push the patchset?  And would you be willing to add a
> > tested-by: or so?
> 
> Yes, please.
> 
> Tested-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>

Ping?  Should I send your patchset with my fix?

---
Atsushi Nemoto

Re: [PATCH 1/2] mtdpart: Avoid divide-by-zero on out-of-reach path

[Jörn Engel]

On Thu, 17 July 2008 00:10:23 +0900, Atsushi Nemoto wrote:
> 
> Ping?  Should I send your patchset with my fix?

Yes, please do.

Jörn

-- 
Fantasy is more important than knowledge. Knowledge is limited,
while fantasy embraces the whole world.
-- Albert Einstein

Re: [PATCH 1/2] mtdpart: Avoid divide-by-zero on out-of-reach path

[Atsushi Nemoto]

On Thu, 17 Jul 2008 16:55:46 +0200, Jörn Engel <joern@logfs.org> wrote:
> > Ping?  Should I send your patchset with my fix?
> 
> Yes, please do.

OK, I will do soon.
---
Atsushi Nemoto