Re: [PATCH 2/3] of/numa: fix a memory@ dt node can only contains one memory block

2016-05-27 Thread David Daney 

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.





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

r = of_address_to_resource(np, 0, );
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, ); 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:



)) {
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

.

Re: [PATCH 2/3] of/numa: fix a memory@ dt node can only contains one memory block

2016-05-27 Thread David Daney 

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.





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

r = of_address_to_resource(np, 0, );
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, ); 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:



)) {
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

.

Re: [PATCH 2/3] of/numa: fix a memory@ dt node can only contains one memory block

2016-05-27 Thread Leizhen (ThunderTown) 


On 2016/5/27 12:20, Rob Herring wrote:
> On Thu, May 26, 2016 at 10:36 PM, Leizhen (ThunderTown)
>  wrote:
>>
>>
>> On 2016/5/26 21:13, Rob Herring wrote:
>>> On Thu, May 26, 2016 at 10:43:58AM +0800, Zhen Lei wrote:
 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 
 ---
  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, );
 -if (r) {
 -pr_err("NUMA: bad reg property in memory node\n");
 -break;
 +for (i = 0; ; i++) {
 +r = of_address_to_resource(np, i, );
 +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", );
>>>   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.
> 
>>
>>>   else if (r)
>>>   /* some other error */
>>>   break;
>>>
>>>   r = of_address_to_resource(np, 0, );
>>>   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, ); 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", );
if (r == -EINVAL)
/*
 * property doesn't exist if -EINVAL, continue
 * looking for more memory nodes with
 * "numa-node-id" property
 */
continue;
//I 
deleted the

Re: [PATCH 2/3] of/numa: fix a memory@ dt node can only contains one memory block

2016-05-27 Thread Leizhen (ThunderTown) 


On 2016/5/27 12:20, Rob Herring wrote:
> On Thu, May 26, 2016 at 10:36 PM, Leizhen (ThunderTown)
>  wrote:
>>
>>
>> On 2016/5/26 21:13, Rob Herring wrote:
>>> On Thu, May 26, 2016 at 10:43:58AM +0800, Zhen Lei wrote:
 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 
 ---
  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, );
 -if (r) {
 -pr_err("NUMA: bad reg property in memory node\n");
 -break;
 +for (i = 0; ; i++) {
 +r = of_address_to_resource(np, i, );
 +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", );
>>>   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.
> 
>>
>>>   else if (r)
>>>   /* some other error */
>>>   break;
>>>
>>>   r = of_address_to_resource(np, 0, );
>>>   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, ); 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", );
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

Re: [PATCH 2/3] of/numa: fix a memory@ dt node can only contains one memory block

2016-05-26 Thread Rob Herring 
On Thu, May 26, 2016 at 10:36 PM, Leizhen (ThunderTown)
 wrote:
>
>
> On 2016/5/26 21:13, Rob Herring wrote:
>> On Thu, May 26, 2016 at 10:43:58AM +0800, Zhen Lei wrote:
>>> 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 
>>> ---
>>>  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, );
>>> -if (r) {
>>> -pr_err("NUMA: bad reg property in memory node\n");
>>> -break;
>>> +for (i = 0; ; i++) {
>>> +r = of_address_to_resource(np, i, );
>>> +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", );
>>   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.

>
>>   else if (r)
>>   /* some other error */
>>   break;
>>
>>   r = of_address_to_resource(np, 0, );
>>   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, ); 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

Re: [PATCH 2/3] of/numa: fix a memory@ dt node can only contains one memory block

2016-05-26 Thread Rob Herring 
On Thu, May 26, 2016 at 10:36 PM, Leizhen (ThunderTown)
 wrote:
>
>
> On 2016/5/26 21:13, Rob Herring wrote:
>> On Thu, May 26, 2016 at 10:43:58AM +0800, Zhen Lei wrote:
>>> 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 
>>> ---
>>>  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, );
>>> -if (r) {
>>> -pr_err("NUMA: bad reg property in memory node\n");
>>> -break;
>>> +for (i = 0; ; i++) {
>>> +r = of_address_to_resource(np, i, );
>>> +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", );
>>   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.

>
>>   else if (r)
>>   /* some other error */
>>   break;
>>
>>   r = of_address_to_resource(np, 0, );
>>   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, ); 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

Re: [PATCH 2/3] of/numa: fix a memory@ dt node can only contains one memory block

2016-05-26 Thread Leizhen (ThunderTown) 


On 2016/5/26 21:13, Rob Herring wrote:
> On Thu, May 26, 2016 at 10:43:58AM +0800, Zhen Lei wrote:
>> 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 
>> ---
>>  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, );
>> -if (r) {
>> -pr_err("NUMA: bad reg property in memory node\n");
>> -break;
>> +for (i = 0; ; i++) {
>> +r = of_address_to_resource(np, i, );
>> +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", );
>   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, );
>   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, ); 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:


> )) {
>   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
> 
> .
>

Re: [PATCH 2/3] of/numa: fix a memory@ dt node can only contains one memory block

2016-05-26 Thread Leizhen (ThunderTown) 


On 2016/5/26 21:13, Rob Herring wrote:
> On Thu, May 26, 2016 at 10:43:58AM +0800, Zhen Lei wrote:
>> 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 
>> ---
>>  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, );
>> -if (r) {
>> -pr_err("NUMA: bad reg property in memory node\n");
>> -break;
>> +for (i = 0; ; i++) {
>> +r = of_address_to_resource(np, i, );
>> +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", );
>   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, );
>   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, ); 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:


> )) {
>   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
> 
> .
>

Re: [PATCH 2/3] of/numa: fix a memory@ dt node can only contains one memory block

2016-05-26 Thread Rob Herring 
On Thu, May 26, 2016 at 10:43:58AM +0800, Zhen Lei wrote:
> 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 
> ---
>  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, );
> - if (r) {
> - pr_err("NUMA: bad reg property in memory node\n");
> - break;
> + for (i = 0; ; i++) {
> + r = of_address_to_resource(np, i, );
> + 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") {
r = of_property_read_u32(np, "numa-node-id", );
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, );
for (i = 0; !r; i++, r = of_address_to_resource(np, i, 
)) {
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

Re: [PATCH 2/3] of/numa: fix a memory@ dt node can only contains one memory block

2016-05-26 Thread Rob Herring 
On Thu, May 26, 2016 at 10:43:58AM +0800, Zhen Lei wrote:
> 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 
> ---
>  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, );
> - if (r) {
> - pr_err("NUMA: bad reg property in memory node\n");
> - break;
> + for (i = 0; ; i++) {
> + r = of_address_to_resource(np, i, );
> + 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") {
r = of_property_read_u32(np, "numa-node-id", );
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, );
for (i = 0; !r; i++, r = of_address_to_resource(np, i, 
)) {
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

[PATCH 2/3] of/numa: fix a memory@ dt node can only contains one memory block

2016-05-25 Thread Zhen Lei 
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 
---
 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, );
-   if (r) {
-   pr_err("NUMA: bad reg property in memory node\n");
-   break;
+   for (i = 0; ; i++) {
+   r = of_address_to_resource(np, i, );
+   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;
--
2.5.0

[PATCH 2/3] of/numa: fix a memory@ dt node can only contains one memory block

2016-05-25 Thread Zhen Lei 
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 
---
 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, );
-   if (r) {
-   pr_err("NUMA: bad reg property in memory node\n");
-   break;
+   for (i = 0; ; i++) {
+   r = of_address_to_resource(np, i, );
+   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;
--
2.5.0