Message ID | 21ad981db8c65d3f00be3d9a254cae78cf40b96d.1413878259.git.cyrille.pitchen@atmel.com |
---|---|
State | Changes Requested |
Headers | show |
On Tue, 2014-10-21 at 10:05 +0200, Cyrille Pitchen wrote: > This patch fixes __jffs2_dbg_dump_buffer(): this function prints a message > claiming to dump len bytes. However, depending on the start offset, the former > code drops up to 31 (JFFS2_BUFDUMP_BYTES_PER_LINE - 1) bytes. > > Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com> > --- > fs/jffs2/debug.c | 21 ++++++++------------- > 1 file changed, 8 insertions(+), 13 deletions(-) > > diff --git a/fs/jffs2/debug.c b/fs/jffs2/debug.c > index 07bd5bc..1b515b2 100644 > --- a/fs/jffs2/debug.c > +++ b/fs/jffs2/debug.c > @@ -736,30 +736,25 @@ void > __jffs2_dbg_dump_buffer(unsigned char *buf, int len, uint32_t offs) > { > int skip; > - int i; > + int i, pos; > > printk(JFFS2_DBG_MSG_PREFIX " dump from offset %#08x to offset %#08x (%x bytes).\n", > offs, offs + len, len); > - i = skip = offs % JFFS2_BUFDUMP_BYTES_PER_LINE; > + i = skip = offs & (JFFS2_BUFDUMP_BYTES_PER_LINE - 1); > offs = offs & ~(JFFS2_BUFDUMP_BYTES_PER_LINE - 1); > > - if (skip != 0) > - printk(JFFS2_DBG "%#08x: ", offs); > - > + printk(JFFS2_DBG "%#08x: ", offs); > while (skip--) > printk(" "); > > - while (i < len) { > - if ((i % JFFS2_BUFDUMP_BYTES_PER_LINE) == 0 && i != len -1) { > - if (i != 0) > - printk("\n"); > + for (pos = 0; pos < len; ++pos, ++i) { > + if (i == JFFS2_BUFDUMP_BYTES_PER_LINE) { > offs += JFFS2_BUFDUMP_BYTES_PER_LINE; > - printk(JFFS2_DBG "%0#8x: ", offs); > + printk(JFFS2_DBG "\n%0#8x: ", offs); > + i = 0; print_hex_dump would be better
Le 21/10/2014 10:17, Joe Perches a écrit : > On Tue, 2014-10-21 at 10:05 +0200, Cyrille Pitchen wrote: >> This patch fixes __jffs2_dbg_dump_buffer(): this function prints a message >> claiming to dump len bytes. However, depending on the start offset, the former >> code drops up to 31 (JFFS2_BUFDUMP_BYTES_PER_LINE - 1) bytes. >> >> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com> >> --- >> fs/jffs2/debug.c | 21 ++++++++------------- >> 1 file changed, 8 insertions(+), 13 deletions(-) >> >> diff --git a/fs/jffs2/debug.c b/fs/jffs2/debug.c >> index 07bd5bc..1b515b2 100644 >> --- a/fs/jffs2/debug.c >> +++ b/fs/jffs2/debug.c >> @@ -736,30 +736,25 @@ void >> __jffs2_dbg_dump_buffer(unsigned char *buf, int len, uint32_t offs) >> { >> int skip; >> - int i; >> + int i, pos; >> >> printk(JFFS2_DBG_MSG_PREFIX " dump from offset %#08x to offset %#08x (%x bytes).\n", >> offs, offs + len, len); >> - i = skip = offs % JFFS2_BUFDUMP_BYTES_PER_LINE; >> + i = skip = offs & (JFFS2_BUFDUMP_BYTES_PER_LINE - 1); >> offs = offs & ~(JFFS2_BUFDUMP_BYTES_PER_LINE - 1); >> >> - if (skip != 0) >> - printk(JFFS2_DBG "%#08x: ", offs); >> - >> + printk(JFFS2_DBG "%#08x: ", offs); >> while (skip--) >> printk(" "); >> >> - while (i < len) { >> - if ((i % JFFS2_BUFDUMP_BYTES_PER_LINE) == 0 && i != len -1) { >> - if (i != 0) >> - printk("\n"); >> + for (pos = 0; pos < len; ++pos, ++i) { >> + if (i == JFFS2_BUFDUMP_BYTES_PER_LINE) { >> offs += JFFS2_BUFDUMP_BYTES_PER_LINE; >> - printk(JFFS2_DBG "%0#8x: ", offs); >> + printk(JFFS2_DBG "\n%0#8x: ", offs); >> + i = 0; > > print_hex_dump would be better > > Hi Joe, thanks for your comment. Indeed using print_hex_dump() would be a good idea since it would avoid each driver to implement its own version of buffer dumps. Reading the source code of print_hex_dump(), the output format would change a little bit: the output would no longer be aligned to JFFS2_BUFDUMP_BYTES_PER_LINE boundary using leading spaces. If it's ok to change the output format it's good for me as well.
On Tue, 2014-10-21 at 11:47 +0200, Cyrille Pitchen wrote: > thanks for your comment. Indeed using print_hex_dump() would be a good idea > since it would avoid each driver to implement its own version of buffer dumps. > Reading the source code of print_hex_dump(), the output format would change a > little bit: > the output would no longer be aligned to JFFS2_BUFDUMP_BYTES_PER_LINE boundary > using leading spaces. > If it's ok to change the output format it's good for me as well. Changing format is OK, this is just a debugging cruft. But it is OK only if you test the changes. If you are unable to test the changes, it is better to avoid doing non-trivial changes.
Le 21/10/2014 11:54, Artem Bityutskiy a écrit : > On Tue, 2014-10-21 at 11:47 +0200, Cyrille Pitchen wrote: >> thanks for your comment. Indeed using print_hex_dump() would be a good idea >> since it would avoid each driver to implement its own version of buffer dumps. >> Reading the source code of print_hex_dump(), the output format would change a >> little bit: >> the output would no longer be aligned to JFFS2_BUFDUMP_BYTES_PER_LINE boundary >> using leading spaces. >> If it's ok to change the output format it's good for me as well. > > Changing format is OK, this is just a debugging cruft. But it is OK only > if you test the changes. If you are unable to test the changes, it is > better to avoid doing non-trivial changes. > The version provided in this patch was tested. However I don't know how to cope with the offs parameter of __jffs2_dbg_dump_buffer() if I replace the current source code by a call to print_hex_dump(). If I use DUMP_PREFIX_ADDRESS as prefix_type value, the printed offsets are relative to the address of buf in RAM, not to the hardware offset in my dataflash provided by the offs parameter. Also, if I use DUMP_PREFIX_OFFSET, the printed offsets start from 0 instead of offs value. I think keeping the right offsets in the output dump is helpfull.
diff --git a/fs/jffs2/debug.c b/fs/jffs2/debug.c index 07bd5bc..1b515b2 100644 --- a/fs/jffs2/debug.c +++ b/fs/jffs2/debug.c @@ -736,30 +736,25 @@ void __jffs2_dbg_dump_buffer(unsigned char *buf, int len, uint32_t offs) { int skip; - int i; + int i, pos; printk(JFFS2_DBG_MSG_PREFIX " dump from offset %#08x to offset %#08x (%x bytes).\n", offs, offs + len, len); - i = skip = offs % JFFS2_BUFDUMP_BYTES_PER_LINE; + i = skip = offs & (JFFS2_BUFDUMP_BYTES_PER_LINE - 1); offs = offs & ~(JFFS2_BUFDUMP_BYTES_PER_LINE - 1); - if (skip != 0) - printk(JFFS2_DBG "%#08x: ", offs); - + printk(JFFS2_DBG "%#08x: ", offs); while (skip--) printk(" "); - while (i < len) { - if ((i % JFFS2_BUFDUMP_BYTES_PER_LINE) == 0 && i != len -1) { - if (i != 0) - printk("\n"); + for (pos = 0; pos < len; ++pos, ++i) { + if (i == JFFS2_BUFDUMP_BYTES_PER_LINE) { offs += JFFS2_BUFDUMP_BYTES_PER_LINE; - printk(JFFS2_DBG "%0#8x: ", offs); + printk(JFFS2_DBG "\n%0#8x: ", offs); + i = 0; } - printk("%02x ", buf[i]); - - i += 1; + printk("%02x ", buf[pos]); } printk("\n");
This patch fixes __jffs2_dbg_dump_buffer(): this function prints a message claiming to dump len bytes. However, depending on the start offset, the former code drops up to 31 (JFFS2_BUFDUMP_BYTES_PER_LINE - 1) bytes. Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com> --- fs/jffs2/debug.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-)