Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Leo Yan <leo.yan@linaro.org>
To:     Arnaldo Carvalho de Melo <acme@kernel.org>,
        Andre Przywara <andre.przywara@arm.com>,
        Dave Martin <Dave.Martin@arm.com>,
        James Clark <james.clark@arm.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@redhat.com>,
        Namhyung Kim <namhyung@kernel.org>,
        Al Grant <Al.Grant@arm.com>, Wei Li <liwei391@huawei.com>,
        linux-kernel@vger.kernel.org
Cc:     Leo Yan <leo.yan@linaro.org>
Subject: [PATCH v7 07/22] perf arm-spe: Consolidate arm_spe_pkt_desc()'s return value
Date:   Fri,  6 Nov 2020 09:41:21 +0800
Message-Id: <20201106014136.14103-8-leo.yan@linaro.org>
In-Reply-To: <20201106014136.14103-1-leo.yan@linaro.org>
References: <20201106014136.14103-1-leo.yan@linaro.org>
Precedence: bulk

arm_spe_pkt_desc() returns the length of consumed the buffer for
the success case; otherwise, it delivers the return value from
arm_spe_pkt_snprintf(), and returns the last return value if there have
multiple calling arm_spe_pkt_snprintf().

Since arm_spe_pkt_snprintf() has the same semantics with vsnprintf() for
the return value, and vsnprintf() might return value equals to or bigger
than the parameter 'size' to indicate the truncation.  Because the
return value is >= 0 when the string is truncated, this condition will
be returned up the stack as "success".

This patch simplifies the return value for arm_spe_pkt_desc(): '0' means
success and other values mean an error has occurred.  To realize this,
it relies on arm_spe_pkt_snprintf()'s parameter 'err', the 'err' is a
cumulative value, returns its final value if printing buffer is called
for one time or multiple times.

To unify the error value generation, this patch handles error in a
central place, rather than directly bailing out in switch-cases,
it returns error at the end of arm_spe_pkt_desc().

This patch changes the caller arm_spe_dump() to respect the updated
return value semantics of arm_spe_pkt_desc().

Suggested-by: Dave Martin <Dave.Martin@arm.com>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 128 +++++++++---------
 tools/perf/util/arm-spe.c                     |   2 +-
 2 files changed, 68 insertions(+), 62 deletions(-)

diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
index 1970686f7020..33baef0c2c0b 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
@@ -301,9 +301,10 @@ static int arm_spe_pkt_snprintf(int *err, char **buf_p, size_t *blen,
 int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
 		     size_t buf_len)
 {
-	int ret, ns, el, idx = packet->index;
+	int ns, el, idx = packet->index;
 	unsigned long long payload = packet->payload;
 	const char *name = arm_spe_pkt_name(packet->type);
+	char *buf_orig = buf;
 	size_t blen = buf_len;
 	int err = 0;
 
@@ -311,82 +312,76 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
 	case ARM_SPE_BAD:
 	case ARM_SPE_PAD:
 	case ARM_SPE_END:
-		return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s", name);
+		arm_spe_pkt_snprintf(&err, &buf, &blen, "%s", name);
+		break;
 	case ARM_SPE_EVENTS:
-		ret = arm_spe_pkt_snprintf(&err, &buf, &blen, "EV");
+		arm_spe_pkt_snprintf(&err, &buf, &blen, "EV");
 
 		if (payload & 0x1)
-			ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " EXCEPTION-GEN");
+			arm_spe_pkt_snprintf(&err, &buf, &blen, " EXCEPTION-GEN");
 		if (payload & 0x2)
-			ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " RETIRED");
+			arm_spe_pkt_snprintf(&err, &buf, &blen, " RETIRED");
 		if (payload & 0x4)
-			ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " L1D-ACCESS");
+			arm_spe_pkt_snprintf(&err, &buf, &blen, " L1D-ACCESS");
 		if (payload & 0x8)
-			ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " L1D-REFILL");
+			arm_spe_pkt_snprintf(&err, &buf, &blen, " L1D-REFILL");
 		if (payload & 0x10)
-			ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " TLB-ACCESS");
+			arm_spe_pkt_snprintf(&err, &buf, &blen, " TLB-ACCESS");
 		if (payload & 0x20)
-			ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " TLB-REFILL");
+			arm_spe_pkt_snprintf(&err, &buf, &blen, " TLB-REFILL");
 		if (payload & 0x40)
-			ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " NOT-TAKEN");
+			arm_spe_pkt_snprintf(&err, &buf, &blen, " NOT-TAKEN");
 		if (payload & 0x80)
-			ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " MISPRED");
+			arm_spe_pkt_snprintf(&err, &buf, &blen, " MISPRED");
 		if (idx > 1) {
 			if (payload & 0x100)
-				ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-ACCESS");
+				arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-ACCESS");
 			if (payload & 0x200)
-				ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-REFILL");
+				arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-REFILL");
 			if (payload & 0x400)
-				ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " REMOTE-ACCESS");
+				arm_spe_pkt_snprintf(&err, &buf, &blen, " REMOTE-ACCESS");
 		}
-		if (ret < 0)
-			return ret;
-		blen -= ret;
-		return buf_len - blen;
+		break;
 	case ARM_SPE_OP_TYPE:
 		switch (idx) {
 		case 0:
-			return arm_spe_pkt_snprintf(&err, &buf, &blen,
+			arm_spe_pkt_snprintf(&err, &buf, &blen,
 					payload & 0x1 ? "COND-SELECT" : "INSN-OTHER");
+			break;
 		case 1:
-			ret = arm_spe_pkt_snprintf(&err, &buf, &blen,
-						   payload & 0x1 ? "ST" : "LD");
+			arm_spe_pkt_snprintf(&err, &buf, &blen,
+					     payload & 0x1 ? "ST" : "LD");
 
 			if (payload & 0x2) {
 				if (payload & 0x4)
-					ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " AT");
+					arm_spe_pkt_snprintf(&err, &buf, &blen, " AT");
 				if (payload & 0x8)
-					ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " EXCL");
+					arm_spe_pkt_snprintf(&err, &buf, &blen, " EXCL");
 				if (payload & 0x10)
-					ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " AR");
+					arm_spe_pkt_snprintf(&err, &buf, &blen, " AR");
 			} else if (payload & 0x4) {
-				ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " SIMD-FP");
+				arm_spe_pkt_snprintf(&err, &buf, &blen, " SIMD-FP");
 			}
-
-			if (ret < 0)
-				return ret;
-			blen -= ret;
-			return buf_len - blen;
-
+			break;
 		case 2:
-			ret = arm_spe_pkt_snprintf(&err, &buf, &blen, "B");
+			arm_spe_pkt_snprintf(&err, &buf, &blen, "B");
 
 			if (payload & 0x1)
-				ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " COND");
+				arm_spe_pkt_snprintf(&err, &buf, &blen, " COND");
 			if (payload & 0x2)
-				ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " IND");
-
-			if (ret < 0)
-				return ret;
-			blen -= ret;
-			return buf_len - blen;
+				arm_spe_pkt_snprintf(&err, &buf, &blen, " IND");
 
+			break;
 		default:
-			return 0;
+			/* Unknown index */
+			err = -1;
+			break;
 		}
+		break;
 	case ARM_SPE_DATA_SOURCE:
 	case ARM_SPE_TIMESTAMP:
-		return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s %lld", name, payload);
+		arm_spe_pkt_snprintf(&err, &buf, &blen, "%s %lld", name, payload);
+		break;
 	case ARM_SPE_ADDRESS:
 		switch (idx) {
 		case 0:
@@ -394,48 +389,59 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
 			ns = !!(packet->payload & NS_FLAG);
 			el = (packet->payload & EL_FLAG) >> 61;
 			payload &= ~(0xffULL << 56);
-			return arm_spe_pkt_snprintf(&err, &buf, &blen,
+			arm_spe_pkt_snprintf(&err, &buf, &blen,
 					"%s 0x%llx el%d ns=%d",
 				        (idx == 1) ? "TGT" : "PC", payload, el, ns);
+			break;
 		case 2:
-			return arm_spe_pkt_snprintf(&err, &buf, &blen,
-						    "VA 0x%llx", payload);
+			arm_spe_pkt_snprintf(&err, &buf, &blen,
+					     "VA 0x%llx", payload);
+			break;
 		case 3:
 			ns = !!(packet->payload & NS_FLAG);
 			payload &= ~(0xffULL << 56);
-			return arm_spe_pkt_snprintf(&err, &buf, &blen,
-						    "PA 0x%llx ns=%d", payload, ns);
+			arm_spe_pkt_snprintf(&err, &buf, &blen,
+					     "PA 0x%llx ns=%d", payload, ns);
+			break;
 		default:
-			return 0;
+			/* Unknown index */
+			err = -1;
+			break;
 		}
+		break;
 	case ARM_SPE_CONTEXT:
-		return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s 0x%lx el%d",
-					    name, (unsigned long)payload, idx + 1);
+		arm_spe_pkt_snprintf(&err, &buf, &blen, "%s 0x%lx el%d",
+				     name, (unsigned long)payload, idx + 1);
+		break;
 	case ARM_SPE_COUNTER:
-		ret = arm_spe_pkt_snprintf(&err, &buf, &blen, "%s %d ", name,
-					   (unsigned short)payload);
+		arm_spe_pkt_snprintf(&err, &buf, &blen, "%s %d ", name,
+				     (unsigned short)payload);
 		switch (idx) {
 		case 0:
-			ret = arm_spe_pkt_snprintf(&err, &buf, &blen, "TOT");
+			arm_spe_pkt_snprintf(&err, &buf, &blen, "TOT");
 			break;
 		case 1:
-			ret = arm_spe_pkt_snprintf(&err, &buf, &blen, "ISSUE");
+			arm_spe_pkt_snprintf(&err, &buf, &blen, "ISSUE");
 			break;
 		case 2:
-			ret = arm_spe_pkt_snprintf(&err, &buf, &blen, "XLAT");
+			arm_spe_pkt_snprintf(&err, &buf, &blen, "XLAT");
 			break;
 		default:
-			ret = 0;
 			break;
 		}
-		if (ret < 0)
-			return ret;
-		blen -= ret;
-		return buf_len - blen;
+		break;
 	default:
+		/* Unknown index */
+		err = -1;
 		break;
 	}
 
-	return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s 0x%llx (%d)",
-				    name, payload, packet->index);
+	/* Output raw data if detect any error */
+	if (err) {
+		err = 0;
+		arm_spe_pkt_snprintf(&err, &buf_orig, &blen, "%s 0x%llx (%d)",
+				     name, payload, packet->index);
+	}
+
+	return err;
 }
diff --git a/tools/perf/util/arm-spe.c b/tools/perf/util/arm-spe.c
index 3882a5360ada..8901a1656a41 100644
--- a/tools/perf/util/arm-spe.c
+++ b/tools/perf/util/arm-spe.c
@@ -113,7 +113,7 @@ static void arm_spe_dump(struct arm_spe *spe __maybe_unused,
 		if (ret > 0) {
 			ret = arm_spe_pkt_desc(&packet, desc,
 					       ARM_SPE_PKT_DESC_MAX);
-			if (ret > 0)
+			if (!ret)
 				color_fprintf(stdout, color, " %s\n", desc);
 		} else {
 			color_fprintf(stdout, color, " Bad packet!\n");
-- 
2.17.1