[PATCH] powerpc/pseries: Fix numa FORM2 parsing fallback code

[Michael Ellerman]

Nicholas Piggin <npiggin at gmail.com> writes:
> Excerpts from Michael Ellerman's message of November 8, 2021 3:20 pm:
>> Nicholas Piggin <npiggin at gmail.com> writes:
>>> In case the FORM2 distance table from firmware is not the expected size,
>>> there is fallback code that just populates the lookup table as local vs
>>> remote.
>>>
>>> However it then continues on to use the distance table. Fix.
>>>
>>> Cc: Aneesh Kumar K.V <aneesh.kumar at linux.ibm.com>
>>> Fixes: 1c6b5a7e7405 ("powerpc/pseries: Add support for FORM2 associativity")
>>> Signed-off-by: Nicholas Piggin <npiggin at gmail.com>
>>> ---
>>>  arch/powerpc/mm/numa.c | 29 +++++++++++++----------------
>>>  1 file changed, 13 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
>>> index 6f14c8fb6359..0789cde7f658 100644
>>> --- a/arch/powerpc/mm/numa.c
>>> +++ b/arch/powerpc/mm/numa.c
>>> @@ -380,6 +380,7 @@ static void initialize_form2_numa_distance_lookup_table(void)
>>>  	const __be32 *numa_lookup_index;
>>>  	int numa_dist_table_length;
>>>  	int max_numa_index, distance_index;
>>> +	bool good = true;
>> 
>> numa_dist_table is a pointer, so couldn't we just set it to NULL if the
>> info it's pointing at is invalid?
>
> Yeah probably could just do that.
>
>> 
>>>  
>>>  	if (firmware_has_feature(FW_FEATURE_OPAL))
>>>  		root = of_find_node_by_path("/ibm,opal");
>>> @@ -407,30 +408,26 @@ static void initialize_form2_numa_distance_lookup_table(void)
>>>  
>>>  	if (numa_dist_table_length != max_numa_index * max_numa_index) {
>>>  		WARN(1, "Wrong NUMA distance information\n");
>>> -		/* consider everybody else just remote. */
>>> -		for (i = 0;  i < max_numa_index; i++) {
>>> -			for (j = 0; j < max_numa_index; j++) {
>>> -				int nodeA = numa_id_index_table[i];
>>> -				int nodeB = numa_id_index_table[j];
>>> -
>>> -				if (nodeA == nodeB)
>>> -					numa_distance_table[nodeA][nodeB] = LOCAL_DISTANCE;
>>> -				else
>>> -					numa_distance_table[nodeA][nodeB] = REMOTE_DISTANCE;
>>> -			}
>>> -		}
>>> +		good = false;
>> 
>> ie.		numa_dist_table = NULL;
>> 
>>>  	}
>>> -
>>>  	distance_index = 0;
>>>  	for (i = 0;  i < max_numa_index; i++) {
>>>  		for (j = 0; j < max_numa_index; j++) {
>>>  			int nodeA = numa_id_index_table[i];
>>>  			int nodeB = numa_id_index_table[j];
>>> -
>>> -			numa_distance_table[nodeA][nodeB] = numa_dist_table[distance_index++];
>>> -			pr_debug("dist[%d][%d]=%d ", nodeA, nodeB, numa_distance_table[nodeA][nodeB]);
>>> +			int dist;
>>> +
>>> +			if (good)
>> 
>> 			if (numa_dist_table)
>> 
>>> +				dist = numa_dist_table[distance_index++];
>>> +			else if (nodeA == nodeB)
>>> +				dist = LOCAL_DISTANCE;
>>> +			else
>>> +				dist = REMOTE_DISTANCE;
>>> +			numa_distance_table[nodeA][nodeB] = dist;
>>> +			pr_debug("dist[%d][%d]=%d ", nodeA, nodeB, dist);
>>>  		}
>>>  	}
>>> +
>>>  	of_node_put(root);
>>>  }
>> 
>> 
>> But maybe before we do that we can rename it, because it is really easy
>> to confuse numa_dist_table and numa_distance_table if you don't look
>> closely.
>
> Maybe dt_form2_distances?

Or just "form2_distances", it's only a local so the fact that it's from
the dt is clear enough I think.

cheers