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
