linux-kernel - Re: [PATCH v6 00/17] memory: omap-gpmc: mtd: nand: Support GPMC NAND on non-OMAP platforms

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 18 Apr 2016 17:39:01 +0300
From:	Roger Quadros <rogerq@...com>
To:	Boris Brezillon <boris.brezillon@...e-electrons.com>
CC:	Tony Lindgren <tony@...mide.com>, <javier@...hile0.org>,
	<fcooper@...com>, <nsekhar@...com>,
	<linux-mtd@...ts.infradead.org>, <linux-omap@...r.kernel.org>,
	<computersforpeace@...il.com>, <dwmw2@...radead.org>,
	<ezequiel@...guardiasur.com.ar>, <devicetree@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v6 00/17] memory: omap-gpmc: mtd: nand: Support GPMC NAND
 on non-OMAP platforms

On 18/04/16 17:10, Boris Brezillon wrote:
> On Mon, 18 Apr 2016 16:48:26 +0300
> Roger Quadros <rogerq@...com> wrote:
> 
>> Boris,
>>
>> On 18/04/16 16:13, Boris Brezillon wrote:
>>> Hi Roger,
>>>
>>> On Mon, 18 Apr 2016 15:52:58 +0300
>>> Roger Quadros <rogerq@...com> wrote:
>>>
>>>> On 18/04/16 15:31, Roger Quadros wrote:
>>>>> On 16/04/16 11:57, Boris Brezillon wrote:
>>>>>> On Fri, 15 Apr 2016 09:19:51 -0700
>>>>>> Tony Lindgren <tony@...mide.com> wrote:
>>>>>>
>>>>>>>
>>>>>>>> Or should I just pull this immutable branch in my current nand/next and
>>>>>>>> let you pull the same immutable branch in omap-soc. I mean, would this
>>>>>>>> prevent conflicts when our branches are merged into linux-next, no
>>>>>>>> matter the order.
>>>>>>>
>>>>>>> Ideally just one or more branches with just minimal changes in
>>>>>>> them against -rc1. But you may have other dependencies in
>>>>>>> your NAND tree so that may no longer be doable :) Usually if
>>>>>>> I merge something that may need to get merged into other
>>>>>>> branches, I just apply them into a separate branch against -rc1
>>>>>>> to start with, then merge that branch in.
>>>>>>
>>>>>> Okay, in this case, that's pretty much what I did from the beginning,
>>>>>> except the immutable branch was provided by Roger (based on 4.6-rc1).
>>>>>> Thanks for this detailed explanation, I'll try to remember that when
>>>>>> I'll need to provide an immutable branch for another subsystem.
>>>>>>
>>>>>> Roger, my request remains, could you check/test my conflict resolution
>>>>>> (branch nand/next-with-gpmc-rework)?
>>>>>
>>>>> I couldn't test that branch yet as nand/next is broken on omap platforms
>>>>> (at least on dra7-evm).
>>>>>
>>>>> The commit where it breaks is:
>>>>> a662ef4 mtd: nand: omap2: use mtd_ooblayout_xxx() helpers where appropriate
>>>>>
>>>>> I'm trying to figure out what went wrong there. Failure log below.
>>>>
>>>> OK. I was able to fix it when at commit a662ef4 with the below patch.
>>>
>>> Thanks for debugging that.
>>>
>>>>
>>>> Looks like we need to read exactly the ECC bytes through the ECC engine and not
>>>> the entire OOB region.
>>>
>>> Hm, it looks like there's a bug somewhere else, because I don't see any
>>> reason why the controller wouldn't be able to read the full OOB region.
>>
>> The controller can read the full OOB region but we only want it to read just
>> the ECC bytes because that is the way the ELM ECC engine works.
> 
> Ok, I think I got it: the ECC correction is pipelined with data read,
> and the controller expect to have ECC bytes right after the in-band
> data, is that correct?

That is correct.

> 
>>
>>>>
>>>> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
>>>> index e622a1b..46b61d2 100644
>>>> --- a/drivers/mtd/nand/omap2.c
>>>> +++ b/drivers/mtd/nand/omap2.c
>>>> @@ -1547,8 +1547,8 @@ static int omap_read_page_bch(struct mtd_info *mtd, struct nand_chip *chip,
>>>>  	chip->read_buf(mtd, buf, mtd->writesize);
>>>>  
>>>>  	/* Read oob bytes */
>>>> -	chip->cmdfunc(mtd, NAND_CMD_RNDOUT, mtd->writesize, -1);
>>>> -	chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
>>>> +	chip->cmdfunc(mtd, NAND_CMD_RNDOUT, mtd->writesize + chip->ecc.layout->eccpos[0], -1);
>>>
>>> The whole point of this series is to get rid of chip->ecc.layout, so
>>> we'd rather use the mtd_ooblayout_find_eccregion() instead of
>>> chip->ecc.layout->eccpos[0].
>>
>> We just need the position of the first ECC byte offset.
>> Is that the most optimal way to get it?
> 
> For the BCH case, it seems that ECC bytes always start at offset
> BADBLOCK_MARKER_LENGTH, so you can just pass
> mtd->writesize + BADBLOCK_MARKER_LENGTH.
> 
> Let me know if this works, and I'll squash those changes into the
> faulty commit (I know this implies a rebase + push -f, but IMO that's
> better than breaking bisectability).
> 
> 

So, the below patch works as well. Please feel free to fold it with your patch.

--
cheers,
-roger

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index e622a1b..eb85d6b 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -1547,8 +1547,8 @@ static int omap_read_page_bch(struct mtd_info *mtd, struct nand_chip *chip,
 	chip->read_buf(mtd, buf, mtd->writesize);
 
 	/* Read oob bytes */
-	chip->cmdfunc(mtd, NAND_CMD_RNDOUT, mtd->writesize, -1);
-	chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
+	chip->cmdfunc(mtd, NAND_CMD_RNDOUT, mtd->writesize + BADBLOCK_MARKER_LENGTH, -1);
+	chip->read_buf(mtd, chip->oob_poi, chip->ecc.total);
 
 	/* Calculate ecc bytes */
 	chip->ecc.calculate(mtd, buf, ecc_calc);