Message ID | 20140324153611.GA16245@red-moon |
---|---|
State | New |
Headers | show |
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?
On Mon, Mar 24, 2014 at 03:45:50PM +0000, Mark Brown wrote: > 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? of_parse_phandle() does, am I right ? > > 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. cpu_node has to be put, since it is obtained through of_parse_phandle(). > > @@ -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. Gah, right. I will let you fix it please as you deem fit. > Of course all this refcounting does absolutely nothing for FDT but hey > ho. We comply with the interface, so that at least from an API standpoint code is complete. I understand your point. > > @@ -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? It makes it tidier, but that's just cosmetic and my opinion (but thanks for having a look since it is not a significant change). Again, squash it in as you deem fit. Thanks ! Lorenzo
On Mon, Mar 24, 2014 at 04:02:36PM +0000, Lorenzo Pieralisi wrote: > On Mon, Mar 24, 2014 at 03:45:50PM +0000, Mark Brown wrote: > > of_get_cpu_node() doesn't visibly take a reference and isn't documented > > as doing such? > of_parse_phandle() does, am I right ? Oh, yes. I was looking at the wrong thing.
From 134718e495a8d989c84dafe27d1b56b3c2e029c3 Mon Sep 17 00:00:00 2001 From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Date: Mon, 24 Mar 2014 14:52:24 +0000 Subject: [PATCH 2/2] arm64: kernel: topology updates Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> --- arch/arm64/kernel/topology.c | 33 ++++++++++++++++++++------------- 1 files changed, 20 insertions(+), 13 deletions(-) diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c index 0e2cfeb..60f5f47 100644 --- a/arch/arm64/kernel/topology.c +++ b/arch/arm64/kernel/topology.c @@ -31,12 +31,15 @@ static int __init get_cpu_for_node(struct device_node *node) if (!cpu_node) return -1; - 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; - } + } pr_crit("Unable to find CPU node for %s\n", cpu_node->full_name); + + of_node_put(cpu_node); return -1; } @@ -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); } i++; } while (t); @@ -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++; } while (c); @@ -124,20 +127,24 @@ static int __init parse_cluster(struct device_node *cluster, int depth) if (c) { has_cores = true; - if (depth == 0) + if (depth == 0) { pr_err("%s: cpu-map children should be clusters\n", c->full_name); + of_node_put(c); + return -EINVAL; + } if (leaf) { ret = parse_core(c, cluster_id, core_id++); - if (ret != 0) - return ret; } else { pr_err("%s: Non-leaf cluster with core %s\n", cluster->full_name, name); - return -EINVAL; + ret = -EINVAL; } + of_node_put(c); + if (ret != 0) + return ret; } i++; } while (c); @@ -173,9 +180,7 @@ static int __init parse_dt_topology(void) ret = parse_cluster(map, 0); if (ret != 0) - goto out; - - of_node_put(map); + goto out_map; /* * Check that all cores are in the topology; the SMP code will @@ -189,6 +194,8 @@ static int __init parse_dt_topology(void) } } +out_map: + of_node_put(map); out: of_node_put(cn); return ret; -- 1.7.5.4