From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ch1ehsobe006.messaging.microsoft.com ([216.32.181.186] helo=ch1outboundpool.messaging.microsoft.com) by merlin.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1WNeJj-0001FN-Km for linux-mtd@lists.infradead.org; Wed, 12 Mar 2014 08:15:12 +0000 Date: Wed, 12 Mar 2014 15:26:57 +0800 From: Huang Shijie To: Brian Norris Subject: Re: [PATCH V4] mtd: gpmi: fix the bitflips for erased page Message-ID: <20140312072654.GA30808@shlinux2.ap.freescale.net> References: <1389602838-25212-1-git-send-email-b32955@freescale.com> <20140308033238.GD5232@ld-irv-0074> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <20140308033238.GD5232@ld-irv-0074> Cc: eliedebrauwer@gmail.com, bpringlemeir@nbsps.com, Vikram.Narayanan@in.bosch.com, linux-mtd@lists.infradead.org, Pekon Gupta , dwmw2@infradead.org List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Fri, Mar 07, 2014 at 07:32:38PM -0800, Brian Norris wrote: > > > > This patch does a check for the uncorrectable failure in the following steps: > > > > [0] set the threshold. > > The threshold is set based on the truth: > > "A single 0 bit will lead to gf_len(13 or 14) bits 0 after the BCH > > do the ECC." > > > > For the sake of safe, we will set the threshold with half the gf_len, and > > do not make it bigger the ECC strength. > > This threshold looks wrong here. I shrink the threshold on purpose. The ECC strength can be 40 some times. I only met NAND which only has 1bit bitflips for the whole page. If i meet a NAND which may has 40bits flips, it will make me crazy. > > > [1] count the bitflips of the current ECC chunk, assume it is N. > > > > [2] if the (N <= threshold) is true, we continue to read out the page with > > ECC disabled. and we count the bitflips again, assume it is N2. > > (We read out the whole page, not just a chunk, this makes the check > > more strictly, and make the code more simple.) > > You can read a whole page, but the counting should not be against the > whole page; it should allow 'threshold' # of bitflips in each sector. I just want to make the check more tough. :) > > > [3] if the (N2 <= threshold) is true again, we can regard this is a erased > > page. This is because a real erased page is full of 0xFF(maybe also has > > several bitflips), while a page contains the 0xFF data will definitely > > has many bitflips in the ECC parity areas. > > This threshold definitely shouldn't be based on gf_len; it should be > based on ECC strength. Ditto. > > > [4] if the [3] fails, we can regard this is a page filled with the '0xFF' > > data. > > > > Tested-by: Elie De Brauwer > > Reported-by: Elie De Brauwer > > Signed-off-by: Huang Shijie > > --- > > v3 --> v4: > > [1] update the mtd->ecc_stats.corrected when we detect an erased page. > > [2] add the shortcut when counting bitflips for non-ECC buffer. > > [3] add more comments about why read a whole page, but not a chunk. > > > > --- > > drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 58 ++++++++++++++++++++++++++++++++ > > 1 files changed, 58 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > > index e2f5820..6b6e707 100644 > > --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > > @@ -958,6 +958,60 @@ static void block_mark_swapping(struct gpmi_nand_data *this, > > p[1] = (p[1] & mask) | (from_oob >> (8 - bit)); > > } > > > > +static bool gpmi_erased_check(struct gpmi_nand_data *this, > > + unsigned char *data, unsigned int chunk, int page, > > + unsigned int *max_bitflips) > > +{ > > + struct nand_chip *chip = &this->nand; > > + struct mtd_info *mtd = &this->mtd; > > + struct bch_geometry *geo = &this->bch_geometry; > > + int base = geo->ecc_chunk_size * chunk; > > + unsigned int flip_bits = 0, flip_bits_noecc = 0; > > + uint64_t *buf = (uint64_t *)this->data_buffer_dma; > > + unsigned int threshold; > > + int i; > > + > > + threshold = geo->gf_len / 2; > > + if (threshold > geo->ecc_strength) > > + threshold = geo->ecc_strength; > > + > > + /* Count bitflips */ > > + for (i = 0; i < geo->ecc_chunk_size; i++) { > > + flip_bits += hweight8(~data[base + i]); > > + if (flip_bits > threshold) > > + return false; > > + } > > ^^^ This is the only part that's unique to this patch, I think, but it's > not really special to GPMI. And it's not correct, either. Suppose I have > a high ECC strength flash (20-bit correction?) but gf_len is 14. Then > threshold == 7. Now, if we see 10 bitflips in an erased page, we will > return false here, saying this page was not erased. But 10 is easily > within the ECC spec for this flash. That's a problem, no? Do you meet a NAND which has 10 bitflips? > > So this check should be based on the ECC strength. But is this a good > optimization in the first place? I think it could be thrown out for > simplicity in the generic case (e.g., in my sample pasted below). > > > + > > + /* > > + * Read out the whole page with ECC disabled, and check it again, > > + * This is more strict then just read out a chunk, and it makes > > + * the code more simple. > > + */ > > + chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page); > > + chip->read_buf(mtd, (uint8_t *)buf, mtd->writesize); > > + > > + /* Count the bitflips for the no ECC buffer */ > > + for (i = 0; i < mtd->writesize / 8; i++) { > > + flip_bits_noecc += hweight64(~buf[i]); > > + if (flip_bits_noecc > threshold) > > + return false; > > + } > > ^^^ this loop should be broken up into sectors, so that you actually > count max_bitflips per sector, not for the whole page. count the max_bitflips per sector make the code more complicated. > > > + > > + /* Tell the upper layer the bitflips we corrected. */ > > + mtd->ecc_stats.corrected += flip_bits; > > + *max_bitflips = max_t(unsigned int, *max_bitflips, flip_bits); > > This is wrong. You don't want to use the existing *max_bitflips value > from the previous read (remember, you're re-reading the data). You > should be doing a max() over each sector in the raw page, as mentioned > above. yes, I just tell the upper layer the max_bitflips for the ECC read, not the raw read. > > > + > > + /* > > + * The geo->payload_size maybe not equal to the page size > > + * when the Subpage-Read is enabled. > > + */ > > + memset(data, 0xff, geo->payload_size); > > + > > + dev_dbg(this->dev, "The page(%d) is an erased page(%d,%d,%d,%d).\n", > > + page, chunk, threshold, flip_bits, flip_bits_noecc); > > + return true; > > +} > > + > > static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip, > > uint8_t *buf, int oob_required, int page) > > { > > @@ -1007,6 +1061,10 @@ static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip, > > continue; > > > > if (*status == STATUS_UNCORRECTABLE) { > > + if (gpmi_erased_check(this, payload_virt, i, > > + page, &max_bitflips)) > > + break; > > + > > mtd->ecc_stats.failed++; > > continue; > > } > > I'll post my alternate implementation below. I can work this up as a > real patch if you want. It could probably be optimized a bit to error > out quicker, but I don't think that is actually a performance concern > here. > > Signed-off-by: Brian Norris > > /* > * Check a page to see if it is erased (w/ bitflips) after an uncorrectable ECC > * error > * > * On a real error, return a negative error code (-EBADMSG for ECC error), and > * buf will contain raw data. > * Otherwise, fill buf with 0xff and return the maximum number of > * bitflips-per-ECC-sector to the caller. > * > */ > static int nand_verify_erased_page(struct mtd_info *mtd, > struct nand_chip *chip, u32 *buf, u64 addr) > { > int i, oob_step, oob_nbits, data_nbits; > u8 *oob_buf = (u8 *) chip->oob_poi; > unsigned int max_bitflips = 0; > int page = addr >> chip->page_shift; > int ret; > > oob_step = mtd->oobsize / chip->ecc.steps; > oob_nbits = oob_step << 3; > data_nbits = chip->ecc.size << 3; > > /* read without ecc for verification */ > ret = chip->ecc.read_page_raw(mtd, chip, (u8 *)buf, true, page); > if (ret) > return ret; > > for (i = 0; i < chip->ecc.steps; i++, oob_buf += oob_step) { > unsigned int bitflips = 0; > > bitflips += oob_nbits - > bitmap_weight((unsigned long *)oob_buf, oob_nbits); > bitflips += data_nbits - > bitmap_weight((unsigned long *)buf, data_nbits); > > buf += chip->ecc.size; > addr += chip->ecc.size; > > /* Too many bitflips */ > if (bitflips > chip->ecc.strength) > return -EBADMSG; > > max_bitflips = max(max_bitflips, bitflips); > } > > pr_debug("correcting bitflips in erased page (max %u)\n", > max_bitflips); > > return max_bitflips; > } > > I will comment on your formal patch. thanks Huang Shijie