[U-Boot] [PATCH v3 1/4] nand: Extend nand_(read|write)_skip_bad with *actual and limit parameters

[Tom Rini]

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 at antoniou-consulting.com>
> >Signed-off-by: Tom Rini <trini at 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 NULL
> 
> s/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 buffer
> 
> s/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;
> > 		return -EINVAL;
> > 	}
> 
> 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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130301/706cb408/attachment.pgp>