| Message ID | 1424392679-27859-10-git-send-email-mikey@neuling.org |
|---|---|
| State | Accepted |
| Headers | show |
On Fri, Feb 20, 2015 at 11:07 AM, Michael Neuling <mikey@neuling.org> wrote: > Add ffs_flash_read() which mimics flash_read() but handles ECC checking, > correction and uncorrectable errors. Shouldn't this be in libflash? It doesn't take a struct ffs_handle, and doesn't need anything in libffs.h afaict. The implementation looks okay. A comment about block size aligned reads below. > > Signed-off-by: Michael Neuling <mikey@neuling.org> > --- > libflash/libffs.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > libflash/libffs.h | 4 ++++ > libflash/libflash.h | 1 + > 3 files changed, 57 insertions(+) > > diff --git a/libflash/libffs.c b/libflash/libffs.c > index cfd5456..c28fd66 100644 > --- a/libflash/libffs.c > +++ b/libflash/libffs.c > +int ffs_flash_read(struct flash_chip *c, uint32_t pos, void *buf, uint32_t len, > + bool ecc) > +{ > + uint64_t *bufecc; > + uint32_t copylen; > + int rc; > + uint8_t ret; > + > + if (!ecc) > + return flash_read(c, pos, buf, len); > + > + /* Copy the buffer in chunks */ > + 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); > + Is there any code to ensure that copylen is going to be flash page size aligned? Similar with pos. > + /* Read ECCed data from flash */ > + rc = flash_read(c, pos, bufecc, ECC_BUFFER_SIZE(copylen)); > + if (rc) > + goto err;
On Fri, 2015-02-20 at 14:27 +1030, Joel Stanley wrote: > On Fri, Feb 20, 2015 at 11:07 AM, Michael Neuling <mikey@neuling.org> wrote: > > Add ffs_flash_read() which mimics flash_read() but handles ECC checking, > > correction and uncorrectable errors. > > Shouldn't this be in libflash? It doesn't take a struct ffs_handle, > and doesn't need anything in libffs.h afaict. We can move it if you like. I don't have a strong opinion. where it goes. > The implementation looks okay. A comment about block size aligned reads below. > > > > Signed-off-by: Michael Neuling <mikey@neuling.org> > > --- > > libflash/libffs.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > libflash/libffs.h | 4 ++++ > > libflash/libflash.h | 1 + > > 3 files changed, 57 insertions(+) > > > > diff --git a/libflash/libffs.c b/libflash/libffs.c > > index cfd5456..c28fd66 100644 > > --- a/libflash/libffs.c > > +++ b/libflash/libffs.c > > > +int ffs_flash_read(struct flash_chip *c, uint32_t pos, void *buf, uint32_t len, > > + bool ecc) > > +{ > > + uint64_t *bufecc; > > + uint32_t copylen; > > + int rc; > > + uint8_t ret; > > + > > + if (!ecc) > > + return flash_read(c, pos, buf, len); > > + > > + /* Copy the buffer in chunks */ > > + 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); > > + > > Is there any code to ensure that copylen is going to be flash page > size aligned? Similar with pos. I don't think we need to since pos and len can be passed to the flash_read() call above anyway. This is just a drop in replacement for flash_read(). But maybe I'm missing something? Mikey > > + /* Read ECCed data from flash */ > > + rc = flash_read(c, pos, bufecc, ECC_BUFFER_SIZE(copylen)); > > + if (rc) > > + goto err; >
On Fri, 2015-02-20 at 14:27 +1030, Joel Stanley wrote: > On Fri, Feb 20, 2015 at 11:07 AM, Michael Neuling <mikey@neuling.org> wrote: > > Add ffs_flash_read() which mimics flash_read() but handles ECC checking, > > correction and uncorrectable errors. > > Shouldn't this be in libflash? It doesn't take a struct ffs_handle, > and doesn't need anything in libffs.h afaict. > > The implementation looks okay. A comment about block size aligned reads below. That or in a separate libflash_ecc ... Cheers, Ben. > > > > Signed-off-by: Michael Neuling <mikey@neuling.org> > > --- > > libflash/libffs.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > libflash/libffs.h | 4 ++++ > > libflash/libflash.h | 1 + > > 3 files changed, 57 insertions(+) > > > > diff --git a/libflash/libffs.c b/libflash/libffs.c > > index cfd5456..c28fd66 100644 > > --- a/libflash/libffs.c > > +++ b/libflash/libffs.c > > > +int ffs_flash_read(struct flash_chip *c, uint32_t pos, void *buf, uint32_t len, > > + bool ecc) > > +{ > > + uint64_t *bufecc; > > + uint32_t copylen; > > + int rc; > > + uint8_t ret; > > + > > + if (!ecc) > > + return flash_read(c, pos, buf, len); > > + > > + /* Copy the buffer in chunks */ > > + 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); > > + > > Is there any code to ensure that copylen is going to be flash page > size aligned? Similar with pos. > > > + /* Read ECCed data from flash */ > > + rc = flash_read(c, pos, bufecc, ECC_BUFFER_SIZE(copylen)); > > + if (rc) > > + goto err;
diff --git a/libflash/libffs.c b/libflash/libffs.c index cfd5456..c28fd66 100644 --- a/libflash/libffs.c +++ b/libflash/libffs.c @@ -18,6 +18,7 @@ #include <stdlib.h> #include <stdio.h> #include <string.h> +#include <ecc.h> #include <ccan/endian/endian.h> @@ -286,3 +287,54 @@ int ffs_update_act_size(struct ffs_handle *ffs, uint32_t part_idx, return 0; return flash_smart_write(ffs->chip, offset, ent, FFS_ENTRY_SIZE); } + +#define COPY_BUFFER_LENGTH 4096 + +/* + * This provides a wrapper around flash_read on ECCed data + * len is length of data without ECC attached + */ +int ffs_flash_read(struct flash_chip *c, uint32_t pos, void *buf, uint32_t len, + bool ecc) +{ + uint64_t *bufecc; + uint32_t copylen; + int rc; + uint8_t ret; + + if (!ecc) + return flash_read(c, pos, buf, len); + + /* Copy the buffer in chunks */ + 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); + + /* Read ECCed data from flash */ + rc = flash_read(c, pos, bufecc, ECC_BUFFER_SIZE(copylen)); + if (rc) + goto err; + + /* Extract data from ECCed data */ + ret = eccmemcpy(buf, bufecc, copylen); + if (ret == UE) { + rc = FLASH_ERR_ECC_INVALID; + goto err; + } + + /* Update for next copy */ + len -= copylen; + buf = (uint8_t *)buf + copylen; + pos += ECC_BUFFER_SIZE(copylen); + } + + return 0; + +err: + free(bufecc); + return rc; +} diff --git a/libflash/libffs.h b/libflash/libffs.h index 69e44bb..15ed3c5 100644 --- a/libflash/libffs.h +++ b/libflash/libffs.h @@ -32,6 +32,7 @@ struct ffs_handle; #define FFS_ERR_BAD_VERSION 101 #define FFS_ERR_BAD_CKSUM 102 #define FFS_ERR_PART_NOT_FOUND 103 +#define FFS_ERR_BAD_ECC 104 int ffs_open_flash(struct flash_chip *chip, uint32_t offset, uint32_t max_size, struct ffs_handle **ffs); @@ -52,5 +53,8 @@ int ffs_part_info(struct ffs_handle *ffs, uint32_t part_idx, int ffs_update_act_size(struct ffs_handle *ffs, uint32_t part_idx, uint32_t act_size); +int ffs_flash_read(struct flash_chip *c, uint32_t pos, void *buf, uint32_t len, + bool ecc); + #endif /* __LIBFFS_H */ diff --git a/libflash/libflash.h b/libflash/libflash.h index 31d3562..8e031dd 100644 --- a/libflash/libflash.h +++ b/libflash/libflash.h @@ -51,6 +51,7 @@ extern bool libflash_debug; #define FLASH_ERR_CHIP_ER_NOT_SUPPORTED 11 #define FLASH_ERR_CTRL_CMD_UNSUPPORTED 12 #define FLASH_ERR_CTRL_TIMEOUT 13 +#define FLASH_ERR_ECC_INVALID 14 /* Flash chip, opaque */ struct flash_chip;
Add ffs_flash_read() which mimics flash_read() but handles ECC checking, correction and uncorrectable errors. Signed-off-by: Michael Neuling <mikey@neuling.org> --- libflash/libffs.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++ libflash/libffs.h | 4 ++++ libflash/libflash.h | 1 + 3 files changed, 57 insertions(+)