[PATCH] nand_base: fix regression on nand with 64/128 oob size

[PATCH] nand_base: fix regression on nand with 64/128 oob size

[Alexander Couzens]

41b207a70d3a accidently changed the oob layout for nand when ECC doesn't use
the full available ecc reserved data.
This only affects controllers which use the default large page ops
nand_ooblayout_ecc_lp.

E.g. for davinci:
- nand has 2048 byte page, 64 byte oob
- 3 byte ecc every 512 byte using 1bit hw ecc
- requires 12 byte of oob space for ecc
- old layout reserved 24 byte based on 64 byte oob

Old layout started using byte from offset 40 while new layout
uses the last X bytes so it fits into the end of oob.
Meaning in this example the old layout uses byte 40-51, while
the new layout use byte 52-63.

Signed-off-by: Alexander Couzens <lynxis@fe80.eu>
---
 drivers/mtd/nand/nand_base.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index b0524f8accb6..ba88345fd334 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -112,8 +112,25 @@ static int nand_ooblayout_ecc_lp(struct mtd_info *mtd, int section,
 	if (section)
 		return -ERANGE;
 
+	/* to be compatible with previous layouts
+	 * 64 byte oob must have ecc data at byte 40,
+	 * 128 byte oob must have ecc data at byte 80 */
+	switch (mtd->oobsize) {
+	case 64:
+		oobregion->offset = 40;
+		break;
+	case 128:
+		oobregion->offset = 80;
+		break;
+	default:
+		oobregion->offset = mtd->oobsize - oobregion->length;
+		break;
+	}
+
 	oobregion->length = ecc->total;
-	oobregion->offset = mtd->oobsize - oobregion->length;
+	if (oobregion->offset + oobregion->length > mtd->oobsize) {
+		return -ERANGE;
+	}
 
 	return 0;
 }
-- 
2.12.0

Re: [PATCH] nand_base: fix regression on nand with 64/128 oob size

[Boris Brezillon]

Hi Alexander,

On Sun, 12 Mar 2017 21:02:32 +0100
Alexander Couzens <lynxis@fe80.eu> wrote:

> 41b207a70d3a

When you refer to a commit, please put the commit description as well:

41b207a70d3a ("mtd: nand: implement the default mtd_ooblayout_ops")

> accidently changed the oob layout for nand when ECC doesn't use
> the full available ecc reserved data.

Oh crap! I didn't notice that when switching to the ooblayout approach.

> This only affects controllers which use the default large page ops
> nand_ooblayout_ecc_lp.
> 
> E.g. for davinci:
> - nand has 2048 byte page, 64 byte oob
> - 3 byte ecc every 512 byte using 1bit hw ecc
> - requires 12 byte of oob space for ecc
> - old layout reserved 24 byte based on 64 byte oob
> 
> Old layout started using byte from offset 40 while new layout
> uses the last X bytes so it fits into the end of oob.
> Meaning in this example the old layout uses byte 40-51, while
> the new layout use byte 52-63.
> 
> Signed-off-by: Alexander Couzens <lynxis@fe80.eu>

Fixes + Cc-stable tags please.

> ---
>  drivers/mtd/nand/nand_base.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index b0524f8accb6..ba88345fd334 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -112,8 +112,25 @@ static int nand_ooblayout_ecc_lp(struct mtd_info *mtd, int section,
>  	if (section)
>  		return -ERANGE;
>  
> +	/* to be compatible with previous layouts
> +	 * 64 byte oob must have ecc data at byte 40,
> +	 * 128 byte oob must have ecc data at byte 80 */
> +	switch (mtd->oobsize) {
> +	case 64:
> +		oobregion->offset = 40;
> +		break;
> +	case 128:
> +		oobregion->offset = 80;
> +		break;
> +	default:
> +		oobregion->offset = mtd->oobsize - oobregion->length;
> +		break;
> +	}
> +
>  	oobregion->length = ecc->total;
> -	oobregion->offset = mtd->oobsize - oobregion->length;
> +	if (oobregion->offset + oobregion->length > mtd->oobsize) {
> +		return -ERANGE;
> +	}

By doing that you break new users of the standard large page layout
which expect all ECC bytes to be placed at the end of the OOB region.
In all cases except the one you're fixing (Hamming in 1bit/512bytes) we
want the ECC start offset to be calculated based on the actual number
of ECC bytes, and not the max number of ECC bytes considering the
strongest case.

I'd recommend that you add a new layout for Hamming ECC in the 64 or 128
OOB bytes situation to fix the problem.

Thanks,

Boris

[PATCH 0/3][v2] fixing 1bit hamming

[Alexander Couzens]

v1:
Use the old layout for all drivers/chips which doesn't set it explicit.

v2:
Only use the old layout for 1bit hamming if the layout isn't set
by the driver or devicetree.

Alexander Couzens (3):
  mtd/nand: add ooblayout for old hamming layout
  nand_base: use nand_ooblayout_lp_hamming_ops for 1bit hamming as
    default
  mtd: nand: davinci: set ECC algorithm explicitly for HW based ECC

 drivers/mtd/nand/davinci_nand.c |  4 ++-
 drivers/mtd/nand/nand_base.c    | 77 ++++++++++++++++++++++++++++++++++++++++-
 include/linux/mtd/nand.h        |  1 +
 3 files changed, 80 insertions(+), 2 deletions(-)

-- 
2.12.2

Re: [PATCH 0/3][v2] fixing 1bit hamming

[Boris Brezillon]

Hi Alexander,

Please use the "mtd: nand: " prefix for all NAND related patches.

On Tue,  2 May 2017 10:13:20 +0200
Alexander Couzens <lynxis@fe80.eu> wrote:

A detailed explanation of what you're trying to do here would have been
useful. As is, your cover letter is just useless.

> v1:
> Use the old layout for all drivers/chips which doesn't set it explicit.
> 
> v2:
> Only use the old layout for 1bit hamming if the layout isn't set
> by the driver or devicetree.
> 
> Alexander Couzens (3):
>   mtd/nand: add ooblayout for old hamming layout
>   nand_base: use nand_ooblayout_lp_hamming_ops for 1bit hamming as
>     default
>   mtd: nand: davinci: set ECC algorithm explicitly for HW based ECC
> 
>  drivers/mtd/nand/davinci_nand.c |  4 ++-
>  drivers/mtd/nand/nand_base.c    | 77 ++++++++++++++++++++++++++++++++++++++++-
>  include/linux/mtd/nand.h        |  1 +
>  3 files changed, 80 insertions(+), 2 deletions(-)
> 

[PATCH][v3] mtd: nand: add ooblayout for old hamming layout

[Alexander Couzens]

The old 1-bit hamming layout requires ECC data to be placed at a
fixed offset, and not necessarily at the end of the OOB area.
Add this old layout back in order to fix legacy setups.

Fixes: 41b207a70d3a ("mtd: nand: implement the default mtd_ooblayout_ops")
CC: Stable <stable@vger.kernel.org>
Signed-off-by: Alexander Couzens <lynxis@fe80.eu>
---
 drivers/mtd/nand/nand_base.c | 70 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 69 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index b0524f8accb6..44c0faf84426 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -139,6 +139,74 @@ const struct mtd_ooblayout_ops nand_ooblayout_lp_ops = {
 };
 EXPORT_SYMBOL_GPL(nand_ooblayout_lp_ops);
 
+/*
+ * Support the old "large page" layout used for 1-bit Hamming ECC where ECC
+ * are placed at a fixed offset.
+ */
+static int nand_ooblayout_ecc_lp_hamming(struct mtd_info *mtd, int section,
+					 struct mtd_oob_region *oobregion)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct nand_ecc_ctrl *ecc = &chip->ecc;
+
+	if (section)
+		return -ERANGE;
+
+	switch (mtd->oobsize) {
+	case 64:
+		oobregion->offset = 40;
+		break;
+	case 128:
+		oobregion->offset = 80;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	oobregion->length = ecc->total;
+	if (oobregion->offset + oobregion->length > mtd->oobsize)
+		return -ERANGE;
+
+	return 0;
+}
+
+static int nand_ooblayout_free_lp_hamming(struct mtd_info *mtd, int section,
+					  struct mtd_oob_region *oobregion)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct nand_ecc_ctrl *ecc = &chip->ecc;
+	int ecc_offset = 0;
+
+	if (section < 0 || section > 1)
+		return -ERANGE;
+
+	switch (mtd->oobsize) {
+	case 64:
+		ecc_offset = 40;
+		break;
+	case 128:
+		ecc_offset = 80;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (section == 0) {
+		oobregion->offset = 2;
+		oobregion->length = ecc_offset - 2;
+	} else {
+		oobregion->offset = ecc_offset + ecc->total;
+		oobregion->length = mtd->oobsize - oobregion->offset;
+	}
+
+	return 0;
+}
+
+const struct mtd_ooblayout_ops nand_ooblayout_lp_hamming_ops = {
+	.ecc = nand_ooblayout_ecc_lp_hamming,
+	.free = nand_ooblayout_free_lp_hamming,
+};
+
 static int check_offs_len(struct mtd_info *mtd,
 					loff_t ofs, uint64_t len)
 {
@@ -4653,7 +4721,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 			break;
 		case 64:
 		case 128:
-			mtd_set_ooblayout(mtd, &nand_ooblayout_lp_ops);
+			mtd_set_ooblayout(mtd, &nand_ooblayout_lp_hamming_ops);
 			break;
 		default:
 			WARN(1, "No oob scheme defined for oobsize %d\n",
-- 
2.12.2

Re: [PATCH][v3] mtd: nand: add ooblayout for old hamming layout

[Boris Brezillon]

+Brian

On Tue,  2 May 2017 12:19:00 +0200
Alexander Couzens <lynxis@fe80.eu> wrote:

> The old 1-bit hamming layout requires ECC data to be placed at a
> fixed offset, and not necessarily at the end of the OOB area.
> Add this old layout back in order to fix legacy setups.
> 
> Fixes: 41b207a70d3a ("mtd: nand: implement the default mtd_ooblayout_ops")
> CC: Stable <stable@vger.kernel.org>
> Signed-off-by: Alexander Couzens <lynxis@fe80.eu>

Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>

Brian, maybe you can consider taking this patch directly in
l2-mtd/master before preparing your PR to Linus (if you're happy with
the patch of course). Otherwise, I'll take it into my tree and send a
fixes PR after 4.12-rc1 is out.

> ---

Alexander, just for the record, you should put your changelog here.

>  drivers/mtd/nand/nand_base.c | 70 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 69 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index b0524f8accb6..44c0faf84426 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -139,6 +139,74 @@ const struct mtd_ooblayout_ops nand_ooblayout_lp_ops = {
>  };
>  EXPORT_SYMBOL_GPL(nand_ooblayout_lp_ops);
>  
> +/*
> + * Support the old "large page" layout used for 1-bit Hamming ECC where ECC
> + * are placed at a fixed offset.
> + */
> +static int nand_ooblayout_ecc_lp_hamming(struct mtd_info *mtd, int section,
> +					 struct mtd_oob_region *oobregion)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +	struct nand_ecc_ctrl *ecc = &chip->ecc;
> +
> +	if (section)
> +		return -ERANGE;
> +
> +	switch (mtd->oobsize) {
> +	case 64:
> +		oobregion->offset = 40;
> +		break;
> +	case 128:
> +		oobregion->offset = 80;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	oobregion->length = ecc->total;
> +	if (oobregion->offset + oobregion->length > mtd->oobsize)
> +		return -ERANGE;
> +
> +	return 0;
> +}
> +
> +static int nand_ooblayout_free_lp_hamming(struct mtd_info *mtd, int section,
> +					  struct mtd_oob_region *oobregion)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +	struct nand_ecc_ctrl *ecc = &chip->ecc;
> +	int ecc_offset = 0;
> +
> +	if (section < 0 || section > 1)
> +		return -ERANGE;
> +
> +	switch (mtd->oobsize) {
> +	case 64:
> +		ecc_offset = 40;
> +		break;
> +	case 128:
> +		ecc_offset = 80;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (section == 0) {
> +		oobregion->offset = 2;
> +		oobregion->length = ecc_offset - 2;
> +	} else {
> +		oobregion->offset = ecc_offset + ecc->total;
> +		oobregion->length = mtd->oobsize - oobregion->offset;
> +	}
> +
> +	return 0;
> +}
> +
> +const struct mtd_ooblayout_ops nand_ooblayout_lp_hamming_ops = {
> +	.ecc = nand_ooblayout_ecc_lp_hamming,
> +	.free = nand_ooblayout_free_lp_hamming,
> +};
> +
>  static int check_offs_len(struct mtd_info *mtd,
>  					loff_t ofs, uint64_t len)
>  {
> @@ -4653,7 +4721,7 @@ int nand_scan_tail(struct mtd_info *mtd)
>  			break;
>  		case 64:
>  		case 128:
> -			mtd_set_ooblayout(mtd, &nand_ooblayout_lp_ops);
> +			mtd_set_ooblayout(mtd, &nand_ooblayout_lp_hamming_ops);
>  			break;
>  		default:
>  			WARN(1, "No oob scheme defined for oobsize %d\n",

Re: [PATCH][v3] mtd: nand: add ooblayout for old hamming layout

[Brian Norris]

On Tue, May 02, 2017 at 02:03:00PM +0200, Boris Brezillon wrote:
> +Brian
> 
> On Tue,  2 May 2017 12:19:00 +0200
> Alexander Couzens <lynxis@fe80.eu> wrote:
> 
> > The old 1-bit hamming layout requires ECC data to be placed at a
> > fixed offset, and not necessarily at the end of the OOB area.
> > Add this old layout back in order to fix legacy setups.
> > 
> > Fixes: 41b207a70d3a ("mtd: nand: implement the default mtd_ooblayout_ops")
> > CC: Stable <stable@vger.kernel.org>
> > Signed-off-by: Alexander Couzens <lynxis@fe80.eu>
> 
> Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> 
> Brian, maybe you can consider taking this patch directly in
> l2-mtd/master before preparing your PR to Linus (if you're happy with
> the patch of course). Otherwise, I'll take it into my tree and send a
> fixes PR after 4.12-rc1 is out.

Applied to l2-mtd.git/master.

[PATCH 1/3][v2] mtd/nand: add ooblayout for old hamming layout

[Alexander Couzens]

The old 1bit hamming layout requires the ecc data to be exact at
predefined offsets. This can not changed because old installations
would break.

Signed-off-by: Alexander Couzens <lynxis@fe80.eu>
---
 drivers/mtd/nand/nand_base.c | 71 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mtd/nand.h     |  1 +
 2 files changed, 72 insertions(+)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index b0524f8accb6..daf3df157885 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -139,6 +139,77 @@ const struct mtd_ooblayout_ops nand_ooblayout_lp_ops = {
 };
 EXPORT_SYMBOL_GPL(nand_ooblayout_lp_ops);
 
+/* support the old large page layout for Hamming 1 bit where the ECC start at
+ * a defined offset. Should only used by old devices to compatible with old
+ * layout. Use nand_ooblayout_lp_ops if possible.
+ */
+static int nand_ooblayout_ecc_lp_hamming(struct mtd_info *mtd, int section,
+				 struct mtd_oob_region *oobregion)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct nand_ecc_ctrl *ecc = &chip->ecc;
+
+	if (section)
+		return -ERANGE;
+
+	switch (mtd->oobsize) {
+	case 64:
+		oobregion->offset = 40;
+		break;
+	case 128:
+		oobregion->offset = 80;
+		break;
+	default:
+		return -ERANGE;
+	}
+
+	oobregion->length = ecc->total;
+	if (oobregion->offset + oobregion->length > mtd->oobsize)
+		return -ERANGE;
+
+	return 0;
+}
+
+static int nand_ooblayout_free_lp_hamming(struct mtd_info *mtd, int section,
+				  struct mtd_oob_region *oobregion)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct nand_ecc_ctrl *ecc = &chip->ecc;
+	int ecc_offset = 0;
+
+	if (section < 0 || section > 1)
+		return -ERANGE;
+
+	switch (mtd->oobsize) {
+	case 64:
+		ecc_offset = 40;
+		break;
+	case 128:
+		ecc_offset = 80;
+		break;
+	default:
+		return -ERANGE;
+	}
+
+	if (section == 0) {
+		/* return the space before the ecc */
+		oobregion->offset = 2;
+		oobregion->length = ecc_offset - 2;
+	} else { /* section == 1 */
+		/* return free space after ecc */
+		oobregion->offset = ecc_offset + ecc->total;
+		oobregion->length = mtd->oobsize - oobregion->offset;
+	}
+
+	return 0;
+}
+
+const struct mtd_ooblayout_ops nand_ooblayout_lp_hamming_ops = {
+	.ecc = nand_ooblayout_ecc_lp_hamming,
+	.free = nand_ooblayout_free_lp_hamming,
+};
+EXPORT_SYMBOL_GPL(nand_ooblayout_lp_hamming_ops);
+
 static int check_offs_len(struct mtd_info *mtd,
 					loff_t ofs, uint64_t len)
 {
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 9591e0fbe5bd..da33933950a5 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -914,6 +914,7 @@ struct nand_chip {
 
 extern const struct mtd_ooblayout_ops nand_ooblayout_sp_ops;
 extern const struct mtd_ooblayout_ops nand_ooblayout_lp_ops;
+extern const struct mtd_ooblayout_ops nand_ooblayout_lp_hamming_ops;
 
 static inline void nand_set_flash_node(struct nand_chip *chip,
 				       struct device_node *np)
-- 
2.12.2

Re: [PATCH 1/3][v2] mtd/nand: add ooblayout for old hamming layout

[Boris Brezillon]

On Tue,  2 May 2017 10:13:21 +0200
Alexander Couzens <lynxis@fe80.eu> wrote:

> The old 1bit hamming layout requires the ecc data to be exact at
> predefined offsets.

"
The old 1-bit hamming layout requires ECC data to be placed at a
fixed offset, and not necessarily at the end of the OOB area.
"

> This can not changed because old installations
> would break.

"
Add this old layout back in order to fix legacy setup.
"

> 
> Signed-off-by: Alexander Couzens <lynxis@fe80.eu>

You should probably add Fixes and Cc:stable tags.

> ---
>  drivers/mtd/nand/nand_base.c | 71 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mtd/nand.h     |  1 +
>  2 files changed, 72 insertions(+)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index b0524f8accb6..daf3df157885 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -139,6 +139,77 @@ const struct mtd_ooblayout_ops nand_ooblayout_lp_ops = {
>  };
>  EXPORT_SYMBOL_GPL(nand_ooblayout_lp_ops);
>  
> +/* support the old large page layout for Hamming 1 bit where the ECC start at
> + * a defined offset.

Please use regular comment style:

/*
 * <content>
 */

s/support/Support/

BTW, I'm not sure I understand this sentence. How about

"
Support the old "large page" layout used for 1-bit Hamming ECC where ECC
are placed at a fixed offset.
"

> Should only used by old devices to compatible with old
> + * layout. Use nand_ooblayout_lp_ops if possible.
> + */
> +static int nand_ooblayout_ecc_lp_hamming(struct mtd_info *mtd, int section,
> +				 struct mtd_oob_region *oobregion)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +	struct nand_ecc_ctrl *ecc = &chip->ecc;
> +
> +	if (section)
> +		return -ERANGE;
> +
> +	switch (mtd->oobsize) {
> +	case 64:
> +		oobregion->offset = 40;
> +		break;
> +	case 128:
> +		oobregion->offset = 80;
> +		break;
> +	default:
> +		return -ERANGE;

Hm, can you return -EINVAL instead?

> +	}
> +
> +	oobregion->length = ecc->total;
> +	if (oobregion->offset + oobregion->length > mtd->oobsize)
> +		return -ERANGE;
> +
> +	return 0;
> +}
> +
> +static int nand_ooblayout_free_lp_hamming(struct mtd_info *mtd, int section,
> +				  struct mtd_oob_region *oobregion)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +	struct nand_ecc_ctrl *ecc = &chip->ecc;
> +	int ecc_offset = 0;
> +
> +	if (section < 0 || section > 1)
> +		return -ERANGE;
> +
> +	switch (mtd->oobsize) {
> +	case 64:
> +		ecc_offset = 40;
> +		break;
> +	case 128:
> +		ecc_offset = 80;
> +		break;
> +	default:
> +		return -ERANGE;

Ditto.

> +	}
> +
> +	if (section == 0) {
> +		/* return the space before the ecc */

Not sure this comment is needed, it's pretty obvious that this section
is placed before the ECC bytes (and after the BBM).

> +		oobregion->offset = 2;
> +		oobregion->length = ecc_offset - 2;
> +	} else { /* section == 1 */

Drop this comment.

> +		/* return free space after ecc */

Ditto.

> +		oobregion->offset = ecc_offset + ecc->total;
> +		oobregion->length = mtd->oobsize - oobregion->offset;
> +	}
> +
> +	return 0;
> +}
> +
> +const struct mtd_ooblayout_ops nand_ooblayout_lp_hamming_ops = {
> +	.ecc = nand_ooblayout_ecc_lp_hamming,
> +	.free = nand_ooblayout_free_lp_hamming,
> +};
> +EXPORT_SYMBOL_GPL(nand_ooblayout_lp_hamming_ops);
> +
>  static int check_offs_len(struct mtd_info *mtd,
>  					loff_t ofs, uint64_t len)
>  {
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 9591e0fbe5bd..da33933950a5 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -914,6 +914,7 @@ struct nand_chip {
>  
>  extern const struct mtd_ooblayout_ops nand_ooblayout_sp_ops;
>  extern const struct mtd_ooblayout_ops nand_ooblayout_lp_ops;
> +extern const struct mtd_ooblayout_ops nand_ooblayout_lp_hamming_ops;
>  
>  static inline void nand_set_flash_node(struct nand_chip *chip,
>  				       struct device_node *np)

Re: [PATCH 1/3][v2] mtd/nand: add ooblayout for old hamming layout

[Boris Brezillon]

Just a nit, I prefer "mtd: nand: " instead of "mtd/nand: ", and since
you have to send a v3 anyway, I thought it wouldn't be a problem to
mention that ;-).

On Tue,  2 May 2017 10:13:21 +0200
Alexander Couzens <lynxis@fe80.eu> wrote:

> The old 1bit hamming layout requires the ecc data to be exact at
> predefined offsets. This can not changed because old installations
> would break.
> 
> Signed-off-by: Alexander Couzens <lynxis@fe80.eu>
> ---
>  drivers/mtd/nand/nand_base.c | 71 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mtd/nand.h     |  1 +
>  2 files changed, 72 insertions(+)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index b0524f8accb6..daf3df157885 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -139,6 +139,77 @@ const struct mtd_ooblayout_ops nand_ooblayout_lp_ops = {
>  };
>  EXPORT_SYMBOL_GPL(nand_ooblayout_lp_ops);
>  
> +/* support the old large page layout for Hamming 1 bit where the ECC start at
> + * a defined offset. Should only used by old devices to compatible with old
> + * layout. Use nand_ooblayout_lp_ops if possible.
> + */
> +static int nand_ooblayout_ecc_lp_hamming(struct mtd_info *mtd, int section,
> +				 struct mtd_oob_region *oobregion)

Another nit: try to align arguments to the open parenthesis when
possible.

> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +	struct nand_ecc_ctrl *ecc = &chip->ecc;
> +
> +	if (section)
> +		return -ERANGE;
> +
> +	switch (mtd->oobsize) {
> +	case 64:
> +		oobregion->offset = 40;
> +		break;
> +	case 128:
> +		oobregion->offset = 80;
> +		break;
> +	default:
> +		return -ERANGE;
> +	}
> +
> +	oobregion->length = ecc->total;
> +	if (oobregion->offset + oobregion->length > mtd->oobsize)
> +		return -ERANGE;
> +
> +	return 0;
> +}
> +
> +static int nand_ooblayout_free_lp_hamming(struct mtd_info *mtd, int section,
> +				  struct mtd_oob_region *oobregion)

Ditto.

> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +	struct nand_ecc_ctrl *ecc = &chip->ecc;
> +	int ecc_offset = 0;
> +
> +	if (section < 0 || section > 1)
> +		return -ERANGE;
> +
> +	switch (mtd->oobsize) {
> +	case 64:
> +		ecc_offset = 40;
> +		break;
> +	case 128:
> +		ecc_offset = 80;
> +		break;
> +	default:
> +		return -ERANGE;
> +	}
> +
> +	if (section == 0) {
> +		/* return the space before the ecc */
> +		oobregion->offset = 2;
> +		oobregion->length = ecc_offset - 2;
> +	} else { /* section == 1 */
> +		/* return free space after ecc */
> +		oobregion->offset = ecc_offset + ecc->total;
> +		oobregion->length = mtd->oobsize - oobregion->offset;
> +	}
> +
> +	return 0;
> +}
> +
> +const struct mtd_ooblayout_ops nand_ooblayout_lp_hamming_ops = {
> +	.ecc = nand_ooblayout_ecc_lp_hamming,
> +	.free = nand_ooblayout_free_lp_hamming,
> +};
> +EXPORT_SYMBOL_GPL(nand_ooblayout_lp_hamming_ops);

Please do not export symbols if it's not really needed.

> +
>  static int check_offs_len(struct mtd_info *mtd,
>  					loff_t ofs, uint64_t len)
>  {
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 9591e0fbe5bd..da33933950a5 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -914,6 +914,7 @@ struct nand_chip {
>  
>  extern const struct mtd_ooblayout_ops nand_ooblayout_sp_ops;
>  extern const struct mtd_ooblayout_ops nand_ooblayout_lp_ops;
> +extern const struct mtd_ooblayout_ops nand_ooblayout_lp_hamming_ops;

Drop that line too.

>  
>  static inline void nand_set_flash_node(struct nand_chip *chip,
>  				       struct device_node *np)

[PATCH 2/3][v2] nand_base: use nand_ooblayout_lp_hamming_ops for 1bit hamming as default

[Alexander Couzens]

commit 41b207a70d3a ("mtd: nand: implement the default mtd_ooblayout_ops")
uses a different ooblayout for platforms which doesn't set the
layout by itself. Use for 1bit hamming the old layout.

Signed-off-by: Alexander Couzens <lynxis@fe80.eu>
---
 drivers/mtd/nand/nand_base.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index daf3df157885..6a9e99377181 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -4724,7 +4724,11 @@ int nand_scan_tail(struct mtd_info *mtd)
 			break;
 		case 64:
 		case 128:
-			mtd_set_ooblayout(mtd, &nand_ooblayout_lp_ops);
+			if (ecc->algo == NAND_ECC_HAMMING && ecc->strength == 1)
+				mtd_set_ooblayout(mtd,
+						&nand_ooblayout_lp_hamming_ops);
+			else
+				mtd_set_ooblayout(mtd, &nand_ooblayout_lp_ops);
 			break;
 		default:
 			WARN(1, "No oob scheme defined for oobsize %d\n",
-- 
2.12.2

Re: [PATCH 2/3][v2] nand_base: use nand_ooblayout_lp_hamming_ops for 1bit hamming as default

[Boris Brezillon]

On Tue,  2 May 2017 10:13:22 +0200
Alexander Couzens <lynxis@fe80.eu> wrote:

> commit 41b207a70d3a ("mtd: nand: implement the default mtd_ooblayout_ops")
> uses a different ooblayout for platforms which doesn't set the
> layout by itself. Use for 1bit hamming the old layout.

Please merge patch 1 and 2 so that we end up with a
simple/self-contained patch that can be backported to stable kernels.

> 
> Signed-off-by: Alexander Couzens <lynxis@fe80.eu>
> ---
>  drivers/mtd/nand/nand_base.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index daf3df157885..6a9e99377181 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -4724,7 +4724,11 @@ int nand_scan_tail(struct mtd_info *mtd)
>  			break;
>  		case 64:
>  		case 128:
> -			mtd_set_ooblayout(mtd, &nand_ooblayout_lp_ops);
> +			if (ecc->algo == NAND_ECC_HAMMING && ecc->strength == 1)
> +				mtd_set_ooblayout(mtd,
> +						&nand_ooblayout_lp_hamming_ops);
> +			else
> +				mtd_set_ooblayout(mtd, &nand_ooblayout_lp_ops);

Why not unconditionally setting it to nand_ooblayout_lp_hamming_ops to
restore the initial behavior. If one wants to use the new "large page"
layout he can still explicitly assign it (which is a good practice
anyway).

>  			break;
>  		default:
>  			WARN(1, "No oob scheme defined for oobsize %d\n",

[PATCH 3/3][v3] mtd: nand: davinci: set ECC algorithm explicitly for HW based ECC

[Alexander Couzens]

Required to set the ooblayout based on the algorithm.

Signed-off-by: Alexander Couzens <lynxis@fe80.eu>
---
 drivers/mtd/nand/davinci_nand.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
index 27fa8b87cd5f..9e5167496ce1 100644
--- a/drivers/mtd/nand/davinci_nand.c
+++ b/drivers/mtd/nand/davinci_nand.c
@@ -760,11 +760,13 @@ static int nand_davinci_probe(struct platform_device *pdev)
 			info->chip.ecc.hwctl = nand_davinci_hwctl_4bit;
 			info->chip.ecc.bytes = 10;
 			info->chip.ecc.options = NAND_ECC_GENERIC_ERASED_CHECK;
-		} else {
+			info->chip.ecc.algo = NAND_ECC_BCH;
+		} else { /* 1bit ecc hamming */
 			info->chip.ecc.calculate = nand_davinci_calculate_1bit;
 			info->chip.ecc.correct = nand_davinci_correct_1bit;
 			info->chip.ecc.hwctl = nand_davinci_hwctl_1bit;
 			info->chip.ecc.bytes = 3;
+			info->chip.ecc.algo = NAND_ECC_HAMMING;
 		}
 		info->chip.ecc.size = 512;
 		info->chip.ecc.strength = pdata->ecc_bits;
-- 
2.12.2

Re: [PATCH 3/3][v3] mtd: nand: davinci: set ECC algorithm explicitly for HW based ECC

[Boris Brezillon]

On Tue,  2 May 2017 10:13:23 +0200
Alexander Couzens <lynxis@fe80.eu> wrote:

> Required to set the ooblayout based on the algorithm.

Not strictly required if you take my comment into account (see my
reply to patch 2). This being said, it's always to have this kind of
information, I'll just have to check if it doesn't conflict with
Masahiro's patches.

> 
> Signed-off-by: Alexander Couzens <lynxis@fe80.eu>
> ---
>  drivers/mtd/nand/davinci_nand.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
> index 27fa8b87cd5f..9e5167496ce1 100644
> --- a/drivers/mtd/nand/davinci_nand.c
> +++ b/drivers/mtd/nand/davinci_nand.c
> @@ -760,11 +760,13 @@ static int nand_davinci_probe(struct platform_device *pdev)
>  			info->chip.ecc.hwctl = nand_davinci_hwctl_4bit;
>  			info->chip.ecc.bytes = 10;
>  			info->chip.ecc.options = NAND_ECC_GENERIC_ERASED_CHECK;
> -		} else {
> +			info->chip.ecc.algo = NAND_ECC_BCH;
> +		} else { /* 1bit ecc hamming */

Please put this comment on the next line.

>  			info->chip.ecc.calculate = nand_davinci_calculate_1bit;
>  			info->chip.ecc.correct = nand_davinci_correct_1bit;
>  			info->chip.ecc.hwctl = nand_davinci_hwctl_1bit;
>  			info->chip.ecc.bytes = 3;
> +			info->chip.ecc.algo = NAND_ECC_HAMMING;
>  		}
>  		info->chip.ecc.size = 512;
>  		info->chip.ecc.strength = pdata->ecc_bits;

Re: [PATCH 3/3][v3] mtd: nand: davinci: set ECC algorithm explicitly for HW based ECC

[Boris Brezillon]

On Tue, 2 May 2017 11:00:45 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> On Tue,  2 May 2017 10:13:23 +0200
> Alexander Couzens <lynxis@fe80.eu> wrote:
> 
> > Required to set the ooblayout based on the algorithm.  
> 
> Not strictly required if you take my comment into account (see my
> reply to patch 2). This being said, it's always to have this kind of
> information, I'll just have to check if it doesn't conflict with
> Masahiro's patches.

Never mind, I'm just mixing up davinci and denali :-).

> 
> > 
> > Signed-off-by: Alexander Couzens <lynxis@fe80.eu>
> > ---
> >  drivers/mtd/nand/davinci_nand.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
> > index 27fa8b87cd5f..9e5167496ce1 100644
> > --- a/drivers/mtd/nand/davinci_nand.c
> > +++ b/drivers/mtd/nand/davinci_nand.c
> > @@ -760,11 +760,13 @@ static int nand_davinci_probe(struct platform_device *pdev)
> >  			info->chip.ecc.hwctl = nand_davinci_hwctl_4bit;
> >  			info->chip.ecc.bytes = 10;
> >  			info->chip.ecc.options = NAND_ECC_GENERIC_ERASED_CHECK;
> > -		} else {
> > +			info->chip.ecc.algo = NAND_ECC_BCH;
> > +		} else { /* 1bit ecc hamming */  
> 
> Please put this comment on the next line.
> 
> >  			info->chip.ecc.calculate = nand_davinci_calculate_1bit;
> >  			info->chip.ecc.correct = nand_davinci_correct_1bit;
> >  			info->chip.ecc.hwctl = nand_davinci_hwctl_1bit;
> >  			info->chip.ecc.bytes = 3;
> > +			info->chip.ecc.algo = NAND_ECC_HAMMING;
> >  		}
> >  		info->chip.ecc.size = 512;
> >  		info->chip.ecc.strength = pdata->ecc_bits;  
>