[PATCH 1/3] of/numa: remove a duplicated pr_debug information

This information will be printed in the subfunction numa_add_memblk.
They are not the same, but very similar.

Signed-off-by: Zhen Lei <redacted>
---
 drivers/of/of_numa.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/of/of_numa.c b/drivers/of/of_numa.c
index 0f2784b..21d831f 100644
--- a/drivers/of/of_numa.c
+++ b/drivers/of/of_numa.c

@@ -88,9 +88,6 @@ static int __init of_numa_parse_memory_nodes(void)
 			break;
 		}

-		pr_debug("NUMA:  base = %llx len = %llx, node = %u\n",
-			 rsrc.start, rsrc.end - rsrc.start + 1, nid);
-
 		r = numa_add_memblk(nid, rsrc.start,
 				    rsrc.end - rsrc.start + 1);
 		if (r)

For a normal memory@ devicetree node, its reg property can contains more
memory blocks.

Because we don't known how many memory blocks maybe contained, so we try
from index=0, increase 1 until error returned(the end).

Signed-off-by: Zhen Lei <redacted>
---
 drivers/of/of_numa.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/of/of_numa.c b/drivers/of/of_numa.c
index 21d831f..2c5f249 100644
--- a/drivers/of/of_numa.c
+++ b/drivers/of/of_numa.c

@@ -63,7 +63,7 @@ static int __init of_numa_parse_memory_nodes(void)
 	struct device_node *np = NULL;
 	struct resource rsrc;
 	u32 nid;
-	int r = 0;
+	int i, r = 0;

 	for (;;) {
 		np = of_find_node_by_type(np, "memory");

@@ -82,17 +82,27 @@ static int __init of_numa_parse_memory_nodes(void)
 			/* some other error */
 			break;

-		r = of_address_to_resource(np, 0, &rsrc);
-		if (r) {
-			pr_err("NUMA: bad reg property in memory node\n");
-			break;
+		for (i = 0; ; i++) {
+			r = of_address_to_resource(np, i, &rsrc);
+			if (r) {
+				/* reached the end of of_address */
+				if (i > 0) {
+					r = 0;
+					break;
+				}
+
+				pr_err("NUMA: bad reg property in memory node\n");
+				goto finished;
+			}
+
+			r = numa_add_memblk(nid, rsrc.start,
+					    rsrc.end - rsrc.start + 1);
+			if (r)
+				goto finished;
 		}
-
-		r = numa_add_memblk(nid, rsrc.start,
-				    rsrc.end - rsrc.start + 1);
-		if (r)
-			break;
 	}
+
+finished:
 	of_node_put(np);

 	return r;

numa_init(of_numa_init) may returned error because of numa configuration
error. So "No NUMA configuration found" is inaccurate. In fact, specific
configuration error information can be immediately printed by the
testing branch. So "No NUMA..." only needs to be printed when numa_off.

Signed-off-by: Zhen Lei <redacted>
---
 arch/arm64/mm/numa.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/mm/numa.c b/arch/arm64/mm/numa.c
index 98dc104..9937cc1 100644
--- a/arch/arm64/mm/numa.c
+++ b/arch/arm64/mm/numa.c

@@ -362,7 +362,8 @@ static int __init dummy_numa_init(void)
 	int ret;
 	struct memblock_region *mblk;

-	pr_info("%s\n", "No NUMA configuration found");
+	if (numa_off)
+		pr_info("%s\n", "No NUMA configuration found");
 	pr_info("NUMA: Faking a node at [mem %#018Lx-%#018Lx]\n",
 	       0LLU, PFN_PHYS(max_pfn) - 1);

--

2.5.0


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, May 26, 2016 at 10:43:58AM +0800, Zhen Lei wrote:

This function can be simplified down to:

	for_each_node_by_type(np, "memory") {
		r = of_property_read_u32(np, "numa-node-id", &nid);
		if (r == -EINVAL)
			/*
			 * property doesn't exist if -EINVAL, continue
			 * looking for more memory nodes with
			 * "numa-node-id" property
			 */
			continue;
		else if (r)
			/* some other error */
			break;

		r = of_address_to_resource(np, 0, &rsrc);
		for (i = 0; !r; i++, r = of_address_to_resource(np, i, 
&rsrc)) {
			r = numa_add_memblk(nid, rsrc.start,
				    rsrc.end - rsrc.start + 1);
		}
	}
	of_node_put(np);

	return r;


Perhaps with a "if (!i && r) pr_err()" for an error message at the end.

Rob

On Wed, May 25, 2016 at 7:43 PM, Zhen Lei [off-list ref] wrote:

IIRC, it should be
if (!numa_off)
we want to print this message when we failed to find proper numa configuration.
when numa_off is set, we will not look for any numa configuration.

+               pr_info("%s\n", "No NUMA configuration found");
        pr_info("NUMA: Faking a node at [mem %#018Lx-%#018Lx]\n",
               0LLU, PFN_PHYS(max_pfn) - 1);

--
2.5.0

ganapat

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Thu, 2016-05-26 at 09:22 -0700, Ganapatrao Kulkarni wrote:

On Wed, May 25, 2016 at 7:43 PM, Zhen Lei [off-list ref] wrote:

quoted

numa_init(of_numa_init) may returned error because of numa configuration
error. So "No NUMA configuration found" is inaccurate. In fact, specific
configuration error information can be immediately printed by the
testing branch. So "No NUMA..." only needs to be printed when numa_off.

[]

quoted

diff --git a/arch/arm64/mm/numa.c b/arch/arm64/mm/numa.c

[]

quoted

@@ -362,7 +362,8 @@ static int __init dummy_numa_init(void)

        int ret;
        struct memblock_region *mblk;

-       pr_info("%s\n", "No NUMA configuration found");
+       if (numa_off)

IIRC, it should be
if (!numa_off)
we want to print this message when we failed to find proper numa configuration.
when numa_off is set, we will not look for any numa configuration.

quoted

+               pr_info("%s\n", "No NUMA configuration found");

trivia:

Using printk("%s\n", "string") just makes the object code larger
for no particular benefit.

	pr_info("No NUMA configuration found\n");

would be smaller, faster and more intelligible for humans too.

Maybe something like this:
---
 arch/arm64/mm/numa.c | 41 ++++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/arch/arm64/mm/numa.c b/arch/arm64/mm/numa.c
index 98dc104..2042452 100644
--- a/arch/arm64/mm/numa.c
+++ b/arch/arm64/mm/numa.c

@@ -17,6 +17,8 @@

  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#define pr_fmt(fmt) "NUMA: " fmt
+
 #include <linux/bootmem.h>
 #include <linux/memblock.h>
 #include <linux/module.h>

@@ -36,7 +38,7 @@ static __init int numa_parse_early_param(char *opt)

 	if (!opt)
 		return -EINVAL;
 	if (!strncmp(opt, "off", 3)) {
-		pr_info("%s\n", "NUMA turned off");
+		pr_info("NUMA turned off\n");
 		numa_off = 1;
 	}
 	return 0;

@@ -107,7 +109,7 @@ static void __init setup_node_to_cpumask_map(void)

 		set_cpu_numa_node(cpu, NUMA_NO_NODE);
 
 	/* cpumask_of_node() will now work */
-	pr_debug("NUMA: Node to cpumask map for %d nodes\n", nr_node_ids);
+	pr_debug("Node to cpumask map for %d nodes\n", nr_node_ids);
 }
 
 /*

@@ -142,14 +144,14 @@ int __init numa_add_memblk(int nid, u64 start, u64 size)

 
 	ret = memblock_set_node(start, size, &memblock.memory, nid);
 	if (ret < 0) {
-		pr_err("NUMA: memblock [0x%llx - 0x%llx] failed to add on node %d\n",
-			start, (start + size - 1), nid);
+		pr_err("memblock [0x%llx - 0x%llx] failed to add on node %d\n",
+		       start, (start + size - 1), nid);
 		return ret;
 	}
 
 	node_set(nid, numa_nodes_parsed);
-	pr_info("NUMA: Adding memblock [0x%llx - 0x%llx] on node %d\n",
-			start, (start + size - 1), nid);
+	pr_info("Adding memblock [0x%llx - 0x%llx] on node %d\n",
+		start, (start + size - 1), nid);
 	return ret;
 }
 

@@ -163,19 +165,18 @@ static void __init setup_node_data(int nid, u64 start_pfn, u64 end_pfn)

 	void *nd;
 	int tnid;
 
-	pr_info("NUMA: Initmem setup node %d [mem %#010Lx-%#010Lx]\n",
-			nid, start_pfn << PAGE_SHIFT,
-			(end_pfn << PAGE_SHIFT) - 1);
+	pr_info("Initmem setup node %d [mem %#010Lx-%#010Lx]\n",
+		nid, start_pfn << PAGE_SHIFT, (end_pfn << PAGE_SHIFT) - 1);
 
 	nd_pa = memblock_alloc_try_nid(nd_size, SMP_CACHE_BYTES, nid);
 	nd = __va(nd_pa);
 
 	/* report and initialize */
-	pr_info("NUMA: NODE_DATA [mem %#010Lx-%#010Lx]\n",
+	pr_info("NODE_DATA [mem %#010Lx-%#010Lx]\n",
 		nd_pa, nd_pa + nd_size - 1);
 	tnid = early_pfn_to_nid(nd_pa >> PAGE_SHIFT);
 	if (tnid != nid)
-		pr_info("NUMA: NODE_DATA(%d) on node %d\n", nid, tnid);
+		pr_info("NODE_DATA(%d) on node %d\n", nid, tnid);
 
 	node_data[nid] = nd;
 	memset(NODE_DATA(nid), 0, sizeof(pg_data_t));

@@ -232,8 +233,7 @@ static int __init numa_alloc_distance(void)

 			numa_distance[i * numa_distance_cnt + j] = i == j ?
 				LOCAL_DISTANCE : REMOTE_DISTANCE;
 
-	pr_debug("NUMA: Initialized distance table, cnt=%d\n",
-			numa_distance_cnt);
+	pr_debug("Initialized distance table, cnt=%d\n", numa_distance_cnt);
 
 	return 0;
 }

@@ -254,20 +254,20 @@ static int __init numa_alloc_distance(void)

 void __init numa_set_distance(int from, int to, int distance)
 {
 	if (!numa_distance) {
-		pr_warn_once("NUMA: Warning: distance table not allocated yet\n");
+		pr_warn_once("Warning: distance table not allocated yet\n");
 		return;
 	}
 
 	if (from >= numa_distance_cnt || to >= numa_distance_cnt ||
 			from < 0 || to < 0) {
-		pr_warn_once("NUMA: Warning: node ids are out of bound, from=%d to=%d distance=%d\n",
-			    from, to, distance);
+		pr_warn_once("Warning: node ids are out of bound, from=%d to=%d distance=%d\n",
+			     from, to, distance);
 		return;
 	}
 
 	if ((u8)distance != distance ||
 	    (from == to && distance != LOCAL_DISTANCE)) {
-		pr_warn_once("NUMA: Warning: invalid distance parameter, from=%d to=%d distance=%d\n",
+		pr_warn_once("Warning: invalid distance parameter, from=%d to=%d distance=%d\n",
 			     from, to, distance);
 		return;
 	}

@@ -294,7 +294,7 @@ static int __init numa_register_nodes(void)

 	/* Check that valid nid is set to memblks */
 	for_each_memblock(memory, mblk)
 		if (mblk->nid == NUMA_NO_NODE || mblk->nid >= MAX_NUMNODES) {
-			pr_warn("NUMA: Warning: invalid memblk node %d [mem %#010Lx-%#010Lx]\n",
+			pr_warn("Warning: invalid memblk node %d [mem %#010Lx-%#010Lx]\n",
 				mblk->nid, mblk->base,
 				mblk->base + mblk->size - 1);
 			return -EINVAL;

@@ -362,9 +362,8 @@ static int __init dummy_numa_init(void)

 	int ret;
 	struct memblock_region *mblk;
 
-	pr_info("%s\n", "No NUMA configuration found");
-	pr_info("NUMA: Faking a node at [mem %#018Lx-%#018Lx]\n",
-	       0LLU, PFN_PHYS(max_pfn) - 1);
+	pr_info("No NUMA configuration found - faking a node at [mem %#018Lx-%#018Lx]\n",
+		0LLU, PFN_PHYS(max_pfn) - 1);
 
 	for_each_memblock(memory, mblk) {
 		ret = numa_add_memblk(0, mblk->base, mblk->size);

The current patch to correct this problem is here:

https://lkml.org/lkml/2016/5/24/679

Since v7 of the ACPI/NUMA patches are likely going to be added to 
linux-next as soon as the current merge window ends, further 
simplifications of the informational prints should probably be rebased 
on top of it.

David Daney

On 05/26/2016 09:35 AM, Joe Perches wrote:

On 2016/5/27 1:12, David Daney wrote:

The current patch to correct this problem is here:

https://lkml.org/lkml/2016/5/24/679

Since v7 of the ACPI/NUMA patches are likely going to be added to linux-next as soon as the current merge window ends, further simplifications of the informational prints should probably be rebased on top of it.

David Daney

quoted

On Thu, 2016-05-26 at 09:22 -0700, Ganapatrao Kulkarni wrote:

quoted

IIRC, it should be
if (!numa_off)
we want to print this message when we failed to find proper numa configuration.
when numa_off is set, we will not look for any numa configuration.

quoted

+               pr_info("%s\n", "No NUMA configuration found");

OK, I think I also missed some cases.

But my problem still have not been resolved by "https://lkml.org/lkml/2016/5/24/679", see below. I will update my patches base on it.


[    0.000000] NUMA: Adding memblock [0x0 - 0x6affffff] on node 0
[    0.000000] NUMA: parsing numa-distance-map-v1
[    0.000000] NUMA: Warning: invalid memblk node 4 [mem 0x6b000000-0x7fbfffff]		//My numa configuration is incorrect, but not "No ... found"
[    0.000000] No NUMA configuration found						//Above warning is very detail, this can be removed
[    0.000000] NUMA: Faking a node at [mem 0x0000000000000000-0x00000017ffffffff]

On 2016/5/26 21:13, Rob Herring wrote:

On Thu, May 26, 2016 at 10:43:58AM +0800, Zhen Lei wrote:

quoted

For a normal memory@ devicetree node, its reg property can contains more
memory blocks.

Because we don't known how many memory blocks maybe contained, so we try
from index=0, increase 1 until error returned(the end).

Signed-off-by: Zhen Lei <redacted>
---
 drivers/of/of_numa.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/of/of_numa.c b/drivers/of/of_numa.c
index 21d831f..2c5f249 100644
--- a/drivers/of/of_numa.c
+++ b/drivers/of/of_numa.c

@@ -63,7 +63,7 @@ static int __init of_numa_parse_memory_nodes(void)
 	struct device_node *np = NULL;
 	struct resource rsrc;
 	u32 nid;
-	int r = 0;
+	int i, r = 0;

 	for (;;) {
 		np = of_find_node_by_type(np, "memory");

@@ -82,17 +82,27 @@ static int __init of_numa_parse_memory_nodes(void)
 			/* some other error */
 			break;

-		r = of_address_to_resource(np, 0, &rsrc);
-		if (r) {
-			pr_err("NUMA: bad reg property in memory node\n");
-			break;
+		for (i = 0; ; i++) {
+			r = of_address_to_resource(np, i, &rsrc);
+			if (r) {
+				/* reached the end of of_address */
+				if (i > 0) {
+					r = 0;
+					break;
+				}
+
+				pr_err("NUMA: bad reg property in memory node\n");
+				goto finished;
+			}
+
+			r = numa_add_memblk(nid, rsrc.start,
+					    rsrc.end - rsrc.start + 1);
+			if (r)
+				goto finished;
 		}
-
-		r = numa_add_memblk(nid, rsrc.start,
-				    rsrc.end - rsrc.start + 1);
-		if (r)
-			break;
 	}
+
+finished:
 	of_node_put(np);

This function can be simplified down to:

	for_each_node_by_type(np, "memory") {

OK, That's good.

		r = of_property_read_u32(np, "numa-node-id", &nid);
		if (r == -EINVAL)
			/*
			 * property doesn't exist if -EINVAL, continue
			 * looking for more memory nodes with
			 * "numa-node-id" property
			 */
			continue;

Hi, everybody:
    If some "memory" node contains "numa-node-id", but some others missed. Can we simply ignored it?
I think we should break out too, and faking to only have node0.

		else if (r)
			/* some other error */
			break;

		r = of_address_to_resource(np, 0, &rsrc);
		for (i = 0; !r; i++, r = of_address_to_resource(np, i, 

But r(non-zero) is just break this loop, the original is break the outer for (;;) loop

How about as below?

	for_each_node_by_type(np, "memory") {
		... ...

		for (i = 0; !of_address_to_resource(np, i, &rsrc); i++) {
                        r = numa_add_memblk(nid, rsrc.start,
                                            rsrc.end - rsrc.start + 1);
                        if (r)
                                goto finished;
                }

		if (!i)
			pr_err("NUMA: bad reg property in memory node\n");
	}

finished:
	

&rsrc)) {
			r = numa_add_memblk(nid, rsrc.start,
				    rsrc.end - rsrc.start + 1);
		}
	}
	of_node_put(np);

	return r;


Perhaps with a "if (!i && r) pr_err()" for an error message at the end.

Rob

.

On Thu, May 26, 2016 at 10:36 PM, Leizhen (ThunderTown)
[off-list ref] wrote:

On 2016/5/26 21:13, Rob Herring wrote:

quoted

On Thu, May 26, 2016 at 10:43:58AM +0800, Zhen Lei wrote:

quoted

For a normal memory@ devicetree node, its reg property can contains more
memory blocks.

Because we don't known how many memory blocks maybe contained, so we try
from index=0, increase 1 until error returned(the end).

Signed-off-by: Zhen Lei <redacted>
---
 drivers/of/of_numa.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/of/of_numa.c b/drivers/of/of_numa.c
index 21d831f..2c5f249 100644
--- a/drivers/of/of_numa.c
+++ b/drivers/of/of_numa.c

@@ -63,7 +63,7 @@ static int __init of_numa_parse_memory_nodes(void)
     struct device_node *np = NULL;
     struct resource rsrc;
     u32 nid;
-    int r = 0;
+    int i, r = 0;

     for (;;) {
             np = of_find_node_by_type(np, "memory");

@@ -82,17 +82,27 @@ static int __init of_numa_parse_memory_nodes(void)
                     /* some other error */
                     break;

-            r = of_address_to_resource(np, 0, &rsrc);
-            if (r) {
-                    pr_err("NUMA: bad reg property in memory node\n");
-                    break;
+            for (i = 0; ; i++) {
+                    r = of_address_to_resource(np, i, &rsrc);
+                    if (r) {
+                            /* reached the end of of_address */
+                            if (i > 0) {
+                                    r = 0;
+                                    break;
+                            }
+
+                            pr_err("NUMA: bad reg property in memory node\n");
+                            goto finished;
+                    }
+
+                    r = numa_add_memblk(nid, rsrc.start,
+                                        rsrc.end - rsrc.start + 1);
+                    if (r)
+                            goto finished;
             }
-
-            r = numa_add_memblk(nid, rsrc.start,
-                                rsrc.end - rsrc.start + 1);
-            if (r)
-                    break;
     }
+
+finished:
     of_node_put(np);

This function can be simplified down to:

      for_each_node_by_type(np, "memory") {

OK, That's good.

quoted

              r = of_property_read_u32(np, "numa-node-id", &nid);
              if (r == -EINVAL)
                      /*
                       * property doesn't exist if -EINVAL, continue
                       * looking for more memory nodes with
                       * "numa-node-id" property
                       */
                      continue;

Hi, everybody:
    If some "memory" node contains "numa-node-id", but some others missed. Can we simply ignored it?
I think we should break out too, and faking to only have node0.

Continuing to work is probably better than not.

quoted

              else if (r)
                      /* some other error */
                      break;

              r = of_address_to_resource(np, 0, &rsrc);
              for (i = 0; !r; i++, r = of_address_to_resource(np, i,

But r(non-zero) is just break this loop, the original is break the outer for (;;) loop

It is not really the kernel's job to validate the DT. If there's
random things in it then kernel's behavior is undefined.

How about as below?

        for_each_node_by_type(np, "memory") {
                ... ...

                for (i = 0; !of_address_to_resource(np, i, &rsrc); i++) {
                        r = numa_add_memblk(nid, rsrc.start,
                                            rsrc.end - rsrc.start + 1);
                        if (r)
                                goto finished;
                }

                if (!i)
                        pr_err("NUMA: bad reg property in memory node\n");
        }

finished:

Please try to avoid the goto. You can check r in the outer loop too.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 2016/5/27 12:20, Rob Herring wrote:

On Thu, May 26, 2016 at 10:36 PM, Leizhen (ThunderTown)
[off-list ref] wrote:

quoted

On 2016/5/26 21:13, Rob Herring wrote:

quoted

On Thu, May 26, 2016 at 10:43:58AM +0800, Zhen Lei wrote:

quoted

For a normal memory@ devicetree node, its reg property can contains more
memory blocks.

Because we don't known how many memory blocks maybe contained, so we try
from index=0, increase 1 until error returned(the end).

Signed-off-by: Zhen Lei <redacted>
---
 drivers/of/of_numa.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/of/of_numa.c b/drivers/of/of_numa.c
index 21d831f..2c5f249 100644
--- a/drivers/of/of_numa.c
+++ b/drivers/of/of_numa.c

@@ -63,7 +63,7 @@ static int __init of_numa_parse_memory_nodes(void)
     struct device_node *np = NULL;
     struct resource rsrc;
     u32 nid;
-    int r = 0;
+    int i, r = 0;

     for (;;) {
             np = of_find_node_by_type(np, "memory");

@@ -82,17 +82,27 @@ static int __init of_numa_parse_memory_nodes(void)
                     /* some other error */
                     break;

-            r = of_address_to_resource(np, 0, &rsrc);
-            if (r) {
-                    pr_err("NUMA: bad reg property in memory node\n");
-                    break;
+            for (i = 0; ; i++) {
+                    r = of_address_to_resource(np, i, &rsrc);
+                    if (r) {
+                            /* reached the end of of_address */
+                            if (i > 0) {
+                                    r = 0;
+                                    break;
+                            }
+
+                            pr_err("NUMA: bad reg property in memory node\n");
+                            goto finished;
+                    }
+
+                    r = numa_add_memblk(nid, rsrc.start,
+                                        rsrc.end - rsrc.start + 1);
+                    if (r)
+                            goto finished;
             }
-
-            r = numa_add_memblk(nid, rsrc.start,
-                                rsrc.end - rsrc.start + 1);
-            if (r)
-                    break;
     }
+
+finished:
     of_node_put(np);

This function can be simplified down to:

      for_each_node_by_type(np, "memory") {

OK, That's good.

quoted

              r = of_property_read_u32(np, "numa-node-id", &nid);
              if (r == -EINVAL)
                      /*
                       * property doesn't exist if -EINVAL, continue
                       * looking for more memory nodes with
                       * "numa-node-id" property
                       */
                      continue;

Hi, everybody:
    If some "memory" node contains "numa-node-id", but some others missed. Can we simply ignored it?
I think we should break out too, and faking to only have node0.

Continuing to work is probably better than not.

quoted

quoted

              else if (r)
                      /* some other error */
                      break;

              r = of_address_to_resource(np, 0, &rsrc);
              for (i = 0; !r; i++, r = of_address_to_resource(np, i,

But r(non-zero) is just break this loop, the original is break the outer for (;;) loop

It is not really the kernel's job to validate the DT. If there's
random things in it then kernel's behavior is undefined.

quoted

How about as below?

        for_each_node_by_type(np, "memory") {
                ... ...

                for (i = 0; !of_address_to_resource(np, i, &rsrc); i++) {
                        r = numa_add_memblk(nid, rsrc.start,
                                            rsrc.end - rsrc.start + 1);
                        if (r)
                                goto finished;
                }

                if (!i)
                        pr_err("NUMA: bad reg property in memory node\n");
        }

finished:

Please try to avoid the goto. You can check r in the outer loop too.

OK. I have rewritten this function according to your advice.

	for_each_node_by_type(np, "memory") {
                r = of_property_read_u32(np, "numa-node-id", &nid);
                if (r == -EINVAL)
                        /*
                         * property doesn't exist if -EINVAL, continue
                         * looking for more memory nodes with
                         * "numa-node-id" property
                         */
                        continue;
									//I deleted the break of "some other error", and it will break in below "if (!i || r)" branch

                for (i = 0; !r && !of_address_to_resource(np, i, &rsrc); i++)
                        r = numa_add_memblk(nid, rsrc.start,
                                            rsrc.end - rsrc.start + 1);

                if (!i || r) {
                        of_node_put(np);				//I moved here, so that it looks more clear. Because in the normal use of for_each_node_by_type, of_node_put is not required
                        pr_err("NUMA: bad property in memory node\n");	//Deleted "reg", so that it's suitable or harmless for other error cases
                        break;
                }
        }

	return r;

Rob

.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 05/26/2016 08:36 PM, Leizhen (ThunderTown) wrote:
[...]		continue;

Hi, everybody:
     If some "memory" node contains "numa-node-id", but some others missed. Can we simply ignored it?
I think we should break out too, and faking to only have node0.

I think if some "memory" nodes contain "numa-node-id" and others do not, 
then you have a defective device tree.  In this case I think we must 
continue with the existing behavior, and indicate failure.  This will 
cause the architecture specific NUMA code to disable NUMA and force 
everything onto a singl pseudo-NUMA-node.

I doubt there is anything to be gained by guessing which NUMA node 
orphaned "memory" nodes belong to.

quoted

		else if (r)
			/* some other error */
			break;

		r = of_address_to_resource(np, 0, &rsrc);
		for (i = 0; !r; i++, r = of_address_to_resource(np, i,

But r(non-zero) is just break this loop, the original is break the outer for (;;) loop

How about as below?

	for_each_node_by_type(np, "memory") {
		... ...

		for (i = 0; !of_address_to_resource(np, i, &rsrc); i++) {
                         r = numa_add_memblk(nid, rsrc.start,
                                             rsrc.end - rsrc.start + 1);
                         if (r)
                                 goto finished;
                 }

		if (!i)
			pr_err("NUMA: bad reg property in memory node\n");
	}

finished:
	

quoted

&rsrc)) {
			r = numa_add_memblk(nid, rsrc.start,
				    rsrc.end - rsrc.start + 1);
		}
	}
	of_node_put(np);

	return r;


Perhaps with a "if (!i && r) pr_err()" for an error message at the end.

Rob

.