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