Re: [PATCH v2 -next] powerpc/pseries/memory-hotplug: Fix return value type of find_aa_index

[YueHaibing]

On 2018/10/9 15:00, Michael Ellerman wrote:
> YueHaibing <yuehaibing@xxxxxxxxxx> writes:
>> 'aa_index' is defined as an unsigned value, but find_aa_index
>> may return -1 when dlpar_clone_property fails. So we use an rc
>> value to track the validation of finding the aa_index instead
>> of the 'aa_index' value itself
>>
>> Fixes: c05a5a40969e ("powerpc/pseries: Dynamic add entires to associativity lookup array")
>> Signed-off-by: YueHaibing <yuehaibing@xxxxxxxxxx>
>> ---
>> v2: use 'rc' track the validation of aa_index
> 
> Thanks for sending a v2, some more comments ...
> 
>> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
>> index 9a15d39..796e68b 100644
>> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
>> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
>> @@ -101,13 +101,12 @@ static struct property *dlpar_clone_property(struct property *prop,
>>  	return new_prop;
>>  }
>>  
>> -static u32 find_aa_index(struct device_node *dr_node,
>> -			 struct property *ala_prop, const u32 *lmb_assoc)
>> +static int find_aa_index(struct device_node *dr_node, struct property *ala_prop,
>> +			 const u32 *lmb_assoc, u32 *aa_index)
>>  {
>>  	u32 *assoc_arrays;
>> -	u32 aa_index;
>>  	int aa_arrays, aa_array_entries, aa_array_sz;
>> -	int i, index;
>> +	int i, index, rc = -1;
> 
> It's preferable to leave rc uninitialised until we actually need to
> initialise it, that gives the compiler the chance to warn us if we use
> it inadvertently before that.
> 
>>  
>>  	/*
>>  	 * The ibm,associativity-lookup-arrays property is defined to be
>> @@ -121,18 +120,18 @@ static u32 find_aa_index(struct device_node *dr_node,
>>  	aa_array_entries = be32_to_cpu(assoc_arrays[1]);
>>  	aa_array_sz = aa_array_entries * sizeof(u32);
>>  
>> -	aa_index = -1;
> 
> So that would be here:
> 	rc = -1;
> 
> But ..
> 
>>  	for (i = 0; i < aa_arrays; i++) {
>>  		index = (i * aa_array_entries) + 2;
>>  
>>  		if (memcmp(&assoc_arrays[index], &lmb_assoc[1], aa_array_sz))
>>  			continue;
>>  
>> -		aa_index = i;
>> +		*aa_index = i;
>> +		rc = 0;
>>  		break;
>>  	}
> 
> The 'rc' variable is basically a boolean now, it means "we found something".
> 
> And all we do with it in the found case (rc = 0) is test it below and return.
> 
> So can't we just return directly in the for loop above, rather than breaking?
> 
> In which case we don't need the rc variable at all.
> 
> And the whole function may as well return bool, rather than int.
> 
> Does that make sense?

Yes, will do that in v3.

> 
> cheers
> 
>> -	if (aa_index == -1) {
>> +	if (rc == -1) {
>>  		struct property *new_prop;
>>  		u32 new_prop_size;
>>  
>> @@ -157,10 +156,11 @@ static u32 find_aa_index(struct device_node *dr_node,
>>  		 * number of entries - 1 since we added its associativity
>>  		 * to the end of the lookup array.
>>  		 */
>> -		aa_index = be32_to_cpu(assoc_arrays[0]) - 1;
>> +		*aa_index = be32_to_cpu(assoc_arrays[0]) - 1;
>> +		rc = 0;
>>  	}
>>  
>> -	return aa_index;
>> +	return rc;
>>  }
>>  
>>  static int update_lmb_associativity_index(struct drmem_lmb *lmb)
> 
> .
>