Skip to contentSkip to conversation

Fix missing L2 cache size in /sys/devices/system/cpu

PATCH Superseded [email protected] 2 participants 6 messages Feb 26, 2015 – Apr 3, 2015 PowerPC

lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH v3 1/1 Fix missing L2 cache size in /sys/devices/system/cpu

From: Dave Olson <[email protected]>

This problem appears to have been introduced in 2.6.29 by 93197a36a9c16a85fb24cf5a8639f7bf9af838a3.

This caused lscpu to error out on e500v2 devices, and probably others
error: cannot open /sys/devices/system/cpu/cpu0/cache/index2/size: No such file or directory

Some embedded powerpc systems use cache-size in DTS for the unified L2 cache size, not d-cache-size, so we need to allow for both DTS names. Added a new CACHE_TYPE_UNIFIED_D cache_type_info structure to handle this.

This patch is against git://git.kernel.org/pub/scm/linux/kernel/git/benh/powerpc.git
next branch. It is the same as the v2 patch, but against next, rather than master. I can rebase, if desired.

Signed-off-by Dave Olson <[email protected]>
---
 arch/powerpc/kernel/cacheinfo.c |   38 ++++++++++++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 4 deletions(-)
arch/powerpc/kernel/cacheinfo.c +34 -4
index ae77b7e..860ed54 100644
@@ -61,12 +61,22 @@ struct cache_type_info {
 };
 
 /* These are used to index the cache_type_info array. */
-#define CACHE_TYPE_UNIFIED     0
-#define CACHE_TYPE_INSTRUCTION 1
-#define CACHE_TYPE_DATA        2
+#define CACHE_TYPE_UNIFIED     0 /* cache-size, cache-block-size, etc. */
+#define CACHE_TYPE_UNIFIED_D   1 /* d-cache-size, d-cache-block-size, etc */
+#define CACHE_TYPE_INSTRUCTION 2
+#define CACHE_TYPE_DATA        3
 
 static const struct cache_type_info cache_type_info[] = {
 	{
+		/* Embedded systems that use cache-size, cache-block-size,
+		 * etc. for the Unified (typically L2) cache. */
+		.name            = "Unified",
+		.size_prop       = "cache-size",
+		.line_size_props = { "cache-line-size",
+				     "cache-block-size", },
+		.nr_sets_prop    = "cache-sets",
+	},
+	{
 		/* PowerPC Processor binding says the [di]-cache-*
 		 * must be equal on unified caches, so just use
 		 * d-cache properties. */

@@ -293,7 +303,8 @@ static struct cache *cache_find_first_sibling(struct cache *cache)
 {
 	struct cache *iter;
 
-	if (cache->type == CACHE_TYPE_UNIFIED)
+	if (cache->type == CACHE_TYPE_UNIFIED ||
+	    cache->type == CACHE_TYPE_UNIFIED_D)
 		return cache;
 
 	list_for_each_entry(iter, &cache_list, list)

@@ -324,14 +335,33 @@ static bool cache_node_is_unified(const struct device_node *np)
 	return of_get_property(np, "cache-unified", NULL);
 }
 
+/*
+ * Handle unified caches that have two different types of tags.  Most embedded
+ * use cache-size, etc. for the unified cache size, but open firmware systems
+ * use d-cache-size, etc.   Since they all appear to be consistent, check on
+ * initialization for which type we are, and use the appropriate structure.
+ */
 static struct cache *cache_do_one_devnode_unified(struct device_node *node,
 						  int level)
 {
 	struct cache *cache;
+	int ucache;
 
 	pr_debug("creating L%d ucache for %s\n", level, node->full_name);
 
 	cache = new_cache(CACHE_TYPE_UNIFIED, level, node);
+	if (of_get_property(node,
+		cache_type_info[CACHE_TYPE_UNIFIED_D].size_prop, NULL)) {
+		ucache = CACHE_TYPE_UNIFIED_D;
+	} else {
+		ucache = CACHE_TYPE_UNIFIED; /* assume embedded */
+		if (of_get_property(node,
+			cache_type_info[CACHE_TYPE_UNIFIED].size_prop, NULL) ==
+			NULL)
+			printk(KERN_WARNING "Unified cache property missing\n");
+	}
+
+	cache = new_cache(ucache, level, node);
 
 	return cache;
 }
Signature · 1 line
1.7.10.4
reviewer
#1 lore raw
Quoted 9 lines · Dave Olson
arch/powerpc/kernel/cacheinfo.c +5 -0
@@ -324,14 +335,33 @@ static bool cache_node_is_unified(const struct device_node *np)
 	return of_get_property(np, "cache-unified", NULL);
 }
 
+/*
+ * Handle unified caches that have two different types of tags.  Most embedded
+ * use cache-size, etc. for the unified cache size, but open firmware systems
+ * use d-cache-size, etc.   Since they all appear to be consistent, check on
+ * initialization for which type we are, and use the appropriate structure.

arch/powerpc/kernel/cacheinfo.c static bool cache_node_is_unified(const struct device_node *np) · line 352

Quoted 10 lines
arch/powerpc/kernel/cacheinfo.c +2 -0
+ */
 static struct cache *cache_do_one_devnode_unified(struct device_node *node,
 						  int level)
 {
 	struct cache *cache;
+	int ucache;
 
 	pr_debug("creating L%d ucache for %s\n", level, node->full_name);
 
 	cache = new_cache(CACHE_TYPE_UNIFIED, level, node);

        ^^

arch/powerpc/kernel/cacheinfo.c static bool cache_node_is_unified(const struct device_node *np) · line 364

Quoted 12 lines
arch/powerpc/kernel/cacheinfo.c +12 -0
+	if (of_get_property(node,
+		cache_type_info[CACHE_TYPE_UNIFIED_D].size_prop, NULL)) {
+		ucache = CACHE_TYPE_UNIFIED_D;
+	} else {
+		ucache = CACHE_TYPE_UNIFIED; /* assume embedded */
+		if (of_get_property(node,
+			cache_type_info[CACHE_TYPE_UNIFIED].size_prop, NULL) ==
+			NULL)
+			printk(KERN_WARNING "Unified cache property missing\n");
+	}
+
+	cache = new_cache(ucache, level, node);

        ^^

arch/powerpc/kernel/cacheinfo.c static bool cache_node_is_unified(const struct device_node *np) · line 367

Quoted 3 lines
arch/powerpc/kernel/cacheinfo.c +0 -0
 	return cache;
 }

That looks fishy. You create a cache, and then throw it away and create another one and return that. I don't think that's what you intended, is it?

It would also be cleaner I think if you created another helper, eg. cache_is_unified_d() to do the property lookup.

And also I don't think you need to do the second property lookup, especially if all you're going to do is print a warning.

cheers

author
#2 lore raw
Quoted 36 lines · Michael Ellerman
Quoted 9 lines · Dave Olson
arch/powerpc/kernel/cacheinfo.c +5 -0
@@ -324,14 +335,33 @@ static bool cache_node_is_unified(const struct device_node *np)
 	return of_get_property(np, "cache-unified", NULL);
 }
 
+/*
+ * Handle unified caches that have two different types of tags.  Most embedded
+ * use cache-size, etc. for the unified cache size, but open firmware systems
+ * use d-cache-size, etc.   Since they all appear to be consistent, check on
+ * initialization for which type we are, and use the appropriate structure.

arch/powerpc/kernel/cacheinfo.c static bool cache_node_is_unified(const struct device_node *np) · line 352

Quoted 10 lines
arch/powerpc/kernel/cacheinfo.c +2 -0
+ */
 static struct cache *cache_do_one_devnode_unified(struct device_node *node,
 						  int level)
 {
 	struct cache *cache;
+	int ucache;
 
 	pr_debug("creating L%d ucache for %s\n", level, node->full_name);
 
 	cache = new_cache(CACHE_TYPE_UNIFIED, level, node);

        ^^

arch/powerpc/kernel/cacheinfo.c static bool cache_node_is_unified(const struct device_node *np) · line 364

Quoted 12 lines
arch/powerpc/kernel/cacheinfo.c +12 -0
+	if (of_get_property(node,
+		cache_type_info[CACHE_TYPE_UNIFIED_D].size_prop, NULL)) {
+		ucache = CACHE_TYPE_UNIFIED_D;
+	} else {
+		ucache = CACHE_TYPE_UNIFIED; /* assume embedded */
+		if (of_get_property(node,
+			cache_type_info[CACHE_TYPE_UNIFIED].size_prop, NULL) ==
+			NULL)
+			printk(KERN_WARNING "Unified cache property missing\n");
+	}
+
+	cache = new_cache(ucache, level, node);

        ^^
Quoted 1 line
Quoted 5 lines
Quoted 2 lines

return cache;
}

That looks fishy. You create a cache, and then throw it away and create another one and return that. I don't think that's what you intended, is it?

It looks like I missed something when I regenerated the patch, yes. My version of cacheinfo.c doesn't have the first new_cache() call.

Quoted 2 lines

It would also be cleaner I think if you created another helper, eg. cache_is_unified_d() to do the property lookup.

OK.

Quoted 2 lines

And also I don't think you need to do the second property lookup, especially if all you're going to do is print a warning.

I wanted to make sure that if a similar issue arose in the future, that a message was printed so it got caught and fixed. I don't see a way to do that without the 2nd lookup.

Let me know what you think, and I'll generate another patch.

Dave Olson
[email protected]

reviewer
#3 lore raw
Quoted 38 lines · Dave Olson
Quoted 36 lines · Michael Ellerman
Quoted 9 lines · Dave Olson
arch/powerpc/kernel/cacheinfo.c +5 -0
@@ -324,14 +335,33 @@ static bool cache_node_is_unified(const struct device_node *np)
 	return of_get_property(np, "cache-unified", NULL);
 }
 
+/*
+ * Handle unified caches that have two different types of tags.  Most embedded
+ * use cache-size, etc. for the unified cache size, but open firmware systems
+ * use d-cache-size, etc.   Since they all appear to be consistent, check on
+ * initialization for which type we are, and use the appropriate structure.

arch/powerpc/kernel/cacheinfo.c static bool cache_node_is_unified(const struct device_node *np) · line 352

Quoted 10 lines
arch/powerpc/kernel/cacheinfo.c +2 -0
+ */
 static struct cache *cache_do_one_devnode_unified(struct device_node *node,
 						  int level)
 {
 	struct cache *cache;
+	int ucache;
 
 	pr_debug("creating L%d ucache for %s\n", level, node->full_name);
 
 	cache = new_cache(CACHE_TYPE_UNIFIED, level, node);

        ^^

arch/powerpc/kernel/cacheinfo.c static bool cache_node_is_unified(const struct device_node *np) · line 364

Quoted 12 lines
arch/powerpc/kernel/cacheinfo.c +12 -0
+	if (of_get_property(node,
+		cache_type_info[CACHE_TYPE_UNIFIED_D].size_prop, NULL)) {
+		ucache = CACHE_TYPE_UNIFIED_D;
+	} else {
+		ucache = CACHE_TYPE_UNIFIED; /* assume embedded */
+		if (of_get_property(node,
+			cache_type_info[CACHE_TYPE_UNIFIED].size_prop, NULL) ==
+			NULL)
+			printk(KERN_WARNING "Unified cache property missing\n");
+	}
+
+	cache = new_cache(ucache, level, node);

        ^^
Quoted 1 line
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

return cache;
}

That looks fishy. You create a cache, and then throw it away and create another one and return that. I don't think that's what you intended, is it?

It looks like I missed something when I regenerated the patch, yes. My version of cacheinfo.c doesn't have the first new_cache() call.

OK, not sure how you did that. I recommend using git to generate the patch.

Quoted 6 lines
Quoted 2 lines

And also I don't think you need to do the second property lookup, especially if all you're going to do is print a warning.

I wanted to make sure that if a similar issue arose in the future, that a message was printed so it got caught and fixed. I don't see a way to do that without the 2nd lookup.

That's an admirable goal but I don't think that's the right way to catch it. If it's important then someone needs to be testing the end result, ie. the contents of /sys/devices/system/cpu/*/cache.

Also a single printk is never going to get noticed in the boot log, unless someone's already looking for it.

cheers

author
#4 lore raw

PATCH 1/1 Fix missing L2 cache size in /sys/devices/system/cpu

In reply to Dave Olson

From: Dave Olson <[email protected]>

This is version 4 of the patch, incorporating Michael Ellerman's suggested changes from March 26, 2015.
================

This problem appears to have been introduced in 2.6.29 by the commit in the Fixes line below.

This caused lscpu to error out on e500v2 devices, and probably others
error: cannot open /sys/devices/system/cpu/cpu0/cache/index2/size: No such file or directory

Some embedded powerpc systems use cache-size in DTS for the unified L2 cache size, not d-cache-size, so we need to allow for both DTS names. Added a new CACHE_TYPE_UNIFIED_D cache_type_info structure to handle this.

Fixes 93197a36a9c16a85fb24cf5a8639f7bf9af838a3
Signed-off-by Dave Olson <[email protected]>
---
 arch/powerpc/kernel/cacheinfo.c |   44 ++++++++++++++++++++++++++++++---------
 1 file changed, 34 insertions(+), 10 deletions(-)
arch/powerpc/kernel/cacheinfo.c +34 -10
index ae77b7e..b9df547 100644
@@ -61,12 +61,22 @@ struct cache_type_info {
 };
 
 /* These are used to index the cache_type_info array. */
-#define CACHE_TYPE_UNIFIED     0
-#define CACHE_TYPE_INSTRUCTION 1
-#define CACHE_TYPE_DATA        2
+#define CACHE_TYPE_UNIFIED     0 /* cache-size, cache-block-size, etc. */
+#define CACHE_TYPE_UNIFIED_D   1 /* d-cache-size, d-cache-block-size, etc */
+#define CACHE_TYPE_INSTRUCTION 2
+#define CACHE_TYPE_DATA        3
 
 static const struct cache_type_info cache_type_info[] = {
 	{
+		/* Embedded systems that use cache-size, cache-block-size,
+		 * etc. for the Unified (typically L2) cache. */
+		.name            = "Unified",
+		.size_prop       = "cache-size",
+		.line_size_props = { "cache-line-size",
+				     "cache-block-size", },
+		.nr_sets_prop    = "cache-sets",
+	},
+	{
 		/* PowerPC Processor binding says the [di]-cache-*
 		 * must be equal on unified caches, so just use
 		 * d-cache properties. */

@@ -293,7 +303,8 @@ static struct cache *cache_find_first_sibling(struct cache *cache)
 {
 	struct cache *iter;
 
-	if (cache->type == CACHE_TYPE_UNIFIED)
+	if (cache->type == CACHE_TYPE_UNIFIED ||
+	    cache->type == CACHE_TYPE_UNIFIED_D)
 		return cache;
 
 	list_for_each_entry(iter, &cache_list, list)

@@ -324,16 +335,29 @@ static bool cache_node_is_unified(const struct device_node *np)
 	return of_get_property(np, "cache-unified", NULL);
 }
 
-static struct cache *cache_do_one_devnode_unified(struct device_node *node,
-						  int level)
+/*
+ * Unified caches can have two different sets of tags.  Most embedded
+ * use cache-size, etc. for the unified cache size, but open firmware systems
+ * use d-cache-size, etc.   Check on initialization for which type we have, and
+ * return the appropriate structure type.  Assume it's embedded if it isn't
+ * open firmware.  If it's yet a 3rd type, then there will be missing entries
+ * in /sys/devices/system/cpu/cpu0/cache/index2/, and this code will need
+ * to be extended further.
+ */
+static int __cpuinit cache_is_unified_d(const struct device_node *np)
 {
-	struct cache *cache;
+	return of_get_property(np,
+		cache_type_info[CACHE_TYPE_UNIFIED_D].size_prop, NULL) ?
+		CACHE_TYPE_UNIFIED_D : CACHE_TYPE_UNIFIED;
+}
 
+/*
+ */
+static struct cache *__cpuinit cache_do_one_devnode_unified(struct device_node *node, int level)
+{
 	pr_debug("creating L%d ucache for %s\n", level, node->full_name);
 
-	cache = new_cache(CACHE_TYPE_UNIFIED, level, node);
-
-	return cache;
+	return new_cache(cache_is_unified_d(node), level, node);
 }
 
 static struct cache *cache_do_one_devnode_split(struct device_node *node,
Signature · 1 line
1.7.10.4
author
#5 lore raw

PATCH v5 1/1 Fix missing L2 cache size in /sys/devices/system/cpu

In reply to Dave Olson

From: Dave Olson <[email protected]>

This is version 5 of the patch, incorporating Michael Ellerman's suggested changes from March 26, 2015. v5 removes the __cpuinit that had crept in from my patch based on an older kernel, but is otherwise the same as v4.
================

This problem appears to have been introduced in 2.6.29 by the commit in the Fixes line below.

This caused lscpu to error out on e500v2 devices, and probably others
error: cannot open /sys/devices/system/cpu/cpu0/cache/index2/size: No such file or directory

Some embedded powerpc systems use cache-size in DTS for the unified L2 cache size, not d-cache-size, so we need to allow for both DTS names. Added a new CACHE_TYPE_UNIFIED_D cache_type_info structure to handle this.

Fixes 93197a36a9c16a85fb24cf5a8639f7bf9af838a3
Signed-off-by Dave Olson <[email protected]>
---
 arch/powerpc/kernel/cacheinfo.c |   44 ++++++++++++++++++++++++++++++---------
 1 file changed, 34 insertions(+), 10 deletions(-)
arch/powerpc/kernel/cacheinfo.c +34 -10
index ae77b7e..c641983 100644
@@ -61,12 +61,22 @@ struct cache_type_info {
 };
 
 /* These are used to index the cache_type_info array. */
-#define CACHE_TYPE_UNIFIED     0
-#define CACHE_TYPE_INSTRUCTION 1
-#define CACHE_TYPE_DATA        2
+#define CACHE_TYPE_UNIFIED     0 /* cache-size, cache-block-size, etc. */
+#define CACHE_TYPE_UNIFIED_D   1 /* d-cache-size, d-cache-block-size, etc */
+#define CACHE_TYPE_INSTRUCTION 2
+#define CACHE_TYPE_DATA        3
 
 static const struct cache_type_info cache_type_info[] = {
 	{
+		/* Embedded systems that use cache-size, cache-block-size,
+		 * etc. for the Unified (typically L2) cache. */
+		.name            = "Unified",
+		.size_prop       = "cache-size",
+		.line_size_props = { "cache-line-size",
+				     "cache-block-size", },
+		.nr_sets_prop    = "cache-sets",
+	},
+	{
 		/* PowerPC Processor binding says the [di]-cache-*
 		 * must be equal on unified caches, so just use
 		 * d-cache properties. */

@@ -293,7 +303,8 @@ static struct cache *cache_find_first_sibling(struct cache *cache)
 {
 	struct cache *iter;
 
-	if (cache->type == CACHE_TYPE_UNIFIED)
+	if (cache->type == CACHE_TYPE_UNIFIED ||
+	    cache->type == CACHE_TYPE_UNIFIED_D)
 		return cache;
 
 	list_for_each_entry(iter, &cache_list, list)

@@ -324,16 +335,29 @@ static bool cache_node_is_unified(const struct device_node *np)
 	return of_get_property(np, "cache-unified", NULL);
 }
 
-static struct cache *cache_do_one_devnode_unified(struct device_node *node,
-						  int level)
+/*
+ * Unified caches can have two different sets of tags.  Most embedded
+ * use cache-size, etc. for the unified cache size, but open firmware systems
+ * use d-cache-size, etc.   Check on initialization for which type we have, and
+ * return the appropriate structure type.  Assume it's embedded if it isn't
+ * open firmware.  If it's yet a 3rd type, then there will be missing entries
+ * in /sys/devices/system/cpu/cpu0/cache/index2/, and this code will need
+ * to be extended further.
+ */
+static int cache_is_unified_d(const struct device_node *np)
 {
-	struct cache *cache;
+	return of_get_property(np,
+		cache_type_info[CACHE_TYPE_UNIFIED_D].size_prop, NULL) ?
+		CACHE_TYPE_UNIFIED_D : CACHE_TYPE_UNIFIED;
+}
 
+/*
+ */
+static struct cache *cache_do_one_devnode_unified(struct device_node *node, int level)
+{
 	pr_debug("creating L%d ucache for %s\n", level, node->full_name);
 
-	cache = new_cache(CACHE_TYPE_UNIFIED, level, node);
-
-	return cache;
+	return new_cache(cache_is_unified_d(node), level, node);
 }
 
 static struct cache *cache_do_one_devnode_split(struct device_node *node,
Signature · 1 line
1.7.10.4