| Message ID | 1464230639-9852-2-git-send-email-thunder.leizhen@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <thunder.leizhen@huawei.com> > --- > 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") { 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 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 <thunder.leizhen@huawei.com> >> --- >> 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) <thunder.leizhen@huawei.com> 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 <thunder.leizhen@huawei.com> >>> --- >>> 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. Rob
On 2016/5/27 12:20, Rob Herring wrote: > On Thu, May 26, 2016 at 10:36 PM, Leizhen (ThunderTown) > <thunder.leizhen@huawei.com> 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 <thunder.leizhen@huawei.com> >>>> --- >>>> 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 > > . >
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, &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 >> >> . >> >
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;
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 <thunder.leizhen@huawei.com> --- drivers/of/of_numa.c | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) -- 2.5.0