[PATCH v3 0/5] Target sense data handling modifications

[PATCH v3 0/5] Target sense data handling modifications

[Sagi Grimberg]

This patch set modifies the target sense data handling.
First, cleanup transport_send_check_condition_and_sense()
by splitting the sense translation to a separate function.
Second, convert sense reason the switch statement to a table
driven code. Third, Use scsi common helpers to correctly set
the sense buffer. Last, Fix wrong setting of t10-pi errors
(non descriptor format).

Changes from v2:
- Pass sense_reason_t to scsi_translate_sense()
- Split patch 3:
  1) move the helpers to scsi_common.c
  2) use helpers in the target code
  3) always use descriptor-type sense data for PI errors

Changes from v1:
- Added Reviewed-by tags for patches 1,2
- Fixed compilation error after testing patch #3 on scsi/for-next
  branch. Moved scsi_sense_desc_find() to scsi_common as well (dependency)
  and also moved <asm/unaligned.h> include to scsi_common.h

Changes from v0:
- Added Bart's patches and converted my patch to apply over his
- Moved scsi sense helpers to scsi_common

Bart Van Assche (2):
  target: Inline transport_get_sense_codes()
  target: Split transport_send_check_condition_and_sense()

Sagi Grimberg (3):
  scsi: Move sense handling routines to scsi_common
  target: Use scsi helpers to build the sense data correctly
  target: Fix wrong setting of sense format for PI errors

 drivers/scsi/scsi_common.c             |  98 ++++++++
 drivers/scsi/scsi_error.c              |  99 +-------
 drivers/target/target_core_spc.c       |  31 +--
 drivers/target/target_core_transport.c | 399 ++++++++++++---------------------
 include/scsi/scsi_common.h             |   5 +
 include/scsi/scsi_eh.h                 |   7 +-
 include/target/target_core_base.h      |   1 +
 7 files changed, 256 insertions(+), 384 deletions(-)

-- 
1.8.4.3

[PATCH v3 1/5] target: Inline transport_get_sense_codes()

[Sagi Grimberg]

From: Bart Van Assche <bart.vanassche@sandisk.com>

Inline this function in its call site since it performs a trivial
task and since it is only called once.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 drivers/target/target_core_transport.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 1bc8378..e895156 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2620,17 +2620,6 @@ bool transport_wait_for_tasks(struct se_cmd *cmd)
 }
 EXPORT_SYMBOL(transport_wait_for_tasks);
 
-static int transport_get_sense_codes(
-	struct se_cmd *cmd,
-	u8 *asc,
-	u8 *ascq)
-{
-	*asc = cmd->scsi_asc;
-	*ascq = cmd->scsi_ascq;
-
-	return 0;
-}
-
 static
 void transport_err_sector_info(unsigned char *buffer, sector_t bad_sector)
 {
@@ -2824,9 +2813,8 @@ transport_send_check_condition_and_sense(struct se_cmd *cmd,
 		buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
 		/* Not Ready */
 		buffer[SPC_SENSE_KEY_OFFSET] = NOT_READY;
-		transport_get_sense_codes(cmd, &asc, &ascq);
-		buffer[SPC_ASC_KEY_OFFSET] = asc;
-		buffer[SPC_ASCQ_KEY_OFFSET] = ascq;
+		buffer[SPC_ASC_KEY_OFFSET] = cmd->scsi_asc;
+		buffer[SPC_ASCQ_KEY_OFFSET] = cmd->scsi_ascq;
 		break;
 	case TCM_MISCOMPARE_VERIFY:
 		/* CURRENT ERROR */
-- 
1.8.4.3

[PATCH v3 2/5] target: Split transport_send_check_condition_and_sense()

[Sagi Grimberg]

From: Bart Van Assche <bart.vanassche@sandisk.com>

Move the code for translating a sense_reason_t code into a SCSI status
ASC and ASCQ codes from transport_send_check_condition_and_sense() into
the new function translate_sense_reason(). Convert the switch statement
that performs the translation into table-driven code.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
---
 drivers/target/target_core_transport.c | 383 +++++++++++++--------------------
 1 file changed, 148 insertions(+), 235 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index e895156..fb134d9 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2633,13 +2633,155 @@ void transport_err_sector_info(unsigned char *buffer, sector_t bad_sector)
 	put_unaligned_be64(bad_sector, &buffer[12]);
 }
 
+struct sense_info {
+	u8 key;
+	u8 asc;
+	u8 ascq;
+	bool add_sector_info;
+};
+
+static const struct sense_info sense_info_table[] = {
+	[TCM_NO_SENSE] = {
+		.key = NOT_READY
+	},
+	[TCM_NON_EXISTENT_LUN] = {
+		.key = ILLEGAL_REQUEST,
+		.asc = 0x25 /* LOGICAL UNIT NOT SUPPORTED */
+	},
+	[TCM_UNSUPPORTED_SCSI_OPCODE] = {
+		.key = ILLEGAL_REQUEST,
+		.asc = 0x20, /* INVALID COMMAND OPERATION CODE */
+	},
+	[TCM_SECTOR_COUNT_TOO_MANY] = {
+		.key = ILLEGAL_REQUEST,
+		.asc = 0x20, /* INVALID COMMAND OPERATION CODE */
+	},
+	[TCM_UNKNOWN_MODE_PAGE] = {
+		.key = ILLEGAL_REQUEST,
+		.asc = 0x24, /* INVALID FIELD IN CDB */
+	},
+	[TCM_CHECK_CONDITION_ABORT_CMD] = {
+		.key = ABORTED_COMMAND,
+		.asc = 0x29, /* BUS DEVICE RESET FUNCTION OCCURRED */
+		.ascq = 0x03,
+	},
+	[TCM_INCORRECT_AMOUNT_OF_DATA] = {
+		.key = ABORTED_COMMAND,
+		.asc = 0x0c, /* WRITE ERROR */
+		.ascq = 0x0d, /* NOT ENOUGH UNSOLICITED DATA */
+	},
+	[TCM_INVALID_CDB_FIELD] = {
+		.key = ILLEGAL_REQUEST,
+		.asc = 0x24, /* INVALID FIELD IN CDB */
+	},
+	[TCM_INVALID_PARAMETER_LIST] = {
+		.key = ILLEGAL_REQUEST,
+		.asc = 0x26, /* INVALID FIELD IN PARAMETER LIST */
+	},
+	[TCM_PARAMETER_LIST_LENGTH_ERROR] = {
+		.key = ILLEGAL_REQUEST,
+		.asc = 0x1a, /* PARAMETER LIST LENGTH ERROR */
+	},
+	[TCM_UNEXPECTED_UNSOLICITED_DATA] = {
+		.key = ILLEGAL_REQUEST,
+		.asc = 0x0c, /* WRITE ERROR */
+		.ascq = 0x0c, /* UNEXPECTED_UNSOLICITED_DATA */
+	},
+	[TCM_SERVICE_CRC_ERROR] = {
+		.key = ABORTED_COMMAND,
+		.asc = 0x47, /* PROTOCOL SERVICE CRC ERROR */
+		.ascq = 0x05, /* N/A */
+	},
+	[TCM_SNACK_REJECTED] = {
+		.key = ABORTED_COMMAND,
+		.asc = 0x11, /* READ ERROR */
+		.ascq = 0x13, /* FAILED RETRANSMISSION REQUEST */
+	},
+	[TCM_WRITE_PROTECTED] = {
+		.key = DATA_PROTECT,
+		.asc = 0x27, /* WRITE PROTECTED */
+	},
+	[TCM_ADDRESS_OUT_OF_RANGE] = {
+		.key = ILLEGAL_REQUEST,
+		.asc = 0x21, /* LOGICAL BLOCK ADDRESS OUT OF RANGE */
+	},
+	[TCM_CHECK_CONDITION_UNIT_ATTENTION] = {
+		.key = UNIT_ATTENTION,
+	},
+	[TCM_CHECK_CONDITION_NOT_READY] = {
+		.key = NOT_READY,
+	},
+	[TCM_MISCOMPARE_VERIFY] = {
+		.key = MISCOMPARE,
+		.asc = 0x1d, /* MISCOMPARE DURING VERIFY OPERATION */
+		.ascq = 0x00,
+	},
+	[TCM_LOGICAL_BLOCK_GUARD_CHECK_FAILED] = {
+		.key = ILLEGAL_REQUEST,
+		.asc = 0x10,
+		.ascq = 0x01, /* LOGICAL BLOCK GUARD CHECK FAILED */
+		.add_sector_info = true,
+	},
+	[TCM_LOGICAL_BLOCK_APP_TAG_CHECK_FAILED] = {
+		.key = ILLEGAL_REQUEST,
+		.asc = 0x10,
+		.ascq = 0x02, /* LOGICAL BLOCK APPLICATION TAG CHECK FAILED */
+		.add_sector_info = true,
+	},
+	[TCM_LOGICAL_BLOCK_REF_TAG_CHECK_FAILED] = {
+		.key = ILLEGAL_REQUEST,
+		.asc = 0x10,
+		.ascq = 0x03, /* LOGICAL BLOCK REFERENCE TAG CHECK FAILED */
+		.add_sector_info = true,
+	},
+	[TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE] = {
+		/*
+		 * Returning ILLEGAL REQUEST would cause immediate IO errors on
+		 * Solaris initiators.  Returning NOT READY instead means the
+		 * operations will be retried a finite number of times and we
+		 * can survive intermittent errors.
+		 */
+		.key = NOT_READY,
+		.asc = 0x08, /* LOGICAL UNIT COMMUNICATION FAILURE */
+	},
+};
+
+static void translate_sense_reason(struct se_cmd *cmd, sense_reason_t reason)
+{
+	const struct sense_info *si;
+	u8 *buffer = cmd->sense_buffer;
+	int r = (__force int)reason;
+	u8 asc, ascq;
+
+	if (r < ARRAY_SIZE(sense_info_table) && sense_info_table[r].key)
+		si = &sense_info_table[r];
+	else
+		si = &sense_info_table[(__force int)
+				       TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE];
+
+	buffer[SPC_SENSE_KEY_OFFSET] = si->key;
+	if (r == (__force int)TCM_CHECK_CONDITION_UNIT_ATTENTION) {
+		core_scsi3_ua_for_check_condition(cmd, &asc, &ascq);
+		WARN_ON_ONCE(asc == 0);
+	} else if (si->asc == 0) {
+		WARN_ON_ONCE(cmd->scsi_asc == 0);
+		asc = cmd->scsi_asc;
+		ascq = cmd->scsi_ascq;
+	} else {
+		asc = si->asc;
+		ascq = si->ascq;
+	}
+	buffer[SPC_ASC_KEY_OFFSET] = asc;
+	buffer[SPC_ASCQ_KEY_OFFSET] = ascq;
+	if (si->add_sector_info)
+		transport_err_sector_info(cmd->sense_buffer, cmd->bad_sector);
+}
+
 int
 transport_send_check_condition_and_sense(struct se_cmd *cmd,
 		sense_reason_t reason, int from_transport)
 {
-	unsigned char *buffer = cmd->sense_buffer;
 	unsigned long flags;
-	u8 asc = 0, ascq = 0;
 
 	spin_lock_irqsave(&cmd->t_state_lock, flags);
 	if (cmd->se_cmd_flags & SCF_SENT_CHECK_CONDITION) {
@@ -2649,242 +2791,13 @@ transport_send_check_condition_and_sense(struct se_cmd *cmd,
 	cmd->se_cmd_flags |= SCF_SENT_CHECK_CONDITION;
 	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
 
-	if (!reason && from_transport)
-		goto after_reason;
-
-	if (!from_transport)
+	if (!from_transport) {
 		cmd->se_cmd_flags |= SCF_EMULATED_TASK_SENSE;
-
-	/*
-	 * Actual SENSE DATA, see SPC-3 7.23.2  SPC_SENSE_KEY_OFFSET uses
-	 * SENSE KEY values from include/scsi/scsi.h
-	 */
-	switch (reason) {
-	case TCM_NO_SENSE:
-		/* CURRENT ERROR */
-		buffer[0] = 0x70;
-		buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
-		/* Not Ready */
-		buffer[SPC_SENSE_KEY_OFFSET] = NOT_READY;
-		/* NO ADDITIONAL SENSE INFORMATION */
-		buffer[SPC_ASC_KEY_OFFSET] = 0;
-		buffer[SPC_ASCQ_KEY_OFFSET] = 0;
-		break;
-	case TCM_NON_EXISTENT_LUN:
-		/* CURRENT ERROR */
-		buffer[0] = 0x70;
-		buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
-		/* ILLEGAL REQUEST */
-		buffer[SPC_SENSE_KEY_OFFSET] = ILLEGAL_REQUEST;
-		/* LOGICAL UNIT NOT SUPPORTED */
-		buffer[SPC_ASC_KEY_OFFSET] = 0x25;
-		break;
-	case TCM_UNSUPPORTED_SCSI_OPCODE:
-	case TCM_SECTOR_COUNT_TOO_MANY:
-		/* CURRENT ERROR */
-		buffer[0] = 0x70;
-		buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
-		/* ILLEGAL REQUEST */
-		buffer[SPC_SENSE_KEY_OFFSET] = ILLEGAL_REQUEST;
-		/* INVALID COMMAND OPERATION CODE */
-		buffer[SPC_ASC_KEY_OFFSET] = 0x20;
-		break;
-	case TCM_UNKNOWN_MODE_PAGE:
-		/* CURRENT ERROR */
-		buffer[0] = 0x70;
-		buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
-		/* ILLEGAL REQUEST */
-		buffer[SPC_SENSE_KEY_OFFSET] = ILLEGAL_REQUEST;
-		/* INVALID FIELD IN CDB */
-		buffer[SPC_ASC_KEY_OFFSET] = 0x24;
-		break;
-	case TCM_CHECK_CONDITION_ABORT_CMD:
-		/* CURRENT ERROR */
-		buffer[0] = 0x70;
-		buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
-		/* ABORTED COMMAND */
-		buffer[SPC_SENSE_KEY_OFFSET] = ABORTED_COMMAND;
-		/* BUS DEVICE RESET FUNCTION OCCURRED */
-		buffer[SPC_ASC_KEY_OFFSET] = 0x29;
-		buffer[SPC_ASCQ_KEY_OFFSET] = 0x03;
-		break;
-	case TCM_INCORRECT_AMOUNT_OF_DATA:
-		/* CURRENT ERROR */
-		buffer[0] = 0x70;
-		buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
-		/* ABORTED COMMAND */
-		buffer[SPC_SENSE_KEY_OFFSET] = ABORTED_COMMAND;
-		/* WRITE ERROR */
-		buffer[SPC_ASC_KEY_OFFSET] = 0x0c;
-		/* NOT ENOUGH UNSOLICITED DATA */
-		buffer[SPC_ASCQ_KEY_OFFSET] = 0x0d;
-		break;
-	case TCM_INVALID_CDB_FIELD:
-		/* CURRENT ERROR */
-		buffer[0] = 0x70;
-		buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
-		/* ILLEGAL REQUEST */
-		buffer[SPC_SENSE_KEY_OFFSET] = ILLEGAL_REQUEST;
-		/* INVALID FIELD IN CDB */
-		buffer[SPC_ASC_KEY_OFFSET] = 0x24;
-		break;
-	case TCM_INVALID_PARAMETER_LIST:
-		/* CURRENT ERROR */
-		buffer[0] = 0x70;
-		buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
-		/* ILLEGAL REQUEST */
-		buffer[SPC_SENSE_KEY_OFFSET] = ILLEGAL_REQUEST;
-		/* INVALID FIELD IN PARAMETER LIST */
-		buffer[SPC_ASC_KEY_OFFSET] = 0x26;
-		break;
-	case TCM_PARAMETER_LIST_LENGTH_ERROR:
-		/* CURRENT ERROR */
-		buffer[0] = 0x70;
-		buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
-		/* ILLEGAL REQUEST */
-		buffer[SPC_SENSE_KEY_OFFSET] = ILLEGAL_REQUEST;
-		/* PARAMETER LIST LENGTH ERROR */
-		buffer[SPC_ASC_KEY_OFFSET] = 0x1a;
-		break;
-	case TCM_UNEXPECTED_UNSOLICITED_DATA:
-		/* CURRENT ERROR */
-		buffer[0] = 0x70;
-		buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
-		/* ABORTED COMMAND */
-		buffer[SPC_SENSE_KEY_OFFSET] = ABORTED_COMMAND;
-		/* WRITE ERROR */
-		buffer[SPC_ASC_KEY_OFFSET] = 0x0c;
-		/* UNEXPECTED_UNSOLICITED_DATA */
-		buffer[SPC_ASCQ_KEY_OFFSET] = 0x0c;
-		break;
-	case TCM_SERVICE_CRC_ERROR:
-		/* CURRENT ERROR */
-		buffer[0] = 0x70;
-		buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
-		/* ABORTED COMMAND */
-		buffer[SPC_SENSE_KEY_OFFSET] = ABORTED_COMMAND;
-		/* PROTOCOL SERVICE CRC ERROR */
-		buffer[SPC_ASC_KEY_OFFSET] = 0x47;
-		/* N/A */
-		buffer[SPC_ASCQ_KEY_OFFSET] = 0x05;
-		break;
-	case TCM_SNACK_REJECTED:
-		/* CURRENT ERROR */
-		buffer[0] = 0x70;
-		buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
-		/* ABORTED COMMAND */
-		buffer[SPC_SENSE_KEY_OFFSET] = ABORTED_COMMAND;
-		/* READ ERROR */
-		buffer[SPC_ASC_KEY_OFFSET] = 0x11;
-		/* FAILED RETRANSMISSION REQUEST */
-		buffer[SPC_ASCQ_KEY_OFFSET] = 0x13;
-		break;
-	case TCM_WRITE_PROTECTED:
-		/* CURRENT ERROR */
-		buffer[0] = 0x70;
-		buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
-		/* DATA PROTECT */
-		buffer[SPC_SENSE_KEY_OFFSET] = DATA_PROTECT;
-		/* WRITE PROTECTED */
-		buffer[SPC_ASC_KEY_OFFSET] = 0x27;
-		break;
-	case TCM_ADDRESS_OUT_OF_RANGE:
-		/* CURRENT ERROR */
-		buffer[0] = 0x70;
-		buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
-		/* ILLEGAL REQUEST */
-		buffer[SPC_SENSE_KEY_OFFSET] = ILLEGAL_REQUEST;
-		/* LOGICAL BLOCK ADDRESS OUT OF RANGE */
-		buffer[SPC_ASC_KEY_OFFSET] = 0x21;
-		break;
-	case TCM_CHECK_CONDITION_UNIT_ATTENTION:
-		/* CURRENT ERROR */
-		buffer[0] = 0x70;
-		buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
-		/* UNIT ATTENTION */
-		buffer[SPC_SENSE_KEY_OFFSET] = UNIT_ATTENTION;
-		core_scsi3_ua_for_check_condition(cmd, &asc, &ascq);
-		buffer[SPC_ASC_KEY_OFFSET] = asc;
-		buffer[SPC_ASCQ_KEY_OFFSET] = ascq;
-		break;
-	case TCM_CHECK_CONDITION_NOT_READY:
-		/* CURRENT ERROR */
-		buffer[0] = 0x70;
-		buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
-		/* Not Ready */
-		buffer[SPC_SENSE_KEY_OFFSET] = NOT_READY;
-		buffer[SPC_ASC_KEY_OFFSET] = cmd->scsi_asc;
-		buffer[SPC_ASCQ_KEY_OFFSET] = cmd->scsi_ascq;
-		break;
-	case TCM_MISCOMPARE_VERIFY:
-		/* CURRENT ERROR */
-		buffer[0] = 0x70;
-		buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
-		buffer[SPC_SENSE_KEY_OFFSET] = MISCOMPARE;
-		/* MISCOMPARE DURING VERIFY OPERATION */
-		buffer[SPC_ASC_KEY_OFFSET] = 0x1d;
-		buffer[SPC_ASCQ_KEY_OFFSET] = 0x00;
-		break;
-	case TCM_LOGICAL_BLOCK_GUARD_CHECK_FAILED:
-		/* CURRENT ERROR */
-		buffer[0] = 0x70;
-		buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
-		/* ILLEGAL REQUEST */
-		buffer[SPC_SENSE_KEY_OFFSET] = ILLEGAL_REQUEST;
-		/* LOGICAL BLOCK GUARD CHECK FAILED */
-		buffer[SPC_ASC_KEY_OFFSET] = 0x10;
-		buffer[SPC_ASCQ_KEY_OFFSET] = 0x01;
-		transport_err_sector_info(buffer, cmd->bad_sector);
-		break;
-	case TCM_LOGICAL_BLOCK_APP_TAG_CHECK_FAILED:
-		/* CURRENT ERROR */
-		buffer[0] = 0x70;
-		buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
-		/* ILLEGAL REQUEST */
-		buffer[SPC_SENSE_KEY_OFFSET] = ILLEGAL_REQUEST;
-		/* LOGICAL BLOCK APPLICATION TAG CHECK FAILED */
-		buffer[SPC_ASC_KEY_OFFSET] = 0x10;
-		buffer[SPC_ASCQ_KEY_OFFSET] = 0x02;
-		transport_err_sector_info(buffer, cmd->bad_sector);
-		break;
-	case TCM_LOGICAL_BLOCK_REF_TAG_CHECK_FAILED:
-		/* CURRENT ERROR */
-		buffer[0] = 0x70;
-		buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
-		/* ILLEGAL REQUEST */
-		buffer[SPC_SENSE_KEY_OFFSET] = ILLEGAL_REQUEST;
-		/* LOGICAL BLOCK REFERENCE TAG CHECK FAILED */
-		buffer[SPC_ASC_KEY_OFFSET] = 0x10;
-		buffer[SPC_ASCQ_KEY_OFFSET] = 0x03;
-		transport_err_sector_info(buffer, cmd->bad_sector);
-		break;
-	case TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE:
-	default:
-		/* CURRENT ERROR */
-		buffer[0] = 0x70;
-		buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
-		/*
-		 * Returning ILLEGAL REQUEST would cause immediate IO errors on
-		 * Solaris initiators.  Returning NOT READY instead means the
-		 * operations will be retried a finite number of times and we
-		 * can survive intermittent errors.
-		 */
-		buffer[SPC_SENSE_KEY_OFFSET] = NOT_READY;
-		/* LOGICAL UNIT COMMUNICATION FAILURE */
-		buffer[SPC_ASC_KEY_OFFSET] = 0x08;
-		break;
+		translate_sense_reason(cmd, reason);
+		cmd->scsi_status = SAM_STAT_CHECK_CONDITION;
+		cmd->scsi_sense_length  = TRANSPORT_SENSE_BUFFER;
 	}
-	/*
-	 * This code uses linux/include/scsi/scsi.h SAM status codes!
-	 */
-	cmd->scsi_status = SAM_STAT_CHECK_CONDITION;
-	/*
-	 * Automatically padded, this value is encoded in the fabric's
-	 * data_length response PDU containing the SCSI defined sense data.
-	 */
-	cmd->scsi_sense_length  = TRANSPORT_SENSE_BUFFER;
 
-after_reason:
 	trace_target_cmd_complete(cmd);
 	return cmd->se_tfo->queue_status(cmd);
 }
-- 
1.8.4.3

[PATCH v3 3/5] scsi: Move sense handling routines to scsi_common

[Sagi Grimberg]

Sense data handling is also done in the target stack.
Hence, move sense handling routines to scsi_common so
the target will be able to use them as well.

Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
---
 drivers/scsi/scsi_common.c | 98 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/scsi_error.c  | 99 +---------------------------------------------
 include/scsi/scsi_common.h |  5 +++
 include/scsi/scsi_eh.h     |  7 +---
 4 files changed, 105 insertions(+), 104 deletions(-)

diff --git a/drivers/scsi/scsi_common.c b/drivers/scsi/scsi_common.c
index 2ff0922..41432c1 100644
--- a/drivers/scsi/scsi_common.c
+++ b/drivers/scsi/scsi_common.c
@@ -5,6 +5,7 @@
 #include <linux/bug.h>
 #include <linux/kernel.h>
 #include <linux/string.h>
+#include <asm/unaligned.h>
 #include <scsi/scsi_common.h>
 
 /* NB: These are exposed through /proc/scsi/scsi and form part of the ABI.
@@ -176,3 +177,100 @@ bool scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
 	return true;
 }
 EXPORT_SYMBOL(scsi_normalize_sense);
+
+/**
+ * scsi_sense_desc_find - search for a given descriptor type in	descriptor sense data format.
+ * @sense_buffer:	byte array of descriptor format sense data
+ * @sb_len:		number of valid bytes in sense_buffer
+ * @desc_type:		value of descriptor type to find
+ *			(e.g. 0 -> information)
+ *
+ * Notes:
+ *	only valid when sense data is in descriptor format
+ *
+ * Return value:
+ *	pointer to start of (first) descriptor if found else NULL
+ */
+const u8 * scsi_sense_desc_find(const u8 * sense_buffer, int sb_len,
+				int desc_type)
+{
+	int add_sen_len, add_len, desc_len, k;
+	const u8 * descp;
+
+	if ((sb_len < 8) || (0 == (add_sen_len = sense_buffer[7])))
+		return NULL;
+	if ((sense_buffer[0] < 0x72) || (sense_buffer[0] > 0x73))
+		return NULL;
+	add_sen_len = (add_sen_len < (sb_len - 8)) ?
+			add_sen_len : (sb_len - 8);
+	descp = &sense_buffer[8];
+	for (desc_len = 0, k = 0; k < add_sen_len; k += desc_len) {
+		descp += desc_len;
+		add_len = (k < (add_sen_len - 1)) ? descp[1]: -1;
+		desc_len = add_len + 2;
+		if (descp[0] == desc_type)
+			return descp;
+		if (add_len < 0) // short descriptor ??
+			break;
+	}
+	return NULL;
+}
+EXPORT_SYMBOL(scsi_sense_desc_find);
+
+/**
+ * scsi_build_sense_buffer - build sense data in a buffer
+ * @desc:	Sense format (non zero == descriptor format,
+ *              0 == fixed format)
+ * @buf:	Where to build sense data
+ * @key:	Sense key
+ * @asc:	Additional sense code
+ * @ascq:	Additional sense code qualifier
+ *
+ **/
+void scsi_build_sense_buffer(int desc, u8 *buf, u8 key, u8 asc, u8 ascq)
+{
+	if (desc) {
+		buf[0] = 0x72;	/* descriptor, current */
+		buf[1] = key;
+		buf[2] = asc;
+		buf[3] = ascq;
+		buf[7] = 0;
+	} else {
+		buf[0] = 0x70;	/* fixed, current */
+		buf[2] = key;
+		buf[7] = 0xa;
+		buf[12] = asc;
+		buf[13] = ascq;
+	}
+}
+EXPORT_SYMBOL(scsi_build_sense_buffer);
+
+/**
+ * scsi_set_sense_information - set the information field in a
+ *		formatted sense data buffer
+ * @buf:	Where to build sense data
+ * @info:	64-bit information value to be set
+ *
+ **/
+void scsi_set_sense_information(u8 *buf, u64 info)
+{
+	if ((buf[0] & 0x7f) == 0x72) {
+		u8 *ucp, len;
+
+		len = buf[7];
+		ucp = (char *)scsi_sense_desc_find(buf, len + 8, 0);
+		if (!ucp) {
+			buf[7] = len + 0xa;
+			ucp = buf + 8 + len;
+		}
+		ucp[0] = 0;
+		ucp[1] = 0xa;
+		ucp[2] = 0x80; /* Valid bit */
+		ucp[3] = 0;
+		put_unaligned_be64(info, &ucp[4]);
+	} else if ((buf[0] & 0x7f) == 0x70) {
+		buf[0] |= 0x80;
+		put_unaligned_be64(info, &buf[3]);
+	}
+}
+EXPORT_SYMBOL(scsi_set_sense_information);
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 106884a..6e6b2d2 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -26,7 +26,6 @@
 #include <linux/blkdev.h>
 #include <linux/delay.h>
 #include <linux/jiffies.h>
-#include <asm/unaligned.h>
 
 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>
@@ -34,6 +33,7 @@
 #include <scsi/scsi_device.h>
 #include <scsi/scsi_driver.h>
 #include <scsi/scsi_eh.h>
+#include <scsi/scsi_common.h>
 #include <scsi/scsi_transport.h>
 #include <scsi/scsi_host.h>
 #include <scsi/scsi_ioctl.h>
@@ -2408,45 +2408,6 @@ bool scsi_command_normalize_sense(const struct scsi_cmnd *cmd,
 EXPORT_SYMBOL(scsi_command_normalize_sense);
 
 /**
- * scsi_sense_desc_find - search for a given descriptor type in	descriptor sense data format.
- * @sense_buffer:	byte array of descriptor format sense data
- * @sb_len:		number of valid bytes in sense_buffer
- * @desc_type:		value of descriptor type to find
- *			(e.g. 0 -> information)
- *
- * Notes:
- *	only valid when sense data is in descriptor format
- *
- * Return value:
- *	pointer to start of (first) descriptor if found else NULL
- */
-const u8 * scsi_sense_desc_find(const u8 * sense_buffer, int sb_len,
-				int desc_type)
-{
-	int add_sen_len, add_len, desc_len, k;
-	const u8 * descp;
-
-	if ((sb_len < 8) || (0 == (add_sen_len = sense_buffer[7])))
-		return NULL;
-	if ((sense_buffer[0] < 0x72) || (sense_buffer[0] > 0x73))
-		return NULL;
-	add_sen_len = (add_sen_len < (sb_len - 8)) ?
-			add_sen_len : (sb_len - 8);
-	descp = &sense_buffer[8];
-	for (desc_len = 0, k = 0; k < add_sen_len; k += desc_len) {
-		descp += desc_len;
-		add_len = (k < (add_sen_len - 1)) ? descp[1]: -1;
-		desc_len = add_len + 2;
-		if (descp[0] == desc_type)
-			return descp;
-		if (add_len < 0) // short descriptor ??
-			break;
-	}
-	return NULL;
-}
-EXPORT_SYMBOL(scsi_sense_desc_find);
-
-/**
  * scsi_get_sense_info_fld - get information field from sense data (either fixed or descriptor format)
  * @sense_buffer:	byte array of sense data
  * @sb_len:		number of valid bytes in sense_buffer
@@ -2495,61 +2456,3 @@ int scsi_get_sense_info_fld(const u8 * sense_buffer, int sb_len,
 	}
 }
 EXPORT_SYMBOL(scsi_get_sense_info_fld);
-
-/**
- * scsi_build_sense_buffer - build sense data in a buffer
- * @desc:	Sense format (non zero == descriptor format,
- * 		0 == fixed format)
- * @buf:	Where to build sense data
- * @key:	Sense key
- * @asc:	Additional sense code
- * @ascq:	Additional sense code qualifier
- *
- **/
-void scsi_build_sense_buffer(int desc, u8 *buf, u8 key, u8 asc, u8 ascq)
-{
-	if (desc) {
-		buf[0] = 0x72;	/* descriptor, current */
-		buf[1] = key;
-		buf[2] = asc;
-		buf[3] = ascq;
-		buf[7] = 0;
-	} else {
-		buf[0] = 0x70;	/* fixed, current */
-		buf[2] = key;
-		buf[7] = 0xa;
-		buf[12] = asc;
-		buf[13] = ascq;
-	}
-}
-EXPORT_SYMBOL(scsi_build_sense_buffer);
-
-/**
- * scsi_set_sense_information - set the information field in a
- *		formatted sense data buffer
- * @buf:	Where to build sense data
- * @info:	64-bit information value to be set
- *
- **/
-void scsi_set_sense_information(u8 *buf, u64 info)
-{
-	if ((buf[0] & 0x7f) == 0x72) {
-		u8 *ucp, len;
-
-		len = buf[7];
-		ucp = (char *)scsi_sense_desc_find(buf, len + 8, 0);
-		if (!ucp) {
-			buf[7] = len + 0xa;
-			ucp = buf + 8 + len;
-		}
-		ucp[0] = 0;
-		ucp[1] = 0xa;
-		ucp[2] = 0x80; /* Valid bit */
-		ucp[3] = 0;
-		put_unaligned_be64(info, &ucp[4]);
-	} else if ((buf[0] & 0x7f) == 0x70) {
-		buf[0] |= 0x80;
-		put_unaligned_be64(info, &buf[3]);
-	}
-}
-EXPORT_SYMBOL(scsi_set_sense_information);
diff --git a/include/scsi/scsi_common.h b/include/scsi/scsi_common.h
index 676b03b..156d673 100644
--- a/include/scsi/scsi_common.h
+++ b/include/scsi/scsi_common.h
@@ -61,4 +61,9 @@ static inline bool scsi_sense_valid(const struct scsi_sense_hdr *sshdr)
 extern bool scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
 				 struct scsi_sense_hdr *sshdr);
 
+extern void scsi_build_sense_buffer(int desc, u8 *buf, u8 key, u8 asc, u8 ascq);
+extern void scsi_set_sense_information(u8 *buf, u64 info);
+extern const u8 * scsi_sense_desc_find(const u8 * sense_buffer, int sb_len,
+				       int desc_type);
+
 #endif /* _SCSI_COMMON_H_ */
diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
index 4942710..dbb8c64 100644
--- a/include/scsi/scsi_eh.h
+++ b/include/scsi/scsi_eh.h
@@ -4,6 +4,7 @@
 #include <linux/scatterlist.h>
 
 #include <scsi/scsi_cmnd.h>
+#include <scsi/scsi_common.h>
 struct scsi_device;
 struct Scsi_Host;
 
@@ -21,15 +22,9 @@ static inline bool scsi_sense_is_deferred(const struct scsi_sense_hdr *sshdr)
 	return ((sshdr->response_code >= 0x70) && (sshdr->response_code & 1));
 }
 
-extern const u8 * scsi_sense_desc_find(const u8 * sense_buffer, int sb_len,
-				       int desc_type);
-
 extern int scsi_get_sense_info_fld(const u8 * sense_buffer, int sb_len,
 				   u64 * info_out);
 
-extern void scsi_build_sense_buffer(int desc, u8 *buf, u8 key, u8 asc, u8 ascq);
-extern void scsi_set_sense_information(u8 *buf, u64 info);
-
 extern int scsi_ioctl_reset(struct scsi_device *, int __user *);
 
 struct scsi_eh_save {
-- 
1.8.4.3

[PATCH v3 4/5] target: Use scsi helpers to build the sense data correctly

[Sagi Grimberg]

Instead of open coding the sense buffer construction, use
scsi scsi_build_sense_buffer() and scsi_set_sense_information()
helpers which moved to scsi_common.

Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
---
 drivers/target/target_core_spc.c       | 31 +++++--------------------------
 drivers/target/target_core_transport.c | 20 +++-----------------
 include/target/target_core_base.h      |  1 +
 3 files changed, 9 insertions(+), 43 deletions(-)

diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index b074443..c43dcbf 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -1157,32 +1157,11 @@ static sense_reason_t spc_emulate_request_sense(struct se_cmd *cmd)
 	if (!rbuf)
 		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 
-	if (!core_scsi3_ua_clear_for_request_sense(cmd, &ua_asc, &ua_ascq)) {
-		/*
-		 * CURRENT ERROR, UNIT ATTENTION
-		 */
-		buf[0] = 0x70;
-		buf[SPC_SENSE_KEY_OFFSET] = UNIT_ATTENTION;
-
-		/*
-		 * The Additional Sense Code (ASC) from the UNIT ATTENTION
-		 */
-		buf[SPC_ASC_KEY_OFFSET] = ua_asc;
-		buf[SPC_ASCQ_KEY_OFFSET] = ua_ascq;
-		buf[7] = 0x0A;
-	} else {
-		/*
-		 * CURRENT ERROR, NO SENSE
-		 */
-		buf[0] = 0x70;
-		buf[SPC_SENSE_KEY_OFFSET] = NO_SENSE;
-
-		/*
-		 * NO ADDITIONAL SENSE INFORMATION
-		 */
-		buf[SPC_ASC_KEY_OFFSET] = 0x00;
-		buf[7] = 0x0A;
-	}
+	if (!core_scsi3_ua_clear_for_request_sense(cmd, &ua_asc, &ua_ascq))
+		scsi_build_sense_buffer(0, buf, UNIT_ATTENTION,
+					ua_asc, ua_ascq);
+	else
+		scsi_build_sense_buffer(0, buf, NO_SENSE, 0x0, 0x0);
 
 	memcpy(rbuf, buf, min_t(u32, sizeof(buf), cmd->data_length));
 	transport_kunmap_data_sg(cmd);
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index fb134d9..0181f8b 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2620,19 +2620,6 @@ bool transport_wait_for_tasks(struct se_cmd *cmd)
 }
 EXPORT_SYMBOL(transport_wait_for_tasks);
 
-static
-void transport_err_sector_info(unsigned char *buffer, sector_t bad_sector)
-{
-	/* Place failed LBA in sense data information descriptor 0. */
-	buffer[SPC_ADD_SENSE_LEN_OFFSET] = 0xc;
-	buffer[SPC_DESC_TYPE_OFFSET] = 0; /* Information */
-	buffer[SPC_ADDITIONAL_DESC_LEN_OFFSET] = 0xa;
-	buffer[SPC_VALIDITY_OFFSET] = 0x80;
-
-	/* Descriptor Information: failing sector */
-	put_unaligned_be64(bad_sector, &buffer[12]);
-}
-
 struct sense_info {
 	u8 key;
 	u8 asc;
@@ -2759,7 +2746,6 @@ static void translate_sense_reason(struct se_cmd *cmd, sense_reason_t reason)
 		si = &sense_info_table[(__force int)
 				       TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE];
 
-	buffer[SPC_SENSE_KEY_OFFSET] = si->key;
 	if (r == (__force int)TCM_CHECK_CONDITION_UNIT_ATTENTION) {
 		core_scsi3_ua_for_check_condition(cmd, &asc, &ascq);
 		WARN_ON_ONCE(asc == 0);
@@ -2771,10 +2757,10 @@ static void translate_sense_reason(struct se_cmd *cmd, sense_reason_t reason)
 		asc = si->asc;
 		ascq = si->ascq;
 	}
-	buffer[SPC_ASC_KEY_OFFSET] = asc;
-	buffer[SPC_ASCQ_KEY_OFFSET] = ascq;
+
+	scsi_build_sense_buffer(0, buffer, si->key, asc, ascq);
 	if (si->add_sector_info)
-		transport_err_sector_info(cmd->sense_buffer, cmd->bad_sector);
+		scsi_set_sense_information(buffer, cmd->bad_sector);
 }
 
 int
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index a681644..47dea1b 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -7,6 +7,7 @@
 #include <linux/blkdev.h>
 #include <linux/percpu_ida.h>
 #include <linux/t10-pi.h>
+#include <scsi/scsi_common.h>
 #include <net/sock.h>
 #include <net/tcp.h>
 
-- 
1.8.4.3

[PATCH v3 5/5] target: Fix wrong setting of sense format for PI errors

[Sagi Grimberg]

PI errors should be reported in a descriptor format sense data.
Fix that by adding a desc_format flag to struct sense_info and pass
it to scsi_build_sense_buffer() to do the right thing.

Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
---
 drivers/target/target_core_transport.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 0181f8b..79bb8d1 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2625,6 +2625,7 @@ struct sense_info {
 	u8 asc;
 	u8 ascq;
 	bool add_sector_info;
+	int desc_format;
 };
 
 static const struct sense_info sense_info_table[] = {
@@ -2708,18 +2709,21 @@ static const struct sense_info sense_info_table[] = {
 		.asc = 0x10,
 		.ascq = 0x01, /* LOGICAL BLOCK GUARD CHECK FAILED */
 		.add_sector_info = true,
+		.desc_format = 1,
 	},
 	[TCM_LOGICAL_BLOCK_APP_TAG_CHECK_FAILED] = {
 		.key = ILLEGAL_REQUEST,
 		.asc = 0x10,
 		.ascq = 0x02, /* LOGICAL BLOCK APPLICATION TAG CHECK FAILED */
 		.add_sector_info = true,
+		.desc_format = 1,
 	},
 	[TCM_LOGICAL_BLOCK_REF_TAG_CHECK_FAILED] = {
 		.key = ILLEGAL_REQUEST,
 		.asc = 0x10,
 		.ascq = 0x03, /* LOGICAL BLOCK REFERENCE TAG CHECK FAILED */
 		.add_sector_info = true,
+		.desc_format = 1,
 	},
 	[TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE] = {
 		/*
@@ -2758,7 +2762,7 @@ static void translate_sense_reason(struct se_cmd *cmd, sense_reason_t reason)
 		ascq = si->ascq;
 	}
 
-	scsi_build_sense_buffer(0, buffer, si->key, asc, ascq);
+	scsi_build_sense_buffer(si->desc_format, buffer, si->key, asc, ascq);
 	if (si->add_sector_info)
 		scsi_set_sense_information(buffer, cmd->bad_sector);
 }
-- 
1.8.4.3

Re: [PATCH v3 3/5] scsi: Move sense handling routines to scsi_common

[Bart Van Assche]

On 07/06/2015 06:15 AM, Sagi Grimberg wrote:
> Sense data handling is also done in the target stack.
> Hence, move sense handling routines to scsi_common so
> the target will be able to use them as well.
>
> Signed-off-by: Sagi Grimberg <sagig@mellanox.com>

Reviewed-by: Bart Van Assche <bart.vanassche@sandisk.com>

Re: [PATCH v3 5/5] target: Fix wrong setting of sense format for PI errors

[Bart Van Assche]

On 07/06/2015 06:15 AM, Sagi Grimberg wrote:
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 0181f8b..79bb8d1 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -2625,6 +2625,7 @@ struct sense_info {
>   	u8 asc;
>   	u8 ascq;
>   	bool add_sector_info;
> +	int desc_format;
>   };

Something minor: has it been considered to use the data type "bool" 
instead of "int" for desc_format ?

Bart.

Re: [PATCH v3 5/5] target: Fix wrong setting of sense format for PI errors

[Sagi Grimberg]

On 7/6/2015 6:28 PM, Bart Van Assche wrote:
> On 07/06/2015 06:15 AM, Sagi Grimberg wrote:
>> diff --git a/drivers/target/target_core_transport.c
>> b/drivers/target/target_core_transport.c
>> index 0181f8b..79bb8d1 100644
>> --- a/drivers/target/target_core_transport.c
>> +++ b/drivers/target/target_core_transport.c
>> @@ -2625,6 +2625,7 @@ struct sense_info {
>>       u8 asc;
>>       u8 ascq;
>>       bool add_sector_info;
>> +    int desc_format;
>>   };
>
> Something minor: has it been considered to use the data type "bool"
> instead of "int" for desc_format ?

I've considered that, but since scsi_build_sense_buffer() desc argument
is an int, I figured it would be better than passing desc_format ? 1 : 0

But I can change it if you prefer.

Re: [PATCH v3 5/5] target: Fix wrong setting of sense format for PI errors

[Bart Van Assche]

On 07/06/2015 09:14 AM, Sagi Grimberg wrote:
> On 7/6/2015 6:28 PM, Bart Van Assche wrote:
>> On 07/06/2015 06:15 AM, Sagi Grimberg wrote:
>>> diff --git a/drivers/target/target_core_transport.c
>>> b/drivers/target/target_core_transport.c
>>> index 0181f8b..79bb8d1 100644
>>> --- a/drivers/target/target_core_transport.c
>>> +++ b/drivers/target/target_core_transport.c
>>> @@ -2625,6 +2625,7 @@ struct sense_info {
>>>        u8 asc;
>>>        u8 ascq;
>>>        bool add_sector_info;
>>> +    int desc_format;
>>>    };
>>
>> Something minor: has it been considered to use the data type "bool"
>> instead of "int" for desc_format ?
>
> I've considered that, but since scsi_build_sense_buffer() desc argument
> is an int, I figured it would be better than passing desc_format ? 1 : 0
>
> But I can change it if you prefer.

Hello Sagi,

The C language supports implicit conversion from bool to int so I think 
"? 1 : 0" is not necessary to convert a bool into an int.

Bart.

Re: [PATCH v3 0/5] Target sense data handling modifications

[Nicholas A. Bellinger]

On Mon, 2015-07-06 at 16:15 +0300, Sagi Grimberg wrote:
> This patch set modifies the target sense data handling.
> First, cleanup transport_send_check_condition_and_sense()
> by splitting the sense translation to a separate function.
> Second, convert sense reason the switch statement to a table
> driven code. Third, Use scsi common helpers to correctly set
> the sense buffer. Last, Fix wrong setting of t10-pi errors
> (non descriptor format).
> 
> Changes from v2:
> - Pass sense_reason_t to scsi_translate_sense()
> - Split patch 3:
>   1) move the helpers to scsi_common.c
>   2) use helpers in the target code
>   3) always use descriptor-type sense data for PI errors
> 
> Changes from v1:
> - Added Reviewed-by tags for patches 1,2
> - Fixed compilation error after testing patch #3 on scsi/for-next
>   branch. Moved scsi_sense_desc_find() to scsi_common as well (dependency)
>   and also moved <asm/unaligned.h> include to scsi_common.h
> 
> Changes from v0:
> - Added Bart's patches and converted my patch to apply over his
> - Moved scsi sense helpers to scsi_common
> 
> Bart Van Assche (2):
>   target: Inline transport_get_sense_codes()
>   target: Split transport_send_check_condition_and_sense()
> 
> Sagi Grimberg (3):
>   scsi: Move sense handling routines to scsi_common
>   target: Use scsi helpers to build the sense data correctly
>   target: Fix wrong setting of sense format for PI errors
> 
>  drivers/scsi/scsi_common.c             |  98 ++++++++
>  drivers/scsi/scsi_error.c              |  99 +-------
>  drivers/target/target_core_spc.c       |  31 +--
>  drivers/target/target_core_transport.c | 399 ++++++++++++---------------------
>  include/scsi/scsi_common.h             |   5 +
>  include/scsi/scsi_eh.h                 |   7 +-
>  include/target/target_core_base.h      |   1 +
>  7 files changed, 256 insertions(+), 384 deletions(-)
> 

Nice series to make scsi-core and target-core use common code.  ;)

Applied to target-pending/for-next.

hch + jejb, please let me know if you have any objections to taking this
through target-pending.

Re: [PATCH v3 0/5] Target sense data handling modifications

[Sagi Grimberg]

On 7/7/2015 1:51 AM, Nicholas A. Bellinger wrote:
> On Mon, 2015-07-06 at 16:15 +0300, Sagi Grimberg wrote:
>> This patch set modifies the target sense data handling.
>> First, cleanup transport_send_check_condition_and_sense()
>> by splitting the sense translation to a separate function.
>> Second, convert sense reason the switch statement to a table
>> driven code. Third, Use scsi common helpers to correctly set
>> the sense buffer. Last, Fix wrong setting of t10-pi errors
>> (non descriptor format).
>>
>> Changes from v2:
>> - Pass sense_reason_t to scsi_translate_sense()
>> - Split patch 3:
>>    1) move the helpers to scsi_common.c
>>    2) use helpers in the target code
>>    3) always use descriptor-type sense data for PI errors
>>
>> Changes from v1:
>> - Added Reviewed-by tags for patches 1,2
>> - Fixed compilation error after testing patch #3 on scsi/for-next
>>    branch. Moved scsi_sense_desc_find() to scsi_common as well (dependency)
>>    and also moved <asm/unaligned.h> include to scsi_common.h
>>
>> Changes from v0:
>> - Added Bart's patches and converted my patch to apply over his
>> - Moved scsi sense helpers to scsi_common
>>
>> Bart Van Assche (2):
>>    target: Inline transport_get_sense_codes()
>>    target: Split transport_send_check_condition_and_sense()
>>
>> Sagi Grimberg (3):
>>    scsi: Move sense handling routines to scsi_common
>>    target: Use scsi helpers to build the sense data correctly
>>    target: Fix wrong setting of sense format for PI errors
>>
>>   drivers/scsi/scsi_common.c             |  98 ++++++++
>>   drivers/scsi/scsi_error.c              |  99 +-------
>>   drivers/target/target_core_spc.c       |  31 +--
>>   drivers/target/target_core_transport.c | 399 ++++++++++++---------------------
>>   include/scsi/scsi_common.h             |   5 +
>>   include/scsi/scsi_eh.h                 |   7 +-
>>   include/target/target_core_base.h      |   1 +
>>   7 files changed, 256 insertions(+), 384 deletions(-)
>>
>
> Nice series to make scsi-core and target-core use common code.  ;)
>
> Applied to target-pending/for-next.
>
> hch + jejb, please let me know if you have any objections to taking this
> through target-pending.

Can you please fold into patch #5 this change suggested by Bart:

diff --git a/drivers/target/target_core_transport.c 
b/drivers/target/target_core_transport.c
index 79bb8d1..9c34937 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2625,7 +2625,7 @@ struct sense_info {
         u8 asc;
         u8 ascq;
         bool add_sector_info;
-   int desc_format;
+ bool desc_format;
  };

  static const struct sense_info sense_info_table[] = {
@@ -2709,21 +2709,21 @@ static const struct sense_info 
sense_info_table[] = {
                 .asc = 0x10,
                 .ascq = 0x01, /* LOGICAL BLOCK GUARD CHECK FAILED */
                 .add_sector_info = true,
-           .desc_format = 1,
+         .desc_format = true,
         },
         [TCM_LOGICAL_BLOCK_APP_TAG_CHECK_FAILED] = {
                 .key = ILLEGAL_REQUEST,
                 .asc = 0x10,
                 .ascq = 0x02, /* LOGICAL BLOCK APPLICATION TAG CHECK 
FAILED */
                 .add_sector_info = true,
-           .desc_format = 1,
+         .desc_format = true,
         },
         [TCM_LOGICAL_BLOCK_REF_TAG_CHECK_FAILED] = {
                 .key = ILLEGAL_REQUEST,
                 .asc = 0x10,
                 .ascq = 0x03, /* LOGICAL BLOCK REFERENCE TAG CHECK 
FAILED */
                 .add_sector_info = true,
-           .desc_format = 1,
+         .desc_format = true,
         },
         [TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE] = {
                 /*
--

Thanks!

Re: [PATCH v3 0/5] Target sense data handling modifications

[Nicholas A. Bellinger]

On Tue, 2015-07-07 at 09:18 +0300, Sagi Grimberg wrote:
> On 7/7/2015 1:51 AM, Nicholas A. Bellinger wrote:
> > On Mon, 2015-07-06 at 16:15 +0300, Sagi Grimberg wrote:
> >> This patch set modifies the target sense data handling.
> >> First, cleanup transport_send_check_condition_and_sense()
> >> by splitting the sense translation to a separate function.
> >> Second, convert sense reason the switch statement to a table
> >> driven code. Third, Use scsi common helpers to correctly set
> >> the sense buffer. Last, Fix wrong setting of t10-pi errors
> >> (non descriptor format).
> >>
> >> Changes from v2:
> >> - Pass sense_reason_t to scsi_translate_sense()
> >> - Split patch 3:
> >>    1) move the helpers to scsi_common.c
> >>    2) use helpers in the target code
> >>    3) always use descriptor-type sense data for PI errors
> >>
> >> Changes from v1:
> >> - Added Reviewed-by tags for patches 1,2
> >> - Fixed compilation error after testing patch #3 on scsi/for-next
> >>    branch. Moved scsi_sense_desc_find() to scsi_common as well (dependency)
> >>    and also moved <asm/unaligned.h> include to scsi_common.h
> >>
> >> Changes from v0:
> >> - Added Bart's patches and converted my patch to apply over his
> >> - Moved scsi sense helpers to scsi_common
> >>
> >> Bart Van Assche (2):
> >>    target: Inline transport_get_sense_codes()
> >>    target: Split transport_send_check_condition_and_sense()
> >>
> >> Sagi Grimberg (3):
> >>    scsi: Move sense handling routines to scsi_common
> >>    target: Use scsi helpers to build the sense data correctly
> >>    target: Fix wrong setting of sense format for PI errors
> >>
> >>   drivers/scsi/scsi_common.c             |  98 ++++++++
> >>   drivers/scsi/scsi_error.c              |  99 +-------
> >>   drivers/target/target_core_spc.c       |  31 +--
> >>   drivers/target/target_core_transport.c | 399 ++++++++++++---------------------
> >>   include/scsi/scsi_common.h             |   5 +
> >>   include/scsi/scsi_eh.h                 |   7 +-
> >>   include/target/target_core_base.h      |   1 +
> >>   7 files changed, 256 insertions(+), 384 deletions(-)
> >>
> >
> > Nice series to make scsi-core and target-core use common code.  ;)
> >
> > Applied to target-pending/for-next.
> >
> > hch + jejb, please let me know if you have any objections to taking this
> > through target-pending.
> 
> Can you please fold into patch #5 this change suggested by Bart:
> 
> diff --git a/drivers/target/target_core_transport.c 
> b/drivers/target/target_core_transport.c
> index 79bb8d1..9c34937 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -2625,7 +2625,7 @@ struct sense_info {
>          u8 asc;
>          u8 ascq;
>          bool add_sector_info;
> -   int desc_format;
> + bool desc_format;
>   };
> 
>   static const struct sense_info sense_info_table[] = {
> @@ -2709,21 +2709,21 @@ static const struct sense_info 
> sense_info_table[] = {
>                  .asc = 0x10,
>                  .ascq = 0x01, /* LOGICAL BLOCK GUARD CHECK FAILED */
>                  .add_sector_info = true,
> -           .desc_format = 1,
> +         .desc_format = true,
>          },
>          [TCM_LOGICAL_BLOCK_APP_TAG_CHECK_FAILED] = {
>                  .key = ILLEGAL_REQUEST,
>                  .asc = 0x10,
>                  .ascq = 0x02, /* LOGICAL BLOCK APPLICATION TAG CHECK 
> FAILED */
>                  .add_sector_info = true,
> -           .desc_format = 1,
> +         .desc_format = true,
>          },
>          [TCM_LOGICAL_BLOCK_REF_TAG_CHECK_FAILED] = {
>                  .key = ILLEGAL_REQUEST,
>                  .asc = 0x10,
>                  .ascq = 0x03, /* LOGICAL BLOCK REFERENCE TAG CHECK 
> FAILED */
>                  .add_sector_info = true,
> -           .desc_format = 1,
> +         .desc_format = true,
>          },
>          [TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE] = {
>                  /*
> --
> 

Folded into the original patch.

Re: [PATCH v3 2/5] target: Split transport_send_check_condition_and_sense()

[Christoph Hellwig]

> +	if (r == (__force int)TCM_CHECK_CONDITION_UNIT_ATTENTION) {

You probably want to compare reason here to avoid the cast.

Otherwise looks good,

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

Re: [PATCH v3 3/5] scsi: Move sense handling routines to scsi_common

[Christoph Hellwig]

Looks good,

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

Re: [PATCH v3 4/5] target: Use scsi helpers to build the sense data correctly

[Christoph Hellwig]

> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -7,6 +7,7 @@
>  #include <linux/blkdev.h>
>  #include <linux/percpu_ida.h>
>  #include <linux/t10-pi.h>
> +#include <scsi/scsi_common.h>
>  #include <net/sock.h>
>  #include <net/tcp.h>

Please only add the include in the files that need it.  (And many of the
existing includes should be fixed up the same way, but that's a
different story).

Otherwise looks good:

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

Re: [PATCH v3 5/5] target: Fix wrong setting of sense format for PI errors

[Christoph Hellwig]

On Mon, Jul 06, 2015 at 04:15:08PM +0300, Sagi Grimberg wrote:
> PI errors should be reported in a descriptor format sense data.
> Fix that by adding a desc_format flag to struct sense_info and pass
> it to scsi_build_sense_buffer() to do the right thing.

Do we really need the additional flag?  We only need the descriptor
sense format because we add the sector information.  So just checking
for that should be enough, especially when paired with a comment
explaining that logic in the source file.

Re: [PATCH v3 4/5] target: Use scsi helpers to build the sense data correctly

[Sagi Grimberg]

On 7/8/2015 1:17 PM, Christoph Hellwig wrote:
>> --- a/include/target/target_core_base.h
>> +++ b/include/target/target_core_base.h
>> @@ -7,6 +7,7 @@
>>   #include <linux/blkdev.h>
>>   #include <linux/percpu_ida.h>
>>   #include <linux/t10-pi.h>
>> +#include <scsi/scsi_common.h>
>>   #include <net/sock.h>
>>   #include <net/tcp.h>
>
> Please only add the include in the files that need it.  (And many of the
> existing includes should be fixed up the same way, but that's a
> different story).
>

OK, I'll move it.

Re: [PATCH v3 2/5] target: Split transport_send_check_condition_and_sense()

[Sagi Grimberg]

On 7/8/2015 1:15 PM, Christoph Hellwig wrote:
>> +	if (r == (__force int)TCM_CHECK_CONDITION_UNIT_ATTENTION) {
>
> You probably want to compare reason here to avoid the cast.
>

I do want it...

Re: [PATCH v3 5/5] target: Fix wrong setting of sense format for PI errors

[Sagi Grimberg]

On 7/8/2015 1:19 PM, Christoph Hellwig wrote:
> On Mon, Jul 06, 2015 at 04:15:08PM +0300, Sagi Grimberg wrote:
>> PI errors should be reported in a descriptor format sense data.
>> Fix that by adding a desc_format flag to struct sense_info and pass
>> it to scsi_build_sense_buffer() to do the right thing.
>
> Do we really need the additional flag?  We only need the descriptor
> sense format because we add the sector information.  So just checking
> for that should be enough, especially when paired with a comment
> explaining that logic in the source file.

We don't have any other information today, but sector is not the only
information that is requires a descriptor format, so maybe it will be a
bit awkward to condition the descriptor format on the sector info?

Re: [PATCH v3 5/5] target: Fix wrong setting of sense format for PI errors

[Christoph Hellwig]

On Wed, Jul 08, 2015 at 01:36:04PM +0300, Sagi Grimberg wrote:
> We don't have any other information today, but sector is not the only
> information that is requires a descriptor format, so maybe it will be a
> bit awkward to condition the descriptor format on the sector info?

The only reason why you'd want to support descriptor type sense data is
because you need to add a second descriptor.  If we have another case
that needs descriptor sense data it'll also need to add that additional
descriptor.  So we'll need a conditional for it in the sense data
generation anyway.

Re: [PATCH v3 5/5] target: Fix wrong setting of sense format for PI errors

[Hannes Reinecke]

On 07/08/2015 12:49 PM, Christoph Hellwig wrote:
> On Wed, Jul 08, 2015 at 01:36:04PM +0300, Sagi Grimberg wrote:
>> We don't have any other information today, but sector is not the only
>> information that is requires a descriptor format, so maybe it will be a
>> bit awkward to condition the descriptor format on the sector info?
> 
> The only reason why you'd want to support descriptor type sense data is
> because you need to add a second descriptor.  If we have another case
> that needs descriptor sense data it'll also need to add that additional
> descriptor.  So we'll need a conditional for it in the sense data
> generation anyway.
> 
Actually it's controlled by the D_SENSE bit in the Control mode page
(that's bit[2] of byte 2 in the control mode page).
Which is currently set to '0', ie we will be returning fixed sense
information.
_If_ we were to report descriptor sense we will need to change that,
too.

And it's actually not true that you'd need descriptor sense to
encode the sector information; it'll be stored in the 'information'
section (byte 3-6) for fixed format sense.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
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 v3 5/5] target: Fix wrong setting of sense format for PI errors

[Sagi Grimberg]

On 7/8/2015 1:59 PM, Hannes Reinecke wrote:
> On 07/08/2015 12:49 PM, Christoph Hellwig wrote:
>> On Wed, Jul 08, 2015 at 01:36:04PM +0300, Sagi Grimberg wrote:
>>> We don't have any other information today, but sector is not the only
>>> information that is requires a descriptor format, so maybe it will be a
>>> bit awkward to condition the descriptor format on the sector info?
>>
>> The only reason why you'd want to support descriptor type sense data is
>> because you need to add a second descriptor.  If we have another case
>> that needs descriptor sense data it'll also need to add that additional
>> descriptor.  So we'll need a conditional for it in the sense data
>> generation anyway.
>>
> Actually it's controlled by the D_SENSE bit in the Control mode page
> (that's bit[2] of byte 2 in the control mode page).
> Which is currently set to '0', ie we will be returning fixed sense
> information.
> _If_ we were to report descriptor sense we will need to change that,
> too.

I missed that bit.

>
> And it's actually not true that you'd need descriptor sense to
> encode the sector information; it'll be stored in the 'information'
> section (byte 3-6) for fixed format sense.

But when I return the sector info in a fixed size format, the initiator
is not able to decode the faulty sector:

kernel: DIFv1 Type 1 reference failed on sector: 15 tag: 0xfffffff0 
sector MSB: 0x0000000f
kernel: sd 10:0:1:0: [sdc] tag#0 FAILED Result: hostbyte=DID_OK 
driverbyte=DRIVER_SENSE
kernel: sd 10:0:1:0: [sdc] tag#0 Sense Key : Aborted Command [current]
kernel: sd 10:0:1:0: [sdc] tag#0 Add. Sense: No additional sense information
kernel: sd 10:0:1:0: [sdc] tag#0 CDB: Read(10) 28 20 00 00 00 00 00 00 10 00
kernel: blk_update_request: I/O error, dev sdc, sector 0

Is that a bug?

Re: [PATCH v3 5/5] target: Fix wrong setting of sense format for PI errors

[Christoph Hellwig]

On Wed, Jul 08, 2015 at 12:59:18PM +0200, Hannes Reinecke wrote:
> Actually it's controlled by the D_SENSE bit in the Control mode page
> (that's bit[2] of byte 2 in the control mode page).
> Which is currently set to '0', ie we will be returning fixed sense
> information.
> _If_ we were to report descriptor sense we will need to change that,
> too.

Just looked over SPC, and indeed D_SENSE is a strict either fixed
or descriptor, not a may return descriptor data.

So this patch actually is wrong as we never must return different sense
data types based on the sense code.

Re: [PATCH v3 5/5] target: Fix wrong setting of sense format for PI errors

[Sagi Grimberg]

On 7/8/2015 2:44 PM, Christoph Hellwig wrote:
> On Wed, Jul 08, 2015 at 12:59:18PM +0200, Hannes Reinecke wrote:
>> Actually it's controlled by the D_SENSE bit in the Control mode page
>> (that's bit[2] of byte 2 in the control mode page).
>> Which is currently set to '0', ie we will be returning fixed sense
>> information.
>> _If_ we were to report descriptor sense we will need to change that,
>> too.
>
> Just looked over SPC, and indeed D_SENSE is a strict either fixed
> or descriptor, not a may return descriptor data.
>
> So this patch actually is wrong as we never must return different sense
> data types based on the sense code.
>

I'll send out v4 without this patch altogether.

Re: [PATCH v3 5/5] target: Fix wrong setting of sense format for PI errors

[Sagi Grimberg]

On 7/8/2015 2:14 PM, Sagi Grimberg wrote:
>>
>> And it's actually not true that you'd need descriptor sense to
>> encode the sector information; it'll be stored in the 'information'
>> section (byte 3-6) for fixed format sense.
>
> But when I return the sector info in a fixed size format, the initiator
> is not able to decode the faulty sector:
>
> kernel: DIFv1 Type 1 reference failed on sector: 15 tag: 0xfffffff0
> sector MSB: 0x0000000f
> kernel: sd 10:0:1:0: [sdc] tag#0 FAILED Result: hostbyte=DID_OK
> driverbyte=DRIVER_SENSE
> kernel: sd 10:0:1:0: [sdc] tag#0 Sense Key : Aborted Command [current]
> kernel: sd 10:0:1:0: [sdc] tag#0 Add. Sense: No additional sense
> information
> kernel: sd 10:0:1:0: [sdc] tag#0 CDB: Read(10) 28 20 00 00 00 00 00 00
> 10 00
> kernel: blk_update_request: I/O error, dev sdc, sector 0
>
> Is that a bug?

Bleh, found the bug... It was in scsi_set_sense_information()

For Fixed sized sense the information field is 4 bytes so this fixes it:
diff --git a/drivers/scsi/scsi_common.c b/drivers/scsi/scsi_common.c
index 41432c1..8cfb7ee 100644
--- a/drivers/scsi/scsi_common.c
+++ b/drivers/scsi/scsi_common.c
@@ -270,7 +270,7 @@ void scsi_set_sense_information(u8 *buf, u64 info)
                 put_unaligned_be64(info, &ucp[4]);
         } else if ((buf[0] & 0x7f) == 0x70) {
                 buf[0] |= 0x80;
-           put_unaligned_be64(info, &buf[3]);
+         put_unaligned_be32(info, &buf[3]);
         }
  }
  EXPORT_SYMBOL(scsi_set_sense_information);

I'll send out a separate patch.

Thanks Hannes and Christoph for catching this.

Sagi.