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

[Nicholas Piggin <npiggin@gmail.com>]

Excerpts from Michael Ellerman's message of November 8, 2021 3:20 pm:
> Nicholas Piggin <npiggin@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@linux.ibm.com>
>> Fixes: 1c6b5a7e7405 ("powerpc/pseries: Add support for FORM2 associativity")
>> Signed-off-by: Nicholas Piggin <npiggin@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?

Thanks,
Nick