On Mon, Mar 24, 2014 at 03:36:11PM +0000, Lorenzo Pieralisi wrote:
- for_each_possible_cpu(cpu) {
if (of_get_cpu_node(cpu, NULL) == cpu_node)
- for_each_possible_cpu(cpu)
if (of_get_cpu_node(cpu, NULL) == cpu_node) {
of_node_put(cpu_node); return cpu;
- }
}
of_get_cpu_node() doesn't visibly take a reference and isn't documented as doing such?
pr_crit("Unable to find CPU node for %s\n", cpu_node->full_name);
- of_node_put(cpu_node);
I don't understand this one - the caller passed in cpu_node, we didn't acquire a reference here so I'd expect the caller to be doing any frees that are needed.
@@ -55,6 +58,7 @@ static int __init parse_core(struct device_node *core, int cluster_id, if (t) { leaf = false; cpu = get_cpu_for_node(t);
of_node_put(t); if (cpu >= 0) { cpu_topology[cpu].cluster_id = cluster_id; cpu_topology[cpu].core_id = core_id;
@@ -64,7 +68,6 @@ static int __init parse_core(struct device_node *core, int cluster_id, t->full_name); return -EINVAL; }
of_node_put(t);
This causes us to reference the just released t when displaying the error message in the error case (t->full_name in the quoted context). You're right that the reference is leaked in the error path but the free needs to be inside the error handling case.
Of course all this refcounting does absolutely nothing for FDT but hey ho.
@@ -107,11 +110,11 @@ static int __init parse_cluster(struct device_node *cluster, int depth) snprintf(name, sizeof(name), "cluster%d", i); c = of_get_child_by_name(cluster, name); if (c) {
leaf = false; ret = parse_cluster(c, depth + 1);
of_node_put(c); if (ret != 0) return ret;
leaf = false;
of_node_put(c);
I don't understand why you moved the assignment of leaf?