Re: [PATCH 3/9] scsi: use external buffer for command logging
On Wed, 2015-01-14 at 10:36 +0100, h...@lst.de wrote: > On Tue, Jan 13, 2015 at 06:56:17PM +, James Bottomley wrote: > > I really hate using an on-stack buffer here ... we're pretty deep in the > > call chain already. > > > > Since it's just required for printing a "command: " prefix, why not just > > use scsi_print_command()? > > Both the ch and sr callers are not really a problem in terms of stack > usage, as they are directly in file operations of character or block > devices, so the whole FS/MM stack stack problem isn't an issue. Trying to calculate minutely how much additional overhead there is isn't really the point. The point is we can just use a different function that doesn't consume the on stack buffer, so we should, just as a matter of best practise. I agree it would have been a more finely balanced problem if we definitely needed the allocation > But using higher level functions might still be useful here. James
Re: [PATCH 3/9] scsi: use external buffer for command logging
On Tue, Jan 13, 2015 at 06:56:17PM +, James Bottomley wrote: > I really hate using an on-stack buffer here ... we're pretty deep in the > call chain already. > > Since it's just required for printing a "command: " prefix, why not just > use scsi_print_command()? Both the ch and sr callers are not really a problem in terms of stack usage, as they are directly in file operations of character or block devices, so the whole FS/MM stack stack problem isn't an issue. But using higher level functions might still be useful here. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/9] scsi: use external buffer for command logging
On Thu, 2015-01-08 at 07:43 +0100, Hannes Reinecke wrote:
> Use an external buffer for __scsi_print_command() and
> move command logging over to use the per-cpu logging
> buffer. With that we can guarantee the command always
> will always be formatted in one line.
> So we can even print out a variable length command
> correctly across several lines.
> Finally rename __scsi_print_command() to
> __scsi_format_comment() to better reflect the
> functionality.
>
> Tested-by: Robert Elliott
> Reviewed-by: Robert Elliott
> Reviewed-by: Christoph Hellwig
> Signed-off-by: Hannes Reinecke
> ---
> drivers/scsi/ch.c | 6 +-
> drivers/scsi/constants.c| 74 +-
> drivers/scsi/scsi_logging.c | 182
>
> drivers/scsi/sr_ioctl.c | 13 +++-
> include/scsi/scsi.h | 3 +
> include/scsi/scsi_dbg.h | 6 +-
> 6 files changed, 192 insertions(+), 92 deletions(-)
>
> diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
> index 6bac8a7..79e462f 100644
> --- a/drivers/scsi/ch.c
> +++ b/drivers/scsi/ch.c
> @@ -195,8 +195,10 @@ ch_do_scsi(scsi_changer *ch, unsigned char *cmd, int
> cmd_len,
> retry:
> errno = 0;
> if (debug) {
> - DPRINTK("command: ");
> - __scsi_print_command(cmd, cmd_len);
> + char logbuf[SCSI_LOG_BUFSIZE];
> +
> + __scsi_format_command(logbuf, sizeof(logbuf), cmd, cmd_len);
> + DPRINTK("command: %s", logbuf);
I really hate using an on-stack buffer here ... we're pretty deep in the
call chain already.
Since it's just required for printing a "command: " prefix, why not just
use scsi_print_command()?
> diff --git a/drivers/scsi/sr_ioctl.c b/drivers/scsi/sr_ioctl.c
> index fb929fa..e8deb9c 100644
> --- a/drivers/scsi/sr_ioctl.c
> +++ b/drivers/scsi/sr_ioctl.c
> @@ -188,6 +188,7 @@ int sr_do_ioctl(Scsi_CD *cd, struct packet_command *cgc)
> struct scsi_sense_hdr sshdr;
> int result, err = 0, retries = 0;
> struct request_sense *sense = cgc->sense;
> + char logbuf[SCSI_LOG_BUFSIZE];
Same issue here.
> SDev = cd->device;
>
> @@ -257,14 +258,20 @@ int sr_do_ioctl(Scsi_CD *cd, struct packet_command *cgc)
> /* sense: Invalid command operation code */
> err = -EDRIVE_CANT_DO_THIS;
> #ifdef DEBUG
> - __scsi_print_command(cgc->cmd, CDROM_PACKET_SIZE);
> + __scsi_format_command(logbuf, sizeof(logbuf),
> + cgc->cmd, CDROM_PACKET_SIZE);
> + sr_printk(KERN_INFO, cd,
> + "CDROM (ioctl) invalid command: %s\n",
> + logbuf);
Looks like scsi_print_command() will work here too.
> scsi_print_sense_hdr(cd->device, cd->cdi.name,
> &sshdr);
> #endif
> break;
> default:
> + __scsi_format_command(logbuf, sizeof(logbuf),
> + cgc->cmd, CDROM_PACKET_SIZE);
> sr_printk(KERN_ERR, cd,
> - "CDROM (ioctl) error, command: ");
> - __scsi_print_command(cgc->cmd, CDROM_PACKET_SIZE);
> + "CDROM (ioctl) error, command: %s\n",
> + logbuf);
And realistically here (with the appropriate sdev_print()'s preceding).
> scsi_print_sense_hdr(cd->device, cd->cdi.name,
> &sshdr);
> err = -EIO;
> }
James
[PATCH 3/9] scsi: use external buffer for command logging
Use an external buffer for __scsi_print_command() and
move command logging over to use the per-cpu logging
buffer. With that we can guarantee the command always
will always be formatted in one line.
So we can even print out a variable length command
correctly across several lines.
Finally rename __scsi_print_command() to
__scsi_format_comment() to better reflect the
functionality.
Tested-by: Robert Elliott
Reviewed-by: Robert Elliott
Reviewed-by: Christoph Hellwig
Signed-off-by: Hannes Reinecke
---
drivers/scsi/ch.c | 6 +-
drivers/scsi/constants.c| 74 +-
drivers/scsi/scsi_logging.c | 182
drivers/scsi/sr_ioctl.c | 13 +++-
include/scsi/scsi.h | 3 +
include/scsi/scsi_dbg.h | 6 +-
6 files changed, 192 insertions(+), 92 deletions(-)
diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
index 6bac8a7..79e462f 100644
--- a/drivers/scsi/ch.c
+++ b/drivers/scsi/ch.c
@@ -195,8 +195,10 @@ ch_do_scsi(scsi_changer *ch, unsigned char *cmd, int
cmd_len,
retry:
errno = 0;
if (debug) {
- DPRINTK("command: ");
- __scsi_print_command(cmd, cmd_len);
+ char logbuf[SCSI_LOG_BUFSIZE];
+
+ __scsi_format_command(logbuf, sizeof(logbuf), cmd, cmd_len);
+ DPRINTK("command: %s", logbuf);
}
result = scsi_execute_req(ch->device, cmd, direction, buffer,
diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c
index 55a7157..7792960 100644
--- a/drivers/scsi/constants.c
+++ b/drivers/scsi/constants.c
@@ -24,8 +24,6 @@
#define THIRD_PARTY_COPY_OUT 0x83
#define THIRD_PARTY_COPY_IN 0x84
-#define VENDOR_SPECIFIC_CDB 0xc0
-
struct sa_name_list {
int opcode;
const struct value_name_pair *arr;
@@ -281,8 +279,8 @@ static struct sa_name_list sa_names_arr[] = {
};
#endif /* CONFIG_SCSI_CONSTANTS */
-static bool scsi_opcode_sa_name(int opcode, int service_action,
- const char **cdb_name, const char **sa_name)
+bool scsi_opcode_sa_name(int opcode, int service_action,
+const char **cdb_name, const char **sa_name)
{
struct sa_name_list *sa_name_ptr;
const struct value_name_pair *arr = NULL;
@@ -315,74 +313,6 @@ static bool scsi_opcode_sa_name(int opcode, int
service_action,
return true;
}
-static void print_opcode_name(const unsigned char *cdbp, size_t cdb_len)
-{
- int sa, cdb0;
- const char *cdb_name = NULL, *sa_name = NULL;
-
- cdb0 = cdbp[0];
- if (cdb0 == VARIABLE_LENGTH_CMD) {
- if (cdb_len < 10) {
- printk("short variable length command, len=%zu",
- cdb_len);
- return;
- }
- sa = (cdbp[8] << 8) + cdbp[9];
- } else
- sa = cdbp[1] & 0x1f;
-
- if (!scsi_opcode_sa_name(cdb0, sa, &cdb_name, &sa_name)) {
- if (cdb_name)
- printk("%s", cdb_name);
- else if (cdb0 >= VENDOR_SPECIFIC_CDB)
- printk("cdb[0]=0x%x (vendor)", cdb0);
- else if (cdb0 >= 0x60 && cdb0 < 0x7e)
- printk("cdb[0]=0x%x (reserved)", cdb0);
- else
- printk("cdb[0]=0x%x", cdb0);
- } else {
- if (sa_name)
- printk("%s", sa_name);
- else if (cdb_name)
- printk("%s, sa=0x%x", cdb_name, sa);
- else
- printk("cdb[0]=0x%x, sa=0x%x", cdb0, sa);
- }
-}
-
-void __scsi_print_command(const unsigned char *cdb, size_t cdb_len)
-{
- int k, len;
-
- print_opcode_name(cdb, cdb_len);
- len = scsi_command_size(cdb);
- if (cdb_len < len)
- len = cdb_len;
- /* print out all bytes in cdb */
- for (k = 0; k < len; ++k)
- printk(" %02x", cdb[k]);
- printk("\n");
-}
-EXPORT_SYMBOL(__scsi_print_command);
-
-void scsi_print_command(struct scsi_cmnd *cmd)
-{
- int k;
-
- if (cmd->cmnd == NULL)
- return;
-
- scmd_printk(KERN_INFO, cmd, "CDB: ");
- print_opcode_name(cmd->cmnd, cmd->cmd_len);
-
- /* print out all bytes in cdb */
- printk(":");
- for (k = 0; k < cmd->cmd_len; ++k)
- printk(" %02x", cmd->cmnd[k]);
- printk("\n");
-}
-EXPORT_SYMBOL(scsi_print_command);
-
#ifdef CONFIG_SCSI_CONSTANTS
struct error_info {
diff --git a/drivers/scsi/scsi_logging.c b/drivers/scsi/scsi_logging.c
index 4d20132..afba995 100644
--- a/drivers/scsi/scsi_logging.c
+++ b/drivers/scsi/scsi_logging.c
@@ -13,10 +13,10 @@
#include
#include
#include
+#include
#include
#define SCSI_LOG_SPOOLSIZE 4096
-#define SCSI_LOG_BUFSIZE 128
#if (SCSI_LOG_SPOOLSIZE / SCSI_LOG_BUFSIZE) > BITS_PER_LONG
#warning SCSI logging bitmask too large
@@
