missing flush_dcache_page call in 2.4 kernel

missing flush_dcache_page call in 2.4 kernel

[Atsushi Nemoto]

I noticed that reading from file with mmap sometimes return wrong data
on 2.4 kernel.

This is a test program to reproduce the problem.

#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>
#include <sys/mman.h>

int main(int argc, char **argv)
{
	int fd;
	struct stat st;
	volatile unsigned char *buf;
	unsigned char dat, dat2;

	fd = open(argv[1], O_RDONLY);
	fstat(fd, &st);
	buf = mmap(NULL, st.st_size, PROT_READ, MAP_SHARED, fd, 0);

	dat = *buf;
	cacheflush(0, 0, 0);	// flush cache all
	dat2 = *buf;

	printf("dat %x dat2 %x\n", dat, dat2);

	munmap(buf, st.st_size);
	close(fd);
	return 0;
}

'dat' and 'dat2' should be same value, of course.  But sometimes they
differ.

This problem often happens when I read a file in IDE disk (using PIO)
just after mounted.  I saw same problem on a mtd JFFS2 partition a
while ago.  I suppose it is not a filesystem/driver problem.

After calling cacheflush(), it returns correct data.  And I checked
the virtual/physical address return by the mmap and found they had
different 'color' when the problem happens.  So it seems to be a
virtual aliasing problem.

Documentation/cachetlb.txt says:

  void flush_dcache_page(struct page *page)

	Any time the kernel writes to a page cache page, _OR_
	the kernel is about to read from a page cache page and
	user space shared/writable mappings of this page potentially
	exist, this routine is called.

But flush_dcache_page() did not called between the mmap() call and the
cacheflush() call.

Tracing the code path on the page fault, I noticed filemap_nopage()
uses old flush_page_to_ram() interface.  I suppose flush_dcache_page()
should be called in same place.  Is this a correct fix?


--- linux-2.4.25/mm/filemap.c	Wed Feb 18 22:36:32 2004
+++ linux/mm/filemap.c	Thu Mar 25 21:19:29 2004
@@ -2111,6 +2111,7 @@
 	 * and possibly copy it over to another page..
 	 */
 	mark_page_accessed(page);
+	flush_dcache_page(page);
 	flush_page_to_ram(page);
 	return page;
 
---
Atsushi Nemoto

Re: missing flush_dcache_page call in 2.4 kernel

[Ralf Baechle]

On Thu, Mar 25, 2004 at 10:42:29PM +0900, Atsushi Nemoto wrote:

> I noticed that reading from file with mmap sometimes return wrong data
> on 2.4 kernel.
> 
> This is a test program to reproduce the problem.

This seems to be the same problem as reported by Peter Horton as while
ago; in his case that was with PIO IDE.

  Ralf

Re: missing flush_dcache_page call in 2.4 kernel

[Peter Horton]

Ralf Baechle wrote:

>On Thu, Mar 25, 2004 at 10:42:29PM +0900, Atsushi Nemoto wrote:
>
>  
>
>>I noticed that reading from file with mmap sometimes return wrong data
>>on 2.4 kernel.
>>
>>This is a test program to reproduce the problem.
>>    
>>
>
>This seems to be the same problem as reported by Peter Horton as while
>ago; in his case that was with PIO IDE.
>
>  
>
Looks like it.

The fix we're using on Cobalt's at the moment is below (required for 
2.4.x and 2.6.x).

Fixing it this way fixes the problem with both page cache pages and swap 
pages.

For more details see the threads "Kernel 2.4.23 on Cobalt Qube2 - area 
of problem" and "Instability / caching problems on Qube 2 - solved ?" 
from December last year.

P.

diff -urN linux.cvs/arch/mips/mm/c-r4k.c linux/arch/mips/mm/c-r4k.c
--- linux.cvs/arch/mips/mm/c-r4k.c	Mon Jan 12 18:19:51 2004
+++ linux/arch/mips/mm/c-r4k.c	Sun Feb  1 13:35:55 2004
@@ -400,8 +400,10 @@
 	 * If there's no context yet, or the page isn't executable, no icache
 	 * flush is needed.
 	 */
+#ifndef CONFIG_MIPS_COBALT
 	if (!(vma->vm_flags & VM_EXEC))
 		return;
+#endif
 
 	/*
 	 * Tricky ...  Because we don't know the virtual address we've got the
@@ -425,6 +427,11 @@
 		r4k_blast_dcache_page(addr);
 		ClearPageDcacheDirty(page);
 	}
+
+#ifdef CONFIG_MIPS_COBALT
+	if (!(vma->vm_flags & VM_EXEC))
+		return;
+#endif
 
 	/*
 	 * We're not sure of the virtual address(es) involved here, so

Re: missing flush_dcache_page call in 2.4 kernel

[Atsushi Nemoto]

>>>>> On Thu, 25 Mar 2004 14:50:09 +0000, Peter Horton <phorton@bitbox.co.uk> said:
>> This seems to be the same problem as reported by Peter Horton as
>> while ago; in his case that was with PIO IDE.
>> 
phorton> Looks like it.

phorton> The fix we're using on Cobalt's at the moment is below
phorton> (required for 2.4.x and 2.6.x).

phorton> Fixing it this way fixes the problem with both page cache
phorton> pages and swap pages.

phorton> For more details see the threads "Kernel 2.4.23 on Cobalt
phorton> Qube2 - area of problem" and "Instability / caching problems
phorton> on Qube 2 - solved ?"  from December last year.

Thanks, I agree (maybe I should read ML messages more carefully ...)

This patch fixes my problem also, thanks, but ... I do not think
r4k_flush_icache_page is a best place to fix since my test program is
not related I-cache at all.

I'm quite sure that it's a kernel bug and may cause problems if any
PIO block device (PIO ide, ide-cs, mtdblock, etc.) are used on CPUs
which have d-cache aliases (not only MIPS).  We need a correct fix ...

---
Atsushi Nemoto

Re: missing flush_dcache_page call in 2.4 kernel

[Peter Horton]

On Fri, Mar 26, 2004 at 12:22:58PM +0900, Atsushi Nemoto wrote:
>
> phorton> Looks like it.
> 
> phorton> The fix we're using on Cobalt's at the moment is below
> phorton> (required for 2.4.x and 2.6.x).
> 
> phorton> Fixing it this way fixes the problem with both page cache
> phorton> pages and swap pages.
> 
> phorton> For more details see the threads "Kernel 2.4.23 on Cobalt
> phorton> Qube2 - area of problem" and "Instability / caching problems
> phorton> on Qube 2 - solved ?"  from December last year.
> 
> Thanks, I agree (maybe I should read ML messages more carefully ...)
> 
> This patch fixes my problem also, thanks, but ... I do not think
> r4k_flush_icache_page is a best place to fix since my test program is
> not related I-cache at all.
> 

Agreed. The fix went here because this is arch specific code and it's
called in exactly the two places where there is a problem.

> I'm quite sure that it's a kernel bug and may cause problems if any
> PIO block device (PIO ide, ide-cs, mtdblock, etc.) are used on CPUs
> which have d-cache aliases (not only MIPS).  We need a correct fix ...
> 

True. A proper fix would flush the relevant page after PIO transfers
into the page cache / swap pages. Unfortunately this would require a
hook in the generic kernel.

P.

Re: missing flush_dcache_page call in 2.4 kernel

[Maciej W. Rozycki]

On Fri, 26 Mar 2004, Peter Horton wrote:

> > I'm quite sure that it's a kernel bug and may cause problems if any
> > PIO block device (PIO ide, ide-cs, mtdblock, etc.) are used on CPUs
> > which have d-cache aliases (not only MIPS).  We need a correct fix ...
> 
> True. A proper fix would flush the relevant page after PIO transfers
> into the page cache / swap pages. Unfortunately this would require a
> hook in the generic kernel.

 Why is it unfortunate?  A bug in generic code deserves a fix just like 
any other.

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +

Re: missing flush_dcache_page call in 2.4 kernel

[Atsushi Nemoto]

>>>>> On Fri, 26 Mar 2004 18:43:17 +0000, Peter Horton <pdh@colonel-panic.org> said:

>> I'm quite sure that it's a kernel bug and may cause problems if any
>> PIO block device (PIO ide, ide-cs, mtdblock, etc.) are used on CPUs
>> which have d-cache aliases (not only MIPS).  We need a correct fix
>> ...

pdh> True. A proper fix would flush the relevant page after PIO
pdh> transfers into the page cache / swap pages. Unfortunately this
pdh> would require a hook in the generic kernel.

I found somewhat long discussions in linux-kernel ML.

Subject: [patch] cache flush bug in mm/filemap.c (all kernels >= 2.5.30(at least))
http://www.ussg.iu.edu/hypermail/linux/kernel/0305.2/1205.html
http://www.ussg.iu.edu/hypermail/linux/kernel/0305.3/0151.html

Still I do not understand whole story on the thread, David S. Miller
said that architecture defined IDE insw/outsw macro should do the
flushing in this case, if I understand correctly.  Definitely sparc64
__ide_insw do it.  Hmm ...

---
Atsushi Nemoto

Re: missing flush_dcache_page call in 2.4 kernel

[Peter Horton]

On Sat, Mar 27, 2004 at 10:49:52PM +0900, Atsushi Nemoto wrote:
> 
> pdh> True. A proper fix would flush the relevant page after PIO
> pdh> transfers into the page cache / swap pages. Unfortunately this
> pdh> would require a hook in the generic kernel.
> 
> I found somewhat long discussions in linux-kernel ML.
> 
> Subject: [patch] cache flush bug in mm/filemap.c (all kernels >= 2.5.30(at least))
> http://www.ussg.iu.edu/hypermail/linux/kernel/0305.2/1205.html
> http://www.ussg.iu.edu/hypermail/linux/kernel/0305.3/0151.html
> 
> Still I do not understand whole story on the thread, David S. Miller
> said that architecture defined IDE insw/outsw macro should do the
> flushing in this case, if I understand correctly.  Definitely sparc64
> __ide_insw do it.  Hmm ...
> 

I've ditched the original Cobalt hack in c-r4k.c, and am using the patch
below instead. Seems to work okay ...

P.

--- linux.cvs/include/asm-mips/io.h	Tue Feb 25 22:03:12 2003
+++ linux.pdh/include/asm-mips/io.h	Sun Mar 28 13:53:54 2004
@@ -400,35 +400,35 @@
 	}
 }
 
-static inline void __insb(unsigned long port, void *addr, unsigned int count)
+static inline void __outsw(unsigned long port, void *addr, unsigned int count)
 {
 	while (count--) {
-		*(u8 *)addr = inb(port);
-		addr++;
+		outw(*(u16 *)addr, port);
+		addr += 2;
 	}
 }
 
-static inline void __outsw(unsigned long port, void *addr, unsigned int count)
+static inline void __outsl(unsigned long port, void *addr, unsigned int count)
 {
 	while (count--) {
-		outw(*(u16 *)addr, port);
-		addr += 2;
+		outl(*(u32 *)addr, port);
+		addr += 4;
 	}
 }
 
-static inline void __insw(unsigned long port, void *addr, unsigned int count)
+static inline void __insb(unsigned long port, void *addr, unsigned int count)
 {
 	while (count--) {
-		*(u16 *)addr = inw(port);
-		addr += 2;
+		*(u8 *)addr = inb(port);
+		addr++;
 	}
 }
 
-static inline void __outsl(unsigned long port, void *addr, unsigned int count)
+static inline void __insw(unsigned long port, void *addr, unsigned int count)
 {
 	while (count--) {
-		outl(*(u32 *)addr, port);
-		addr += 4;
+		*(u16 *)addr = inw(port);
+		addr += 2;
 	}
 }
 
@@ -440,12 +440,69 @@
 	}
 }
 
+/*
+ * String "in" operations which additionally write back & invalidate the
+ * data that's read into the D-cache to prevent unexpected aliases.
+ *
+ * We have no flush_data_cache_range(from, to) so we blast a whole page
+ * at a time.
+ */
+
+static inline void __insb_f(unsigned long port, void *addr, unsigned int count)
+{
+	u8 *ptr, *end;
+
+	ptr = addr;
+	end = ptr + count;
+
+	while (ptr < end)
+		*ptr++ = inb(port);
+
+	for (; addr < (void *) end; addr += PAGE_SIZE)
+		flush_data_cache_page((unsigned long) addr);
+}
+
+static inline void __insw_f(unsigned long port, void *addr, unsigned int count)
+{
+	u16 *ptr, *end;
+
+	ptr = addr;
+	end = ptr + count;
+
+	while (ptr < end)
+		*ptr++ = inw(port);
+
+	for (; addr < (void *) end; addr += PAGE_SIZE)
+		flush_data_cache_page((unsigned long) addr);
+}
+
+static inline void __insl_f(unsigned long port, void *addr, unsigned int count)
+{
+	u32 *ptr, *end;
+
+	ptr = addr;
+	end = ptr + count;
+
+	while (ptr < end)
+		*ptr++ = inl(port);
+
+	for (; addr < (void *) end; addr += PAGE_SIZE)
+		flush_data_cache_page((unsigned long) addr);
+}
+
 #define outsb(port, addr, count) __outsb(port, addr, count)
-#define insb(port, addr, count) __insb(port, addr, count)
 #define outsw(port, addr, count) __outsw(port, addr, count)
-#define insw(port, addr, count) __insw(port, addr, count)
 #define outsl(port, addr, count) __outsl(port, addr, count)
-#define insl(port, addr, count) __insl(port, addr, count)
+
+#ifdef CONFIG_MIPS_COBALT
+# define insb(port, addr, count) __insb_f(port, addr, count)
+# define insw(port, addr, count) __insw_f(port, addr, count)
+# define insl(port, addr, count) __insl_f(port, addr, count)
+#else
+# define insb(port, addr, count) __insb(port, addr, count)
+# define insw(port, addr, count) __insw(port, addr, count)
+# define insl(port, addr, count) __insl(port, addr, count)
+#endif
 
 /*
  * The caches on some architectures aren't dma-coherent and have need to

Re: missing flush_dcache_page call in 2.4 kernel

[Atsushi Nemoto]

>>>>> On Sun, 28 Mar 2004 14:04:00 +0100, Peter Horton <pdh@colonel-panic.org> said:
pdh> I've ditched the original Cobalt hack in c-r4k.c, and am using
pdh> the patch below instead. Seems to work okay ...

+	for (; addr < (void *) end; addr += PAGE_SIZE)
+		flush_data_cache_page((unsigned long) addr);

dma_cache_wback() will be more efficient ?

Also, I personally think replacing all insb/insw/insl is a bit
overkill.  I'd prefer redefine insb/insw/insl in asm-mips/ide.h, but
I'm not sure it is enough. (really all ins[bwl] should take care of
the cache inconsistency?)

---
Atsushi Nemoto

Re: missing flush_dcache_page call in 2.4 kernel

[Peter Horton]

Atsushi Nemoto wrote:

>>>>>>On Sun, 28 Mar 2004 14:04:00 +0100, Peter Horton <pdh@colonel-panic.org> said:
>>>>>>            
>>>>>>
>pdh> I've ditched the original Cobalt hack in c-r4k.c, and am using
>pdh> the patch below instead. Seems to work okay ...
>
>+	for (; addr < (void *) end; addr += PAGE_SIZE)
>+		flush_data_cache_page((unsigned long) addr);
>
>dma_cache_wback() will be more efficient ?
>  
>
Well technically it should be dma_cache_wback_inv(), though they equate 
to the same function currently.

It would be more efficient, but the dma_cache_*() functions are only 
available under CONFIG_DMA_NONCOHERENT, and our problem has nothing to 
do with DMA coherency at all.

All we really need to do is add a flush_dcache_range(from,to) function. 
I'm working on this at the moment.

>Also, I personally think replacing all insb/insw/insl is a bit
>overkill.  I'd prefer redefine insb/insw/insl in asm-mips/ide.h, but
>I'm not sure it is enough. (really all ins[bwl] should take care of
>the cache inconsistency?)
>
>  
>
There maybe other block drivers (SCSI?) that use insb/insw/insl that 
would also cause us grief, but we could provide both versions of the 
functions and select them as necessary.

P.