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