[U-Boot] [PATCH v3 1/4] nand: Extend nand_(read|write)_skip_bad with *actual and limit parameters
Tom Rini
trini at ti.com
Fri Mar 1 16:57:40 CET 2013
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>
More information about the U-Boot
mailing list