On Thu, 19 Oct 2017 17:23:24 +0300
Roger Quadros [off-list ref] wrote:

On 19/10/17 17:20, Boris Brezillon wrote:

quoted

On Thu, 19 Oct 2017 17:11:34 +0300
Roger Quadros [off-list ref] wrote:
  

quoted

On 19/10/17 16:51, Boris Brezillon wrote:  

quoted

On Thu, 19 Oct 2017 11:41:29 +0300
Roger Quadros [off-list ref] wrote:
    

quoted

Since v4.12, NAND subpage writes were causing a NULL pointer
dereference on OMAP platforms (omap2-nand) using OMAP_ECC_BCH4_CODE_HW,
OMAP_ECC_BCH8_CODE_HW and OMAP_ECC_BCH16_CODE_HW.

This is because for those ECC modes, omap_calculate_ecc_bch()
generates ECC bytes for the entire (multi-sector) page and this can
overflow the ECC buffer provided by nand_write_subpage_hwecc()
as it expects ecc.calculate() to return ECC bytes for just one sector.

However, the root cause of the problem is present much before
v4.12 but was not seen then as NAND buffers were being allocated
as one big chunck prior to
commit 3deb9979c731 ("mtd: nand: allocate aligned buffers if NAND_OWN_BUFFERS is unset")

Fix the issue by providing a OMAP optimized write_subpage() implementation.

cc: <redacted> # v4.12+
Signed-off-by: Roger Quadros <redacted>
---
 drivers/mtd/nand/omap2.c | 338 +++++++++++++++++++++++++++++++----------------
 1 file changed, 225 insertions(+), 113 deletions(-)

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 54540c8..a0bd456 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c

@@ -1133,129 +1133,172 @@ static u8  bch8_polynomial[] = {0xef, 0x51, 0x2e, 0x09, 0xed, 0x93, 0x9a, 0xc2,
 				0x97, 0x79, 0xe5, 0x24, 0xb5};
     

<snip>
 

quoted

quoted

+
+/**
  * omap_read_page_bch - BCH ecc based page read function for entire page
  * @mtd:		mtd info structure
  * @chip:		nand chip info structure

@@ -2044,7 +2153,7 @@ static int omap_nand_probe(struct platform_device *pdev)
 		nand_chip->ecc.strength		= 4;
 		nand_chip->ecc.hwctl		= omap_enable_hwecc_bch;
 		nand_chip->ecc.correct		= nand_bch_correct_data;
-		nand_chip->ecc.calculate	= omap_calculate_ecc_bch;
+		nand_chip->ecc.calculate	= omap_calculate_ecc_bch_sw;
 		mtd_set_ooblayout(mtd, &omap_sw_ooblayout_ops);
 		/* Reserve one byte for the OMAP marker */
 		oobbytes_per_step		= nand_chip->ecc.bytes + 1;

@@ -2066,9 +2175,10 @@ static int omap_nand_probe(struct platform_device *pdev)
 		nand_chip->ecc.strength		= 4;
 		nand_chip->ecc.hwctl		= omap_enable_hwecc_bch;
 		nand_chip->ecc.correct		= omap_elm_correct_data;
-		nand_chip->ecc.calculate	= omap_calculate_ecc_bch;
+		nand_chip->ecc.calculate	= omap_calculate_ecc_bch_multi;
 		nand_chip->ecc.read_page	= omap_read_page_bch;
 		nand_chip->ecc.write_page	= omap_write_page_bch;
+		nand_chip->ecc.write_subpage	= omap_write_subpage_bch;
 		mtd_set_ooblayout(mtd, &omap_ooblayout_ops);
 		oobbytes_per_step		= nand_chip->ecc.bytes;
 

@@ -2087,7 +2197,7 @@ static int omap_nand_probe(struct platform_device *pdev)
 		nand_chip->ecc.strength		= 8;
 		nand_chip->ecc.hwctl		= omap_enable_hwecc_bch;
 		nand_chip->ecc.correct		= nand_bch_correct_data;
-		nand_chip->ecc.calculate	= omap_calculate_ecc_bch;
+		nand_chip->ecc.calculate	= omap_calculate_ecc_bch_sw;
 		mtd_set_ooblayout(mtd, &omap_sw_ooblayout_ops);
 		/* Reserve one byte for the OMAP marker */
 		oobbytes_per_step		= nand_chip->ecc.bytes + 1;

@@ -2109,9 +2219,10 @@ static int omap_nand_probe(struct platform_device *pdev)
 		nand_chip->ecc.strength		= 8;
 		nand_chip->ecc.hwctl		= omap_enable_hwecc_bch;
 		nand_chip->ecc.correct		= omap_elm_correct_data;
-		nand_chip->ecc.calculate	= omap_calculate_ecc_bch;
+		nand_chip->ecc.calculate	= omap_calculate_ecc_bch_multi;    

Hm, it still looks wrong. omap_calculate_ecc_bch_multi() will generate
the same overflow when called by the core, or am I missing something?
    

In the current setup core will never call ecc.calculate as we're overriding every op
that can be used.  

Do you have a custom ->read_subpage()? If you don't, the core will use
nand_read_subpage(), and it's calling ->calculate() internally.  

read_subpage() will only be used if NAND_SUBPAGE_READ is set. We don't set it for omap2-nand.

Right, I always forget NAND_SUBPAGE_READ is an opt-in flag.

quoted

quoted

The thing is that omap driver code uses these hooks as is so I wasn't sure
if I should change the caller code to call the multi versions directly and fix these
hooks to single sector versions.  

I'd prefer this solution.
  

quoted

Alternatively, is it OK to set them to NULL?  

Hm, I'm pretty sure it's not, see my comment about ->read_subpage().
  

Now is it?

It should be. Note that ->hwctl() shouldn't be used by the core either,
so if the only user is the driver itself, it might be good to get rid of
the ecc->hwctl = omap_enable_hwecc_bch assignement and call
omap_enable_hwecc_bch() directly. Just suggesting that as a follow-up
patch, let's try to keep the fix as small as possible.

quoted

quoted

quoted

quoted

 		nand_chip->ecc.read_page	= omap_read_page_bch;
 		nand_chip->ecc.write_page	= omap_write_page_bch;
+		nand_chip->ecc.write_subpage	= omap_write_subpage_bch;
 		mtd_set_ooblayout(mtd, &omap_ooblayout_ops);
 		oobbytes_per_step		= nand_chip->ecc.bytes;
 

@@ -2131,9 +2242,10 @@ static int omap_nand_probe(struct platform_device *pdev)
 		nand_chip->ecc.strength		= 16;
 		nand_chip->ecc.hwctl		= omap_enable_hwecc_bch;
 		nand_chip->ecc.correct		= omap_elm_correct_data;
-		nand_chip->ecc.calculate	= omap_calculate_ecc_bch;
+		nand_chip->ecc.calculate	= omap_calculate_ecc_bch_multi;
 		nand_chip->ecc.read_page	= omap_read_page_bch;
 		nand_chip->ecc.write_page	= omap_write_page_bch;
+		nand_chip->ecc.write_subpage	= omap_write_subpage_bch;
 		mtd_set_ooblayout(mtd, &omap_ooblayout_ops);
 		oobbytes_per_step		= nand_chip->ecc.bytes;