Patchwork [6/7] mtd: nand: sanity checks of ecc strength in nand_scan_tail()

login
register
mail settings
Submitter Mike Dunn
Date April 24, 2012, 7:18 p.m.
Message ID <1335295105-7981-7-git-send-email-mikedunn@newsguy.com>
Download mbox | patch
Permalink /patch/154751/
State New
Headers show

Comments

Mike Dunn - April 24, 2012, 7:18 p.m.
This patch adds sanity checks that ensure that drivers for controllers with
hardware ECC set the 'strength' element in struct nand_ecc_ctrl.  Also stylistic
changes to the line that calculates strength for software ECC.  Both suggested
during discussion [1].

[1] http://lists.infradead.org/pipermail/linux-mtd/2012-March/040467.html

Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
---
 drivers/mtd/nand/nand_base.c |   28 ++++++++++++++++++++++------
 1 files changed, 22 insertions(+), 6 deletions(-)
Brian Norris - April 25, 2012, 4:09 a.m.
Hi Mike,

On Tue, Apr 24, 2012 at 12:18 PM, Mike Dunn <mikedunn@newsguy.com> wrote:
> This patch adds sanity checks that ensure that drivers for controllers with
> hardware ECC set the 'strength' element in struct nand_ecc_ctrl.  Also stylistic
> changes to the line that calculates strength for software ECC.  Both suggested
> during discussion [1].
>
> [1] http://lists.infradead.org/pipermail/linux-mtd/2012-March/040467.html

Thanks for addressing these issues! Note my comment below though.

> ---
>  drivers/mtd/nand/nand_base.c |   28 ++++++++++++++++++++++------
>  1 files changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 3137abd..275c43f 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -3302,10 +3302,21 @@ int nand_scan_tail(struct mtd_info *mtd)
>                                   "hardware ECC not possible\n");
>                        BUG();
>                }
> +
> +               if (!chip->ecc.strength) {
> +                       pr_warn("Driver must set ecc.strength when using hardware ECC\n");
> +                       BUG();
> +               }
> +
>                if (!chip->ecc.read_page)
>                        chip->ecc.read_page = nand_read_page_hwecc_oob_first;
>
>        case NAND_ECC_HW:
> +               if (!chip->ecc.strength) {
> +                       pr_warn("Driver must set ecc.strength when using hardware ECC\n");
> +                       BUG();
> +               }
> +
>                /* Use standard hwecc read page function? */
>                if (!chip->ecc.read_page)
>                        chip->ecc.read_page = nand_read_page_hwecc;
> @@ -3345,12 +3356,17 @@ int nand_scan_tail(struct mtd_info *mtd)
>                if (!chip->ecc.write_oob)
>                        chip->ecc.write_oob = nand_write_oob_syndrome;
>
> -               if (mtd->writesize >= chip->ecc.size)
> -                       break;
> -               pr_warn("%d byte HW ECC not possible on "
> -                          "%d byte page size, fallback to SW ECC\n",
> -                          chip->ecc.size, mtd->writesize);
> +               if (mtd->writesize >= chip->ecc.size) {
> +                       if (!chip->ecc.strength) {
> +                               pr_warn("Driver must set ecc.strength when using hardware ECC\n");
> +                               BUG();
> +                       }
> +                       break;  /* all's well */
> +               }
> +               pr_warn("%d byte HW ECC not possible on %d byte page size, fallback to SW ECC\n",
> +                       chip->ecc.size, mtd->writesize);
>                chip->ecc.mode = NAND_ECC_SOFT;
> +               /* fall through */
>
>        case NAND_ECC_SOFT:
>                chip->ecc.calculate = nand_calculate_ecc;

I think you only need this last check (under ECC_HW_SYNDROME), since
the ECC_HW and ECC_HW_OOB_FIRST cases necessarily fall through to
ECC_HW_SYNDROME case. (Note the lack of 'break;' statements in the
first two 'case's.)

Brian
Mike Dunn - April 25, 2012, 3:08 p.m.
On 04/24/2012 09:09 PM, Brian Norris wrote:
> 
> I think you only need this last check (under ECC_HW_SYNDROME), since
> the ECC_HW and ECC_HW_OOB_FIRST cases necessarily fall through to
> ECC_HW_SYNDROME case. (Note the lack of 'break;' statements in the
> first two 'case's.)


Oops... you're right, I did miss that.  This should have been a much simpler
patch.  Thanks!

Mike

Patch

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 3137abd..275c43f 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3302,10 +3302,21 @@  int nand_scan_tail(struct mtd_info *mtd)
 				   "hardware ECC not possible\n");
 			BUG();
 		}
+
+		if (!chip->ecc.strength) {
+			pr_warn("Driver must set ecc.strength when using hardware ECC\n");
+			BUG();
+		}
+
 		if (!chip->ecc.read_page)
 			chip->ecc.read_page = nand_read_page_hwecc_oob_first;
 
 	case NAND_ECC_HW:
+		if (!chip->ecc.strength) {
+			pr_warn("Driver must set ecc.strength when using hardware ECC\n");
+			BUG();
+		}
+
 		/* Use standard hwecc read page function? */
 		if (!chip->ecc.read_page)
 			chip->ecc.read_page = nand_read_page_hwecc;
@@ -3345,12 +3356,17 @@  int nand_scan_tail(struct mtd_info *mtd)
 		if (!chip->ecc.write_oob)
 			chip->ecc.write_oob = nand_write_oob_syndrome;
 
-		if (mtd->writesize >= chip->ecc.size)
-			break;
-		pr_warn("%d byte HW ECC not possible on "
-			   "%d byte page size, fallback to SW ECC\n",
-			   chip->ecc.size, mtd->writesize);
+		if (mtd->writesize >= chip->ecc.size) {
+			if (!chip->ecc.strength) {
+				pr_warn("Driver must set ecc.strength when using hardware ECC\n");
+				BUG();
+			}
+			break;	/* all's well */
+		}
+		pr_warn("%d byte HW ECC not possible on %d byte page size, fallback to SW ECC\n",
+			chip->ecc.size, mtd->writesize);
 		chip->ecc.mode = NAND_ECC_SOFT;
+		/* fall through */
 
 	case NAND_ECC_SOFT:
 		chip->ecc.calculate = nand_calculate_ecc;
@@ -3401,7 +3417,7 @@  int nand_scan_tail(struct mtd_info *mtd)
 			BUG();
 		}
 		chip->ecc.strength =
-			chip->ecc.bytes*8 / fls(8*chip->ecc.size);
+			chip->ecc.bytes * 8 / fls(8 * chip->ecc.size);
 		break;
 
 	case NAND_ECC_NONE: