On Thu, Feb 28, 2013 at 07:37:51PM -0600, Scott Wood wrote:
On 02/28/2013 01:09:05 PM, Tom Rini wrote:
We make these two functions take a size_t pointer to how much space was used on NAND to read or write the buffer (when reads/writes happen) so that bad blocks can be accounted for. We also make them take an loff_t limit on how much data can be read or written. This means that we can now catch the case of when writing to a partition would exceed the partition size due to bad blocks. To do this we also need to make check_skip_len count not just complete blocks used but partial ones as well. All callers of nand_(read|write)_skip_bad are adjusted to call these with the most sensible limits available.
The changes were started by Pantelis and finished by Tom.
Signed-off-by: Pantelis Antoniou panto@antoniou-consulting.com Signed-off-by: Tom Rini trini@ti.com
Changes in v3:
- Reworked skip_check_len changes to just add accounting for *used to
the logic.
- Allow for actual to be NULL in nand_(read|write)_skip_bad, only DFU
calls this with a non-NULL parameter. Make sure the comments for both functions explain the parameters and their behavior.
- Other style changes requested by Scott.
- As nand_(write|read)_skip_bad passes back just a used length now.
Changes in v2:
- NAND skip_check_len changes reworked to allow
nand_(read|write)_skip_bad to return this information to the caller.
common/cmd_nand.c | 56 +++++++++++++++++++---------------- common/env_nand.c | 3 +- drivers/mtd/nand/nand_util.c | 67 +++++++++++++++++++++++++++++++++++++----- include/nand.h | 4 +-- 4 files changed, 93 insertions(+), 37 deletions(-)
Looks mostly good, just some minor issues:
- if (*size > maxsize) {
puts("Size exceeds partition or device limit\n");return -1;- }
I assume you're removing this because you rely on the read/write functions printing the error... what about other users of this such as erase, lock, etc?
Ah true. And I don't see an easy way to make Harvey's patch cover limit exceeds request, so we'll need to keep the limit stuff, I'll go add this check back.
@@ -476,20 +483,30 @@ static size_t drop_ffs(const nand_info_t *nand, const u_char *buf,
- Write image to NAND flash.
- Blocks that are marked bad are skipped and the is written to
the next
- block instead as long as the image is short enough to fit even
after
- skipping the bad blocks.
- skipping the bad blocks. Due to bad blocks we may not be able to
- perform the requested write. In the case where the write would
- extend beyond the end of the NAND device, both length and
actual (if
- not NULL) are set to 0. In the case where the write would extend
- beyond the limit we are passed, length is set to 0 and actual
is set
- to the required length.
- @param nand NAND device
- @param offset offset in flash
- @param length buffer length
- @param actual set to size required to write length worth of
buffer or 0, if not NULLs/or 0/or 0 on error/ or s/or 0/in case of success/
The read function doesn't have the "or 0" comment...
I'll go reword.
- @param lim maximum size that length may be in order to not
exceed the buffers/that length may be/that actual may be/
No? We check that the requested length to something will fit even if the caller doesn't care to know what actual is.
- @param buffer buffer to read from
- @param flags flags modifying the behaviour of the write to
NAND
- @return 0 in case of success
*/ int nand_write_skip_bad(nand_info_t *nand, loff_t offset, size_t *length,
u_char *buffer, int flags)
size_t *actual, loff_t lim, u_char *buffer, int flags){ int rval = 0, blocksize; size_t left_to_write = *length;
- size_t used_for_write = 0; u_char *p_buffer = buffer; int need_skip;
@@ -526,16 +543,28 @@ int nand_write_skip_bad(nand_info_t *nand, loff_t offset, size_t *length, if ((offset & (nand->writesize - 1)) != 0) { printf("Attempt to write non page-aligned data\n"); *length = 0;
if (actual)*actual = 0;Again, what about the returns in the WITH_YAFFS_OOB section? Or if we document that "actual" is undefined for error returns we can not worry about this.
OK. Currently we don't set length to 0 on WITH_YAFFS_OOB errors, but we ought to. And we can deal with actual the same way.