Re: [PATCH 4/6] x86/platform/uv/BAU: Implement uv4_wait_completion with read_status

[Thomas Gleixner]

On Tue, 14 Feb 2017, Andrew Banman wrote:
> UV4 does not employ a software-timeout as in previous generations so a new
> wait_completion routine without this logic is required. Certain completion
> statuses require the AUX status bit in addition to ERROR and BUSY.
> 
> Add the read_status routine to construct the full completion status. Use
> read_status in the uv4_wait_completion routine to handle all possible
> completion statuses.

Ok.

> +/*
> + * Returns the status of current BAU message for cpu desc as a bit field
> + * [Error][Busy][Aux]
> + */
> +static unsigned long read_status(unsigned long status_mmr, int index, int desc)
> +{
> +	unsigned long descriptor_status;
> +
> +	descriptor_status =
> +		((read_lmmr(status_mmr) >> index) & UV_ACT_STATUS_MASK) << 1;
> +
> +	descriptor_status |=
> +		(read_lmmr(UVH_LB_BAU_SB_ACTIVATION_STATUS_2) >> desc) & 0x1;

You can spare all those ugly line breaks by chosing a short variable
name. You explain already in the comment that the returned value is the
status for a cpu descriptor. So where is the point of making the local
helper variable repeat than info?

> +	return descriptor_status;
> +}
> +
> +static int uv4_wait_completion(struct bau_desc *bau_desc,
> +				struct bau_control *bcp, long try)
> +{
> +	unsigned long descriptor_stat;
> +	unsigned long err_busy_mmr;
> +	int err_busy_index;
> +	int desc = bcp->uvhub_cpu;
> +	struct ptc_stats *stat = bcp->statp;

We usually order the local variables in reverse fir tree mode:

> +	struct ptc_stats *stat = bcp->statp;
> +	unsigned long descriptor_stat;
> +	unsigned long err_busy_mmr;
> +	int desc = bcp->uvhub_cpu;
> +	int err_busy_index;

It's simpler to parse than the random line length mode above.

> +	status_mmr_loc(&err_busy_mmr, &err_busy_index, desc);
> +	descriptor_stat = read_status(err_busy_mmr, err_busy_index, desc);
> +
> +	/* spin on the status MMR, waiting for it to go idle */
> +	while (descriptor_stat != UV2H_DESC_IDLE) {
> +		switch (descriptor_stat) {
> +		case UV2H_DESC_SOURCE_TIMEOUT:
> +			stat->s_stimeout++;
> +			return FLUSH_GIVEUP;
> +
> +		case UV2H_DESC_DEST_TIMEOUT:
> +			stat->s_dtimeout++;
> +			bcp->conseccompletes = 0;
> +			return FLUSH_RETRY_TIMEOUT;
> +
> +		case UV2H_DESC_DEST_STRONG_NACK:
> +			stat->s_plugged++;
> +			bcp->conseccompletes = 0;
> +			return FLUSH_RETRY_PLUGGED;
> +
> +		case UV2H_DESC_DEST_PUT_ERR:
> +			bcp->conseccompletes = 0;
> +			return FLUSH_GIVEUP;
> +
> +		default:
> +			/* descriptor_stat is still BUSY */
> +			cpu_relax();
> +		}
> +		descriptor_stat =
> +			read_status(err_busy_mmr, err_busy_index, desc);

Again, making the variable name shorter spares you this ugly line
break. 'stat' is clear enough.

Other than those nitpicks, that's all fine.

Thanks,

	tglx