[Skiboot] [PATCH V2 1/7] libflash/ecc: Simplify and cleanup ecc code.
Alistair Popple
alistair at popple.id.au
Mon Jun 22 18:18:47 AEST 2015
Makes it look a bit more readable. Thanks!
Reviewed-by: Alistair Popple <alistair at popple.id.au>
On Tue, 16 Jun 2015 19:01:04 Cyril Bur wrote:
> 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.
>
> Signed-off-by: Cyril Bur <cyril.bur at au1.ibm.com>
> ---
> core/flash.c | 10 +++++-----
> external/gard/gard.c | 2 +-
> libflash/ecc.c | 51 +++++++++++++++++++++++++++++++-----------------
> libflash/ecc.h | 44 +++++++++++++++++++++--------------------
> libflash/libflash.c | 27 +++++++++++++------------
> libflash/test/test-ecc.c | 16 +++++++--------
> 6 files changed, 84 insertions(+), 66 deletions(-)
>
> diff --git a/core/flash.c b/core/flash.c
> index 7cd9153..c65aefa 100644
> --- a/core/flash.c
> +++ b/core/flash.c
> @@ -418,7 +418,7 @@ static int flash_find_subpartition(struct blocklevel_device *bl, uint32_t subid,
> /* Get raw partition size without ECC */
> partsize = *total_size;
> if (ecc)
> - partsize = BUFFER_SIZE_MINUS_ECC(*total_size);
> + partsize = ecc_buffer_size_minus_ecc(*total_size);
>
> /* Get the TOC */
> rc = flash_read_corrected(bl, *start, header,
> @@ -483,8 +483,8 @@ static int flash_find_subpartition(struct blocklevel_device *bl, uint32_t subid,
> *start += offset;
> *total_size = size;
> if (ecc) {
> - *start += ECC_SIZE(offset);
> - *total_size += ECC_SIZE(size);
> + *start += ecc_size(offset);
> + *total_size += ecc_size(size);
> }
> rc = 0;
> goto end;
> @@ -577,12 +577,12 @@ static int flash_load_resource(enum resource_id id, uint32_t subid,
> /* Work out what the final size of buffer will be without ECC */
> size = part_size;
> if (ecc) {
> - if ECC_BUFFER_SIZE_CHECK(part_size) {
> + if (ecc_buffer_size_check(part_size)) {
> prerror("FLASH: %s image invalid size for ECC %d\n",
> name, part_size);
> goto out_free_ffs;
> }
> - size = BUFFER_SIZE_MINUS_ECC(part_size);
> + size = ecc_buffer_size_minus_ecc(part_size);
> }
>
> if (size > *len) {
> diff --git a/external/gard/gard.c b/external/gard/gard.c
> index c43e05a..864c500 100644
> --- a/external/gard/gard.c
> +++ b/external/gard/gard.c
> @@ -63,7 +63,7 @@ struct gard_ctx {
> */
> static inline size_t sizeof_gard(struct gard_ctx *ctx)
> {
> - return ctx->ecc ? ECC_BUFFER_SIZE(sizeof(struct gard_record)) : sizeof(struct gard_record);
> + return ctx->ecc ? ecc_buffer_size(sizeof(struct gard_record)) : sizeof(struct gard_record);
> }
>
> static void show_flash_err(int rc)
> diff --git a/libflash/ecc.c b/libflash/ecc.c
> index 8ab1f31..b3c49cf 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);
> }
>
More information about the Skiboot
mailing list