[U-Boot-Users] PATCH - Fix oob data copied into supplied buffer
This patch correctly sets the oobavail variable and fixes a bug where the oob data was not valid when there where multiple groups in oobfree. It also defines a new variable so that you can tell which parts of a large page have ecc errors.
Signed-off-by: Troy Kisky troy.kisky@boundarydevices.com
diff --git a/drivers/nand/nand_base.c b/drivers/nand/nand_base.c index 151f535..45b8fb0 100644 --- a/drivers/nand/nand_base.c +++ b/drivers/nand/nand_base.c @@ -931,7 +931,7 @@ static int nand_write_page (struct mtd_info *mtd, struct nand_chip *this, int pa for (i = 0; i < eccbytes; i++, eccidx++) oob_buf[oob_config[eccidx]] = ecc_code[i]; /* If the hardware ecc provides syndromes then - * the ecc code must be written immidiately after + * the ecc code must be written immediately after * the data bytes (words) */ if (this->options & NAND_HWECC_SYNDROME) this->write_buf(mtd, ecc_code, eccbytes); @@ -1092,7 +1092,7 @@ static int nand_read (struct mtd_info *mtd, loff_t from, size_t len, size_t * re return nand_read_ecc (mtd, from, len, retlen, buf, NULL, NULL); }
- +ulong eccReadMask; /* bitmask off ecc err groups*/ /** * nand_read_ecc - [MTD Interface] Read data with ECC * @mtd: MTD device structure @@ -1267,6 +1267,7 @@ static int nand_read_ecc (struct mtd_info *mtd, loff_t from, size_t len, ecc_code[j] = oob_data[oob_config[j]];
/* correct data, if neccecary */ + eccReadMask = 0; for (i = 0, j = 0, datidx = 0; i < this->eccsteps; i++, datidx += ecc) { ecc_status = this->correct_data(mtd, &data_poi[datidx], &ecc_code[j], &ecc_calc[j]);
@@ -1283,6 +1284,7 @@ static int nand_read_ecc (struct mtd_info *mtd, loff_t from, size_t len, }
if (ecc_status == -1) { + eccReadMask |= (1<<i); DEBUG (MTD_DEBUG_LEVEL0, "nand_read_ecc: " "Failed ECC read, page 0x%08x\n", page); ecc_failed++; } @@ -1299,7 +1301,7 @@ static int nand_read_ecc (struct mtd_info *mtd, loff_t from, size_t len, for (i = 0, j = 0; j < mtd->oobavail; i++) { int from = oobsel->oobfree[i][0]; int num = oobsel->oobfree[i][1]; - memcpy(&oob_buf[oob], &oob_data[from], num); + memcpy(&oob_buf[oob+j], &oob_data[from], num); j+= num; } oob += mtd->oobavail; @@ -1644,8 +1646,10 @@ static int nand_write_ecc (struct mtd_info *mtd, loff_t to, size_t len, this->select_chip(mtd, chipnr);
/* Check, if it is write protected */ - if (nand_check_wp(mtd)) + if (nand_check_wp(mtd)) { + printk (KERN_NOTICE "nand_write_ecc: Device is write protected\n"); goto out; + }
/* if oobsel is NULL, use chip defaults */ if (oobsel == NULL) @@ -2486,12 +2490,9 @@ int nand_scan (struct mtd_info *mtd, int maxchips)
/* The number of bytes available for the filesystem to place fs dependend * oob data */ - if (this->options & NAND_BUSWIDTH_16) { - mtd->oobavail = mtd->oobsize - (this->autooob->eccbytes + 2); - if (this->autooob->eccbytes & 0x01) - mtd->oobavail--; - } else - mtd->oobavail = mtd->oobsize - (this->autooob->eccbytes + 1); + mtd->oobavail = 0; + for (i=0; this->autooob->oobfree[i][1]; i++) + mtd->oobavail += this->autooob->oobfree[i][1];
/* * check ECC mode, default to software
On Thu, 20 Sep 2007, Troy Kisky wrote:
This doesn't look right to me.
First chunk fixes a minor spelling error. Then I can't understand why introduce a new variable. Everything works fine with existing structures and it's been like this for ages, both in Linux kernel and U-Boot.
This patch correctly sets the oobavail variable and fixes a bug where the oob data was not valid when there where multiple groups in oobfree. It also defines a new variable so that you can tell which parts of a large page have ecc errors.
Signed-off-by: Troy Kisky troy.kisky@boundarydevices.com
diff --git a/drivers/nand/nand_base.c b/drivers/nand/nand_base.c index 151f535..45b8fb0 100644 --- a/drivers/nand/nand_base.c +++ b/drivers/nand/nand_base.c @@ -931,7 +931,7 @@ static int nand_write_page (struct mtd_info *mtd, struct nand_chip *this, int pa for (i = 0; i < eccbytes; i++, eccidx++) oob_buf[oob_config[eccidx]] = ecc_code[i]; /* If the hardware ecc provides syndromes then
* the ecc code must be written immidiatelyafter
* the ecc code must be written immediatelyafter * the data bytes (words) */ if (this->options & NAND_HWECC_SYNDROME) this->write_buf(mtd, ecc_code, eccbytes); @@ -1092,7 +1092,7 @@ static int nand_read (struct mtd_info *mtd, loff_t from, size_t len, size_t * re return nand_read_ecc (mtd, from, len, retlen, buf, NULL, NULL); }
+ulong eccReadMask; /* bitmask off ecc err groups*/ /**
- nand_read_ecc - [MTD Interface] Read data with ECC
- @mtd: MTD device structure
@@ -1267,6 +1267,7 @@ static int nand_read_ecc (struct mtd_info *mtd, loff_t from, size_t len, ecc_code[j] = oob_data[oob_config[j]];
/* correct data, if neccecary */
eccReadMask = 0;datidx += ecc) { ecc_status = this->correct_data(mtd, &data_poi[datidx], &ecc_code[j], &ecc_calc[j]);
@@ -1283,6 +1284,7 @@ static int nand_read_ecc (struct mtd_info *mtd, loff_t from, size_t len, }
if (ecc_status == -1) {
eccReadMask |= (1<<i); DEBUG (MTD_DEBUG_LEVEL0, "nand_read_ecc:" "Failed ECC read, page 0x%08x\n", page); ecc_failed++; } @@ -1299,7 +1301,7 @@ static int nand_read_ecc (struct mtd_info *mtd, loff_t from, size_t len, for (i = 0, j = 0; j < mtd->oobavail; i++) { int from = oobsel->oobfree[i][0]; int num = oobsel->oobfree[i][1];
memcpy(&oob_buf[oob],&oob_data[from], num);
memcpy(&oob_buf[oob+j],&oob_data[from], num); j+= num; } oob += mtd->oobavail; @@ -1644,8 +1646,10 @@ static int nand_write_ecc (struct mtd_info *mtd, loff_t to, size_t len, this->select_chip(mtd, chipnr);
/* Check, if it is write protected */
- if (nand_check_wp(mtd))
- if (nand_check_wp(mtd)) {
printk (KERN_NOTICE "nand_write_ecc: Device is writeprotected\n"); goto out;
}
/* if oobsel is NULL, use chip defaults */ if (oobsel == NULL)
@@ -2486,12 +2490,9 @@ int nand_scan (struct mtd_info *mtd, int maxchips)
/* The number of bytes available for the filesystem to place fs dependend * oob data */
- if (this->options & NAND_BUSWIDTH_16) {
mtd->oobavail = mtd->oobsize - (this->autooob->eccbytes
- 2);
if (this->autooob->eccbytes & 0x01)mtd->oobavail--;- } else
mtd->oobavail = mtd->oobsize - (this->autooob->eccbytes
1);
mtd->oobavail = 0;
for (i=0; this->autooob->oobfree[i][1]; i++)
mtd->oobavail += this->autooob->oobfree[i][1];/*
- check ECC mode, default to software
This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2005. http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/ _______________________________________________ U-Boot-Users mailing list U-Boot-Users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/u-boot-users
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************
ksi@koi8.net wrote:
On Thu, 20 Sep 2007, Troy Kisky wrote:
This doesn't look right to me.
First chunk fixes a minor spelling error. Then I can't understand why introduce a new variable. Everything works fine with existing structures and it's been like this for ages, both in Linux kernel and U-Boot.
If you look at the kernel, you'll see that oobavail is set there the way I did it. The other bug doesn't show up unless you have multiple oobfree groups. The new variable is used when reading or writing pages that an initial program loader reads. It's purpose is to correct pages without using a bad block table. I'll submit that code if this is approved.
Troy
In message 46F3294C.2030206@boundarydevices.com you wrote:
First chunk fixes a minor spelling error. Then I can't understand why introduce a new variable. Everything works fine with existing structures and it's been like this for ages, both in Linux kernel and U-Boot.
If you look at the kernel, you'll see that oobavail is set there the way I did it. The other bug doesn't show up unless you have multiple oobfree groups. The new variable is used when reading or writing pages that an initial program loader reads. It's purpose is to correct pages without using a bad block table. I'll submit that code if this is approved.
kis's comments were legitimate.
Please add this variable only when adding code that uses it. I will always reject patches that attempt to add unused code or variables.
Also, please describe *all* your changes.
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
In message 46F3294C.2030206@boundarydevices.com you wrote:
First chunk fixes a minor spelling error. Then I can't understand why introduce a new variable. Everything works fine with existing structures and it's been like this for ages, both in Linux kernel and U-Boot.
If you look at the kernel, you'll see that oobavail is set there the way I did it. The other bug doesn't show up unless you have multiple oobfree groups. The new variable is used when reading or writing pages that an initial program loader reads. It's purpose is to correct pages without using a bad block table. I'll submit that code if this is approved.
kis's comments were legitimate.
Please add this variable only when adding code that uses it. I will always reject patches that attempt to add unused code or variables.
I should have known that, guess I have a lazy streak in me.
Also, please describe *all* your changes.
Sorry, I didn't realize that you wanted typo fixes, and error messages described.
Best regards,
Wolfgang Denk
First segment fixes a typo Second segment fixes a bug where oob data may be copied incorrectly. Third segment adds an error message when exiting due to write protect. Forth segment fixes a bug where oobavail may be set incorrectly.
All segments are independent and may be accepted or rejected as deemed appropriate. Do they need to be separate patches? Signed-off-by: Troy Kisky troy.kisky@boundarydevices.com
diff --git a/drivers/nand/nand_base.c b/drivers/nand/nand_base.c index 151f535..2da1d46 100644 --- a/drivers/nand/nand_base.c +++ b/drivers/nand/nand_base.c @@ -931,7 +931,7 @@ static int nand_write_page (struct mtd_info *mtd, struct nand_chip *this, int pa for (i = 0; i < eccbytes; i++, eccidx++) oob_buf[oob_config[eccidx]] = ecc_code[i]; /* If the hardware ecc provides syndromes then - * the ecc code must be written immidiately after + * the ecc code must be written immediately after * the data bytes (words) */ if (this->options & NAND_HWECC_SYNDROME) this->write_buf(mtd, ecc_code, eccbytes); @@ -1299,7 +1299,7 @@ static int nand_read_ecc (struct mtd_info *mtd, loff_t from, size_t len, for (i = 0, j = 0; j < mtd->oobavail; i++) { int from = oobsel->oobfree[i][0]; int num = oobsel->oobfree[i][1]; - memcpy(&oob_buf[oob], &oob_data[from], num); + memcpy(&oob_buf[oob+j], &oob_data[from], num); j+= num; } oob += mtd->oobavail; @@ -1644,8 +1644,10 @@ static int nand_write_ecc (struct mtd_info *mtd, loff_t to, size_t len, this->select_chip(mtd, chipnr);
/* Check, if it is write protected */ - if (nand_check_wp(mtd)) + if (nand_check_wp(mtd)) { + printk (KERN_NOTICE "nand_write_ecc: Device is write protected\n"); goto out; + }
/* if oobsel is NULL, use chip defaults */ if (oobsel == NULL) @@ -2486,12 +2488,9 @@ int nand_scan (struct mtd_info *mtd, int maxchips)
/* The number of bytes available for the filesystem to place fs dependend * oob data */ - if (this->options & NAND_BUSWIDTH_16) { - mtd->oobavail = mtd->oobsize - (this->autooob->eccbytes + 2); - if (this->autooob->eccbytes & 0x01) - mtd->oobavail--; - } else - mtd->oobavail = mtd->oobsize - (this->autooob->eccbytes + 1); + mtd->oobavail = 0; + for (i=0; this->autooob->oobfree[i][1]; i++) + mtd->oobavail += this->autooob->oobfree[i][1];
/* * check ECC mode, default to software
On Tuesday 25 September 2007, Troy Kisky wrote:
First segment fixes a typo Second segment fixes a bug where oob data may be copied incorrectly. Third segment adds an error message when exiting due to write protect. Forth segment fixes a bug where oobavail may be set incorrectly.
All segments are independent and may be accepted or rejected as deemed appropriate. Do they need to be separate patches? Signed-off-by: Troy Kisky troy.kisky@boundarydevices.com
Sorry, I missed this for a long time and Wolfgang just reminded me of it.
Added to u-boot-nand-flash repo. Thanks.
Best regards, Stefan
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de =====================================================================
participants (4)
-
ksi＠koi8.net -
Stefan Roese -
Troy Kisky -
Wolfgang Denk