[PATCH] mtd: spinand: Handle the case where PROGRAM LOAD does not reset the cache

[PATCH] mtd: spinand: Handle the case where PROGRAM LOAD does not reset the cache

[Boris Brezillon]

Looks like PROGRAM LOAD (AKA write cache) does not necessarily reset
the cache content to 0xFF (depends on vendor implementation), so we
must fill the page cache entirely even if we only want to program the
data portion of the page, otherwise we might corrupt the BBM or user
data previously programmed in OOB area.

Fixes: 7529df465248 ("mtd: nand: Add core infrastructure to support SPI NANDs")
Reported-by: Stefan Roese <sr@denx.de>
Cc: <stable@vger.kernel.org>
Signed-off-by: Boris Brezillon <bbrezillon@kernel.org>
---
 drivers/mtd/nand/spi/core.c | 42 ++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 22 deletions(-)

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 479c2f2cf17f..8bf37da19663 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -304,24 +304,30 @@ static int spinand_write_to_cache_op(struct spinand_device *spinand,
 	struct nand_device *nand = spinand_to_nand(spinand);
 	struct mtd_info *mtd = nanddev_to_mtd(nand);
 	struct nand_page_io_req adjreq = *req;
-	unsigned int nbytes = 0;
-	void *buf = NULL;
+	void *buf = spinand->databuf;
+	unsigned int nbytes;
 	u16 column = 0;
 	int ret;
 
-	memset(spinand->databuf, 0xff,
-	       nanddev_page_size(nand) +
-	       nanddev_per_page_oobsize(nand));
+	/*
+	 * Looks like PROGRAM LOAD (AKA write cache) does not necessarily reset
+	 * the cache content to 0xFF (depends on vendor implementation), so we
+	 * must fill the page cache entirely even if we only want to program
+	 * the data portion of the page, otherwise we might corrupt the BBM or
+	 * user data previously programmed in OOB area.
+	 */
+	nbytes = nanddev_page_size(nand) + nanddev_per_page_oobsize(nand);
+	memset(spinand->databuf, 0xff, nbytes);
+	adjreq.dataoffs = 0;
+	adjreq.datalen = nanddev_page_size(nand);
+	adjreq.databuf.out = spinand->databuf;
+	adjreq.ooblen = nanddev_per_page_oobsize(nand);
+	adjreq.ooboffs = 0;
+	adjreq.oobbuf.out = spinand->oobbuf;
 
-	if (req->datalen) {
+	if (req->datalen)
 		memcpy(spinand->databuf + req->dataoffs, req->databuf.out,
 		       req->datalen);
-		adjreq.dataoffs = 0;
-		adjreq.datalen = nanddev_page_size(nand);
-		adjreq.databuf.out = spinand->databuf;
-		nbytes = adjreq.datalen;
-		buf = spinand->databuf;
-	}
 
 	if (req->ooblen) {
 		if (req->mode == MTD_OPS_AUTO_OOB)
@@ -332,14 +338,6 @@ static int spinand_write_to_cache_op(struct spinand_device *spinand,
 		else
 			memcpy(spinand->oobbuf + req->ooboffs, req->oobbuf.out,
 			       req->ooblen);
-
-		adjreq.ooblen = nanddev_per_page_oobsize(nand);
-		adjreq.ooboffs = 0;
-		nbytes += nanddev_per_page_oobsize(nand);
-		if (!buf) {
-			buf = spinand->oobbuf;
-			column = nanddev_page_size(nand);
-		}
 	}
 
 	spinand_cache_op_adjust_colum(spinand, &adjreq, &column);
@@ -370,8 +368,8 @@ static int spinand_write_to_cache_op(struct spinand_device *spinand,
 
 		/*
 		 * We need to use the RANDOM LOAD CACHE operation if there's
-		 * more than one iteration, because the LOAD operation resets
-		 * the cache to 0xff.
+		 * more than one iteration, because the LOAD operation might
+		 * reset the cache to 0xff.
 		 */
 		if (nbytes) {
 			column = op.addr.val;
-- 
2.17.1

Re: [PATCH] mtd: spinand: Handle the case where PROGRAM LOAD does not reset the cache

[Stefan Roese]

On 24.01.19 15:20, Boris Brezillon wrote:
> Looks like PROGRAM LOAD (AKA write cache) does not necessarily reset
> the cache content to 0xFF (depends on vendor implementation), so we
> must fill the page cache entirely even if we only want to program the
> data portion of the page, otherwise we might corrupt the BBM or user
> data previously programmed in OOB area.
> 
> Fixes: 7529df465248 ("mtd: nand: Add core infrastructure to support SPI NANDs")
> Reported-by: Stefan Roese <sr@denx.de>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Boris Brezillon <bbrezillon@kernel.org>

Works fine (limited testing only yet), so:

Tested-by: Stefan Roese <sr@denx.de>
Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

Re: [PATCH] mtd: spinand: Handle the case where PROGRAM LOAD does not reset the cache

[Emil Lenngren]

Hi,

Den tors 24 jan. 2019 kl 16:28 skrev Stefan Roese <sr@denx.de>:
>
> On 24.01.19 15:20, Boris Brezillon wrote:
> > Looks like PROGRAM LOAD (AKA write cache) does not necessarily reset
> > the cache content to 0xFF (depends on vendor implementation), so we
> > must fill the page cache entirely even if we only want to program the
> > data portion of the page, otherwise we might corrupt the BBM or user
> > data previously programmed in OOB area.
> >
> > Fixes: 7529df465248 ("mtd: nand: Add core infrastructure to support SPI NANDs")
> > Reported-by: Stefan Roese <sr@denx.de>
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Boris Brezillon <bbrezillon@kernel.org>
>
> Works fine (limited testing only yet), so:
>
> Tested-by: Stefan Roese <sr@denx.de>
> Reviewed-by: Stefan Roese <sr@denx.de>
>
> Thanks,
> Stefan
>

Can this quirk be made vendor specific? It seems a waste of SPI
transfer cycles to write 0xff to the whole OOB area when we only want
to program the data area if the chip doesn't need this quirk.
For which logic is this needed anyway? According to the GigaDevice
datasheet, if a Program Load is followed by Program Execute,
"uninitialized" bytes will be set to 0xff, which is the flow that is
used by spinand core.

/Emil

Re: [PATCH] mtd: spinand: Handle the case where PROGRAM LOAD does not reset the cache

[Boris Brezillon]

On Thu, 24 Jan 2019 17:16:37 +0100
Emil Lenngren <emil.lenngren@gmail.com> wrote:

> Hi,
> 
> Den tors 24 jan. 2019 kl 16:28 skrev Stefan Roese <sr@denx.de>:
> >
> > On 24.01.19 15:20, Boris Brezillon wrote:  
> > > Looks like PROGRAM LOAD (AKA write cache) does not necessarily reset
> > > the cache content to 0xFF (depends on vendor implementation), so we
> > > must fill the page cache entirely even if we only want to program the
> > > data portion of the page, otherwise we might corrupt the BBM or user
> > > data previously programmed in OOB area.
> > >
> > > Fixes: 7529df465248 ("mtd: nand: Add core infrastructure to support SPI NANDs")
> > > Reported-by: Stefan Roese <sr@denx.de>
> > > Cc: <stable@vger.kernel.org>
> > > Signed-off-by: Boris Brezillon <bbrezillon@kernel.org>  
> >
> > Works fine (limited testing only yet), so:
> >
> > Tested-by: Stefan Roese <sr@denx.de>
> > Reviewed-by: Stefan Roese <sr@denx.de>
> >
> > Thanks,
> > Stefan
> >  
> 
> Can this quirk be made vendor specific?

We can make it vendor specific, as long as it's an opt-in thing. This
way, the default behavior is the safest one, and only when we know a
chip does reset the cache content on a PROGRAM LOAD time can we add this
flag.

> It seems a waste of SPI
> transfer cycles to write 0xff to the whole OOB area when we only want
> to program the data area if the chip doesn't need this quirk.
> For which logic is this needed anyway? According to the GigaDevice
> datasheet, if a Program Load is followed by Program Execute,
> "uninitialized" bytes will be set to 0xff, which is the flow that is
> used by spinand core.

Except you're not guaranteed that the controller can fill the page in a
single spi_mem_exec_op() call (because of FIFO size limitations), so
you might have one PROGRAM LOAD followed by several PROGRAM RANDOM LOAD.
I guess most manufacturers reset the cache content at PROGRAM LOAD
time, but GigaDevice does not.

> 
> /Emil

Re: [PATCH] mtd: spinand: Handle the case where PROGRAM LOAD does not reset the cache

[Miquel Raynal]

Hello,

Boris Brezillon <bbrezillon@kernel.org> wrote on Thu, 24 Jan 2019
17:33:57 +0100:

> On Thu, 24 Jan 2019 17:16:37 +0100
> Emil Lenngren <emil.lenngren@gmail.com> wrote:
> 
> > Hi,
> > 
> > Den tors 24 jan. 2019 kl 16:28 skrev Stefan Roese <sr@denx.de>:  
> > >
> > > On 24.01.19 15:20, Boris Brezillon wrote:    
> > > > Looks like PROGRAM LOAD (AKA write cache) does not necessarily reset
> > > > the cache content to 0xFF (depends on vendor implementation), so we
> > > > must fill the page cache entirely even if we only want to program the
> > > > data portion of the page, otherwise we might corrupt the BBM or user
> > > > data previously programmed in OOB area.
> > > >
> > > > Fixes: 7529df465248 ("mtd: nand: Add core infrastructure to support SPI NANDs")
> > > > Reported-by: Stefan Roese <sr@denx.de>
> > > > Cc: <stable@vger.kernel.org>
> > > > Signed-off-by: Boris Brezillon <bbrezillon@kernel.org>    
> > >
> > > Works fine (limited testing only yet), so:
> > >
> > > Tested-by: Stefan Roese <sr@denx.de>
> > > Reviewed-by: Stefan Roese <sr@denx.de>
> > >
> > > Thanks,
> > > Stefan
> > >    
> > 
> > Can this quirk be made vendor specific?  
> 
> We can make it vendor specific, as long as it's an opt-in thing. This
> way, the default behavior is the safest one, and only when we know a
> chip does reset the cache content on a PROGRAM LOAD time can we add this
> flag.

I am fine with this approach.


Thanks,
Miquèl

Re: [PATCH] mtd: spinand: Handle the case where PROGRAM LOAD does not reset the cache

[Boris Brezillon]

On Fri, 25 Jan 2019 12:09:10 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hello,
> 
> Boris Brezillon <bbrezillon@kernel.org> wrote on Thu, 24 Jan 2019
> 17:33:57 +0100:
> 
> > On Thu, 24 Jan 2019 17:16:37 +0100
> > Emil Lenngren <emil.lenngren@gmail.com> wrote:
> >   
> > > Hi,
> > > 
> > > Den tors 24 jan. 2019 kl 16:28 skrev Stefan Roese <sr@denx.de>:    
> > > >
> > > > On 24.01.19 15:20, Boris Brezillon wrote:      
> > > > > Looks like PROGRAM LOAD (AKA write cache) does not necessarily reset
> > > > > the cache content to 0xFF (depends on vendor implementation), so we
> > > > > must fill the page cache entirely even if we only want to program the
> > > > > data portion of the page, otherwise we might corrupt the BBM or user
> > > > > data previously programmed in OOB area.
> > > > >
> > > > > Fixes: 7529df465248 ("mtd: nand: Add core infrastructure to support SPI NANDs")
> > > > > Reported-by: Stefan Roese <sr@denx.de>
> > > > > Cc: <stable@vger.kernel.org>
> > > > > Signed-off-by: Boris Brezillon <bbrezillon@kernel.org>      
> > > >
> > > > Works fine (limited testing only yet), so:
> > > >
> > > > Tested-by: Stefan Roese <sr@denx.de>
> > > > Reviewed-by: Stefan Roese <sr@denx.de>
> > > >
> > > > Thanks,
> > > > Stefan
> > > >      
> > > 
> > > Can this quirk be made vendor specific?    
> > 
> > We can make it vendor specific, as long as it's an opt-in thing. This
> > way, the default behavior is the safest one, and only when we know a
> > chip does reset the cache content on a PROGRAM LOAD time can we add this
> > flag.  
> 
> I am fine with this approach.

Does that stand for a Reviewed-by/Acked-by? To make it clear, I was
saying that we should fix things first (with this fix) and only then
optimize things for chips that actually reset the cache when PROGRAM
LOAD is executed. I was not planning on sending a new version of this
patch, unless you see good reasons to do so.

Re: [PATCH] mtd: spinand: Handle the case where PROGRAM LOAD does not reset the cache

[Miquel Raynal]

Hi Boris,

Boris Brezillon <bbrezillon@kernel.org> wrote on Fri, 25 Jan 2019
17:08:51 +0100:

> On Fri, 25 Jan 2019 12:09:10 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Hello,
> > 
> > Boris Brezillon <bbrezillon@kernel.org> wrote on Thu, 24 Jan 2019
> > 17:33:57 +0100:
> >   
> > > On Thu, 24 Jan 2019 17:16:37 +0100
> > > Emil Lenngren <emil.lenngren@gmail.com> wrote:
> > >     
> > > > Hi,
> > > > 
> > > > Den tors 24 jan. 2019 kl 16:28 skrev Stefan Roese <sr@denx.de>:      
> > > > >
> > > > > On 24.01.19 15:20, Boris Brezillon wrote:        
> > > > > > Looks like PROGRAM LOAD (AKA write cache) does not necessarily reset
> > > > > > the cache content to 0xFF (depends on vendor implementation), so we
> > > > > > must fill the page cache entirely even if we only want to program the
> > > > > > data portion of the page, otherwise we might corrupt the BBM or user
> > > > > > data previously programmed in OOB area.
> > > > > >
> > > > > > Fixes: 7529df465248 ("mtd: nand: Add core infrastructure to support SPI NANDs")
> > > > > > Reported-by: Stefan Roese <sr@denx.de>
> > > > > > Cc: <stable@vger.kernel.org>
> > > > > > Signed-off-by: Boris Brezillon <bbrezillon@kernel.org>        
> > > > >
> > > > > Works fine (limited testing only yet), so:
> > > > >
> > > > > Tested-by: Stefan Roese <sr@denx.de>
> > > > > Reviewed-by: Stefan Roese <sr@denx.de>
> > > > >
> > > > > Thanks,
> > > > > Stefan
> > > > >        
> > > > 
> > > > Can this quirk be made vendor specific?      
> > > 
> > > We can make it vendor specific, as long as it's an opt-in thing. This
> > > way, the default behavior is the safest one, and only when we know a
> > > chip does reset the cache content on a PROGRAM LOAD time can we add this
> > > flag.    
> > 
> > I am fine with this approach.  
> 
> Does that stand for a Reviewed-by/Acked-by? To make it clear, I was
> saying that we should fix things first (with this fix) and only then
> optimize things for chips that actually reset the cache when PROGRAM
> LOAD is executed. I was not planning on sending a new version of this
> patch, unless you see good reasons to do so.

I thought you would send a v2 but I am fine with this approach too.

Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>


Thanks,
Miquèl

Re: mtd: spinand: Handle the case where PROGRAM LOAD does not reset the cache

[Boris Brezillon]

On Thu, 2019-01-24 at 14:20:07 UTC, Boris Brezillon wrote:
> Looks like PROGRAM LOAD (AKA write cache) does not necessarily reset
> the cache content to 0xFF (depends on vendor implementation), so we
> must fill the page cache entirely even if we only want to program the
> data portion of the page, otherwise we might corrupt the BBM or user
> data previously programmed in OOB area.
> 
> Fixes: 7529df465248 ("mtd: nand: Add core infrastructure to support SPI NANDs")
> Reported-by: Stefan Roese <sr@denx.de>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Boris Brezillon <bbrezillon@kernel.org>
> Tested-by: Stefan Roese <sr@denx.de>
> Reviewed-by: Stefan Roese <sr@denx.de>

Applied to http://git.infradead.org/linux-mtd.git mtd/fixes.

Boris