Messages in this thread |  | | From | "Zengtao (B)" <> | Subject | RE: [PATCH] cpu-topology: Skip the exist but not possible cpu nodes | Date | Wed, 8 Jan 2020 01:57:34 +0000 |
| |
> -----Original Message----- > From: Sudeep Holla [mailto:sudeep.holla@arm.com] > Sent: Tuesday, January 07, 2020 10:50 PM > To: Zengtao (B) > Cc: Linuxarm; Greg Kroah-Hartman; Rafael J. Wysocki; Sudeep Holla; > linux-kernel@vger.kernel.org > Subject: Re: [PATCH] cpu-topology: Skip the exist but not possible cpu > nodes > > On Thu, Jan 02, 2020 at 11:24:49AM +0800, Zeng Tao wrote: > > When CONFIG_NR_CPUS is smaller than the cpu nodes defined in the > device > > tree, the cpu node parsing will fail. And this is not reasonable for a > > legal device tree configs. > > In this patch, skip such cpu nodes rather than return an error. > > > > Signed-off-by: Zeng Tao <prime.zeng@hisilicon.com> > > --- > > drivers/base/arch_topology.c | 35 > ++++++++++++++++++++++++++--------- > > 1 file changed, 26 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c > > index 5fe44b3..4cddfeb 100644 > > --- a/drivers/base/arch_topology.c > > +++ b/drivers/base/arch_topology.c > > @@ -250,20 +250,34 @@ core_initcall(free_raw_capacity); > > #if defined(CONFIG_ARM64) || defined(CONFIG_RISCV) > > static int __init get_cpu_for_node(struct device_node *node) > > { > > - struct device_node *cpu_node; > > + struct device_node *cpu_node, *t; > > int cpu; > > + bool found = false; > > > > cpu_node = of_parse_phandle(node, "cpu", 0); > > if (!cpu_node) > > - return -1; > > + return -EINVAL; > > + > > + for_each_of_cpu_node(t) > > + if (t == cpu_node) { > > + found = true; > > + break; > > + } > > + > > + if (!found) { > > + pr_crit("Unable to find CPU node for %pOF\n", cpu_node); > > + return -EINVAL; > > + } > > > > The whole extra logic added above sounds redundant, details below...
The above logic is different from what is done in of_cpu_node_to_id: 1. The above checks if the cpu node exist in the dts. 2. The of_cpu_node_to_id checks if the cpu node exist in the possible cpus.
And basically my idea is: 1. check if the cpu node exist or not. If not exist, just return an error to indicate that this is a broken dts. If exist, goto 2. 2. check if the cpu node is a possible one? And happy to continue if possible, or just skip and warn if not possible.
> > > cpu = of_cpu_node_to_id(cpu_node); > > if (cpu >= 0) > > topology_parse_cpu_capacity(cpu_node, cpu); > > - else > > - pr_crit("Unable to find CPU node for %pOF\n", cpu_node); > > + else { > > + pr_warn("CPU node for %pOF exist but the possible cpu range > is :%*pbl\n", > > + cpu_node, cpumask_pr_args(cpu_possible_mask)); > > + cpu = -ENODEV; > > .. of_cpu_node_to_id returns -ENODEV anyways so above assignment is > also > redundant. All you achieved is explicit error message. I think we should > be fine combining them. Just extend existing error log with both message. > > > + } > > > > - of_node_put(cpu_node); > > return cpu; > > } > > > > @@ -287,10 +301,13 @@ static int __init parse_core(struct > device_node *core, int package_id, > > cpu_topology[cpu].core_id = core_id; > > cpu_topology[cpu].thread_id = i; > > } else { > > - pr_err("%pOF: Can't get CPU for thread\n", > > - t); > > + if (cpu != -ENODEV) > > + pr_err("%pOF: Can't get CPU for thread\n", > > + t); > > + else > > + cpu = 0; > > I would rather use another variable instead of reusing 'cpu' > > > of_node_put(t); > > - return -EINVAL; > > + return cpu; > > Shouldn't we continue here if cpu == -ENODEV instead of returning 0 ?
Good catch, I just focus on core parsing, and thread parsing shoud work the same way.
> > > } > > of_node_put(t); > > } > > @@ -307,7 +324,7 @@ static int __init parse_core(struct device_node > *core, int package_id, > > > > cpu_topology[cpu].package_id = package_id; > > cpu_topology[cpu].core_id = core_id; > > - } else if (leaf) { > > + } else if (leaf && cpu != -ENODEV) { > > I am still not sure on the approach, it is based on -ENODEV as valid > error and allow to continue. It may be fine, I just need to make sure. >
I have the same concern, I have tried to find out some other return values But seems not good enough. Maybe it's better to introduce a new function to replace of_cpu_node_to_id Any good suggestion?
Thanks
Regards Zengtao
|  |