Hi Jeremy, On 2017/10/17 23:22, Jeremy Linton wrote: > Hi, > > On 10/17/2017 08:25 AM, Tomasz Nowicki wrote: >> Hi Jeremy, >> >> I did second round of review and have some more comments, please see below: >> >> On 12.10.2017 21:48, Jeremy Linton wrote: > I disagree, that routine is shared by the two code paths because its functionality is 99% duplicated between the two. The difference being whether it terminates the search at a given level, or continues searching until it runs out of nodes. The latter case is simply a degenerate version of the first. > > >> >> Also, seems like we can merge acpi_parse_pptt() & acpi_process_node(). > > That is true, but I fail to see how any of this is actually fixes anything. There are a million ways to do this, including as pointed out by building another data-structure to simplify the parsing what is a table that is less than ideal for runtime parsing (starting with the direction of the relative pointers, and ending with having to "infer" information that isn't directly flagged). I actually built a couple other versions of this, including a nice cute version which is about 1/8 this size of this and really easy to understand but of course is recursive... > > Maybe you can see my version below. It is similar to what you said above. It may give some help. https://github.com/fenghusthu/acpi_pptt Thanks Xiongfeng Wang >> >> Here are my suggestions: >> >> >> static struct acpi_pptt_cache *acpi_pptt_cache_type_level( >> struct acpi_table_header *table_hdr, >> struct acpi_subtable_header *res, >> int *local_level, >> int level, int type) >> { >> struct acpi_pptt_cache *cache = (struct acpi_pptt_cache *) res; >> >> if (res->type != ACPI_PPTT_TYPE_CACHE) >> return NULL; >> >> while (cache) { >> if ((*local_level == level) && >> (cache->flags & ACPI_PPTT_CACHE_TYPE_VALID) && >> ((cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) >> 2 == type)) { >> >> pr_debug("Found cache @ level %d\n", level); >> return cache; >> } >> cache = fetch_pptt_cache(table_hdr, cache->next_level_of_cache); >> (*local_level)++; >> } >> return NULL; >> } >> >> static struct acpi_pptt_cache *_acpi_find_cache_node( >> struct acpi_table_header *table_hdr, >> struct acpi_pptt_processor *cpu_node, >> int *local_level, int level, int type) >> { >> struct acpi_subtable_header *res; >> struct acpi_pptt_cache *cache_tmp, *cache = NULL; >> int resource = 0; >> >> /* walk down from the processor node */ >> while ((res = acpi_get_pptt_resource(table_hdr, cpu_node, resource))) { >> >> cache_tmp = acpi_pptt_cache_type_level(table_hdr, res, >> local_level, level, type); >> if (cache_tmp) { >> if (cache) >> pr_err("Found duplicate cache level/type unable to determine uniqueness\n"); >> >> cache = cache_tmp; >> } >> resource++; >> } >> return cache; >> } >> >> /* find the ACPI node describing the cache type/level for the given CPU */ >> static struct acpi_pptt_cache *acpi_find_cache_node( >> struct acpi_table_header *table_hdr, u32 acpi_cpu_id, >> enum cache_type type, unsigned int level, >> struct acpi_pptt_processor **node) >> { >> int total_levels = 0; >> struct acpi_pptt_cache *found = NULL; >> struct acpi_pptt_processor *cpu_node; >> u8 acpi_type = acpi_cache_type(type); >> >> pr_debug("Looking for CPU %d's level %d cache type %d\n", >> acpi_cpu_id, level, acpi_type); >> >> cpu_node = acpi_find_processor_node(table_hdr, acpi_cpu_id); >> if (!cpu_node) >> return NULL; >> >> do { >> found = _acpi_find_cache_node(table_hdr, cpu_node, >> &total_levels, level, acpi_type); >> *node = cpu_node; >> cpu_node = fetch_pptt_node(table_hdr, cpu_node->parent); >> } while ((cpu_node) && (!found)); >> >> return found; >> } >> >> static int acpi_pptt_cache_level(struct acpi_table_header *table_hdr, >> struct acpi_subtable_header *res) >> { >> struct acpi_pptt_cache *cache = (struct acpi_pptt_cache *) res; >> int local_level = 1; >> >> if (res->type != ACPI_PPTT_TYPE_CACHE) >> return 0; >> >> while ((cache = fetch_pptt_cache(table_hdr, cache->next_level_of_cache))) >> local_level++; >> return local_level; >> } >> >> static int _acpi_count_cache_level( >> struct acpi_table_header *table_hdr, >> struct acpi_pptt_processor *cpu_node) >> { >> struct acpi_subtable_header *res; >> int levels = 0, resource = 0, number_of_levels = 0; >> >> /* walk down from the processor node */ >> while ((res = acpi_get_pptt_resource(table_hdr, cpu_node, resource))) { >> levels = acpi_pptt_cache_level(table_hdr, res); >> >> /* >> * we are looking for the max depth. Since its potentially >> * possible for a given node to have resources with differing >> * depths verify that the depth we have found is the largest. >> */ >> if (levels > number_of_levels) >> number_of_levels = levels; >> >> resource++; >> } >> return number_of_levels; >> } >> >> static int acpi_count_cache_level(struct acpi_table_header *table_hdr, >> u32 acpi_cpu_id) >> { >> int total_levels = 0; >> struct acpi_pptt_processor *cpu_node; >> >> cpu_node = acpi_find_processor_node(table_hdr, acpi_cpu_id); >> while (cpu_node) { >> total_levels += _acpi_count_cache_level(table_hdr, cpu_node); >> cpu_node = fetch_pptt_node(table_hdr, cpu_node->parent); >> } >> >> return total_levels; >> } >> >> >> Did not compile the code so I may have missed somthing. >> >> Thanks, >> Tomasz > > > . > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
