[PATCH 0/3] mtd: rawnand: Fix HW ECC handling

[PATCH 0/3] mtd: rawnand: Fix HW ECC handling

[Tudor Ambarus]

Fix a wrongful fallthrough NAND_ECC_SOFT when a valid HW ECC is
available. Follow with two patches to make the code more readable.

Following error was seen in case of a valid HW ECC:
nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xda
nand: Micron MT29F2G08ABAEAWP
nand: 256 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64
------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at drivers/mtd/nand/raw/nand_base.c:5148 nand_scan_with_ids+0x12b8/0x1674
Modules linked in:
CPU: 0 PID: 1 Comm: swapper Not tainted 5.9.0-rc5-next-20200914 #2
Hardware name: Atmel SAMA5
[<c010c1e0>] (unwind_backtrace) from [<c0109aac>] (show_stack+0x10/0x14)
[<c0109aac>] (show_stack) from [<c07e9380>] (__warn+0xb8/0xd0)
[<c07e9380>] (__warn) from [<c07e93f8>] (warn_slowpath_fmt+0x60/0xbc)
[<c07e93f8>] (warn_slowpath_fmt) from [<c04cbc84>] (nand_scan_with_ids+0x12b8/0x1674)
[<c04cbc84>] (nand_scan_with_ids) from [<c04d32e0>] (atmel_nand_controller_add_nand+0x54/0x100)
[<c04d32e0>] (atmel_nand_controller_add_nand) from [<c04d36a8>] (atmel_nand_controller_add_nands+0x31c/0x60c)
[<c04d36a8>] (atmel_nand_controller_add_nands) from [<c04d55c0>] (atmel_hsmc_nand_controller_probe+0x188/0x284)
[<c04d55c0>] (atmel_hsmc_nand_controller_probe) from [<c04d40ec>] (atmel_nand_controller_probe+0x54/0x134)
[<c04d40ec>] (atmel_nand_controller_probe) from [<c047ef2c>] (platform_drv_probe+0x48/0x98)
[<c047ef2c>] (platform_drv_probe) from [<c047cfd0>] (really_probe+0x1e0/0x3b8)
[<c047cfd0>] (really_probe) from [<c047d310>] (driver_probe_device+0x5c/0xb4)
[<c047d310>] (driver_probe_device) from [<c047b3c0>] (bus_for_each_drv+0x80/0xc4)
[<c047b3c0>] (bus_for_each_drv) from [<c047cd7c>] (__device_attach+0xe0/0x14c)
[<c047cd7c>] (__device_attach) from [<c047c064>] (bus_probe_device+0x84/0x8c)
[<c047c064>] (bus_probe_device) from [<c04790bc>] (device_add+0x408/0x774)
[<c04790bc>] (device_add) from [<c05d742c>] (of_platform_device_create_pdata+0x98/0xd4)
[<c05d742c>] (of_platform_device_create_pdata) from [<c05d7604>] (of_platform_bus_create+0x190/0x220)
[<c05d7604>] (of_platform_bus_create) from [<c05d77c8>] (of_platform_populate+0x60/0xc0)
[<c05d77c8>] (of_platform_populate) from [<c05dd6e4>] (atmel_ebi_probe+0x358/0x530)
[<c05dd6e4>] (atmel_ebi_probe) from [<c047ef2c>] (platform_drv_probe+0x48/0x98)
[<c047ef2c>] (platform_drv_probe) from [<c047cfd0>] (really_probe+0x1e0/0x3b8)
[<c047cfd0>] (really_probe) from [<c047d310>] (driver_probe_device+0x5c/0xb4)
[<c047d310>] (driver_probe_device) from [<c047d508>] (device_driver_attach+0x58/0x60)
[<c047d508>] (device_driver_attach) from [<c047d568>] (__driver_attach+0x58/0xcc)
[<c047d568>] (__driver_attach) from [<c047b2ec>] (bus_for_each_dev+0x74/0xb4)
[<c047b2ec>] (bus_for_each_dev) from [<c047c258>] (bus_add_driver+0xf4/0x1d8)
[<c047c258>] (bus_add_driver) from [<c047dd88>] (driver_register+0x74/0x108)
[<c047dd88>] (driver_register) from [<c047f33c>] (__platform_driver_probe+0x68/0xc8)
[<c047f33c>] (__platform_driver_probe) from [<c01016d0>] (do_one_initcall+0x7c/0x1cc)
[<c01016d0>] (do_one_initcall) from [<c0b00f78>] (kernel_init_freeable+0x15c/0x1c0)
[<c0b00f78>] (kernel_init_freeable) from [<c07f23a4>] (kernel_init+0x8/0x114)
[<c07f23a4>] (kernel_init) from [<c0100148>] (ret_from_fork+0x14/0x2c)
Exception stack(0xcf427fb0 to 0xcf427ff8)
7fa0:                                     00000000 00000000 00000000 00000000
7fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
7fe0: 00000000 00000000 00000000 00000000 00000013 00000000
---[ end trace 7fad321bf58a54e1 ]---
atmel-nand-controller 10000000.ebi:nand-controller: NAND scan failed: -22
atmel-nand-controller: probe of 10000000.ebi:nand-controller failed with error -22

Tudor Ambarus (3):
  mtd: rawnand: Fix wrongful fallthrough NAND_ECC_SOFT
  mtd: rawnand: Introduce nand_set_ecc_on_host_ops()
  mtd: rawnand: Don't overwrite the error code from
    nand_set_ecc_soft_ops()

 drivers/mtd/nand/raw/nand_base.c | 134 ++++++++++++++++---------------
 1 file changed, 70 insertions(+), 64 deletions(-)

-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 1/3] mtd: rawnand: Fix wrongful fallthrough NAND_ECC_SOFT

[Tudor Ambarus]

In case of valid HW ECC, where the fallback to SW ECC is not needed,
the mentioned commit breaks the "switch (ecc->placement)" statement,
but then wrongly falls through the "case NAND_ECC_SOFT" of the
parent "switch (ecc->mode)". This causes an -EINVAL in
nand_set_ecc_soft_ops(), because for the valid HW ECC cases
ecc->mode is set to NAND_ECC_HW, but the nand_set_ecc_soft_ops()
expects a NAND_ECC_SOFT mode, thus -EINVAL.

Move the fallback to SW ECC after the setting of the ECC on host ops.
With this, when a valid HW ECC is available, we break the
"switch (ecc->mode)" statement, and when a fallback to SW ECC is
needed, we fallthrough "case NAND_ECC_SOFT".

Fixes: d3f8ec8e979b ("mtd: rawnand: Separate the ECC engine type and the ECC byte placement")
Reported-by: Santiago Esteban <santiago.esteban@microchip.com>
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/nand/raw/nand_base.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index d42832bedff5..bd30f6632fe2 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -5665,19 +5665,6 @@ static int nand_scan_tail(struct nand_chip *chip)
 				ecc->read_oob = nand_read_oob_syndrome;
 			if (!ecc->write_oob)
 				ecc->write_oob = nand_write_oob_syndrome;
-
-			if (mtd->writesize >= ecc->size) {
-				if (!ecc->strength) {
-					WARN(1, "Driver must set ecc.strength when using hardware ECC\n");
-					ret = -EINVAL;
-					goto err_nand_manuf_cleanup;
-				}
-				break;
-			}
-			pr_warn("%d byte HW ECC not possible on %d byte page size, fallback to SW ECC\n",
-				ecc->size, mtd->writesize);
-			ecc->engine_type = NAND_ECC_ENGINE_TYPE_SOFT;
-			ecc->algo = NAND_ECC_ALGO_HAMMING;
 			break;
 
 		default:
@@ -5686,6 +5673,19 @@ static int nand_scan_tail(struct nand_chip *chip)
 			ret = -EINVAL;
 			goto err_nand_manuf_cleanup;
 		}
+
+		if (mtd->writesize >= ecc->size) {
+			if (!ecc->strength) {
+				WARN(1, "Driver must set ecc.strength when using hardware ECC\n");
+				ret = -EINVAL;
+				goto err_nand_manuf_cleanup;
+			}
+			break;
+		}
+		pr_warn("%d byte HW ECC not possible on %d byte page size, fallback to SW ECC\n",
+			ecc->size, mtd->writesize);
+		ecc->engine_type = NAND_ECC_ENGINE_TYPE_SOFT;
+		ecc->algo = NAND_ECC_ALGO_HAMMING;
 		fallthrough;
 
 	case NAND_ECC_ENGINE_TYPE_SOFT:
-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 2/3] mtd: rawnand: Introduce nand_set_ecc_on_host_ops()

[Tudor Ambarus]

There were too many levels of indentation and the code was
hard to read. Introduce a new function, similar to
nand_set_ecc_soft_ops().

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/nand/raw/nand_base.c | 114 +++++++++++++++++--------------
 1 file changed, 61 insertions(+), 53 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index bd30f6632fe2..d3e0ca66382f 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -5139,6 +5139,65 @@ static void nand_scan_ident_cleanup(struct nand_chip *chip)
 	kfree(chip->parameters.onfi);
 }
 
+static int nand_set_ecc_on_host_ops(struct nand_chip *chip)
+{
+	struct nand_ecc_ctrl *ecc = &chip->ecc;
+
+	switch (ecc->placement) {
+	case NAND_ECC_PLACEMENT_UNKNOWN:
+	case NAND_ECC_PLACEMENT_OOB:
+		/* Use standard hwecc read page function? */
+		if (!ecc->read_page)
+			ecc->read_page = nand_read_page_hwecc;
+		if (!ecc->write_page)
+			ecc->write_page = nand_write_page_hwecc;
+		if (!ecc->read_page_raw)
+			ecc->read_page_raw = nand_read_page_raw;
+		if (!ecc->write_page_raw)
+			ecc->write_page_raw = nand_write_page_raw;
+		if (!ecc->read_oob)
+			ecc->read_oob = nand_read_oob_std;
+		if (!ecc->write_oob)
+			ecc->write_oob = nand_write_oob_std;
+		if (!ecc->read_subpage)
+			ecc->read_subpage = nand_read_subpage;
+		if (!ecc->write_subpage && ecc->hwctl && ecc->calculate)
+			ecc->write_subpage = nand_write_subpage_hwecc;
+		fallthrough;
+
+	case NAND_ECC_PLACEMENT_INTERLEAVED:
+		if ((!ecc->calculate || !ecc->correct || !ecc->hwctl) &&
+		    (!ecc->read_page ||
+		     ecc->read_page == nand_read_page_hwecc ||
+		     !ecc->write_page ||
+		     ecc->write_page == nand_write_page_hwecc)) {
+			WARN(1, "No ECC functions supplied; hardware ECC not possible\n");
+			return -EINVAL;
+		}
+		/* Use standard syndrome read/write page function? */
+		if (!ecc->read_page)
+			ecc->read_page = nand_read_page_syndrome;
+		if (!ecc->write_page)
+			ecc->write_page = nand_write_page_syndrome;
+		if (!ecc->read_page_raw)
+			ecc->read_page_raw = nand_read_page_raw_syndrome;
+		if (!ecc->write_page_raw)
+			ecc->write_page_raw = nand_write_page_raw_syndrome;
+		if (!ecc->read_oob)
+			ecc->read_oob = nand_read_oob_syndrome;
+		if (!ecc->write_oob)
+			ecc->write_oob = nand_write_oob_syndrome;
+		break;
+
+	default:
+		pr_warn("Invalid NAND_ECC_PLACEMENT %d\n",
+			ecc->placement);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int nand_set_ecc_soft_ops(struct nand_chip *chip)
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
@@ -5619,60 +5678,9 @@ static int nand_scan_tail(struct nand_chip *chip)
 
 	switch (ecc->engine_type) {
 	case NAND_ECC_ENGINE_TYPE_ON_HOST:
-
-		switch (ecc->placement) {
-		case NAND_ECC_PLACEMENT_UNKNOWN:
-		case NAND_ECC_PLACEMENT_OOB:
-			/* Use standard hwecc read page function? */
-			if (!ecc->read_page)
-				ecc->read_page = nand_read_page_hwecc;
-			if (!ecc->write_page)
-				ecc->write_page = nand_write_page_hwecc;
-			if (!ecc->read_page_raw)
-				ecc->read_page_raw = nand_read_page_raw;
-			if (!ecc->write_page_raw)
-				ecc->write_page_raw = nand_write_page_raw;
-			if (!ecc->read_oob)
-				ecc->read_oob = nand_read_oob_std;
-			if (!ecc->write_oob)
-				ecc->write_oob = nand_write_oob_std;
-			if (!ecc->read_subpage)
-				ecc->read_subpage = nand_read_subpage;
-			if (!ecc->write_subpage && ecc->hwctl && ecc->calculate)
-				ecc->write_subpage = nand_write_subpage_hwecc;
-			fallthrough;
-
-		case NAND_ECC_PLACEMENT_INTERLEAVED:
-			if ((!ecc->calculate || !ecc->correct || !ecc->hwctl) &&
-			    (!ecc->read_page ||
-			     ecc->read_page == nand_read_page_hwecc ||
-			     !ecc->write_page ||
-			     ecc->write_page == nand_write_page_hwecc)) {
-				WARN(1, "No ECC functions supplied; hardware ECC not possible\n");
-				ret = -EINVAL;
-				goto err_nand_manuf_cleanup;
-			}
-			/* Use standard syndrome read/write page function? */
-			if (!ecc->read_page)
-				ecc->read_page = nand_read_page_syndrome;
-			if (!ecc->write_page)
-				ecc->write_page = nand_write_page_syndrome;
-			if (!ecc->read_page_raw)
-				ecc->read_page_raw = nand_read_page_raw_syndrome;
-			if (!ecc->write_page_raw)
-				ecc->write_page_raw = nand_write_page_raw_syndrome;
-			if (!ecc->read_oob)
-				ecc->read_oob = nand_read_oob_syndrome;
-			if (!ecc->write_oob)
-				ecc->write_oob = nand_write_oob_syndrome;
-			break;
-
-		default:
-			pr_warn("Invalid NAND_ECC_PLACEMENT %d\n",
-				ecc->placement);
-			ret = -EINVAL;
+		ret = nand_set_ecc_on_host_ops(chip);
+		if (ret)
 			goto err_nand_manuf_cleanup;
-		}
 
 		if (mtd->writesize >= ecc->size) {
 			if (!ecc->strength) {
-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 3/3] mtd: rawnand: Don't overwrite the error code from nand_set_ecc_soft_ops()

[Tudor Ambarus]

The error code received from nand_set_ecc_soft_ops() was overwritten,
drop this redundant assignment and use the error code received from
the callee.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/nand/raw/nand_base.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index d3e0ca66382f..78feed59c8b5 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -5698,10 +5698,8 @@ static int nand_scan_tail(struct nand_chip *chip)
 
 	case NAND_ECC_ENGINE_TYPE_SOFT:
 		ret = nand_set_ecc_soft_ops(chip);
-		if (ret) {
-			ret = -EINVAL;
+		if (ret)
 			goto err_nand_manuf_cleanup;
-		}
 		break;
 
 	case NAND_ECC_ENGINE_TYPE_ON_DIE:
-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 3/3] mtd: rawnand: Don't overwrite the error code from nand_set_ecc_soft_ops()

[Miquel Raynal]

On Thu, 2020-09-17 at 07:52:13 UTC, Tudor Ambarus wrote:
> The error code received from nand_set_ecc_soft_ops() was overwritten,
> drop this redundant assignment and use the error code received from
> the callee.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.

Miquel

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/3] mtd: rawnand: Introduce nand_set_ecc_on_host_ops()

[Miquel Raynal]

On Thu, 2020-09-17 at 07:52:12 UTC, Tudor Ambarus wrote:
> There were too many levels of indentation and the code was
> hard to read. Introduce a new function, similar to
> nand_set_ecc_soft_ops().
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.

Miquel

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/3] mtd: rawnand: Fix wrongful fallthrough NAND_ECC_SOFT

[Miquel Raynal]

On Thu, 2020-09-17 at 07:52:11 UTC, Tudor Ambarus wrote:
> In case of valid HW ECC, where the fallback to SW ECC is not needed,
> the mentioned commit breaks the "switch (ecc->placement)" statement,
> but then wrongly falls through the "case NAND_ECC_SOFT" of the
> parent "switch (ecc->mode)". This causes an -EINVAL in
> nand_set_ecc_soft_ops(), because for the valid HW ECC cases
> ecc->mode is set to NAND_ECC_HW, but the nand_set_ecc_soft_ops()
> expects a NAND_ECC_SOFT mode, thus -EINVAL.
> 
> Move the fallback to SW ECC after the setting of the ECC on host ops.
> With this, when a valid HW ECC is available, we break the
> "switch (ecc->mode)" statement, and when a fallback to SW ECC is
> needed, we fallthrough "case NAND_ECC_SOFT".
> 
> Fixes: d3f8ec8e979b ("mtd: rawnand: Separate the ECC engine type and the ECC byte placement")
> Reported-by: Santiago Esteban <santiago.esteban@microchip.com>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.

Miquel

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/