On Wed, Sep 12, 2018 at 09:57:14AM -0600, Jeffrey Hugo wrote: > On 9/12/2018 9:38 AM, Sudeep Holla wrote: > > > > > >On 12/09/18 16:27, Sudeep Holla wrote: > >> > >> > >>On 12/09/18 15:41, Jeffrey Hugo wrote: > > > >[...] > > > >>> > >>>Correct. However, what if you have a NOCACHE (not architecturally > >>>specified), that is fully described in PPTT, as a non-unified cache > >>>(data only)? Unlikely? Maybe. Still seem possible though, therefore I > >>>feel this assumption is suspect. > >>> > >> > >>Yes, we have other issues if the architecturally not specified cache is > >>not unified irrespective of what PPTT says. So we may need to review and > >>see if that assumption is removed everywhere. > >> > >>Until then why can't a simple change fix the issue you have: > >> > >>-->8 > >> > >>diff --git i/drivers/acpi/pptt.c w/drivers/acpi/pptt.c > >>index d1e26cb599bf..f74131201f5e 100644 > >>--- i/drivers/acpi/pptt.c > >>+++ w/drivers/acpi/pptt.c > >>@@ -406,7 +406,8 @@ static void update_cache_properties(struct cacheinfo > >>*this_leaf, > >> * update the cache type as well. > >> */ > >> if (this_leaf->type == CACHE_TYPE_NOCACHE && > >>- valid_flags == PPTT_CHECKED_ATTRIBUTES) > >>+ (valid_flags == PPTT_CHECKED_ATTRIBUTES || > >>+ found_cache->flags & ACPI_PPTT_CACHE_TYPE_VALID)) > > > >Looking at this again, if we are supporting just presence of cache type > >and size(may be), then we can drop the whole valid_flags thing here. > > > >> this_leaf->type = CACHE_TYPE_UNIFIED; > >> } > >> > > Yes, this change fixes my usecase. I think it invalidates the comment, and > really, the comment should probably mention that we assume unified type > because there are other issues in supporting architecturally not specified > inst/data only caches. > Agreed. > Do you want a V2 with this? If so, do you want the fixes tag removed since > you seem to view this as not a bug? > Yes please, I am fine to retain fixes tag but that's my opinion. > I don't think I clearly understand the purpose of the valid flags, therefore > I feel as though I'm not sure if it can be dropped or not. Is it fair to > say that what the valid flags is accomplishing is identifying if we have a > sufficient level of information to support this cache? If not, then should > the cacheinfo driver not expose any sysfs information about the cache? > I don't see the use of the flag if we *have to* support the case where all the cache geometry is not present but just cache type (and maybe size?) is present. If that's the case we can drop valid flags entirely. I really don't like the idea of supporting that, but I don't have strong reasons to defend my idea, so I am fine with that. Further, I think in your case with NOCACHE type set, sysfs dir shouldn't have been created at the first place ideally. I think we need something like below to fix that. -->8 diff --git i/drivers/base/cacheinfo.c w/drivers/base/cacheinfo.c index 5d5b5988e88b..cf78fa6d470d 100644 --- i/drivers/base/cacheinfo.c +++ w/drivers/base/cacheinfo.c @@ -615,6 +615,8 @@ static int cache_add_dev(unsigned int cpu) this_leaf = this_cpu_ci->info_list + i; if (this_leaf->disable_sysfs) continue; + if (this_leaf->type == CACHE_TYPE_NOCACHE) + break; cache_groups = cache_get_attribute_groups(this_leaf); ci_dev = cpu_device_create(parent, this_leaf, cache_groups, "index%1u", i);
