libflash/ecc: Simplify and cleanup ecc code.

The ecc 'memcpy' style functions return success or fail in terms of the ECC
enum. This doesn't really make sense, use true or false. As the result the
ecc enum doesn't need to be exposed anymore, which makes more sense, not
clear why it was exposed in the first place.

Convert some of the ecc #defines to static inlines, shouldn't make any
difference but feels safer.

Fix minor stylistic and typo issues.

Reviewed-By: Alistair Popple <alistair@popple.id.au>
Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
Signed-off-by: Stewart Smith <stewart@linux.vnet.ibm.com>

4 files changed, 78 insertions, 60 deletions

diff --git a/libflash/ecc.c b/libflash/ecc.c
index 78c88d0..05c0e14 100644
--- a/libflash/ecc.c
+++ b/libflash/ecc.c
@@ -23,6 +23,22 @@
 #include "libflash.h"
 #include "ecc.h"
 
+/* Bit field identifiers for syndrome calculations. */
+enum eccbitfields
+{
+        GD = 0xff,      //< Good, ECC matches.
+        UE = 0xfe,      //< Uncorrectable.
+        E0 = 71,        //< Error in ECC bit 0
+        E1 = 70,        //< Error in ECC bit 1
+        E2 = 69,        //< Error in ECC bit 2
+        E3 = 68,        //< Error in ECC bit 3
+        E4 = 67,        //< Error in ECC bit 4
+        E5 = 66,        //< Error in ECC bit 5
+        E6 = 65,        //< Error in ECC bit 6
+        E7 = 64         //< Error in ECC bit 7
+        /* 0-63 Correctable bit in byte */
+};
+
 /*
  * Matrix used for ECC calculation.
  *
@@ -73,7 +89,7 @@ static uint64_t eccmatrix[] = {
  *
  *  Bits are in MSB order.
  */
-static uint8_t syndromematrix[] = {
+static enum eccbitfields syndromematrix[] = {
         GD, E7, E6, UE, E5, UE, UE, 47, E4, UE, UE, 37, UE, 35, 39, UE,
         E3, UE, UE, 48, UE, 30, 29, UE, UE, 57, 27, UE, 31, UE, UE, UE,
         E2, UE, UE, 17, UE, 18, 40, UE, UE, 58, 22, UE, 21, UE, UE, UE,
@@ -121,7 +137,7 @@ static uint8_t eccgenerate(uint64_t data)
  * @retval UE - Indicates the data is uncorrectable.
  * @retval all others - Indication of which bit is incorrect.
  */
-static uint8_t eccverify(uint64_t data, uint8_t ecc)
+static enum eccbitfields eccverify(uint64_t data, uint8_t ecc)
 {
 	return syndromematrix[eccgenerate(data) ^ ecc];
 }
@@ -142,15 +158,14 @@ static inline uint64_t eccflipbit(uint64_t data, uint8_t bit)
  * @dst:	destination buffer without ECC
  * @src:	source buffer with ECC
  * @len:	number of bytes of data to copy (without ecc).
-                     Must be 8 byte aligned.
+ *                   Must be 8 byte aligned.
  *
- * @return:	eccBitfield or 0-64.
+ * @return:	Success or error
  *
- * @retval GD - Data is good.
- * @retval UE - Data is uncorrectable.
- * @retval all others - which bit was corrected.
+ * @retval: 0 - success
+ * @retfal: other - fail
  */
-uint8_t memcpy_from_ecc(uint64_t *dst, struct ecc64 *src, uint32_t len)
+int memcpy_from_ecc(uint64_t *dst, struct ecc64 *src, uint32_t len)
 {
 	beint64_t data;
 	uint8_t ecc;
@@ -161,7 +176,7 @@ uint8_t memcpy_from_ecc(uint64_t *dst, struct ecc64 *src, uint32_t len)
 		/* TODO: we could probably handle this */
 		FL_ERR("ECC data length must be 8 byte aligned length:%i\n",
 			len);
-		return UE;
+		return -1;
 	}
 
 	/* Handle in chunks of 8 bytes, so adjust the length */
@@ -184,7 +199,7 @@ uint8_t memcpy_from_ecc(uint64_t *dst, struct ecc64 *src, uint32_t len)
 			*dst = (uint64_t)be64_to_cpu(eccflipbit(be64_to_cpu(data), badbit));
 		dst++;
 	}
-	return GD;
+	return 0;
 }
 
 /**
@@ -194,23 +209,23 @@ uint8_t memcpy_from_ecc(uint64_t *dst, struct ecc64 *src, uint32_t len)
  * @src:	source buffer without ECC
  * @len:	number of bytes of data to copy (without ecc, length of src).
  *       Note: dst must be big enough to hold ecc bytes as well.
-                     Must be 8 byte aligned.
+ *                   Must be 8 byte aligned.
  *
- * @return:	eccBitfield.
+ * @return:	success or failure
  *
- * @retval GD - Success.
- * @retval UE - Length is not 8 byte aligned.
+ * @retval: 0 - success
+ * @retfal: other - fail
  */
-uint8_t memcpy_to_ecc(struct ecc64 *dst, const uint64_t *src, uint32_t len)
+int memcpy_to_ecc(struct ecc64 *dst, const uint64_t *src, uint32_t len)
 {
 	struct ecc64 ecc_word;
 	uint32_t i;
 
 	if (len & 0x7) {
-		/* TODO: we could problably handle this */
+		/* TODO: we could probably handle this */
 		FL_ERR("Data to add ECC bytes to must be 8 byte aligned length: %i\n",
 				len);
-		return UE;
+		return -1;
 	}
 
 	/* Handle in chunks of 8 bytes, so adjust the length */
@@ -223,5 +238,5 @@ uint8_t memcpy_to_ecc(struct ecc64 *dst, const uint64_t *src, uint32_t len)
 		*(dst + i) = ecc_word;
 	}
 
-	return GD;
+	return 0;
 }
diff --git a/libflash/ecc.h b/libflash/ecc.h
index d58ec3e..1e14724 100644
--- a/libflash/ecc.h
+++ b/libflash/ecc.h
@@ -22,29 +22,14 @@
 #include <stdint.h>
 #include <ccan/endian/endian.h>
 
-/* Bit field identifiers for syndrome calculations. */
-enum eccbitfields
-{
-        GD = 0xff,      //< Good, ECC matches.
-        UE = 0xfe,      //< Uncorrectable.
-        E0 = 71,        //< Error in ECC bit 0
-        E1 = 70,        //< Error in ECC bit 1
-        E2 = 69,        //< Error in ECC bit 2
-        E3 = 68,        //< Error in ECC bit 3
-        E4 = 67,        //< Error in ECC bit 4
-        E5 = 66,        //< Error in ECC bit 5
-        E6 = 65,        //< Error in ECC bit 6
-        E7 = 64         //< Error in ECC bit 7
-};
-
 struct ecc64 {
 	beint64_t data;
 	uint8_t ecc;
 } __attribute__((__packed__));
 
-extern uint8_t memcpy_from_ecc(uint64_t *dst, struct ecc64 *src, uint32_t len);
+extern int memcpy_from_ecc(uint64_t *dst, struct ecc64 *src, uint32_t len);
 
-extern uint8_t memcpy_to_ecc(struct ecc64 *dst, const uint64_t *src, uint32_t len);
+extern int memcpy_to_ecc(struct ecc64 *dst, const uint64_t *src, uint32_t len);
 
 /*
  * Calculate the size of a buffer if ECC is added
@@ -56,9 +41,26 @@ extern uint8_t memcpy_to_ecc(struct ecc64 *dst, const uint64_t *src, uint32_t le
 #define ALIGN_UP(_v, _a)	(((_v) + (_a) - 1) & ~((_a) - 1))
 #endif
 
-#define ECC_SIZE(len) (ALIGN_UP((len), 8) >> 3)
-#define ECC_BUFFER_SIZE(len) (ALIGN_UP((len), 8) + ECC_SIZE(len))
-#define ECC_BUFFER_SIZE_CHECK(len) ((len) % 9)
-#define BUFFER_SIZE_MINUS_ECC(len) ((len) * 8 / 9)
+#define BYTES_PER_ECC 8
+
+static inline uint32_t ecc_size(uint32_t len)
+{
+	return ALIGN_UP(len, BYTES_PER_ECC) >> 3;
+}
+
+static inline uint32_t ecc_buffer_size(uint32_t len)
+{
+	return ALIGN_UP(len, BYTES_PER_ECC) + ecc_size(len);
+}
+
+static inline int ecc_buffer_size_check(uint32_t len)
+{
+	return len % (BYTES_PER_ECC + 1);
+}
+
+static inline uint32_t ecc_buffer_size_minus_ecc(uint32_t len)
+{
+	return len * BYTES_PER_ECC / (BYTES_PER_ECC + 1);
+}
 
 #endif
diff --git a/libflash/libflash.c b/libflash/libflash.c
index e7b3c8e..a142e17 100644
--- a/libflash/libflash.c
+++ b/libflash/libflash.c
@@ -153,7 +153,7 @@ int flash_read_corrected(struct blocklevel_device *bl, uint32_t pos, void *buf,
 		return flash_read(bl, pos, buf, len);
 
 	/* Copy the buffer in chunks */
-	bufecc = malloc(ECC_BUFFER_SIZE(COPY_BUFFER_LENGTH));
+	bufecc = malloc(ecc_buffer_size(COPY_BUFFER_LENGTH));
 	if (!bufecc)
 		return FLASH_ERR_MALLOC_FAILED;
 
@@ -162,13 +162,13 @@ int flash_read_corrected(struct blocklevel_device *bl, uint32_t pos, void *buf,
 		copylen = MIN(len, COPY_BUFFER_LENGTH);
 
 		/* Read ECCed data from flash */
-		rc = flash_read(bl, pos, bufecc, ECC_BUFFER_SIZE(copylen));
+		rc = flash_read(bl, pos, bufecc, ecc_buffer_size(copylen));
 		if (rc)
 			goto err;
 
 		/* Extract data from ECCed data */
 		ret = memcpy_from_ecc(buf, bufecc, copylen);
-		if (ret == UE) {
+		if (ret) {
 			rc = FLASH_ERR_ECC_INVALID;
 			goto err;
 		}
@@ -176,7 +176,7 @@ int flash_read_corrected(struct blocklevel_device *bl, uint32_t pos, void *buf,
 		/* Update for next copy */
 		len -= copylen;
 		buf = (uint8_t *)buf + copylen;
-		pos += ECC_BUFFER_SIZE(copylen);
+		pos += ecc_buffer_size(copylen);
 	}
 
 	rc = 0;
@@ -397,7 +397,7 @@ int flash_write_corrected(struct blocklevel_device *bl, uint32_t pos, const void
 		uint32_t len, bool verify, bool ecc)
 {
 	struct ecc64 *bufecc;
-	uint32_t copylen;
+	uint32_t copylen, copylen_minus_ecc;
 	int rc;
 	uint8_t ret;
 
@@ -405,17 +405,18 @@ int flash_write_corrected(struct blocklevel_device *bl, uint32_t pos, const void
 		return flash_write(bl, pos, buf, len, verify);
 
 	/* Copy the buffer in chunks */
-	bufecc = malloc(ECC_BUFFER_SIZE(COPY_BUFFER_LENGTH));
+	bufecc = malloc(ecc_buffer_size(COPY_BUFFER_LENGTH));
 	if (!bufecc)
 		return FLASH_ERR_MALLOC_FAILED;
 
 	while (len > 0) {
 		/* What's left to copy? */
 		copylen = MIN(len, COPY_BUFFER_LENGTH);
+		copylen_minus_ecc = ecc_buffer_size_minus_ecc(copylen);
 
 		/* Add the ecc byte to the data */
-		ret = memcpy_to_ecc(bufecc, buf, BUFFER_SIZE_MINUS_ECC(copylen));
-		if (ret == UE) {
+		ret = memcpy_to_ecc(bufecc, buf, copylen_minus_ecc);
+		if (ret) {
 			rc = FLASH_ERR_ECC_INVALID;
 			goto err;
 		}
@@ -426,8 +427,8 @@ int flash_write_corrected(struct blocklevel_device *bl, uint32_t pos, const void
 			goto err;
 
 		/* Update for next copy */
-		len -= BUFFER_SIZE_MINUS_ECC(copylen);
-		buf = (uint8_t *)buf + BUFFER_SIZE_MINUS_ECC(copylen);
+		len -= copylen_minus_ecc;
+		buf = (uint8_t *)buf + copylen_minus_ecc;
 		pos += copylen;
 	}
 
@@ -554,17 +555,17 @@ int flash_smart_write_corrected(struct blocklevel_device *bl, uint32_t dst, cons
 	if (!ecc)
 		return flash_smart_write(bl, dst, src, size);
 
-	buf = malloc(ECC_BUFFER_SIZE(size));
+	buf = malloc(ecc_buffer_size(size));
 	if (!buf)
 		return FLASH_ERR_MALLOC_FAILED;
 
 	rc = memcpy_to_ecc(buf, src, size);
-	if (rc != GD) {
+	if (rc) {
 		rc = FLASH_ERR_ECC_INVALID;
 		goto out;
 	}
 
-	rc = flash_smart_write(bl, dst, buf, ECC_BUFFER_SIZE(size));
+	rc = flash_smart_write(bl, dst, buf, ecc_buffer_size(size));
 
 out:
 	free(buf);
diff --git a/libflash/test/test-ecc.c b/libflash/test/test-ecc.c
index 8bbce9d..9d5a43a 100644
--- a/libflash/test/test-ecc.c
+++ b/libflash/test/test-ecc.c
@@ -389,7 +389,7 @@ int main(void)
 	printf("Testing bitflip recovery\n");
 	for (i = 0; i < 64; i++) {
 		ret_memcpy = memcpy_from_ecc(&dst, &ecc_data[i], 8);
-		if (dst != 0xffffffffffffffff || ret_memcpy != GD) {
+		if (dst != 0xffffffffffffffff || ret_memcpy) {
 			ERR("ECC code didn't correct bad bit %d in 0x%016lx\n", 63 - i, be64toh(ecc_data[i].data));
 			exit(1);
 		}
@@ -412,7 +412,7 @@ int main(void)
 	/* Test a large memcpy */
 	printf("Testing a large(ish) memcpy_from_ecc()\n");
 	ret_memcpy = memcpy_from_ecc(buf, ecc_data, NUM_ECC_ROWS * sizeof(*buf));
-	if (ret_memcpy != GD) {
+	if (ret_memcpy) {
 		ERR("ECC Couldn't memcpy entire buffer\n");
 		exit(1);
 	}
@@ -436,14 +436,14 @@ int main(void)
 
 	/* Test a memcpy to add ecc data */
 	printf("Testing a large(ish) memcpy_to_ecc()\n");
-	ret_buf = malloc(ECC_BUFFER_SIZE(NUM_ECC_ROWS * sizeof(*buf)));
+	ret_buf = malloc(ecc_buffer_size(NUM_ECC_ROWS * sizeof(*buf)));
 	if (!buf) {
 		ERR("malloc #2 failed during ecc test\n");
 		exit(1);
 	}
 
 	ret_memcpy = memcpy_to_ecc(ret_buf, buf, NUM_ECC_ROWS * sizeof(*buf));
-	if (ret_memcpy != GD) {
+	if (ret_memcpy) {
 		ERR("ECC Couldn't memcpy entire buffer\n");
 		exit(1);
 	}
@@ -466,20 +466,20 @@ int main(void)
 	printf("ECC tests pass\n");
 
 	printf("ECC test error conditions\n");
-	if (memcpy_to_ecc(ret_buf, buf, 7) != UE) {
+	if (memcpy_to_ecc(ret_buf, buf, 7) == 0) {
 		ERR("memcpy_to_ecc didn't detect bad size 7\n");
 		exit(1);
 	}
 
-	if (memcpy_to_ecc(ret_buf, buf, 15) != UE) {
+	if (memcpy_to_ecc(ret_buf, buf, 15) == 0) {
 		ERR("memcpy_to_ecc didn't detect bad size 15\n");
 		exit(1);
 	}
-	if (memcpy_from_ecc(buf, ret_buf, 7) != UE) {
+	if (memcpy_from_ecc(buf, ret_buf, 7) == 0) {
 		ERR("memcpy_from_ecc didn't detect bad size 7\n");
 		exit(1);
 	}
-	if (memcpy_from_ecc(buf, ret_buf, 15) != UE) {
+	if (memcpy_from_ecc(buf, ret_buf, 15) == 0) {
 		ERR("memcpy_from_ecc didn't detect bad size 15\n");
 		exit(1);
 	}