[PATCH] [MTD] Correct partition failed erase address

[PATCH] [MTD] Correct partition failed erase address

[Adrian Hunter]

If an erase operation fails, the address at which the
failure occurred is returned by the driver.  The MTD
partition must adjust this address (by subtracting the
partition offset) before returning to the caller.
This was not happening, which caused JFFS2 to mark
the wrong block bad!

Signed-off-by: Adrian Hunter <ext-adrian.hunter@nokia.com>
---
 drivers/mtd/mtdpart.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 633def3..476fbb6 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -200,6 +200,11 @@ static int part_erase (struct mtd_info *
 		return -EINVAL;
 	instr->addr += part->offset;
 	ret = part->master->erase(part->master, instr);
+	if (ret) {
+		if (instr->fail_addr != 0xffffffff)
+			instr->fail_addr -= part->offset;
+		instr->addr -= part->offset;
+	}
 	return ret;
 }
 
@@ -559,4 +564,3 @@ EXPORT_SYMBOL_GPL(deregister_mtd_parser)
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Nicolas Pitre <nico@cam.org>");
 MODULE_DESCRIPTION("Generic support for partitioning of MTD devices");
-
-- 
1.4.3

Re: [PATCH] [MTD] Correct partition failed erase address

[David Woodhouse]

On Thu, 2007-03-08 at 12:20 +0200, Adrian Hunter wrote:
> If an erase operation fails, the address at which the
> failure occurred is returned by the driver.  The MTD
> partition must adjust this address (by subtracting the
> partition offset) before returning to the caller.
> This was not happening, which caused JFFS2 to mark
> the wrong block bad!
> 
> Signed-off-by: Adrian Hunter <ext-adrian.hunter@nokia.com>
> ---
>  drivers/mtd/mtdpart.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index 633def3..476fbb6 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -200,6 +200,11 @@ static int part_erase (struct mtd_info *
>  		return -EINVAL;
>  	instr->addr += part->offset;
>  	ret = part->master->erase(part->master, instr);
> +	if (ret) {
> +		if (instr->fail_addr != 0xffffffff)
> +			instr->fail_addr -= part->offset;
> +		instr->addr -= part->offset;
> +	}
>  	return ret;
>  }
 
Erase isn't always synchronous. It can return success (i.e. erase queued
successfully) and later call the callback with an indication of failure.

-- 
dwmw2

Re: [PATCH] [MTD] Correct partition failed erase address

[David Woodhouse]

On Thu, 2007-03-08 at 10:27 +0000, David Woodhouse wrote:
> 
> Erase isn't always synchronous. It can return success (i.e. erase queued
> successfully) and later call the callback with an indication of failure. 

... and mtd_erase_callback() already subtracts the necessary offset.

I really don't like our API very much, but it doesn't seem to be broken
in this particular way.

When we fix things to cope with flash larger than 4GiB, I think we
should make larger changes to the way that partitioning works as well as
reconsidering the read/write/erase API.

-- 
dwmw2

Re: [PATCH] [MTD] Correct partition failed erase address

[David Woodhouse]

On Thu, 2007-03-08 at 12:36 +0200, Adrian Hunter wrote:
> ext David Woodhouse wrote:
> > On Thu, 2007-03-08 at 10:27 +0000, David Woodhouse wrote:
> >> Erase isn't always synchronous. It can return success (i.e. erase queued
> >> successfully) and later call the callback with an indication of failure. 
> > 
> > ... and mtd_erase_callback() already subtracts the necessary offset.
> > 
> > I really don't like our API very much, but it doesn't seem to be broken
> > in this particular way.
> 
> Yes it is broken. Both when the callback is not used at all, and when 
> an error occurs immediately and the callback is not called.

Ew, you're right -- and the API is worse than I thought.

Sorry (on both counts) :)

-- 
dwmw2

Re: [PATCH] [MTD] Correct partition failed erase address

[Jörn Engel]

On Thu, 8 March 2007 10:43:13 +0000, David Woodhouse wrote:
> 
> Ew, you're right -- and the API is worse than I thought.

And the API is _never_ used asynchronously.  For current drivers, it
is pure overhead.

Jörn

-- 
Joern's library part 1:
http://lwn.net/Articles/2.6-kernel-api/