Message ID | 1424909087-2819-1-git-send-email-olson@cumulusnetworks.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Michael Ellerman |
Headers | show |
On Thu, 2015-26-02 at 00:04:47 UTC, Dave Olson wrote: > @@ -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; > } 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
Michael Ellerman <mpe@ellerman.id.au> wrote: > On Thu, 2015-26-02 at 00:04:47 UTC, Dave Olson wrote: > > @@ -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; > > } > > 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. > It would also be cleaner I think if you created another helper, eg. > cache_is_unified_d() to do the property lookup. OK. > 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 olson@cumulusnetworks.com
On Thu, 2015-03-26 at 12:49 -0700, Dave Olson wrote: > Michael Ellerman <mpe@ellerman.id.au> wrote: > > > On Thu, 2015-26-02 at 00:04:47 UTC, Dave Olson wrote: > > > @@ -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; > > > } > > > > 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. > > 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
diff --git a/arch/powerpc/kernel/cacheinfo.c b/arch/powerpc/kernel/cacheinfo.c index ae77b7e..860ed54 100644 --- a/arch/powerpc/kernel/cacheinfo.c +++ b/arch/powerpc/kernel/cacheinfo.c @@ -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; }