| Message ID | 20210205142725.13225-2-s.hauer@pengutronix.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/2] mtd: nand: fix error handling in nand_prog_page_op() #1 | expand |
On Fri, 2021-02-05 at 14:27:25 UTC, Sascha Hauer wrote: > On success nand_exec_prog_page_op() returns the NAND status byte, but on > failure it returns a negative error code. nand_prog_page_op() interprets > the return value as NAND status byte without error checking. This means > a failure in nand_exec_prog_page_op() can go through unnoticed. > > The straight forward fix would be to add the missing error checking. To > clean the code a bit we can move the nand status check to > nand_prog_page_op(). This way we can get rid of the overloaded return > value from nand_exec_prog_page_op() and return a plain error code which > is less error prone. > > nand_exec_prog_page_op() is only called from one other place and in this > call the 'prog' parameter is false in which case the nand status check > is skipped, so it's correct to not add the NAND status check there. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks. Miquel
diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c index d2b2a1f70fde..73d76f4a262f 100644 --- a/drivers/mtd/nand/raw/nand_base.c +++ b/drivers/mtd/nand/raw/nand_base.c @@ -1294,8 +1294,6 @@ static int nand_exec_prog_page_op(struct nand_chip *chip, unsigned int page, }; struct nand_operation op = NAND_OPERATION(chip->cur_cs, instrs); int naddrs = nand_fill_column_cycles(chip, addrs, offset_in_page); - int ret; - u8 status; if (naddrs < 0) return naddrs; @@ -1335,15 +1333,7 @@ static int nand_exec_prog_page_op(struct nand_chip *chip, unsigned int page, op.ninstrs--; } - ret = nand_exec_op(chip, &op); - if (!prog || ret) - return ret; - - ret = nand_status_op(chip, &status); - if (ret) - return ret; - - return status; + return nand_exec_op(chip, &op); } /** @@ -1449,7 +1439,8 @@ int nand_prog_page_op(struct nand_chip *chip, unsigned int page, unsigned int len) { struct mtd_info *mtd = nand_to_mtd(chip); - int status; + u8 status; + int ret; if (!len || !buf) return -EINVAL; @@ -1458,8 +1449,14 @@ int nand_prog_page_op(struct nand_chip *chip, unsigned int page, return -EINVAL; if (nand_has_exec_op(chip)) { - status = nand_exec_prog_page_op(chip, page, offset_in_page, buf, + ret = nand_exec_prog_page_op(chip, page, offset_in_page, buf, len, true); + if (ret) + return ret; + + ret = nand_status_op(chip, &status); + if (ret) + return ret; } else { chip->legacy.cmdfunc(chip, NAND_CMD_SEQIN, offset_in_page, page);
On success nand_exec_prog_page_op() returns the NAND status byte, but on failure it returns a negative error code. nand_prog_page_op() interprets the return value as NAND status byte without error checking. This means a failure in nand_exec_prog_page_op() can go through unnoticed. The straight forward fix would be to add the missing error checking. To clean the code a bit we can move the nand status check to nand_prog_page_op(). This way we can get rid of the overloaded return value from nand_exec_prog_page_op() and return a plain error code which is less error prone. nand_exec_prog_page_op() is only called from one other place and in this call the 'prog' parameter is false in which case the nand status check is skipped, so it's correct to not add the NAND status check there. Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- drivers/mtd/nand/raw/nand_base.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-)