From patchwork Wed Mar 22 20:07:10 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Masahiro Yamada X-Patchwork-Id: 95840 Delivered-To: patch@linaro.org Received: by 10.140.89.233 with SMTP id v96csp415108qgd; Wed, 22 Mar 2017 13:17:03 -0700 (PDT) X-Received: by 10.84.208.102 with SMTP id f35mr58460636plh.19.1490213823527; Wed, 22 Mar 2017 13:17:03 -0700 (PDT) Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id s5si3000811plj.108.2017.03.22.13.17.03; Wed, 22 Mar 2017 13:17:03 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@nifty.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752673AbdCVUQ6 (ORCPT + 11 others); Wed, 22 Mar 2017 16:16:58 -0400 Received: from condef-06.nifty.com ([202.248.20.71]:40732 "EHLO condef-06.nifty.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751327AbdCVUPz (ORCPT ); Wed, 22 Mar 2017 16:15:55 -0400 Received: from conuserg-07.nifty.com ([10.126.8.70])by condef-06.nifty.com with ESMTP id v2MKBpWd013405 for ; Thu, 23 Mar 2017 05:11:51 +0900 Received: from grover.sesame (FL1-111-169-71-157.osk.mesh.ad.jp [111.169.71.157]) (authenticated) by conuserg-07.nifty.com with ESMTP id v2MK8961029452; Thu, 23 Mar 2017 05:08:31 +0900 DKIM-Filter: OpenDKIM Filter v2.10.3 conuserg-07.nifty.com v2MK8961029452 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nifty.com; s=dec2015msa; t=1490213312; bh=qYbsLp7Yfe0wPu41bq2nDxLiTJkmpwAeFevL0dSGe0g=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=lGE0TiZjY4HeNb7AmiYAZ8KlaJi01cmvGsqdWtERzEGrCcNBhNuyCQySAxSXkbkaQ rcMJvxd44YEak+4QvfevEqLswc7tmQW9DHxJK4vMzl09+aCrimohgnEa9gBFsJn6s3 CQ0o1cFDMMy1NYJtbiwbP+KRLgY4J+9dPsZQX5K5bFarwaZKJoVmud0Udo2fx4Pou3 V31HYAAIBD44wSATIKiBTo2OY7JJ5VMp23YwuKSH8BrGCdFPJzri82IG6jfg+9jNk+ JCrG//l1WIU6/FCvSjyC+rhcTkD4lYvovZwbRy7YvpjYQmsN/x9epB97RMkf5BAo3l i8EPnN9QN8Xdw== X-Nifty-SrcIP: [111.169.71.157] From: Masahiro Yamada To: linux-mtd@lists.infradead.org Cc: laurent.monat@idquantique.com, thorsten.christiansson@idquantique.com, Enrico Jorns , Artem Bityutskiy , Dinh Nguyen , Boris Brezillon , Marek Vasut , Graham Moore , David Woodhouse , Masami Hiramatsu , Chuanxiao Dong , Jassi Brar , Masahiro Yamada , linux-kernel@vger.kernel.org, Brian Norris , Richard Weinberger , Cyrille Pitchen Subject: [RESEND PATCH v2 11/53] mtd: nand: denali: fix bitflips calculation in handle_ecc() Date: Thu, 23 Mar 2017 05:07:10 +0900 Message-Id: <1490213273-8571-12-git-send-email-yamada.masahiro@socionext.com> X-Mailer: git-send-email 2.7.4 In-Reply-To: <1490213273-8571-1-git-send-email-yamada.masahiro@socionext.com> References: <1490213273-8571-1-git-send-email-yamada.masahiro@socionext.com> Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org This function is wrong in multiple ways: [1] Counting corrected bytes instead of corrected bits. The following code is counting the number of corrected _bytes_. /* correct the ECC error */ buf[offset] ^= err_cor_value; mtd->ecc_stats.corrected++; bitflips++; What the core framework expects is the number of corrected _bits_. They can be different if multiple bitflips occur within one byte. [2] total number of errors instead of max of per-sector errors The core framework expects that corrected errors are counted per sector, then the max value should be taken. The current code simply iterates over the whole page, i.e. counts the total number of correction in the page. This means "too many bitflips" is triggered earlier than it should be, i.e. the NAND device is worn out sooner. Besides those bugs, this function is unreadable due to the deep nesting. Notice the whole code in this function is wrapped in if (irq_status & INTR__ECC_ERR), so this conditional can be moved out of the function. Also, use shorter names for local variables. Re-work the function to fix all the issues. Signed-off-by: Masahiro Yamada --- Changes in v2: - Use shorter names for local variables. - Fix bugs addressed by [1], [2] drivers/mtd/nand/denali.c | 157 ++++++++++++++++++++++++---------------------- 1 file changed, 82 insertions(+), 75 deletions(-) -- 2.7.4 diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c index 86381ac..608fe6f 100644 --- a/drivers/mtd/nand/denali.c +++ b/drivers/mtd/nand/denali.c @@ -888,80 +888,87 @@ static void read_oob_data(struct mtd_info *mtd, uint8_t *buf, int page) #define ECC_SECTOR(x) (((x) & ECC_ERROR_ADDRESS__SECTOR_NR) >> 12) #define ECC_BYTE(x) (((x) & ECC_ERROR_ADDRESS__OFFSET)) #define ECC_CORRECTION_VALUE(x) ((x) & ERR_CORRECTION_INFO__BYTEMASK) -#define ECC_ERROR_CORRECTABLE(x) (!((x) & ERR_CORRECTION_INFO__ERROR_TYPE)) +#define ECC_ERROR_UNCORRECTABLE(x) ((x) & ERR_CORRECTION_INFO__ERROR_TYPE) #define ECC_ERR_DEVICE(x) (((x) & ERR_CORRECTION_INFO__DEVICE_NR) >> 8) #define ECC_LAST_ERR(x) ((x) & ERR_CORRECTION_INFO__LAST_ERR_INFO) -static bool handle_ecc(struct denali_nand_info *denali, uint8_t *buf, - uint32_t irq_status, unsigned int *max_bitflips) +static int handle_ecc(struct mtd_info *mtd, + struct denali_nand_info *denali, uint8_t *buf) { - bool check_erased_page = false; unsigned int bitflips = 0; + unsigned int max_bitflips = 0; + unsigned int total_bitflips = 0; + uint32_t err_addr, err_cor_info; + unsigned int err_byte, err_sector, err_device; + uint8_t err_cor_value; + unsigned int prev_sector = 0; + int ret = 0; + + /* read the ECC errors. we'll ignore them for now */ + denali_set_intr_modes(denali, false); - if (irq_status & INTR__ECC_ERR) { - /* read the ECC errors. we'll ignore them for now */ - uint32_t err_address, err_correction_info, err_byte, - err_sector, err_device, err_correction_value; - denali_set_intr_modes(denali, false); - - do { - err_address = ioread32(denali->flash_reg + - ECC_ERROR_ADDRESS); - err_sector = ECC_SECTOR(err_address); - err_byte = ECC_BYTE(err_address); - - err_correction_info = ioread32(denali->flash_reg + - ERR_CORRECTION_INFO); - err_correction_value = - ECC_CORRECTION_VALUE(err_correction_info); - err_device = ECC_ERR_DEVICE(err_correction_info); - - if (ECC_ERROR_CORRECTABLE(err_correction_info)) { - /* - * If err_byte is larger than ECC_SECTOR_SIZE, - * means error happened in OOB, so we ignore - * it. It's no need for us to correct it - * err_device is represented the NAND error - * bits are happened in if there are more - * than one NAND connected. - */ - if (err_byte < ECC_SECTOR_SIZE) { - struct mtd_info *mtd = - nand_to_mtd(&denali->nand); - int offset; - - offset = (err_sector * - ECC_SECTOR_SIZE + - err_byte) * - denali->devnum + - err_device; - /* correct the ECC error */ - buf[offset] ^= err_correction_value; - mtd->ecc_stats.corrected++; - bitflips++; - } - } else { - /* - * if the error is not correctable, need to - * look at the page to see if it is an erased - * page. if so, then it's not a real ECC error - */ - check_erased_page = true; - } - } while (!ECC_LAST_ERR(err_correction_info)); - /* - * Once handle all ecc errors, controller will triger - * a ECC_TRANSACTION_DONE interrupt, so here just wait - * for a while for this interrupt - */ - while (!(read_interrupt_status(denali) & - INTR__ECC_TRANSACTION_DONE)) - cpu_relax(); - clear_interrupts(denali); - denali_set_intr_modes(denali, true); - } - *max_bitflips = bitflips; - return check_erased_page; + do { + err_addr = ioread32(denali->flash_reg + ECC_ERROR_ADDRESS); + err_sector = ECC_SECTOR(err_addr); + err_byte = ECC_BYTE(err_addr); + + err_cor_info = ioread32(denali->flash_reg + ERR_CORRECTION_INFO); + err_cor_value = ECC_CORRECTION_VALUE(err_cor_info); + err_device = ECC_ERR_DEVICE(err_cor_info); + + /* reset the bitflip counter when crossing ECC sector */ + if (err_sector != prev_sector) + bitflips = 0; + + if (ECC_ERROR_UNCORRECTABLE(err_cor_info)) { + /* + * if the error is not correctable, need to look at the + * page to see if it is an erased page. if so, then + * it's not a real ECC error + */ + ret = -EBADMSG; + } else if (err_byte < ECC_SECTOR_SIZE) { + /* + * If err_byte is larger than ECC_SECTOR_SIZE, means error + * happened in OOB, so we ignore it. It's no need for + * us to correct it err_device is represented the NAND + * error bits are happened in if there are more than + * one NAND connected. + */ + int offset; + unsigned int flips_in_byte; + + offset = (err_sector * ECC_SECTOR_SIZE + err_byte) * + denali->devnum + err_device; + + /* correct the ECC error */ + flips_in_byte = hweight8(buf[offset] ^ err_cor_value); + bitflips += flips_in_byte; + total_bitflips += flips_in_byte; + buf[offset] ^= err_cor_value; + + max_bitflips = max(max_bitflips, bitflips); + } + + prev_sector = err_sector; + } while (!ECC_LAST_ERR(err_cor_info)); + + /* + * Once handle all ecc errors, controller will trigger a + * ECC_TRANSACTION_DONE interrupt, so here just wait for + * a while for this interrupt + */ + while (!(read_interrupt_status(denali) & INTR__ECC_TRANSACTION_DONE)) + cpu_relax(); + clear_interrupts(denali); + denali_set_intr_modes(denali, true); + + if (ret) + return ret; + + mtd->ecc_stats.corrected += total_bitflips; + + return max_bitflips; } /* programs the controller to either enable/disable DMA transfers */ @@ -1097,7 +1104,6 @@ static int denali_read_oob(struct mtd_info *mtd, struct nand_chip *chip, static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip, uint8_t *buf, int oob_required, int page) { - unsigned int max_bitflips; struct denali_nand_info *denali = mtd_to_denali(mtd); dma_addr_t addr = denali->buf.dma_buf; @@ -1105,8 +1111,7 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip, uint32_t irq_status; uint32_t irq_mask = INTR__ECC_TRANSACTION_DONE | INTR__ECC_ERR; - bool check_erased_page = false; - int stat; + int stat = 0; if (page != denali->page) { dev_err(denali->dev, @@ -1130,10 +1135,11 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip, memcpy(buf, denali->buf.buf, mtd->writesize); - check_erased_page = handle_ecc(denali, buf, irq_status, &max_bitflips); + if (irq_status & INTR__ECC_ERR) + stat = handle_ecc(mtd, denali, buf); denali_enable_dma(denali, false); - if (check_erased_page) { + if (stat == -EBADMSG) { read_oob_data(mtd, chip->oob_poi, denali->page); stat = nand_check_erased_ecc_chunk( @@ -1144,10 +1150,11 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip, if (stat < 0) { mtd->ecc_stats.failed++; /* return 0 for uncorrectable bitflips */ - max_bitflips = 0; + stat = 0; } } - return max_bitflips; + + return stat; } static int denali_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip,