Patchwork mtd-utils: formatting of odd-sized OOB in nanddump

login
register
mail settings
Submitter Brian Norris
Date June 16, 2010, 4:42 p.m.
Message ID <1276706527-4563-1-git-send-email-norris@broadcom.com>
Download mbox | patch
Permalink /patch/55913/
State New
Headers show

Comments

Brian Norris - June 16, 2010, 4:42 p.m.
Changed OOB output formatting in nanddump.
Most NAND devices have OOB regions with multiples of 16 bytes.
With newer devices, a 218-byte OOB does not split nicely into
lines of 16 bytes. This patch fixes the formatting, allowing a
last line of 10 bytes instead of 16.

Signed-off-by: Brian Norris <norris@broadcom.com>
---
 nanddump.c |   31 ++++++++++++++++++++++---------
 1 files changed, 22 insertions(+), 9 deletions(-)
Artem Bityutskiy - July 7, 2010, 2:07 p.m.
On Wed, 2010-06-16 at 09:42 -0700, Brian Norris wrote:
> +				if (i+15 < meminfo.oobsize) {
> +					/* Print 16 bytes */
> +					sprintf(pretty_buf, "  OOB Data: %02x %02x %02x %02x %02x %02x "
> +							"%02x %02x %02x %02x %02x %02x %02x %02x %02x %02x\n",
> +							oobbuf[i], oobbuf[i+1], oobbuf[i+2],
> +							oobbuf[i+3], oobbuf[i+4], oobbuf[i+5],
> +							oobbuf[i+6], oobbuf[i+7], oobbuf[i+8],
> +							oobbuf[i+9], oobbuf[i+10], oobbuf[i+11],
> +							oobbuf[i+12], oobbuf[i+13], oobbuf[i+14],
> +							oobbuf[i+15]);
> +					write(ofd, pretty_buf, 60);
> +				} else {
> +					/* Print 10 bytes */
> +					sprintf(pretty_buf, "  OOB Data: %02x %02x %02x %02x %02x %02x "
> +							"%02x %02x %02x %02x\n",
> +							oobbuf[i], oobbuf[i+1], oobbuf[i+2],
> +							oobbuf[i+3], oobbuf[i+4], oobbuf[i+5],
> +							oobbuf[i+6], oobbuf[i+7], oobbuf[i+8],
> +							oobbuf[i+9]);
> +					write(ofd, pretty_buf, 42);
> +				}

Why 10 bytes, why not 12 or 14?

I think it is better to just copy-paste-modify the kernel
print_hex_dump() and utilize it, instead of this ugly crocodile code ...
Brian Norris - July 7, 2010, 6:21 p.m.
> Why 10 bytes, why not 12 or 14?

As of now, the only OOB-size that's not a multiple of 16 supported in 
the other bits of ugly code is 218-bytes.

218 % 16 = 10

What's the point of checking for a "supported" page-size/OOB-size 
earlier if we're not gonna use that info? :)

> I think it is better to just copy-paste-modify the kernel
> print_hex_dump() and utilize it, instead of this ugly crocodile code ...

I'm looking at that, since it seems a better solution. Does anyone 
oppose including ASCII output in the "pretty" option? It's built in to 
the print_hex_dump() already, so it shouldn't be too difficult to adapt. 
Anyway, it seems that someone intended to print ASCII at some point 
(nanddump.c, line 77):

static bool             pretty_print = false;   // print nice in ascii
Artem Bityutskiy - July 8, 2010, 4:09 a.m.
Hi,

On Wed, 2010-07-07 at 11:21 -0700, Brian Norris wrote:
> > Why 10 bytes, why not 12 or 14?
> 
> As of now, the only OOB-size that's not a multiple of 16 supported in 
> the other bits of ugly code is 218-bytes.
> 
> 218 % 16 = 10
> 
> What's the point of checking for a "supported" page-size/OOB-size 
> earlier if we're not gonna use that info? :)

No point, as well as not much point introducing magic hard-coded
constant like 10 - the printing should be generic.
> 
> > I think it is better to just copy-paste-modify the kernel
> > print_hex_dump() and utilize it, instead of this ugly crocodile code ...
> 
> I'm looking at that, since it seems a better solution. Does anyone 
> oppose including ASCII output in the "pretty" option? It's built in to 
> the print_hex_dump() already, so it shouldn't be too difficult to adapt. 
> Anyway, it seems that someone intended to print ASCII at some point 
> (nanddump.c, line 77):
> 
> static bool             pretty_print = false;   // print nice in ascii

Well, it is better to make this configurable or not print ASCII, IMO,
because it potentially may break scripts which use nanddump.
Brian Norris - July 8, 2010, 8:50 p.m.
Hello,

This patch series includes a few different fixes. Patches 1, 2, 3,
and 6 are fixes to the nanddump utility, while 4 and 5 are style
and minor issue patches.

As per the recommendation, I adapted the linux kernel dump function for
nanddump (patch 2).

The patches are:

[PATCH 1/6] mtd-utils/nanddump.c: Increase max OOB size
[PATCH 2/6] mtd-utils/nanddump.c: Robust pretty hexdump
[PATCH 3/6] mtd-utils/nanddump.c: Add canonical (hex+ascii) flag
[PATCH 4/6] mtd-utils/mkfs.jffs2: fixed warnings
[PATCH 5/6] mtd-utils/nandtest.c: Fixed indentation
[PATCH 6/6] mtd-utils/nanddump.c: Add "forcebinary" flag

Feedback is welcome, especially on the feature additions of patches
2, 3 and 6.

Thanks,
Brian

Patch

diff --git a/nanddump.c b/nanddump.c
index e44ab36..190a653 100644
--- a/nanddump.c
+++ b/nanddump.c
@@ -381,15 +381,28 @@  int main(int argc, char * const argv[])
 			}
 
 			for (i = 0; i < meminfo.oobsize; i += 16) {
-				sprintf(pretty_buf, "  OOB Data: %02x %02x %02x %02x %02x %02x "
-						"%02x %02x %02x %02x %02x %02x %02x %02x %02x %02x\n",
-						oobbuf[i], oobbuf[i+1], oobbuf[i+2],
-						oobbuf[i+3], oobbuf[i+4], oobbuf[i+5],
-						oobbuf[i+6], oobbuf[i+7], oobbuf[i+8],
-						oobbuf[i+9], oobbuf[i+10], oobbuf[i+11],
-						oobbuf[i+12], oobbuf[i+13], oobbuf[i+14],
-						oobbuf[i+15]);
-				write(ofd, pretty_buf, 60);
+				/* Check if we have a full 16 bytes left in the OOB */
+				if (i+15 < meminfo.oobsize) {
+					/* Print 16 bytes */
+					sprintf(pretty_buf, "  OOB Data: %02x %02x %02x %02x %02x %02x "
+							"%02x %02x %02x %02x %02x %02x %02x %02x %02x %02x\n",
+							oobbuf[i], oobbuf[i+1], oobbuf[i+2],
+							oobbuf[i+3], oobbuf[i+4], oobbuf[i+5],
+							oobbuf[i+6], oobbuf[i+7], oobbuf[i+8],
+							oobbuf[i+9], oobbuf[i+10], oobbuf[i+11],
+							oobbuf[i+12], oobbuf[i+13], oobbuf[i+14],
+							oobbuf[i+15]);
+					write(ofd, pretty_buf, 60);
+				} else {
+					/* Print 10 bytes */
+					sprintf(pretty_buf, "  OOB Data: %02x %02x %02x %02x %02x %02x "
+							"%02x %02x %02x %02x\n",
+							oobbuf[i], oobbuf[i+1], oobbuf[i+2],
+							oobbuf[i+3], oobbuf[i+4], oobbuf[i+5],
+							oobbuf[i+6], oobbuf[i+7], oobbuf[i+8],
+							oobbuf[i+9]);
+					write(ofd, pretty_buf, 42);
+				}
 			}
 		} else
 			write(ofd, oobbuf, meminfo.oobsize);