[PATCH 00/10] scsi logging update: the real thing

[PATCH 00/10] scsi logging update: the real thing

[Hannes Reinecke]

Hi all,

this is the second part of my scsi logging update, covering
CDB and sense code printing. With this patchset CDBs and
sense code bytes will be formatted on a single line
(where possible), and prefixed with the device and command
tag.

To ensure CDBs and sense code bytes are not broken up during
printk I've implemented a per-cpu buffer for formatting
messages. One can allocate a chunk from the buffer using
scsi_log_reserve_buffer() and return it via scsi_log_release_buffer().
Both function do an implicit preempt_disable() / preempt_enable()
to ensure we're not scheduled away from that cpu whilst writing
into the buffer.

With that I've been able to clean up constants.c to remove all the
#ifdef CONFIG_SCSI_CONSTANTS statements. It'll now be compiled in
conditionally if CONFIG_SCSI_CONSTANTS is set.

Additionally I've updated the SCSI command definitions to SPC-3.

Thanks go to Stephen Rostedt who suffered my annoying questions
during LPC.

As usual, comments and reviews are welcome.

Hannes Reinecke (10):
  scsi: Use real functions for logging
  scsi: log request tag for scmd_printk()
  scsi: use external buffer for command logging
  libata: use __scsi_format_command()
  scsi: use per-cpu buffer for formatting sense
  scsi: use per-cpu buffer for formatting scsi_print_result()
  scsi: Rename SERVICE_ACTION_IN to SERVICE_ACTION_IN_16
  scsi: Add SPC-3 command definitions
  scsi: Conditionally compile in constants.c
  scsi: Move remaining printk() statement to scmd_printk()

 block/scsi_ioctl.c                         |   2 +-
 drivers/ata/libata-eh.c                    |  13 +-
 drivers/ata/libata-scsi.c                  |   2 +-
 drivers/block/nvme-scsi.c                  |   4 +-
 drivers/scsi/Makefile                      |   6 +-
 drivers/scsi/aacraid/aachba.c              |   4 +-
 drivers/scsi/aacraid/linit.c               |   2 +-
 drivers/scsi/ch.c                          |   6 +-
 drivers/scsi/constants.c                   | 250 +----------------
 drivers/scsi/gdth.c                        |   4 +-
 drivers/scsi/scsi.c                        |  12 +-
 drivers/scsi/scsi_debug.c                  |   2 +-
 drivers/scsi/scsi_logging.c                | 437 +++++++++++++++++++++++++++++
 drivers/scsi/scsi_trace.c                  |   2 +-
 drivers/scsi/sd.c                          |   9 +-
 drivers/scsi/sr_ioctl.c                    |  13 +-
 drivers/scsi/ufs/ufshcd.c                  |   6 +-
 drivers/target/target_core_alua.c          |   2 +-
 drivers/target/target_core_pr.c            |   2 +-
 drivers/target/target_core_sbc.c           |   2 +-
 drivers/usb/gadget/legacy/tcm_usb_gadget.c |   2 +-
 drivers/xen/xen-scsiback.c                 |   1 +
 include/scsi/scsi.h                        |  11 +-
 include/scsi/scsi_dbg.h                    |  53 +++-
 include/scsi/scsi_device.h                 |  21 +-
 include/trace/events/scsi.h                |   2 +-
 include/trace/events/target.h              |   2 +-
 tools/lib/traceevent/plugin_scsi.c         |   9 +-
 28 files changed, 563 insertions(+), 318 deletions(-)
 create mode 100644 drivers/scsi/scsi_logging.c

-- 
1.8.5.2

[PATCH 01/10] scsi: Use real functions for logging

[Hannes Reinecke]

Implement a per-cpu buffer for formatting messages,
and add real functions for scmd_printk and sdev_prefix_printk
using that buffer and use dev_printk() to print out things.
And make sdev_printk() a wrapper for sdev_prefix_printk().

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/Makefile       |   2 +-
 drivers/scsi/scsi_logging.c | 119 ++++++++++++++++++++++++++++++++++++++++++++
 include/scsi/scsi_device.h  |  21 +++-----
 3 files changed, 128 insertions(+), 14 deletions(-)
 create mode 100644 drivers/scsi/scsi_logging.c

diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
index 59f1ce6..4991b62 100644
--- a/drivers/scsi/Makefile
+++ b/drivers/scsi/Makefile
@@ -166,7 +166,7 @@ scsi_mod-y			+= scsi_scan.o scsi_sysfs.o scsi_devinfo.o
 scsi_mod-$(CONFIG_SCSI_NETLINK)	+= scsi_netlink.o
 scsi_mod-$(CONFIG_SYSCTL)	+= scsi_sysctl.o
 scsi_mod-$(CONFIG_SCSI_PROC_FS)	+= scsi_proc.o
-scsi_mod-y			+= scsi_trace.o
+scsi_mod-y			+= scsi_trace.o scsi_logging.o
 scsi_mod-$(CONFIG_PM)		+= scsi_pm.o
 
 hv_storvsc-y			:= storvsc_drv.o
diff --git a/drivers/scsi/scsi_logging.c b/drivers/scsi/scsi_logging.c
new file mode 100644
index 0000000..4a76796
--- /dev/null
+++ b/drivers/scsi/scsi_logging.c
@@ -0,0 +1,119 @@
+/*
+ * scsi_logging.c
+ *
+ * Copyright (C) 2014 SUSE Linux Products GmbH
+ * Copyright (C) 2014 Hannes Reinecke <hare@suse.de>
+ *
+ * This file is released under the GPLv2
+ */
+
+#include <linux/kernel.h>
+#include <linux/atomic.h>
+
+#include <scsi/scsi.h>
+#include <scsi/scsi_cmnd.h>
+#include <scsi/scsi_device.h>
+#include <scsi/scsi_dbg.h>
+
+#define SCSI_LOG_SPOOLSIZE 4096
+#define SCSI_LOG_BUFSIZE 128
+
+struct scsi_log_buf {
+	char buffer[SCSI_LOG_SPOOLSIZE];
+	unsigned long map;
+};
+
+static DEFINE_PER_CPU(struct scsi_log_buf, scsi_format_log);
+
+static char *scsi_log_reserve_buffer(size_t *len)
+{
+	struct scsi_log_buf *buf;
+	unsigned long map_bits = SCSI_LOG_SPOOLSIZE / SCSI_LOG_BUFSIZE;
+	unsigned long idx = 0;
+
+	WARN_ON(map_bits > BITS_PER_LONG);
+	preempt_disable();
+	buf = this_cpu_ptr(&scsi_format_log);
+	idx = find_first_zero_bit(&buf->map, map_bits);
+	while (test_and_set_bit(idx, &buf->map)) {
+		idx = find_next_zero_bit(&buf->map, map_bits, idx);
+		if (idx >= map_bits) {
+			break;
+		}
+	}
+	if (WARN_ON(idx >= map_bits)) {
+		preempt_enable();
+		return NULL;
+	}
+	*len = SCSI_LOG_BUFSIZE;
+	return buf->buffer + idx * SCSI_LOG_BUFSIZE;
+}
+
+static void scsi_log_release_buffer(char *bufptr)
+{
+	struct scsi_log_buf *buf;
+	unsigned long idx;
+	int ret;
+
+	buf = this_cpu_ptr(&scsi_format_log);
+	if (bufptr < buf->buffer + SCSI_LOG_SPOOLSIZE) {
+		idx = (bufptr - buf->buffer) / SCSI_LOG_BUFSIZE;
+		ret = test_and_clear_bit(idx, &buf->map);
+		WARN_ON(!ret);
+	}
+	preempt_enable();
+}
+
+int sdev_prefix_printk(const char *level, const struct scsi_device *sdev,
+		       const char *name, const char *fmt, ...)
+{
+	va_list args;
+	char *logbuf;
+	size_t off = 0, logbuf_len;
+	int ret;
+
+	if (!sdev)
+		return 0;
+
+	logbuf = scsi_log_reserve_buffer(&logbuf_len);
+	if (!logbuf)
+		return 0;
+
+	if (name)
+		off += scnprintf(logbuf + off, logbuf_len - off,
+				 "[%s] ", name);
+	va_start(args, fmt);
+	off += vscnprintf(logbuf + off, logbuf_len - off, fmt, args);
+	va_end(args);
+	ret = dev_printk(level, &sdev->sdev_gendev, "%s", logbuf);
+	scsi_log_release_buffer(logbuf);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(sdev_prefix_printk);
+
+int scmd_printk(const char *level, const struct scsi_cmnd *scmd,
+		const char *fmt, ...)
+{
+	struct gendisk *disk = scmd->request->rq_disk;
+	va_list args;
+	char *logbuf;
+	size_t off = 0, logbuf_len;
+	int ret;
+
+	if (!scmd || scmd->cmnd == NULL)
+		return 0;
+
+	logbuf = scsi_log_reserve_buffer(&logbuf_len);
+	if (!logbuf)
+		return 0;
+	if (disk)
+		off += scnprintf(logbuf + off, logbuf_len - off,
+				 "[%s] ", disk->disk_name);
+	va_start(args, fmt);
+	off += vscnprintf(logbuf + off, logbuf_len - off, fmt, args);
+	va_end(args);
+	ret = dev_printk(level, &scmd->device->sdev_gendev, "%s", logbuf);
+	scsi_log_release_buffer(logbuf);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(scmd_printk);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 0b18a09..daa62df 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -238,9 +238,6 @@ struct scsi_dh_data {
 #define transport_class_to_sdev(class_dev) \
 	to_scsi_device(class_dev->parent)
 
-#define sdev_printk(prefix, sdev, fmt, a...)	\
-	dev_printk(prefix, &(sdev)->sdev_gendev, fmt, ##a)
-
 #define sdev_dbg(sdev, fmt, a...) \
 	dev_dbg(&(sdev)->sdev_gendev, fmt, ##a)
 
@@ -248,16 +245,14 @@ struct scsi_dh_data {
  * like scmd_printk, but the device name is passed in
  * as a string pointer
  */
-#define sdev_prefix_printk(l, sdev, p, fmt, a...)			\
-	(p) ?								\
-	sdev_printk(l, sdev, "[%s] " fmt, p, ##a) :			\
-	sdev_printk(l, sdev, fmt, ##a)
-
-#define scmd_printk(prefix, scmd, fmt, a...)				\
-        (scmd)->request->rq_disk ?					\
-	sdev_printk(prefix, (scmd)->device, "[%s] " fmt,		\
-		    (scmd)->request->rq_disk->disk_name, ##a) :		\
-	sdev_printk(prefix, (scmd)->device, fmt, ##a)
+extern int sdev_prefix_printk(const char *, const struct scsi_device *,
+			      const char *, const char *, ...);
+
+#define sdev_printk(l, sdev, fmt, a...)				\
+	sdev_prefix_printk(l, sdev, NULL, fmt, ##a)
+
+extern int scmd_printk(const char *, const struct scsi_cmnd *,
+		       const char *, ...);
 
 #define scmd_dbg(scmd, fmt, a...)					   \
 	do {								   \
-- 
1.8.5.2

[PATCH 02/10] scsi: log request tag for scmd_printk()

[Hannes Reinecke]

The request tag provides a concise identification of a SCSI
command, so we should be printing that out for scmd_printk().

Suggested-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/scsi_logging.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/scsi/scsi_logging.c b/drivers/scsi/scsi_logging.c
index 4a76796..dca45fe 100644
--- a/drivers/scsi/scsi_logging.c
+++ b/drivers/scsi/scsi_logging.c
@@ -109,6 +109,10 @@ int scmd_printk(const char *level, const struct scsi_cmnd *scmd,
 	if (disk)
 		off += scnprintf(logbuf + off, logbuf_len - off,
 				 "[%s] ", disk->disk_name);
+
+	if (scmd->request->tag >= 0)
+		off += scnprintf(logbuf + off, logbuf_len - off,
+				 "tag#%d ", scmd->request->tag);
 	va_start(args, fmt);
 	off += vscnprintf(logbuf + off, logbuf_len - off, fmt, args);
 	va_end(args);
-- 
1.8.5.2

[PATCH 03/10] scsi: use external buffer for command logging

[Hannes Reinecke]

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.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/ch.c           |   6 +-
 drivers/scsi/constants.c    |  74 +----------------------
 drivers/scsi/scsi_logging.c | 144 +++++++++++++++++++++++++++++++++++++++++---
 drivers/scsi/sr_ioctl.c     |  13 +++-
 include/scsi/scsi.h         |   3 +
 include/scsi/scsi_dbg.h     |   6 +-
 6 files changed, 160 insertions(+), 86 deletions(-)

diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
index 226ef77..225a867 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, SCSI_LOG_BUFSIZE, 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 a1a7fca..68c6a66 100644
--- a/drivers/scsi/constants.c
+++ b/drivers/scsi/constants.c
@@ -29,8 +29,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;
@@ -286,8 +284,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;
@@ -320,74 +318,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 dca45fe..d166d12 100644
--- a/drivers/scsi/scsi_logging.c
+++ b/drivers/scsi/scsi_logging.c
@@ -13,10 +13,10 @@
 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>
 #include <scsi/scsi_device.h>
+#include <scsi/scsi_eh.h>
 #include <scsi/scsi_dbg.h>
 
 #define SCSI_LOG_SPOOLSIZE 4096
-#define SCSI_LOG_BUFSIZE 128
 
 struct scsi_log_buf {
 	char buffer[SCSI_LOG_SPOOLSIZE];
@@ -64,6 +64,21 @@ static void scsi_log_release_buffer(char *bufptr)
 	preempt_enable();
 }
 
+static size_t scmd_format_header(char *logbuf, size_t logbuf_len,
+				 struct gendisk *disk, int tag)
+{
+	size_t off = 0;
+
+	if (disk)
+		off += scnprintf(logbuf + off, logbuf_len - off,
+				 "[%s] ", disk->disk_name);
+
+	if (tag >= 0)
+		off += scnprintf(logbuf + off, logbuf_len - off,
+				 "tag#%d ", tag);
+	return off;
+}
+
 int sdev_prefix_printk(const char *level, const struct scsi_device *sdev,
 		       const char *name, const char *fmt, ...)
 {
@@ -106,13 +121,8 @@ int scmd_printk(const char *level, const struct scsi_cmnd *scmd,
 	logbuf = scsi_log_reserve_buffer(&logbuf_len);
 	if (!logbuf)
 		return 0;
-	if (disk)
-		off += scnprintf(logbuf + off, logbuf_len - off,
-				 "[%s] ", disk->disk_name);
-
-	if (scmd->request->tag >= 0)
-		off += scnprintf(logbuf + off, logbuf_len - off,
-				 "tag#%d ", scmd->request->tag);
+	off = scmd_format_header(logbuf, logbuf_len, disk,
+				 scmd->request->tag);
 	va_start(args, fmt);
 	off += vscnprintf(logbuf + off, logbuf_len - off, fmt, args);
 	va_end(args);
@@ -121,3 +131,121 @@ int scmd_printk(const char *level, const struct scsi_cmnd *scmd,
 	return ret;
 }
 EXPORT_SYMBOL_GPL(scmd_printk);
+
+static size_t scsi_format_opcode_name(char *buffer, size_t buf_len,
+				      const unsigned char *cdbp)
+{
+	int sa, cdb0;
+	const char *cdb_name = NULL, *sa_name = NULL;
+	size_t off;
+
+	cdb0 = cdbp[0];
+	if (cdb0 == VARIABLE_LENGTH_CMD) {
+		int len = scsi_varlen_cdb_length(cdbp);
+
+		if (len < 10) {
+			off = scnprintf(buffer, buf_len,
+					"short variable length command, len=%d",
+					len);
+			return off;
+		}
+		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)
+			off = scnprintf(buffer, buf_len, "%s", cdb_name);
+		else {
+			off = scnprintf(buffer, buf_len, "cdb[0]=0x%x", cdb0);
+			if (cdb0 > VENDOR_SPECIFIC_CDB)
+				off += scnprintf(buffer + off, buf_len - off,
+						 " (vendor)");
+			else if (cdb0 > 0x60 && cdb0 < 0x7e)
+				off += scnprintf(buffer + off, buf_len - off,
+						 " (reserved)");
+		}
+	} else {
+		if (sa_name)
+			off = scnprintf(buffer, buf_len, "%s", sa_name);
+		else if (cdb_name)
+			off = scnprintf(buffer, buf_len, "%s, sa=0x%x",
+					cdb_name, sa);
+		else
+			off = scnprintf(buffer, buf_len,
+					"cdb[0]=0x%x, sa=0x%x", cdb0, sa);
+	}
+	return off;
+}
+
+size_t __scsi_format_command(char *logbuf, size_t logbuf_len,
+			     const unsigned char *cdb, size_t cdb_len)
+{
+	int len, k;
+	size_t off;
+
+	off = scsi_format_opcode_name(logbuf, logbuf_len, cdb);
+	len = scsi_command_size(cdb);
+	if (cdb_len < len)
+		len = cdb_len;
+	/* print out all bytes in cdb */
+	for (k = 0; k < len; ++k) {
+		if (off + 3 > logbuf_len)
+			break;
+		off += scnprintf(logbuf + off, logbuf_len - off,
+				 " %02x", cdb[k]);
+	}
+	return off;
+}
+EXPORT_SYMBOL(__scsi_format_command);
+
+void scsi_print_command(struct scsi_cmnd *cmd)
+{
+	struct gendisk *disk = cmd->request->rq_disk;
+	int k;
+	char *logbuf;
+	size_t off, logbuf_len;
+
+	if (cmd->cmnd == NULL)
+		return;
+
+	logbuf = scsi_log_reserve_buffer(&logbuf_len);
+	if (!logbuf)
+		return;
+
+	off = scmd_format_header(logbuf, logbuf_len, disk, cmd->request->tag);
+	off += scnprintf(logbuf + off, logbuf_len - off, "CDB: ");
+	off += scsi_format_opcode_name(logbuf + off, logbuf_len - off,
+				       cmd->cmnd);
+	/* print out all bytes in cdb */
+	if (cmd->cmd_len > 16) {
+		/* Print opcode in one line and use separate lines for CDB */
+		off += scnprintf(logbuf + off, logbuf_len - off, "\n");
+		dev_printk(KERN_INFO, &cmd->device->sdev_gendev, logbuf);
+		scsi_log_release_buffer(logbuf);
+		for (k = 0; k < cmd->cmd_len; k += 16) {
+			size_t linelen = min(cmd->cmd_len - k, 16);
+
+			logbuf = scsi_log_reserve_buffer(&logbuf_len);
+			if (!logbuf)
+				break;
+			off = scmd_format_header(logbuf, logbuf_len, disk,
+						 cmd->request->tag);
+			off += scnprintf(logbuf + off, logbuf_len - off,
+					 "CDB[%02x]: ", k);
+			hex_dump_to_buffer(&cmd->cmnd[k], linelen, 16, 1,
+					   logbuf + off, logbuf_len - off,
+					   false);
+			dev_printk(KERN_INFO, &cmd->device->sdev_gendev,
+				   logbuf);
+			scsi_log_release_buffer(logbuf);
+		}
+	} else {
+		off += scnprintf(logbuf + off, logbuf_len - off, " ");
+		hex_dump_to_buffer(cmd->cmnd, cmd->cmd_len, 16, 1,
+				   logbuf + off, logbuf_len - off, false);
+		dev_printk(KERN_INFO, &cmd->device->sdev_gendev, logbuf);
+		scsi_log_release_buffer(logbuf);
+	}
+}
+EXPORT_SYMBOL(scsi_print_command);
diff --git a/drivers/scsi/sr_ioctl.c b/drivers/scsi/sr_ioctl.c
index fb929fa..4f9fb3c 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];
 
 	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, SCSI_LOG_BUFSIZE,
+					      cgc->cmd, CDROM_PACKET_SIZE);
+			sr_printk(KERN_INFO, cd,
+				  "CDROM (ioctl) invalid command: %s\n",
+				  logbuf);
 			scsi_print_sense_hdr(cd->device, cd->cdi.name, &sshdr);
 #endif
 			break;
 		default:
+			__scsi_format_command(logbuf, SCSI_LOG_BUFSIZE,
+					      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);
 			scsi_print_sense_hdr(cd->device, cd->cdi.name, &sshdr);
 			err = -EIO;
 		}
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index d17178e..afd2a1b 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -191,6 +191,9 @@ enum scsi_timeouts {
 #define	ATA_16		      0x85	/* 16-byte pass-thru */
 #define	ATA_12		      0xa1	/* 12-byte pass-thru */
 
+/* Vendor specific CDBs start here */
+#define VENDOR_SPECIFIC_CDB 0xc0
+
 /*
  *	SCSI command lengths
  */
diff --git a/include/scsi/scsi_dbg.h b/include/scsi/scsi_dbg.h
index 7982795..c7ed7b8 100644
--- a/include/scsi/scsi_dbg.h
+++ b/include/scsi/scsi_dbg.h
@@ -5,8 +5,12 @@ struct scsi_cmnd;
 struct scsi_device;
 struct scsi_sense_hdr;
 
+#define SCSI_LOG_BUFSIZE 128
+
+extern bool scsi_opcode_sa_name(int, int, const char **, const char **);
 extern void scsi_print_command(struct scsi_cmnd *);
-extern void __scsi_print_command(const unsigned char *, size_t);
+extern size_t __scsi_format_command(char *, size_t,
+				   const unsigned char *, size_t);
 extern void scsi_show_extd_sense(const struct scsi_device *, const char *,
 				 unsigned char, unsigned char);
 extern void scsi_show_sense_hdr(const struct scsi_device *, const char *,
-- 
1.8.5.2

[PATCH 04/10] libata: use __scsi_format_command()

[Hannes Reinecke]

libata already uses an internal buffer, so we should be using
__scsi_format_command() here.

Cc: linux-ide@vger.kernel.org
Cc: LKML <linux-kernel@vger.kernel.org>
Acked-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/ata/libata-eh.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index dad83df..6550a9a 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -2509,15 +2509,12 @@ static void ata_eh_link_report(struct ata_link *link)
 
 		if (ata_is_atapi(qc->tf.protocol)) {
 			if (qc->scsicmd)
-				scsi_print_command(qc->scsicmd);
+				__scsi_format_command(cdb_buf, sizeof(cdb_buf),
+						      qc->scsicmd->cmnd,
+						      qc->scsicmd->cmd_len);
 			else
-				snprintf(cdb_buf, sizeof(cdb_buf),
-				 "cdb %02x %02x %02x %02x %02x %02x %02x %02x  "
-				 "%02x %02x %02x %02x %02x %02x %02x %02x\n         ",
-				 cdb[0], cdb[1], cdb[2], cdb[3],
-				 cdb[4], cdb[5], cdb[6], cdb[7],
-				 cdb[8], cdb[9], cdb[10], cdb[11],
-				 cdb[12], cdb[13], cdb[14], cdb[15]);
+				__scsi_format_command(cdb_buf, sizeof(cdb_buf),
+						      cdb, qc->dev->cdb_len);
 		} else {
 			const char *descr = ata_get_cmd_descript(cmd->command);
 			if (descr)
-- 
1.8.5.2

[PATCH 05/10] scsi: use per-cpu buffer for formatting sense

[Hannes Reinecke]

Convert sense buffer logging to use the per-cpu
buffer to avoid line breakup.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/constants.c    | 107 -------------------------------
 drivers/scsi/scsi_logging.c | 150 ++++++++++++++++++++++++++++++++++++++++++--
 drivers/scsi/sd.c           |   7 +--
 drivers/scsi/ufs/ufshcd.c   |   6 +-
 4 files changed, 150 insertions(+), 120 deletions(-)

diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c
index 68c6a66..5a2a3e4 100644
--- a/drivers/scsi/constants.c
+++ b/drivers/scsi/constants.c
@@ -1221,113 +1221,6 @@ scsi_extd_sense_format(unsigned char asc, unsigned char ascq, const char **fmt)
 }
 EXPORT_SYMBOL(scsi_extd_sense_format);
 
-void
-scsi_show_extd_sense(const struct scsi_device *sdev, const char *name,
-		     unsigned char asc, unsigned char ascq)
-{
-	const char *extd_sense_fmt = NULL;
-	const char *extd_sense_str = scsi_extd_sense_format(asc, ascq,
-							    &extd_sense_fmt);
-
-	if (extd_sense_str) {
-		if (extd_sense_fmt)
-			sdev_prefix_printk(KERN_INFO, sdev, name,
-					   "Add. Sense: %s (%s%x)",
-					   extd_sense_str, extd_sense_fmt,
-					   ascq);
-		else
-			sdev_prefix_printk(KERN_INFO, sdev, name,
-					   "Add. Sense: %s", extd_sense_str);
-
-	} else {
-		sdev_prefix_printk(KERN_INFO, sdev, name,
-				   "%sASC=0x%x %sASCQ=0x%x\n",
-				   asc >= 0x80 ? "<<vendor>> " : "", asc,
-				   ascq >= 0x80 ? "<<vendor>> " : "", ascq);
-	}
-}
-EXPORT_SYMBOL(scsi_show_extd_sense);
-
-void
-scsi_show_sense_hdr(const struct scsi_device *sdev, const char *name,
-		    const struct scsi_sense_hdr *sshdr)
-{
-	const char *sense_txt;
-
-	sense_txt = scsi_sense_key_string(sshdr->sense_key);
-	if (sense_txt)
-		sdev_prefix_printk(KERN_INFO, sdev, name,
-				   "Sense Key : %s [%s]%s\n", sense_txt,
-				   scsi_sense_is_deferred(sshdr) ?
-				   "deferred" : "current",
-				   sshdr->response_code >= 0x72 ?
-				   " [descriptor]" : "");
-	else
-		sdev_prefix_printk(KERN_INFO, sdev, name,
-				   "Sense Key : 0x%x [%s]%s", sshdr->sense_key,
-				   scsi_sense_is_deferred(sshdr) ?
-				   "deferred" : "current",
-				   sshdr->response_code >= 0x72 ?
-				   " [descriptor]" : "");
-}
-EXPORT_SYMBOL(scsi_show_sense_hdr);
-
-/*
- * Print normalized SCSI sense header with a prefix.
- */
-void
-scsi_print_sense_hdr(const struct scsi_device *sdev, const char *name,
-		     const struct scsi_sense_hdr *sshdr)
-{
-	scsi_show_sense_hdr(sdev, name, sshdr);
-	scsi_show_extd_sense(sdev, name, sshdr->asc, sshdr->ascq);
-}
-EXPORT_SYMBOL(scsi_print_sense_hdr);
-
-static void
-scsi_dump_sense_buffer(const unsigned char *sense_buffer, int sense_len)
-{
-	int k, num;
-
-	num = (sense_len < 32) ? sense_len : 32;
-	printk("Unrecognized sense data (in hex):");
-	for (k = 0; k < num; ++k) {
-		if (0 == (k % 16)) {
-			printk("\n");
-			printk(KERN_INFO "        ");
-		}
-		printk("%02x ", sense_buffer[k]);
-	}
-	printk("\n");
-	return;
-}
-
-/* Normalize and print sense buffer with name prefix */
-void __scsi_print_sense(const struct scsi_device *sdev, const char *name,
-			const unsigned char *sense_buffer, int sense_len)
-{
-	struct scsi_sense_hdr sshdr;
-
-	if (!scsi_normalize_sense(sense_buffer, sense_len, &sshdr)) {
-		scsi_dump_sense_buffer(sense_buffer, sense_len);
-		return;
-	}
-	scsi_show_sense_hdr(sdev, name, &sshdr);
-	scsi_show_extd_sense(sdev, name, sshdr.asc, sshdr.ascq);
-}
-EXPORT_SYMBOL(__scsi_print_sense);
-
-/* Normalize and print sense buffer in SCSI command */
-void scsi_print_sense(const struct scsi_cmnd *cmd)
-{
-	struct gendisk *disk = cmd->request->rq_disk;
-	const char *disk_name = disk ? disk->disk_name : NULL;
-
-	__scsi_print_sense(cmd->device, disk_name, cmd->sense_buffer,
-			   SCSI_SENSE_BUFFERSIZE);
-}
-EXPORT_SYMBOL(scsi_print_sense);
-
 #ifdef CONFIG_SCSI_CONSTANTS
 
 static const char * const hostbyte_table[]={
diff --git a/drivers/scsi/scsi_logging.c b/drivers/scsi/scsi_logging.c
index d166d12..adcd348 100644
--- a/drivers/scsi/scsi_logging.c
+++ b/drivers/scsi/scsi_logging.c
@@ -64,14 +64,14 @@ static void scsi_log_release_buffer(char *bufptr)
 	preempt_enable();
 }
 
-static size_t scmd_format_header(char *logbuf, size_t logbuf_len,
-				 struct gendisk *disk, int tag)
+static size_t sdev_format_header(char *logbuf, size_t logbuf_len,
+				 const char *name, int tag)
 {
 	size_t off = 0;
 
-	if (disk)
+	if (name)
 		off += scnprintf(logbuf + off, logbuf_len - off,
-				 "[%s] ", disk->disk_name);
+				 "[%s] ", name);
 
 	if (tag >= 0)
 		off += scnprintf(logbuf + off, logbuf_len - off,
@@ -79,6 +79,9 @@ static size_t scmd_format_header(char *logbuf, size_t logbuf_len,
 	return off;
 }
 
+#define scmd_format_header(b, l, d, t) \
+	sdev_format_header(b, l, (d) ? (d)->disk_name : NULL, t)
+
 int sdev_prefix_printk(const char *level, const struct scsi_device *sdev,
 		       const char *name, const char *fmt, ...)
 {
@@ -249,3 +252,142 @@ void scsi_print_command(struct scsi_cmnd *cmd)
 	}
 }
 EXPORT_SYMBOL(scsi_print_command);
+
+static size_t
+scsi_format_extd_sense(char *buffer, size_t buf_len,
+		       unsigned char asc, unsigned char ascq)
+{
+	size_t off = 0;
+	const char *extd_sense_fmt = NULL;
+	const char *extd_sense_str = scsi_extd_sense_format(asc, ascq,
+							    &extd_sense_fmt);
+
+	if (extd_sense_str) {
+		off = scnprintf(buffer, buf_len, "Add. Sense: %s",
+				extd_sense_str);
+		if (extd_sense_fmt)
+			off += scnprintf(buffer + off, buf_len - off,
+					 "(%s%x)", extd_sense_fmt, ascq);
+	} else {
+		if (asc >= 0x80)
+			off = scnprintf(buffer, buf_len, "<<vendor>>");
+		off += scnprintf(buffer + off, buf_len - off,
+				 "ASC=0x%x ", asc);
+		if (ascq >= 0x80)
+			off += scnprintf(buffer + off, buf_len - off,
+					 "<<vendor>>");
+		off += scnprintf(buffer + off, buf_len - off,
+				 "ASCQ=0x%x ", ascq);
+	}
+	return off;
+}
+
+static size_t
+scsi_format_sense_hdr(char *buffer, size_t buf_len,
+		      const struct scsi_sense_hdr *sshdr)
+{
+	const char *sense_txt;
+	size_t off;
+
+	off = scnprintf(buffer, buf_len, "Sense Key : ");
+	sense_txt = scsi_sense_key_string(sshdr->sense_key);
+	if (sense_txt)
+		off += scnprintf(buffer + off, buf_len - off,
+				 "%s ", sense_txt);
+	else
+		off += scnprintf(buffer + off, buf_len - off,
+				 "0x%x ", sshdr->sense_key);
+	if (scsi_sense_is_deferred(sshdr))
+		off += scnprintf(buffer + off, buf_len - off, "[deferred] ");
+	else
+		off += scnprintf(buffer + off, buf_len - off, "[current] ");
+
+	if (sshdr->response_code >= 0x72)
+		off += scnprintf(buffer + off, buf_len - off, "[descriptor] ");
+	return off;
+}
+
+static void scsi_log_print_sense_hdr(const struct scsi_device *sdev,
+				     const char *name, int tag,
+				     const struct scsi_sense_hdr *sshdr)
+{
+	char *logbuf;
+	size_t off, logbuf_len;
+
+	logbuf = scsi_log_reserve_buffer(&logbuf_len);
+	if (!logbuf)
+		return;
+	off = sdev_format_header(logbuf, logbuf_len, name, tag);
+	off += scsi_format_sense_hdr(logbuf + off, logbuf_len - off, sshdr);
+	dev_printk(KERN_INFO, &sdev->sdev_gendev, logbuf);
+	scsi_log_release_buffer(logbuf);
+
+	logbuf = scsi_log_reserve_buffer(&logbuf_len);
+	if (!logbuf)
+		return;
+	off = sdev_format_header(logbuf, logbuf_len, name, tag);
+	off += scsi_format_extd_sense(logbuf + off, logbuf_len - off,
+				      sshdr->asc, sshdr->ascq);
+	dev_printk(KERN_INFO, &sdev->sdev_gendev, logbuf);
+	scsi_log_release_buffer(logbuf);
+}
+
+static void
+scsi_log_print_sense(const struct scsi_device *sdev, const char *name, int tag,
+		     const unsigned char *sense_buffer, int sense_len)
+{
+	struct scsi_sense_hdr sshdr;
+
+	if (!scsi_normalize_sense(sense_buffer, sense_len, &sshdr)) {
+		int i;
+
+		for (i = 0; i < sense_len; i += 16) {
+			char *logbuf;
+			int len = min(sense_len - i, 16);
+			size_t off, logbuf_len;
+
+			logbuf = scsi_log_reserve_buffer(&logbuf_len);
+			if (!logbuf)
+				break;
+			off = sdev_format_header(logbuf, logbuf_len,
+						 name, tag);
+			hex_dump_to_buffer(&sense_buffer[i], len, 16, 1,
+					   logbuf + off, logbuf_len - off,
+					   false);
+			dev_printk(KERN_INFO, &sdev->sdev_gendev, logbuf);
+			scsi_log_release_buffer(logbuf);
+		}
+		return;
+	}
+	scsi_log_print_sense_hdr(sdev, name, tag, &sshdr);
+}
+
+/*
+ * Print normalized SCSI sense header with a prefix.
+ */
+void
+scsi_print_sense_hdr(const struct scsi_device *sdev, const char *name,
+		     const struct scsi_sense_hdr *sshdr)
+{
+	scsi_log_print_sense_hdr(sdev, name, -1, sshdr);
+}
+EXPORT_SYMBOL(scsi_print_sense_hdr);
+
+/* Normalize and print sense buffer with name prefix */
+void __scsi_print_sense(const struct scsi_device *sdev, const char *name,
+			const unsigned char *sense_buffer, int sense_len)
+{
+	scsi_log_print_sense(sdev, name, -1, sense_buffer, sense_len);
+}
+EXPORT_SYMBOL(__scsi_print_sense);
+
+/* Normalize and print sense buffer in SCSI command */
+void scsi_print_sense(const struct scsi_cmnd *cmd)
+{
+	struct gendisk *disk = cmd->request->rq_disk;
+	const char *disk_name = disk ? disk->disk_name : NULL;
+
+	scsi_log_print_sense(cmd->device, disk_name, cmd->request->tag,
+			     cmd->sense_buffer, SCSI_SENSE_BUFFERSIZE);
+}
+EXPORT_SYMBOL(scsi_print_sense);
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 242f9b1..e7333de 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3327,11 +3327,8 @@ module_exit(exit_sd);
 static void sd_print_sense_hdr(struct scsi_disk *sdkp,
 			       struct scsi_sense_hdr *sshdr)
 {
-	scsi_show_sense_hdr(sdkp->device,
-			    sdkp->disk ? sdkp->disk->disk_name : NULL, sshdr);
-	scsi_show_extd_sense(sdkp->device,
-			     sdkp->disk ? sdkp->disk->disk_name : NULL,
-			     sshdr->asc, sshdr->ascq);
+	scsi_print_sense_hdr(sdkp->device,
+			     sdkp->disk ? sdkp->disk->disk_name : NULL, sshdr);
 }
 
 static void sd_print_result(const struct scsi_disk *sdkp, const char *msg,
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 9da3191..5c0298e 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4709,10 +4709,8 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
 		sdev_printk(KERN_WARNING, sdp,
 			    "START_STOP failed for power mode: %d, result %x\n",
 			    pwr_mode, ret);
-		if (driver_byte(ret) & DRIVER_SENSE) {
-			scsi_show_sense_hdr(sdp, NULL, &sshdr);
-			scsi_show_extd_sense(sdp, NULL, sshdr.asc, sshdr.ascq);
-		}
+		if (driver_byte(ret) & DRIVER_SENSE)
+			scsi_print_sense_hdr(sdp, NULL, &sshdr);
 	}
 
 	if (!ret)
-- 
1.8.5.2

[PATCH 06/10] scsi: use per-cpu buffer for formatting scsi_print_result()

[Hannes Reinecke]

Convert scsi_print_result() to use the per-cpu buffer for
decoding the command result and disposition.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/constants.c    | 22 ----------------------
 drivers/scsi/scsi.c         |  2 +-
 drivers/scsi/scsi_logging.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 45 insertions(+), 23 deletions(-)

diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c
index 5a2a3e4..4ec2d79e 100644
--- a/drivers/scsi/constants.c
+++ b/drivers/scsi/constants.c
@@ -1291,25 +1291,3 @@ const char *scsi_mlreturn_string(int result)
 	return NULL;
 }
 EXPORT_SYMBOL(scsi_mlreturn_string);
-
-void scsi_print_result(struct scsi_cmnd *cmd, const char *msg, int disposition)
-{
-	const char *mlret_string = scsi_mlreturn_string(disposition);
-	const char *hb_string = scsi_hostbyte_string(cmd->result);
-	const char *db_string = scsi_driverbyte_string(cmd->result);
-
-	if (hb_string || db_string)
-		scmd_printk(KERN_INFO, cmd,
-			    "%s%s Result: hostbyte=%s driverbyte=%s",
-			    msg ? msg : "",
-			    mlret_string ? mlret_string : "UNKNOWN",
-			    hb_string ? hb_string : "invalid",
-			    db_string ? db_string : "invalid");
-	else
-		scmd_printk(KERN_INFO, cmd,
-			    "%s%s Result: hostbyte=0x%02x driverbyte=0x%02x",
-			    msg ? msg : "",
-			    mlret_string ? mlret_string : "UNKNOWN",
-			    host_byte(cmd->result), driver_byte(cmd->result));
-}
-EXPORT_SYMBOL(scsi_print_result);
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index bc52bbd..92d5912 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -572,7 +572,7 @@ void scsi_log_completion(struct scsi_cmnd *cmd, int disposition)
 				       SCSI_LOG_MLCOMPLETE_BITS);
 		if (((level > 0) && (cmd->result || disposition != SUCCESS)) ||
 		    (level > 1)) {
-			scsi_print_result(cmd, "Done: ", disposition);
+			scsi_print_result(cmd, "Done", disposition);
 			scsi_print_command(cmd);
 			if (status_byte(cmd->result) & CHECK_CONDITION)
 				scsi_print_sense(cmd);
diff --git a/drivers/scsi/scsi_logging.c b/drivers/scsi/scsi_logging.c
index adcd348..16e93c8 100644
--- a/drivers/scsi/scsi_logging.c
+++ b/drivers/scsi/scsi_logging.c
@@ -391,3 +391,47 @@ void scsi_print_sense(const struct scsi_cmnd *cmd)
 			     cmd->sense_buffer, SCSI_SENSE_BUFFERSIZE);
 }
 EXPORT_SYMBOL(scsi_print_sense);
+
+void scsi_print_result(struct scsi_cmnd *cmd, const char *msg, int disposition)
+{
+	char *logbuf;
+	size_t off, logbuf_len;
+	struct gendisk *disk = cmd->request->rq_disk;
+	const char *mlret_string = scsi_mlreturn_string(disposition);
+	const char *hb_string = scsi_hostbyte_string(cmd->result);
+	const char *db_string = scsi_driverbyte_string(cmd->result);
+
+	logbuf = scsi_log_reserve_buffer(&logbuf_len);
+	if (!logbuf)
+		return;
+
+	off = scmd_format_header(logbuf, logbuf_len, disk, cmd->request->tag);
+
+	if (msg)
+		off += scnprintf(logbuf + off, logbuf_len - off,
+				 "%s: ", msg);
+	if (mlret_string)
+		off += scnprintf(logbuf + off, logbuf_len - off,
+				 "%s ", mlret_string);
+	else
+		off += scnprintf(logbuf + off, logbuf_len - off,
+				 "UNKNOWN(0x%02x) ", disposition);
+	off += scnprintf(logbuf + off, logbuf_len - off, "Result: ");
+	if (hb_string)
+		off += scnprintf(logbuf + off, logbuf_len - off,
+				 "hostbyte=%s ", hb_string);
+	else
+		off += scnprintf(logbuf + off, logbuf_len - off,
+				 "hostbyte=0x%02x ", host_byte(cmd->result));
+
+	if (db_string)
+		off += scnprintf(logbuf + off, logbuf_len - off,
+				 "driverbyte=%s", db_string);
+	else
+		off += scnprintf(logbuf + off, logbuf_len - off,
+				 "driverbyte=0x%02x", driver_byte(cmd->result));
+
+	dev_printk(KERN_INFO, &cmd->device->sdev_gendev, logbuf);
+	scsi_log_release_buffer(logbuf);
+}
+EXPORT_SYMBOL(scsi_print_result);
-- 
1.8.5.2

[PATCH 07/10] scsi: Rename SERVICE_ACTION_IN to SERVICE_ACTION_IN_16

[Hannes Reinecke]

SPC-3 defines SERVICE ACTION IN(12) and SERVICE ACTION IN(16).
So rename SERVICE_ACTION_IN to SERVICE_ACTION_IN_16 to be
consistent with SPC and to allow for better distinction.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 block/scsi_ioctl.c                         | 2 +-
 drivers/ata/libata-scsi.c                  | 2 +-
 drivers/block/nvme-scsi.c                  | 4 ++--
 drivers/scsi/aacraid/aachba.c              | 4 ++--
 drivers/scsi/aacraid/linit.c               | 2 +-
 drivers/scsi/constants.c                   | 1 -
 drivers/scsi/gdth.c                        | 4 ++--
 drivers/scsi/scsi_debug.c                  | 2 +-
 drivers/scsi/scsi_trace.c                  | 2 +-
 drivers/scsi/sd.c                          | 2 +-
 drivers/target/target_core_alua.c          | 2 +-
 drivers/target/target_core_sbc.c           | 2 +-
 drivers/usb/gadget/legacy/tcm_usb_gadget.c | 2 +-
 include/scsi/scsi.h                        | 2 +-
 include/trace/events/scsi.h                | 2 +-
 include/trace/events/target.h              | 2 +-
 tools/lib/traceevent/plugin_scsi.c         | 4 ++--
 17 files changed, 20 insertions(+), 21 deletions(-)

diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 1e053d9..c7bbc19 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -142,7 +142,7 @@ static void blk_set_cmd_filter_defaults(struct blk_cmd_filter *filter)
 	__set_bit(GPCMD_VERIFY_10, filter->read_ok);
 	__set_bit(VERIFY_16, filter->read_ok);
 	__set_bit(REPORT_LUNS, filter->read_ok);
-	__set_bit(SERVICE_ACTION_IN, filter->read_ok);
+	__set_bit(SERVICE_ACTION_IN_16, filter->read_ok);
 	__set_bit(RECEIVE_DIAGNOSTIC, filter->read_ok);
 	__set_bit(MAINTENANCE_IN, filter->read_ok);
 	__set_bit(GPCMD_READ_BUFFER_CAPACITY, filter->read_ok);
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 0586f66..52a1b7e 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3570,7 +3570,7 @@ void ata_scsi_simulate(struct ata_device *dev, struct scsi_cmnd *cmd)
 		ata_scsi_rbuf_fill(&args, ata_scsiop_read_cap);
 		break;
 
-	case SERVICE_ACTION_IN:
+	case SERVICE_ACTION_IN_16:
 		if ((scsicmd[1] & 0x1f) == SAI_READ_CAPACITY_16)
 			ata_scsi_rbuf_fill(&args, ata_scsiop_read_cap);
 		else
diff --git a/drivers/block/nvme-scsi.c b/drivers/block/nvme-scsi.c
index a4cd6d6..0b4b277 100644
--- a/drivers/block/nvme-scsi.c
+++ b/drivers/block/nvme-scsi.c
@@ -329,7 +329,7 @@ INQUIRY_EVPD_BIT_MASK) ? 1 : 0)
 (GET_U32_FROM_CDB(cdb, READ_CAP_16_CDB_ALLOC_LENGTH_OFFSET))
 
 #define IS_READ_CAP_16(cdb)					\
-((cdb[0] == SERVICE_ACTION_IN && cdb[1] == SAI_READ_CAPACITY_16) ? 1 : 0)
+((cdb[0] == SERVICE_ACTION_IN_16 && cdb[1] == SAI_READ_CAPACITY_16) ? 1 : 0)
 
 /* Request Sense Helper Macros */
 #define GET_REQUEST_SENSE_ALLOC_LENGTH(cdb)			\
@@ -2947,7 +2947,7 @@ static int nvme_scsi_translate(struct nvme_ns *ns, struct sg_io_hdr *hdr)
 	case READ_CAPACITY:
 		retcode = nvme_trans_read_capacity(ns, hdr, cmd);
 		break;
-	case SERVICE_ACTION_IN:
+	case SERVICE_ACTION_IN_16:
 		if (IS_READ_CAP_16(cmd))
 			retcode = nvme_trans_read_capacity(ns, hdr, cmd);
 		else
diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index 681434e..b32e77d 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -2181,7 +2181,7 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
 			  (fsa_dev_ptr[cid].sense_data.sense_key ==
 			   NOT_READY)) {
 				switch (scsicmd->cmnd[0]) {
-				case SERVICE_ACTION_IN:
+				case SERVICE_ACTION_IN_16:
 					if (!(dev->raw_io_interface) ||
 					    !(dev->raw_io_64) ||
 					    ((scsicmd->cmnd[1] & 0x1f) != SAI_READ_CAPACITY_16))
@@ -2309,7 +2309,7 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
 		scsi_sg_copy_from_buffer(scsicmd, &inq_data, sizeof(inq_data));
 		return aac_get_container_name(scsicmd);
 	}
-	case SERVICE_ACTION_IN:
+	case SERVICE_ACTION_IN_16:
 		if (!(dev->raw_io_interface) ||
 		    !(dev->raw_io_64) ||
 		    ((scsicmd->cmnd[1] & 0x1f) != SAI_READ_CAPACITY_16))
diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
index a759cb2..0ba7758 100644
--- a/drivers/scsi/aacraid/linit.c
+++ b/drivers/scsi/aacraid/linit.c
@@ -555,7 +555,7 @@ static int aac_eh_abort(struct scsi_cmnd* cmd)
 		AAC_DRIVERNAME,
 		host->host_no, sdev_channel(dev), sdev_id(dev), dev->lun);
 	switch (cmd->cmnd[0]) {
-	case SERVICE_ACTION_IN:
+	case SERVICE_ACTION_IN_16:
 		if (!(aac->raw_io_interface) ||
 		    !(aac->raw_io_64) ||
 		    ((cmd->cmnd[1] & 0x1f) != SAI_READ_CAPACITY_16))
diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c
index 4ec2d79e..0d2ec4d 100644
--- a/drivers/scsi/constants.c
+++ b/drivers/scsi/constants.c
@@ -24,7 +24,6 @@
 #define SERVICE_ACTION_IN_12 0xab
 #define SERVICE_ACTION_OUT_12 0xa9
 #define SERVICE_ACTION_BIDIRECTIONAL 0x9d
-#define SERVICE_ACTION_IN_16 0x9e
 #define SERVICE_ACTION_OUT_16 0x9f
 #define THIRD_PARTY_COPY_OUT 0x83
 #define THIRD_PARTY_COPY_IN 0x84
diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
index 0f1ae13..28c7b54 100644
--- a/drivers/scsi/gdth.c
+++ b/drivers/scsi/gdth.c
@@ -2159,7 +2159,7 @@ static void gdth_next(gdth_ha_str *ha)
               case VERIFY:
               case START_STOP:
               case MODE_SENSE:
-              case SERVICE_ACTION_IN:
+              case SERVICE_ACTION_IN_16:
                 TRACE(("cache cmd %x/%x/%x/%x/%x/%x\n",nscp->cmnd[0],
                        nscp->cmnd[1],nscp->cmnd[2],nscp->cmnd[3],
                        nscp->cmnd[4],nscp->cmnd[5]));
@@ -2391,7 +2391,7 @@ static int gdth_internal_cache_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp)
         gdth_copy_internal_data(ha, scp, (char*)&rdc, sizeof(gdth_rdcap_data));
         break;
 
-      case SERVICE_ACTION_IN:
+      case SERVICE_ACTION_IN_16:
         if ((scp->cmnd[1] & 0x1f) == SAI_READ_CAPACITY_16 &&
             (ha->cache_feat & GDT_64BIT)) {
             gdth_rdcap16_data rdc16;
diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 238e06f..99fc31d 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -4214,7 +4214,7 @@ scsi_debug_queuecommand(struct scsi_cmnd *SCpnt)
 	case READ_CAPACITY:
 		errsts = resp_readcap(SCpnt, devip);
 		break;
-	case SERVICE_ACTION_IN:
+	case SERVICE_ACTION_IN_16:
 		if (cmd[1] == SAI_READ_CAPACITY_16)
 			errsts = resp_readcap16(SCpnt, devip);
 		else if (cmd[1] == SAI_GET_LBA_STATUS) {
diff --git a/drivers/scsi/scsi_trace.c b/drivers/scsi/scsi_trace.c
index 503594e..82af28b 100644
--- a/drivers/scsi/scsi_trace.c
+++ b/drivers/scsi/scsi_trace.c
@@ -278,7 +278,7 @@ scsi_trace_parse_cdb(struct trace_seq *p, unsigned char *cdb, int len)
 		return scsi_trace_rw16(p, cdb, len);
 	case UNMAP:
 		return scsi_trace_unmap(p, cdb, len);
-	case SERVICE_ACTION_IN:
+	case SERVICE_ACTION_IN_16:
 		return scsi_trace_service_action_in(p, cdb, len);
 	case VARIABLE_LENGTH_CMD:
 		return scsi_trace_varlen(p, cdb, len);
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index e7333de..27a9ea4 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1992,7 +1992,7 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
 
 	do {
 		memset(cmd, 0, 16);
-		cmd[0] = SERVICE_ACTION_IN;
+		cmd[0] = SERVICE_ACTION_IN_16;
 		cmd[1] = SAI_READ_CAPACITY_16;
 		cmd[13] = RC16_LEN;
 		memset(buffer, 0, RC16_LEN);
diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
index fb87780..75cbde1 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -576,7 +576,7 @@ static inline int core_alua_state_standby(
 	case SEND_DIAGNOSTIC:
 	case READ_CAPACITY:
 		return 0;
-	case SERVICE_ACTION_IN:
+	case SERVICE_ACTION_IN_16:
 		switch (cdb[1] & 0x1f) {
 		case SAI_READ_CAPACITY_16:
 			return 0;
diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index ebe62af..8d171ff 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -852,7 +852,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		size = READ_CAP_LEN;
 		cmd->execute_cmd = sbc_emulate_readcapacity;
 		break;
-	case SERVICE_ACTION_IN:
+	case SERVICE_ACTION_IN_16:
 		switch (cmd->t_task_cdb[1] & 0x1f) {
 		case SAI_READ_CAPACITY_16:
 			cmd->execute_cmd = sbc_emulate_readcapacity_16;
diff --git a/drivers/usb/gadget/legacy/tcm_usb_gadget.c b/drivers/usb/gadget/legacy/tcm_usb_gadget.c
index 6cdb7a5..024f584 100644
--- a/drivers/usb/gadget/legacy/tcm_usb_gadget.c
+++ b/drivers/usb/gadget/legacy/tcm_usb_gadget.c
@@ -912,7 +912,7 @@ static int get_cmd_dir(const unsigned char *cdb)
 	case INQUIRY:
 	case MODE_SENSE:
 	case MODE_SENSE_10:
-	case SERVICE_ACTION_IN:
+	case SERVICE_ACTION_IN_16:
 	case MAINTENANCE_IN:
 	case PERSISTENT_RESERVE_IN:
 	case SECURITY_PROTOCOL_IN:
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index afd2a1b..046fe12 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -151,7 +151,7 @@ enum scsi_timeouts {
 #define VERIFY_16	      0x8f
 #define SYNCHRONIZE_CACHE_16  0x91
 #define WRITE_SAME_16	      0x93
-#define SERVICE_ACTION_IN     0x9e
+#define SERVICE_ACTION_IN_16  0x9e
 /* values for service action in */
 #define	SAI_READ_CAPACITY_16  0x10
 #define SAI_GET_LBA_STATUS    0x12
diff --git a/include/trace/events/scsi.h b/include/trace/events/scsi.h
index db6c935..079bd10 100644
--- a/include/trace/events/scsi.h
+++ b/include/trace/events/scsi.h
@@ -94,7 +94,7 @@
 		scsi_opcode_name(WRITE_16),			\
 		scsi_opcode_name(VERIFY_16),			\
 		scsi_opcode_name(WRITE_SAME_16),		\
-		scsi_opcode_name(SERVICE_ACTION_IN),		\
+		scsi_opcode_name(SERVICE_ACTION_IN_16),		\
 		scsi_opcode_name(SAI_READ_CAPACITY_16),		\
 		scsi_opcode_name(SAI_GET_LBA_STATUS),		\
 		scsi_opcode_name(MI_REPORT_TARGET_PGS),		\
diff --git a/include/trace/events/target.h b/include/trace/events/target.h
index da9cc0f..4540344 100644
--- a/include/trace/events/target.h
+++ b/include/trace/events/target.h
@@ -96,7 +96,7 @@
 		scsi_opcode_name(WRITE_16),			\
 		scsi_opcode_name(VERIFY_16),			\
 		scsi_opcode_name(WRITE_SAME_16),		\
-		scsi_opcode_name(SERVICE_ACTION_IN),		\
+		scsi_opcode_name(SERVICE_ACTION_IN_16),		\
 		scsi_opcode_name(SAI_READ_CAPACITY_16),		\
 		scsi_opcode_name(SAI_GET_LBA_STATUS),		\
 		scsi_opcode_name(MI_REPORT_TARGET_PGS),		\
diff --git a/tools/lib/traceevent/plugin_scsi.c b/tools/lib/traceevent/plugin_scsi.c
index eda326f..c699f47 100644
--- a/tools/lib/traceevent/plugin_scsi.c
+++ b/tools/lib/traceevent/plugin_scsi.c
@@ -107,7 +107,7 @@ typedef unsigned int u32;
 #define VERIFY_16			0x8f
 #define SYNCHRONIZE_CACHE_16		0x91
 #define WRITE_SAME_16			0x93
-#define SERVICE_ACTION_IN		0x9e
+#define SERVICE_ACTION_IN_16		0x9e
 /* values for service action in */
 #define	SAI_READ_CAPACITY_16		0x10
 #define SAI_GET_LBA_STATUS		0x12
@@ -393,7 +393,7 @@ scsi_trace_parse_cdb(struct trace_seq *p, unsigned char *cdb, int len)
 		return scsi_trace_rw16(p, cdb, len);
 	case UNMAP:
 		return scsi_trace_unmap(p, cdb, len);
-	case SERVICE_ACTION_IN:
+	case SERVICE_ACTION_IN_16:
 		return scsi_trace_service_action_in(p, cdb, len);
 	case VARIABLE_LENGTH_CMD:
 		return scsi_trace_varlen(p, cdb, len);
-- 
1.8.5.2

[PATCH 08/10] scsi: Add SPC-3 command definitions

[Hannes Reinecke]

SPC-3 defines SERVICE ACTION IN(12), SERVICE_ACTION OUT(12),
SERVICE ACTION OUT(16), and SERVICE ACTION BIDIRECTIONAL.
And READ MEDIA SERIAL NUMBER has long since been deprecated.
So update callers to refer to the new cdb name.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/constants.c           | 4 ----
 drivers/target/target_core_pr.c    | 2 +-
 include/scsi/scsi.h                | 6 +++++-
 tools/lib/traceevent/plugin_scsi.c | 5 ++++-
 4 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c
index 0d2ec4d..dfa8639 100644
--- a/drivers/scsi/constants.c
+++ b/drivers/scsi/constants.c
@@ -21,10 +21,6 @@
 
 
 /* Commands with service actions that change the command name */
-#define SERVICE_ACTION_IN_12 0xab
-#define SERVICE_ACTION_OUT_12 0xa9
-#define SERVICE_ACTION_BIDIRECTIONAL 0x9d
-#define SERVICE_ACTION_OUT_16 0x9f
 #define THIRD_PARTY_COPY_OUT 0x83
 #define THIRD_PARTY_COPY_IN 0x84
 
diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index 8c60a1a..806bfba 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -459,7 +459,7 @@ static int core_scsi3_pr_seq_non_holder(
 	case ACCESS_CONTROL_OUT:
 	case INQUIRY:
 	case LOG_SENSE:
-	case READ_MEDIA_SERIAL_NUMBER:
+	case SERVICE_ACTION_IN_12:
 	case REPORT_LUNS:
 	case REQUEST_SENSE:
 	case PERSISTENT_RESERVE_IN:
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index 046fe12..5607594 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -128,8 +128,10 @@ enum scsi_timeouts {
 #define MOVE_MEDIUM           0xa5
 #define EXCHANGE_MEDIUM       0xa6
 #define READ_12               0xa8
+#define SERVICE_ACTION_OUT_12 0xa9
 #define WRITE_12              0xaa
-#define READ_MEDIA_SERIAL_NUMBER 0xab
+#define READ_MEDIA_SERIAL_NUMBER 0xab /* Obsolete with SPC-2 */
+#define SERVICE_ACTION_IN_12 0xab
 #define WRITE_VERIFY_12       0xae
 #define VERIFY_12	      0xaf
 #define SEARCH_HIGH_12        0xb0
@@ -151,7 +153,9 @@ enum scsi_timeouts {
 #define VERIFY_16	      0x8f
 #define SYNCHRONIZE_CACHE_16  0x91
 #define WRITE_SAME_16	      0x93
+#define SERVICE_ACTION_BIDIRECTIONAL 0x9d
 #define SERVICE_ACTION_IN_16  0x9e
+#define SERVICE_ACTION_OUT_16 0x9f
 /* values for service action in */
 #define	SAI_READ_CAPACITY_16  0x10
 #define SAI_GET_LBA_STATUS    0x12
diff --git a/tools/lib/traceevent/plugin_scsi.c b/tools/lib/traceevent/plugin_scsi.c
index c699f47..63aba97 100644
--- a/tools/lib/traceevent/plugin_scsi.c
+++ b/tools/lib/traceevent/plugin_scsi.c
@@ -85,8 +85,9 @@ typedef unsigned int u32;
 #define MOVE_MEDIUM			0xa5
 #define EXCHANGE_MEDIUM			0xa6
 #define READ_12				0xa8
+#define SERVICE_ACTION_out_12		0xa9
 #define WRITE_12			0xaa
-#define READ_MEDIA_SERIAL_NUMBER	0xab
+#define SERVICE_ACTION_IN_12		0xab
 #define WRITE_VERIFY_12			0xae
 #define VERIFY_12			0xaf
 #define SEARCH_HIGH_12			0xb0
@@ -107,7 +108,9 @@ typedef unsigned int u32;
 #define VERIFY_16			0x8f
 #define SYNCHRONIZE_CACHE_16		0x91
 #define WRITE_SAME_16			0x93
+#define SERVICE_ACTION_BIDIRECTIONAL	0x9d
 #define SERVICE_ACTION_IN_16		0x9e
+#define SERVICE_ACTION_OUT_16		0x9f
 /* values for service action in */
 #define	SAI_READ_CAPACITY_16		0x10
 #define SAI_GET_LBA_STATUS		0x12
-- 
1.8.5.2

[PATCH 09/10] scsi: Conditionally compile in constants.c

[Hannes Reinecke]

Instead of having constants.c littered with ifdef statements
we should be moving dummy functions into the header and
condintionally compile in constants.c if selected.

Suggested-by: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/Makefile      |  4 ++--
 drivers/scsi/constants.c   | 42 ---------------------------------------
 drivers/xen/xen-scsiback.c |  1 +
 include/scsi/scsi_dbg.h    | 49 ++++++++++++++++++++++++++++++++++++++++++----
 4 files changed, 48 insertions(+), 48 deletions(-)

diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
index 4991b62..76f8932 100644
--- a/drivers/scsi/Makefile
+++ b/drivers/scsi/Makefile
@@ -158,9 +158,9 @@ obj-$(CONFIG_SCSI_OSD_INITIATOR) += osd/
 
 # This goes last, so that "real" scsi devices probe earlier
 obj-$(CONFIG_SCSI_DEBUG)	+= scsi_debug.o
-
-scsi_mod-y			+= scsi.o hosts.o scsi_ioctl.o constants.o \
+scsi_mod-y			+= scsi.o hosts.o scsi_ioctl.o \
 				   scsicam.o scsi_error.o scsi_lib.o
+scsi_mod-$(CONFIG_SCSI_CONSTANTS) += constants.o
 scsi_mod-$(CONFIG_SCSI_DMA)	+= scsi_lib_dma.o
 scsi_mod-y			+= scsi_scan.o scsi_sysfs.o scsi_devinfo.o
 scsi_mod-$(CONFIG_SCSI_NETLINK)	+= scsi_netlink.o
diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c
index dfa8639..a66572b 100644
--- a/drivers/scsi/constants.c
+++ b/drivers/scsi/constants.c
@@ -18,8 +18,6 @@
 #include <scsi/scsi_eh.h>
 #include <scsi/scsi_dbg.h>
 
-
-
 /* Commands with service actions that change the command name */
 #define THIRD_PARTY_COPY_OUT 0x83
 #define THIRD_PARTY_COPY_IN 0x84
@@ -35,7 +33,6 @@ struct value_name_pair {
 	const char * name;
 };
 
-#ifdef CONFIG_SCSI_CONSTANTS
 static const char * cdb_byte0_names[] = {
 /* 00-03 */ "Test Unit Ready", "Rezero Unit/Rewind", NULL, "Request Sense",
 /* 04-07 */ "Format Unit/Medium", "Read Block Limits", NULL,
@@ -259,26 +256,6 @@ static struct sa_name_list sa_names_arr[] = {
 	{0, NULL, 0},
 };
 
-#else /* ifndef CONFIG_SCSI_CONSTANTS */
-static const char *cdb_byte0_names[0];
-
-static struct sa_name_list sa_names_arr[] = {
-	{VARIABLE_LENGTH_CMD, NULL, 0},
-	{MAINTENANCE_IN, NULL, 0},
-	{MAINTENANCE_OUT, NULL, 0},
-	{PERSISTENT_RESERVE_IN, NULL, 0},
-	{PERSISTENT_RESERVE_OUT, NULL, 0},
-	{SERVICE_ACTION_IN_12, NULL, 0},
-	{SERVICE_ACTION_OUT_12, NULL, 0},
-	{SERVICE_ACTION_BIDIRECTIONAL, NULL, 0},
-	{SERVICE_ACTION_IN_16, NULL, 0},
-	{SERVICE_ACTION_OUT_16, NULL, 0},
-	{THIRD_PARTY_COPY_IN, NULL, 0},
-	{THIRD_PARTY_COPY_OUT, NULL, 0},
-	{0, NULL, 0},
-};
-#endif /* CONFIG_SCSI_CONSTANTS */
-
 bool scsi_opcode_sa_name(int opcode, int service_action,
 			 const char **cdb_name, const char **sa_name)
 {
@@ -313,8 +290,6 @@ bool scsi_opcode_sa_name(int opcode, int service_action,
 	return true;
 }
 
-#ifdef CONFIG_SCSI_CONSTANTS
-
 struct error_info {
 	unsigned short code12;	/* 0x0302 looks better than 0x03,0x02 */
 	const char * text;
@@ -1177,15 +1152,12 @@ static const char * const snstext[] = {
 	"Completed",	    /* F: command completed sense data reported,
 				  may occur for successful command */
 };
-#endif
 
 /* Get sense key string or NULL if not available */
 const char *
 scsi_sense_key_string(unsigned char key) {
-#ifdef CONFIG_SCSI_CONSTANTS
 	if (key <= 0xE)
 		return snstext[key];
-#endif
 	return NULL;
 }
 EXPORT_SYMBOL(scsi_sense_key_string);
@@ -1197,7 +1169,6 @@ EXPORT_SYMBOL(scsi_sense_key_string);
 const char *
 scsi_extd_sense_format(unsigned char asc, unsigned char ascq, const char **fmt)
 {
-#ifdef CONFIG_SCSI_CONSTANTS
 	int i;
 	unsigned short code = ((asc << 8) | ascq);
 
@@ -1211,13 +1182,10 @@ scsi_extd_sense_format(unsigned char asc, unsigned char ascq, const char **fmt)
 			*fmt = additional2[i].fmt;
 			return additional2[i].str;
 	}
-#endif
 	return NULL;
 }
 EXPORT_SYMBOL(scsi_extd_sense_format);
 
-#ifdef CONFIG_SCSI_CONSTANTS
-
 static const char * const hostbyte_table[]={
 "DID_OK", "DID_NO_CONNECT", "DID_BUS_BUSY", "DID_TIME_OUT", "DID_BAD_TARGET",
 "DID_ABORT", "DID_PARITY", "DID_ERROR", "DID_RESET", "DID_BAD_INTR",
@@ -1229,17 +1197,13 @@ static const char * const driverbyte_table[]={
 "DRIVER_OK", "DRIVER_BUSY", "DRIVER_SOFT",  "DRIVER_MEDIA", "DRIVER_ERROR",
 "DRIVER_INVALID", "DRIVER_TIMEOUT", "DRIVER_HARD", "DRIVER_SENSE"};
 
-#endif
-
 const char *scsi_hostbyte_string(int result)
 {
 	const char *hb_string = NULL;
-#ifdef CONFIG_SCSI_CONSTANTS
 	int hb = host_byte(result);
 
 	if (hb < ARRAY_SIZE(hostbyte_table))
 		hb_string = hostbyte_table[hb];
-#endif
 	return hb_string;
 }
 EXPORT_SYMBOL(scsi_hostbyte_string);
@@ -1247,17 +1211,14 @@ EXPORT_SYMBOL(scsi_hostbyte_string);
 const char *scsi_driverbyte_string(int result)
 {
 	const char *db_string = NULL;
-#ifdef CONFIG_SCSI_CONSTANTS
 	int db = driver_byte(result);
 
 	if (db < ARRAY_SIZE(driverbyte_table))
 		db_string = driverbyte_table[db];
-#endif
 	return db_string;
 }
 EXPORT_SYMBOL(scsi_driverbyte_string);
 
-#ifdef CONFIG_SCSI_CONSTANTS
 #define scsi_mlreturn_name(result)	{ result, #result }
 static const struct value_name_pair scsi_mlreturn_arr[] = {
 	scsi_mlreturn_name(NEEDS_RETRY),
@@ -1270,11 +1231,9 @@ static const struct value_name_pair scsi_mlreturn_arr[] = {
 	scsi_mlreturn_name(SCSI_RETURN_NOT_HANDLED),
 	scsi_mlreturn_name(FAST_IO_FAIL)
 };
-#endif
 
 const char *scsi_mlreturn_string(int result)
 {
-#ifdef CONFIG_SCSI_CONSTANTS
 	const struct value_name_pair *arr = scsi_mlreturn_arr;
 	int k;
 
@@ -1282,7 +1241,6 @@ const char *scsi_mlreturn_string(int result)
 		if (result == arr->value)
 			return arr->name;
 	}
-#endif
 	return NULL;
 }
 EXPORT_SYMBOL(scsi_mlreturn_string);
diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
index 50610a6..cc0ebf2 100644
--- a/drivers/xen/xen-scsiback.c
+++ b/drivers/xen/xen-scsiback.c
@@ -47,6 +47,7 @@
 
 #include <generated/utsrelease.h>
 
+#include <scsi/scsi.h>
 #include <scsi/scsi_dbg.h>
 #include <scsi/scsi_eh.h>
 #include <scsi/scsi_tcq.h>
diff --git a/include/scsi/scsi_dbg.h b/include/scsi/scsi_dbg.h
index c7ed7b8..56a6f27 100644
--- a/include/scsi/scsi_dbg.h
+++ b/include/scsi/scsi_dbg.h
@@ -7,7 +7,6 @@ struct scsi_sense_hdr;
 
 #define SCSI_LOG_BUFSIZE 128
 
-extern bool scsi_opcode_sa_name(int, int, const char **, const char **);
 extern void scsi_print_command(struct scsi_cmnd *);
 extern size_t __scsi_format_command(char *, size_t,
 				   const unsigned char *, size_t);
@@ -22,11 +21,53 @@ extern void __scsi_print_sense(const struct scsi_device *, const char *name,
 			       const unsigned char *sense_buffer,
 			       int sense_len);
 extern void scsi_print_result(struct scsi_cmnd *, const char *, int);
-extern const char *scsi_hostbyte_string(int);
-extern const char *scsi_driverbyte_string(int);
-extern const char *scsi_mlreturn_string(int);
+
+#ifdef CONFIG_SCSI_CONSTANTS
+extern bool scsi_opcode_sa_name(int, int, const char **, const char **);
 extern const char *scsi_sense_key_string(unsigned char);
 extern const char *scsi_extd_sense_format(unsigned char, unsigned char,
 					  const char **);
+extern const char *scsi_mlreturn_string(int);
+extern const char *scsi_hostbyte_string(int);
+extern const char *scsi_driverbyte_string(int);
+#else
+static inline bool
+scsi_opcode_sa_name(int cmd, int sa,
+		    const char **cdb_name, const char **sa_name)
+{
+	*cdb_name = NULL;
+	switch (cmd) {
+	case VARIABLE_LENGTH_CMD:
+	case MAINTENANCE_IN:
+	case MAINTENANCE_OUT:
+	case PERSISTENT_RESERVE_IN:
+	case PERSISTENT_RESERVE_OUT:
+	case SERVICE_ACTION_IN_12:
+	case SERVICE_ACTION_OUT_12:
+	case SERVICE_ACTION_BIDIRECTIONAL:
+	case SERVICE_ACTION_IN_16:
+	case SERVICE_ACTION_OUT_16:
+	case EXTENDED_COPY:
+	case RECEIVE_COPY_RESULTS:
+		*sa_name = NULL;
+		return true;
+	default:
+		return false;
+	}
+}
+
+static inline const char *scsi_sense_key_string(unsigned char key)
+{ return NULL; }
+static inline const char *scsi_extd_sense_format(unsigned char asc,
+					  unsigned char ascq,
+					  const char **fmt)
+{ *fmt = NULL; return NULL; }
+static inline const char *scsi_mlreturn_string(int result)
+{ return NULL; }
+static inline const char *scsi_hostbyte_string(int result)
+{ return NULL; }
+static inline const char *scsi_driverbyte_string(int result)
+{ return NULL; }
+#endif
 
 #endif /* _SCSI_SCSI_DBG_H */
-- 
1.8.5.2

[PATCH 10/10] scsi: Move remaining printk() statement to scmd_printk()

[Hannes Reinecke]

One statement was missing from the conversion to dev_printk().

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/scsi.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 92d5912..9880b59 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -541,10 +541,12 @@ void scsi_log_send(struct scsi_cmnd *cmd)
 				    "Send: scmd 0x%p\n", cmd);
 			scsi_print_command(cmd);
 			if (level > 3) {
-				printk(KERN_INFO "buffer = 0x%p, bufflen = %d,"
-				       " queuecommand 0x%p\n",
-					scsi_sglist(cmd), scsi_bufflen(cmd),
-					cmd->device->host->hostt->queuecommand);
+				struct Scsi_Host *shost = cmd->device->host;
+				scmd_printk(KERN_INFO, cmd,
+					    "buffer = 0x%p, bufflen = %d,"
+					    " queuecommand 0x%p\n",
+					    scsi_sglist(cmd), scsi_bufflen(cmd),
+					    shost->hostt->queuecommand);
 
 			}
 		}
-- 
1.8.5.2

Re: [PATCH 01/10] scsi: Use real functions for logging

[Christoph Hellwig]

On Tue, Nov 04, 2014 at 09:06:40AM +0100, Hannes Reinecke wrote:
> Implement a per-cpu buffer for formatting messages,
> and add real functions for scmd_printk and sdev_prefix_printk
> using that buffer and use dev_printk() to print out things.
> And make sdev_printk() a wrapper for sdev_prefix_printk().

The "real functions" mostly seem to be a side effect of the
implementation, how about a more descriptive subject line?

Also adding a sentence or two why this is done to the patch description
would be very helpful.

> +/*
> + * scsi_logging.c

Please don't mention the file name in top of file comments, it's bound
to get out of date.

> +#define SCSI_LOG_SPOOLSIZE 4096
> +#define SCSI_LOG_BUFSIZE 128
> +
> +struct scsi_log_buf {
> +	char buffer[SCSI_LOG_SPOOLSIZE];
> +	unsigned long map;
> +};
> +
> +static DEFINE_PER_CPU(struct scsi_log_buf, scsi_format_log);

Some sort of comment explaining why we need the per-cpu buffers also
would be very useful.  To me this seems like a little bit too much
duplication of the tracing code.

Re: [PATCH 01/10] scsi: Use real functions for logging

[Hannes Reinecke]

On 11/04/2014 02:58 PM, Christoph Hellwig wrote:
> On Tue, Nov 04, 2014 at 09:06:40AM +0100, Hannes Reinecke wrote:
>> Implement a per-cpu buffer for formatting messages,
>> and add real functions for scmd_printk and sdev_prefix_printk
>> using that buffer and use dev_printk() to print out things.
>> And make sdev_printk() a wrapper for sdev_prefix_printk().
> 
> The "real functions" mostly seem to be a side effect of the
> implementation, how about a more descriptive subject line?
> 
> Also adding a sentence or two why this is done to the patch description
> would be very helpful.
> 
Ok.

>> +/*
>> + * scsi_logging.c
> 
> Please don't mention the file name in top of file comments, it's bound
> to get out of date.
> 
Ok.

>> +#define SCSI_LOG_SPOOLSIZE 4096
>> +#define SCSI_LOG_BUFSIZE 128
>> +
>> +struct scsi_log_buf {
>> +	char buffer[SCSI_LOG_SPOOLSIZE];
>> +	unsigned long map;
>> +};
>> +
>> +static DEFINE_PER_CPU(struct scsi_log_buf, scsi_format_log);
> 
> Some sort of comment explaining why we need the per-cpu buffers also
> would be very useful.  To me this seems like a little bit too much
> duplication of the tracing code.
> 
Hehe. It is _not_.
(Not now, anyway).

The problem with the current tracing code is that it's split in two,
a writing side and a reading side.
So you cannot write to it and have it read back in the same function
without using really heavy locking.
But as we _do_ want to have the message printed out as soon as it's
being written we cannot use the tracing code as-is.

_And_ the current tracing infrastructure is using a single buffer,
so if we were to use it our messages would be interspersed with
other, unrelated, tracing messages.

So we'd need to implement our own buffer anyway, which is what
I did. And it was actually suggested by Stephen Rostedt at LPC :-)
Stephen is working on abstracting away some parts of the
tracing infrastructure for general consumption, but until
that's done one has to do it manually.

(The whole issue is just because printk() is so damn stupid.
If we had a printk() workqueue for printing out messages we could
separate the submission side from the logging side. Then
we could expose the internal functions which adds items
to the internal printk buffer, and we could work on those items
directly. But we haven't, and I don't think it's going to
change anytime soon.
Hmm. Would be fit topic for KS. For which I won't be invited.
So there.)

But yeah, I can update the description here.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/10] scsi: log request tag for scmd_printk()

[Christoph Hellwig]

On Tue, Nov 04, 2014 at 09:06:41AM +0100, Hannes Reinecke wrote:
> The request tag provides a concise identification of a SCSI
> command, so we should be printing that out for scmd_printk().
> 
> Suggested-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Hannes Reinecke <hare@suse.de>

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 03/10] scsi: use external buffer for command logging

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 04/10] libata: use __scsi_format_command()

[Christoph Hellwig]

On Tue, Nov 04, 2014 at 09:06:43AM +0100, Hannes Reinecke wrote:
> libata already uses an internal buffer, so we should be using
> __scsi_format_command() here.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 07/10] scsi: Rename SERVICE_ACTION_IN to SERVICE_ACTION_IN_16

[Christoph Hellwig]

On Tue, Nov 04, 2014 at 09:06:46AM +0100, Hannes Reinecke wrote:
> SPC-3 defines SERVICE ACTION IN(12) and SERVICE ACTION IN(16).
> So rename SERVICE_ACTION_IN to SERVICE_ACTION_IN_16 to be
> consistent with SPC and to allow for better distinction.

Looks fine, I'd really like to take this at the beginning of the series.

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 08/10] scsi: Add SPC-3 command definitions

[Christoph Hellwig]

On Tue, Nov 04, 2014 at 09:06:47AM +0100, Hannes Reinecke wrote:
> SPC-3 defines SERVICE ACTION IN(12), SERVICE_ACTION OUT(12),
> SERVICE ACTION OUT(16), and SERVICE ACTION BIDIRECTIONAL.
> And READ MEDIA SERIAL NUMBER has long since been deprecated.
> So update callers to refer to the new cdb name.

Looks good, and again I'd like to take this before all the major
logging rework.

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 09/10] scsi: Conditionally compile in constants.c

[Christoph Hellwig]

> +static inline const char *scsi_sense_key_string(unsigned char key)
> +{ return NULL; }
> +static inline const char *scsi_extd_sense_format(unsigned char asc,
> +					  unsigned char ascq,
> +					  const char **fmt)
> +{ *fmt = NULL; return NULL; }
> +static inline const char *scsi_mlreturn_string(int result)
> +{ return NULL; }
> +static inline const char *scsi_hostbyte_string(int result)
> +{ return NULL; }
> +static inline const char *scsi_driverbyte_string(int result)
> +{ return NULL; }

Please apply the normal code formatting rules to these.

Otherwise looks fine,,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 10/10] scsi: Move remaining printk() statement to scmd_printk()

[Christoph Hellwig]

On Tue, Nov 04, 2014 at 09:06:49AM +0100, Hannes Reinecke wrote:
> One statement was missing from the conversion to dev_printk().
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/scsi/scsi.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 92d5912..9880b59 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -541,10 +541,12 @@ void scsi_log_send(struct scsi_cmnd *cmd)
>  				    "Send: scmd 0x%p\n", cmd);
>  			scsi_print_command(cmd);
>  			if (level > 3) {
> -				printk(KERN_INFO "buffer = 0x%p, bufflen = %d,"
> -				       " queuecommand 0x%p\n",
> -					scsi_sglist(cmd), scsi_bufflen(cmd),
> -					cmd->device->host->hostt->queuecommand);
> +				struct Scsi_Host *shost = cmd->device->host;
> +				scmd_printk(KERN_INFO, cmd,
> +					    "buffer = 0x%p, bufflen = %d,"
> +					    " queuecommand 0x%p\n",
> +					    scsi_sglist(cmd), scsi_bufflen(cmd),
> +					    shost->hostt->queuecommand);

This printk doesn't look very useful, I'd rather kill it.

Re: [PATCH 05/10] scsi: use per-cpu buffer for formatting sense

[Christoph Hellwig]

> +#define scmd_format_header(b, l, d, t) \
> +	sdev_format_header(b, l, (d) ? (d)->disk_name : NULL, t)

I'd rather have a

static inline const char *scmd_name(struct scsi_cmnd *scmd)
{
	return scmd->request->rq_disk ?
		scmd->request->rq_disk->disk_name : NULL;
}

helper and use that directlu, which also gets rid of all the
local gendisk variables.

> +	if (scsi_sense_is_deferred(sshdr))
> +		off += scnprintf(buffer + off, buf_len - off, "[deferred] ");
> +	else
> +		off += scnprintf(buffer + off, buf_len - off, "[current] ");

	off += scnprintf(buffer + off, buf_len - off,
		scsi_sense_is_deferred(sshdr) ? "[deferred] " : "[current] ");

> +{
> +	struct scsi_sense_hdr sshdr;
> +
> +	if (!scsi_normalize_sense(sense_buffer, sense_len, &sshdr)) {
> +		int i;
> +
> +		for (i = 0; i < sense_len; i += 16) {
> +			char *logbuf;
> +			int len = min(sense_len - i, 16);
> +			size_t off, logbuf_len;
> +
> +			logbuf = scsi_log_reserve_buffer(&logbuf_len);
> +			if (!logbuf)
> +				break;
> +			off = sdev_format_header(logbuf, logbuf_len,
> +						 name, tag);
> +			hex_dump_to_buffer(&sense_buffer[i], len, 16, 1,
> +					   logbuf + off, logbuf_len - off,
> +					   false);
> +			dev_printk(KERN_INFO, &sdev->sdev_gendev, logbuf);
> +			scsi_log_release_buffer(logbuf);
> +		}
> +		return;

Keeping the code inside the if in a separate helper would probably be a
lot more readable.

Re: [PATCH 10/10] scsi: Move remaining printk() statement to scmd_printk()

[Hannes Reinecke]

On 11/06/2014 08:20 AM, Christoph Hellwig wrote:
> On Tue, Nov 04, 2014 at 09:06:49AM +0100, Hannes Reinecke wrote:
>> One statement was missing from the conversion to dev_printk().
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>  drivers/scsi/scsi.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
>> index 92d5912..9880b59 100644
>> --- a/drivers/scsi/scsi.c
>> +++ b/drivers/scsi/scsi.c
>> @@ -541,10 +541,12 @@ void scsi_log_send(struct scsi_cmnd *cmd)
>>  				    "Send: scmd 0x%p\n", cmd);
>>  			scsi_print_command(cmd);
>>  			if (level > 3) {
>> -				printk(KERN_INFO "buffer = 0x%p, bufflen = %d,"
>> -				       " queuecommand 0x%p\n",
>> -					scsi_sglist(cmd), scsi_bufflen(cmd),
>> -					cmd->device->host->hostt->queuecommand);
>> +				struct Scsi_Host *shost = cmd->device->host;
>> +				scmd_printk(KERN_INFO, cmd,
>> +					    "buffer = 0x%p, bufflen = %d,"
>> +					    " queuecommand 0x%p\n",
>> +					    scsi_sglist(cmd), scsi_bufflen(cmd),
>> +					    shost->hostt->queuecommand);
> 
> This printk doesn't look very useful, I'd rather kill it.
> 
Be my guest.

I'm planning on removing all the references to command pointers
anyway, but for that I'll need a consistent scsi command
enumeration. Your tcq rework should help for that.

But that will be done in a later patch series.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/10] scsi: use per-cpu buffer for formatting sense

[Hannes Reinecke]

On 11/06/2014 08:33 AM, Christoph Hellwig wrote:
>> +#define scmd_format_header(b, l, d, t) \
>> +	sdev_format_header(b, l, (d) ? (d)->disk_name : NULL, t)
> 
> I'd rather have a
> 
> static inline const char *scmd_name(struct scsi_cmnd *scmd)
> {
> 	return scmd->request->rq_disk ?
> 		scmd->request->rq_disk->disk_name : NULL;
> }
> 
> helper and use that directlu, which also gets rid of all the
> local gendisk variables.
> 
Ok.

>> +	if (scsi_sense_is_deferred(sshdr))
>> +		off += scnprintf(buffer + off, buf_len - off, "[deferred] ");
>> +	else
>> +		off += scnprintf(buffer + off, buf_len - off, "[current] ");
> 
> 	off += scnprintf(buffer + off, buf_len - off,
> 		scsi_sense_is_deferred(sshdr) ? "[deferred] " : "[current] ");
> 
>> +{
>> +	struct scsi_sense_hdr sshdr;
>> +
>> +	if (!scsi_normalize_sense(sense_buffer, sense_len, &sshdr)) {
>> +		int i;
>> +
>> +		for (i = 0; i < sense_len; i += 16) {
>> +			char *logbuf;
>> +			int len = min(sense_len - i, 16);
>> +			size_t off, logbuf_len;
>> +
>> +			logbuf = scsi_log_reserve_buffer(&logbuf_len);
>> +			if (!logbuf)
>> +				break;
>> +			off = sdev_format_header(logbuf, logbuf_len,
>> +						 name, tag);
>> +			hex_dump_to_buffer(&sense_buffer[i], len, 16, 1,
>> +					   logbuf + off, logbuf_len - off,
>> +					   false);
>> +			dev_printk(KERN_INFO, &sdev->sdev_gendev, logbuf);
>> +			scsi_log_release_buffer(logbuf);
>> +		}
>> +		return;
> 
> Keeping the code inside the if in a separate helper would probably be a
> lot more readable.
> 
Ok. Will be doing so.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 01/10] scsi: Use real functions for logging

[Elliott, Robert (Server Storage)]

> -----Original Message-----
> From: Hannes Reinecke [mailto:hare@suse.de]
> Sent: Tuesday, 04 November, 2014 2:07 AM
...
> diff --git a/drivers/scsi/scsi_logging.c
> b/drivers/scsi/scsi_logging.c
...
> @@ -0,0 +1,119 @@
> +/*
> + * scsi_logging.c
> + *
> + * Copyright (C) 2014 SUSE Linux Products GmbH
> + * Copyright (C) 2014 Hannes Reinecke <hare@suse.de>
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/atomic.h>
> +
> +#include <scsi/scsi.h>
> +#include <scsi/scsi_cmnd.h>
> +#include <scsi/scsi_device.h>
> +#include <scsi/scsi_dbg.h>
> +
> +#define SCSI_LOG_SPOOLSIZE 4096
> +#define SCSI_LOG_BUFSIZE 128
> +
> +struct scsi_log_buf {
> +	char buffer[SCSI_LOG_SPOOLSIZE];
> +	unsigned long map;
> +};
> +
> +static DEFINE_PER_CPU(struct scsi_log_buf, scsi_format_log);
> +
> +static char *scsi_log_reserve_buffer(size_t *len)
> +{
> +	struct scsi_log_buf *buf;
> +	unsigned long map_bits = SCSI_LOG_SPOOLSIZE / SCSI_LOG_BUFSIZE;
> +	unsigned long idx = 0;
> +
> +	WARN_ON(map_bits > BITS_PER_LONG);

Since SCSI_LOG_SPOOLSIZE, SCSI_LOG_BUFSIZE, and BITS_PER_LONG
are constants, that can be a compile-time check.

> +	preempt_disable();
> +	buf = this_cpu_ptr(&scsi_format_log);
> +	idx = find_first_zero_bit(&buf->map, map_bits);

If this fails to find a bit, it returns map_bits.
This could result in the next test_and_set_bit call 
accessing an address that is outside the bounds of
buf->map.

A safety check seems prudent before the test_and_set_bit:
	if (likely(idx < map_bits))

> +	while (test_and_set_bit(idx, &buf->map)) {
> +		idx = find_next_zero_bit(&buf->map, map_bits, idx);
> +		if (idx >= map_bits) {
> +			break;
> +		}

scripts/checkpatch.pl -f doesn't like the {} on that.

> +	}
> +	if (WARN_ON(idx >= map_bits)) {
> +		preempt_enable();
> +		return NULL;
> +	}
> +	*len = SCSI_LOG_BUFSIZE;
> +	return buf->buffer + idx * SCSI_LOG_BUFSIZE;
> +}
> +
> +static void scsi_log_release_buffer(char *bufptr)
> +{
> +	struct scsi_log_buf *buf;
> +	unsigned long idx;
> +	int ret;
> +
> +	buf = this_cpu_ptr(&scsi_format_log);
> +	if (bufptr < buf->buffer + SCSI_LOG_SPOOLSIZE) {

Should that also check that bufptr > buf->buffer?

> +		idx = (bufptr - buf->buffer) / SCSI_LOG_BUFSIZE;
> +		ret = test_and_clear_bit(idx, &buf->map);
> +		WARN_ON(!ret);
> +	}
> +	preempt_enable();
> +}
> +
> +int sdev_prefix_printk(const char *level, const struct scsi_device
> *sdev,
> +		       const char *name, const char *fmt, ...)
> +{
> +	va_list args;
> +	char *logbuf;
> +	size_t off = 0, logbuf_len;
> +	int ret;
> +
> +	if (!sdev)
> +		return 0;
> +
> +	logbuf = scsi_log_reserve_buffer(&logbuf_len);
> +	if (!logbuf)
> +		return 0;
> +
> +	if (name)
> +		off += scnprintf(logbuf + off, logbuf_len - off,
> +				 "[%s] ", name);
> +	va_start(args, fmt);
> +	off += vscnprintf(logbuf + off, logbuf_len - off, fmt, args);
> +	va_end(args);
> +	ret = dev_printk(level, &sdev->sdev_gendev, "%s", logbuf);
> +	scsi_log_release_buffer(logbuf);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(sdev_prefix_printk);
> +
> +int scmd_printk(const char *level, const struct scsi_cmnd *scmd,
> +		const char *fmt, ...)
> +{
> +	struct gendisk *disk = scmd->request->rq_disk;
> +	va_list args;
> +	char *logbuf;
> +	size_t off = 0, logbuf_len;
> +	int ret;
> +
> +	if (!scmd || scmd->cmnd == NULL)
> +		return 0;

!scmd->cmnd seems more common in neighboring code.

> +
> +	logbuf = scsi_log_reserve_buffer(&logbuf_len);
> +	if (!logbuf)
> +		return 0;
> +	if (disk)
> +		off += scnprintf(logbuf + off, logbuf_len - off,
> +				 "[%s] ", disk->disk_name);
> +	va_start(args, fmt);
> +	off += vscnprintf(logbuf + off, logbuf_len - off, fmt, args);
> +	va_end(args);
> +	ret = dev_printk(level, &scmd->device->sdev_gendev, "%s",
> logbuf);
> +	scsi_log_release_buffer(logbuf);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(scmd_printk);
...

dev_printk is EXPORT_SYMBOL.  The former macro versions of 
sdev_printk and scmd_printk just called dev_printk, and not
being symbols, did not change that.

So, I think these function versions should use EXPORT_SYMBOL, so 
the hated binary drivers can still call them.


---
Rob Elliott    HP Server Storage

Re: [PATCH 01/10] scsi: Use real functions for logging

[Hannes Reinecke]

On 11/11/2014 06:38 PM, Elliott, Robert (Server Storage) wrote:
> 
> 
>> -----Original Message-----
>> From: Hannes Reinecke [mailto:hare@suse.de]
>> Sent: Tuesday, 04 November, 2014 2:07 AM
> ...
>> diff --git a/drivers/scsi/scsi_logging.c
>> b/drivers/scsi/scsi_logging.c
> ...
>> @@ -0,0 +1,119 @@
>> +/*
>> + * scsi_logging.c
>> + *
>> + * Copyright (C) 2014 SUSE Linux Products GmbH
>> + * Copyright (C) 2014 Hannes Reinecke <hare@suse.de>
>> + *
>> + * This file is released under the GPLv2
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/atomic.h>
>> +
>> +#include <scsi/scsi.h>
>> +#include <scsi/scsi_cmnd.h>
>> +#include <scsi/scsi_device.h>
>> +#include <scsi/scsi_dbg.h>
>> +
>> +#define SCSI_LOG_SPOOLSIZE 4096
>> +#define SCSI_LOG_BUFSIZE 128
>> +
>> +struct scsi_log_buf {
>> +	char buffer[SCSI_LOG_SPOOLSIZE];
>> +	unsigned long map;
>> +};
>> +
>> +static DEFINE_PER_CPU(struct scsi_log_buf, scsi_format_log);
>> +
>> +static char *scsi_log_reserve_buffer(size_t *len)
>> +{
>> +	struct scsi_log_buf *buf;
>> +	unsigned long map_bits = SCSI_LOG_SPOOLSIZE / SCSI_LOG_BUFSIZE;
>> +	unsigned long idx = 0;
>> +
>> +	WARN_ON(map_bits > BITS_PER_LONG);
> 
> Since SCSI_LOG_SPOOLSIZE, SCSI_LOG_BUFSIZE, and BITS_PER_LONG
> are constants, that can be a compile-time check.
> 
Ok.

>> +	preempt_disable();
>> +	buf = this_cpu_ptr(&scsi_format_log);
>> +	idx = find_first_zero_bit(&buf->map, map_bits);
> 
> If this fails to find a bit, it returns map_bits.
> This could result in the next test_and_set_bit call 
> accessing an address that is outside the bounds of
> buf->map.
> 
> A safety check seems prudent before the test_and_set_bit:
> 	if (likely(idx < map_bits))
> 
Ah. I wasn't quite sure what find_first_zero_bit would return
on failure. But yeah, that check seems to be appropriate.

>> +	while (test_and_set_bit(idx, &buf->map)) {
>> +		idx = find_next_zero_bit(&buf->map, map_bits, idx);
>> +		if (idx >= map_bits) {
>> +			break;
>> +		}
> 
> scripts/checkpatch.pl -f doesn't like the {} on that.
> 
Sigh.

>> +	}
>> +	if (WARN_ON(idx >= map_bits)) {
>> +		preempt_enable();
>> +		return NULL;
>> +	}
>> +	*len = SCSI_LOG_BUFSIZE;
>> +	return buf->buffer + idx * SCSI_LOG_BUFSIZE;
>> +}
>> +
>> +static void scsi_log_release_buffer(char *bufptr)
>> +{
>> +	struct scsi_log_buf *buf;
>> +	unsigned long idx;
>> +	int ret;
>> +
>> +	buf = this_cpu_ptr(&scsi_format_log);
>> +	if (bufptr < buf->buffer + SCSI_LOG_SPOOLSIZE) {
> 
> Should that also check that bufptr > buf->buffer?
> 
Probably.

>> +		idx = (bufptr - buf->buffer) / SCSI_LOG_BUFSIZE;
>> +		ret = test_and_clear_bit(idx, &buf->map);
>> +		WARN_ON(!ret);
>> +	}
>> +	preempt_enable();
>> +}
>> +
>> +int sdev_prefix_printk(const char *level, const struct scsi_device
>> *sdev,
>> +		       const char *name, const char *fmt, ...)
>> +{
>> +	va_list args;
>> +	char *logbuf;
>> +	size_t off = 0, logbuf_len;
>> +	int ret;
>> +
>> +	if (!sdev)
>> +		return 0;
>> +
>> +	logbuf = scsi_log_reserve_buffer(&logbuf_len);
>> +	if (!logbuf)
>> +		return 0;
>> +
>> +	if (name)
>> +		off += scnprintf(logbuf + off, logbuf_len - off,
>> +				 "[%s] ", name);
>> +	va_start(args, fmt);
>> +	off += vscnprintf(logbuf + off, logbuf_len - off, fmt, args);
>> +	va_end(args);
>> +	ret = dev_printk(level, &sdev->sdev_gendev, "%s", logbuf);
>> +	scsi_log_release_buffer(logbuf);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(sdev_prefix_printk);
>> +
>> +int scmd_printk(const char *level, const struct scsi_cmnd *scmd,
>> +		const char *fmt, ...)
>> +{
>> +	struct gendisk *disk = scmd->request->rq_disk;
>> +	va_list args;
>> +	char *logbuf;
>> +	size_t off = 0, logbuf_len;
>> +	int ret;
>> +
>> +	if (!scmd || scmd->cmnd == NULL)
>> +		return 0;
> 
> !scmd->cmnd seems more common in neighboring code.
> 
Ok.

>> +
>> +	logbuf = scsi_log_reserve_buffer(&logbuf_len);
>> +	if (!logbuf)
>> +		return 0;
>> +	if (disk)
>> +		off += scnprintf(logbuf + off, logbuf_len - off,
>> +				 "[%s] ", disk->disk_name);
>> +	va_start(args, fmt);
>> +	off += vscnprintf(logbuf + off, logbuf_len - off, fmt, args);
>> +	va_end(args);
>> +	ret = dev_printk(level, &scmd->device->sdev_gendev, "%s",
>> logbuf);
>> +	scsi_log_release_buffer(logbuf);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(scmd_printk);
> ...
> 
> dev_printk is EXPORT_SYMBOL.  The former macro versions of 
> sdev_printk and scmd_printk just called dev_printk, and not
> being symbols, did not change that.
> 
> So, I think these function versions should use EXPORT_SYMBOL, so 
> the hated binary drivers can still call them.
> 
Okay, will be fixing things up for the next revision.

Thanks for the review.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html