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

[Nathan Fontenot]

On 10/09/2018 08:59 AM, YueHaibing wrote:
> 'aa_index' is defined as an unsigned value, but find_aa_index
> may return -1 when dlpar_clone_property fails. So change 
> find_aa_index return value type to bool, which indicate 'aa_index'
> whether found or not.
> 
> Fixes: c05a5a40969e ("powerpc/pseries: Dynamic add entires to associativity lookup array")
> Signed-off-by: YueHaibing <yuehaibing@xxxxxxxxxx>

Reviewed-by: Nathan Fontenot nfont@xxxxxxxxxxxxxxxxxx>
 
> ---
> v3: change find_aa_index return type to bool
> v2: use 'rc' track the validation of aa_index
> ---
>  arch/powerpc/platforms/pseries/hotplug-memory.c | 61 ++++++++++++-------------
>  1 file changed, 28 insertions(+), 33 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
> index d26a771..4db510f 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
> @@ -101,11 +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 bool 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;
> +	u32 *assoc_arrays, new_prop_size;
> +	struct property *new_prop;
>  	int aa_arrays, aa_array_entries, aa_array_sz;
>  	int i, index;
> 
> @@ -121,46 +122,39 @@ 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;
> -		break;
> +		*aa_index = i;
> +		return true;
>  	}
> 
> -	if (aa_index == -1) {
> -		struct property *new_prop;
> -		u32 new_prop_size;
> -
> -		new_prop_size = ala_prop->length + aa_array_sz;
> -		new_prop = dlpar_clone_property(ala_prop, new_prop_size);
> -		if (!new_prop)
> -			return -1;
> -
> -		assoc_arrays = new_prop->value;
> +	new_prop_size = ala_prop->length + aa_array_sz;
> +	new_prop = dlpar_clone_property(ala_prop, new_prop_size);
> +	if (!new_prop)
> +		return false;
> 
> -		/* increment the number of entries in the lookup array */
> -		assoc_arrays[0] = cpu_to_be32(aa_arrays + 1);
> +	assoc_arrays = new_prop->value;
> 
> -		/* copy the new associativity into the lookup array */
> -		index = aa_arrays * aa_array_entries + 2;
> -		memcpy(&assoc_arrays[index], &lmb_assoc[1], aa_array_sz);
> +	/* increment the number of entries in the lookup array */
> +	assoc_arrays[0] = cpu_to_be32(aa_arrays + 1);
> 
> -		of_update_property(dr_node, new_prop);
> +	/* copy the new associativity into the lookup array */
> +	index = aa_arrays * aa_array_entries + 2;
> +	memcpy(&assoc_arrays[index], &lmb_assoc[1], aa_array_sz);
> 
> -		/*
> -		 * The associativity lookup array index for this lmb is
> -		 * 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;
> -	}
> +	of_update_property(dr_node, new_prop);
> 
> -	return aa_index;
> +	/*
> +	 * The associativity lookup array index for this lmb is
> +	 * 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;
> +	return true;
>  }
> 
>  static int update_lmb_associativity_index(struct drmem_lmb *lmb)
> @@ -169,6 +163,7 @@ static int update_lmb_associativity_index(struct drmem_lmb *lmb)
>  	struct property *ala_prop;
>  	const u32 *lmb_assoc;
>  	u32 aa_index;
> +	bool is_found;
> 
>  	parent = of_find_node_by_path("/");
>  	if (!parent)
> @@ -200,11 +195,11 @@ static int update_lmb_associativity_index(struct drmem_lmb *lmb)
>  		return -ENODEV;
>  	}
> 
> -	aa_index = find_aa_index(dr_node, ala_prop, lmb_assoc);
> +	is_found = find_aa_index(dr_node, ala_prop, lmb_assoc, &aa_index);
> 
>  	dlpar_free_cc_nodes(lmb_node);
> 
> -	if (aa_index < 0) {
> +	if (!is_found) {
>  		pr_err("Could not find LMB associativity\n");
>  		return -1;
>  	}
>