[PATCH][CFI] Wrong cache invalidation bug fix

[PATCH][CFI] Wrong cache invalidation bug fix

[massimo cirillo]

Hi,

We encountered the problem with cache invalidation in CFI driver. It
causes corruptions of data read from flash.

The problem is investigated. The original code performs cache
invalidation from "adr" to "adr + len" in do_write_buffer(). Since len
and adr could be updated in the code before invalidation - it causes
improper setting of cache invalidation regions.
The following patch fixes this problem. This patch has been verified
on our OMAP based platform.
Could you please acknowledge and commit this patch?

Thanks a lot.

Signed-off-by Massimo Cirillo <maxcir@gmail.com> and Giuseppe D'Eliseo
<giuseppedeliseo@gmail.com>

------

diff -aur a/drivers/mtd/chips/cfi_cmdset_0001.c
b/drivers/mtd/chips/cfi_cmdset_0001.c

--- a/drivers/mtd/chips/cfi_cmdset_0001.c	2007-11-20 20:15:02.000000000 +0300
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c	2007-11-29 19:42:08.000000000 +0300
@@ -1530,9 +1530,12 @@
 	int ret, wbufsize, word_gap, words;
 	const struct kvec *vec;
 	unsigned long vec_seek;
+	unsigned long initial_adr;
+	int initial_len=len;

 	wbufsize = cfi_interleave(cfi) << cfi->cfiq->MaxBufWriteSize;
 	adr += chip->start;
+	initial_adr = adr;
 	cmd_adr = adr & ~(wbufsize-1);

 	/* Let's determine this according to the interleave only once */
@@ -1636,7 +1639,7 @@
 	chip->state = FL_WRITING;

 	ret = INVAL_CACHE_AND_WAIT(map, chip, cmd_adr,
-				   adr, len,
+				   initial_adr, initial_len,
 				   chip->buffer_write_time);
 	if (ret) {
 		map_write(map, CMD(0x70), cmd_adr);

------

Re: [PATCH][CFI] Wrong cache invalidation bug fix

[Nicolas Pitre]

On Mon, 3 Dec 2007, massimo cirillo wrote:

> Hi,
> 
> We encountered the problem with cache invalidation in CFI driver. It
> causes corruptions of data read from flash.
> 
> The problem is investigated. The original code performs cache
> invalidation from "adr" to "adr + len" in do_write_buffer(). Since len
> and adr could be updated in the code before invalidation - it causes
> improper setting of cache invalidation regions.
> The following patch fixes this problem. This patch has been verified
> on our OMAP based platform.
> Could you please acknowledge and commit this patch?
> 
> Thanks a lot.
> 
> Signed-off-by Massimo Cirillo <maxcir@gmail.com> and Giuseppe D'Eliseo
> <giuseppedeliseo@gmail.com>

This patch is correct.  However, to make it more obvious, please change 
the used variable in the XIP_INVAL_CACHED_RANGE() call used in the same 
function as well.

With that change you can add "Acked-by: Nicolas Pitre <nico@cam.org>".


Nicolas

Re: [PATCH][CFI] Wrong cache invalidation bug fix

[Jörn Engel]

On Mon, 3 December 2007 12:12:37 -0500, Nicolas Pitre wrote:
> 
> With that change you can add "Acked-by: Nicolas Pitre <nico@cam.org>".

Mine as well, if you feel so inclined.

Jörn

-- 
Beware of bugs in the above code; I have only proved it correct, but
not tried it.
-- Donald Knuth

Re: [PATCH][CFI] Wrong cache invalidation bug fix

[massimo cirillo]

OK, I've verified your suggestion and the tests are ok.
Following the overall patch for the cfi_cmdset_0001.c

Signed-off-by Massimo Cirillo <maxcir@gmail.com> and Giuseppe D'Eliseo
<giuseppedeliseo@gmail.com>
Acked-by: Nicolas Pitre <nico@cam.org> and Jörn Engel <joern@logfs.org>

------
diff -aur a/drivers/mtd/chips/cfi_cmdset_0001.c
b/drivers/mtd/chips/cfi_cmdset_0001.c

--- a/drivers/mtd/chips/cfi_cmdset_0001.c       2007-12-05
10:45:20.000000000 +0100
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c       2007-11-16
19:14:27.000000000 +0100
@@ -1486,9 +1486,12 @@
       int ret, wbufsize, word_gap, words;
       const struct kvec *vec;
       unsigned long vec_seek;
+       unsigned long initial_adr;
+       int initial_len=len;

       wbufsize = cfi_interleave(cfi) << cfi->cfiq->MaxBufWriteSize;
       adr += chip->start;
+       initial_adr = adr;
       cmd_adr = adr & ~(wbufsize-1);

       /* Let's determine this according to the interleave only once */
@@ -1501,7 +1504,7 @@
               return ret;
       }

-       XIP_INVAL_CACHED_RANGE(map, adr, len);
+       XIP_INVAL_CACHED_RANGE(map, initial_adr, initial_len);
       ENABLE_VPP(map);
       xip_disable(map, chip, cmd_adr);

@@ -1592,7 +1595,7 @@
       chip->state = FL_WRITING;

       ret = INVAL_CACHE_AND_WAIT(map, chip, cmd_adr,
-                                  adr, len,
+                                  initial_adr, initial_len,
                                  chip->buffer_write_time);
       if (ret) {
               map_write(map, CMD(0x70), cmd_adr);
------

2007/12/3, Nicolas Pitre <nico@cam.org>:
> On Mon, 3 Dec 2007, massimo cirillo wrote:
>
> > Hi,
> >
> > We encountered the problem with cache invalidation in CFI driver. It
> > causes corruptions of data read from flash.
> >
> > The problem is investigated. The original code performs cache
> > invalidation from "adr" to "adr + len" in do_write_buffer(). Since len
> > and adr could be updated in the code before invalidation - it causes
> > improper setting of cache invalidation regions.
> > The following patch fixes this problem. This patch has been verified
> > on our OMAP based platform.
> > Could you please acknowledge and commit this patch?
> >
> > Thanks a lot.
> >
> > Signed-off-by Massimo Cirillo <maxcir@gmail.com> and Giuseppe D'Eliseo
> > <giuseppedeliseo@gmail.com>
>
> This patch is correct.  However, to make it more obvious, please change
> the used variable in the XIP_INVAL_CACHED_RANGE() call used in the same
> function as well.
>
> With that change you can add "Acked-by: Nicolas Pitre <nico@cam.org>".
>
>
> Nicolas
>

Re: [PATCH][CFI] Wrong cache invalidation bug fix

[massimo cirillo]

Hi David,

Could you please apply the patch? It has been tested and acknowledged
by Nicolas and Jörn.

Description
This patch fixes improper setting of cache invalidation regions and
solves the issue of data corrupting.

Signed-off-by: Massimo Cirillo <maxcir@gmail.com>
Signed-off-by: Giuseppe D'Eliseo <giuseppedeliseo@gmail.com>
Acked-by: Nicolas Pitre <nico@cam.org>
Acked-by: Jörn Engel <joern@logfs.org>

------
diff -aur a/drivers/mtd/chips/cfi_cmdset_0001.c
b/drivers/mtd/chips/cfi_cmdset_0001.c

--- a/drivers/mtd/chips/cfi_cmdset_0001.c       2007-11-16
19:14:27.000000000 +0100
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c       2007-12-05
10:45:20.000000000 +0100
@@ -1486,9 +1486,12 @@
        int ret, wbufsize, word_gap, words;
        const struct kvec *vec;
        unsigned long vec_seek;
+       unsigned long initial_adr;
+       int initial_len=len;

        wbufsize = cfi_interleave(cfi) << cfi->cfiq->MaxBufWriteSize;
        adr += chip->start;
+       initial_adr = adr;
        cmd_adr = adr & ~(wbufsize-1);

        /* Let's determine this according to the interleave only once */
@@ -1501,7 +1504,7 @@
                return ret;
        }

-       XIP_INVAL_CACHED_RANGE(map, adr, len);
+       XIP_INVAL_CACHED_RANGE(map, initial_adr, initial_len);
        ENABLE_VPP(map);
        xip_disable(map, chip, cmd_adr);

@@ -1592,7 +1595,7 @@
        chip->state = FL_WRITING;

        ret = INVAL_CACHE_AND_WAIT(map, chip, cmd_adr,
-                                  adr, len,
+                                  initial_adr, initial_len,
                                   chip->buffer_write_time);
        if (ret) {
                map_write(map, CMD(0x70), cmd_adr);
------

2007/12/4, Jörn Engel <joern@logfs.org>:
> On Mon, 3 December 2007 12:12:37 -0500, Nicolas Pitre wrote:
> >
> > With that change you can add "Acked-by: Nicolas Pitre <nico@cam.org>".
>
> Mine as well, if you feel so inclined.
>
> Jörn
>
> --
> Beware of bugs in the above code; I have only proved it correct, but
> not tried it.
> -- Donald Knuth
>

Re: [PATCH][CFI] Wrong cache invalidation bug fix

[Uli Luckas]

On Thursday, 13. December 2007, massimo cirillo wrote:
> Hi David,
>
> Could you please apply the patch? It has been tested and acknowledged
> by Nicolas and Jörn.
>
> Description
> This patch fixes improper setting of cache invalidation regions and
> solves the issue of data corrupting.
>
What happend to this patch? As it fixes a grave bug, shouldn't it be pushed 
out for 2.6.24?

regards,
Uli

-- 

------- ROAD ...the handyPC Company - - -  ) ) )

Uli Luckas
Software Development

ROAD GmbH
Bennigsenstr. 14 | 12159 Berlin | Germany
fon: +49 (30) 230069 - 64 | fax: +49 (30) 230069 - 69
url: www.road.de

Amtsgericht Charlottenburg: HRB 96688 B
Managing directors: Hans-Peter Constien, Hubertus von Streit

Re: [PATCH][CFI] Wrong cache invalidation bug fix

[Alexey Korolev]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1555 bytes --]

Hi David,

Please include patch from Massimo? It fixes bad dug in CFI driver.

Thanks,
Alexey







Signed-off-by Massimo Cirillo <maxcir@gmail.com> and Giuseppe D'Eliseo
<giuseppedeliseo@gmail.com>
Acked-by: Nicolas Pitre <nico@cam.org> and Jörn Engel <joern@logfs.org>

------
diff -aur a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
--- a/drivers/mtd/chips/cfi_cmdset_0001.c       2007-12-05 10:45:20.000000000 +0100
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c       2007-11-16 19:14:27.000000000 +0100
@@ -1486,9 +1486,12 @@
       int ret, wbufsize, word_gap, words;
       const struct kvec *vec;
       unsigned long vec_seek;
+       unsigned long initial_adr;
+       int initial_len=len;

       wbufsize = cfi_interleave(cfi) << cfi->cfiq->MaxBufWriteSize;
       adr += chip->start;
+       initial_adr = adr;
       cmd_adr = adr & ~(wbufsize-1);

       /* Let's determine this according to the interleave only once */
@@ -1501,7 +1504,7 @@
               return ret;
       }

-       XIP_INVAL_CACHED_RANGE(map, adr, len);
+       XIP_INVAL_CACHED_RANGE(map, initial_adr, initial_len);
       ENABLE_VPP(map);
       xip_disable(map, chip, cmd_adr);

@@ -1592,7 +1595,7 @@
       chip->state = FL_WRITING;

       ret = INVAL_CACHE_AND_WAIT(map, chip, cmd_adr,
-                                  adr, len,
+                                  initial_adr, initial_len,
                                  chip->buffer_write_time);
       if (ret) {
               map_write(map, CMD(0x70), cmd_adr);
---------------

Re: [PATCH][CFI] Wrong cache invalidation bug fix

[David Woodhouse]

On Fri, 2008-01-11 at 10:50 +0000, Alexey Korolev wrote:
> Please include patch from Massimo? It fixes bad dug in CFI driver.

I don't think I've ever received a version of the patch in email which
actually applies. Please remember to save the mail you get back from the
mailing list and try to apply it for yourself to a clean tree. Or better
still, send it to yourself first, _before_ sending it out.

That isn't an excuse for ignoring it for a month -- on that I plead
incompetence.

I've taken the version from Jared's git tree and sent it to Linus.

-- 
dwmw2

Re: [PATCH][CFI] Wrong cache invalidation bug fix

[Jared Hulbert]

> That isn't an excuse for ignoring it for a month -- on that I plead
> incompetence.
>
> I've taken the version from Jared's git tree and sent it to Linus.

I don't see it anywhere in Linus' or your tree.  Can we make sure it gets in?