Wrong cache invalidation in cfi_cmdset0001.c (2.6.21 kernel)

Wrong cache invalidation in cfi_cmdset0001.c (2.6.21 kernel)

[Massimo CIRILLO]

We found an issue in cfi_cmdset0001.c file of 2.6.21 kernel.
It is related to cache region invalidation in the buffered
write procedure.

The original code performs cache invalidation from "adr" to "adr + len" in
do_write_buffer() while we modify region from "cmd_adr" to "len2" 
where len2 is equal to initial value of len.

The following is the patch to apply for 2.6.21 kernel.

--- a/drivers/mtd/chips/cfi_cmdset_0001.c	2007-11-26 18:06:37.000000000 +0100
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c	2007-11-26 18:06:44.000000000 +0100
@@ -1472,6 +1472,7 @@ static int __xipram do_write_buffer(stru
 	int ret, wbufsize, word_gap, words;
 	const struct kvec *vec;
 	unsigned long vec_seek;
+	int len2=len;
 
 	wbufsize = cfi_interleave(cfi) << cfi->cfiq->MaxBufWriteSize;
 	adr += chip->start;
@@ -1578,7 +1579,7 @@ static int __xipram do_write_buffer(stru
 	chip->state = FL_WRITING;
 
 	ret = INVAL_CACHE_AND_WAIT(map, chip, cmd_adr,
-				   adr, len,
+				   cmd_adr, len2,
 				   chip->buffer_write_time);
 	if (ret) {
 		map_write(map, CMD(0x70), cmd_adr);

In any case we believe this patch should be valid also for the 2.6.22 and 2.6.23 kernel.

Looking forward feedbacks.

Re: Wrong cache invalidation in cfi_cmdset0001.c (2.6.21 kernel)

[Jörn Engel]

On Mon, 26 November 2007 18:23:09 +0100, Massimo CIRILLO wrote:
> 
> We found an issue in cfi_cmdset0001.c file of 2.6.21 kernel.
> It is related to cache region invalidation in the buffered
> write procedure.
> 
> The original code performs cache invalidation from "adr" to "adr + len" in
> do_write_buffer() while we modify region from "cmd_adr" to "len2" 
> where len2 is equal to initial value of len.

Could use a better name, initial_len or something like that.  And David
would surely appreciate a Signed-off-by: line.  Otherwise appears to
make sense.

> The following is the patch to apply for 2.6.21 kernel.
> 
> --- a/drivers/mtd/chips/cfi_cmdset_0001.c	2007-11-26 18:06:37.000000000 +0100
> +++ b/drivers/mtd/chips/cfi_cmdset_0001.c	2007-11-26 18:06:44.000000000 +0100
> @@ -1472,6 +1472,7 @@ static int __xipram do_write_buffer(stru
>  	int ret, wbufsize, word_gap, words;
>  	const struct kvec *vec;
>  	unsigned long vec_seek;
> +	int len2=len;
>  
>  	wbufsize = cfi_interleave(cfi) << cfi->cfiq->MaxBufWriteSize;
>  	adr += chip->start;
> @@ -1578,7 +1579,7 @@ static int __xipram do_write_buffer(stru
>  	chip->state = FL_WRITING;
>  
>  	ret = INVAL_CACHE_AND_WAIT(map, chip, cmd_adr,
> -				   adr, len,
> +				   cmd_adr, len2,
>  				   chip->buffer_write_time);
>  	if (ret) {
>  		map_write(map, CMD(0x70), cmd_adr);
> 

Jörn

-- 
But this is not to say that the main benefit of Linux and other GPL
software is lower-cost. Control is the main benefit--cost is secondary.
-- Bruce Perens

Re: Wrong cache invalidation in cfi_cmdset0001.c (2.6.21 kernel)

[Jared Hulbert]

> > We found an issue in cfi_cmdset0001.c file of 2.6.21 kernel.
> > It is related to cache region invalidation in the buffered
> > write procedure.

What was the test setup that uncovered this?  I surprised this wasn't
found earlier.

> > The original code performs cache invalidation from "adr" to "adr + len" in
> > do_write_buffer() while we modify region from "cmd_adr" to "len2"
> > where len2 is equal to initial value of len.
>
> Could use a better name, initial_len or something like that.  And David
> would surely appreciate a Signed-off-by: line.  Otherwise appears to
> make sense.
>
> > The following is the patch to apply for 2.6.21 kernel.
> >
> > --- a/drivers/mtd/chips/cfi_cmdset_0001.c     2007-11-26 18:06:37.000000000 +0100
> > +++ b/drivers/mtd/chips/cfi_cmdset_0001.c     2007-11-26 18:06:44.000000000 +0100
> > @@ -1472,6 +1472,7 @@ static int __xipram do_write_buffer(stru
> >       int ret, wbufsize, word_gap, words;
> >       const struct kvec *vec;
> >       unsigned long vec_seek;
> > +     int len2=len;
> >
> >       wbufsize = cfi_interleave(cfi) << cfi->cfiq->MaxBufWriteSize;
> >       adr += chip->start;
> > @@ -1578,7 +1579,7 @@ static int __xipram do_write_buffer(stru
> >       chip->state = FL_WRITING;
> >
> >       ret = INVAL_CACHE_AND_WAIT(map, chip, cmd_adr,
> > -                                adr, len,
> > +                                cmd_adr, len2,
> >                                  chip->buffer_write_time);
> >       if (ret) {
> >               map_write(map, CMD(0x70), cmd_adr);
> >

If cmd_adr <= adr then shouldn't initial_len >=len?  Something like:

initial_len = (len + wbufsize) & ~(wbufsize-1);