[PATCH] mtd: rawnand: check return code of nand_reset() and nand_readid_op()

[PATCH] mtd: rawnand: check return code of nand_reset() and nand_readid_op()

[Masahiro Yamada]

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.

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>
---

 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);
-- 
2.7.4

Re: [PATCH] mtd: rawnand: check return code of nand_reset() and nand_readid_op()

[Boris Brezillon]

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);

Re: [PATCH] mtd: rawnand: check return code of nand_reset() and nand_readid_op()

[Masahiro Yamada]

On Mon, Jan 21, 2019 at 10:14 PM Boris Brezillon <bbrezillon@kernel.org> wrote:
>
> 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).


I know this.
This is a problem for denali because
I have not decoupled chip/controller yet.


Maybe, is the following better?


------------------>8-----------------------
nand_scan_ident() iterates over maxchips to find as many homogeneous
chips as possible.

Since commit 2d472aba15ff ("mtd: nand: document the NAND controller/NAND
chip DT representation"), new drivers should pass in the exact number of
CS lines instead of possible max, but old platforms may still rely on
nand_scan_ident() to detect the actual number of connected CS lines.

In that case, this loop bails out when manufacturer or device ID
unmatches. The reason of unmatch is most likely no chip is connected
to that CS line. If so, nand_reset() should already have failed,
and the following nand_readid_op() is pointless.

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.
------------------------------>8--------------------------------






> >
> > 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);
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/



--
Best Regards
Masahiro Yamada

Re: [PATCH] mtd: rawnand: check return code of nand_reset() and nand_readid_op()

[Boris Brezillon]

On Tue, 22 Jan 2019 00:57:43 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> 
> Maybe, is the following better?

Sounds good, even if the original commit message was fine too. I was
just pointing out that nand_scan() should, when possible, be passed
the real number of CS lines connected to the chip instead of the max
number of CS lines supported by the controller.

> 
> 
> ------------------>8-----------------------  
> nand_scan_ident() iterates over maxchips to find as many homogeneous
> chips as possible.
> 
> Since commit 2d472aba15ff ("mtd: nand: document the NAND controller/NAND
> chip DT representation"), new drivers should pass in the exact number of
> CS lines instead of possible max, but old platforms may still rely on
> nand_scan_ident() to detect the actual number of connected CS lines.
> 
> In that case, this loop bails out when manufacturer or device ID
> unmatches. The reason of unmatch is most likely no chip is connected
> to that CS line. If so, nand_reset() should already have failed,
> and the following nand_readid_op() is pointless.
> 
> 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.

Re: [PATCH] mtd: rawnand: check return code of nand_reset() and nand_readid_op()

[Miquel Raynal]

Hi Masahiro,

Masahiro Yamada <yamada.masahiro@socionext.com> wrote on Tue, 22 Jan
2019 00:57:43 +0900:

> On Mon, Jan 21, 2019 at 10:14 PM Boris Brezillon <bbrezillon@kernel.org> wrote:
> >
> > 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).  
> 
> 
> I know this.
> This is a problem for denali because
> I have not decoupled chip/controller yet.
> 
> 
> Maybe, is the following better?
> 
> 
> ------------------>8-----------------------  
> nand_scan_ident() iterates over maxchips to find as many homogeneous
> chips as possible.
> 
> Since commit 2d472aba15ff ("mtd: nand: document the NAND controller/NAND
> chip DT representation"), new drivers should pass in the exact number of
> CS lines instead of possible max, but old platforms may still rely on
> nand_scan_ident() to detect the actual number of connected CS lines.
> 
> In that case, this loop bails out when manufacturer or device ID
> unmatches. The reason of unmatch is most likely no chip is connected
> to that CS line. If so, nand_reset() should already have failed,
> and the following nand_readid_op() is pointless.
> 
> 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.
> ------------------------------>8--------------------------------  
> 

Patch applied to nand/next with the second commit log you proposed.

Thanks for all your work!
Miquèl