[v2,-next] powerpc/pseries/memory-hotplug: Fix return value type of find_aa_index
diff mbox series

Message ID 20181004094113.5492-1-yuehaibing@huawei.com
State Changes Requested
Headers show
Series
  • [v2,-next] powerpc/pseries/memory-hotplug: Fix return value type of find_aa_index
Related show

Checks

Context Check Description
snowpatch_ozlabs/build-ppc32 success Test build-ppc32 on branch next
snowpatch_ozlabs/build-ppc64e success Test build-ppc64e on branch next
snowpatch_ozlabs/build-ppc64be success Test build-ppc64be on branch next
snowpatch_ozlabs/build-ppc64le warning Test build-ppc64le on branch next
snowpatch_ozlabs/checkpatch success Test checkpatch on branch next
snowpatch_ozlabs/apply_patch success next/apply_patch Successfully applied

Commit Message

YueHaibing Oct. 4, 2018, 9:41 a.m. UTC
'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@huawei.com>
---
v2: use 'rc' track the validation of aa_index
---
 arch/powerpc/platforms/pseries/hotplug-memory.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

Comments

Michael Ellerman Oct. 9, 2018, 7 a.m. UTC | #1
YueHaibing <yuehaibing@huawei.com> 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@huawei.com>
> ---
> 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?

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)
YueHaibing Oct. 9, 2018, 2:04 p.m. UTC | #2
On 2018/10/9 15:00, Michael Ellerman wrote:
> YueHaibing <yuehaibing@huawei.com> 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@huawei.com>
>> ---
>> 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)
> 
> .
>

Patch
diff mbox series

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;
 
 	/*
 	 * 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;
 	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;
 	}
 
-	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)
@@ -169,6 +169,7 @@  static int update_lmb_associativity_index(struct drmem_lmb *lmb)
 	struct property *ala_prop;
 	const u32 *lmb_assoc;
 	u32 aa_index;
+	int rc;
 
 	parent = of_find_node_by_path("/");
 	if (!parent)
@@ -200,11 +201,11 @@  static int update_lmb_associativity_index(struct drmem_lmb *lmb)
 		return -ENODEV;
 	}
 
-	aa_index = find_aa_index(dr_node, ala_prop, lmb_assoc);
+	rc = find_aa_index(dr_node, ala_prop, lmb_assoc, &aa_index);
 
 	dlpar_free_cc_nodes(lmb_node);
 
-	if (aa_index < 0) {
+	if (rc < 0) {
 		pr_err("Could not find LMB associativity\n");
 		return -1;
 	}