Linux modified the MTD driver interface in commit edbc4540 (with the same name as this commit). The effect is that calls to mtd_read will not return -EUCLEAN if the number of ECC-corrected bit errors is below a certain threshold, which defaults to the strength of the ECC. This allows -EUCLEAN to stop indicating "some bits were corrected" and begin indicating "a large number of bits were corrected, the data held in this region of flash may be lost soon". UBI makes use of this and when -EUCLEAN is returned from mtd_read it will move data to another block of flash. Without adopting this interface change UBI on U-boot attempts to move data between blocks every time a single bit is corrected using the ECC, which is a very common occurance on some devices.
For some devices where bit errors are common enough, UBI can get stuck constantly moving data around because each block it attempts to use has a single bit error. This condition is hit when wear_leveling_worker attempts to move data from one PEB to another in response to an -EUCLEAN/UBI_IO_BITFLIPS error. When this happens ubi_eba_copy_leb is called to perform the data copy, and after the data is written it is read back to check its validity. If that read returns UBI_IO_BITFLIPS (in response to an MTD -EUCLEAN) then ubi_eba_copy_leb returns 1 to wear_leveling worker, which then proceeds to schedule the destination PEB for erasure. This leads to erase_worker running on the PEB, and following a successful erase wear_leveling_worker is called which begins this whole cycle all over again. The end result is that (without UBI debug output enabled) the boot appears to simply hang whilst in reality U-boot busily works away at destroying a block of the NAND flash. Debug output from this situation:
UBI DBG: ensure_wear_leveling: schedule scrubbing UBI DBG: wear_leveling_worker: scrub PEB 1027 to PEB 4083 UBI DBG: ubi_io_read_vid_hdr: read VID header from PEB 1027 UBI DBG: ubi_io_read: read 4096 bytes from PEB 1027:4096 UBI DBG: ubi_eba_copy_leb: copy LEB 0:0, PEB 1027 to PEB 4083 UBI DBG: ubi_eba_copy_leb: read 1040384 bytes of data UBI DBG: ubi_io_read: read 1040384 bytes from PEB 1027:8192 UBI: fixable bit-flip detected at PEB 1027 UBI DBG: ubi_io_write_vid_hdr: write VID header to PEB 4083 UBI DBG: ubi_io_write: write 4096 bytes to PEB 4083:4096 UBI DBG: ubi_io_read_vid_hdr: read VID header from PEB 4083 UBI DBG: ubi_io_read: read 4096 bytes from PEB 4083:4096 UBI DBG: ubi_io_write: write 4096 bytes to PEB 4083:8192 UBI DBG: ubi_io_read: read 4096 bytes from PEB 4083:8192 UBI: fixable bit-flip detected at PEB 4083 UBI DBG: schedule_erase: schedule erasure of PEB 4083, EC 55, torture 0 UBI DBG: erase_worker: erase PEB 4083 EC 55 UBI DBG: sync_erase: erase PEB 4083, old EC 55 UBI DBG: do_sync_erase: erase PEB 4083 UBI DBG: sync_erase: erased PEB 4083, new EC 56 UBI DBG: ubi_io_write_ec_hdr: write EC header to PEB 4083 UBI DBG: ubi_io_write: write 4096 bytes to PEB 4083:0 UBI DBG: ensure_wear_leveling: schedule scrubbing UBI DBG: wear_leveling_worker: scrub PEB 1027 to PEB 4083 ...
This patch adopts the interface change as in Linux commit edbc4540 in order to avoid such situations. Given that none of the drivers under drivers/mtd return -EUCLEAN, this should only affect those using software ECC. I have tested that it works on a board which is currently out of tree, but which I hope to be able to begin upstreaming soon.
Signed-off-by: Paul Burton paul.burton@imgtec.com --- Changes from v1: - Expanded upon the condition this fixes in the commit message. --- drivers/mtd/mtdcore.c | 14 +++++++++++++- drivers/mtd/mtdpart.c | 12 ++++++------ drivers/mtd/nand/nand_base.c | 18 ++++++++++++++---- drivers/mtd/onenand/onenand_base.c | 3 ++- include/linux/mtd/nand.h | 3 +++ 5 files changed, 38 insertions(+), 12 deletions(-)
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index 49c0814..deda5f2 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -217,11 +217,23 @@ int mtd_erase(struct mtd_info *mtd, struct erase_info *instr) int mtd_read(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen, u_char *buf) { + int ret_code; if (from < 0 || from > mtd->size || len > mtd->size - from) return -EINVAL; if (!len) return 0; - return mtd->_read(mtd, from, len, retlen, buf); + + /* + * In the absence of an error, drivers return a non-negative integer + * representing the maximum number of bitflips that were corrected on + * any one ecc region (if applicable; zero otherwise). + */ + ret_code = mtd->_read(mtd, from, len, retlen, buf); + if (unlikely(ret_code < 0)) + return ret_code; + if (mtd->ecc_strength == 0) + return 0; /* device lacks ecc */ + return ret_code >= mtd->bitflip_threshold ? -EUCLEAN : 0; }
int mtd_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen, diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c index 9dfe7bb..146ce11 100644 --- a/drivers/mtd/mtdpart.c +++ b/drivers/mtd/mtdpart.c @@ -53,12 +53,12 @@ static int part_read(struct mtd_info *mtd, loff_t from, size_t len,
stats = part->master->ecc_stats; res = mtd_read(part->master, from + part->offset, len, retlen, buf); - if (unlikely(res)) { - if (mtd_is_bitflip(res)) - mtd->ecc_stats.corrected += part->master->ecc_stats.corrected - stats.corrected; - if (mtd_is_eccerr(res)) - mtd->ecc_stats.failed += part->master->ecc_stats.failed - stats.failed; - } + if (unlikely(mtd_is_eccerr(res))) + mtd->ecc_stats.failed += + part->master->ecc_stats.failed - stats.failed; + else + mtd->ecc_stats.corrected += + part->master->ecc_stats.corrected - stats.corrected; return res; }
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index 9e05cef..d4d586c 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -1238,6 +1238,7 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from, mtd->oobavail : mtd->oobsize;
uint8_t *bufpoi, *oob, *buf; + unsigned int max_bitflips = 0;
stats = mtd->ecc_stats;
@@ -1265,7 +1266,10 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page);
- /* Now read the page into the buffer */ + /* + * Now read the page into the buffer. Absent an error, + * the read methods return max bitflips per ecc step. + */ if (unlikely(ops->mode == MTD_OPS_RAW)) ret = chip->ecc.read_page_raw(mtd, chip, bufpoi, oob_required, @@ -1284,15 +1288,19 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from, break; }
+ max_bitflips = max_t(unsigned int, max_bitflips, ret); + /* Transfer not aligned data */ if (!aligned) { if (!NAND_HAS_SUBPAGE_READ(chip) && !oob && !(mtd->ecc_stats.failed - stats.failed) && - (ops->mode != MTD_OPS_RAW)) + (ops->mode != MTD_OPS_RAW)) { chip->pagebuf = realpage; - else + chip->pagebuf_bitflips = ret; + } else { /* Invalidate page cache */ chip->pagebuf = -1; + } memcpy(buf, chip->buffers->databuf + col, bytes); }
@@ -1310,6 +1318,8 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from, } else { memcpy(buf, chip->buffers->databuf + col, bytes); buf += bytes; + max_bitflips = max_t(unsigned int, max_bitflips, + chip->pagebuf_bitflips); }
readlen -= bytes; @@ -1341,7 +1351,7 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from, if (mtd->ecc_stats.failed - stats.failed) return -EBADMSG;
- return mtd->ecc_stats.corrected - stats.corrected ? -EUCLEAN : 0; + return max_bitflips; }
/** diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c index ddfe7e7..067f8ef 100644 --- a/drivers/mtd/onenand/onenand_base.c +++ b/drivers/mtd/onenand/onenand_base.c @@ -969,7 +969,8 @@ static int onenand_read_ops_nolock(struct mtd_info *mtd, loff_t from, if (mtd->ecc_stats.failed - stats.failed) return -EBADMSG;
- return mtd->ecc_stats.corrected - stats.corrected ? -EUCLEAN : 0; + /* return max bitflips per ecc step; ONENANDs correct 1 bit only */ + return mtd->ecc_stats.corrected != stats.corrected ? 1 : 0; }
/** diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h index 2055584..0546565 100644 --- a/include/linux/mtd/nand.h +++ b/include/linux/mtd/nand.h @@ -464,6 +464,8 @@ struct nand_buffers { * @pagemask: [INTERN] page number mask = number of (pages / chip) - 1 * @pagebuf: [INTERN] holds the pagenumber which is currently in * data_buf. + * @pagebuf_bitflips: [INTERN] holds the bitflip count for the page which is + * currently in data_buf. * @subpagesize: [INTERN] holds the subpagesize * @onfi_version: [INTERN] holds the chip ONFI version (BCD encoded), * non 0 if ONFI supported. @@ -531,6 +533,7 @@ struct nand_chip { uint64_t chipsize; int pagemask; int pagebuf; + unsigned int pagebuf_bitflips; int subpagesize; uint8_t cellinfo; int badblockpos;