Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751562AbdFGNdW convert rfc822-to-8bit (ORCPT ); Wed, 7 Jun 2017 09:33:22 -0400 Received: from mail.free-electrons.com ([62.4.15.54]:46077 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751444AbdFGNdU (ORCPT ); Wed, 7 Jun 2017 09:33:20 -0400 Date: Wed, 7 Jun 2017 15:33:18 +0200 From: Boris Brezillon To: Masahiro Yamada Cc: linux-mtd@lists.infradead.org, Enrico Jorns , Artem Bityutskiy , Dinh Nguyen , Marek Vasut , David Woodhouse , Masami Hiramatsu , Chuanxiao Dong , Jassi Brar , Cyrille Pitchen , linux-kernel@vger.kernel.org, Brian Norris , Richard Weinberger Subject: Re: [PATCH v5 07/23] mtd: nand: denali: do not propagate NAND_STATUS_FAIL to waitfunc() Message-ID: <20170607153318.7e341dff@bbrezillon> In-Reply-To: <1496836352-8016-8-git-send-email-yamada.masahiro@socionext.com> References: <1496836352-8016-1-git-send-email-yamada.masahiro@socionext.com> <1496836352-8016-8-git-send-email-yamada.masahiro@socionext.com> X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3128 Lines: 97 On Wed, 7 Jun 2017 20:52:16 +0900 Masahiro Yamada wrote: > Currently, the error handling of denali_write_page(_raw) is a bit > complicated. If the program command fails, NAND_STATUS_FAIL is set > to the driver internal denali->status, then read out later by > denali_waitfunc(). > > We can avoid it by exploiting the nand_write_page() implementation. > If chip->ecc.write_page(_raw) returns negative code (i.e. -EIO), it > errors out immediately. This gives the same result as returning > NAND_STATUS_FAIL from chip->waitfunc. In either way, -EIO is > returned to the upper MTD layer. Actually, this is how it's supposed to work now (when they set the NAND_ECC_CUSTOM_PAGE_ACCESS flag, drivers are expected to wait for the program operation to finish and return -EIO if it failed), so you're all good ;-). > > Signed-off-by: Masahiro Yamada > --- > > Changes in v5: None > Changes in v4: None > Changes in v3: None > Changes in v2: > - Newly added > > drivers/mtd/nand/denali.c | 12 ++++-------- > drivers/mtd/nand/denali.h | 1 - > 2 files changed, 4 insertions(+), 9 deletions(-) > > diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c > index 1897fe238290..22acfc34b546 100644 > --- a/drivers/mtd/nand/denali.c > +++ b/drivers/mtd/nand/denali.c > @@ -1005,6 +1005,7 @@ static int write_page(struct mtd_info *mtd, struct nand_chip *chip, > size_t size = mtd->writesize + mtd->oobsize; > uint32_t irq_status; > uint32_t irq_mask = INTR__DMA_CMD_COMP | INTR__PROGRAM_FAIL; As mentioned in my previous patch, I think you should wait for INTR__PROGRAM_COMP | INTR__PROGRAM_FAIL here. > + int ret = 0; > > denali->page = page; > > @@ -1038,13 +1039,13 @@ static int write_page(struct mtd_info *mtd, struct nand_chip *chip, > if (irq_status == 0) { > dev_err(denali->dev, "timeout on write_page (type = %d)\n", > raw_xfer); > - denali->status = NAND_STATUS_FAIL; > + ret = -EIO; > } if (irq_status & INTR__PROGRAM_FAIL) { dev_err(denali->dev, "page program failed (type = %d)\n", raw_xfer); ret = -EIO; } > > denali_enable_dma(denali, false); > dma_sync_single_for_cpu(denali->dev, addr, size, DMA_TO_DEVICE); > > - return 0; > + return ret; > } > > /* NAND core entry points */ > @@ -1196,12 +1197,7 @@ static void denali_select_chip(struct mtd_info *mtd, int chip) > > static int denali_waitfunc(struct mtd_info *mtd, struct nand_chip *chip) > { > - struct denali_nand_info *denali = mtd_to_denali(mtd); > - int status = denali->status; > - > - denali->status = 0; > - > - return status; > + return 0; > } > > static int denali_erase(struct mtd_info *mtd, int page) > diff --git a/drivers/mtd/nand/denali.h b/drivers/mtd/nand/denali.h > index a06ed741b550..352d8328b94a 100644 > --- a/drivers/mtd/nand/denali.h > +++ b/drivers/mtd/nand/denali.h > @@ -323,7 +323,6 @@ struct nand_buf { > struct denali_nand_info { > struct nand_chip nand; > int flash_bank; /* currently selected chip */ > - int status; > int platform; > struct nand_buf buf; > struct device *dev;