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

Submitted by Brian Norris on June 16, 2010, 4:42 p.m.

Details

Message ID 1276706527-4563-1-git-send-email-norris@broadcom.com
State New, archived
Headers show

Commit Message

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(-)

Comments

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 hide | download patch | download mbox

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);