On Thu, 2015-03-26 at 12:49 -0700, Dave Olson wrote:
> Michael Ellerman <m...@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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
