[PATCH 0/2] target: Support MI_REPORT_TARGET_PGS extended header format

[PATCH 0/2] target: Support MI_REPORT_TARGET_PGS extended header format

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

Hi Hannes & Co,

This series adds support for MI_REPORT_TARGET_PGS extended header format
within ALUA target port group infrastructure, and adds 'implict_trans_secs'
tg_pt_gp attribute to set this value individually via configfs for each
/sys/kernel/config/target/core/$HBA/$DEV/alua/$ALUA_TG_PT_GP/ group.

This feature has been on my TODO list for a while, and now that Rob Evers
has posted the scsi_dh_alua.c changes to take advantage of this information
array side, it's now time to get this supported within target-core.

Note so far this has been tested with 'sg_raw a3 2a' on loopback LUNs, and
appears to be working as expected.  Please have a look and let me know if you
have any concerns.

Thanks!

Nicholas Bellinger (2):
  target: Fix MAINTENANCE_IN service action CDB checks to use lower 5
    bits
  target: Add MI_REPORT_TARGET_PGS ext. header + implict_trans_secs
    attribute

 drivers/target/target_core_alua.c      |   88 ++++++++++++++++++++++++++-----
 drivers/target/target_core_alua.h      |   10 ++++
 drivers/target/target_core_configfs.c  |   21 ++++++++
 drivers/target/target_core_transport.c |    3 +-
 include/target/target_core_base.h      |    1 +
 5 files changed, 107 insertions(+), 16 deletions(-)

-- 
1.7.2.5

[PATCH 1/2] target: Fix MAINTENANCE_IN service action CDB checks to use lower 5 bits

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch fixes the MAINTENANCE_IN service action type checks to only
look at the proper lower 5 bits of cdb byte 1.  This addresses the case
where MI_REPORT_TARGET_PGS w/ extended header using the upper three bits of
cdb byte 1 was not processed correctly in transport_generic_cmd_sequencer,
as well as the three cases for standby, unavailable, and transition ALUA
primary access state checks.

Also add MAINTENANCE_IN to the excluded list in transport_generic_prepare_cdb()
to prevent the PARAMETER DATA FORMAT bits from being cleared.

Cc: Hannes Reinecke <hare@suse.de>
Cc: Rob Evers <revers@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Roland Dreier <roland@purestorage.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_alua.c      |    6 +++---
 drivers/target/target_core_transport.c |    3 ++-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
index c39ae7c..be7251e 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -387,7 +387,7 @@ static inline int core_alua_state_standby(
 	case RECEIVE_DIAGNOSTIC:
 	case SEND_DIAGNOSTIC:
 	case MAINTENANCE_IN:
-		switch (cdb[1]) {
+		switch (cdb[1] & 0x1f) {
 		case MI_REPORT_TARGET_PGS:
 			return 0;
 		default:
@@ -429,7 +429,7 @@ static inline int core_alua_state_unavailable(
 	case INQUIRY:
 	case REPORT_LUNS:
 	case MAINTENANCE_IN:
-		switch (cdb[1]) {
+		switch (cdb[1] & 0x1f) {
 		case MI_REPORT_TARGET_PGS:
 			return 0;
 		default:
@@ -469,7 +469,7 @@ static inline int core_alua_state_transition(
 	case INQUIRY:
 	case REPORT_LUNS:
 	case MAINTENANCE_IN:
-		switch (cdb[1]) {
+		switch (cdb[1] & 0x1f) {
 		case MI_REPORT_TARGET_PGS:
 			return 0;
 		default:
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 2d75c29..0590dc6 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1329,6 +1329,7 @@ static inline void transport_generic_prepare_cdb(
 	case VERIFY_16: /* SBC - VRProtect */
 	case WRITE_VERIFY: /* SBC - VRProtect */
 	case WRITE_VERIFY_12: /* SBC - VRProtect */
+	case MAINTENANCE_IN: /* SPC - Parameter Data Format for SA RTPG */
 		break;
 	default:
 		cdb[1] &= 0x1f; /* clear logical unit number */
@@ -2597,7 +2598,7 @@ static int transport_generic_cmd_sequencer(
 			/*
 			 * Check for emulated MI_REPORT_TARGET_PGS.
 			 */
-			if (cdb[1] == MI_REPORT_TARGET_PGS &&
+			if ((cdb[1] & 0x1f) == MI_REPORT_TARGET_PGS &&
 			    su_dev->t10_alua.alua_type == SPC3_ALUA_EMULATED) {
 				cmd->execute_cmd =
 					target_emulate_report_target_port_groups;
-- 
1.7.2.5

[PATCH 2/2] target: Add MI_REPORT_TARGET_PGS ext. header + implict_trans_secs attribute

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch adds support for ALUA MI_REPORT_TARGET_PGS extended header
format defined within SPC-4.  It changes target core ALUA emulation logic
within target_emulate_report_target_port_groups() to support both the
extended and original length only header formats.

It includes adding a new 'implict_trans_secs' attribute for each ALUA
target port group to control the value returned to the application client
for an recommended implict translation timeout in seconds.  By default
this value is currently set to zero, and limited up to 255 by virtue of
using a single byte in the extended header format.

This value is used by target_emulate_report_target_port_groups() within
the extended header logic to set IMPLICIT TRANSITION TIME as defined by
spc4r30.

Cc: Hannes Reinecke <hare@suse.de>
Cc: Rob Evers <revers@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Roland Dreier <roland@purestorage.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_alua.c     |   82 ++++++++++++++++++++++++++++-----
 drivers/target/target_core_alua.h     |   10 ++++
 drivers/target/target_core_configfs.c |   21 ++++++++
 include/target/target_core_base.h     |    1 +
 4 files changed, 102 insertions(+), 12 deletions(-)

diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
index be7251e..e624b83 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -66,18 +66,24 @@ int target_emulate_report_target_port_groups(struct se_cmd *cmd)
 	struct t10_alua_tg_pt_gp *tg_pt_gp;
 	struct t10_alua_tg_pt_gp_member *tg_pt_gp_mem;
 	unsigned char *buf;
-	u32 rd_len = 0, off = 4; /* Skip over RESERVED area to first
-				    Target port group descriptor */
+	u32 rd_len = 0, off;
+	int ext_hdr = (cmd->t_task_cdb[1] & 0x20);
 	/*
-	 * Need at least 4 bytes of response data or else we can't
-	 * even fit the return data length.
+	 * Skip over RESERVED area to first Target port group descriptor
+	 * depending on the PARAMETER DATA FORMAT type..
 	 */
-	if (cmd->data_length < 4) {
-		pr_warn("REPORT TARGET PORT GROUPS allocation length %u"
-			" too small\n", cmd->data_length);
+	if (ext_hdr != 0)
+		off = 8;
+	else
+		off = 4;
+
+	if (cmd->data_length < off) {
+		pr_warn("REPORT TARGET PORT GROUPS allocation length %u too"
+			" small for %s header\n", cmd->data_length,
+			(ext_hdr) ? "extended" : "normal");
+		cmd->scsi_sense_reason = TCM_INVALID_CDB_FIELD;
 		return -EINVAL;
 	}
-
 	buf = transport_kmap_data_sg(cmd);
 
 	spin_lock(&su_dev->t10_alua.tg_pt_gps_lock);
@@ -158,11 +164,31 @@ int target_emulate_report_target_port_groups(struct se_cmd *cmd)
 	/*
 	 * Set the RETURN DATA LENGTH set in the header of the DataIN Payload
 	 */
-	buf[0] = ((rd_len >> 24) & 0xff);
-	buf[1] = ((rd_len >> 16) & 0xff);
-	buf[2] = ((rd_len >> 8) & 0xff);
-	buf[3] = (rd_len & 0xff);
+	put_unaligned_be32(rd_len, &buf[0]);
 
+	/*
+	 * Fill in the Extended header parameter data format if requested
+	 */
+	if (ext_hdr != 0) {
+		buf[4] = 0x10;
+		/*
+		 * Set the implict transition time (in seconds) for the application
+		 * client to use as a base for it's transition timeout value.
+		 *
+		 * Use the current tg_pt_gp_mem -> tg_pt_gp membership from the LUN
+		 * this CDB was received upon to determine this value individually
+		 * for ALUA target port group.
+		 */
+		port = cmd->se_lun->lun_sep;
+		tg_pt_gp_mem = port->sep_alua_tg_pt_gp_mem;
+		if (tg_pt_gp_mem) {
+			spin_lock(&tg_pt_gp_mem->tg_pt_gp_mem_lock);
+			tg_pt_gp = tg_pt_gp_mem->tg_pt_gp;
+			if (tg_pt_gp)
+				buf[5] = tg_pt_gp->tg_pt_gp_implict_trans_secs;
+			spin_unlock(&tg_pt_gp_mem->tg_pt_gp_mem_lock);
+		}
+	}
 	transport_kunmap_data_sg(cmd);
 
 	target_complete_cmd(cmd, GOOD);
@@ -1355,6 +1381,7 @@ struct t10_alua_tg_pt_gp *core_alua_allocate_tg_pt_gp(
 	 */
 	tg_pt_gp->tg_pt_gp_nonop_delay_msecs = ALUA_DEFAULT_NONOP_DELAY_MSECS;
 	tg_pt_gp->tg_pt_gp_trans_delay_msecs = ALUA_DEFAULT_TRANS_DELAY_MSECS;
+	tg_pt_gp->tg_pt_gp_implict_trans_secs = ALUA_DEFAULT_IMPLICT_TRANS_SECS;
 
 	if (def_group) {
 		spin_lock(&su_dev->t10_alua.tg_pt_gps_lock);
@@ -1851,6 +1878,37 @@ ssize_t core_alua_store_trans_delay_msecs(
 	return count;
 }
 
+ssize_t core_alua_show_implict_trans_secs(
+	struct t10_alua_tg_pt_gp *tg_pt_gp,
+	char *page)
+{
+	return sprintf(page, "%d\n", tg_pt_gp->tg_pt_gp_implict_trans_secs);
+}
+
+ssize_t core_alua_store_implict_trans_secs(
+	struct t10_alua_tg_pt_gp *tg_pt_gp,
+	const char *page,
+	size_t count)
+{
+	unsigned long tmp;
+	int ret;
+
+	ret = strict_strtoul(page, 0, &tmp);
+	if (ret < 0) {
+		pr_err("Unable to extract implict_trans_secs\n");
+		return -EINVAL;
+	}
+	if (tmp > ALUA_MAX_IMPLICT_TRANS_SECS) {
+		pr_err("Passed implict_trans_secs: %lu, exceeds"
+			" ALUA_MAX_IMPLICT_TRANS_SECS: %d\n", tmp,
+			ALUA_MAX_IMPLICT_TRANS_SECS);
+		return  -EINVAL;
+	}
+	tg_pt_gp->tg_pt_gp_implict_trans_secs = (int)tmp;
+
+	return count;
+}
+
 ssize_t core_alua_show_preferred_bit(
 	struct t10_alua_tg_pt_gp *tg_pt_gp,
 	char *page)
diff --git a/drivers/target/target_core_alua.h b/drivers/target/target_core_alua.h
index e264543..f920c17 100644
--- a/drivers/target/target_core_alua.h
+++ b/drivers/target/target_core_alua.h
@@ -52,6 +52,12 @@
 #define ALUA_DEFAULT_TRANS_DELAY_MSECS			0
 #define ALUA_MAX_TRANS_DELAY_MSECS			30000 /* 30 seconds */
 /*
+ * Used for the recommended application client implict transition timeout
+ * in seconds, returned by the REPORT_TARGET_PORT_GROUPS w/ extended header.
+ */
+#define ALUA_DEFAULT_IMPLICT_TRANS_SECS			0
+#define ALUA_MAX_IMPLICT_TRANS_SECS			255
+/*
  * Used by core_alua_update_tpg_primary_metadata() and
  * core_alua_update_tpg_secondary_metadata()
  */
@@ -107,6 +113,10 @@ extern ssize_t core_alua_show_trans_delay_msecs(struct t10_alua_tg_pt_gp *,
 					char *);
 extern ssize_t core_alua_store_trans_delay_msecs(struct t10_alua_tg_pt_gp *,
 					const char *, size_t);
+extern ssize_t core_alua_show_implict_trans_secs(struct t10_alua_tg_pt_gp *,
+					char *);
+extern ssize_t core_alua_store_implict_trans_secs(struct t10_alua_tg_pt_gp *,
+					const char *, size_t);
 extern ssize_t core_alua_show_preferred_bit(struct t10_alua_tg_pt_gp *,
 					char *);
 extern ssize_t core_alua_store_preferred_bit(struct t10_alua_tg_pt_gp *,
diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index d13a340..11cba94 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -2447,6 +2447,26 @@ static ssize_t target_core_alua_tg_pt_gp_store_attr_trans_delay_msecs(
 SE_DEV_ALUA_TG_PT_ATTR(trans_delay_msecs, S_IRUGO | S_IWUSR);
 
 /*
+ * implict_trans_secs
+ */
+static ssize_t target_core_alua_tg_pt_gp_show_attr_implict_trans_secs(
+	struct t10_alua_tg_pt_gp *tg_pt_gp,
+	char *page)
+{
+	return core_alua_show_implict_trans_secs(tg_pt_gp, page);
+}
+
+static ssize_t target_core_alua_tg_pt_gp_store_attr_implict_trans_secs(
+	struct t10_alua_tg_pt_gp *tg_pt_gp,
+	const char *page,
+	size_t count)
+{
+	return core_alua_store_implict_trans_secs(tg_pt_gp, page, count);
+}
+
+SE_DEV_ALUA_TG_PT_ATTR(implict_trans_secs, S_IRUGO | S_IWUSR);
+
+/*
  * preferred
  */
 
@@ -2570,6 +2590,7 @@ static struct configfs_attribute *target_core_alua_tg_pt_gp_attrs[] = {
 	&target_core_alua_tg_pt_gp_alua_write_metadata.attr,
 	&target_core_alua_tg_pt_gp_nonop_delay_msecs.attr,
 	&target_core_alua_tg_pt_gp_trans_delay_msecs.attr,
+	&target_core_alua_tg_pt_gp_implict_trans_secs.attr,
 	&target_core_alua_tg_pt_gp_preferred.attr,
 	&target_core_alua_tg_pt_gp_tg_pt_gp_id.attr,
 	&target_core_alua_tg_pt_gp_members.attr,
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 847fec5..6816191 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -330,6 +330,7 @@ struct t10_alua_tg_pt_gp {
 	int	tg_pt_gp_alua_access_type;
 	int	tg_pt_gp_nonop_delay_msecs;
 	int	tg_pt_gp_trans_delay_msecs;
+	int	tg_pt_gp_implict_trans_secs;
 	int	tg_pt_gp_pref;
 	int	tg_pt_gp_write_metadata;
 	/* Used by struct t10_alua_tg_pt_gp->tg_pt_gp_md_buf_len */
-- 
1.7.2.5