From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754791AbdJSOMO (ORCPT ); Thu, 19 Oct 2017 10:12:14 -0400 Received: from fllnx210.ext.ti.com ([198.47.19.17]:14782 "EHLO fllnx210.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752697AbdJSOMI (ORCPT ); Thu, 19 Oct 2017 10:12:08 -0400 Subject: Re: [PATCH] mtd: nand: omap2: Fix subpage write To: Boris Brezillon CC: , , , , , "# v4 . 12+" References: <1508402489-7436-1-git-send-email-rogerq@ti.com> <20171019155140.027d9b32@bbrezillon> From: Roger Quadros Message-ID: <6ca33c39-e425-ff8b-f50a-3ce30b6c2d34@ti.com> Date: Thu, 19 Oct 2017 17:11:34 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171019155140.027d9b32@bbrezillon> Content-Type: text/plain; charset="utf-8" Content-Language: en-GB Content-Transfer-Encoding: 8bit X-EXCLAIMER-MD-CONFIG: e1e8a2fd-e40a-4ac6-ac9b-f7e9cc9ee180 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 19/10/17 16:51, Boris Brezillon wrote: > On Thu, 19 Oct 2017 11:41:29 +0300 > Roger Quadros wrote: > >> 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: # v4.12+ >> Signed-off-by: Roger Quadros >> --- >> 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}; >> >> + >> +/** >> * 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. 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. Alternatively, is it OK to set them to NULL? >> 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; >> > -- cheers, -roger Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki From mboxrd@z Thu Jan 1 00:00:00 1970 From: Roger Quadros Subject: Re: [PATCH] mtd: nand: omap2: Fix subpage write Date: Thu, 19 Oct 2017 17:11:34 +0300 Message-ID: <6ca33c39-e425-ff8b-f50a-3ce30b6c2d34@ti.com> References: <1508402489-7436-1-git-send-email-rogerq@ti.com> <20171019155140.027d9b32@bbrezillon> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit Return-path: In-Reply-To: <20171019155140.027d9b32@bbrezillon> Content-Language: en-GB Sender: linux-kernel-owner@vger.kernel.org To: Boris Brezillon Cc: richard@nod.at, tony@atomide.com, linux-mtd@lists.infradead.org, linux-omap@vger.kernel.org, linux-kernel@vger.kernel.org, "# v4 . 12+" List-Id: linux-omap@vger.kernel.org On 19/10/17 16:51, Boris Brezillon wrote: > On Thu, 19 Oct 2017 11:41:29 +0300 > Roger Quadros wrote: > >> 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: # v4.12+ >> Signed-off-by: Roger Quadros >> --- >> 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}; >> >> + >> +/** >> * 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. 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. Alternatively, is it OK to set them to NULL? >> 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; >> > -- cheers, -roger Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki