From: Dave Olson <olson@cumulusnetworks•com>
To: Michael Ellerman <mpe@ellerman•id.au>
Cc: linuxppc-dev@lists•ozlabs.org
Subject: Re: [v3] Fix missing L2 cache size in /sys/devices/system/cpu
Date: Thu, 26 Mar 2015 12:49:14 -0700 [thread overview]
Message-ID: <20150326194914.GC23655@cumulusnetworks.com> (raw)
In-Reply-To: <20150326044927.EAD4D1400B7@ozlabs.org>
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
next prev parent reply other threads:[~2015-03-26 19:49 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-02-26 0:04 [PATCH v3] Fix missing L2 cache size in /sys/devices/system/cpu olson
2015-03-26 4:49 ` [v3] " Michael Ellerman
2015-03-26 19:49 ` Dave Olson [this message]
2015-03-26 23:39 ` Michael Ellerman
2015-04-03 3:33 ` [PATCH] " olson
2015-04-03 4:28 ` [PATCHv5 1/1] " olson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150326194914.GC23655@cumulusnetworks.com \
--to=olson@cumulusnetworks$(echo .)com \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
--cc=mpe@ellerman$(echo .)id.au \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox