[PATCH 2/2] ide-scsi: use print_hex_dump from <linux/kernel.h>

[PATCH 2/2] ide-scsi: use print_hex_dump from <linux/kernel.h>

[Denis Cheng]

these utilities implemented in lib/hexdump.c are more handy, please use this.

Cc: Randy Dunlap <randy.dunlap@oracle.com>
Signed-off-by: Denis Cheng <crquan@gmail.com>
---
there are still much other private hexdump implementations in the source,
which reinvent the wheel, we can find them through:

  $ grep -RsIn hexdump <kernel-src>
  ...

 drivers/scsi/ide-scsi.c |   18 ++++--------------
 1 files changed, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/scsi/ide-scsi.c b/drivers/scsi/ide-scsi.c
index 8d0244c..8f3fc1d 100644
--- a/drivers/scsi/ide-scsi.c
+++ b/drivers/scsi/ide-scsi.c
@@ -242,16 +242,6 @@ static void idescsi_output_buffers (ide_drive_t *drive, idescsi_pc_t *pc, unsign
 	}
 }
 
-static void hexdump(u8 *x, int len)
-{
-	int i;
-
-	printk("[ ");
-	for (i = 0; i < len; i++)
-		printk("%x ", x[i]);
-	printk("]\n");
-}
-
 static int idescsi_check_condition(ide_drive_t *drive, struct request *failed_command)
 {
 	idescsi_scsi_t *scsi = drive_to_idescsi(drive);
@@ -282,7 +272,7 @@ static int idescsi_check_condition(ide_drive_t *drive, struct request *failed_co
 	pc->scsi_cmd = ((idescsi_pc_t *) failed_command->special)->scsi_cmd;
 	if (test_bit(IDESCSI_LOG_CMD, &scsi->log)) {
 		printk ("ide-scsi: %s: queue cmd = ", drive->name);
-		hexdump(pc->c, 6);
+		print_hex_dump(KERN_DEBUG, "", DUMP_PREFIX_OFFSET, 16, 1, pc->c, 6, 1);
 	}
 	rq->rq_disk = scsi->disk;
 	return ide_do_drive_cmd(drive, rq, ide_preempt);
@@ -337,7 +327,7 @@ static int idescsi_end_request (ide_drive_t *drive, int uptodate, int nrsecs)
 		idescsi_pc_t *opc = (idescsi_pc_t *) rq->buffer;
 		if (log) {
 			printk ("ide-scsi: %s: wrap up check %lu, rst = ", drive->name, opc->scsi_cmd->serial_number);
-			hexdump(pc->buffer,16);
+			print_hex_dump(KERN_DEBUG, "", DUMP_PREFIX_OFFSET, 16, 1, pc->buffer, 16, 1);
 		}
 		memcpy((void *) opc->scsi_cmd->sense_buffer, pc->buffer, SCSI_SENSE_BUFFERSIZE);
 		kfree(pc->buffer);
@@ -816,10 +806,10 @@ static int idescsi_queue (struct scsi_cmnd *cmd,
 
 	if (test_bit(IDESCSI_LOG_CMD, &scsi->log)) {
 		printk ("ide-scsi: %s: que %lu, cmd = ", drive->name, cmd->serial_number);
-		hexdump(cmd->cmnd, cmd->cmd_len);
+		print_hex_dump(KERN_DEBUG, "", DUMP_PREFIX_OFFSET, 16, 1, cmd->cmnd, cmd->cmd_len, 1);
 		if (memcmp(pc->c, cmd->cmnd, cmd->cmd_len)) {
 			printk ("ide-scsi: %s: que %lu, tsl = ", drive->name, cmd->serial_number);
-			hexdump(pc->c, 12);
+			print_hex_dump(KERN_DEBUG, "", DUMP_PREFIX_OFFSET, 16, 1, pc->c, 12, 1);
 		}
 	}
 
-- 
1.5.3.5

Re: [PATCH 2/2] ide-scsi: use print_hex_dump from <linux/kernel.h>

[rae l]

On Nov 26, 2007 3:41 PM, Joe Perches <joe@perches.com> wrote:
> On Mon, 2007-11-26 at 15:16 +0800, Denis Cheng wrote:
> > diff --git a/drivers/scsi/ide-scsi.c b/drivers/scsi/ide-scsi.c
> > index 8d0244c..8f3fc1d 100644
> > --- a/drivers/scsi/ide-scsi.c
> > +++ b/drivers/scsi/ide-scsi.c
> > @@ -282,7 +272,7 @@ static int idescsi_check_condition(ide_drive_t *drive, struct request *failed_co
> >       pc->scsi_cmd = ((idescsi_pc_t *) failed_command->special)->scsi_cmd;
> >       if (test_bit(IDESCSI_LOG_CMD, &scsi->log)) {
> >               printk ("ide-scsi: %s: queue cmd = ", drive->name);
> > -             hexdump(pc->c, 6);
> > +             print_hex_dump(KERN_DEBUG, "", DUMP_PREFIX_OFFSET, 16, 1, pc->c, 6, 1);
> >       }
> >       rq->rq_disk = scsi->disk;
> >       return ide_do_drive_cmd(drive, rq, ide_preempt);
>
> Hi Denis.
>
> These aren't really equivalent.  You need to look at the
> line above to determine if a KERN_ prefix needs to be
> used at all.
>
> You should probably use print_hex_dump_bytes here.
I know this is different from the original hexdump in ide-scsi.c, I
just want to tell someone that there's a good implementation of
hexdump in kernel.h, and I think the default KERN_DEBUG and
print_hex_dump is more informative and has better output. However,
anyone want more precise control on debug message could make her/his
improvements with print_hex_dump.

>
> cheers, Joe
>
>

-- 
Denis Cheng

Re: [PATCH 2/2] ide-scsi: use print_hex_dump from <linux/kernel.h>

[Matthew Wilcox]

On Mon, Nov 26, 2007 at 04:37:50PM +0800, rae l wrote:
> I know this is different from the original hexdump in ide-scsi.c, I
> just want to tell someone that there's a good implementation of
> hexdump in kernel.h, and I think the default KERN_DEBUG and
> print_hex_dump is more informative and has better output. However,
> anyone want more precise control on debug message could make her/his
> improvements with print_hex_dump.

using KERN_DEBUG is wrong -- this is part of a line, so you need to use
KERN_NONE or simply "".

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH 2/2] ide-scsi: use print_hex_dump from <linux/kernel.h>

[Randy Dunlap]

Matthew Wilcox wrote:
> On Mon, Nov 26, 2007 at 04:37:50PM +0800, rae l wrote:
>> I know this is different from the original hexdump in ide-scsi.c, I
>> just want to tell someone that there's a good implementation of
>> hexdump in kernel.h, and I think the default KERN_DEBUG and
>> print_hex_dump is more informative and has better output. However,
>> anyone want more precise control on debug message could make her/his
>> improvements with print_hex_dump.
> 
> using KERN_DEBUG is wrong -- this is part of a line, so you need to use
> KERN_NONE or simply "".

s/KERN_NONE/KERN_CONT/ for continuation lines.

-- 
~Randy

Re: [PATCH 2/2] ide-scsi: use print_hex_dump from <linux/kernel.h>

[Andrew Morton]

On Mon, 26 Nov 2007 15:16:13 +0800 Denis Cheng <crquan@gmail.com> wrote:

> these utilities implemented in lib/hexdump.c are more handy, please use this.
> 
> ...
>
> --- a/drivers/scsi/ide-scsi.c
> +++ b/drivers/scsi/ide-scsi.c
> @@ -242,16 +242,6 @@ static void idescsi_output_buffers (ide_drive_t *drive, idescsi_pc_t *pc, unsign
>  	}
>  }
>  
> -static void hexdump(u8 *x, int len)
> -{
> -	int i;
> -
> -	printk("[ ");
> -	for (i = 0; i < len; i++)
> -		printk("%x ", x[i]);
> -	printk("]\n");
> -}
> -
>  static int idescsi_check_condition(ide_drive_t *drive, struct request *failed_command)
>  {
>  	idescsi_scsi_t *scsi = drive_to_idescsi(drive);
> @@ -282,7 +272,7 @@ static int idescsi_check_condition(ide_drive_t *drive, struct request *failed_co
>  	pc->scsi_cmd = ((idescsi_pc_t *) failed_command->special)->scsi_cmd;
>  	if (test_bit(IDESCSI_LOG_CMD, &scsi->log)) {
>  		printk ("ide-scsi: %s: queue cmd = ", drive->name);
> -		hexdump(pc->c, 6);
> +		print_hex_dump(KERN_DEBUG, "", DUMP_PREFIX_OFFSET, 16, 1, pc->c, 6, 1);
>  	}
>  	rq->rq_disk = scsi->disk;
>  	return ide_do_drive_cmd(drive, rq, ide_preempt);
> @@ -337,7 +327,7 @@ static int idescsi_end_request (ide_drive_t *drive, int uptodate, int nrsecs)
>  		idescsi_pc_t *opc = (idescsi_pc_t *) rq->buffer;
>  		if (log) {
>  			printk ("ide-scsi: %s: wrap up check %lu, rst = ", drive->name, opc->scsi_cmd->serial_number);
> -			hexdump(pc->buffer,16);
> +			print_hex_dump(KERN_DEBUG, "", DUMP_PREFIX_OFFSET, 16, 1, pc->buffer, 16, 1);
>  		}
>  		memcpy((void *) opc->scsi_cmd->sense_buffer, pc->buffer, SCSI_SENSE_BUFFERSIZE);
>  		kfree(pc->buffer);
> @@ -816,10 +806,10 @@ static int idescsi_queue (struct scsi_cmnd *cmd,
>  
>  	if (test_bit(IDESCSI_LOG_CMD, &scsi->log)) {
>  		printk ("ide-scsi: %s: que %lu, cmd = ", drive->name, cmd->serial_number);
> -		hexdump(cmd->cmnd, cmd->cmd_len);
> +		print_hex_dump(KERN_DEBUG, "", DUMP_PREFIX_OFFSET, 16, 1, cmd->cmnd, cmd->cmd_len, 1);
>  		if (memcmp(pc->c, cmd->cmnd, cmd->cmd_len)) {
>  			printk ("ide-scsi: %s: que %lu, tsl = ", drive->name, cmd->serial_number);
> -			hexdump(pc->c, 12);
> +			print_hex_dump(KERN_DEBUG, "", DUMP_PREFIX_OFFSET, 16, 1, pc->c, 12, 1);
>  		}
>  	}
>  

Would you believe that this patch (which removes code) actually increases
drivers/scsi/ide-scsi.o .text by 75 bytes?

I didn't look to see why - probably that huge arg count is hurting,
possibly some additional strings being emitted?

Either way, perhaps a simple little front-end to print_hex_dump() is called
for.

Re: [PATCH 2/2] ide-scsi: use print_hex_dump from <linux/kernel.h>

[Joe Perches]

On Tue, 2007-11-27 at 01:31 -0800, Andrew Morton wrote:
> Either way, perhaps a simple little front-end to print_hex_dump() is called
> for.

Perhaps an even simpler generic version of what's
now used in crypto/tcrypt.c:

void print_cont_hex_dump_bytes(const void *buf, size_t len)
{
	print_hex_dump(KERN_CONT, "", DUMP_PREFIX_NONE,
		       16, 1, buf, len, false);
}

or just use print_hex_dump_bytes.

Re: [PATCH 2/2] ide-scsi: use print_hex_dump from <linux/kernel.h>

[Bartlomiej Zolnierkiewicz]

On Tuesday 27 November 2007, Andrew Morton wrote:
> On Mon, 26 Nov 2007 15:16:13 +0800 Denis Cheng <crquan@gmail.com> wrote:
> 
> > these utilities implemented in lib/hexdump.c are more handy, please use this.
> > 
> > ...
> >
> > --- a/drivers/scsi/ide-scsi.c
> > +++ b/drivers/scsi/ide-scsi.c
> > @@ -242,16 +242,6 @@ static void idescsi_output_buffers (ide_drive_t *drive, idescsi_pc_t *pc, unsign
> >  	}
> >  }
> >  
> > -static void hexdump(u8 *x, int len)
> > -{
> > -	int i;
> > -
> > -	printk("[ ");
> > -	for (i = 0; i < len; i++)
> > -		printk("%x ", x[i]);
> > -	printk("]\n");
> > -}
> > -
> >  static int idescsi_check_condition(ide_drive_t *drive, struct request *failed_command)
> >  {
> >  	idescsi_scsi_t *scsi = drive_to_idescsi(drive);
> > @@ -282,7 +272,7 @@ static int idescsi_check_condition(ide_drive_t *drive, struct request *failed_co
> >  	pc->scsi_cmd = ((idescsi_pc_t *) failed_command->special)->scsi_cmd;
> >  	if (test_bit(IDESCSI_LOG_CMD, &scsi->log)) {
> >  		printk ("ide-scsi: %s: queue cmd = ", drive->name);
> > -		hexdump(pc->c, 6);
> > +		print_hex_dump(KERN_DEBUG, "", DUMP_PREFIX_OFFSET, 16, 1, pc->c, 6, 1);
> >  	}
> >  	rq->rq_disk = scsi->disk;
> >  	return ide_do_drive_cmd(drive, rq, ide_preempt);
> > @@ -337,7 +327,7 @@ static int idescsi_end_request (ide_drive_t *drive, int uptodate, int nrsecs)
> >  		idescsi_pc_t *opc = (idescsi_pc_t *) rq->buffer;
> >  		if (log) {
> >  			printk ("ide-scsi: %s: wrap up check %lu, rst = ", drive->name, 
opc->scsi_cmd->serial_number);
> > -			hexdump(pc->buffer,16);
> > +			print_hex_dump(KERN_DEBUG, "", DUMP_PREFIX_OFFSET, 16, 1, pc->buffer, 16, 1);
> >  		}
> >  		memcpy((void *) opc->scsi_cmd->sense_buffer, pc->buffer, SCSI_SENSE_BUFFERSIZE);
> >  		kfree(pc->buffer);
> > @@ -816,10 +806,10 @@ static int idescsi_queue (struct scsi_cmnd *cmd,
> >  
> >  	if (test_bit(IDESCSI_LOG_CMD, &scsi->log)) {
> >  		printk ("ide-scsi: %s: que %lu, cmd = ", drive->name, cmd->serial_number);
> > -		hexdump(cmd->cmnd, cmd->cmd_len);
> > +		print_hex_dump(KERN_DEBUG, "", DUMP_PREFIX_OFFSET, 16, 1, cmd->cmnd, cmd->cmd_len, 1);
> >  		if (memcmp(pc->c, cmd->cmnd, cmd->cmd_len)) {
> >  			printk ("ide-scsi: %s: que %lu, tsl = ", drive->name, cmd->serial_number);
> > -			hexdump(pc->c, 12);
> > +			print_hex_dump(KERN_DEBUG, "", DUMP_PREFIX_OFFSET, 16, 1, pc->c, 12, 1);
> >  		}
> >  	}
> >  

I applied the patch with a couple of fixes:
- s/KERN_DEBUG/KERN_CONT/ as pointed out by Randy
- s/DUMP_PREFIX_OFFSET/DUMP_PREFIX_NONE/
- don't include ASCII dump
- respect 80-columns limit

> Would you believe that this patch (which removes code) actually increases
> drivers/scsi/ide-scsi.o .text by 75 bytes?

Yeah, it somehow shrank down to +58 bytes after fixes but this is still bad.

> I didn't look to see why - probably that huge arg count is hurting,
> possibly some additional strings being emitted?

Probably.

> Either way, perhaps a simple little front-end to print_hex_dump() is called
> for.

Alternatively: it seems that we can easily replace 'prefix_type', 'rowsize',
'groupsize' and 'ascii' args by a single 'flags' arg.

Thanks,
Bart

Re: [PATCH 2/2] ide-scsi: use print_hex_dump from <linux/kernel.h>

[Bartlomiej Zolnierkiewicz]

On Tuesday 27 November 2007, Bartlomiej Zolnierkiewicz wrote:
> On Tuesday 27 November 2007, Andrew Morton wrote:
> > On Mon, 26 Nov 2007 15:16:13 +0800 Denis Cheng <crquan@gmail.com> wrote:
> > 
> > > these utilities implemented in lib/hexdump.c are more handy, please use this.
> > > 
> > > ...
> > >
> > > --- a/drivers/scsi/ide-scsi.c
> > > +++ b/drivers/scsi/ide-scsi.c
> > > @@ -242,16 +242,6 @@ static void idescsi_output_buffers (ide_drive_t *drive, idescsi_pc_t *pc, unsign
> > >  	}
> > >  }
> > >  
> > > -static void hexdump(u8 *x, int len)
> > > -{
> > > -	int i;
> > > -
> > > -	printk("[ ");
> > > -	for (i = 0; i < len; i++)
> > > -		printk("%x ", x[i]);
> > > -	printk("]\n");
> > > -}
> > > -
> > >  static int idescsi_check_condition(ide_drive_t *drive, struct request *failed_command)
> > >  {
> > >  	idescsi_scsi_t *scsi = drive_to_idescsi(drive);
> > > @@ -282,7 +272,7 @@ static int idescsi_check_condition(ide_drive_t *drive, struct request *failed_co
> > >  	pc->scsi_cmd = ((idescsi_pc_t *) failed_command->special)->scsi_cmd;
> > >  	if (test_bit(IDESCSI_LOG_CMD, &scsi->log)) {
> > >  		printk ("ide-scsi: %s: queue cmd = ", drive->name);
> > > -		hexdump(pc->c, 6);
> > > +		print_hex_dump(KERN_DEBUG, "", DUMP_PREFIX_OFFSET, 16, 1, pc->c, 6, 1);
> > >  	}
> > >  	rq->rq_disk = scsi->disk;
> > >  	return ide_do_drive_cmd(drive, rq, ide_preempt);
> > > @@ -337,7 +327,7 @@ static int idescsi_end_request (ide_drive_t *drive, int uptodate, int nrsecs)
> > >  		idescsi_pc_t *opc = (idescsi_pc_t *) rq->buffer;
> > >  		if (log) {
> > >  			printk ("ide-scsi: %s: wrap up check %lu, rst = ", drive->name, 
> opc->scsi_cmd->serial_number);
> > > -			hexdump(pc->buffer,16);
> > > +			print_hex_dump(KERN_DEBUG, "", DUMP_PREFIX_OFFSET, 16, 1, pc->buffer, 16, 1);
> > >  		}
> > >  		memcpy((void *) opc->scsi_cmd->sense_buffer, pc->buffer, SCSI_SENSE_BUFFERSIZE);
> > >  		kfree(pc->buffer);
> > > @@ -816,10 +806,10 @@ static int idescsi_queue (struct scsi_cmnd *cmd,
> > >  
> > >  	if (test_bit(IDESCSI_LOG_CMD, &scsi->log)) {
> > >  		printk ("ide-scsi: %s: que %lu, cmd = ", drive->name, cmd->serial_number);
> > > -		hexdump(cmd->cmnd, cmd->cmd_len);
> > > +		print_hex_dump(KERN_DEBUG, "", DUMP_PREFIX_OFFSET, 16, 1, cmd->cmnd, cmd->cmd_len, 1);
> > >  		if (memcmp(pc->c, cmd->cmnd, cmd->cmd_len)) {
> > >  			printk ("ide-scsi: %s: que %lu, tsl = ", drive->name, cmd->serial_number);
> > > -			hexdump(pc->c, 12);
> > > +			print_hex_dump(KERN_DEBUG, "", DUMP_PREFIX_OFFSET, 16, 1, pc->c, 12, 1);
> > >  		}
> > >  	}
> > >  
> 
> I applied the patch with a couple of fixes:
> - s/KERN_DEBUG/KERN_CONT/ as pointed out by Randy
> - s/DUMP_PREFIX_OFFSET/DUMP_PREFIX_NONE/
> - don't include ASCII dump
> - respect 80-columns limit
> 
> > Would you believe that this patch (which removes code) actually increases
> > drivers/scsi/ide-scsi.o .text by 75 bytes?
> 
> Yeah, it somehow shrank down to +58 bytes after fixes but this is still bad.

Heh, incremental (since I pushed the previous one to Linus already)
patch which cuts 85 bytes from ide-scsi...

[PATCH] ide-scsi: add ide_scsi_hex_dump() helper

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Denis Cheng <crquan@gmail.com>
Cc: Randy Dunlap <randy.dunlap@oracle.com>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/scsi/ide-scsi.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Index: b/drivers/scsi/ide-scsi.c
===================================================================
--- a/drivers/scsi/ide-scsi.c
+++ b/drivers/scsi/ide-scsi.c
@@ -242,6 +242,11 @@ static void idescsi_output_buffers (ide_
 	}
 }
 
+static void ide_scsi_hex_dump(u8 *data, int len)
+{
+	print_hex_dump(KERN_CONT, "", DUMP_PREFIX_NONE, 16, 1, data, len, 0);
+}
+
 static int idescsi_check_condition(ide_drive_t *drive, struct request *failed_command)
 {
 	idescsi_scsi_t *scsi = drive_to_idescsi(drive);
@@ -272,8 +277,7 @@ static int idescsi_check_condition(ide_d
 	pc->scsi_cmd = ((idescsi_pc_t *) failed_command->special)->scsi_cmd;
 	if (test_bit(IDESCSI_LOG_CMD, &scsi->log)) {
 		printk ("ide-scsi: %s: queue cmd = ", drive->name);
-		print_hex_dump(KERN_CONT, "", DUMP_PREFIX_NONE, 16, 1, pc->c,
-			       6, 0);
+		ide_scsi_hex_dump(pc->c, 6);
 	}
 	rq->rq_disk = scsi->disk;
 	return ide_do_drive_cmd(drive, rq, ide_preempt);
@@ -328,8 +332,7 @@ static int idescsi_end_request (ide_driv
 		idescsi_pc_t *opc = (idescsi_pc_t *) rq->buffer;
 		if (log) {
 			printk ("ide-scsi: %s: wrap up check %lu, rst = ", drive->name, 
opc->scsi_cmd->serial_number);
-			print_hex_dump(KERN_CONT, "", DUMP_PREFIX_NONE, 16, 1,
-				       pc->buffer, 16, 0);
+			ide_scsi_hex_dump(pc->buffer, 16);
 		}
 		memcpy((void *) opc->scsi_cmd->sense_buffer, pc->buffer, SCSI_SENSE_BUFFERSIZE);
 		kfree(pc->buffer);
@@ -808,12 +811,10 @@ static int idescsi_queue (struct scsi_cm
 
 	if (test_bit(IDESCSI_LOG_CMD, &scsi->log)) {
 		printk ("ide-scsi: %s: que %lu, cmd = ", drive->name, cmd->serial_number);
-		print_hex_dump(KERN_CONT, "", DUMP_PREFIX_NONE, 16, 1,
-			       cmd->cmnd, cmd->cmd_len, 0);
+		ide_scsi_hex_dump(cmd->cmnd, cmd->cmd_len);
 		if (memcmp(pc->c, cmd->cmnd, cmd->cmd_len)) {
 			printk ("ide-scsi: %s: que %lu, tsl = ", drive->name, cmd->serial_number);
-			print_hex_dump(KERN_CONT, "", DUMP_PREFIX_NONE, 16, 1,
-				       pc->c, 12, 0);
+			ide_scsi_hex_dump(pc->c, 12);
 		}
 	}