[2/4] arm64: topology: Add support for topology DT bindings

Hi Mark,

On Fri, Mar 21, 2014 at 05:27:59PM +0000, Mark Brown wrote:
> From: Mark Brown <broonie@linaro.org>
> 
> Add support for parsing the explicit topology bindings to discover the
> topology of the system.
> 
> Since it is not currently clear how to map multi-level clusters for the
> scheduler all leaf clusters are presented to the scheduler at the same
> level. This should be enough to provide good support for current systems.
> 
> Signed-off-by: Mark Brown <broonie@linaro.org>

Please apply the patches attached, that fix some stale comments and
add of_node_put in paths that could otherwise be broken by return
statements.

Patch "arm64: kernel: topology: removed stale comment" applies to patch
1 of this series.

Patch "arm64: kernel: topology updates" applies to patch 2 of this
series.

Both should be squashed into your original patches. Please have a final
look and test them, they work for me.

Having said that, on the updated patch 1 and 2 of this series:

Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

> ---
> 
>  - Discard the DT data if any CPUs are omitted from the topology.
>  - Call of_node_put() where required.
> 
>  arch/arm64/kernel/topology.c | 192 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 186 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> index ff662b23af5f..d0cb687fb7f5 100644
> --- a/arch/arm64/kernel/topology.c
> +++ b/arch/arm64/kernel/topology.c
> @@ -17,10 +17,183 @@
>  #include <linux/percpu.h>
>  #include <linux/node.h>
>  #include <linux/nodemask.h>
> +#include <linux/of.h>
>  #include <linux/sched.h>
>  
>  #include <asm/topology.h>
>  
> +static int __init get_cpu_for_node(struct device_node *node)
> +{
> +	struct device_node *cpu_node;
> +	int cpu;
> +
> +	cpu_node = of_parse_phandle(node, "cpu", 0);
> +	if (!cpu_node)
> +		return -1;
> +
> +	for_each_possible_cpu(cpu) {
> +		if (of_get_cpu_node(cpu, NULL) == cpu_node)
> +			return cpu;
> +	}
> +
> +	pr_crit("Unable to find CPU node for %s\n", cpu_node->full_name);
> +	return -1;
> +}
> +
> +static int __init parse_core(struct device_node *core, int cluster_id,
> +			     int core_id)
> +{
> +	char name[10];
> +	bool leaf = true;
> +	int i = 0;
> +	int cpu;
> +	struct device_node *t;
> +
> +	do {
> +		snprintf(name, sizeof(name), "thread%d", i);
> +		t = of_get_child_by_name(core, name);
> +		if (t) {
> +			leaf = false;
> +			cpu = get_cpu_for_node(t);
> +			if (cpu >= 0) {
> +				cpu_topology[cpu].cluster_id = cluster_id;
> +				cpu_topology[cpu].core_id = core_id;
> +				cpu_topology[cpu].thread_id = i;
> +			} else {
> +				pr_err("%s: Can't get CPU for thread\n",
> +				       t->full_name);
> +				return -EINVAL;
> +			}
> +			of_node_put(t);
> +		}
> +		i++;
> +	} while (t);
> +
> +	cpu = get_cpu_for_node(core);
> +	if (cpu >= 0) {
> +		if (!leaf) {
> +			pr_err("%s: Core has both threads and CPU\n",
> +			       core->full_name);
> +			return -EINVAL;
> +		}
> +
> +		cpu_topology[cpu].cluster_id = cluster_id;
> +		cpu_topology[cpu].core_id = core_id;
> +	} else if (leaf) {
> +		pr_err("%s: Can't get CPU for leaf core\n", core->full_name);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int __init parse_cluster(struct device_node *cluster, int depth)
> +{
> +	char name[10];
> +	bool leaf = true;
> +	bool has_cores = false;
> +	struct device_node *c;
> +	static int cluster_id __initdata;
> +	int core_id = 0;
> +	int i, ret;
> +
> +	/*
> +	 * First check for child clusters; we currently ignore any
> +	 * information about the nesting of clusters and present the
> +	 * scheduler with a flat list of them.
> +	 */
> +	i = 0;
> +	do {
> +		snprintf(name, sizeof(name), "cluster%d", i);
> +		c = of_get_child_by_name(cluster, name);
> +		if (c) {
> +			ret = parse_cluster(c, depth + 1);
> +			if (ret != 0)
> +				return ret;
> +			leaf = false;
> +			of_node_put(c);
> +		}
> +		i++;
> +	} while (c);
> +
> +	/* Now check for cores */
> +	i = 0;
> +	do {
> +		snprintf(name, sizeof(name), "core%d", i);
> +		c = of_get_child_by_name(cluster, name);
> +		if (c) {
> +			has_cores = true;
> +
> +			if (depth == 0)
> +				pr_err("%s: cpu-map children should be clusters\n",
> +				       c->full_name);
> +
> +			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;
> +			}
> +			of_node_put(c);
> +		}
> +		i++;
> +	} while (c);
> +
> +	if (leaf && !has_cores)
> +		pr_warn("%s: empty cluster\n", cluster->full_name);
> +
> +	if (leaf)
> +		cluster_id++;
> +
> +	return 0;
> +}
> +
> +static int __init parse_dt_topology(void)
> +{
> +	struct device_node *cn, *map;
> +	int ret = 0;
> +	int cpu;
> +
> +	cn = of_find_node_by_path("/cpus");
> +	if (!cn) {
> +		pr_err("No CPU information found in DT\n");
> +		return 0;
> +	}
> +
> +	/*
> +	 * When topology is provided cpu-map is essentially a root
> +	 * cluster with restricted subnodes.
> +	 */
> +	map = of_get_child_by_name(cn, "cpu-map");
> +	if (!map)
> +		goto out;
> +
> +	ret = parse_cluster(map, 0);
> +	if (ret != 0)
> +		goto out;
> +
> +	of_node_put(map);
> +
> +	/*
> +	 * Check that all cores are in the topology; the SMP code will
> +	 * only mark cores described in the DT as possible.
> +	 */
> +	for_each_possible_cpu(cpu) {
> +		if (cpu_topology[cpu].cluster_id == -1) {
> +			pr_err("CPU%d: No topology information specified\n",
> +			       cpu);
> +			ret = -EINVAL;
> +		}
> +	}
> +
> +out:
> +	of_node_put(cn);
> +	return ret;
> +}
> +
>  /*
>   * cpu topology table
>   */
> @@ -71,15 +244,10 @@ void store_cpu_topology(unsigned int cpuid)
>  	update_siblings_masks(cpuid);
>  }
>  
> -/*
> - * init_cpu_topology is called at boot when only one cpu is running
> - * which prevent simultaneous write access to cpu_topology array
> - */
> -void __init init_cpu_topology(void)
> +static void __init reset_cpu_topology(void)
>  {
>  	unsigned int cpu;
>  
> -	/* init core mask and power*/
>  	for_each_possible_cpu(cpu) {
>  		struct cpu_topology *cpu_topo = &cpu_topology[cpu];
>  
> @@ -93,3 +261,15 @@ void __init init_cpu_topology(void)
>  		cpumask_set_cpu(cpu, &cpu_topo->thread_sibling);
>  	}
>  }
> +
> +void __init init_cpu_topology(void)
> +{
> +	reset_cpu_topology();
> +
> +	/*
> +	 * Discard anything that was parsed if we hit an error so we
> +	 * don't use partial information.
> +	 */
> +	if (parse_dt_topology())
> +		reset_cpu_topology();
> +}
> -- 
> 1.9.1
> 
>

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.