From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932437AbcE0HFs (ORCPT ); Fri, 27 May 2016 03:05:48 -0400 Received: from szxga03-in.huawei.com ([119.145.14.66]:23881 "EHLO szxga03-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932100AbcE0HFq (ORCPT ); Fri, 27 May 2016 03:05:46 -0400 Subject: Re: [PATCH 2/3] of/numa: fix a memory@ dt node can only contains one memory block To: Rob Herring References: <1464230639-9852-1-git-send-email-thunder.leizhen@huawei.com> <1464230639-9852-2-git-send-email-thunder.leizhen@huawei.com> <20160526131345.GA12024@rob-hp-laptop> <5747C0BF.505@huawei.com> CC: Catalin Marinas , Will Deacon , linux-arm-kernel , Ganapatrao Kulkarni , Robert Richter , "David Daney" , Frank Rowand , "Grant Likely" , devicetree , linux-kernel , Zefan Li , Xinwei Hu , Tianhong Ding , Hanjun Guo From: "Leizhen (ThunderTown)" Message-ID: <5747F16B.5050404@huawei.com> Date: Fri, 27 May 2016 15:04:11 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.177.23.164] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A020205.5747F179.01C2,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2013-05-26 15:14:31, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: f53ddda27bf22eccd269e1a5f459fb4c Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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, &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. > > Continuing to work is probably better than not. > >> >>> 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. 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 > > . >