[PATCH 0/9] target: Allow fabric to expose PI + add WRITE_INSERT/READ_STRIP

[PATCH 0/9] target: Allow fabric to expose PI + add WRITE_INSERT/READ_STRIP

[Nicholas A. Bellinger]

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

Hi MKP, Sagi, Quinn & Co,

This series allows fabric drivers to expose what TARGET_PROT hardware
operations they support at session initialization time, along with
adding proper WRITE_INSERT/READ_STRIP emulation support (as originally
mentioned by Sagi) into target-core.

This allows legacy initiator ports and/or legacy fabrics to interact
using hardware (or software) offloads together with T10 PI enabled
backend devices.

The code allows for the same backend device to report PROTECT=1
across one protected fabric port, while reporting PROTECT=0 to another
and performing locally generated WRITE_INSERT/READ_STRIP software
operations.

So far this is working across a RAMDISK_MCP backend using loopback
in existing TARGET_PROT PASS mode, and a traditional iSCSI target port
using WRITE_INSERT/READ_STRIP.

Please review.

--nab

Nicholas Bellinger (9):
  target/iblock: Fix double bioset_integrity_free bug
  target: Pass in transport supported PI at session initialization
  target/spc: Only expose PI inquiry bits when supported by fabric
  target/spc: Only expose PI mode page bits when supported by fabric
  target/sbc: Only expose PI read_cap16 bits when supported by fabric
  target/sbc: Add sbc_dif_write_insert software emulation
  target: Enable WRITE_INSERT emulation in target_execute_cmd
  target/sbc: Add sbc_dif_read_strip software emulation
  target: Enable READ_STRIP emulation in target_complete_ok_work

 drivers/infiniband/ulp/isert/ib_isert.c   |   13 +++++
 drivers/infiniband/ulp/srpt/ib_srpt.c     |    2 +-
 drivers/scsi/qla2xxx/tcm_qla2xxx.c        |    2 +-
 drivers/target/iscsi/iscsi_target.c       |    6 +++
 drivers/target/iscsi/iscsi_target_login.c |    4 +-
 drivers/target/loopback/tcm_loop.c        |    2 +-
 drivers/target/sbp/sbp_target.c           |    2 +-
 drivers/target/target_core_iblock.c       |    5 +-
 drivers/target/target_core_sbc.c          |   82 ++++++++++++++++++++++++++---
 drivers/target/target_core_spc.c          |   49 ++++++++++-------
 drivers/target/target_core_transport.c    |   50 ++++++++++++++++--
 drivers/target/tcm_fc/tfc_sess.c          |    3 +-
 drivers/usb/gadget/tcm_usb_gadget.c       |    2 +-
 drivers/vhost/scsi.c                      |    3 +-
 include/target/iscsi/iscsi_transport.h    |    1 +
 include/target/target_core_backend.h      |    2 +
 include/target/target_core_base.h         |   19 ++++---
 include/target/target_core_fabric.h       |    5 +-
 18 files changed, 205 insertions(+), 47 deletions(-)

-- 
1.7.10.4

[PATCH 1/9] target/iblock: Fix double bioset_integrity_free bug

[Nicholas A. Bellinger]

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

This patch fixes a double free bug during IBLOCK backend shutdown
where bioset_integrity_free() was incorrectly called ahead of
bioset_free(), who is already making the same call directly.

This bug was introduced with commit ecebbf6cc, and will end up
triggering a general protection fault in iblock_free_device()

Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Or Gerlitz <ogerlitz@mellanox.com>
Cc: Quinn Tran <quinn.tran@qlogic.com>
Cc: Giridhar Malavali <giridhar.malavali@qlogic.com>
Cc: <stable@vger.kernel.org> #3.14+
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_iblock.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
index 554d4f7..9e0232c 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -203,10 +203,9 @@ static void iblock_free_device(struct se_device *dev)
 
 	if (ib_dev->ibd_bd != NULL)
 		blkdev_put(ib_dev->ibd_bd, FMODE_WRITE|FMODE_READ|FMODE_EXCL);
-	if (ib_dev->ibd_bio_set != NULL) {
-		bioset_integrity_free(ib_dev->ibd_bio_set);
+	if (ib_dev->ibd_bio_set != NULL)
 		bioset_free(ib_dev->ibd_bio_set);
-	}
+
 	kfree(ib_dev);
 }
 
-- 
1.7.10.4

[PATCH 2/9] target: Pass in transport supported PI at session initialization

[Nicholas A. Bellinger]

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

In order to support local WRITE_INSERT + READ_STRIP operations for
non PI enabled fabrics, the fabric driver needs to be able signal
what protection offload operations are supported.

This is done at session initialization time so the modes can be
signaled by individual se_wwn + se_portal_group endpoints, as well
as optionally across different transports on the same endpoint.

For iser-target, set TARGET_PROT_ALL if the underlying ib_device
has already signaled PI offload support, and allow this to be
exposed via a new iscsit_transport->iscsit_get_sup_prot_ops()
callback.

For loopback, set TARGET_PROT_ALL to signal SCSI initiator mode
operation.

For all other drivers, set TARGET_PROT_NORMAL to disable fabric
level PI.

Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Or Gerlitz <ogerlitz@mellanox.com>
Cc: Quinn Tran <quinn.tran@qlogic.com>
Cc: Giridhar Malavali <giridhar.malavali@qlogic.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/infiniband/ulp/isert/ib_isert.c   |   13 +++++++++++++
 drivers/infiniband/ulp/srpt/ib_srpt.c     |    2 +-
 drivers/scsi/qla2xxx/tcm_qla2xxx.c        |    2 +-
 drivers/target/iscsi/iscsi_target.c       |    6 ++++++
 drivers/target/iscsi/iscsi_target_login.c |    4 +++-
 drivers/target/loopback/tcm_loop.c        |    2 +-
 drivers/target/sbp/sbp_target.c           |    2 +-
 drivers/target/target_core_transport.c    |    8 +++++---
 drivers/target/tcm_fc/tfc_sess.c          |    3 ++-
 drivers/usb/gadget/tcm_usb_gadget.c       |    2 +-
 drivers/vhost/scsi.c                      |    3 ++-
 include/target/iscsi/iscsi_transport.h    |    1 +
 include/target/target_core_base.h         |   19 ++++++++++++-------
 include/target/target_core_fabric.h       |    5 +++--
 14 files changed, 52 insertions(+), 20 deletions(-)

diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index f5cc4af..c98fdb1 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -2196,6 +2196,18 @@ isert_aborted_task(struct iscsi_conn *conn, struct iscsi_cmd *cmd)
 	device->unreg_rdma_mem(isert_cmd, isert_conn);
 }
 
+static enum target_prot_op
+isert_get_sup_prot_ops(struct iscsi_conn *conn)
+{
+	struct isert_conn *isert_conn = (struct isert_conn *)conn->context;
+	struct isert_device *device = isert_conn->conn_device;
+
+	if (device->pi_capable)
+		return TARGET_PROT_ALL;
+
+	return TARGET_PROT_NORMAL;
+}
+
 static int
 isert_put_nopin(struct iscsi_cmd *cmd, struct iscsi_conn *conn,
 		bool nopout_response)
@@ -3252,6 +3264,7 @@ static struct iscsit_transport iser_target_transport = {
 	.iscsit_queue_data_in	= isert_put_datain,
 	.iscsit_queue_status	= isert_put_response,
 	.iscsit_aborted_task	= isert_aborted_task,
+	.iscsit_get_sup_prot_ops = isert_get_sup_prot_ops,
 };
 
 static int __init isert_init(void)
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index f03aafd..bcfb398 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -2580,7 +2580,7 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
 		goto destroy_ib;
 	}
 
-	ch->sess = transport_init_session();
+	ch->sess = transport_init_session(TARGET_PROT_NORMAL);
 	if (IS_ERR(ch->sess)) {
 		rej->reason = __constant_cpu_to_be32(
 				SRP_LOGIN_REJ_INSUFFICIENT_RESOURCES);
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index b23a0ff..68fb66f 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -1482,7 +1482,7 @@ static int tcm_qla2xxx_check_initiator_node_acl(
 	}
 	se_tpg = &tpg->se_tpg;
 
-	se_sess = transport_init_session();
+	se_sess = transport_init_session(TARGET_PROT_NORMAL);
 	if (IS_ERR(se_sess)) {
 		pr_err("Unable to initialize struct se_session\n");
 		return PTR_ERR(se_sess);
diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 96aee43..78cab13 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -511,6 +511,11 @@ static void iscsit_aborted_task(struct iscsi_conn *conn, struct iscsi_cmd *cmd)
 	__iscsit_free_cmd(cmd, scsi_cmd, true);
 }
 
+static enum target_prot_op iscsit_get_sup_prot_ops(struct iscsi_conn *conn)
+{
+	return TARGET_PROT_NORMAL;
+}
+
 static struct iscsit_transport iscsi_target_transport = {
 	.name			= "iSCSI/TCP",
 	.transport_type		= ISCSI_TCP,
@@ -526,6 +531,7 @@ static struct iscsit_transport iscsi_target_transport = {
 	.iscsit_queue_data_in	= iscsit_queue_rsp,
 	.iscsit_queue_status	= iscsit_queue_rsp,
 	.iscsit_aborted_task	= iscsit_aborted_task,
+	.iscsit_get_sup_prot_ops = iscsit_get_sup_prot_ops,
 };
 
 static int __init iscsi_target_init_module(void)
diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
index e29279e..8739b98 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -259,6 +259,7 @@ static int iscsi_login_zero_tsih_s1(
 {
 	struct iscsi_session *sess = NULL;
 	struct iscsi_login_req *pdu = (struct iscsi_login_req *)buf;
+	enum target_prot_op sup_pro_ops;
 	int ret;
 
 	sess = kzalloc(sizeof(struct iscsi_session), GFP_KERNEL);
@@ -320,8 +321,9 @@ static int iscsi_login_zero_tsih_s1(
 		kfree(sess);
 		return -ENOMEM;
 	}
+	sup_pro_ops = conn->conn_transport->iscsit_get_sup_prot_ops(conn);
 
-	sess->se_sess = transport_init_session();
+	sess->se_sess = transport_init_session(sup_pro_ops);
 	if (IS_ERR(sess->se_sess)) {
 		iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
 				ISCSI_LOGIN_STATUS_NO_RESOURCES);
diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
index bdc1ad8..c886ad1 100644
--- a/drivers/target/loopback/tcm_loop.c
+++ b/drivers/target/loopback/tcm_loop.c
@@ -1018,7 +1018,7 @@ static int tcm_loop_make_nexus(
 	/*
 	 * Initialize the struct se_session pointer
 	 */
-	tl_nexus->se_sess = transport_init_session();
+	tl_nexus->se_sess = transport_init_session(TARGET_PROT_ALL);
 	if (IS_ERR(tl_nexus->se_sess)) {
 		ret = PTR_ERR(tl_nexus->se_sess);
 		goto out;
diff --git a/drivers/target/sbp/sbp_target.c b/drivers/target/sbp/sbp_target.c
index ad04ea9..e7e9372 100644
--- a/drivers/target/sbp/sbp_target.c
+++ b/drivers/target/sbp/sbp_target.c
@@ -210,7 +210,7 @@ static struct sbp_session *sbp_session_create(
 		return ERR_PTR(-ENOMEM);
 	}
 
-	sess->se_sess = transport_init_session();
+	sess->se_sess = transport_init_session(TARGET_PROT_NORMAL);
 	if (IS_ERR(sess->se_sess)) {
 		pr_err("failed to init se_session\n");
 
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 9393544..9c820ba 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -235,7 +235,7 @@ void transport_subsystem_check_init(void)
 	sub_api_initialized = 1;
 }
 
-struct se_session *transport_init_session(void)
+struct se_session *transport_init_session(enum target_prot_op sup_prot_ops)
 {
 	struct se_session *se_sess;
 
@@ -251,6 +251,7 @@ struct se_session *transport_init_session(void)
 	INIT_LIST_HEAD(&se_sess->sess_wait_list);
 	spin_lock_init(&se_sess->sess_cmd_lock);
 	kref_init(&se_sess->sess_kref);
+	se_sess->sup_prot_ops = sup_prot_ops;
 
 	return se_sess;
 }
@@ -288,12 +289,13 @@ int transport_alloc_session_tags(struct se_session *se_sess,
 EXPORT_SYMBOL(transport_alloc_session_tags);
 
 struct se_session *transport_init_session_tags(unsigned int tag_num,
-					       unsigned int tag_size)
+					       unsigned int tag_size,
+					       enum target_prot_op sup_prot_ops)
 {
 	struct se_session *se_sess;
 	int rc;
 
-	se_sess = transport_init_session();
+	se_sess = transport_init_session(sup_prot_ops);
 	if (IS_ERR(se_sess))
 		return se_sess;
 
diff --git a/drivers/target/tcm_fc/tfc_sess.c b/drivers/target/tcm_fc/tfc_sess.c
index ae52c08..0475142 100644
--- a/drivers/target/tcm_fc/tfc_sess.c
+++ b/drivers/target/tcm_fc/tfc_sess.c
@@ -211,7 +211,8 @@ static struct ft_sess *ft_sess_create(struct ft_tport *tport, u32 port_id,
 		return NULL;
 
 	sess->se_sess = transport_init_session_tags(TCM_FC_DEFAULT_TAGS,
-						    sizeof(struct ft_cmd));
+						    sizeof(struct ft_cmd),
+						    TARGET_PROT_NORMAL);
 	if (IS_ERR(sess->se_sess)) {
 		kfree(sess);
 		return NULL;
diff --git a/drivers/usb/gadget/tcm_usb_gadget.c b/drivers/usb/gadget/tcm_usb_gadget.c
index f9afa4a..f34b6df 100644
--- a/drivers/usb/gadget/tcm_usb_gadget.c
+++ b/drivers/usb/gadget/tcm_usb_gadget.c
@@ -1731,7 +1731,7 @@ static int tcm_usbg_make_nexus(struct usbg_tpg *tpg, char *name)
 		pr_err("Unable to allocate struct tcm_vhost_nexus\n");
 		goto err_unlock;
 	}
-	tv_nexus->tvn_se_sess = transport_init_session();
+	tv_nexus->tvn_se_sess = transport_init_session(TARGET_PROT_NORMAL);
 	if (IS_ERR(tv_nexus->tvn_se_sess))
 		goto err_free;
 
diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 4a47335..cf50ce9 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -1745,7 +1745,8 @@ static int tcm_vhost_make_nexus(struct tcm_vhost_tpg *tpg,
 	 */
 	tv_nexus->tvn_se_sess = transport_init_session_tags(
 					TCM_VHOST_DEFAULT_TAGS,
-					sizeof(struct tcm_vhost_cmd));
+					sizeof(struct tcm_vhost_cmd),
+					TARGET_PROT_NORMAL);
 	if (IS_ERR(tv_nexus->tvn_se_sess)) {
 		mutex_unlock(&tpg->tv_tpg_mutex);
 		kfree(tv_nexus);
diff --git a/include/target/iscsi/iscsi_transport.h b/include/target/iscsi/iscsi_transport.h
index 8d19339..33b487b 100644
--- a/include/target/iscsi/iscsi_transport.h
+++ b/include/target/iscsi/iscsi_transport.h
@@ -22,6 +22,7 @@ struct iscsit_transport {
 	int (*iscsit_queue_data_in)(struct iscsi_conn *, struct iscsi_cmd *);
 	int (*iscsit_queue_status)(struct iscsi_conn *, struct iscsi_cmd *);
 	void (*iscsit_aborted_task)(struct iscsi_conn *, struct iscsi_cmd *);
+	enum target_prot_op (*iscsit_get_sup_prot_ops)(struct iscsi_conn *);
 };
 
 static inline void *iscsit_priv_cmd(struct iscsi_cmd *cmd)
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index ec3e3a3..9ec9864 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -442,15 +442,19 @@ struct se_tmr_req {
 };
 
 enum target_prot_op {
-	TARGET_PROT_NORMAL = 0,
-	TARGET_PROT_DIN_INSERT,
-	TARGET_PROT_DOUT_INSERT,
-	TARGET_PROT_DIN_STRIP,
-	TARGET_PROT_DOUT_STRIP,
-	TARGET_PROT_DIN_PASS,
-	TARGET_PROT_DOUT_PASS,
+	TARGET_PROT_NORMAL	= 0,
+	TARGET_PROT_DIN_INSERT	= (1 << 0),
+	TARGET_PROT_DOUT_INSERT	= (1 << 1),
+	TARGET_PROT_DIN_STRIP	= (1 << 2),
+	TARGET_PROT_DOUT_STRIP	= (1 << 3),
+	TARGET_PROT_DIN_PASS	= (1 << 4),
+	TARGET_PROT_DOUT_PASS	= (1 << 5),
 };
 
+#define TARGET_PROT_ALL	TARGET_PROT_DIN_INSERT | TARGET_PROT_DOUT_INSERT | \
+			TARGET_PROT_DIN_STRIP | TARGET_PROT_DOUT_STRIP | \
+			TARGET_PROT_DIN_PASS | TARGET_PROT_DOUT_PASS
+
 enum target_prot_type {
 	TARGET_DIF_TYPE0_PROT,
 	TARGET_DIF_TYPE1_PROT,
@@ -605,6 +609,7 @@ struct se_node_acl {
 struct se_session {
 	unsigned		sess_tearing_down:1;
 	u64			sess_bin_isid;
+	enum target_prot_op	sup_prot_ops;
 	struct se_node_acl	*se_node_acl;
 	struct se_portal_group *se_tpg;
 	void			*fabric_sess_ptr;
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 1d10436..22a4e98 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -84,10 +84,11 @@ struct target_core_fabric_ops {
 	void (*fabric_drop_nodeacl)(struct se_node_acl *);
 };
 
-struct se_session *transport_init_session(void);
+struct se_session *transport_init_session(enum target_prot_op);
 int transport_alloc_session_tags(struct se_session *, unsigned int,
 		unsigned int);
-struct se_session *transport_init_session_tags(unsigned int, unsigned int);
+struct se_session *transport_init_session_tags(unsigned int, unsigned int,
+		enum target_prot_op);
 void	__transport_register_session(struct se_portal_group *,
 		struct se_node_acl *, struct se_session *, void *);
 void	transport_register_session(struct se_portal_group *,
-- 
1.7.10.4

[PATCH 3/9] target/spc: Only expose PI inquiry bits when supported by fabric

[Nicholas A. Bellinger]

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

Only expose standard INQUIRY PROTECT=1 and EVPD=0x86 TYPE1/TYPE3
PI control bits if the session + fabric support DIX PASS operations.

Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Or Gerlitz <ogerlitz@mellanox.com>
Cc: Quinn Tran <quinn.tran@qlogic.com>
Cc: Giridhar Malavali <giridhar.malavali@qlogic.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_spc.c |   21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index 3bebc71..d4c6a31 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -71,6 +71,7 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned char *buf)
 {
 	struct se_lun *lun = cmd->se_lun;
 	struct se_device *dev = cmd->se_dev;
+	struct se_session *sess = cmd->se_sess;
 
 	/* Set RMB (removable media) for tape devices */
 	if (dev->transport->get_device_type(dev) == TYPE_TAPE)
@@ -101,10 +102,13 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned char *buf)
 	if (dev->dev_attrib.emulate_3pc)
 		buf[5] |= 0x8;
 	/*
-	 * Set Protection (PROTECT) bit when DIF has been enabled.
+	 * Set Protection (PROTECT) bit when DIF has been enabled on the
+	 * device, and the transport supports VERIFY + PASS.
 	 */
-	if (dev->dev_attrib.pi_prot_type)
-		buf[5] |= 0x1;
+	if (sess->sup_prot_ops & (TARGET_PROT_DIN_PASS | TARGET_PROT_DOUT_PASS)) {
+		if (dev->dev_attrib.pi_prot_type)
+			buf[5] |= 0x1;
+	}
 
 	buf[7] = 0x2; /* CmdQue=1 */
 
@@ -473,16 +477,19 @@ static sense_reason_t
 spc_emulate_evpd_86(struct se_cmd *cmd, unsigned char *buf)
 {
 	struct se_device *dev = cmd->se_dev;
+	struct se_session *sess = cmd->se_sess;
 
 	buf[3] = 0x3c;
 	/*
 	 * Set GRD_CHK + REF_CHK for TYPE1 protection, or GRD_CHK
 	 * only for TYPE3 protection.
 	 */
-	if (dev->dev_attrib.pi_prot_type == TARGET_DIF_TYPE1_PROT)
-		buf[4] = 0x5;
-	else if (dev->dev_attrib.pi_prot_type == TARGET_DIF_TYPE3_PROT)
-		buf[4] = 0x4;
+	if (sess->sup_prot_ops & (TARGET_PROT_DIN_PASS | TARGET_PROT_DOUT_PASS)) {
+		if (dev->dev_attrib.pi_prot_type == TARGET_DIF_TYPE1_PROT)
+			buf[4] = 0x5;
+		else if (dev->dev_attrib.pi_prot_type == TARGET_DIF_TYPE3_PROT)
+			buf[4] = 0x4;
+	}
 
 	/* Set HEADSUP, ORDSUP, SIMPSUP */
 	buf[5] = 0x07;
-- 
1.7.10.4

[PATCH 4/9] target/spc: Only expose PI mode page bits when supported by fabric

[Nicholas A. Bellinger]

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

Only expose the control modepage bit for Application Tag Owner (ATO)
if the session + fabric support DIX PASS operations.

Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Or Gerlitz <ogerlitz@mellanox.com>
Cc: Quinn Tran <quinn.tran@qlogic.com>
Cc: Giridhar Malavali <giridhar.malavali@qlogic.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_spc.c |   28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index d4c6a31..8653666 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -769,7 +769,7 @@ out:
 	return ret;
 }
 
-static int spc_modesense_rwrecovery(struct se_device *dev, u8 pc, u8 *p)
+static int spc_modesense_rwrecovery(struct se_cmd *cmd, u8 pc, u8 *p)
 {
 	p[0] = 0x01;
 	p[1] = 0x0a;
@@ -782,8 +782,11 @@ out:
 	return 12;
 }
 
-static int spc_modesense_control(struct se_device *dev, u8 pc, u8 *p)
+static int spc_modesense_control(struct se_cmd *cmd, u8 pc, u8 *p)
 {
+	struct se_device *dev = cmd->se_dev;
+	struct se_session *sess = cmd->se_sess;
+
 	p[0] = 0x0a;
 	p[1] = 0x0a;
 
@@ -875,8 +878,10 @@ static int spc_modesense_control(struct se_device *dev, u8 pc, u8 *p)
 	 * type, shall not modify the contents of the LOGICAL BLOCK REFERENCE
 	 * TAG field.
 	 */
-	if (dev->dev_attrib.pi_prot_type)
-		p[5] |= 0x80;
+	if (sess->sup_prot_ops & (TARGET_PROT_DIN_PASS | TARGET_PROT_DOUT_PASS)) {
+		if (dev->dev_attrib.pi_prot_type)
+			p[5] |= 0x80;
+	}
 
 	p[8] = 0xff;
 	p[9] = 0xff;
@@ -886,8 +891,10 @@ out:
 	return 12;
 }
 
-static int spc_modesense_caching(struct se_device *dev, u8 pc, u8 *p)
+static int spc_modesense_caching(struct se_cmd *cmd, u8 pc, u8 *p)
 {
+	struct se_device *dev = cmd->se_dev;
+
 	p[0] = 0x08;
 	p[1] = 0x12;
 
@@ -903,7 +910,7 @@ out:
 	return 20;
 }
 
-static int spc_modesense_informational_exceptions(struct se_device *dev, u8 pc, unsigned char *p)
+static int spc_modesense_informational_exceptions(struct se_cmd *cmd, u8 pc, unsigned char *p)
 {
 	p[0] = 0x1c;
 	p[1] = 0x0a;
@@ -919,7 +926,7 @@ out:
 static struct {
 	uint8_t		page;
 	uint8_t		subpage;
-	int		(*emulate)(struct se_device *, u8, unsigned char *);
+	int		(*emulate)(struct se_cmd *, u8, unsigned char *);
 } modesense_handlers[] = {
 	{ .page = 0x01, .subpage = 0x00, .emulate = spc_modesense_rwrecovery },
 	{ .page = 0x08, .subpage = 0x00, .emulate = spc_modesense_caching },
@@ -1057,7 +1064,7 @@ static sense_reason_t spc_emulate_modesense(struct se_cmd *cmd)
 			 * the only two possibilities).
 			 */
 			if ((modesense_handlers[i].subpage & ~subpage) == 0) {
-				ret = modesense_handlers[i].emulate(dev, pc, &buf[length]);
+				ret = modesense_handlers[i].emulate(cmd, pc, &buf[length]);
 				if (!ten && length + ret >= 255)
 					break;
 				length += ret;
@@ -1070,7 +1077,7 @@ static sense_reason_t spc_emulate_modesense(struct se_cmd *cmd)
 	for (i = 0; i < ARRAY_SIZE(modesense_handlers); ++i)
 		if (modesense_handlers[i].page == page &&
 		    modesense_handlers[i].subpage == subpage) {
-			length += modesense_handlers[i].emulate(dev, pc, &buf[length]);
+			length += modesense_handlers[i].emulate(cmd, pc, &buf[length]);
 			goto set_length;
 		}
 
@@ -1102,7 +1109,6 @@ set_length:
 
 static sense_reason_t spc_emulate_modeselect(struct se_cmd *cmd)
 {
-	struct se_device *dev = cmd->se_dev;
 	char *cdb = cmd->t_task_cdb;
 	bool ten = cdb[0] == MODE_SELECT_10;
 	int off = ten ? 8 : 4;
@@ -1138,7 +1144,7 @@ static sense_reason_t spc_emulate_modeselect(struct se_cmd *cmd)
 		if (modesense_handlers[i].page == page &&
 		    modesense_handlers[i].subpage == subpage) {
 			memset(tbuf, 0, SE_MODE_PAGE_BUF);
-			length = modesense_handlers[i].emulate(dev, 0, tbuf);
+			length = modesense_handlers[i].emulate(cmd, 0, tbuf);
 			goto check_contents;
 		}
 
-- 
1.7.10.4

[PATCH 5/9] target/sbc: Only expose PI read_cap16 bits when supported by fabric

[Nicholas A. Bellinger]

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

Only expose the PI protection type bits in READ_CAPACITY_16
if the session + fabric support DIX PASS operations.

Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Or Gerlitz <ogerlitz@mellanox.com>
Cc: Quinn Tran <quinn.tran@qlogic.com>
Cc: Giridhar Malavali <giridhar.malavali@qlogic.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_sbc.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index ec204f7..f2d73dd 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -89,6 +89,7 @@ static sense_reason_t
 sbc_emulate_readcapacity_16(struct se_cmd *cmd)
 {
 	struct se_device *dev = cmd->se_dev;
+	struct se_session *sess = cmd->se_sess;
 	unsigned char *rbuf;
 	unsigned char buf[32];
 	unsigned long long blocks = dev->transport->get_blocks(dev);
@@ -109,8 +110,10 @@ sbc_emulate_readcapacity_16(struct se_cmd *cmd)
 	/*
 	 * Set P_TYPE and PROT_EN bits for DIF support
 	 */
-	if (dev->dev_attrib.pi_prot_type)
-		buf[12] = (dev->dev_attrib.pi_prot_type - 1) << 1 | 0x1;
+	if (sess->sup_prot_ops & (TARGET_PROT_DIN_PASS | TARGET_PROT_DOUT_PASS)) {
+		if (dev->dev_attrib.pi_prot_type)
+			buf[12] = (dev->dev_attrib.pi_prot_type - 1) << 1 | 0x1;
+	}
 
 	if (dev->transport->get_lbppbe)
 		buf[13] = dev->transport->get_lbppbe(dev) & 0x0f;
-- 
1.7.10.4

[PATCH 6/9] target/sbc: Add sbc_dif_write_insert software emulation

[Nicholas A. Bellinger]

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

This patch adds WRITE_INSERT emulation within target-core
using TYPE1 / TYPE3 PI modes.

This is useful in order for existing legacy fabrics that do not
support protection offloads to interact with backend devices that
currently have T10 PI enabled.

Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Or Gerlitz <ogerlitz@mellanox.com>
Cc: Quinn Tran <quinn.tran@qlogic.com>
Cc: Giridhar Malavali <giridhar.malavali@qlogic.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_sbc.c     |   44 ++++++++++++++++++++++++++++++++++
 include/target/target_core_backend.h |    1 +
 2 files changed, 45 insertions(+)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index f2d73dd..97b207d 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -1096,6 +1096,50 @@ err:
 }
 EXPORT_SYMBOL(sbc_execute_unmap);
 
+void
+sbc_dif_write_insert(struct se_cmd *cmd)
+{
+	struct se_device *dev = cmd->se_dev;
+	struct se_dif_v1_tuple *sdt;
+	struct scatterlist *dsg, *psg = cmd->t_prot_sg;
+	sector_t sector = cmd->t_task_lba;
+	void *daddr, *paddr;
+	int i, j, offset = 0;
+
+	for_each_sg(cmd->t_data_sg, dsg, cmd->t_data_nents, i) {
+		daddr = kmap_atomic(sg_page(dsg)) + dsg->offset;
+		paddr = kmap_atomic(sg_page(psg)) + psg->offset;
+
+		for (j = 0; j < dsg->length; j += dev->dev_attrib.block_size) {
+
+			if (offset >= psg->length) {
+				kunmap_atomic(paddr);
+				psg = sg_next(psg);
+				paddr = kmap_atomic(sg_page(psg)) + psg->offset;
+				offset = 0;
+			}
+
+			sdt = paddr + offset;
+			sdt->guard_tag = cpu_to_be16(crc_t10dif(daddr + j,
+						dev->dev_attrib.block_size));
+			if (dev->dev_attrib.pi_prot_type == TARGET_DIF_TYPE1_PROT)
+				sdt->ref_tag = cpu_to_be32(sector & 0xffffffff);
+			sdt->app_tag = 0;
+
+			pr_debug("DIF WRITE INSERT sector: %llu guard_tag: 0x%04x"
+				 " app_tag: 0x%04x ref_tag: %u\n",
+				 (unsigned long long)sector, sdt->guard_tag,
+				 sdt->app_tag, be32_to_cpu(sdt->ref_tag));
+
+			sector++;
+			offset += sizeof(struct se_dif_v1_tuple);
+		}
+
+		kunmap_atomic(paddr);
+		kunmap_atomic(daddr);
+	}
+}
+
 static sense_reason_t
 sbc_dif_v1_verify(struct se_device *dev, struct se_dif_v1_tuple *sdt,
 		  const void *p, sector_t sector, unsigned int ei_lba)
diff --git a/include/target/target_core_backend.h b/include/target/target_core_backend.h
index 7020e33..8e6eac6 100644
--- a/include/target/target_core_backend.h
+++ b/include/target/target_core_backend.h
@@ -73,6 +73,7 @@ sense_reason_t sbc_execute_unmap(struct se_cmd *cmd,
 	sense_reason_t (*do_unmap_fn)(struct se_cmd *cmd, void *priv,
 				      sector_t lba, sector_t nolb),
 	void *priv);
+void	sbc_dif_write_insert(struct se_cmd *);
 sense_reason_t	sbc_dif_verify_write(struct se_cmd *, sector_t, unsigned int,
 				     unsigned int, struct scatterlist *, int);
 sense_reason_t	sbc_dif_verify_read(struct se_cmd *, sector_t, unsigned int,
-- 
1.7.10.4

[PATCH 7/9] target: Enable WRITE_INSERT emulation in target_execute_cmd

[Nicholas A. Bellinger]

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

This patch enables WRITE_INSERT emulation in target_execute_cmd()
in order to locally generate DIF PI before submitting the WRITE
to the underlying backend device.

This is required for fabric drivers that currently don't support
DIF over-the-wire, in order to inact with backend devices that
have hardware (IBLOCK) or software (FILEIO + RAMDISK) support
for handling T10 PI.

Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Or Gerlitz <ogerlitz@mellanox.com>
Cc: Quinn Tran <quinn.tran@qlogic.com>
Cc: Giridhar Malavali <giridhar.malavali@qlogic.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_transport.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 9c820ba..530a9e8 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1767,6 +1767,15 @@ void target_execute_cmd(struct se_cmd *cmd)
 	cmd->t_state = TRANSPORT_PROCESSING;
 	cmd->transport_state |= CMD_T_ACTIVE|CMD_T_BUSY|CMD_T_SENT;
 	spin_unlock_irq(&cmd->t_state_lock);
+	/*
+	 * Perform WRITE_INSERT of PI using software emulation when backend
+	 * device has PI enabled, if the transport has not already generated
+	 * PI using hardware WRITE_INSERT offload.
+	 */
+	if (cmd->prot_op == TARGET_PROT_DOUT_INSERT) {
+		if (!(cmd->se_sess->sup_prot_ops & TARGET_PROT_DOUT_INSERT))
+			sbc_dif_write_insert(cmd);
+	}
 
 	if (target_handle_task_attr(cmd)) {
 		spin_lock_irq(&cmd->t_state_lock);
-- 
1.7.10.4

[PATCH 8/9] target/sbc: Add sbc_dif_read_strip software emulation

[Nicholas A. Bellinger]

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

Split up __sbc_dif_verify_read() so that VERIFY READ emulation can
perform target-core specific READ_STRIP, seperate from the existing
FILEIO/RAMDISK backend emulation code.

Also add sbc_dif_read_strip() in order to determine number of sectors
using cmd->prot_length, and skip the extra sbc_dif_copy_prot().

Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Or Gerlitz <ogerlitz@mellanox.com>
Cc: Quinn Tran <quinn.tran@qlogic.com>
Cc: Giridhar Malavali <giridhar.malavali@qlogic.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_sbc.c     |   31 +++++++++++++++++++++++++++----
 include/target/target_core_backend.h |    1 +
 2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 97b207d..55c3526 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -1271,9 +1271,9 @@ sbc_dif_verify_write(struct se_cmd *cmd, sector_t start, unsigned int sectors,
 }
 EXPORT_SYMBOL(sbc_dif_verify_write);
 
-sense_reason_t
-sbc_dif_verify_read(struct se_cmd *cmd, sector_t start, unsigned int sectors,
-		    unsigned int ei_lba, struct scatterlist *sg, int sg_off)
+static sense_reason_t
+__sbc_dif_verify_read(struct se_cmd *cmd, sector_t start, unsigned int sectors,
+		      unsigned int ei_lba, struct scatterlist *sg, int sg_off)
 {
 	struct se_device *dev = cmd->se_dev;
 	struct se_dif_v1_tuple *sdt;
@@ -1326,8 +1326,31 @@ sbc_dif_verify_read(struct se_cmd *cmd, sector_t start, unsigned int sectors,
 		kunmap_atomic(paddr);
 		kunmap_atomic(daddr);
 	}
-	sbc_dif_copy_prot(cmd, sectors, true, sg, sg_off);
 
 	return 0;
 }
+
+sense_reason_t
+sbc_dif_read_strip(struct se_cmd *cmd)
+{
+	struct se_device *dev = cmd->se_dev;
+	u32 sectors = cmd->prot_length / dev->prot_length;
+
+	return __sbc_dif_verify_read(cmd, cmd->t_task_lba, sectors, 0,
+				     cmd->t_prot_sg, 0);
+}
+
+sense_reason_t
+sbc_dif_verify_read(struct se_cmd *cmd, sector_t start, unsigned int sectors,
+		    unsigned int ei_lba, struct scatterlist *sg, int sg_off)
+{
+	sense_reason_t rc;
+
+	rc = __sbc_dif_verify_read(cmd, start, sectors, ei_lba, sg, sg_off);
+	if (rc)
+		return rc;
+
+	sbc_dif_copy_prot(cmd, sectors, true, sg, sg_off);
+	return 0;
+}
 EXPORT_SYMBOL(sbc_dif_verify_read);
diff --git a/include/target/target_core_backend.h b/include/target/target_core_backend.h
index 8e6eac6..68e2820 100644
--- a/include/target/target_core_backend.h
+++ b/include/target/target_core_backend.h
@@ -78,6 +78,7 @@ sense_reason_t	sbc_dif_verify_write(struct se_cmd *, sector_t, unsigned int,
 				     unsigned int, struct scatterlist *, int);
 sense_reason_t	sbc_dif_verify_read(struct se_cmd *, sector_t, unsigned int,
 				    unsigned int, struct scatterlist *, int);
+sense_reason_t	sbc_dif_read_strip(struct se_cmd *);
 
 void	transport_set_vpd_proto_id(struct t10_vpd *, unsigned char *);
 int	transport_set_vpd_assoc(struct t10_vpd *, unsigned char *);
-- 
1.7.10.4

[PATCH 9/9] target: Enable READ_STRIP emulation in target_complete_ok_work

[Nicholas A. Bellinger]

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

This patch enables the use of READ_STRIP software emulation in
target_complete_ok_work() code for I/O READs.

This is useful when the fabric does not support READ_STRIP hardware
offload, but would still like to interact with backend device
that have T10 PI enabled.

Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Or Gerlitz <ogerlitz@mellanox.com>
Cc: Quinn Tran <quinn.tran@qlogic.com>
Cc: Giridhar Malavali <giridhar.malavali@qlogic.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_transport.c |   33 ++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 530a9e8..a184103 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1905,6 +1905,24 @@ static void transport_handle_queue_full(
 	schedule_work(&cmd->se_dev->qf_work_queue);
 }
 
+static bool target_check_read_strip(struct se_cmd *cmd)
+{
+	sense_reason_t rc;
+
+	if (cmd->prot_op != TARGET_PROT_DIN_STRIP)
+		return false;
+
+	if (!(cmd->se_sess->sup_prot_ops & TARGET_PROT_DIN_STRIP)) {
+		rc = sbc_dif_read_strip(cmd);
+		if (rc) {
+			cmd->pi_err = rc;
+			return true;
+		}
+	}
+
+	return false;
+}
+
 static void target_complete_ok_work(struct work_struct *work)
 {
 	struct se_cmd *cmd = container_of(work, struct se_cmd, work);
@@ -1969,6 +1987,21 @@ static void target_complete_ok_work(struct work_struct *work)
 					cmd->data_length;
 		}
 		spin_unlock(&cmd->se_lun->lun_sep_lock);
+		/*
+		 * Perform READ_STRIP of PI using software emulation when
+		 * backend had PI enabled, if the transport will not be
+		 * performing hardware READ_STRIP offload.
+		 */
+		if (target_check_read_strip(cmd)) {
+			ret = transport_send_check_condition_and_sense(cmd,
+						cmd->pi_err, 0);
+			if (ret == -EAGAIN || ret == -ENOMEM)
+				goto queue_full;
+
+			transport_lun_remove_cmd(cmd);
+			transport_cmd_check_stop_to_fabric(cmd);
+			return;
+		}
 
 		trace_target_cmd_complete(cmd);
 		ret = cmd->se_tfo->queue_data_in(cmd);
-- 
1.7.10.4

Re: [PATCH 1/9] target/iblock: Fix double bioset_integrity_free bug

[Sagi Grimberg]

On 4/3/2014 12:35 PM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> This patch fixes a double free bug during IBLOCK backend shutdown
> where bioset_integrity_free() was incorrectly called ahead of
> bioset_free(), who is already making the same call directly.
>
> This bug was introduced with commit ecebbf6cc, and will end up
> triggering a general protection fault in iblock_free_device()
>
> Cc: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: Sagi Grimberg <sagig@mellanox.com>
> Cc: Or Gerlitz <ogerlitz@mellanox.com>
> Cc: Quinn Tran <quinn.tran@qlogic.com>
> Cc: Giridhar Malavali <giridhar.malavali@qlogic.com>
> Cc: <stable@vger.kernel.org> #3.14+
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>   drivers/target/target_core_iblock.c |    5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
> index 554d4f7..9e0232c 100644
> --- a/drivers/target/target_core_iblock.c
> +++ b/drivers/target/target_core_iblock.c
> @@ -203,10 +203,9 @@ static void iblock_free_device(struct se_device *dev)
>   
>   	if (ib_dev->ibd_bd != NULL)
>   		blkdev_put(ib_dev->ibd_bd, FMODE_WRITE|FMODE_READ|FMODE_EXCL);
> -	if (ib_dev->ibd_bio_set != NULL) {
> -		bioset_integrity_free(ib_dev->ibd_bio_set);
> +	if (ib_dev->ibd_bio_set != NULL)
>   		bioset_free(ib_dev->ibd_bio_set);
> -	}
> +
>   	kfree(ib_dev);
>   }
>   

Looks good.

Reviewed-by: Sagi Grimberg <sagig@mellanox.com>

Re: [PATCH 2/9] target: Pass in transport supported PI at session initialization

[Sagi Grimberg]

On 4/3/2014 12:35 PM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> In order to support local WRITE_INSERT + READ_STRIP operations for
> non PI enabled fabrics, the fabric driver needs to be able signal
> what protection offload operations are supported.
>
> This is done at session initialization time so the modes can be
> signaled by individual se_wwn + se_portal_group endpoints, as well
> as optionally across different transports on the same endpoint.
>
> For iser-target, set TARGET_PROT_ALL if the underlying ib_device
> has already signaled PI offload support, and allow this to be
> exposed via a new iscsit_transport->iscsit_get_sup_prot_ops()
> callback.
>
> For loopback, set TARGET_PROT_ALL to signal SCSI initiator mode
> operation.
>
> For all other drivers, set TARGET_PROT_NORMAL to disable fabric
> level PI.
>
> Cc: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: Sagi Grimberg <sagig@mellanox.com>
> Cc: Or Gerlitz <ogerlitz@mellanox.com>
> Cc: Quinn Tran <quinn.tran@qlogic.com>
> Cc: Giridhar Malavali <giridhar.malavali@qlogic.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>   drivers/infiniband/ulp/isert/ib_isert.c   |   13 +++++++++++++
>   drivers/infiniband/ulp/srpt/ib_srpt.c     |    2 +-
>   drivers/scsi/qla2xxx/tcm_qla2xxx.c        |    2 +-
>   drivers/target/iscsi/iscsi_target.c       |    6 ++++++
>   drivers/target/iscsi/iscsi_target_login.c |    4 +++-
>   drivers/target/loopback/tcm_loop.c        |    2 +-
>   drivers/target/sbp/sbp_target.c           |    2 +-
>   drivers/target/target_core_transport.c    |    8 +++++---
>   drivers/target/tcm_fc/tfc_sess.c          |    3 ++-
>   drivers/usb/gadget/tcm_usb_gadget.c       |    2 +-
>   drivers/vhost/scsi.c                      |    3 ++-
>   include/target/iscsi/iscsi_transport.h    |    1 +
>   include/target/target_core_base.h         |   19 ++++++++++++-------
>   include/target/target_core_fabric.h       |    5 +++--
>   14 files changed, 52 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
> index f5cc4af..c98fdb1 100644
> --- a/drivers/infiniband/ulp/isert/ib_isert.c
> +++ b/drivers/infiniband/ulp/isert/ib_isert.c
> @@ -2196,6 +2196,18 @@ isert_aborted_task(struct iscsi_conn *conn, struct iscsi_cmd *cmd)
>   	device->unreg_rdma_mem(isert_cmd, isert_conn);
>   }
>   
> +static enum target_prot_op
> +isert_get_sup_prot_ops(struct iscsi_conn *conn)
> +{
> +	struct isert_conn *isert_conn = (struct isert_conn *)conn->context;
> +	struct isert_device *device = isert_conn->conn_device;
> +
> +	if (device->pi_capable)
> +		return TARGET_PROT_ALL;
> +
> +	return TARGET_PROT_NORMAL;
> +}
> +
>   static int
>   isert_put_nopin(struct iscsi_cmd *cmd, struct iscsi_conn *conn,
>   		bool nopout_response)
> @@ -3252,6 +3264,7 @@ static struct iscsit_transport iser_target_transport = {
>   	.iscsit_queue_data_in	= isert_put_datain,
>   	.iscsit_queue_status	= isert_put_response,
>   	.iscsit_aborted_task	= isert_aborted_task,
> +	.iscsit_get_sup_prot_ops = isert_get_sup_prot_ops,
>   };
>   
>   static int __init isert_init(void)
> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> index f03aafd..bcfb398 100644
> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> @@ -2580,7 +2580,7 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
>   		goto destroy_ib;
>   	}
>   
> -	ch->sess = transport_init_session();
> +	ch->sess = transport_init_session(TARGET_PROT_NORMAL);
>   	if (IS_ERR(ch->sess)) {
>   		rej->reason = __constant_cpu_to_be32(
>   				SRP_LOGIN_REJ_INSUFFICIENT_RESOURCES);
> diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> index b23a0ff..68fb66f 100644
> --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> @@ -1482,7 +1482,7 @@ static int tcm_qla2xxx_check_initiator_node_acl(
>   	}
>   	se_tpg = &tpg->se_tpg;
>   
> -	se_sess = transport_init_session();
> +	se_sess = transport_init_session(TARGET_PROT_NORMAL);
>   	if (IS_ERR(se_sess)) {
>   		pr_err("Unable to initialize struct se_session\n");
>   		return PTR_ERR(se_sess);
> diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
> index 96aee43..78cab13 100644
> --- a/drivers/target/iscsi/iscsi_target.c
> +++ b/drivers/target/iscsi/iscsi_target.c
> @@ -511,6 +511,11 @@ static void iscsit_aborted_task(struct iscsi_conn *conn, struct iscsi_cmd *cmd)
>   	__iscsit_free_cmd(cmd, scsi_cmd, true);
>   }
>   
> +static enum target_prot_op iscsit_get_sup_prot_ops(struct iscsi_conn *conn)

I think it is more correct that this routine will return a bitmap (u32) 
and not the enumeration itself.

> +{
> +	return TARGET_PROT_NORMAL;
> +}
> +
>   static struct iscsit_transport iscsi_target_transport = {
>   	.name			= "iSCSI/TCP",
>   	.transport_type		= ISCSI_TCP,
> @@ -526,6 +531,7 @@ static struct iscsit_transport iscsi_target_transport = {
>   	.iscsit_queue_data_in	= iscsit_queue_rsp,
>   	.iscsit_queue_status	= iscsit_queue_rsp,
>   	.iscsit_aborted_task	= iscsit_aborted_task,
> +	.iscsit_get_sup_prot_ops = iscsit_get_sup_prot_ops,
>   };
>   
>   static int __init iscsi_target_init_module(void)
> diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
> index e29279e..8739b98 100644
> --- a/drivers/target/iscsi/iscsi_target_login.c
> +++ b/drivers/target/iscsi/iscsi_target_login.c
> @@ -259,6 +259,7 @@ static int iscsi_login_zero_tsih_s1(
>   {
>   	struct iscsi_session *sess = NULL;
>   	struct iscsi_login_req *pdu = (struct iscsi_login_req *)buf;
> +	enum target_prot_op sup_pro_ops;
>   	int ret;
>   
>   	sess = kzalloc(sizeof(struct iscsi_session), GFP_KERNEL);
> @@ -320,8 +321,9 @@ static int iscsi_login_zero_tsih_s1(
>   		kfree(sess);
>   		return -ENOMEM;
>   	}
> +	sup_pro_ops = conn->conn_transport->iscsit_get_sup_prot_ops(conn);
>   
> -	sess->se_sess = transport_init_session();
> +	sess->se_sess = transport_init_session(sup_pro_ops);
>   	if (IS_ERR(sess->se_sess)) {
>   		iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
>   				ISCSI_LOGIN_STATUS_NO_RESOURCES);
> diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
> index bdc1ad8..c886ad1 100644
> --- a/drivers/target/loopback/tcm_loop.c
> +++ b/drivers/target/loopback/tcm_loop.c
> @@ -1018,7 +1018,7 @@ static int tcm_loop_make_nexus(
>   	/*
>   	 * Initialize the struct se_session pointer
>   	 */
> -	tl_nexus->se_sess = transport_init_session();
> +	tl_nexus->se_sess = transport_init_session(TARGET_PROT_ALL);
>   	if (IS_ERR(tl_nexus->se_sess)) {
>   		ret = PTR_ERR(tl_nexus->se_sess);
>   		goto out;
> diff --git a/drivers/target/sbp/sbp_target.c b/drivers/target/sbp/sbp_target.c
> index ad04ea9..e7e9372 100644
> --- a/drivers/target/sbp/sbp_target.c
> +++ b/drivers/target/sbp/sbp_target.c
> @@ -210,7 +210,7 @@ static struct sbp_session *sbp_session_create(
>   		return ERR_PTR(-ENOMEM);
>   	}
>   
> -	sess->se_sess = transport_init_session();
> +	sess->se_sess = transport_init_session(TARGET_PROT_NORMAL);
>   	if (IS_ERR(sess->se_sess)) {
>   		pr_err("failed to init se_session\n");
>   
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 9393544..9c820ba 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -235,7 +235,7 @@ void transport_subsystem_check_init(void)
>   	sub_api_initialized = 1;
>   }
>   
> -struct se_session *transport_init_session(void)
> +struct se_session *transport_init_session(enum target_prot_op sup_prot_ops)
>   {
>   	struct se_session *se_sess;
>   
> @@ -251,6 +251,7 @@ struct se_session *transport_init_session(void)
>   	INIT_LIST_HEAD(&se_sess->sess_wait_list);
>   	spin_lock_init(&se_sess->sess_cmd_lock);
>   	kref_init(&se_sess->sess_kref);
> +	se_sess->sup_prot_ops = sup_prot_ops;
>   
>   	return se_sess;
>   }
> @@ -288,12 +289,13 @@ int transport_alloc_session_tags(struct se_session *se_sess,
>   EXPORT_SYMBOL(transport_alloc_session_tags);
>   
>   struct se_session *transport_init_session_tags(unsigned int tag_num,
> -					       unsigned int tag_size)
> +					       unsigned int tag_size,
> +					       enum target_prot_op sup_prot_ops)
>   {
>   	struct se_session *se_sess;
>   	int rc;
>   
> -	se_sess = transport_init_session();
> +	se_sess = transport_init_session(sup_prot_ops);
>   	if (IS_ERR(se_sess))
>   		return se_sess;
>   
> diff --git a/drivers/target/tcm_fc/tfc_sess.c b/drivers/target/tcm_fc/tfc_sess.c
> index ae52c08..0475142 100644
> --- a/drivers/target/tcm_fc/tfc_sess.c
> +++ b/drivers/target/tcm_fc/tfc_sess.c
> @@ -211,7 +211,8 @@ static struct ft_sess *ft_sess_create(struct ft_tport *tport, u32 port_id,
>   		return NULL;
>   
>   	sess->se_sess = transport_init_session_tags(TCM_FC_DEFAULT_TAGS,
> -						    sizeof(struct ft_cmd));
> +						    sizeof(struct ft_cmd),
> +						    TARGET_PROT_NORMAL);
>   	if (IS_ERR(sess->se_sess)) {
>   		kfree(sess);
>   		return NULL;
> diff --git a/drivers/usb/gadget/tcm_usb_gadget.c b/drivers/usb/gadget/tcm_usb_gadget.c
> index f9afa4a..f34b6df 100644
> --- a/drivers/usb/gadget/tcm_usb_gadget.c
> +++ b/drivers/usb/gadget/tcm_usb_gadget.c
> @@ -1731,7 +1731,7 @@ static int tcm_usbg_make_nexus(struct usbg_tpg *tpg, char *name)
>   		pr_err("Unable to allocate struct tcm_vhost_nexus\n");
>   		goto err_unlock;
>   	}
> -	tv_nexus->tvn_se_sess = transport_init_session();
> +	tv_nexus->tvn_se_sess = transport_init_session(TARGET_PROT_NORMAL);
>   	if (IS_ERR(tv_nexus->tvn_se_sess))
>   		goto err_free;
>   
> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index 4a47335..cf50ce9 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c
> @@ -1745,7 +1745,8 @@ static int tcm_vhost_make_nexus(struct tcm_vhost_tpg *tpg,
>   	 */
>   	tv_nexus->tvn_se_sess = transport_init_session_tags(
>   					TCM_VHOST_DEFAULT_TAGS,
> -					sizeof(struct tcm_vhost_cmd));
> +					sizeof(struct tcm_vhost_cmd),
> +					TARGET_PROT_NORMAL);
>   	if (IS_ERR(tv_nexus->tvn_se_sess)) {
>   		mutex_unlock(&tpg->tv_tpg_mutex);
>   		kfree(tv_nexus);
> diff --git a/include/target/iscsi/iscsi_transport.h b/include/target/iscsi/iscsi_transport.h
> index 8d19339..33b487b 100644
> --- a/include/target/iscsi/iscsi_transport.h
> +++ b/include/target/iscsi/iscsi_transport.h
> @@ -22,6 +22,7 @@ struct iscsit_transport {
>   	int (*iscsit_queue_data_in)(struct iscsi_conn *, struct iscsi_cmd *);
>   	int (*iscsit_queue_status)(struct iscsi_conn *, struct iscsi_cmd *);
>   	void (*iscsit_aborted_task)(struct iscsi_conn *, struct iscsi_cmd *);
> +	enum target_prot_op (*iscsit_get_sup_prot_ops)(struct iscsi_conn *);
>   };
>   
>   static inline void *iscsit_priv_cmd(struct iscsi_cmd *cmd)
> diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
> index ec3e3a3..9ec9864 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -442,15 +442,19 @@ struct se_tmr_req {
>   };
>   
>   enum target_prot_op {
> -	TARGET_PROT_NORMAL = 0,
> -	TARGET_PROT_DIN_INSERT,
> -	TARGET_PROT_DOUT_INSERT,
> -	TARGET_PROT_DIN_STRIP,
> -	TARGET_PROT_DOUT_STRIP,
> -	TARGET_PROT_DIN_PASS,
> -	TARGET_PROT_DOUT_PASS,
> +	TARGET_PROT_NORMAL	= 0,
> +	TARGET_PROT_DIN_INSERT	= (1 << 0),
> +	TARGET_PROT_DOUT_INSERT	= (1 << 1),
> +	TARGET_PROT_DIN_STRIP	= (1 << 2),
> +	TARGET_PROT_DOUT_STRIP	= (1 << 3),
> +	TARGET_PROT_DIN_PASS	= (1 << 4),
> +	TARGET_PROT_DOUT_PASS	= (1 << 5),
>   };
>   
> +#define TARGET_PROT_ALL	TARGET_PROT_DIN_INSERT | TARGET_PROT_DOUT_INSERT | \
> +			TARGET_PROT_DIN_STRIP | TARGET_PROT_DOUT_STRIP | \
> +			TARGET_PROT_DIN_PASS | TARGET_PROT_DOUT_PASS
> +

We only look at the supported protection operations, what about 
supported types?
Do we assume that all types are supported if sup_prot_ops is not empty?
This is fine for the current state, but I think we will require to 
address that.

>   enum target_prot_type {
>   	TARGET_DIF_TYPE0_PROT,
>   	TARGET_DIF_TYPE1_PROT,
> @@ -605,6 +609,7 @@ struct se_node_acl {
>   struct se_session {
>   	unsigned		sess_tearing_down:1;
>   	u64			sess_bin_isid;
> +	enum target_prot_op	sup_prot_ops;
>   	struct se_node_acl	*se_node_acl;
>   	struct se_portal_group *se_tpg;
>   	void			*fabric_sess_ptr;
> diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
> index 1d10436..22a4e98 100644
> --- a/include/target/target_core_fabric.h
> +++ b/include/target/target_core_fabric.h
> @@ -84,10 +84,11 @@ struct target_core_fabric_ops {
>   	void (*fabric_drop_nodeacl)(struct se_node_acl *);
>   };
>   
> -struct se_session *transport_init_session(void);
> +struct se_session *transport_init_session(enum target_prot_op);
>   int transport_alloc_session_tags(struct se_session *, unsigned int,
>   		unsigned int);
> -struct se_session *transport_init_session_tags(unsigned int, unsigned int);
> +struct se_session *transport_init_session_tags(unsigned int, unsigned int,
> +		enum target_prot_op);
>   void	__transport_register_session(struct se_portal_group *,
>   		struct se_node_acl *, struct se_session *, void *);
>   void	transport_register_session(struct se_portal_group *,

Re: [PATCH 3/9] target/spc: Only expose PI inquiry bits when supported by fabric

[Sagi Grimberg]

On 4/3/2014 12:35 PM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> Only expose standard INQUIRY PROTECT=1 and EVPD=0x86 TYPE1/TYPE3
> PI control bits if the session + fabric support DIX PASS operations.
>
> Cc: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: Sagi Grimberg <sagig@mellanox.com>
> Cc: Or Gerlitz <ogerlitz@mellanox.com>
> Cc: Quinn Tran <quinn.tran@qlogic.com>
> Cc: Giridhar Malavali <giridhar.malavali@qlogic.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>   drivers/target/target_core_spc.c |   21 ++++++++++++++-------
>   1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
> index 3bebc71..d4c6a31 100644
> --- a/drivers/target/target_core_spc.c
> +++ b/drivers/target/target_core_spc.c
> @@ -71,6 +71,7 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned char *buf)
>   {
>   	struct se_lun *lun = cmd->se_lun;
>   	struct se_device *dev = cmd->se_dev;
> +	struct se_session *sess = cmd->se_sess;
>   
>   	/* Set RMB (removable media) for tape devices */
>   	if (dev->transport->get_device_type(dev) == TYPE_TAPE)
> @@ -101,10 +102,13 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned char *buf)
>   	if (dev->dev_attrib.emulate_3pc)
>   		buf[5] |= 0x8;
>   	/*
> -	 * Set Protection (PROTECT) bit when DIF has been enabled.
> +	 * Set Protection (PROTECT) bit when DIF has been enabled on the
> +	 * device, and the transport supports VERIFY + PASS.
>   	 */
> -	if (dev->dev_attrib.pi_prot_type)
> -		buf[5] |= 0x1;
> +	if (sess->sup_prot_ops & (TARGET_PROT_DIN_PASS | TARGET_PROT_DOUT_PASS)) {
> +		if (dev->dev_attrib.pi_prot_type)
> +			buf[5] |= 0x1;
> +	}
>   
>   	buf[7] = 0x2; /* CmdQue=1 */
>   
> @@ -473,16 +477,19 @@ static sense_reason_t
>   spc_emulate_evpd_86(struct se_cmd *cmd, unsigned char *buf)
>   {
>   	struct se_device *dev = cmd->se_dev;
> +	struct se_session *sess = cmd->se_sess;
>   
>   	buf[3] = 0x3c;
>   	/*
>   	 * Set GRD_CHK + REF_CHK for TYPE1 protection, or GRD_CHK
>   	 * only for TYPE3 protection.
>   	 */
> -	if (dev->dev_attrib.pi_prot_type == TARGET_DIF_TYPE1_PROT)
> -		buf[4] = 0x5;
> -	else if (dev->dev_attrib.pi_prot_type == TARGET_DIF_TYPE3_PROT)
> -		buf[4] = 0x4;
> +	if (sess->sup_prot_ops & (TARGET_PROT_DIN_PASS | TARGET_PROT_DOUT_PASS)) {
> +		if (dev->dev_attrib.pi_prot_type == TARGET_DIF_TYPE1_PROT)
> +			buf[4] = 0x5;
> +		else if (dev->dev_attrib.pi_prot_type == TARGET_DIF_TYPE3_PROT)
> +			buf[4] = 0x4;
> +	}
>   
>   	/* Set HEADSUP, ORDSUP, SIMPSUP */
>   	buf[5] = 0x07;
Looks good to me.

Reviewed-by: Sagi Grimberg <sagig@mellanox.com>

Re: [PATCH 4/9] target/spc: Only expose PI mode page bits when supported by fabric

[Sagi Grimberg]

On 4/3/2014 12:35 PM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> Only expose the control modepage bit for Application Tag Owner (ATO)
> if the session + fabric support DIX PASS operations.
>
> Cc: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: Sagi Grimberg <sagig@mellanox.com>
> Cc: Or Gerlitz <ogerlitz@mellanox.com>
> Cc: Quinn Tran <quinn.tran@qlogic.com>
> Cc: Giridhar Malavali <giridhar.malavali@qlogic.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>   drivers/target/target_core_spc.c |   28 +++++++++++++++++-----------
>   1 file changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
> index d4c6a31..8653666 100644
> --- a/drivers/target/target_core_spc.c
> +++ b/drivers/target/target_core_spc.c
> @@ -769,7 +769,7 @@ out:
>   	return ret;
>   }
>   
> -static int spc_modesense_rwrecovery(struct se_device *dev, u8 pc, u8 *p)
> +static int spc_modesense_rwrecovery(struct se_cmd *cmd, u8 pc, u8 *p)
>   {
>   	p[0] = 0x01;
>   	p[1] = 0x0a;
> @@ -782,8 +782,11 @@ out:
>   	return 12;
>   }
>   
> -static int spc_modesense_control(struct se_device *dev, u8 pc, u8 *p)
> +static int spc_modesense_control(struct se_cmd *cmd, u8 pc, u8 *p)
>   {
> +	struct se_device *dev = cmd->se_dev;
> +	struct se_session *sess = cmd->se_sess;
> +
>   	p[0] = 0x0a;
>   	p[1] = 0x0a;
>   
> @@ -875,8 +878,10 @@ static int spc_modesense_control(struct se_device *dev, u8 pc, u8 *p)
>   	 * type, shall not modify the contents of the LOGICAL BLOCK REFERENCE
>   	 * TAG field.
>   	 */
> -	if (dev->dev_attrib.pi_prot_type)
> -		p[5] |= 0x80;
> +	if (sess->sup_prot_ops & (TARGET_PROT_DIN_PASS | TARGET_PROT_DOUT_PASS)) {
> +		if (dev->dev_attrib.pi_prot_type)
> +			p[5] |= 0x80;
> +	}
>   
>   	p[8] = 0xff;
>   	p[9] = 0xff;
> @@ -886,8 +891,10 @@ out:
>   	return 12;
>   }
>   
> -static int spc_modesense_caching(struct se_device *dev, u8 pc, u8 *p)
> +static int spc_modesense_caching(struct se_cmd *cmd, u8 pc, u8 *p)
>   {
> +	struct se_device *dev = cmd->se_dev;
> +
>   	p[0] = 0x08;
>   	p[1] = 0x12;
>   
> @@ -903,7 +910,7 @@ out:
>   	return 20;
>   }
>   
> -static int spc_modesense_informational_exceptions(struct se_device *dev, u8 pc, unsigned char *p)
> +static int spc_modesense_informational_exceptions(struct se_cmd *cmd, u8 pc, unsigned char *p)
>   {
>   	p[0] = 0x1c;
>   	p[1] = 0x0a;
> @@ -919,7 +926,7 @@ out:
>   static struct {
>   	uint8_t		page;
>   	uint8_t		subpage;
> -	int		(*emulate)(struct se_device *, u8, unsigned char *);
> +	int		(*emulate)(struct se_cmd *, u8, unsigned char *);
>   } modesense_handlers[] = {
>   	{ .page = 0x01, .subpage = 0x00, .emulate = spc_modesense_rwrecovery },
>   	{ .page = 0x08, .subpage = 0x00, .emulate = spc_modesense_caching },
> @@ -1057,7 +1064,7 @@ static sense_reason_t spc_emulate_modesense(struct se_cmd *cmd)
>   			 * the only two possibilities).
>   			 */
>   			if ((modesense_handlers[i].subpage & ~subpage) == 0) {
> -				ret = modesense_handlers[i].emulate(dev, pc, &buf[length]);
> +				ret = modesense_handlers[i].emulate(cmd, pc, &buf[length]);
>   				if (!ten && length + ret >= 255)
>   					break;
>   				length += ret;
> @@ -1070,7 +1077,7 @@ static sense_reason_t spc_emulate_modesense(struct se_cmd *cmd)
>   	for (i = 0; i < ARRAY_SIZE(modesense_handlers); ++i)
>   		if (modesense_handlers[i].page == page &&
>   		    modesense_handlers[i].subpage == subpage) {
> -			length += modesense_handlers[i].emulate(dev, pc, &buf[length]);
> +			length += modesense_handlers[i].emulate(cmd, pc, &buf[length]);
>   			goto set_length;
>   		}
>   
> @@ -1102,7 +1109,6 @@ set_length:
>   
>   static sense_reason_t spc_emulate_modeselect(struct se_cmd *cmd)
>   {
> -	struct se_device *dev = cmd->se_dev;
>   	char *cdb = cmd->t_task_cdb;
>   	bool ten = cdb[0] == MODE_SELECT_10;
>   	int off = ten ? 8 : 4;
> @@ -1138,7 +1144,7 @@ static sense_reason_t spc_emulate_modeselect(struct se_cmd *cmd)
>   		if (modesense_handlers[i].page == page &&
>   		    modesense_handlers[i].subpage == subpage) {
>   			memset(tbuf, 0, SE_MODE_PAGE_BUF);
> -			length = modesense_handlers[i].emulate(dev, 0, tbuf);
> +			length = modesense_handlers[i].emulate(cmd, 0, tbuf);
>   			goto check_contents;
>   		}
>   
Looks good to me.

Reviewed-by: Sagi Grimberg <sagig@mellanox.com>

Re: [PATCH 5/9] target/sbc: Only expose PI read_cap16 bits when supported by fabric

[Sagi Grimberg]

On 4/3/2014 12:35 PM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> Only expose the PI protection type bits in READ_CAPACITY_16
> if the session + fabric support DIX PASS operations.
>
> Cc: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: Sagi Grimberg <sagig@mellanox.com>
> Cc: Or Gerlitz <ogerlitz@mellanox.com>
> Cc: Quinn Tran <quinn.tran@qlogic.com>
> Cc: Giridhar Malavali <giridhar.malavali@qlogic.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>   drivers/target/target_core_sbc.c |    7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> index ec204f7..f2d73dd 100644
> --- a/drivers/target/target_core_sbc.c
> +++ b/drivers/target/target_core_sbc.c
> @@ -89,6 +89,7 @@ static sense_reason_t
>   sbc_emulate_readcapacity_16(struct se_cmd *cmd)
>   {
>   	struct se_device *dev = cmd->se_dev;
> +	struct se_session *sess = cmd->se_sess;
>   	unsigned char *rbuf;
>   	unsigned char buf[32];
>   	unsigned long long blocks = dev->transport->get_blocks(dev);
> @@ -109,8 +110,10 @@ sbc_emulate_readcapacity_16(struct se_cmd *cmd)
>   	/*
>   	 * Set P_TYPE and PROT_EN bits for DIF support
>   	 */
> -	if (dev->dev_attrib.pi_prot_type)
> -		buf[12] = (dev->dev_attrib.pi_prot_type - 1) << 1 | 0x1;
> +	if (sess->sup_prot_ops & (TARGET_PROT_DIN_PASS | TARGET_PROT_DOUT_PASS)) {
> +		if (dev->dev_attrib.pi_prot_type)
> +			buf[12] = (dev->dev_attrib.pi_prot_type - 1) << 1 | 0x1;
> +	}
>   
>   	if (dev->transport->get_lbppbe)
>   		buf[13] = dev->transport->get_lbppbe(dev) & 0x0f;
Looks good to me.

Reviewed-by: Sagi Grimberg <sagig@mellanox.com>

Re: [PATCH 6/9] target/sbc: Add sbc_dif_write_insert software emulation

[Sagi Grimberg]

On 4/3/2014 12:35 PM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> This patch adds WRITE_INSERT emulation within target-core
> using TYPE1 / TYPE3 PI modes.
>
> This is useful in order for existing legacy fabrics that do not
> support protection offloads to interact with backend devices that
> currently have T10 PI enabled.
>
> Cc: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: Sagi Grimberg <sagig@mellanox.com>
> Cc: Or Gerlitz <ogerlitz@mellanox.com>
> Cc: Quinn Tran <quinn.tran@qlogic.com>
> Cc: Giridhar Malavali <giridhar.malavali@qlogic.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>   drivers/target/target_core_sbc.c     |   44 ++++++++++++++++++++++++++++++++++
>   include/target/target_core_backend.h |    1 +
>   2 files changed, 45 insertions(+)
>
> diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> index f2d73dd..97b207d 100644
> --- a/drivers/target/target_core_sbc.c
> +++ b/drivers/target/target_core_sbc.c
> @@ -1096,6 +1096,50 @@ err:
>   }
>   EXPORT_SYMBOL(sbc_execute_unmap);
>   
> +void
> +sbc_dif_write_insert(struct se_cmd *cmd)

Better to call it sbc_dif_generate()

> +{
> +	struct se_device *dev = cmd->se_dev;
> +	struct se_dif_v1_tuple *sdt;
> +	struct scatterlist *dsg, *psg = cmd->t_prot_sg;
> +	sector_t sector = cmd->t_task_lba;
> +	void *daddr, *paddr;
> +	int i, j, offset = 0;
> +
> +	for_each_sg(cmd->t_data_sg, dsg, cmd->t_data_nents, i) {
> +		daddr = kmap_atomic(sg_page(dsg)) + dsg->offset;
> +		paddr = kmap_atomic(sg_page(psg)) + psg->offset;
> +
> +		for (j = 0; j < dsg->length; j += dev->dev_attrib.block_size) {
> +
> +			if (offset >= psg->length) {
> +				kunmap_atomic(paddr);
> +				psg = sg_next(psg);
> +				paddr = kmap_atomic(sg_page(psg)) + psg->offset;
> +				offset = 0;
> +			}
> +
> +			sdt = paddr + offset;
> +			sdt->guard_tag = cpu_to_be16(crc_t10dif(daddr + j,
> +						dev->dev_attrib.block_size));
> +			if (dev->dev_attrib.pi_prot_type == TARGET_DIF_TYPE1_PROT)
> +				sdt->ref_tag = cpu_to_be32(sector & 0xffffffff);
> +			sdt->app_tag = 0;
> +
> +			pr_debug("DIF WRITE INSERT sector: %llu guard_tag: 0x%04x"
> +				 " app_tag: 0x%04x ref_tag: %u\n",
> +				 (unsigned long long)sector, sdt->guard_tag,
> +				 sdt->app_tag, be32_to_cpu(sdt->ref_tag));
> +
> +			sector++;
> +			offset += sizeof(struct se_dif_v1_tuple);
> +		}
> +
> +		kunmap_atomic(paddr);
> +		kunmap_atomic(daddr);
> +	}
> +}

Seems like a substantial code duplication here, can't you reuse the code 
from verify?
Maybe use this function also from verify (call generate() and then do 
compare to t_prot_sg).

> +
>   static sense_reason_t
>   sbc_dif_v1_verify(struct se_device *dev, struct se_dif_v1_tuple *sdt,
>   		  const void *p, sector_t sector, unsigned int ei_lba)
> diff --git a/include/target/target_core_backend.h b/include/target/target_core_backend.h
> index 7020e33..8e6eac6 100644
> --- a/include/target/target_core_backend.h
> +++ b/include/target/target_core_backend.h
> @@ -73,6 +73,7 @@ sense_reason_t sbc_execute_unmap(struct se_cmd *cmd,
>   	sense_reason_t (*do_unmap_fn)(struct se_cmd *cmd, void *priv,
>   				      sector_t lba, sector_t nolb),
>   	void *priv);
> +void	sbc_dif_write_insert(struct se_cmd *);
>   sense_reason_t	sbc_dif_verify_write(struct se_cmd *, sector_t, unsigned int,
>   				     unsigned int, struct scatterlist *, int);
>   sense_reason_t	sbc_dif_verify_read(struct se_cmd *, sector_t, unsigned int,

Re: [PATCH 7/9] target: Enable WRITE_INSERT emulation in target_execute_cmd

[Sagi Grimberg]

On 4/3/2014 12:35 PM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> This patch enables WRITE_INSERT emulation in target_execute_cmd()
> in order to locally generate DIF PI before submitting the WRITE
> to the underlying backend device.
>
> This is required for fabric drivers that currently don't support
> DIF over-the-wire, in order to inact with backend devices that
> have hardware (IBLOCK) or software (FILEIO + RAMDISK) support
> for handling T10 PI.
>
> Cc: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: Sagi Grimberg <sagig@mellanox.com>
> Cc: Or Gerlitz <ogerlitz@mellanox.com>
> Cc: Quinn Tran <quinn.tran@qlogic.com>
> Cc: Giridhar Malavali <giridhar.malavali@qlogic.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>   drivers/target/target_core_transport.c |    9 +++++++++
>   1 file changed, 9 insertions(+)
>
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 9c820ba..530a9e8 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -1767,6 +1767,15 @@ void target_execute_cmd(struct se_cmd *cmd)
>   	cmd->t_state = TRANSPORT_PROCESSING;
>   	cmd->transport_state |= CMD_T_ACTIVE|CMD_T_BUSY|CMD_T_SENT;
>   	spin_unlock_irq(&cmd->t_state_lock);
> +	/*
> +	 * Perform WRITE_INSERT of PI using software emulation when backend
> +	 * device has PI enabled, if the transport has not already generated
> +	 * PI using hardware WRITE_INSERT offload.
> +	 */
> +	if (cmd->prot_op == TARGET_PROT_DOUT_INSERT) {
> +		if (!(cmd->se_sess->sup_prot_ops & TARGET_PROT_DOUT_INSERT))
> +			sbc_dif_write_insert(cmd);
> +	}
>   
>   	if (target_handle_task_attr(cmd)) {
>   		spin_lock_irq(&cmd->t_state_lock);
Looks good to me.

Reviewed-by: Sagi Grimberg <sagig@mellanox.com>

This is useless though without the code setting this prot_op in 
sbc_set_prot_op_checks()...

Re: [PATCH 8/9] target/sbc: Add sbc_dif_read_strip software emulation

[Sagi Grimberg]

On 4/3/2014 12:35 PM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> Split up __sbc_dif_verify_read() so that VERIFY READ emulation can
> perform target-core specific READ_STRIP, seperate from the existing
> FILEIO/RAMDISK backend emulation code.
>
> Also add sbc_dif_read_strip() in order to determine number of sectors
> using cmd->prot_length, and skip the extra sbc_dif_copy_prot().
>
> Cc: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: Sagi Grimberg <sagig@mellanox.com>
> Cc: Or Gerlitz <ogerlitz@mellanox.com>
> Cc: Quinn Tran <quinn.tran@qlogic.com>
> Cc: Giridhar Malavali <giridhar.malavali@qlogic.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>   drivers/target/target_core_sbc.c     |   31 +++++++++++++++++++++++++++----
>   include/target/target_core_backend.h |    1 +
>   2 files changed, 28 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> index 97b207d..55c3526 100644
> --- a/drivers/target/target_core_sbc.c
> +++ b/drivers/target/target_core_sbc.c
> @@ -1271,9 +1271,9 @@ sbc_dif_verify_write(struct se_cmd *cmd, sector_t start, unsigned int sectors,
>   }
>   EXPORT_SYMBOL(sbc_dif_verify_write);
>   
> -sense_reason_t
> -sbc_dif_verify_read(struct se_cmd *cmd, sector_t start, unsigned int sectors,
> -		    unsigned int ei_lba, struct scatterlist *sg, int sg_off)
> +static sense_reason_t
> +__sbc_dif_verify_read(struct se_cmd *cmd, sector_t start, unsigned int sectors,
> +		      unsigned int ei_lba, struct scatterlist *sg, int sg_off)
>   {
>   	struct se_device *dev = cmd->se_dev;
>   	struct se_dif_v1_tuple *sdt;
> @@ -1326,8 +1326,31 @@ sbc_dif_verify_read(struct se_cmd *cmd, sector_t start, unsigned int sectors,
>   		kunmap_atomic(paddr);
>   		kunmap_atomic(daddr);
>   	}
> -	sbc_dif_copy_prot(cmd, sectors, true, sg, sg_off);
>   
>   	return 0;
>   }
> +
> +sense_reason_t
> +sbc_dif_read_strip(struct se_cmd *cmd)
> +{
> +	struct se_device *dev = cmd->se_dev;
> +	u32 sectors = cmd->prot_length / dev->prot_length;
> +
> +	return __sbc_dif_verify_read(cmd, cmd->t_task_lba, sectors, 0,
> +				     cmd->t_prot_sg, 0);
> +}
> +
> +sense_reason_t
> +sbc_dif_verify_read(struct se_cmd *cmd, sector_t start, unsigned int sectors,
> +		    unsigned int ei_lba, struct scatterlist *sg, int sg_off)
> +{
> +	sense_reason_t rc;
> +
> +	rc = __sbc_dif_verify_read(cmd, start, sectors, ei_lba, sg, sg_off);
> +	if (rc)
> +		return rc;
> +
> +	sbc_dif_copy_prot(cmd, sectors, true, sg, sg_off);
> +	return 0;
> +}
>   EXPORT_SYMBOL(sbc_dif_verify_read);
> diff --git a/include/target/target_core_backend.h b/include/target/target_core_backend.h
> index 8e6eac6..68e2820 100644
> --- a/include/target/target_core_backend.h
> +++ b/include/target/target_core_backend.h
> @@ -78,6 +78,7 @@ sense_reason_t	sbc_dif_verify_write(struct se_cmd *, sector_t, unsigned int,
>   				     unsigned int, struct scatterlist *, int);
>   sense_reason_t	sbc_dif_verify_read(struct se_cmd *, sector_t, unsigned int,
>   				    unsigned int, struct scatterlist *, int);
> +sense_reason_t	sbc_dif_read_strip(struct se_cmd *);
>   
>   void	transport_set_vpd_proto_id(struct t10_vpd *, unsigned char *);
>   int	transport_set_vpd_assoc(struct t10_vpd *, unsigned char *);
Looks good to me.

Reviewed-by: Sagi Grimberg <sagig@mellanox.com>

Re: [PATCH 9/9] target: Enable READ_STRIP emulation in target_complete_ok_work

[Sagi Grimberg]

On 4/3/2014 12:35 PM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> This patch enables the use of READ_STRIP software emulation in
> target_complete_ok_work() code for I/O READs.
>
> This is useful when the fabric does not support READ_STRIP hardware
> offload, but would still like to interact with backend device
> that have T10 PI enabled.
>
> Cc: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: Sagi Grimberg <sagig@mellanox.com>
> Cc: Or Gerlitz <ogerlitz@mellanox.com>
> Cc: Quinn Tran <quinn.tran@qlogic.com>
> Cc: Giridhar Malavali <giridhar.malavali@qlogic.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>   drivers/target/target_core_transport.c |   33 ++++++++++++++++++++++++++++++++
>   1 file changed, 33 insertions(+)
>
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 530a9e8..a184103 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -1905,6 +1905,24 @@ static void transport_handle_queue_full(
>   	schedule_work(&cmd->se_dev->qf_work_queue);
>   }
>   
> +static bool target_check_read_strip(struct se_cmd *cmd)
> +{
> +	sense_reason_t rc;
> +
> +	if (cmd->prot_op != TARGET_PROT_DIN_STRIP)
> +		return false;
> +
> +	if (!(cmd->se_sess->sup_prot_ops & TARGET_PROT_DIN_STRIP)) {
> +		rc = sbc_dif_read_strip(cmd);
> +		if (rc) {
> +			cmd->pi_err = rc;
> +			return true;
> +		}
> +	}
> +
> +	return false;
> +}
> +
>   static void target_complete_ok_work(struct work_struct *work)
>   {
>   	struct se_cmd *cmd = container_of(work, struct se_cmd, work);
> @@ -1969,6 +1987,21 @@ static void target_complete_ok_work(struct work_struct *work)
>   					cmd->data_length;
>   		}
>   		spin_unlock(&cmd->se_lun->lun_sep_lock);
> +		/*
> +		 * Perform READ_STRIP of PI using software emulation when
> +		 * backend had PI enabled, if the transport will not be
> +		 * performing hardware READ_STRIP offload.
> +		 */

You always call read_strip() even if the operation is not READ_STRIP 
(the routine won't do anything in this case).
Personally, I usually prefer to avoid calling a routine rather then 
calling it and perform the checks there.

I suggest:

if (cmd->prot_op == TARGET_PROT_DIN_STRIP &&
     target_check_read_strip(cmd)) {
	ret = transport_send_check_condition_and_sense(cmd, cmd->pi_err, 0);
	...
}


> +		if (target_check_read_strip(cmd)) {
> +			ret = transport_send_check_condition_and_sense(cmd,
> +						cmd->pi_err, 0);
> +			if (ret == -EAGAIN || ret == -ENOMEM)
> +				goto queue_full;
> +
> +			transport_lun_remove_cmd(cmd);
> +			transport_cmd_check_stop_to_fabric(cmd);
> +			return;
> +		}
>   
>   		trace_target_cmd_complete(cmd);
>   		ret = cmd->se_tfo->queue_data_in(cmd);

Re: [PATCH 2/9] target: Pass in transport supported PI at session initialization

[Nicholas A. Bellinger]

On Mon, 2014-04-07 at 10:28 +0300, Sagi Grimberg wrote:
> On 4/3/2014 12:35 PM, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> >
> > In order to support local WRITE_INSERT + READ_STRIP operations for
> > non PI enabled fabrics, the fabric driver needs to be able signal
> > what protection offload operations are supported.
> >
> > This is done at session initialization time so the modes can be
> > signaled by individual se_wwn + se_portal_group endpoints, as well
> > as optionally across different transports on the same endpoint.
> >
> > For iser-target, set TARGET_PROT_ALL if the underlying ib_device
> > has already signaled PI offload support, and allow this to be
> > exposed via a new iscsit_transport->iscsit_get_sup_prot_ops()
> > callback.
> >
> > For loopback, set TARGET_PROT_ALL to signal SCSI initiator mode
> > operation.
> >
> > For all other drivers, set TARGET_PROT_NORMAL to disable fabric
> > level PI.
> >
> > Cc: Martin K. Petersen <martin.petersen@oracle.com>
> > Cc: Sagi Grimberg <sagig@mellanox.com>
> > Cc: Or Gerlitz <ogerlitz@mellanox.com>
> > Cc: Quinn Tran <quinn.tran@qlogic.com>
> > Cc: Giridhar Malavali <giridhar.malavali@qlogic.com>
> > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > ---
> >   drivers/infiniband/ulp/isert/ib_isert.c   |   13 +++++++++++++
> >   drivers/infiniband/ulp/srpt/ib_srpt.c     |    2 +-
> >   drivers/scsi/qla2xxx/tcm_qla2xxx.c        |    2 +-
> >   drivers/target/iscsi/iscsi_target.c       |    6 ++++++
> >   drivers/target/iscsi/iscsi_target_login.c |    4 +++-
> >   drivers/target/loopback/tcm_loop.c        |    2 +-
> >   drivers/target/sbp/sbp_target.c           |    2 +-
> >   drivers/target/target_core_transport.c    |    8 +++++---
> >   drivers/target/tcm_fc/tfc_sess.c          |    3 ++-
> >   drivers/usb/gadget/tcm_usb_gadget.c       |    2 +-
> >   drivers/vhost/scsi.c                      |    3 ++-
> >   include/target/iscsi/iscsi_transport.h    |    1 +
> >   include/target/target_core_base.h         |   19 ++++++++++++-------
> >   include/target/target_core_fabric.h       |    5 +++--
> >   14 files changed, 52 insertions(+), 20 deletions(-)
> >

<SNIP>

> > diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
> > index 96aee43..78cab13 100644
> > --- a/drivers/target/iscsi/iscsi_target.c
> > +++ b/drivers/target/iscsi/iscsi_target.c
> > @@ -511,6 +511,11 @@ static void iscsit_aborted_task(struct iscsi_conn *conn, struct iscsi_cmd *cmd)
> >   	__iscsit_free_cmd(cmd, scsi_cmd, true);
> >   }
> >   
> > +static enum target_prot_op iscsit_get_sup_prot_ops(struct iscsi_conn *conn)
> 
> I think it is more correct that this routine will return a bitmap (u32) 
> and not the enumeration itself.
> 

Yeah, I was originally unsure if using bitmask ops on enums was OK, but
AFAICT it's perfectly safe and works fine here for this case.

--nab 

Re: [PATCH 6/9] target/sbc: Add sbc_dif_write_insert software emulation

[Nicholas A. Bellinger]

On Mon, 2014-04-07 at 10:36 +0300, Sagi Grimberg wrote:
> On 4/3/2014 12:35 PM, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> >
> > This patch adds WRITE_INSERT emulation within target-core
> > using TYPE1 / TYPE3 PI modes.
> >
> > This is useful in order for existing legacy fabrics that do not
> > support protection offloads to interact with backend devices that
> > currently have T10 PI enabled.
> >
> > Cc: Martin K. Petersen <martin.petersen@oracle.com>
> > Cc: Sagi Grimberg <sagig@mellanox.com>
> > Cc: Or Gerlitz <ogerlitz@mellanox.com>
> > Cc: Quinn Tran <quinn.tran@qlogic.com>
> > Cc: Giridhar Malavali <giridhar.malavali@qlogic.com>
> > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > ---
> >   drivers/target/target_core_sbc.c     |   44 ++++++++++++++++++++++++++++++++++
> >   include/target/target_core_backend.h |    1 +
> >   2 files changed, 45 insertions(+)
> >
> > diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> > index f2d73dd..97b207d 100644
> > --- a/drivers/target/target_core_sbc.c
> > +++ b/drivers/target/target_core_sbc.c
> > @@ -1096,6 +1096,50 @@ err:
> >   }
> >   EXPORT_SYMBOL(sbc_execute_unmap);
> >   
> > +void
> > +sbc_dif_write_insert(struct se_cmd *cmd)
> 
> Better to call it sbc_dif_generate()

Changed to sbc_dif_generate().

> 
> > +{
> > +	struct se_device *dev = cmd->se_dev;
> > +	struct se_dif_v1_tuple *sdt;
> > +	struct scatterlist *dsg, *psg = cmd->t_prot_sg;
> > +	sector_t sector = cmd->t_task_lba;
> > +	void *daddr, *paddr;
> > +	int i, j, offset = 0;
> > +
> > +	for_each_sg(cmd->t_data_sg, dsg, cmd->t_data_nents, i) {
> > +		daddr = kmap_atomic(sg_page(dsg)) + dsg->offset;
> > +		paddr = kmap_atomic(sg_page(psg)) + psg->offset;
> > +
> > +		for (j = 0; j < dsg->length; j += dev->dev_attrib.block_size) {
> > +
> > +			if (offset >= psg->length) {
> > +				kunmap_atomic(paddr);
> > +				psg = sg_next(psg);
> > +				paddr = kmap_atomic(sg_page(psg)) + psg->offset;
> > +				offset = 0;
> > +			}
> > +
> > +			sdt = paddr + offset;
> > +			sdt->guard_tag = cpu_to_be16(crc_t10dif(daddr + j,
> > +						dev->dev_attrib.block_size));
> > +			if (dev->dev_attrib.pi_prot_type == TARGET_DIF_TYPE1_PROT)
> > +				sdt->ref_tag = cpu_to_be32(sector & 0xffffffff);
> > +			sdt->app_tag = 0;
> > +
> > +			pr_debug("DIF WRITE INSERT sector: %llu guard_tag: 0x%04x"
> > +				 " app_tag: 0x%04x ref_tag: %u\n",
> > +				 (unsigned long long)sector, sdt->guard_tag,
> > +				 sdt->app_tag, be32_to_cpu(sdt->ref_tag));
> > +
> > +			sector++;
> > +			offset += sizeof(struct se_dif_v1_tuple);
> > +		}
> > +
> > +		kunmap_atomic(paddr);
> > +		kunmap_atomic(daddr);
> > +	}
> > +}
> 
> Seems like a substantial code duplication here, can't you reuse the code 
> from verify?
> Maybe use this function also from verify (call generate() and then do 
> compare to t_prot_sg).

Yeah, there are a few differences between the two cases, and I stopped
short of merging them together for the moment.

Should be a pretty straight-forward post -rc1 change to make this into
common code.

--nab

Re: [PATCH 7/9] target: Enable WRITE_INSERT emulation in target_execute_cmd

[Nicholas A. Bellinger]

On Mon, 2014-04-07 at 10:39 +0300, Sagi Grimberg wrote:
> On 4/3/2014 12:35 PM, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> >
> > This patch enables WRITE_INSERT emulation in target_execute_cmd()
> > in order to locally generate DIF PI before submitting the WRITE
> > to the underlying backend device.
> >
> > This is required for fabric drivers that currently don't support
> > DIF over-the-wire, in order to inact with backend devices that
> > have hardware (IBLOCK) or software (FILEIO + RAMDISK) support
> > for handling T10 PI.
> >
> > Cc: Martin K. Petersen <martin.petersen@oracle.com>
> > Cc: Sagi Grimberg <sagig@mellanox.com>
> > Cc: Or Gerlitz <ogerlitz@mellanox.com>
> > Cc: Quinn Tran <quinn.tran@qlogic.com>
> > Cc: Giridhar Malavali <giridhar.malavali@qlogic.com>
> > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > ---
> >   drivers/target/target_core_transport.c |    9 +++++++++
> >   1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> > index 9c820ba..530a9e8 100644
> > --- a/drivers/target/target_core_transport.c
> > +++ b/drivers/target/target_core_transport.c
> > @@ -1767,6 +1767,15 @@ void target_execute_cmd(struct se_cmd *cmd)
> >   	cmd->t_state = TRANSPORT_PROCESSING;
> >   	cmd->transport_state |= CMD_T_ACTIVE|CMD_T_BUSY|CMD_T_SENT;
> >   	spin_unlock_irq(&cmd->t_state_lock);
> > +	/*
> > +	 * Perform WRITE_INSERT of PI using software emulation when backend
> > +	 * device has PI enabled, if the transport has not already generated
> > +	 * PI using hardware WRITE_INSERT offload.
> > +	 */
> > +	if (cmd->prot_op == TARGET_PROT_DOUT_INSERT) {
> > +		if (!(cmd->se_sess->sup_prot_ops & TARGET_PROT_DOUT_INSERT))
> > +			sbc_dif_write_insert(cmd);
> > +	}
> >   
> >   	if (target_handle_task_attr(cmd)) {
> >   		spin_lock_irq(&cmd->t_state_lock);
> Looks good to me.
> 
> Reviewed-by: Sagi Grimberg <sagig@mellanox.com>
> 
> This is useless though without the code setting this prot_op in 
> sbc_set_prot_op_checks()...

Not sure I follow..  sbc_set_prot_op_checks() is already setting
TARGET_PROT_DOUT_INSERT when no protect field in the WRITE CDB has been
set, eg:

static int
sbc_set_prot_op_checks(u8 protect, enum target_prot_type prot_type,
                       bool is_write, struct se_cmd *cmd)
{
        if (is_write) {
                cmd->prot_op = protect ? TARGET_PROT_DOUT_PASS :
                                         TARGET_PROT_DOUT_INSERT;

                <SNIP>
        }
}

Which in-turn is what causes this code to invoke sbc_dif_write_insert
(now sbc_dif_generate) when the backend supports PI, and the fabric
itself does not support WRITE_INSERT hw offload.

--nab 

Re: [PATCH 7/9] target: Enable WRITE_INSERT emulation in target_execute_cmd

[sagi grimberg]

On 4/7/2014 11:11 AM, Nicholas A. Bellinger wrote:
>> Looks good to me.
>>
>> Reviewed-by: Sagi Grimberg <sagig@mellanox.com>
>>
>> This is useless though without the code setting this prot_op in
>> sbc_set_prot_op_checks()...
> Not sure I follow..  sbc_set_prot_op_checks() is already setting
> TARGET_PROT_DOUT_INSERT when no protect field in the WRITE CDB has been
> set, eg:
>
> static int
> sbc_set_prot_op_checks(u8 protect, enum target_prot_type prot_type,
>                         bool is_write, struct se_cmd *cmd)
> {
>          if (is_write) {
>                  cmd->prot_op = protect ? TARGET_PROT_DOUT_PASS :
>                                           TARGET_PROT_DOUT_INSERT;
>
>                  <SNIP>
>          }
> }
>
> Which in-turn is what causes this code to invoke sbc_dif_write_insert
> (now sbc_dif_generate) when the backend supports PI, and the fabric
> itself does not support WRITE_INSERT hw offload.

pphh,

I confused this with DOUT_STRIP/DIN_INSERT.
You can discard.

Sagi.

Re: [PATCH 9/9] target: Enable READ_STRIP emulation in target_complete_ok_work

[Nicholas A. Bellinger]

On Mon, 2014-04-07 at 10:49 +0300, Sagi Grimberg wrote:
> On 4/3/2014 12:35 PM, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> >
> > This patch enables the use of READ_STRIP software emulation in
> > target_complete_ok_work() code for I/O READs.
> >
> > This is useful when the fabric does not support READ_STRIP hardware
> > offload, but would still like to interact with backend device
> > that have T10 PI enabled.
> >
> > Cc: Martin K. Petersen <martin.petersen@oracle.com>
> > Cc: Sagi Grimberg <sagig@mellanox.com>
> > Cc: Or Gerlitz <ogerlitz@mellanox.com>
> > Cc: Quinn Tran <quinn.tran@qlogic.com>
> > Cc: Giridhar Malavali <giridhar.malavali@qlogic.com>
> > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > ---
> >   drivers/target/target_core_transport.c |   33 ++++++++++++++++++++++++++++++++
> >   1 file changed, 33 insertions(+)
> >
> > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> > index 530a9e8..a184103 100644
> > --- a/drivers/target/target_core_transport.c
> > +++ b/drivers/target/target_core_transport.c
> > @@ -1905,6 +1905,24 @@ static void transport_handle_queue_full(
> >   	schedule_work(&cmd->se_dev->qf_work_queue);
> >   }
> >   
> > +static bool target_check_read_strip(struct se_cmd *cmd)
> > +{
> > +	sense_reason_t rc;
> > +
> > +	if (cmd->prot_op != TARGET_PROT_DIN_STRIP)
> > +		return false;
> > +
> > +	if (!(cmd->se_sess->sup_prot_ops & TARGET_PROT_DIN_STRIP)) {
> > +		rc = sbc_dif_read_strip(cmd);
> > +		if (rc) {
> > +			cmd->pi_err = rc;
> > +			return true;
> > +		}
> > +	}
> > +
> > +	return false;
> > +}
> > +
> >   static void target_complete_ok_work(struct work_struct *work)
> >   {
> >   	struct se_cmd *cmd = container_of(work, struct se_cmd, work);
> > @@ -1969,6 +1987,21 @@ static void target_complete_ok_work(struct work_struct *work)
> >   					cmd->data_length;
> >   		}
> >   		spin_unlock(&cmd->se_lun->lun_sep_lock);
> > +		/*
> > +		 * Perform READ_STRIP of PI using software emulation when
> > +		 * backend had PI enabled, if the transport will not be
> > +		 * performing hardware READ_STRIP offload.
> > +		 */
> 
> You always call read_strip() even if the operation is not READ_STRIP 
> (the routine won't do anything in this case).
> Personally, I usually prefer to avoid calling a routine rather then 
> calling it and perform the checks there.
> 
> I suggest:
> 
> if (cmd->prot_op == TARGET_PROT_DIN_STRIP &&
>      target_check_read_strip(cmd)) {
> 	ret = transport_send_check_condition_and_sense(cmd, cmd->pi_err, 0);
> 	...
> }
> 
> 

Yep, this was done to save the indention in transport_complete_work_ok()
from being too obnoxious..

Squashing the following incremental patch into the original.

Thanks Sagi!

--nab

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 673a72d..d4b9869 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1909,9 +1909,6 @@ static bool target_check_read_strip(struct se_cmd *cmd)
 {
        sense_reason_t rc;
 
-       if (cmd->prot_op != TARGET_PROT_DIN_STRIP)
-               return false;
-
        if (!(cmd->se_sess->sup_prot_ops & TARGET_PROT_DIN_STRIP)) {
                rc = sbc_dif_read_strip(cmd);
                if (rc) {
@@ -1992,7 +1989,8 @@ static void target_complete_ok_work(struct work_struct *work)
                 * backend had PI enabled, if the transport will not be
                 * performing hardware READ_STRIP offload.
                 */
-               if (target_check_read_strip(cmd)) {
+               if (cmd->prot_op == TARGET_PROT_DIN_STRIP &&
+                   target_check_read_strip(cmd)) {
                        ret = transport_send_check_condition_and_sense(cmd,
                                                cmd->pi_err, 0);
                        if (ret == -EAGAIN || ret == -ENOMEM)