Subject: Re: [PATCH 3/9] powerpc/pseries: Add cpu DLPAR support for drc-info property

From: Michael Ellerman <[email-masked]>
Date: Thu, 07 Nov 2019 22:35:26 +1100

Tyrel Datwyler <[email-masked]> writes:
> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> index bbda646..9ba006c 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> @@ -730,24 +774,49 @@ static int find_dlpar_cpus_to_add(u32 *cpu_drcs, u32 cpus_to_add)
>  		return -1;
>  	}
>  
> -	/* Search the ibm,drc-indexes array for possible CPU drcs to
> -	 * add. Note that the format of the ibm,drc-indexes array is
> -	 * the number of entries in the array followed by the array
> -	 * of drc values so we start looking at index = 1.
> -	 */
> -	index = 1;
> -	while (cpus_found < cpus_to_add) {
> -		u32 drc;
> +	info = of_find_property(parent, "ibm,drc-info", NULL);
> +	if (info) {
> +		struct of_drc_info drc;
> +		const __be32 *value;
> +		int count;
>  
> -		rc = of_property_read_u32_index(parent, "ibm,drc-indexes",
> -						index++, &drc);
> -		if (rc)
> -			break;
> +		value = of_prop_next_u32(info, NULL, &count);
> +		if (value)
> +			value++;
>  
> -		if (dlpar_cpu_exists(parent, drc))
> -			continue;
> +		for (i = 0; i < count; i++) {
> +			of_read_drc_info_cell(&info, &value, &drc);
> +			if (strncmp(drc.drc_type, "CPU", 3))
> +				break;
> +
> +			for (j = 0; j < drc.num_sequential_elems && cpus_found < cpus_to_add; j++) {

This line's nearly 100 columns, which suggests that this logic has
gotten too convoluted to be a single function.

So I think you should split one or both arms of the if out into separate
functions.

You're basically doing nothing after the if, so possibly you can just
return the result of the split out functions directly.

cheers

> +				drc_index = drc.drc_index_start + (drc.sequential_inc * j);
> +
> +				if (dlpar_cpu_exists(parent, drc_index))
> +					continue;
> +
> +				cpu_drcs[cpus_found++] = drc_index;
> +			}
> +		}
> +	} else {
> +		/* Search the ibm,drc-indexes array for possible CPU drcs to
> +		 * add. Note that the format of the ibm,drc-indexes array is
> +		 * the number of entries in the array followed by the array
> +		 * of drc values so we start looking at index = 1.
> +		 */
> +		index = 1;
> +		while (cpus_found < cpus_to_add) {
> +			rc = of_property_read_u32_index(parent, "ibm,drc-indexes",
> +							index++, &drc_index);
> +
> +			if (rc)
> +				break;
>  
> -		cpu_drcs[cpus_found++] = drc;
> +			if (dlpar_cpu_exists(parent, drc_index))
> +				continue;
> +
> +			cpu_drcs[cpus_found++] = drc_index;
> +		}
>  	}
>  
>  	of_node_put(parent);