linux-ide.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Fix ide on Swarm and improve it a little
@ 2010-02-28 15:35 Sebastian Andrzej Siewior
  2010-02-28 15:35 ` [PATCH 1/3] mips/ide: flush dcache also if icache does not snoop dcache Sebastian Andrzej Siewior
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Sebastian Andrzej Siewior @ 2010-02-28 15:35 UTC (permalink / raw)
  To: Ralf Baechle; +Cc: linux-mips, David S. Miller, linux-ide

Patch #1 fixes the ide trouble on Swarm. We don't have dcache alliasing
         there but icache does not snoop dcache so we can't execute / boot
         from ide devices unless dcache is flushed.
Patch #2 is ths' old patch rebased with a few optimizations.
Patch #3 moves the dcache flush from arch code into ide subsystem. David,
         I hope I don't break any of your sparcs. The dcache flush in
         write path looks like a nop.

The whole series was tested on MIPS BCM1250 B2 aka Swarm with its default
config and CONFIG_BLK_DEV_PLATFORM=y instead PATA. PATA seems not to work
(yet).

Sebastian

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/3] mips/ide: flush dcache also if icache does not snoop dcache
  2010-02-28 15:35 Fix ide on Swarm and improve it a little Sebastian Andrzej Siewior
@ 2010-02-28 15:35 ` Sebastian Andrzej Siewior
  2010-02-28 15:35 ` [PATCH 2/3] mips/ide: clean up / minimize dcache flushes Sebastian Andrzej Siewior
  2010-02-28 15:35 ` [PATCH 3/3] ide: move dcache flushing to generic ide code Sebastian Andrzej Siewior
  2 siblings, 0 replies; 9+ messages in thread
From: Sebastian Andrzej Siewior @ 2010-02-28 15:35 UTC (permalink / raw)
  To: Ralf Baechle
  Cc: linux-mips, David S. Miller, linux-ide, Sebastian Andrzej Siewior

If this is not done then the new just read data which remains in dcache
will not make it into icache on time. Thus the CPU loads invalid data
and executes crap. The result is that the user is not able to execute
anything from its IDE based media while reading plain data is still
working well.
This problem has been reported as Debian #404951.

Cc: stable@kernel.org
Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---
 arch/mips/include/asm/mach-generic/ide.h |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/mips/include/asm/mach-generic/ide.h b/arch/mips/include/asm/mach-generic/ide.h
index 9c93a5b..e80e47f 100644
--- a/arch/mips/include/asm/mach-generic/ide.h
+++ b/arch/mips/include/asm/mach-generic/ide.h
@@ -23,7 +23,7 @@
 static inline void __ide_flush_prologue(void)
 {
 #ifdef CONFIG_SMP
-	if (cpu_has_dc_aliases)
+	if (cpu_has_dc_aliases || !cpu_has_ic_fills_f_dc)
 		preempt_disable();
 #endif
 }
@@ -31,14 +31,14 @@ static inline void __ide_flush_prologue(void)
 static inline void __ide_flush_epilogue(void)
 {
 #ifdef CONFIG_SMP
-	if (cpu_has_dc_aliases)
+	if (cpu_has_dc_aliases || !cpu_has_ic_fills_f_dc)
 		preempt_enable();
 #endif
 }
 
 static inline void __ide_flush_dcache_range(unsigned long addr, unsigned long size)
 {
-	if (cpu_has_dc_aliases) {
+	if (cpu_has_dc_aliases || !cpu_has_ic_fills_f_dc) {
 		unsigned long end = addr + size;
 
 		while (addr < end) {
-- 
1.6.6


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/3] mips/ide: clean up / minimize dcache flushes
  2010-02-28 15:35 Fix ide on Swarm and improve it a little Sebastian Andrzej Siewior
  2010-02-28 15:35 ` [PATCH 1/3] mips/ide: flush dcache also if icache does not snoop dcache Sebastian Andrzej Siewior
@ 2010-02-28 15:35 ` Sebastian Andrzej Siewior
  2010-02-28 15:35 ` [PATCH 3/3] ide: move dcache flushing to generic ide code Sebastian Andrzej Siewior
  2 siblings, 0 replies; 9+ messages in thread
From: Sebastian Andrzej Siewior @ 2010-02-28 15:35 UTC (permalink / raw)
  To: Ralf Baechle
  Cc: linux-mips, David S. Miller, linux-ide, Sebastian Andrzej Siewior

This patch is based on Thiemo's work where he tried to minimize the
dcache flushes and clean up the API a bit. The patch was posted in
Debian #466977.
The patch in detail:
- instead of an active flush the relevant pages just get a dirty dcache
  bit set
- the dirty bit is only set if we have userspace mapping. So we don't
  set it for kernel internal requests which never see userland.
- since writes to the device are "read only" there is no need set the
  dirty bit.

Signed-off-by: Thiemo Seufer <ths@networkno.de>
Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---
 arch/mips/include/asm/cacheflush.h       |    1 -
 arch/mips/include/asm/mach-generic/ide.h |   70 +++++++-----------------------
 arch/mips/mm/c-r3k.c                     |    5 --
 arch/mips/mm/c-r4k.c                     |    1 -
 arch/mips/mm/c-tx39.c                    |    7 ---
 arch/mips/mm/cache.c                     |    2 -
 6 files changed, 16 insertions(+), 70 deletions(-)

diff --git a/arch/mips/include/asm/cacheflush.h b/arch/mips/include/asm/cacheflush.h
index 40bb9fd..d95a79c 100644
--- a/arch/mips/include/asm/cacheflush.h
+++ b/arch/mips/include/asm/cacheflush.h
@@ -92,7 +92,6 @@ extern void copy_from_user_page(struct vm_area_struct *vma,
 
 extern void (*flush_cache_sigtramp)(unsigned long addr);
 extern void (*flush_icache_all)(void);
-extern void (*local_flush_data_cache_page)(void * addr);
 extern void (*flush_data_cache_page)(unsigned long addr);
 
 /*
diff --git a/arch/mips/include/asm/mach-generic/ide.h b/arch/mips/include/asm/mach-generic/ide.h
index e80e47f..9360586 100644
--- a/arch/mips/include/asm/mach-generic/ide.h
+++ b/arch/mips/include/asm/mach-generic/ide.h
@@ -20,107 +20,69 @@
 #include <asm/processor.h>
 
 /* MIPS port and memory-mapped I/O string operations.  */
-static inline void __ide_flush_prologue(void)
+static inline void __ide_set_pages_dirty(const void *addr, unsigned long size)
 {
-#ifdef CONFIG_SMP
-	if (cpu_has_dc_aliases || !cpu_has_ic_fills_f_dc)
-		preempt_disable();
-#endif
-}
+	unsigned long end = (unsigned long)addr + size;
 
-static inline void __ide_flush_epilogue(void)
-{
-#ifdef CONFIG_SMP
-	if (cpu_has_dc_aliases || !cpu_has_ic_fills_f_dc)
-		preempt_enable();
-#endif
-}
+	if (!(cpu_has_dc_aliases || !cpu_has_ic_fills_f_dc))
+		return;
 
-static inline void __ide_flush_dcache_range(unsigned long addr, unsigned long size)
-{
-	if (cpu_has_dc_aliases || !cpu_has_ic_fills_f_dc) {
-		unsigned long end = addr + size;
+	while ((unsigned long)addr < end) {
+		struct page *p = virt_to_page(addr);
+		struct address_space *mapping = page_mapping(p);
 
-		while (addr < end) {
-			local_flush_data_cache_page((void *)addr);
-			addr += PAGE_SIZE;
-		}
+		if (mapping && mapping_mapped(mapping))
+			SetPageDcacheDirty(p);
+		addr += PAGE_SIZE;
 	}
 }
 
-/*
- * insw() and gang might be called with interrupts disabled, so we can't
- * send IPIs for flushing due to the potencial of deadlocks, see the comment
- * above smp_call_function() in arch/mips/kernel/smp.c.  We work around the
- * problem by disabling preemption so we know we actually perform the flush
- * on the processor that actually has the lines to be flushed which hopefully
- * is even better for performance anyway.
- */
 static inline void __ide_insw(unsigned long port, void *addr,
 	unsigned int count)
 {
-	__ide_flush_prologue();
 	insw(port, addr, count);
-	__ide_flush_dcache_range((unsigned long)addr, count * 2);
-	__ide_flush_epilogue();
+	__ide_set_pages_dirty(addr, count * 2);
 }
 
-static inline void __ide_insl(unsigned long port, void *addr, unsigned int count)
+static inline void __ide_insl(unsigned long port, void *addr,
+	unsigned int count)
 {
-	__ide_flush_prologue();
 	insl(port, addr, count);
-	__ide_flush_dcache_range((unsigned long)addr, count * 4);
-	__ide_flush_epilogue();
+	__ide_set_pages_dirty(addr, count * 4);
 }
 
 static inline void __ide_outsw(unsigned long port, const void *addr,
 	unsigned long count)
 {
-	__ide_flush_prologue();
 	outsw(port, addr, count);
-	__ide_flush_dcache_range((unsigned long)addr, count * 2);
-	__ide_flush_epilogue();
 }
 
 static inline void __ide_outsl(unsigned long port, const void *addr,
 	unsigned long count)
 {
-	__ide_flush_prologue();
 	outsl(port, addr, count);
-	__ide_flush_dcache_range((unsigned long)addr, count * 4);
-	__ide_flush_epilogue();
 }
 
 static inline void __ide_mm_insw(void __iomem *port, void *addr, u32 count)
 {
-	__ide_flush_prologue();
 	readsw(port, addr, count);
-	__ide_flush_dcache_range((unsigned long)addr, count * 2);
-	__ide_flush_epilogue();
+	__ide_set_pages_dirty(addr, count * 2);
 }
 
 static inline void __ide_mm_insl(void __iomem *port, void *addr, u32 count)
 {
-	__ide_flush_prologue();
 	readsl(port, addr, count);
-	__ide_flush_dcache_range((unsigned long)addr, count * 4);
-	__ide_flush_epilogue();
+	__ide_set_pages_dirty(addr, count * 4);
 }
 
 static inline void __ide_mm_outsw(void __iomem *port, void *addr, u32 count)
 {
-	__ide_flush_prologue();
 	writesw(port, addr, count);
-	__ide_flush_dcache_range((unsigned long)addr, count * 2);
-	__ide_flush_epilogue();
 }
 
 static inline void __ide_mm_outsl(void __iomem * port, void *addr, u32 count)
 {
-	__ide_flush_prologue();
 	writesl(port, addr, count);
-	__ide_flush_dcache_range((unsigned long)addr, count * 4);
-	__ide_flush_epilogue();
 }
 
 /* ide_insw calls insw, not __ide_insw.  Why? */
diff --git a/arch/mips/mm/c-r3k.c b/arch/mips/mm/c-r3k.c
index 54e5f7b..d841170 100644
--- a/arch/mips/mm/c-r3k.c
+++ b/arch/mips/mm/c-r3k.c
@@ -267,10 +267,6 @@ static void r3k_flush_cache_page(struct vm_area_struct *vma,
 		r3k_flush_icache_range(kaddr, kaddr + PAGE_SIZE);
 }
 
-static void local_r3k_flush_data_cache_page(void *addr)
-{
-}
-
 static void r3k_flush_data_cache_page(unsigned long addr)
 {
 }
@@ -324,7 +320,6 @@ void __cpuinit r3k_cache_init(void)
 	local_flush_icache_range = r3k_flush_icache_range;
 
 	flush_cache_sigtramp = r3k_flush_cache_sigtramp;
-	local_flush_data_cache_page = local_r3k_flush_data_cache_page;
 	flush_data_cache_page = r3k_flush_data_cache_page;
 
 	_dma_cache_wback_inv = r3k_dma_cache_wback_inv;
diff --git a/arch/mips/mm/c-r4k.c b/arch/mips/mm/c-r4k.c
index 6721ee2..a103185 100644
--- a/arch/mips/mm/c-r4k.c
+++ b/arch/mips/mm/c-r4k.c
@@ -1403,7 +1403,6 @@ void __cpuinit r4k_cache_init(void)
 
 	flush_cache_sigtramp	= r4k_flush_cache_sigtramp;
 	flush_icache_all	= r4k_flush_icache_all;
-	local_flush_data_cache_page	= local_r4k_flush_data_cache_page;
 	flush_data_cache_page	= r4k_flush_data_cache_page;
 	flush_icache_range	= r4k_flush_icache_range;
 	local_flush_icache_range	= local_r4k_flush_icache_range;
diff --git a/arch/mips/mm/c-tx39.c b/arch/mips/mm/c-tx39.c
index 6515b44..f98bfad 100644
--- a/arch/mips/mm/c-tx39.c
+++ b/arch/mips/mm/c-tx39.c
@@ -221,11 +221,6 @@ static void tx39_flush_cache_page(struct vm_area_struct *vma, unsigned long page
 		tx39_blast_icache_page_indexed(page);
 }
 
-static void local_tx39_flush_data_cache_page(void * addr)
-{
-	tx39_blast_dcache_page((unsigned long)addr);
-}
-
 static void tx39_flush_data_cache_page(unsigned long addr)
 {
 	tx39_blast_dcache_page(addr);
@@ -366,7 +361,6 @@ void __cpuinit tx39_cache_init(void)
 		local_flush_icache_range = (void *) tx39h_flush_icache_all;
 
 		flush_cache_sigtramp	= (void *) tx39h_flush_icache_all;
-		local_flush_data_cache_page	= (void *) tx39h_flush_icache_all;
 		flush_data_cache_page	= (void *) tx39h_flush_icache_all;
 
 		_dma_cache_wback_inv	= tx39h_dma_cache_wback_inv;
@@ -395,7 +389,6 @@ void __cpuinit tx39_cache_init(void)
 		local_flush_icache_range = tx39_flush_icache_range;
 
 		flush_cache_sigtramp = tx39_flush_cache_sigtramp;
-		local_flush_data_cache_page = local_tx39_flush_data_cache_page;
 		flush_data_cache_page = tx39_flush_data_cache_page;
 
 		_dma_cache_wback_inv = tx39_dma_cache_wback_inv;
diff --git a/arch/mips/mm/cache.c b/arch/mips/mm/cache.c
index e716caf..cb5bf1a 100644
--- a/arch/mips/mm/cache.c
+++ b/arch/mips/mm/cache.c
@@ -37,11 +37,9 @@ void (*__flush_cache_vunmap)(void);
 
 /* MIPS specific cache operations */
 void (*flush_cache_sigtramp)(unsigned long addr);
-void (*local_flush_data_cache_page)(void * addr);
 void (*flush_data_cache_page)(unsigned long addr);
 void (*flush_icache_all)(void);
 
-EXPORT_SYMBOL_GPL(local_flush_data_cache_page);
 EXPORT_SYMBOL(flush_data_cache_page);
 
 #ifdef CONFIG_DMA_NONCOHERENT
-- 
1.6.6


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 3/3] ide: move dcache flushing to generic ide code
  2010-02-28 15:35 Fix ide on Swarm and improve it a little Sebastian Andrzej Siewior
  2010-02-28 15:35 ` [PATCH 1/3] mips/ide: flush dcache also if icache does not snoop dcache Sebastian Andrzej Siewior
  2010-02-28 15:35 ` [PATCH 2/3] mips/ide: clean up / minimize dcache flushes Sebastian Andrzej Siewior
@ 2010-02-28 15:35 ` Sebastian Andrzej Siewior
  2010-03-01  2:34   ` David Miller
  2 siblings, 1 reply; 9+ messages in thread
From: Sebastian Andrzej Siewior @ 2010-02-28 15:35 UTC (permalink / raw)
  To: Ralf Baechle
  Cc: linux-mips, David S. Miller, linux-ide, Sebastian Andrzej Siewior

the pio callbacks are called with different kind of buffers. It could be
a straight kernel addr, kernel stack or a kmaped highmem page.
Some of this break the virt_to_page() assumptions.
This patch moves the dcache flush from architecture code into generic
ide code. ide_pio_bytes() is the only place where user pages might be
written as far as I can see.
The dcache flush is avoided in two cases:
- data which is written to the device (i.e. they are comming from the
  userland)
- pages without a mapping. Those requests should be issued by
  vfs and not go to the userland.

Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---
 arch/mips/include/asm/mach-generic/ide.h |   21 ---------------------
 arch/sparc/include/asm/ide.h             |   14 --------------
 drivers/ide/ide-taskfile.c               |    7 +++++++
 3 files changed, 7 insertions(+), 35 deletions(-)

diff --git a/arch/mips/include/asm/mach-generic/ide.h b/arch/mips/include/asm/mach-generic/ide.h
index 9360586..7370845 100644
--- a/arch/mips/include/asm/mach-generic/ide.h
+++ b/arch/mips/include/asm/mach-generic/ide.h
@@ -20,35 +20,16 @@
 #include <asm/processor.h>
 
 /* MIPS port and memory-mapped I/O string operations.  */
-static inline void __ide_set_pages_dirty(const void *addr, unsigned long size)
-{
-	unsigned long end = (unsigned long)addr + size;
-
-	if (!(cpu_has_dc_aliases || !cpu_has_ic_fills_f_dc))
-		return;
-
-	while ((unsigned long)addr < end) {
-		struct page *p = virt_to_page(addr);
-		struct address_space *mapping = page_mapping(p);
-
-		if (mapping && mapping_mapped(mapping))
-			SetPageDcacheDirty(p);
-		addr += PAGE_SIZE;
-	}
-}
-
 static inline void __ide_insw(unsigned long port, void *addr,
 	unsigned int count)
 {
 	insw(port, addr, count);
-	__ide_set_pages_dirty(addr, count * 2);
 }
 
 static inline void __ide_insl(unsigned long port, void *addr,
 	unsigned int count)
 {
 	insl(port, addr, count);
-	__ide_set_pages_dirty(addr, count * 4);
 }
 
 static inline void __ide_outsw(unsigned long port, const void *addr,
@@ -66,13 +47,11 @@ static inline void __ide_outsl(unsigned long port, const void *addr,
 static inline void __ide_mm_insw(void __iomem *port, void *addr, u32 count)
 {
 	readsw(port, addr, count);
-	__ide_set_pages_dirty(addr, count * 2);
 }
 
 static inline void __ide_mm_insl(void __iomem *port, void *addr, u32 count)
 {
 	readsl(port, addr, count);
-	__ide_set_pages_dirty(addr, count * 4);
 }
 
 static inline void __ide_mm_outsw(void __iomem *port, void *addr, u32 count)
diff --git a/arch/sparc/include/asm/ide.h b/arch/sparc/include/asm/ide.h
index b7af3d6..c1037ca 100644
--- a/arch/sparc/include/asm/ide.h
+++ b/arch/sparc/include/asm/ide.h
@@ -34,9 +34,6 @@
 
 static inline void __ide_insw(void __iomem *port, void *dst, u32 count)
 {
-#if defined(CONFIG_SPARC64) && defined(DCACHE_ALIASING_POSSIBLE)
-	unsigned long end = (unsigned long)dst + (count << 1);
-#endif
 	u16 *ps = dst;
 	u32 *pi;
 
@@ -56,17 +53,10 @@ static inline void __ide_insw(void __iomem *port, void *dst, u32 count)
 	ps = (u16 *)pi;
 	if(count)
 		*ps++ = __raw_readw(port);
-
-#if defined(CONFIG_SPARC64) && defined(DCACHE_ALIASING_POSSIBLE)
-	__flush_dcache_range((unsigned long)dst, end);
-#endif
 }
 
 static inline void __ide_outsw(void __iomem *port, const void *src, u32 count)
 {
-#if defined(CONFIG_SPARC64) && defined(DCACHE_ALIASING_POSSIBLE)
-	unsigned long end = (unsigned long)src + (count << 1);
-#endif
 	const u16 *ps = src;
 	const u32 *pi;
 
@@ -86,10 +76,6 @@ static inline void __ide_outsw(void __iomem *port, const void *src, u32 count)
 	ps = (const u16 *)pi;
 	if(count)
 		__raw_writew(*ps, port);
-
-#if defined(CONFIG_SPARC64) && defined(DCACHE_ALIASING_POSSIBLE)
-	__flush_dcache_range((unsigned long)src, end);
-#endif
 }
 
 #endif /* __KERNEL__ */
diff --git a/drivers/ide/ide-taskfile.c b/drivers/ide/ide-taskfile.c
index cc8633c..95a9922 100644
--- a/drivers/ide/ide-taskfile.c
+++ b/drivers/ide/ide-taskfile.c
@@ -273,6 +273,13 @@ void ide_pio_bytes(ide_drive_t *drive, struct ide_cmd *cmd,
 		if (page_is_high)
 			local_irq_restore(flags);
 
+		if (!write) {
+			struct address_space *mapping = page_mapping(page);
+
+			if (mapping && mapping_mapped(mapping))
+				flush_dcache_page(page);
+		}
+
 		len -= nr_bytes;
 	}
 }
-- 
1.6.6


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] ide: move dcache flushing to generic ide code
  2010-02-28 15:35 ` [PATCH 3/3] ide: move dcache flushing to generic ide code Sebastian Andrzej Siewior
@ 2010-03-01  2:34   ` David Miller
  2010-03-01 19:58     ` Sebastian Andrzej Siewior
  0 siblings, 1 reply; 9+ messages in thread
From: David Miller @ 2010-03-01  2:34 UTC (permalink / raw)
  To: sebastian; +Cc: ralf, linux-mips, linux-ide

From: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
Date: Sun, 28 Feb 2010 16:35:41 +0100

> the pio callbacks are called with different kind of buffers. It could be
> a straight kernel addr, kernel stack or a kmaped highmem page.
> Some of this break the virt_to_page() assumptions.
> This patch moves the dcache flush from architecture code into generic
> ide code. ide_pio_bytes() is the only place where user pages might be
> written as far as I can see.
> The dcache flush is avoided in two cases:
> - data which is written to the device (i.e. they are comming from the
>   userland)

This needs a flush on sparc, otherwise an alias now exists in the
kernel side copy of the buffer.  The D-cache flush is intentionally
unconditional for PIO mode.  I definitely don't want to take the same
risks you guys seem to be willing to take for this optimization which
is of questionable value.

I also, intrinsically, really don't like these changes.

For one thing, you're optimizing PIO mode.

Secondly, IDE is in deep maintainence mode, if you want to optimize
cache flushing do it in the ATA layer.

Thanks.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] ide: move dcache flushing to generic ide code
  2010-03-01  2:34   ` David Miller
@ 2010-03-01 19:58     ` Sebastian Andrzej Siewior
  2010-03-02  0:39       ` David Miller
  0 siblings, 1 reply; 9+ messages in thread
From: Sebastian Andrzej Siewior @ 2010-03-01 19:58 UTC (permalink / raw)
  To: David Miller; +Cc: ralf, linux-mips, linux-ide

* David Miller | 2010-02-28 18:34:17 [-0800]:

>From: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
>Date: Sun, 28 Feb 2010 16:35:41 +0100
>
>> the pio callbacks are called with different kind of buffers. It could be
>> a straight kernel addr, kernel stack or a kmaped highmem page.
>> Some of this break the virt_to_page() assumptions.
>> This patch moves the dcache flush from architecture code into generic
>> ide code. ide_pio_bytes() is the only place where user pages might be
>> written as far as I can see.
>> The dcache flush is avoided in two cases:
>> - data which is written to the device (i.e. they are comming from the
>>   userland)
>
>This needs a flush on sparc, otherwise an alias now exists in the
>kernel side copy of the buffer.  The D-cache flush is intentionally
>unconditional for PIO mode.  I definitely don't want to take the same
>risks you guys seem to be willing to take for this optimization which
>is of questionable value.
It is not us guys it is just me. And if it is a stupid thing to do then
I get probably punched by Ralf as well once he gets back.
The part I don't get is why you have to flush dcache after the copy
process. I would understand a flush before reading. The dcache alias in
kernel shouldn't hurt since it is not used anymore. Unless we read twice
from the same page. Is this the trick?

>I also, intrinsically, really don't like these changes.
>
>For one thing, you're optimizing PIO mode.
>
>Secondly, IDE is in deep maintainence mode, if you want to optimize
>cache flushing do it in the ATA layer.
This patch patch was mostly driven by the fact that the buffer can be a
normal kernel mapping or a virtual address. The latter doesn't work with
virt_to_page().
Anyway I should probably spent more time getting ATA layer wokring than
on the IDE layer since it is somehow working since patch#1.

>
>Thanks.

Sebastian

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] ide: move dcache flushing to generic ide code
  2010-03-01 19:58     ` Sebastian Andrzej Siewior
@ 2010-03-02  0:39       ` David Miller
  2010-03-02 21:14         ` Sebastian Andrzej Siewior
  2010-03-25 17:17         ` Ralf Baechle
  0 siblings, 2 replies; 9+ messages in thread
From: David Miller @ 2010-03-02  0:39 UTC (permalink / raw)
  To: sebastian; +Cc: ralf, linux-mips, linux-ide

From: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
Date: Mon, 1 Mar 2010 20:58:58 +0100

> The part I don't get is why you have to flush dcache after the copy
> process. I would understand a flush before reading. The dcache alias in
> kernel shouldn't hurt since it is not used anymore. Unless we read twice
> from the same page. Is this the trick?

Anything that puts the data into the cache on the kernel
side is a problem.  The page is still potentially in user
space, as a result there will be thus two active mappings
in the cache, one in the kernel side and one in the user
side.  The user can then do various operations which can
access either mapping.

Writing to it via write() system call, writing to it via
mmap(), making the kernel write to it by doing a read()
with the buffer pointing into the mmap() area.

All we need is a modification on either side for the other
one to potentially become stale.

>>Secondly, IDE is in deep maintainence mode, if you want to optimize
>>cache flushing do it in the ATA layer.
> This patch patch was mostly driven by the fact that the buffer can be a
> normal kernel mapping or a virtual address. The latter doesn't work with
> virt_to_page().
> Anyway I should probably spent more time getting ATA layer wokring than
> on the IDE layer since it is somehow working since patch#1.

All buffers passed to the architecture IDE ops should be PAGE_OFFSET
relative kernel virtual addresses for which __pa() works.

Are kmap()'d things ending up here?

It all works out on sparc64 because we don't have highmem and
kernel stacks are just in normal kernel pages.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] ide: move dcache flushing to generic ide code
  2010-03-02  0:39       ` David Miller
@ 2010-03-02 21:14         ` Sebastian Andrzej Siewior
  2010-03-25 17:17         ` Ralf Baechle
  1 sibling, 0 replies; 9+ messages in thread
From: Sebastian Andrzej Siewior @ 2010-03-02 21:14 UTC (permalink / raw)
  To: David Miller; +Cc: sebastian, ralf, linux-mips, linux-ide

* David Miller | 2010-03-01 16:39:59 [-0800]:

>All buffers passed to the architecture IDE ops should be PAGE_OFFSET
>relative kernel virtual addresses for which __pa() works.
>
>Are kmap()'d things ending up here?
Ah, bounce buffers are used. So no, there no highmem which get kmap()ed.

Sebastian

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] ide: move dcache flushing to generic ide code
  2010-03-02  0:39       ` David Miller
  2010-03-02 21:14         ` Sebastian Andrzej Siewior
@ 2010-03-25 17:17         ` Ralf Baechle
  1 sibling, 0 replies; 9+ messages in thread
From: Ralf Baechle @ 2010-03-25 17:17 UTC (permalink / raw)
  To: David Miller; +Cc: sebastian, linux-mips, linux-ide

On Mon, Mar 01, 2010 at 04:39:59PM -0800, David Miller wrote:

> From: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
> Date: Mon, 1 Mar 2010 20:58:58 +0100
> 
> > The part I don't get is why you have to flush dcache after the copy
> > process. I would understand a flush before reading. The dcache alias in
> > kernel shouldn't hurt since it is not used anymore. Unless we read twice
> > from the same page. Is this the trick?
> 
> Anything that puts the data into the cache on the kernel
> side is a problem.  The page is still potentially in user
> space, as a result there will be thus two active mappings
> in the cache, one in the kernel side and one in the user
> side.  The user can then do various operations which can
> access either mapping.
> 
> Writing to it via write() system call, writing to it via
> mmap(), making the kernel write to it by doing a read()
> with the buffer pointing into the mmap() area.
> 
> All we need is a modification on either side for the other
> one to potentially become stale.
> 
> >>Secondly, IDE is in deep maintainence mode, if you want to optimize
> >>cache flushing do it in the ATA layer.
> > This patch patch was mostly driven by the fact that the buffer can be a
> > normal kernel mapping or a virtual address. The latter doesn't work with
> > virt_to_page().
> > Anyway I should probably spent more time getting ATA layer wokring than
> > on the IDE layer since it is somehow working since patch#1.
> 
> All buffers passed to the architecture IDE ops should be PAGE_OFFSET
> relative kernel virtual addresses for which __pa() works.
> 
> Are kmap()'d things ending up here?
> 
> It all works out on sparc64 because we don't have highmem and
> kernel stacks are just in normal kernel pages.

Highmem on MIPS has always been a bastard child though that will probably
change now that 32-bit embedded systems are coming with more memory than
can be mapped as lowmem.

The system in question that Sebastian is talking about has the IDE cache
problems because it has no aliases.  Neither the IDE code nor the cache
flush layer does any cacheflushes because there are no cache aliases [1] to
deal with.

So eventually code from a page loaded from an IDE disk gets executed and
if the CPU's I-cache refilled from the L2 or memory, not from the D-cache
stale data may get executed.

In an earlier mail in this thread you called the MIPS ins/outs risky.
Maybe - but we have to.  At least at the time when this code was written
the IDE ins/outs variants was possibly being called with interrupts
disabled.  The normal MIPS cache flushing functions are based on
smp_call_function() on SMP systems [2] so can't be used to do cache
maintenance hence we have to avoid them.

  Ralf

[1] Configuring page size to 16k or 64k would get rid of aliases on all
    MIPS processors with VIPT D-caches, so introduce the same situation.
    Such configurations are getting common now.
[2] Because the CACHE instruction only affects the local CPU cache.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2010-03-25 17:17 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-02-28 15:35 Fix ide on Swarm and improve it a little Sebastian Andrzej Siewior
2010-02-28 15:35 ` [PATCH 1/3] mips/ide: flush dcache also if icache does not snoop dcache Sebastian Andrzej Siewior
2010-02-28 15:35 ` [PATCH 2/3] mips/ide: clean up / minimize dcache flushes Sebastian Andrzej Siewior
2010-02-28 15:35 ` [PATCH 3/3] ide: move dcache flushing to generic ide code Sebastian Andrzej Siewior
2010-03-01  2:34   ` David Miller
2010-03-01 19:58     ` Sebastian Andrzej Siewior
2010-03-02  0:39       ` David Miller
2010-03-02 21:14         ` Sebastian Andrzej Siewior
2010-03-25 17:17         ` Ralf Baechle

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).