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

[PATCH] 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, 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;
+	}
 
 	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_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;
 				of_node_put(t);
-				return -EINVAL;
+				return cpu;
 			}
 			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) {
 		pr_err("%pOF: Can't get CPU for leaf core\n", core);
 		return -EINVAL;
 	}
-- 
2.8.1

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

[Dietmar Eggemann]

On 02/01/2020 04:24, 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.

Is this extra code really necessary?

Currently you get warnings indicating that CONFIG_NR_CPUS is too small
so you could correct the setup issue easily.

Example: Arm64 Juno board

$ grep "cpu@" ./arch/arm64/boot/dts/arm/juno.dts
		A57_0: cpu@0 {
		A57_1: cpu@1 {
		A53_0: cpu@100 {
		A53_1: cpu@101 {
		A53_2: cpu@102 {
		A53_3: cpu@103 {

root@juno:~# uname -r
5.5.0-rc5

root@juno:~# zcat /proc/config.gz | grep CONFIG_NR_CPUS
CONFIG_NR_CPUS=4

root@juno:~# cat /proc/cpuinfo | grep ^proc
processor       : 0
processor       : 1
processor       : 2
processor       : 3

root@juno:~# dmesg | grep "Unable\|Can't"
[    0.085089] Unable to find CPU node for /cpus/cpu@102
[    0.090179] /cpus/cpu-map/cluster1/core2: Can't get CPU for leaf core

[...]

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

[Zengtao (B)]

> -----Original Message-----
> From: Dietmar Eggemann [mailto:dietmar.eggemann@arm.com]
> Sent: Tuesday, January 07, 2020 2:42 AM
> To: Zengtao (B); sudeep.holla@arm.com
> Cc: Linuxarm; Greg Kroah-Hartman; Rafael J. Wysocki;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] cpu-topology: Skip the exist but not possible cpu
> nodes
> 
> On 02/01/2020 04:24, 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.
> 
> Is this extra code really necessary?
> 
> Currently you get warnings indicating that CONFIG_NR_CPUS is too small
> so you could correct the setup issue easily.
>

Not only about warning messages, the problem is :
What we are expected to do if the CONFIG_NR_CPUS is too small? I think there
are two choices:
1. Keep the dts parsing result, but skip the the CPU nodes whose id exceeds the 
the CONFIG_NR_CPUS, and this is what this patch do.
2. Just abort all the CPU nodes parsing, and using MPIDR to guess the topology, 
and this is what the current code do.
And i think choice 1 is better because:
1. It's a legal dts, we should keep the same result whether CONFIG_NR_CPUS is
too small or not.
2. In the function of_parse_and_init_cpus, we just do the same way as choice 1.

But i am open for the issue, any suggestions are welcomed.

Thanks
Zengtao 
 
> Example: Arm64 Juno board
> 
> $ grep "cpu@" ./arch/arm64/boot/dts/arm/juno.dts
> 		A57_0: cpu@0 {
> 		A57_1: cpu@1 {
> 		A53_0: cpu@100 {
> 		A53_1: cpu@101 {
> 		A53_2: cpu@102 {
> 		A53_3: cpu@103 {
> 
> root@juno:~# uname -r
> 5.5.0-rc5
> 
> root@juno:~# zcat /proc/config.gz | grep CONFIG_NR_CPUS
> CONFIG_NR_CPUS=4
> 
> root@juno:~# cat /proc/cpuinfo | grep ^proc
> processor       : 0
> processor       : 1
> processor       : 2
> processor       : 3
> 
> root@juno:~# dmesg | grep "Unable\|Can't"
> [    0.085089] Unable to find CPU node for /cpus/cpu@102
> [    0.090179] /cpus/cpu-map/cluster1/core2: Can't get CPU for leaf core
> 
> [...]

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

[Dietmar Eggemann]

On 07/01/2020 02:35, Zengtao (B) wrote:
>> -----Original Message-----
>> From: Dietmar Eggemann [mailto:dietmar.eggemann@arm.com]
>> Sent: Tuesday, January 07, 2020 2:42 AM
>> To: Zengtao (B); sudeep.holla@arm.com
>> Cc: Linuxarm; Greg Kroah-Hartman; Rafael J. Wysocki;
>> linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH] cpu-topology: Skip the exist but not possible cpu
>> nodes
>>
>> On 02/01/2020 04:24, 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.
>>
>> Is this extra code really necessary?
>>
>> Currently you get warnings indicating that CONFIG_NR_CPUS is too small
>> so you could correct the setup issue easily.
>>
> 
> Not only about warning messages, the problem is :
> What we are expected to do if the CONFIG_NR_CPUS is too small? I think there
> are two choices:
> 1. Keep the dts parsing result, but skip the the CPU nodes whose id exceeds the 
> the CONFIG_NR_CPUS, and this is what this patch do.
> 2. Just abort all the CPU nodes parsing, and using MPIDR to guess the topology, 
> and this is what the current code do.

Ah, you're referring to:

530 void __init init_cpu_topology(void)
531 {
...
540         else if (of_have_populated_dt() && parse_dt_topology())
541 -->             reset_cpu_topology();

With my Juno example (6 Cpus in DT but CONFIG_NR_CPUS=4):

root@juno:~# dmesg | grep "\*\*\|mpidr"
[    0.084760] ** get_cpu_for_node() cpu=1
[    0.088706] ** get_cpu_for_node() cpu=2
[    0.092592] ** get_cpu_for_node() cpu=0
[    0.096550] ** get_cpu_for_node() cpu=3
[    0.105578] ** get_cpu_for_node() cpu=-19
[    0.116070] ** store_cpu_topology(): cpuid=0
[    0.120355] CPU0: cluster 1 core 0 thread -1 mpidr 0x00000080000100
[    0.242465] ** store_cpu_topology(): cpuid=1
[    0.242471] CPU1: cluster 0 core 0 thread -1 mpidr 0x00000080000000
[    0.286505] ** store_cpu_topology(): cpuid=2
[    0.286510] CPU2: cluster 0 core 1 thread -1 mpidr 0x00000080000001
[    0.330631] ** store_cpu_topology(): cpuid=3
[    0.330637] CPU3: cluster 1 core 1 thread -1 mpidr 0x00000080000101

and with your patch:

root@juno:~# dmesg | grep "\*\*\|mpidr"
[    0.084778] ** get_cpu_for_node() cpu=1
[    0.088742] ** get_cpu_for_node() cpu=2
[    0.092662] ** get_cpu_for_node() cpu=0
[    0.096627] ** get_cpu_for_node() cpu=3
[    0.107942] ** get_cpu_for_node() cpu=-19
[    0.119429] ** get_cpu_for_node() cpu=-19
[    0.123461] ** store_cpu_topology(): cpuid=0
[    0.243571] ** store_cpu_topology(): cpuid=1
[    0.287610] ** store_cpu_topology(): cpuid=2
[    0.331737] ** store_cpu_topology(): cpuid=3

so we bail out of store_cpu_topology() since 'cpuid_topo->package_id != -1'.

> And i think choice 1 is better because:
> 1. It's a legal dts, we should keep the same result whether CONFIG_NR_CPUS is
> too small or not.
> 2. In the function of_parse_and_init_cpus, we just do the same way as choice 1.
> 
> But i am open for the issue, any suggestions are welcomed.

[...]

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

[Sudeep Holla]

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

>  	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 ?

>  			}
>  			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.

--
Regards,
Sudeep

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

[Zengtao (B)]

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

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

[Zengtao (B)]

> -----Original Message-----
> From: Dietmar Eggemann [mailto:dietmar.eggemann@arm.com]
> Sent: Tuesday, January 07, 2020 9:12 PM
> To: Zengtao (B); sudeep.holla@arm.com
> Cc: Linuxarm; Greg Kroah-Hartman; Rafael J. Wysocki;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] cpu-topology: Skip the exist but not possible cpu
> nodes
> 
> On 07/01/2020 02:35, Zengtao (B) wrote:
> >> -----Original Message-----
> >> From: Dietmar Eggemann [mailto:dietmar.eggemann@arm.com]
> >> Sent: Tuesday, January 07, 2020 2:42 AM
> >> To: Zengtao (B); sudeep.holla@arm.com
> >> Cc: Linuxarm; Greg Kroah-Hartman; Rafael J. Wysocki;
> >> linux-kernel@vger.kernel.org
> >> Subject: Re: [PATCH] cpu-topology: Skip the exist but not possible cpu
> >> nodes
> >>
> >> On 02/01/2020 04:24, 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.
> >>
> >> Is this extra code really necessary?
> >>
> >> Currently you get warnings indicating that CONFIG_NR_CPUS is too
> small
> >> so you could correct the setup issue easily.
> >>
> >
> > Not only about warning messages, the problem is :
> > What we are expected to do if the CONFIG_NR_CPUS is too small? I think
> there
> > are two choices:
> > 1. Keep the dts parsing result, but skip the the CPU nodes whose id
> exceeds the
> > the CONFIG_NR_CPUS, and this is what this patch do.
> > 2. Just abort all the CPU nodes parsing, and using MPIDR to guess the
> topology,
> > and this is what the current code do.
> 
> Ah, you're referring to:
> 
> 530 void __init init_cpu_topology(void)
> 531 {
> ...
> 540         else if (of_have_populated_dt() && parse_dt_topology())
> 541 -->             reset_cpu_topology();
> 
> With my Juno example (6 Cpus in DT but CONFIG_NR_CPUS=4):
> 
> root@juno:~# dmesg | grep "\*\*\|mpidr"
> [    0.084760] ** get_cpu_for_node() cpu=1
> [    0.088706] ** get_cpu_for_node() cpu=2
> [    0.092592] ** get_cpu_for_node() cpu=0
> [    0.096550] ** get_cpu_for_node() cpu=3
> [    0.105578] ** get_cpu_for_node() cpu=-19
> [    0.116070] ** store_cpu_topology(): cpuid=0
> [    0.120355] CPU0: cluster 1 core 0 thread -1 mpidr 0x00000080000100
> [    0.242465] ** store_cpu_topology(): cpuid=1
> [    0.242471] CPU1: cluster 0 core 0 thread -1 mpidr 0x00000080000000
> [    0.286505] ** store_cpu_topology(): cpuid=2
> [    0.286510] CPU2: cluster 0 core 1 thread -1 mpidr 0x00000080000001
> [    0.330631] ** store_cpu_topology(): cpuid=3
> [    0.330637] CPU3: cluster 1 core 1 thread -1 mpidr 0x00000080000101
> 
> and with your patch:
> 
> root@juno:~# dmesg | grep "\*\*\|mpidr"
> [    0.084778] ** get_cpu_for_node() cpu=1
> [    0.088742] ** get_cpu_for_node() cpu=2
> [    0.092662] ** get_cpu_for_node() cpu=0
> [    0.096627] ** get_cpu_for_node() cpu=3
> [    0.107942] ** get_cpu_for_node() cpu=-19
> [    0.119429] ** get_cpu_for_node() cpu=-19
> [    0.123461] ** store_cpu_topology(): cpuid=0
> [    0.243571] ** store_cpu_topology(): cpuid=1
> [    0.287610] ** store_cpu_topology(): cpuid=2
> [    0.331737] ** store_cpu_topology(): cpuid=3
> 
> so we bail out of store_cpu_topology() since 'cpuid_topo->package_id !=
> -1'.
> 

Good, you got me. And I found this issue when I test the NUMA issue.
Thanks.

> > And i think choice 1 is better because:
> > 1. It's a legal dts, we should keep the same result whether
> CONFIG_NR_CPUS is
> > too small or not.
> > 2. In the function of_parse_and_init_cpus, we just do the same way as
> choice 1.
> >
> > But i am open for the issue, any suggestions are welcomed.
> 
> [...]

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

[Sudeep Holla]

On Wed, Jan 08, 2020 at 01:57:34AM +0000, Zengtao (B) wrote:
[...]

> 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?
>

No I don't have any. So please drop the extra logic, add a comment on
-ENODEV return value and use it as needed.

--
Regards,
Sudeep

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

[Zengtao (B)]

> -----Original Message-----
> From: Sudeep Holla [mailto:sudeep.holla@arm.com]
> Sent: Friday, January 10, 2020 7:16 PM
> To: Zengtao (B)
> Cc: Linuxarm; Greg Kroah-Hartman; Rafael J. Wysocki;
> linux-kernel@vger.kernel.org; Sudeep Holla
> Subject: Re: [PATCH] cpu-topology: Skip the exist but not possible cpu
> nodes
> 
> On Wed, Jan 08, 2020 at 01:57:34AM +0000, Zengtao (B) wrote:
> [...]
> 
> > 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?
> >
> 
> No I don't have any. So please drop the extra logic, add a comment on
> -ENODEV return value and use it as needed.

OK, I will address the review comments and send v2.
Thanks 

Zengtao