Sort unit addresses by number

Sort unit addresses by number

[Anton Blanchard]

The sort option in dtc treats unit addresses as strings. This causes
cpu nodes to end up out of order:

# dtc -s -I fs -O dts /proc/proc/device-tree | grep PowerPC,POWER7

		PowerPC,POWER7@30 {
		PowerPC,POWER7@68 {
		PowerPC,POWER7@70 {
		PowerPC,POWER7@828 {
		PowerPC,POWER7@860 {
		PowerPC,POWER7@868 {
		PowerPC,POWER7@8a0 {
		PowerPC,POWER7@8b0 {
		PowerPC,POWER7@8f0 {
		PowerPC,POWER7@a0 {
		PowerPC,POWER7@a8 {
		PowerPC,POWER7@e0 {

If we use this device tree for a kexec boot we end up with a confusing
layout of logical CPUs:

node 0 cpus: 0-23 72-95
node 0 size: 32633 MB

node 1 cpus: 24-71
node 1 size: 32631 MB

The reason for this is that we allocate logical CPU ids as we walk
through the device tree.

In cmp_subnode, if both nodes have a hex unit address and the
basenames match, then compare by number.

This fixes the issue:

# dtc -s -I fs -O dts /proc/proc/device-tree | grep PowerPC,POWER7
		PowerPC,POWER7@30 {
		PowerPC,POWER7@68 {
		PowerPC,POWER7@70 {
		PowerPC,POWER7@a0 {
		PowerPC,POWER7@a8 {
		PowerPC,POWER7@e0 {
		PowerPC,POWER7@828 {
		PowerPC,POWER7@860 {
		PowerPC,POWER7@868 {
		PowerPC,POWER7@8a0 {
		PowerPC,POWER7@8b0 {
		PowerPC,POWER7@8f0 {

And the CPU layout is as expected:

node 0 cpus: 0-47
node 0 size: 32633 MB

node 1 cpus: 48-95
node 1 size: 32631 MB

Signed-off-by: Anton Blanchard <anton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
--

Index: b/livetree.c
===================================================================
--- a/livetree.c
+++ b/livetree.c
@@ -658,12 +658,38 @@ static void sort_properties(struct node
 	free(tbl);
 }
 
+static bool is_hex(const char *str)
+{
+	while (*str) {
+		if (!isxdigit(*str++))
+			return false;
+	}
+
+	return true;
+}
+
 static int cmp_subnode(const void *ax, const void *bx)
 {
-	const struct node *a, *b;
+	struct node *a, *b;
+	const char *a_unit, *b_unit;
+
+	a = *((struct node * const *)ax);
+	b = *((struct node * const *)bx);
+
+	a_unit = get_unitname(a);
+	b_unit = get_unitname(b);
+
+	/* Sort hex unit addresses by number */
+	if (a_unit && b_unit && (a->basenamelen == b->basenamelen) &&
+	    !strncmp(a->name, b->name, a->basenamelen) &&
+	    is_hex(a_unit) && is_hex(b_unit)) {
+		unsigned long long a_num, b_num;
+
+		a_num = strtoull(a_unit, NULL, 16);
+		b_num = strtoull(b_unit, NULL, 16);
 
-	a = *((const struct node * const *)ax);
-	b = *((const struct node * const *)bx);
+		return (a_num > b_num) - (a_num < b_num);
+	}
 
 	return strcmp(a->name, b->name);
 }

Re: Sort unit addresses by number

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2828 bytes --]

On Wed, Aug 12, 2015 at 12:00:04PM +1000, Anton Blanchard wrote:
> The sort option in dtc treats unit addresses as strings. This causes
> cpu nodes to end up out of order:
> 
> # dtc -s -I fs -O dts /proc/proc/device-tree | grep PowerPC,POWER7
> 
> 		PowerPC,POWER7@30 {
> 		PowerPC,POWER7@68 {
> 		PowerPC,POWER7@70 {
> 		PowerPC,POWER7@828 {
> 		PowerPC,POWER7@860 {
> 		PowerPC,POWER7@868 {
> 		PowerPC,POWER7@8a0 {
> 		PowerPC,POWER7@8b0 {
> 		PowerPC,POWER7@8f0 {
> 		PowerPC,POWER7@a0 {
> 		PowerPC,POWER7@a8 {
> 		PowerPC,POWER7@e0 {
> 
> If we use this device tree for a kexec boot we end up with a confusing
> layout of logical CPUs:
> 
> node 0 cpus: 0-23 72-95
> node 0 size: 32633 MB
> 
> node 1 cpus: 24-71
> node 1 size: 32631 MB
> 
> The reason for this is that we allocate logical CPU ids as we walk
> through the device tree.
> 
> In cmp_subnode, if both nodes have a hex unit address and the
> basenames match, then compare by number.
> 
> This fixes the issue:
> 
> # dtc -s -I fs -O dts /proc/proc/device-tree | grep PowerPC,POWER7
> 		PowerPC,POWER7@30 {
> 		PowerPC,POWER7@68 {
> 		PowerPC,POWER7@70 {
> 		PowerPC,POWER7@a0 {
> 		PowerPC,POWER7@a8 {
> 		PowerPC,POWER7@e0 {
> 		PowerPC,POWER7@828 {
> 		PowerPC,POWER7@860 {
> 		PowerPC,POWER7@868 {
> 		PowerPC,POWER7@8a0 {
> 		PowerPC,POWER7@8b0 {
> 		PowerPC,POWER7@8f0 {
> 
> And the CPU layout is as expected:
> 
> node 0 cpus: 0-47
> node 0 size: 32633 MB
> 
> node 1 cpus: 48-95
> node 1 size: 32631 MB
> 
> Signed-off-by: Anton Blanchard <anton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

So.. I agree the current ordering is rather nasty, but changing it
like this is problematic.

The idea of dtc -s is that it will produce the output in a "canonical"
form so that tools like dtdiff can examine without having to care
about order changes.  So, first problem is that changing the -s
behaviour means that the "canonical" form from an old dtc won't be the
same as from a new dtc which kind of defeats the purpose.

Again, in principle, ordering by unit address makes more sense than
ordering by the whole unit name, but it adds further complications.
We could just strcmp() the unit address, but that would lead to the
same ordering oddities you've seen.  We can order assuming it's hex,
but then we'll get strange behaviour on pci or other buses that can
have funny characters in their unit addresses.

So, yeah..

At the very least I'd like to see a change like this activated by a
new option.  Longer term I'm not sure how to proceed sensibly with
improving the ordering.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]