sh: fix wrong icache/dcache address-array start addr in cache-debugfs.

sh: fix wrong icache/dcache address-array start addr in cache-debugfs.

[Srinivas KANDAGATLA]

[-- Attachment #1: Type: text/plain, Size: 550 bytes --]

Hi All,
Recently we discovered a bug in cache-debugfs code. Problem is because 
of static variable (addrstart ) used to remember icache/dcache address 
array start address.
As its static the addrstart ends up with incorrect value after few 
passes of the function.

ex: Correct icache address array start address is 0xF000 0000 but with 
the existing code the start address ends up at 0xF000 8000

This patch makes the variable non-static, reset the start address every 
time it enters the function and cleanup unnecessary variables.

Thanks,
srini


[-- Attachment #2: 0001-sh-fix-wrong-icache-dcache-address-array-start-addr-.patch --]
[-- Type: text/x-patch, Size: 2439 bytes --]

From dff32307c0b29355bb9925a3cdd2c09db77a39b7 Mon Sep 17 00:00:00 2001
From: Srinivas Kandagatla <srinivas.kandagatla@st.com>
Date: Thu, 2 Jun 2011 10:32:36 +0100
Subject: [PATCH sh-2.6.32.y] sh: fix wrong icache/dcache address-array start addr in cache-debugfs.

This patch fixes a icache/dcache address-array start address while
dumping its entires in debugfs. Perviously the code was attempting to
remember the address in static variable, which is no more required
for debugfs, as the function can be executed in one pass.

Without this patch the start address ends up in wrong place and the
/sys/kernel/debug/sh/icache or dcache debugfs contents may not be correct.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@st.com>
---
 arch/sh/mm/cache-debugfs.c |   25 +++++--------------------
 1 files changed, 5 insertions(+), 20 deletions(-)

diff --git a/arch/sh/mm/cache-debugfs.c b/arch/sh/mm/cache-debugfs.c
index 5241146..1157251 100644
--- a/arch/sh/mm/cache-debugfs.c
+++ b/arch/sh/mm/cache-debugfs.c
@@ -26,9 +26,9 @@ static int cache_seq_show(struct seq_file *file, void *iter)
 {
 	unsigned int cache_type = (unsigned int)file->private;
 	struct cache_info *cache;
-	unsigned int waysize, way, cache_size;
-	unsigned long ccr, base;
-	static unsigned long addrstart = 0;
+	unsigned int waysize, way;
+	unsigned long ccr;
+	unsigned long addrstart = 0;
 
 	/*
 	 * Go uncached immediately so we don't skew the results any
@@ -45,28 +45,13 @@ static int cache_seq_show(struct seq_file *file, void *iter)
 	}
 
 	if (cache_type == CACHE_TYPE_DCACHE) {
-		base = CACHE_OC_ADDRESS_ARRAY;
+		addrstart = CACHE_OC_ADDRESS_ARRAY;
 		cache = &current_cpu_data.dcache;
 	} else {
-		base = CACHE_IC_ADDRESS_ARRAY;
+		addrstart = CACHE_IC_ADDRESS_ARRAY;
 		cache = &current_cpu_data.icache;
 	}
 
-	/*
-	 * Due to the amount of data written out (depending on the cache size),
-	 * we may be iterated over multiple times. In this case, keep track of
-	 * the entry position in addrstart, and rewind it when we've hit the
-	 * end of the cache.
-	 *
-	 * Likewise, the same code is used for multiple caches, so care must
-	 * be taken for bouncing addrstart back and forth so the appropriate
-	 * cache is hit.
-	 */
-	cache_size = cache->ways * cache->sets * cache->linesz;
-	if (((addrstart & 0xff000000) != base) ||
-	     (addrstart & 0x00ffffff) > cache_size)
-		addrstart = base;
-
 	waysize = cache->sets;
 
 	/*
-- 
1.6.3.3

Re: sh: fix wrong icache/dcache address-array start addr in cache-debugfs.

[Paul Mundt]

On Thu, Jun 02, 2011 at 11:30:44AM +0100, Srinivas KANDAGATLA wrote:
> This patch fixes a icache/dcache address-array start address while
> dumping its entires in debugfs. Perviously the code was attempting to
> remember the address in static variable, which is no more required
> for debugfs, as the function can be executed in one pass.
> 
If the assertion that we can now do this in one pass is true then
certainly killing of the complexity/brokenness of caching the array can
be dropped. It's been many years since I wrote that code so I have
absolutely no recollection of the re-entry case, but it was necessary at
the time. Has something changed in debugfs or the seq file interface that
I'm unaware of that suddenly makes everything doable in a single pass?

Also, what cache configurations have you tested this with? Our common
case is 4-way set associative 32kB dcache, which if memory serves was
already enough to overrun whatever debugfs had handy (I suppose a 4k
page, as with procfs).

Re: sh: fix wrong icache/dcache address-array start addr in cache-debugfs.

[Srinivas KANDAGATLA]

Hi Paul,
My comments are in-lined.
Paul Mundt wrote:
> On Thu, Jun 02, 2011 at 11:30:44AM +0100, Srinivas KANDAGATLA wrote:
>> This patch fixes a icache/dcache address-array start address while
>> dumping its entires in debugfs. Perviously the code was attempting to
>> remember the address in static variable, which is no more required
>> for debugfs, as the function can be executed in one pass.
>>
> If the assertion that we can now do this in one pass is true then
> certainly killing of the complexity/brokenness of caching the array can
> be dropped. It's been many years since I wrote that code so I have
> absolutely no recollection of the re-entry case, but it was necessary at
> the time. 
Could be that, the existing code was carried from non-seq_file style 
interface (proc read...).
> Has something changed in debugfs or the seq file interface that
> I'm unaware of that suddenly makes everything doable in a single pass?
The seq_file interface, seq_read allocates sufficient buff memory to run 
show function in single pass.
>
> Also, what cache configurations have you tested this with? Our common
> case is 4-way set associative 32kB dcache, which if memory serves was
> already enough to overrun whatever debugfs had handy (I suppose a 4k
> page, as with procfs).
I did test on 2Way cache with 32KB icache/dcache system.
Original issue is not do with overrun, but incorrect results in virtual 
file.



Thanks,
srini

Re: sh: fix wrong icache/dcache address-array start addr in cache-debugfs.

[Paul Mundt]

On Fri, Jun 03, 2011 at 02:42:09PM +0100, Srinivas KANDAGATLA wrote:
> Hi Paul,
> My comments are in-lined.
> Paul Mundt wrote:
> >On Thu, Jun 02, 2011 at 11:30:44AM +0100, Srinivas KANDAGATLA wrote:
> >>This patch fixes a icache/dcache address-array start address while
> >>dumping its entires in debugfs. Perviously the code was attempting to
> >>remember the address in static variable, which is no more required
> >>for debugfs, as the function can be executed in one pass.
> >>
> >If the assertion that we can now do this in one pass is true then
> >certainly killing of the complexity/brokenness of caching the array can
> >be dropped. It's been many years since I wrote that code so I have
> >absolutely no recollection of the re-entry case, but it was necessary at
> >the time. 
> Could be that, the existing code was carried from non-seq_file style 
> interface (proc read...).

That sounds like the most probable case.

> >Also, what cache configurations have you tested this with? Our common
> >case is 4-way set associative 32kB dcache, which if memory serves was
> >already enough to overrun whatever debugfs had handy (I suppose a 4k
> >page, as with procfs).
> I did test on 2Way cache with 32KB icache/dcache system.
> Original issue is not do with overrun, but incorrect results in virtual 
> file.
> 
Good enough for me. Applied, thanks.