Patchwork [U-Boot,3/4,v3] nand_util: drop trailing all-0xff pages if requested

login
register
mail settings
Submitter Ben Gardiner
Date May 24, 2011, 2:18 p.m.
Message ID <0ea56396c07f0fcb92175e7303f4bbda23724a00.1306246651.git.bengardiner@nanometrics.ca>
Download mbox | patch
Permalink /patch/97165/
State Changes Requested
Headers show

Comments

Ben Gardiner - May 24, 2011, 2:18 p.m.
Add a flag to nand_read_skip_bad() such that if true, any trailing
pages in an eraseblock whose contents are entirely 0xff will be
dropped.

The implementation is via a new drop_ffs() function which is
based on the function of the same name from the ubiformat
utility by Artem Bityutskiy.

This is as-per the reccomendations of the UBI FAQ [1]

Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>
CC: Artem Bityutskiy <dedekind1@gmail.com>
CC: Detlev Zundel <dzu@denx.de>

[1] http://www.linux-mtd.infradead.org/doc/ubi.html#L_flasher_algo

---

This behaviour was found to fix both UBI and JFFS2 images written to
cleanly erased NAND partitions on da850evm.

Changes since v2:
 * moved the copyright header addition of nand_util to this patch from
   patch 'cmd_nand: add nand write.trimffs command'
 * Did not add Detlev's Acked-by because of movement of copyright header
Changes since v1:
 * rebased to HEAD of git://git.denx.de/u-boot-nand-flash.git : ff7b4a0
   ("env_nand: zero-initialize variable nand_erase_options")
 * wrap the new functionality in a CONFIG_CMD_NAND_TRIMFFS ifdef to
   reduce size impact of new feature
---
 drivers/mtd/nand/nand_util.c |   35 ++++++++++++++++++++++++++++++++---
 include/nand.h               |    1 +
 2 files changed, 33 insertions(+), 3 deletions(-)
Detlev Zundel - May 27, 2011, 9:43 a.m.
Hi Ben,

> Add a flag to nand_read_skip_bad() such that if true, any trailing
> pages in an eraseblock whose contents are entirely 0xff will be
> dropped.
>
> The implementation is via a new drop_ffs() function which is
> based on the function of the same name from the ubiformat
> utility by Artem Bityutskiy.
>
> This is as-per the reccomendations of the UBI FAQ [1]
>
> Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>
> CC: Artem Bityutskiy <dedekind1@gmail.com>
> CC: Detlev Zundel <dzu@denx.de>
>
> [1] http://www.linux-mtd.infradead.org/doc/ubi.html#L_flasher_algo

Looks good -

Acked-by: Detlev Zundel <dzu@denx.de>

Cheers
  Detlev
Scott Wood - June 6, 2011, 8:58 p.m.
On Tue, May 24, 2011 at 10:18:36AM -0400, Ben Gardiner wrote:
> +#ifdef CONFIG_CMD_NAND_TRIMFFS
> +static size_t drop_ffs(const nand_info_t *nand, const u_char *buf,
> +			const size_t *len)
> +{
> +	size_t i, l = *len;
> +
> +	for (i = l - 1; i >= 0; i--)
> +		if (((const uint8_t *)buf)[i] != 0xFF)
> +			break;

This cast looks unnecessary.

> +	/* The resulting length must be aligned to the minimum flash I/O size */
> +	l = i + 1;
> +	l = (l + nand->writesize - 1) / nand->writesize;
> +	l *=  nand->writesize;
> +	return l;

We allow unaligned lengths (the rest of the page gets padded with 0xff,
see nand_do_page_write-ops).  The input length might be unaligned --
this adjustment could cause you to read beyond the end of the supplied
buffer.

> +}
> +#endif
> +
>  /**
>   * nand_write_skip_bad:
>   *
> @@ -499,7 +520,7 @@ int nand_write_skip_bad(nand_info_t *nand, loff_t offset, size_t *length,
>  		return -EINVAL;
>  	}
>  
> -	if (!need_skip) {
> +	if (!need_skip && !(flags & WITH_DROP_FFS)) {
>  		rval = nand_write (nand, offset, length, buffer);
>  		if (rval == 0)
>  			return 0;

Why not call drop_ffs before this point?

> @@ -512,7 +533,7 @@ int nand_write_skip_bad(nand_info_t *nand, loff_t offset, size_t *length,
>  
>  	while (left_to_write > 0) {
>  		size_t block_offset = offset & (nand->erasesize - 1);
> -		size_t write_size;
> +		size_t write_size, truncated_write_size;
>  
>  		WATCHDOG_RESET ();
>  
> @@ -558,7 +579,15 @@ int nand_write_skip_bad(nand_info_t *nand, loff_t offset, size_t *length,
>  		else
>  #endif
>  		{
> -			rval = nand_write (nand, offset, &write_size, p_buffer);
> +			truncated_write_size = write_size;
> +#ifdef CONFIG_CMD_NAND_TRIMFFS
> +			if (flags & WITH_DROP_FFS)
> +				truncated_write_size = drop_ffs(nand, p_buffer,
> +						&write_size);
> +#endif

What if both WITH_DROP_FFS and WITH_YAFFS_OOB are specified?

-Scott
Ben Gardiner - June 7, 2011, 1:09 p.m.
Hi Scott,

Thanks for the review.

On Mon, Jun 6, 2011 at 4:58 PM, Scott Wood <scottwood@freescale.com> wrote:
> On Tue, May 24, 2011 at 10:18:36AM -0400, Ben Gardiner wrote:
>> +#ifdef CONFIG_CMD_NAND_TRIMFFS
>> +static size_t drop_ffs(const nand_info_t *nand, const u_char *buf,
>> +                     const size_t *len)
>> +{
>> +     size_t i, l = *len;
>> +
>> +     for (i = l - 1; i >= 0; i--)
>> +             if (((const uint8_t *)buf)[i] != 0xFF)
>> +                     break;
>
> This cast looks unnecessary.

You're absolutely right. It will be gone in v4.

>> +     /* The resulting length must be aligned to the minimum flash I/O size */
>> +     l = i + 1;
>> +     l = (l + nand->writesize - 1) / nand->writesize;
>> +     l *=  nand->writesize;
>> +     return l;
>
> We allow unaligned lengths (the rest of the page gets padded with 0xff,
> see nand_do_page_write-ops).  The input length might be unaligned --
> this adjustment could cause you to read beyond the end of the supplied
> buffer.

Right. Sorry I missed that. In v4 I will drop also any trailling 0xff
which do not make-up a full page since they would be padded out to
trailing 0xff.

>> +}
>> +#endif
>> +
>>  /**
>>   * nand_write_skip_bad:
>>   *
>> @@ -499,7 +520,7 @@ int nand_write_skip_bad(nand_info_t *nand, loff_t offset, size_t *length,
>>               return -EINVAL;
>>       }
>>
>> -     if (!need_skip) {
>> +     if (!need_skip && !(flags & WITH_DROP_FFS)) {
>>               rval = nand_write (nand, offset, length, buffer);
>>               if (rval == 0)
>>                       return 0;
>
> Why not call drop_ffs before this point?

To achieve the desired effect, drop_ffs must be called on each
eraseblock sized chunk being written; so it seemed the simplest way
was to force a block-by-block pass with the !WITH_DROP_FFS to enter

  while (left_to_write > 0) {

I'll leave this as-is in v4.

>> @@ -512,7 +533,7 @@ int nand_write_skip_bad(nand_info_t *nand, loff_t offset, size_t *length,
>>
>>       while (left_to_write > 0) {
>>               size_t block_offset = offset & (nand->erasesize - 1);
>> -             size_t write_size;
>> +             size_t write_size, truncated_write_size;
>>
>>               WATCHDOG_RESET ();
>>
>> @@ -558,7 +579,15 @@ int nand_write_skip_bad(nand_info_t *nand, loff_t offset, size_t *length,
>>               else
>>  #endif
>>               {
>> -                     rval = nand_write (nand, offset, &write_size, p_buffer);
>> +                     truncated_write_size = write_size;
>> +#ifdef CONFIG_CMD_NAND_TRIMFFS
>> +                     if (flags & WITH_DROP_FFS)
>> +                             truncated_write_size = drop_ffs(nand, p_buffer,
>> +                                             &write_size);
>> +#endif
>
> What if both WITH_DROP_FFS and WITH_YAFFS_OOB are specified?

I didn't plan for that or intend for it to be supported.

Previous to my introduction of WITH_DROP_FFS; using the YAFFS oob mode
was mutually exclusive with the 'usual' way of writing. The
introduction of WITH_DROP_FFs respects this precedent.

If both flags were set 1) cmd_nand.c would need to be changed ( :) )
and 2) the WITH_YAFFS_OOB behaviour would override.

In v4 I will add a -EINVAL if WITH_YAFFS_OOB flag is used with any other flag.

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca
Scott Wood - June 7, 2011, 4:34 p.m.
On Tue, 7 Jun 2011 09:09:07 -0400
Ben Gardiner <bengardiner@nanometrics.ca> wrote:

> > Why not call drop_ffs before this point?
> 
> To achieve the desired effect, drop_ffs must be called on each
> eraseblock sized chunk being written; so it seemed the simplest way
> was to force a block-by-block pass with the !WITH_DROP_FFS to enter

Ah, I missed that it was within each erase block.

> In v4 I will add a -EINVAL if WITH_YAFFS_OOB flag is used with any other flag.

OK, I just didn't want it silently ignored, with no documentation of the
limitation.

-Scott
Ben Gardiner - June 14, 2011, 8:35 p.m.
This series adds a nand write variant which implements the procedure
reccomended in the UBI FAQ [1] of dropping trailing pages of eraseblocks
containing entirely 0xff's.

[1] http://www.linux-mtd.infradead.org/doc/ubi.html#L_flasher_algo

Changes since v3:
 * rebased to nand-flash/next where patches 1/4 and 2/4 [v3] were already
   applied
 * added patch 1/3 [v4] to treat WITH_YAFFS_OOB as a mode
 * renumbered 3/4 and 4/4 [v3] to 1/3 and 3/3 [v4], respectively
 * remove uneccessary cast
 * wrapped README at 80 columns
 * prevent access past input buffer end

Changes since v2:
 * dropped WITH_DEFAULTS in favour of '0'
 * moved copyright header to nand_util patch
 * added write.trimffs variant to README.nand

Changes since v1:
 * renamed to 'trimffs' from 'ubi'
 * wrapped the new feature in #ifdefs
 * don't make it default for jffs -- patch dropped
 * attribution of the drop_ffs() function from mtd-utils to Artem

Ben Gardiner (3):
  [v4] nand_util: treat WITH_YAFFS_OOB as a mode
  [v4] nand_util: drop trailing all-0xff pages if requested
  [v4] cmd_nand: add nand write.trimffs command

 common/cmd_nand.c            |   16 +++++++++++++++
 doc/README.nand              |   10 +++++++++
 drivers/mtd/nand/nand_util.c |   43 +++++++++++++++++++++++++++++++++++++++--
 include/nand.h               |    5 +++-
 4 files changed, 70 insertions(+), 4 deletions(-)
Scott Wood - June 15, 2011, 11:05 p.m.
On Tue, Jun 14, 2011 at 04:35:04PM -0400, Ben Gardiner wrote:
> This series adds a nand write variant which implements the procedure
> reccomended in the UBI FAQ [1] of dropping trailing pages of eraseblocks
> containing entirely 0xff's.
> 
> [1] http://www.linux-mtd.infradead.org/doc/ubi.html#L_flasher_algo
> 
> Changes since v3:
>  * rebased to nand-flash/next where patches 1/4 and 2/4 [v3] were already
>    applied
>  * added patch 1/3 [v4] to treat WITH_YAFFS_OOB as a mode
>  * renumbered 3/4 and 4/4 [v3] to 1/3 and 3/3 [v4], respectively
>  * remove uneccessary cast
>  * wrapped README at 80 columns
>  * prevent access past input buffer end

Applied to u-boot-nand-flash next

-Scott

Patch

diff --git a/drivers/mtd/nand/nand_util.c b/drivers/mtd/nand/nand_util.c
index 762ac53..82b41a0 100644
--- a/drivers/mtd/nand/nand_util.c
+++ b/drivers/mtd/nand/nand_util.c
@@ -11,6 +11,9 @@ 
  *		nandwrite.c by Steven J. Hill (sjhill@realitydiluted.com)
  *			       and Thomas Gleixner (tglx@linutronix.de)
  *
+ * Copyright (C) 2008 Nokia Corporation: drop_ffs() function by
+ * Artem Bityutskiy <dedekind1@gmail.com> from mtd-utils
+ *
  * See file CREDITS for list of people who contributed to this
  * project.
  *
@@ -436,6 +439,24 @@  static int check_skip_len(nand_info_t *nand, loff_t offset, size_t length)
 	return ret;
 }
 
+#ifdef CONFIG_CMD_NAND_TRIMFFS
+static size_t drop_ffs(const nand_info_t *nand, const u_char *buf,
+			const size_t *len)
+{
+	size_t i, l = *len;
+
+	for (i = l - 1; i >= 0; i--)
+		if (((const uint8_t *)buf)[i] != 0xFF)
+			break;
+
+	/* The resulting length must be aligned to the minimum flash I/O size */
+	l = i + 1;
+	l = (l + nand->writesize - 1) / nand->writesize;
+	l *=  nand->writesize;
+	return l;
+}
+#endif
+
 /**
  * nand_write_skip_bad:
  *
@@ -499,7 +520,7 @@  int nand_write_skip_bad(nand_info_t *nand, loff_t offset, size_t *length,
 		return -EINVAL;
 	}
 
-	if (!need_skip) {
+	if (!need_skip && !(flags & WITH_DROP_FFS)) {
 		rval = nand_write (nand, offset, length, buffer);
 		if (rval == 0)
 			return 0;
@@ -512,7 +533,7 @@  int nand_write_skip_bad(nand_info_t *nand, loff_t offset, size_t *length,
 
 	while (left_to_write > 0) {
 		size_t block_offset = offset & (nand->erasesize - 1);
-		size_t write_size;
+		size_t write_size, truncated_write_size;
 
 		WATCHDOG_RESET ();
 
@@ -558,7 +579,15 @@  int nand_write_skip_bad(nand_info_t *nand, loff_t offset, size_t *length,
 		else
 #endif
 		{
-			rval = nand_write (nand, offset, &write_size, p_buffer);
+			truncated_write_size = write_size;
+#ifdef CONFIG_CMD_NAND_TRIMFFS
+			if (flags & WITH_DROP_FFS)
+				truncated_write_size = drop_ffs(nand, p_buffer,
+						&write_size);
+#endif
+
+			rval = nand_write(nand, offset, &truncated_write_size,
+					p_buffer);
 			offset += write_size;
 			p_buffer += write_size;
 		}
diff --git a/include/nand.h b/include/nand.h
index b0a31b8..5c8a189 100644
--- a/include/nand.h
+++ b/include/nand.h
@@ -116,6 +116,7 @@  int nand_read_skip_bad(nand_info_t *nand, loff_t offset, size_t *length,
 		       u_char *buffer);
 
 #define WITH_YAFFS_OOB	(1 << 0) /* whether write with yaffs format */
+#define WITH_DROP_FFS	(1 << 1) /* drop trailing all-0xff pages */
 
 int nand_write_skip_bad(nand_info_t *nand, loff_t offset, size_t *length,
 			u_char *buffer, int flags);