From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933355AbdBPSZc (ORCPT ); Thu, 16 Feb 2017 13:25:32 -0500 Received: from Galois.linutronix.de ([146.0.238.70]:53616 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932452AbdBPSZa (ORCPT ); Thu, 16 Feb 2017 13:25:30 -0500 Date: Thu, 16 Feb 2017 19:25:21 +0100 (CET) From: Thomas Gleixner To: Andrew Banman cc: mingo@redhat.com, akpm@linux-foundation.org, hpa@zytor.com, mike.travis@hpe.com, rja@hpe.com, sivanich@hpe.com, x86@kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 4/6] x86/platform/uv/BAU: Implement uv4_wait_completion with read_status In-Reply-To: <1487123931-56809-5-git-send-email-abanman@hpe.com> Message-ID: References: <1487123931-56809-1-git-send-email-abanman@hpe.com> <1487123931-56809-5-git-send-email-abanman@hpe.com> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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