Skip to contentSkip to conversation

MTD: treat any negative return value from correct() as an error

PATCH In review [email protected] 4 participants 12 messages Oct 17, 2007 – Oct 21, 2007

Subsystems (2)
Memory technology devices (MTD) NAND flash
lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 1/1 MTD: treat any negative return value from correct() as an error

From: Matt Reimer <[email protected]>

Treat any negative return value from a NAND driver's correct() function as a failure, rather than just -1. Some drivers (e.g. diskonchip) can return error values such as -EIO, which ended up being treated as the number of bits corrected, which in turn resulted in nand_do_read_ops() returning -EUCLEAN rather than -EBADMSG.

Signed-off-by Matt Reimer <[email protected]>
---
 drivers/mtd/nand/nand_base.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
drivers/mtd/nand/nand_base.c +3 -3
index 24ac677..6d40edf 100644
@@ -789,7 +789,7 @@ static int nand_read_page_swecc(struct mtd_info *mtd, struct nand_chip *chip,
 		int stat;
 
 		stat = chip->ecc.correct(mtd, p, &ecc_code[i], &ecc_calc[i]);
-		if (stat == -1)
+		if (stat < 0)
 			mtd->ecc_stats.failed++;
 		else
 			mtd->ecc_stats.corrected += stat;

@@ -833,7 +833,7 @@ static int nand_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip,
 		int stat;
 
 		stat = chip->ecc.correct(mtd, p, &ecc_code[i], &ecc_calc[i]);
-		if (stat == -1)
+		if (stat < 0)
 			mtd->ecc_stats.failed++;
 		else
 			mtd->ecc_stats.corrected += stat;

@@ -874,7 +874,7 @@ static int nand_read_page_syndrome(struct mtd_info *mtd, struct nand_chip *chip,
 		chip->read_buf(mtd, oob, eccbytes);
 		stat = chip->ecc.correct(mtd, p, oob, NULL);
 
-		if (stat == -1)
+		if (stat < 0)
 			mtd->ecc_stats.failed++;
 		else
 			mtd->ecc_stats.corrected += stat;
Signature · 1 line
1.5.3.2
#1 lore raw
Quoted 6 lines · On Wed, 17 October 2007 14:33:23 -0700, [email protected] wrote:

Treat any negative return value from a NAND driver's correct() function as a failure, rather than just -1. Some drivers (e.g. diskonchip) can return error values such as -EIO, which ended up being treated as the number of bits corrected, which in turn resulted in nand_do_read_ops() returning -EUCLEAN rather than -EBADMSG.

What an interesting patch.

NACK on patch description. Several things are wrong with this:
o -EIO shouldn't become -EBADMSG any more than -EUCLEAN. It should remain -EIO and nothing else.
o I cannot see how diskonchip can return -EIO. Only -ERANGE.
o rtc_from4_correct_data() and doc200x_correct_data() will basically pass on -ERANGE from decode_rs8() or decode_rs16(), respectively. Afaict decode_rsXX() should just BUG() instead.

However I like the patch itself, because it allows one to rename those magic -1 return values to self-documenting -EBADMSG.

Jörn

Signature · 3 lines
It is the mark of an educated mind to be able to entertain a thought
without accepting it.
-- Aristotle
#2 lore raw

PATCH 1/1 BUG() when passing illegal parameters to decode_rs8() or decode_rs16()

Returning -ERANGE should never happen.

lib/reed_solomon/decode_rs.c renamed from lib/reed_solomon/decode_rs.c~rs_ERANGE +1 -2
@@ -39,8 +39,7 @@
 
 	/* Check length parameter for validity */
 	pad = nn - nroots - len;
-	if (pad < 0 || pad >= nn)
-		return -ERANGE;
+	BUG_ON(pad < 0 || pad >= nn);
 
 	/* Does the caller provide the syndrome ? */
 	if (s != NULL)
#3 lore raw

PATCH 1/1 Replace -1 with -EBADMSG in nand error correction code

Magic numerical values are just bad style. Particularly so when undocumented.

lib/reed_solomon/decode_rs.c renamed from lib/reed_solomon/decode_rs.c~nand_minusone +1 -1
@@ -202,7 +202,7 @@
 		 * deg(lambda) unequal to number of roots => uncorrectable
 		 * error detected
 		 */
-		count = -1;
+		count = -EBADMSG;
 		goto finish;
 	}
 	/*
lib/reed_solomon/reed_solomon.c renamed from lib/reed_solomon/reed_solomon.c~rs_ERANGE +2 -0
@@ -320,6 +320,7 @@ EXPORT_SYMBOL_GPL(encode_rs8);
  *  The syndrome and parity uses a uint16_t data type to enable
  *  symbol size > 8. The calling code must take care of decoding of the
  *  syndrome result and the received parity before calling this code.
+ *  Returns the number of corrected bits or -EBADMSG for uncorrectable errors.
  */
 int decode_rs8(struct rs_control *rs, uint8_t *data, uint16_t *par, int len,
 	       uint16_t *s, int no_eras, int *eras_pos, uint16_t invmsk,

@@ -363,6 +364,7 @@ EXPORT_SYMBOL_GPL(encode_rs16);
  *  @corr:	buffer to store correction bitmask on eras_pos
  *
  *  Each field in the data array contains up to symbol size bits of valid data.
+ *  Returns the number of corrected bits or -EBADMSG for uncorrectable errors.
  */
 int decode_rs16(struct rs_control *rs, uint16_t *data, uint16_t *par, int len,
 		uint16_t *s, int no_eras, int *eras_pos, uint16_t invmsk,
drivers/mtd/nand/diskonchip.c renamed from drivers/mtd/nand/diskonchip.c~nand_minusone +2 -2
@@ -225,7 +225,7 @@ static int doc_ecc_decode(struct rs_cont
 		}
 	}
 	/* If the parity is wrong, no rescue possible */
-	return parity ? -1 : nerr;
+	return parity ? -EBADMSG : nerr;
 }
 
 static void DoC_Delay(struct doc_priv *doc, unsigned short cycles)

@@ -1039,7 +1039,7 @@ static int doc200x_correct_data(struct m
 		WriteDOC(DOC_ECC_DIS, docptr, Mplus_ECCConf);
 	else
 		WriteDOC(DOC_ECC_DIS, docptr, ECCConf);
-	if (no_ecc_failures && (ret == -1)) {
+	if (no_ecc_failures && (ret == -EBADMSG)) {
 		printk(KERN_ERR "suppressing ECC failure\n");
 		ret = 0;
 	}
drivers/mtd/nand/nand_ecc.c renamed from drivers/mtd/nand/nand_ecc.c~nand_minusone +1 -1
@@ -189,7 +189,7 @@ int nand_correct_data(struct mtd_info *m
 	if(countbits(s0 | ((uint32_t)s1 << 8) | ((uint32_t)s2 <<16)) == 1)
 		return 1;
 
-	return -1;
+	return -EBADMSG;
 }
 EXPORT_SYMBOL(nand_correct_data);
#4 lore raw

Re: [PATCH] Replace -1 with -EBADMSG in nand error correction code

Quoted 2 lines · Jörn Engel

Magic numerical values are just bad style. Particularly so when undocumented.

Please resend with a proper Signed-off-by.
Please add my Acked-by as well.

Thanks,

tglx

#5 lore raw

Re: [PATCH] Replace -1 with -EBADMSG in nand error correction code

Quoted 7 lines · Thomas Gleixner
Quoted 2 lines · Jörn Engel

Magic numerical values are just bad style. Particularly so when undocumented.

Please resend with a proper Signed-off-by.
Please add my Acked-by as well.

Already resent privately to dwmw2 (just added a Signed-Off-By: didn't seem to justify a huge Cc: list). He merged it.

Jörn

Signature · 2 lines
Eighty percent of success is showing up.
-- Woody Allen
#6 lore raw

Re: [PATCH] BUG() when passing illegal parameters to decode_rs8() or decode_rs16()

In reply to Jörn Engel

lib/reed_solomon/decode_rs.c line 42

Quoted 11 lines · Jörn Engel

Returning -ERANGE should never happen.

lib/reed_solomon/decode_rs.c renamed from lib/reed_solomon/decode_rs.c~rs_ERANGE +1 -2
@@ -39,8 +39,7 @@
 
 	/* Check length parameter for validity */
 	pad = nn - nroots - len;
-	if (pad < 0 || pad >= nn)
-		return -ERANGE;
+	BUG_ON(pad < 0 || pad >= nn);

No. This is not a critical error. You escalate a situation where the kernel can still survive to a fatal error.

tglx

#7 lore raw

Re: [PATCH] BUG() when passing illegal parameters to decode_rs8() or decode_rs16()

In reply to Jörn Engel

lib/reed_solomon/decode_rs.c line 42

Quoted 11 lines · Jörn Engel

Returning -ERANGE should never happen.

lib/reed_solomon/decode_rs.c renamed from lib/reed_solomon/decode_rs.c~rs_ERANGE +1 -2
@@ -39,8 +39,7 @@
 
 	/* Check length parameter for validity */
 	pad = nn - nroots - len;
-	if (pad < 0 || pad >= nn)
-		return -ERANGE;
+	BUG_ON(pad < 0 || pad >= nn);

No. It should never happen, but escalating an error which the kernel can survive under a lot of circumstances to a fatal death error is wrong.

Thanks,

tglx

#8 lore raw

Re: [PATCH] BUG() when passing illegal parameters to decode_rs8() or decode_rs16()

Quoted 4 lines · Thomas Gleixner

No. It should never happen, but escalating an error which the kernel can survive under a lot of circumstances to a fatal death error is wrong.

I am unimpressed. This error is not some rare hardware error, it is a programming error, a bug in the kernel. And until now it was handled by hiding it as a _corrected_ crc error. If you want to handle it in a non-fatal way, you can do it. But that requires more than just reverting this patch - you should also properly document why an "impossible" error is indeed possible and how calling code should deal with it. And of course make sure current code follows documentation and does the right thing.

Is all that worth it? I have serious doubts.

Jörn

Signature · 3 lines
Never argue with idiots - first they drag you down to their level,
then they beat you with experience.
-- unknown
author
#9 lore raw
In reply to Jörn Engel
Quoted 8 lines · Jörn Engel
Quoted 6 lines · On Wed, 17 October 2007 14:33:23 -0700, [email protected] wrote:

Treat any negative return value from a NAND driver's correct() function as a failure, rather than just -1. Some drivers (e.g. diskonchip) can return error values such as -EIO, which ended up being treated as the number of bits corrected, which in turn resulted in nand_do_read_ops() returning -EUCLEAN rather than -EBADMSG.

Quoted 5 lines

What an interesting patch.

NACK on patch description. Several things are wrong with this:
o -EIO shouldn't become -EBADMSG any more than -EUCLEAN. It should remain -EIO and nothing else.

My mistake: I did most of this analysis in the 2.6.21 code, where cafe_ecc.c:cafe_correct_ecc() did return -EIO, not diskonchip.

But as for the rest of the description, I'm just reading the code that's there. An error return from ecc.correct bumps stats.failed, which in nand_do_read_ops does become -EBADMSG:

        if (mtd->ecc_stats.failed - stats.failed)
                return -EBADMSG;

        return  mtd->ecc_stats.corrected - stats.corrected ? -EUCLEAN : 0;

So I was just following what I found there, assuming it was standard practice.

Quoted 1 line

o I cannot see how diskonchip can return -EIO. Only -ERANGE.

Again, my bad; I was confusing it with an earlier version of cafe_ecc.c (which I see now didn't pass -EIO up all the way).

Matt

#10 lore raw
Quoted 8 lines · Matt Reimer

My mistake: I did most of this analysis in the 2.6.21 code, where cafe_ecc.c:cafe_correct_ecc() did return -EIO, not diskonchip.

But as for the rest of the description, I'm just reading the code that's there. An error return from ecc.correct bumps stats.failed, which in nand_do_read_ops does become -EBADMSG:

Quoted 6 lines
        if (mtd->ecc_stats.failed - stats.failed)
                return -EBADMSG;

        return  mtd->ecc_stats.corrected - stats.corrected ? -EUCLEAN : 0;

So I was just following what I found there, assuming it was standard practice.

Fair enough. Description aside, your patch, my two, plus your patch to return -1 on uncorrectable ecc errors instead of 0 should be turned into a patch series. Better yet, change your patch to return -EBADMSG.

Do you volunteer or should I do it?

Jörn

Signature · 2 lines
Data expands to fill the space available for storage.
-- Parkinson's Law
#11 lore raw
Quoted 14 lines · Jörn Engel
Quoted 13 lines · Matt Reimer

My mistake: I did most of this analysis in the 2.6.21 code, where cafe_ecc.c:cafe_correct_ecc() did return -EIO, not diskonchip.

But as for the rest of the description, I'm just reading the code that's there. An error return from ecc.correct bumps stats.failed, which in nand_do_read_ops does become -EBADMSG:

        if (mtd->ecc_stats.failed - stats.failed)
                return -EBADMSG;

        return  mtd->ecc_stats.corrected - stats.corrected ? -EUCLEAN : 0;
Quoted 7 lines
Quoted 1 line

So I was just following what I found there, assuming it was standard practice.

Fair enough. Description aside, your patch, my two, plus your patch to return -1 on uncorrectable ecc errors instead of 0 should be turned into a patch series. Better yet, change your patch to return -EBADMSG.

Do you volunteer or should I do it?

I'll do it, if the next couple of days is soon enough. I'd rather keep you free to work on LogFS. :-)

Matt