[PATCH v3] cpu-topology: Skip the exist but not possible cpu nodes

[PATCH v3] cpu-topology: Skip the exist but not possible cpu nodes

[Zeng Tao]

When CONFIG_NR_CPUS is smaller than the cpu nodes defined in the device
tree, all the cpu nodes 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>
---
Changelog:
v2->v3:
 -Fix the Review comments by sudeep, including fix typo, remove redundant check
 logic, change the warning print level etc.
v1->v2:
 -Remove redundant -ENODEV assignment in get_cpu_for_node
 -Add comment to describe the get_cpu_for_node return values
 -Add skip process for cpu threads
 -Update the commit log with more detail
---
 drivers/base/arch_topology.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index 5fe44b3..d4302a1 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -248,6 +248,16 @@ core_initcall(free_raw_capacity);
 #endif
 
 #if defined(CONFIG_ARM64) || defined(CONFIG_RISCV)
+/*
+ * This function returns the logic cpu number of the node.
+ * There are basically three kinds of return values:
+ * (1) logic cpu number which is > 0.
+ * (2) -ENODEV when the node is valid one which can be found in the device tree
+ * but there is no possible cpu nodes to match, when the CONFIG_NR_CPUS is
+ * smaller than cpus node numbers in device tree, this will happen. It's
+ * suggested to just ignore this case.
+ * (3) -1 if the node does not exist in the device tree
+ */
 static int __init get_cpu_for_node(struct device_node *node)
 {
 	struct device_node *cpu_node;
@@ -261,7 +271,8 @@ static int __init get_cpu_for_node(struct device_node *node)
 	if (cpu >= 0)
 		topology_parse_cpu_capacity(cpu_node, cpu);
 	else
-		pr_crit("Unable to find CPU node for %pOF\n", cpu_node);
+		pr_info("CPU node for %pOF exist but the possible cpu range is :%*pbl\n",
+			cpu_node, cpumask_pr_args(cpu_possible_mask));
 
 	of_node_put(cpu_node);
 	return cpu;
@@ -286,9 +297,8 @@ 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;
 				cpu_topology[cpu].thread_id = i;
-			} else {
-				pr_err("%pOF: Can't get CPU for thread\n",
-				       t);
+			} else if (cpu != -ENODEV) {
+				pr_err("%pOF: Can't get CPU for thread\n", t);
 				of_node_put(t);
 				return -EINVAL;
 			}
@@ -307,7 +317,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) {
 		pr_err("%pOF: Can't get CPU for leaf core\n", core);
 		return -EINVAL;
 	}
-- 
2.8.1

Re: [PATCH v3] cpu-topology: Skip the exist but not possible cpu nodes

[Sudeep Holla]

Sorry for being pedantic and not mentioning this earlier. Can you improve
the $subject. I prefer:

cpu-topology: Don't error on more than CONFIG_NR_CPUS CPUs in device tree

On Thu, Jan 16, 2020 at 09:47:35AM +0800, Zeng Tao wrote:
> When CONFIG_NR_CPUS is smaller than the cpu nodes defined in the device
> tree, all the cpu nodes 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.
>

Also the commit log to be:
"
When the kernel is configured with CONFIG_NR_CPUS smaller than the
number of CPU nodes in the device tree(DT), all the CPU nodes parsing
done to fetch topology information will fail. This is not reasonable
as it is legal to have all the physical CPUs in the system in the DT.

Let us just skip such CPU DT nodes that are not used in the kernel
rather than returning an error.
"
> Signed-off-by: Zeng Tao <prime.zeng@hisilicon.com>
> ---
> Changelog:
> v2->v3:
>  -Fix the Review comments by sudeep, including fix typo, remove redundant check
>  logic, change the warning print level etc.
> v1->v2:
>  -Remove redundant -ENODEV assignment in get_cpu_for_node
>  -Add comment to describe the get_cpu_for_node return values
>  -Add skip process for cpu threads
>  -Update the commit log with more detail
> ---
>  drivers/base/arch_topology.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index 5fe44b3..d4302a1 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -248,6 +248,16 @@ core_initcall(free_raw_capacity);
>  #endif
>  
>  #if defined(CONFIG_ARM64) || defined(CONFIG_RISCV)
> +/*
> + * This function returns the logic cpu number of the node.
> + * There are basically three kinds of return values:
> + * (1) logic cpu number which is > 0.
> + * (2) -ENODEV when the node is valid one which can be found in the device tree
> + * but there is no possible cpu nodes to match, when the CONFIG_NR_CPUS is
> + * smaller than cpus node numbers in device tree, this will happen. It's
> + * suggested to just ignore this case.

I prefer (2) to be reword as below:
"
-ENODEV when the device tree(DT) node is valid and found in the DT but
there is no possible logical CPU in the kernel to match. This happens
when CONFIG_NR_CPUS is configure to be smaller than the number of CPU
nodes in DT. We need to just ignore this case.
"

With all these changes you can stick:
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

--
Regards,
Sudeep

RE: [PATCH v3] cpu-topology: Skip the exist but not possible cpu nodes

[Zengtao (B)]

> -----Original Message-----
> From: Sudeep Holla [mailto:sudeep.holla@arm.com]
> Sent: Thursday, January 16, 2020 8:25 PM
> To: Zengtao (B)
> Cc: Linuxarm; Greg Kroah-Hartman; Rafael J. Wysocki; Sudeep Holla;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v3] cpu-topology: Skip the exist but not possible cpu
> nodes
> 
> Sorry for being pedantic and not mentioning this earlier. Can you
> improve
> the $subject. I prefer:
> 
> cpu-topology: Don't error on more than CONFIG_NR_CPUS CPUs in
> device tree
> 
> On Thu, Jan 16, 2020 at 09:47:35AM +0800, Zeng Tao wrote:
> > When CONFIG_NR_CPUS is smaller than the cpu nodes defined in the
> device
> > tree, all the cpu nodes 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.
> >
> 
> Also the commit log to be:
> "
> When the kernel is configured with CONFIG_NR_CPUS smaller than the
> number of CPU nodes in the device tree(DT), all the CPU nodes parsing
> done to fetch topology information will fail. This is not reasonable
> as it is legal to have all the physical CPUs in the system in the DT.
> 
> Let us just skip such CPU DT nodes that are not used in the kernel
> rather than returning an error.
> "
> > Signed-off-by: Zeng Tao <prime.zeng@hisilicon.com>
> > ---
> > Changelog:
> > v2->v3:
> >  -Fix the Review comments by sudeep, including fix typo, remove
> redundant check
> >  logic, change the warning print level etc.
> > v1->v2:
> >  -Remove redundant -ENODEV assignment in get_cpu_for_node
> >  -Add comment to describe the get_cpu_for_node return values
> >  -Add skip process for cpu threads
> >  -Update the commit log with more detail
> > ---
> >  drivers/base/arch_topology.c | 20 +++++++++++++++-----
> >  1 file changed, 15 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/base/arch_topology.c
> b/drivers/base/arch_topology.c
> > index 5fe44b3..d4302a1 100644
> > --- a/drivers/base/arch_topology.c
> > +++ b/drivers/base/arch_topology.c
> > @@ -248,6 +248,16 @@ core_initcall(free_raw_capacity);
> >  #endif
> >
> >  #if defined(CONFIG_ARM64) || defined(CONFIG_RISCV)
> > +/*
> > + * This function returns the logic cpu number of the node.
> > + * There are basically three kinds of return values:
> > + * (1) logic cpu number which is > 0.
> > + * (2) -ENODEV when the node is valid one which can be found in the
> device tree
> > + * but there is no possible cpu nodes to match, when the
> CONFIG_NR_CPUS is
> > + * smaller than cpus node numbers in device tree, this will happen.
> It's
> > + * suggested to just ignore this case.
> 
> I prefer (2) to be reword as below:
> "
> -ENODEV when the device tree(DT) node is valid and found in the DT but
> there is no possible logical CPU in the kernel to match. This happens
> when CONFIG_NR_CPUS is configure to be smaller than the number of
> CPU
> nodes in DT. We need to just ignore this case.
> "
> 
> With all these changes you can stick:
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> 
OK, I will take the above suggestions, thanks for the patience, ^_^

Regards
Zengtao 
> --
> Regards,
> Sudeep