lkml.org 
[lkml]   [2019]   [Jan]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] mtd: rawnand: check return code of nand_reset() and nand_readid_op()
On Mon, 21 Jan 2019 22:05:34 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> nand_scan_ident() iterates over maxchips to find as many homogeneous
> chips as possible.
>
> Currently, this loop bails out only when manufacturer or device ID
> unmatches. The reason of unmatch is most likely no chip is connected
> to that chip select. In this case, nand_reset() has already failed,
> and the following nand_readid_op() is pointless.

While I agree with the following diff, I'd also like to point out that
nand_scan() callers should know how many controller CS lines are
connected to the chip (board file or DT description). The check we do in
nand_scan_ident() should only be here to clamp this value if the board
desc is wrong (maybe we should even fail in that case instead of
silently fixing things).

>
> Before ->exec_op hook was introduced, drivers had no way to tell
> the failure of NAND_CMD_RESET to the framework because the legacy
> ->cmdfunc() has void return type. Now drivers implementing ->exec_op
> hook can return the error code. You can save nand_readid_op() by
> checking the return value of nand_reset(). The return value of
> nand_readid_op() should be checked as well. If it fails, probably
> id[0] and id[1] are undefined values.
>
> Just for consistency, it should be sensible to check the return
> code in nand_do_write_oob() as well.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

Reviewed-by: Boris Brezillon <bbrezillon@kernel.org>

> ---
>
> drivers/mtd/nand/raw/nand_base.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 7ea3f10..3407523 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -457,7 +457,7 @@ static int nand_do_write_oob(struct nand_chip *chip, loff_t to,
> struct mtd_oob_ops *ops)
> {
> struct mtd_info *mtd = nand_to_mtd(chip);
> - int chipnr, page, status, len;
> + int chipnr, page, status, len, ret;
>
> pr_debug("%s: to = 0x%08x, len = %i\n",
> __func__, (unsigned int)to, (int)ops->ooblen);
> @@ -479,7 +479,9 @@ static int nand_do_write_oob(struct nand_chip *chip, loff_t to,
> * if we don't do this. I have no clue why, but I seem to have 'fixed'
> * it in the doc2000 driver in August 1999. dwmw2.
> */
> - nand_reset(chip, chipnr);
> + ret = nand_reset(chip, chipnr);
> + if (ret)
> + return ret;
>
> nand_select_target(chip, chipnr);
>
> @@ -5037,11 +5039,15 @@ static int nand_scan_ident(struct nand_chip *chip, unsigned int maxchips,
> u8 id[2];
>
> /* See comment in nand_get_flash_type for reset */
> - nand_reset(chip, i);
> + ret = nand_reset(chip, i);
> + if (ret)
> + break;
>
> nand_select_target(chip, i);
> /* Send the command for reading device ID */
> - nand_readid_op(chip, 0, id, sizeof(id));
> + ret = nand_readid_op(chip, 0, id, sizeof(id));
> + if (ret)
> + break;
> /* Read manufacturer and device IDs */
> if (nand_maf_id != id[0] || nand_dev_id != id[1]) {
> nand_deselect_target(chip);

\
 
 \ /
  Last update: 2019-01-21 14:14    [W:0.136 / U:0.256 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site