| Message ID | 1490856383-31560-19-git-send-email-yamada.masahiro@socionext.com |
|---|---|
| State | Superseded |
| Delegated to: | Boris Brezillon |
| Headers | show |
On Thu, 30 Mar 2017 15:46:04 +0900 Masahiro Yamada <yamada.masahiro@socionext.com> 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. > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > --- > > 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 79851ca..5da8156 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; > + 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; > } > > 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; I know it's not your fault, and the existing denali_waitfunc() is already buggy, but it's definitely wrong to return 0 here without checking the NAND status. ->waitfunc() is not only used to wait for a page-program operation, it's used every time the NAND enters the busy state (lock, unlock, set_features, ...). Anyway, I'll review the remaining patches before taking a decision. > } > > 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 00ce04e..9e2b787 100644 > --- a/drivers/mtd/nand/denali.h > +++ b/drivers/mtd/nand/denali.h > @@ -329,7 +329,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;
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c index 79851ca..5da8156 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; + 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; } 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 00ce04e..9e2b787 100644 --- a/drivers/mtd/nand/denali.h +++ b/drivers/mtd/nand/denali.h @@ -329,7 +329,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;
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. Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> --- 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(-)