[PATCH 0/7] scsi: target: make RTPI an TPG identifier

[PATCH 0/7] scsi: target: make RTPI an TPG identifier

[Dmitry Bogdanov]

SAM-5 4.6.5.2 (Relative Port Identifier attribute) defines the attribute
as unique across SCSI target ports:
 The Relative Port Identifier attribute identifies a SCSI target port or
 a SCSI initiator port relative to other SCSI ports in a SCSI target
 device and any SCSI initiator devices contained within that SCSI target
 device. A SCSI target device may assign relative port identifiers to
 its SCSI target ports and any SCSI initiator ports. If relative port
 identifiers are assigned, the SCSI target device shall assign each of
 its SCSI target ports and any SCSI initiator ports a unique relative
 port identifier from 1 to 65 535. SCSI target ports and SCSI initiator
 ports share the same number space.

In the current TCM implementation, auto-incremented lun_rtpi weakly
follows the model outlined by SAM-5 and SPC-4. In case of multiple SCSI
target ports (se_portal_group's), which is common to scenario with
multiple HBAs or multiple iSCSI/FC targets, it's possible to have two
backstores (se_device's) with different values of lun_rtpi on the same
SCSI target port.

Similar issue happens during re-export. If a LUN of a backstore is
removed from a target port and added again to the same target port, RTPI
is incremented again and will be different from the first time.

The two issues happen because each se_device increments RTPI for its own
LUNs independently.

The behaviour means that a SCSI application client can't reliably make any
sense of RTPI values reported by a LUN as it's not really related to SCSI
target ports. A conforming target implementation must ensure that RTPI field is
unique per port. The patchset resolves the issue.

Make RTPI be part of se_tpg instead of se_lun. Make it configurable.


Roman Bolshakov (6):
  scsi: target/core: Add cleanup sequence in core_tpg_register()
  scsi: target/core: Add RTPI field to target port
  scsi: target/core: Use RTPI from target port
  scsi: target/core: Drop device-based RTPI
  scsi: target/core: Add common port attributes
  scsi: target/core: Add RTPI attribute for target port

Dmitry Bogdanov (1):
  target: core: check RTPI uniquity for enabled TPG

 drivers/target/target_core_alua.c            |   4 +-
 drivers/target/target_core_configfs.c        |   9 +-
 drivers/target/target_core_device.c          |  43 +----
 drivers/target/target_core_fabric_configfs.c |  68 ++++++-
 drivers/target/target_core_internal.h        |   4 +-
 drivers/target/target_core_pr.c              |   8 +-
 drivers/target/target_core_spc.c             |   2 +-
 drivers/target/target_core_stat.c            |   6 +-
 drivers/target/target_core_tpg.c             | 180 +++++++++++++++++--
 include/target/target_core_base.h            |   8 +-
 10 files changed, 256 insertions(+), 76 deletions(-)

-- 
2.25.1

[PATCH 1/7] scsi: target: core: Add cleanup sequence in core_tpg_register()

[Dmitry Bogdanov]

From: Roman Bolshakov <r.bolshakov@yadro.com>

It does not change any functionality but allows to introduce more steps
in the cleanup sequence without code duplication later.

Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
---
 drivers/target/target_core_tpg.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index 736847c933e5..f0d38d77edcc 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -484,10 +484,8 @@ int core_tpg_register(
 
 		ret = core_tpg_add_lun(se_tpg, se_tpg->tpg_virt_lun0,
 				true, g_lun0_dev);
-		if (ret < 0) {
-			kfree(se_tpg->tpg_virt_lun0);
-			return ret;
-		}
+		if (ret < 0)
+			goto out_free_lun0;
 	}
 
 	pr_debug("TARGET_CORE[%s]: Allocated portal_group for endpoint: %s, "
@@ -497,6 +495,10 @@ int core_tpg_register(
 		se_tpg->proto_id, se_tpg->se_tpg_tfo->tpg_get_tag(se_tpg));
 
 	return 0;
+
+out_free_lun0:
+	kfree(se_tpg->tpg_virt_lun0);
+	return ret;
 }
 EXPORT_SYMBOL(core_tpg_register);
 
-- 
2.25.1

[PATCH 2/7] scsi: target: core: Add RTPI field to target port

[Dmitry Bogdanov]

From: Roman Bolshakov <r.bolshakov@yadro.com>

SAM-5 4.6.5.2 (Relative Port Identifier attribute) defines the attribute
as unique across SCSI target ports.

The change introduces RTPI attribute to se_portal group. The value is
auto-incremented and unique across all SCSI target ports. It also limits
number of SCSI target ports to 65535.

Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
---
 drivers/target/target_core_tpg.c  | 78 +++++++++++++++++++++++++++++--
 include/target/target_core_base.h |  4 ++
 2 files changed, 77 insertions(+), 5 deletions(-)

diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index f0d38d77edcc..325ef439fb42 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -31,6 +31,10 @@
 #include "target_core_ua.h"
 
 extern struct se_device *g_lun0_dev;
+static u16 g_tpg_count;
+static u16 g_tpg_rtpi_counter = 1;
+static LIST_HEAD(g_tpg_list);
+static DEFINE_SPINLOCK(g_tpg_lock);
 
 /*	__core_tpg_get_initiator_node_acl():
  *
@@ -439,6 +443,57 @@ static void core_tpg_lun_ref_release(struct percpu_ref *ref)
 	complete(&lun->lun_shutdown_comp);
 }
 
+static int core_tpg_register_rtpi(struct se_portal_group *se_tpg)
+{
+	struct se_portal_group *tpg;
+
+	/*
+	 * Allocate the next RELATIVE TARGET PORT IDENTIFIER.
+	 * Here is the table from SPC-4 4.3.4:
+	 *
+	 *    Table 34 -- Relative target port identifier values
+	 *
+	 * Value		Description
+	 * 0h			Reserved
+	 * 1h			Relative port 1, historically known as port A
+	 * 2h			Relative port 2, historically known as port B
+	 * 3h to FFFFh		Relative port 3 through 65 535
+	 */
+	spin_lock(&g_tpg_lock);
+
+	if (g_tpg_count == 0xffff) {
+		spin_unlock(&g_tpg_lock);
+		pr_warn("Reached g_tpg_count == 0xffff\n");
+		return -ENOSPC;
+	}
+again:
+	se_tpg->tpg_rtpi = g_tpg_rtpi_counter++;
+	if (!se_tpg->tpg_rtpi)
+		goto again;
+
+	list_for_each_entry(tpg, &g_tpg_list, tpg_list) {
+		/*
+		 * Make sure RELATIVE TARGET PORT IDENTIFIER is unique
+		 * for 16-bit wrap..
+		 */
+		if (se_tpg->tpg_rtpi == tpg->tpg_rtpi)
+			goto again;
+	}
+	list_add(&se_tpg->tpg_list, &g_tpg_list);
+	g_tpg_count++;
+	spin_unlock(&g_tpg_lock);
+
+	return 0;
+}
+
+static void core_tpg_deregister_rtpi(struct se_portal_group *se_tpg)
+{
+	spin_lock(&g_tpg_lock);
+	list_del(&se_tpg->tpg_list);
+	g_tpg_count--;
+	spin_unlock(&g_tpg_lock);
+}
+
 /* Does not change se_wwn->priv. */
 int core_tpg_register(
 	struct se_wwn *se_wwn,
@@ -471,6 +526,7 @@ int core_tpg_register(
 	se_tpg->proto_id = proto_id;
 	se_tpg->se_tpg_wwn = se_wwn;
 	atomic_set(&se_tpg->tpg_pr_ref_count, 0);
+	INIT_LIST_HEAD(&se_tpg->tpg_list);
 	INIT_LIST_HEAD(&se_tpg->acl_node_list);
 	INIT_LIST_HEAD(&se_tpg->tpg_sess_list);
 	spin_lock_init(&se_tpg->session_lock);
@@ -478,9 +534,15 @@ int core_tpg_register(
 	mutex_init(&se_tpg->acl_node_mutex);
 
 	if (se_tpg->proto_id >= 0) {
+		ret = core_tpg_register_rtpi(se_tpg);
+		if (ret < 0)
+			return ret;
+
 		se_tpg->tpg_virt_lun0 = core_tpg_alloc_lun(se_tpg, 0);
-		if (IS_ERR(se_tpg->tpg_virt_lun0))
-			return PTR_ERR(se_tpg->tpg_virt_lun0);
+		if (IS_ERR(se_tpg->tpg_virt_lun0)) {
+			ret = PTR_ERR(se_tpg->tpg_virt_lun0);
+			goto out_deregister_rtpi;
+		}
 
 		ret = core_tpg_add_lun(se_tpg, se_tpg->tpg_virt_lun0,
 				true, g_lun0_dev);
@@ -488,16 +550,20 @@ int core_tpg_register(
 			goto out_free_lun0;
 	}
 
-	pr_debug("TARGET_CORE[%s]: Allocated portal_group for endpoint: %s, "
-		 "Proto: %d, Portal Tag: %u\n", se_tpg->se_tpg_tfo->fabric_name,
+	pr_debug("TARGET_CORE[%s]: Allocated portal_group for endpoint: %s, Proto: %d, Portal Tag: %u, RTPI: %#2x\n",
+		se_tpg->se_tpg_tfo->fabric_name,
 		se_tpg->se_tpg_tfo->tpg_get_wwn(se_tpg) ?
 		se_tpg->se_tpg_tfo->tpg_get_wwn(se_tpg) : NULL,
-		se_tpg->proto_id, se_tpg->se_tpg_tfo->tpg_get_tag(se_tpg));
+		se_tpg->proto_id,
+		se_tpg->se_tpg_tfo->tpg_get_tag(se_tpg),
+		se_tpg->tpg_rtpi);
 
 	return 0;
 
 out_free_lun0:
 	kfree(se_tpg->tpg_virt_lun0);
+out_deregister_rtpi:
+	core_tpg_deregister_rtpi(se_tpg);
 	return ret;
 }
 EXPORT_SYMBOL(core_tpg_register);
@@ -535,6 +601,8 @@ int core_tpg_deregister(struct se_portal_group *se_tpg)
 	if (se_tpg->proto_id >= 0) {
 		core_tpg_remove_lun(se_tpg, se_tpg->tpg_virt_lun0);
 		kfree_rcu(se_tpg->tpg_virt_lun0, rcu_head);
+
+		core_tpg_deregister_rtpi(se_tpg);
 	}
 
 	return 0;
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 28cce4ed3f0e..08e76e400816 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -904,6 +904,8 @@ struct se_portal_group {
 	 */
 	int			proto_id;
 	bool			enabled;
+	/* RELATIVE TARGET PORT IDENTIFIER */
+	u16			tpg_rtpi;
 	/* Used for PR SPEC_I_PT=1 and REGISTER_AND_MOVE */
 	atomic_t		tpg_pr_ref_count;
 	/* Spinlock for adding/removing ACLed Nodes */
@@ -911,6 +913,8 @@ struct se_portal_group {
 	/* Spinlock for adding/removing sessions */
 	spinlock_t		session_lock;
 	struct mutex		tpg_lun_mutex;
+	/* List of all SCSI target ports */
+	struct list_head	tpg_list;
 	/* linked list for initiator ACL list */
 	struct list_head	acl_node_list;
 	struct hlist_head	tpg_lun_hlist;
-- 
2.25.1

[PATCH 3/7] scsi: target: core: Use RTPI from target port

[Dmitry Bogdanov]

From: Roman Bolshakov <r.bolshakov@yadro.com>

Replace all references to RTPI from LUN field to se_portal_group field.
It introduces consitent reporting of RTPI for all LUNs and all target
ports.

Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
---
 drivers/target/target_core_alua.c   | 4 ++--
 drivers/target/target_core_device.c | 2 +-
 drivers/target/target_core_pr.c     | 8 ++++----
 drivers/target/target_core_spc.c    | 2 +-
 drivers/target/target_core_stat.c   | 6 +++---
 5 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
index dbf9a950d01b..0b043b6c13d8 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -224,7 +224,7 @@ target_emulate_report_target_port_groups(struct se_cmd *cmd)
 			/*
 			 * Set RELATIVE TARGET PORT IDENTIFIER
 			 */
-			put_unaligned_be16(lun->lun_rtpi, &buf[off]);
+			put_unaligned_be16(lun->lun_tpg->tpg_rtpi, &buf[off]);
 			off += 2;
 			rd_len += 4;
 		}
@@ -404,7 +404,7 @@ target_emulate_set_target_port_groups(struct se_cmd *cmd)
 			spin_lock(&dev->se_port_lock);
 			list_for_each_entry(lun, &dev->dev_sep_list,
 							lun_dev_link) {
-				if (lun->lun_rtpi != rtpi)
+				if (lun->lun_tpg->tpg_rtpi != rtpi)
 					continue;
 
 				// XXX: racy unlock
diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index b7f16ee8aa0e..ad83338d6140 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -223,7 +223,7 @@ struct se_dev_entry *core_get_se_deve_from_rtpi(
 				tpg->se_tpg_tfo->fabric_name);
 			continue;
 		}
-		if (lun->lun_rtpi != rtpi)
+		if (lun->lun_tpg->tpg_rtpi != rtpi)
 			continue;
 
 		kref_get(&deve->pr_kref);
diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index a1d67554709f..ed0906fd7965 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -663,7 +663,7 @@ static struct t10_pr_registration *__core_scsi3_do_alloc_registration(
 	}
 	pr_reg->pr_res_mapped_lun = mapped_lun;
 	pr_reg->pr_aptpl_target_lun = lun->unpacked_lun;
-	pr_reg->tg_pt_sep_rtpi = lun->lun_rtpi;
+	pr_reg->tg_pt_sep_rtpi = lun->lun_tpg->tpg_rtpi;
 	pr_reg->pr_res_key = sa_res_key;
 	pr_reg->pr_reg_all_tg_pt = all_tg_pt;
 	pr_reg->pr_reg_aptpl = aptpl;
@@ -967,7 +967,7 @@ static int __core_scsi3_check_aptpl_registration(
 			rcu_read_unlock();
 
 			pr_reg->pr_reg_nacl = nacl;
-			pr_reg->tg_pt_sep_rtpi = lun->lun_rtpi;
+			pr_reg->tg_pt_sep_rtpi = lun->lun_tpg->tpg_rtpi;
 			list_del(&pr_reg->pr_reg_aptpl_list);
 			spin_unlock(&pr_tmpl->aptpl_reg_lock);
 			/*
@@ -1567,7 +1567,7 @@ core_scsi3_decode_spec_i_port(
 			 */
 			if (tmp_tpg->proto_id != proto_ident)
 				continue;
-			dest_rtpi = tmp_lun->lun_rtpi;
+			dest_rtpi = tmp_lun->lun_tpg->tpg_rtpi;
 
 			iport_ptr = NULL;
 			i_str = target_parse_pr_out_transport_id(tmp_tpg,
@@ -3210,7 +3210,7 @@ core_scsi3_emulate_pro_register_and_move(struct se_cmd *cmd, u64 res_key,
 
 	spin_lock(&dev->se_port_lock);
 	list_for_each_entry(tmp_lun, &dev->dev_sep_list, lun_dev_link) {
-		if (tmp_lun->lun_rtpi != rtpi)
+		if (tmp_lun->lun_tpg->tpg_rtpi != rtpi)
 			continue;
 		dest_se_tpg = tmp_lun->lun_tpg;
 		dest_tf_ops = dest_se_tpg->se_tpg_tfo;
diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index 32fb38ce98f4..d8a5ea255459 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -331,7 +331,7 @@ spc_emulate_evpd_83(struct se_cmd *cmd, unsigned char *buf)
 		/* Skip over Obsolete field in RTPI payload
 		 * in Table 472 */
 		off += 2;
-		put_unaligned_be16(lun->lun_rtpi, &buf[off]);
+		put_unaligned_be16(lun->lun_tpg->tpg_rtpi, &buf[off]);
 		off += 2;
 		len += 8; /* Header size + Designation descriptor */
 		/*
diff --git a/drivers/target/target_core_stat.c b/drivers/target/target_core_stat.c
index f85ee5b0fd80..c42cbde8a31b 100644
--- a/drivers/target/target_core_stat.c
+++ b/drivers/target/target_core_stat.c
@@ -455,7 +455,7 @@ static ssize_t target_stat_port_indx_show(struct config_item *item, char *page)
 	rcu_read_lock();
 	dev = rcu_dereference(lun->lun_se_dev);
 	if (dev)
-		ret = snprintf(page, PAGE_SIZE, "%u\n", lun->lun_rtpi);
+		ret = snprintf(page, PAGE_SIZE, "%u\n", lun->lun_tpg->tpg_rtpi);
 	rcu_read_unlock();
 	return ret;
 }
@@ -561,7 +561,7 @@ static ssize_t target_stat_tgt_port_indx_show(struct config_item *item,
 	rcu_read_lock();
 	dev = rcu_dereference(lun->lun_se_dev);
 	if (dev)
-		ret = snprintf(page, PAGE_SIZE, "%u\n", lun->lun_rtpi);
+		ret = snprintf(page, PAGE_SIZE, "%u\n", lun->lun_tpg->tpg_rtpi);
 	rcu_read_unlock();
 	return ret;
 }
@@ -579,7 +579,7 @@ static ssize_t target_stat_tgt_port_name_show(struct config_item *item,
 	if (dev)
 		ret = snprintf(page, PAGE_SIZE, "%sPort#%u\n",
 			tpg->se_tpg_tfo->fabric_name,
-			lun->lun_rtpi);
+			lun->lun_tpg->tpg_rtpi);
 	rcu_read_unlock();
 	return ret;
 }
-- 
2.25.1

[PATCH 4/7] scsi: target: core: Drop device-based RTPI

[Dmitry Bogdanov]

From: Roman Bolshakov <r.bolshakov@yadro.com>

The code is not needed since target port-based RTPI allocation replaced
it.

Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
---
 drivers/target/target_core_device.c   | 41 ---------------------------
 drivers/target/target_core_internal.h |  1 -
 drivers/target/target_core_tpg.c      |  6 ----
 include/target/target_core_base.h     |  4 ---
 4 files changed, 52 deletions(-)

diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index ad83338d6140..2209660261d0 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -460,47 +460,6 @@ void core_clear_lun_from_tpg(struct se_lun *lun, struct se_portal_group *tpg)
 	mutex_unlock(&tpg->acl_node_mutex);
 }
 
-int core_alloc_rtpi(struct se_lun *lun, struct se_device *dev)
-{
-	struct se_lun *tmp;
-
-	spin_lock(&dev->se_port_lock);
-	if (dev->export_count == 0x0000ffff) {
-		pr_warn("Reached dev->dev_port_count =="
-				" 0x0000ffff\n");
-		spin_unlock(&dev->se_port_lock);
-		return -ENOSPC;
-	}
-again:
-	/*
-	 * Allocate the next RELATIVE TARGET PORT IDENTIFIER for this struct se_device
-	 * Here is the table from spc4r17 section 7.7.3.8.
-	 *
-	 *    Table 473 -- RELATIVE TARGET PORT IDENTIFIER field
-	 *
-	 * Code      Description
-	 * 0h        Reserved
-	 * 1h        Relative port 1, historically known as port A
-	 * 2h        Relative port 2, historically known as port B
-	 * 3h to FFFFh    Relative port 3 through 65 535
-	 */
-	lun->lun_rtpi = dev->dev_rpti_counter++;
-	if (!lun->lun_rtpi)
-		goto again;
-
-	list_for_each_entry(tmp, &dev->dev_sep_list, lun_dev_link) {
-		/*
-		 * Make sure RELATIVE TARGET PORT IDENTIFIER is unique
-		 * for 16-bit wrap..
-		 */
-		if (lun->lun_rtpi == tmp->lun_rtpi)
-			goto again;
-	}
-	spin_unlock(&dev->se_port_lock);
-
-	return 0;
-}
-
 static void se_release_vpd_for_dev(struct se_device *dev)
 {
 	struct t10_vpd *vpd, *vpd_tmp;
diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h
index a889a6237d9c..7dcc62749edd 100644
--- a/drivers/target/target_core_internal.h
+++ b/drivers/target/target_core_internal.h
@@ -59,7 +59,6 @@ struct target_fabric_configfs {
 extern struct t10_alua_lu_gp *default_lu_gp;
 
 /* target_core_device.c */
-int	core_alloc_rtpi(struct se_lun *lun, struct se_device *dev);
 struct se_dev_entry *core_get_se_deve_from_rtpi(struct se_node_acl *, u16);
 void	target_pr_kref_release(struct kref *);
 void	core_free_device_list_for_node(struct se_node_acl *,
diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index 325ef439fb42..e340ef00652b 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -648,10 +648,6 @@ int core_tpg_add_lun(
 	if (ret < 0)
 		goto out;
 
-	ret = core_alloc_rtpi(lun, dev);
-	if (ret)
-		goto out_kill_ref;
-
 	if (!(dev->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_ALUA) &&
 	    !(dev->se_hba->hba_flags & HBA_FLAGS_INTERNAL_USE))
 		target_attach_tg_pt_gp(lun, dev->t10_alua.default_tg_pt_gp);
@@ -675,8 +671,6 @@ int core_tpg_add_lun(
 
 	return 0;
 
-out_kill_ref:
-	percpu_ref_exit(&lun->lun_ref);
 out:
 	return ret;
 }
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 08e76e400816..e94036cedd85 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -734,8 +734,6 @@ struct se_lun {
 	bool			lun_access_ro;
 	u32			lun_index;
 
-	/* RELATIVE TARGET PORT IDENTIFER */
-	u16			lun_rtpi;
 	atomic_t		lun_acl_count;
 	struct se_device __rcu	*lun_se_dev;
 
@@ -787,8 +785,6 @@ struct se_device_queue {
 };
 
 struct se_device {
-	/* RELATIVE TARGET PORT IDENTIFER Counter */
-	u16			dev_rpti_counter;
 	/* Used for SAM Task Attribute ordering */
 	u32			dev_cur_ordered_id;
 	u32			dev_flags;
-- 
2.25.1

[PATCH 5/7] scsi: target: core: Add common port attributes

[Dmitry Bogdanov]

From: Roman Bolshakov <r.bolshakov@yadro.com>

All SCSI target port attributes (tpgN/attribs/attrname) are defined and
implemented in fabric modules in existing implementation.

The change introduces a way to have common tpg attribs in configfs for
all fabrics without code duplication.

Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
---
 drivers/target/target_core_configfs.c        |  9 ++++-
 drivers/target/target_core_fabric_configfs.c | 39 +++++++++++++++++++-
 drivers/target/target_core_internal.h        |  1 +
 drivers/target/target_core_tpg.c             |  4 ++
 4 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index 7c0e42e782de..40b9808738d2 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -464,13 +464,19 @@ int target_register_template(const struct target_core_fabric_ops *fo)
 	INIT_LIST_HEAD(&tf->tf_list);
 	atomic_set(&tf->tf_access_cnt, 0);
 	tf->tf_ops = fo;
-	target_fabric_setup_cits(tf);
+	ret = target_fabric_setup_cits(tf);
+	if (ret)
+		goto out;
 
 	mutex_lock(&g_tf_lock);
 	list_add_tail(&tf->tf_list, &g_tf_list);
 	mutex_unlock(&g_tf_lock);
 
 	return 0;
+
+out:
+	kfree(tf);
+	return ret;
 }
 EXPORT_SYMBOL(target_register_template);
 
@@ -491,6 +497,7 @@ void target_unregister_template(const struct target_core_fabric_ops *fo)
 			 */
 			rcu_barrier();
 			kfree(t->tf_tpg_base_cit.ct_attrs);
+			kfree(t->tf_tpg_attrib_cit.ct_attrs);
 			kfree(t);
 			return;
 		}
diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/target/target_core_fabric_configfs.c
index 95a88f6224cd..a34b5db4eec5 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -795,7 +795,38 @@ TF_CIT_SETUP(tpg_lun, NULL, &target_fabric_lun_group_ops, NULL);
 
 /* End of tfc_tpg_lun_cit */
 
-TF_CIT_SETUP_DRV(tpg_attrib, NULL, NULL);
+static int
+target_fabric_setup_tpg_attrib_cit(struct target_fabric_configfs *tf)
+{
+	int i, k, len = 0;
+	struct config_item_type *cit = &tf->tf_tpg_attrib_cit;
+	struct configfs_attribute **attrs;
+
+	for (i = 0; core_tpg_attrib_attrs[i]; i++)
+		len += sizeof(struct configfs_attribute *);
+	if (tf->tf_ops->tfc_tpg_attrib_attrs)
+		for (i = 0; tf->tf_ops->tfc_tpg_attrib_attrs[i]; i++)
+			len += sizeof(struct configfs_attribute *);
+	len += sizeof(struct configfs_attribute *);
+
+	attrs = kzalloc(len, GFP_KERNEL);
+	if (!attrs)
+		return -ENOMEM;
+
+	for (i = 0; core_tpg_attrib_attrs[i]; i++)
+		attrs[i] = core_tpg_attrib_attrs[i];
+	if (tf->tf_ops->tfc_tpg_attrib_attrs)
+		for (k = 0; tf->tf_ops->tfc_tpg_attrib_attrs[k]; k++, i++)
+			attrs[i] = tf->tf_ops->tfc_tpg_attrib_attrs[k];
+	attrs[i] = NULL;
+
+	cit->ct_attrs = attrs;
+	cit->ct_owner = tf->tf_ops->module;
+	pr_debug("Setup generic tpg_attrib\n");
+
+	return 0;
+}
+
 TF_CIT_SETUP_DRV(tpg_auth, NULL, NULL);
 TF_CIT_SETUP_DRV(tpg_param, NULL, NULL);
 
@@ -1113,7 +1144,11 @@ int target_fabric_setup_cits(struct target_fabric_configfs *tf)
 	target_fabric_setup_tpg_lun_cit(tf);
 	target_fabric_setup_tpg_np_cit(tf);
 	target_fabric_setup_tpg_np_base_cit(tf);
-	target_fabric_setup_tpg_attrib_cit(tf);
+
+	ret = target_fabric_setup_tpg_attrib_cit(tf);
+	if (ret)
+		return ret;
+
 	target_fabric_setup_tpg_auth_cit(tf);
 	target_fabric_setup_tpg_param_cit(tf);
 	target_fabric_setup_tpg_nacl_cit(tf);
diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h
index 7dcc62749edd..d662cdc9a04c 100644
--- a/drivers/target/target_core_internal.h
+++ b/drivers/target/target_core_internal.h
@@ -117,6 +117,7 @@ int	core_tmr_lun_reset(struct se_device *, struct se_tmr_req *,
 
 /* target_core_tpg.c */
 extern struct se_device *g_lun0_dev;
+extern struct configfs_attribute *core_tpg_attrib_attrs[];
 
 struct se_node_acl *__core_tpg_get_initiator_node_acl(struct se_portal_group *tpg,
 		const char *);
diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index e340ef00652b..b359c83a99c6 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -714,3 +714,7 @@ void core_tpg_remove_lun(
 
 	percpu_ref_exit(&lun->lun_ref);
 }
+
+struct configfs_attribute *core_tpg_attrib_attrs[] = {
+	NULL,
+};
-- 
2.25.1

[PATCH 6/7] scsi: target: core: Add RTPI attribute for target port

[Dmitry Bogdanov]

From: Roman Bolshakov <r.bolshakov@yadro.com>

RELATIVE TARGET PORT IDENTIFIER can be read and configured via configfs:
$ echo 0x10 > $TARGET/tpgt_N/attrib/rtpi

RTPI has to be unique among all target ports, otherwise the write fails.

Configuration of a new RTPI value triggers INQUIRY DATA HAS CHANGED unit
attention as defined in SPC-4 "6.6.1 INQUIRY command introduction":

> If INQUIRY data changes for any reason, the device server shall
> establish a unit attention condition for the initiator port associated
> with every I_T nexus (see SAM-5), with the additional sense code set to
> INQUIRY DATA HAS CHANGED.

Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
---
 drivers/target/target_core_tpg.c | 70 ++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index b359c83a99c6..85c9473a38fd 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -715,6 +715,76 @@ void core_tpg_remove_lun(
 	percpu_ref_exit(&lun->lun_ref);
 }
 
+/* Set a Unit Attention on all I_T nexuses related to the port */
+static void core_tpg_ua(struct se_portal_group *se_tpg, u8 asc, u8 ascq)
+{
+	struct se_lun *lun;
+	struct se_dev_entry *se_deve;
+
+	mutex_lock(&se_tpg->tpg_lun_mutex);
+	hlist_for_each_entry_rcu(lun, &se_tpg->tpg_lun_hlist, link) {
+		spin_lock(&lun->lun_deve_lock);
+		list_for_each_entry(se_deve, &lun->lun_deve_list, lun_link)
+			core_scsi3_ua_allocate(se_deve, asc, ascq);
+		spin_unlock(&lun->lun_deve_lock);
+	}
+	mutex_unlock(&se_tpg->tpg_lun_mutex);
+}
+
+static ssize_t core_tpg_rtpi_show(struct config_item *item, char *page)
+{
+	struct se_portal_group *se_tpg = attrib_to_tpg(item);
+
+	return sprintf(page, "%#x\n", se_tpg->tpg_rtpi);
+}
+
+static ssize_t core_tpg_rtpi_store(struct config_item *item,
+				   const char *page, size_t count)
+{
+	struct se_portal_group *se_tpg = attrib_to_tpg(item);
+	struct se_portal_group *tpg;
+	bool rtpi_changed = false;
+	u16 val;
+	int ret;
+
+	ret = kstrtou16(page, 0, &val);
+	if (ret < 0)
+		return ret;
+	if (val == 0)
+		return -EINVAL;
+
+	/* RTPI shouldn't conflict with values of existing ports */
+	spin_lock(&g_tpg_lock);
+
+	list_for_each_entry(tpg, &g_tpg_list, tpg_list) {
+		if (se_tpg != tpg && val == tpg->tpg_rtpi) {
+			spin_unlock(&g_tpg_lock);
+			pr_err("TARGET_CORE[%s]->TPG[%u] - RTPI %#x conflicts with TARGET_CORE[%s]->TPG[%u]\n",
+			       se_tpg->se_tpg_tfo->fabric_name,
+			       se_tpg->se_tpg_tfo->tpg_get_tag(tpg),
+			       val,
+			       tpg->se_tpg_tfo->fabric_name,
+			       tpg->se_tpg_tfo->tpg_get_tag(tpg));
+			return -EINVAL;
+		}
+	}
+
+	if (se_tpg->tpg_rtpi != val) {
+		se_tpg->tpg_rtpi = val;
+		rtpi_changed = true;
+	}
+	spin_unlock(&g_tpg_lock);
+
+	if (rtpi_changed)
+		core_tpg_ua(se_tpg, 0x3f, ASCQ_3FH_INQUIRY_DATA_HAS_CHANGED);
+	ret = count;
+
+	return ret;
+}
+
+CONFIGFS_ATTR(core_tpg_, rtpi);
+
 struct configfs_attribute *core_tpg_attrib_attrs[] = {
+	&core_tpg_attr_rtpi,
 	NULL,
 };
-- 
2.25.1

[PATCH 7/7] target: core: check RTPI uniquity for enabled TPG

[Dmitry Bogdanov]

Garantee uniquity of RTPI only for enabled target port groups.
Allow any RPTI for disabled tpg until it is enabled.

Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
---
 drivers/target/target_core_fabric_configfs.c | 29 +++++++++++++-
 drivers/target/target_core_internal.h        |  2 +
 drivers/target/target_core_tpg.c             | 40 +++++++++++++-------
 3 files changed, 56 insertions(+), 15 deletions(-)

diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/target/target_core_fabric_configfs.c
index a34b5db4eec5..fc1b8f54fb54 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -857,6 +857,7 @@ static ssize_t target_fabric_tpg_base_enable_store(struct config_item *item,
 						   size_t count)
 {
 	struct se_portal_group *se_tpg = to_tpg(item);
+	struct se_portal_group *tpg;
 	int ret;
 	bool op;
 
@@ -867,11 +868,37 @@ static ssize_t target_fabric_tpg_base_enable_store(struct config_item *item,
 	if (se_tpg->enabled == op)
 		return count;
 
+	spin_lock(&g_tpg_lock);
+
+	if (op) {
+		tpg = core_get_tpg_by_rtpi(se_tpg->tpg_rtpi);
+		if (tpg) {
+			spin_unlock(&g_tpg_lock);
+
+			pr_err("TARGET_CORE[%s]->TPG[%u] - RTPI %#x conflicts with TARGET_CORE[%s]->TPG[%u]\n",
+			       se_tpg->se_tpg_tfo->fabric_name,
+			       se_tpg->se_tpg_tfo->tpg_get_tag(tpg),
+			       se_tpg->tpg_rtpi,
+			       tpg->se_tpg_tfo->fabric_name,
+			       tpg->se_tpg_tfo->tpg_get_tag(tpg));
+			return -EINVAL;
+		}
+	}
+
+	se_tpg->enabled |= 0x10; /* transient state */
+	spin_unlock(&g_tpg_lock);
+
 	ret = se_tpg->se_tpg_tfo->fabric_enable_tpg(se_tpg, op);
-	if (ret)
+
+	spin_lock(&g_tpg_lock);
+	if (ret < 0) {
+		se_tpg->enabled	&= ~0x10; /* clear transient state */
+		spin_unlock(&g_tpg_lock);
 		return ret;
+	}
 
 	se_tpg->enabled = op;
+	spin_unlock(&g_tpg_lock);
 
 	return count;
 }
diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h
index d662cdc9a04c..d4ace697edb0 100644
--- a/drivers/target/target_core_internal.h
+++ b/drivers/target/target_core_internal.h
@@ -116,6 +116,7 @@ int	core_tmr_lun_reset(struct se_device *, struct se_tmr_req *,
 		struct list_head *, struct se_cmd *);
 
 /* target_core_tpg.c */
+extern struct spinlock g_tpg_lock;
 extern struct se_device *g_lun0_dev;
 extern struct configfs_attribute *core_tpg_attrib_attrs[];
 
@@ -131,6 +132,7 @@ void core_tpg_remove_lun(struct se_portal_group *, struct se_lun *);
 struct se_node_acl *core_tpg_add_initiator_node_acl(struct se_portal_group *tpg,
 		const char *initiatorname);
 void core_tpg_del_initiator_node_acl(struct se_node_acl *acl);
+struct se_portal_group *core_get_tpg_by_rtpi(u16 rtpi);
 
 /* target_core_transport.c */
 extern struct kmem_cache *se_tmr_req_cache;
diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index 85c9473a38fd..ef2adbe628e0 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -34,7 +34,7 @@ extern struct se_device *g_lun0_dev;
 static u16 g_tpg_count;
 static u16 g_tpg_rtpi_counter = 1;
 static LIST_HEAD(g_tpg_list);
-static DEFINE_SPINLOCK(g_tpg_lock);
+DEFINE_SPINLOCK(g_tpg_lock);
 
 /*	__core_tpg_get_initiator_node_acl():
  *
@@ -476,7 +476,7 @@ static int core_tpg_register_rtpi(struct se_portal_group *se_tpg)
 		 * Make sure RELATIVE TARGET PORT IDENTIFIER is unique
 		 * for 16-bit wrap..
 		 */
-		if (se_tpg->tpg_rtpi == tpg->tpg_rtpi)
+		if (tpg->enabled && se_tpg->tpg_rtpi == tpg->tpg_rtpi)
 			goto again;
 	}
 	list_add(&se_tpg->tpg_list, &g_tpg_list);
@@ -738,12 +738,26 @@ static ssize_t core_tpg_rtpi_show(struct config_item *item, char *page)
 	return sprintf(page, "%#x\n", se_tpg->tpg_rtpi);
 }
 
+struct se_portal_group *
+core_get_tpg_by_rtpi(u16 rtpi)
+{
+	struct se_portal_group *tpg;
+
+	lockdep_assert_held(&g_tpg_lock);
+
+	list_for_each_entry(tpg, &g_tpg_list, tpg_list) {
+		if (tpg->enabled && rtpi == tpg->tpg_rtpi)
+			return tpg;
+	}
+
+	return NULL;
+}
+
 static ssize_t core_tpg_rtpi_store(struct config_item *item,
 				   const char *page, size_t count)
 {
 	struct se_portal_group *se_tpg = attrib_to_tpg(item);
 	struct se_portal_group *tpg;
-	bool rtpi_changed = false;
 	u16 val;
 	int ret;
 
@@ -753,11 +767,14 @@ static ssize_t core_tpg_rtpi_store(struct config_item *item,
 	if (val == 0)
 		return -EINVAL;
 
-	/* RTPI shouldn't conflict with values of existing ports */
+	if (se_tpg->tpg_rtpi == val)
+		return count;
+
 	spin_lock(&g_tpg_lock);
 
-	list_for_each_entry(tpg, &g_tpg_list, tpg_list) {
-		if (se_tpg != tpg && val == tpg->tpg_rtpi) {
+	if (se_tpg->enabled) {
+		tpg = core_get_tpg_by_rtpi(val);
+		if (tpg) {
 			spin_unlock(&g_tpg_lock);
 			pr_err("TARGET_CORE[%s]->TPG[%u] - RTPI %#x conflicts with TARGET_CORE[%s]->TPG[%u]\n",
 			       se_tpg->se_tpg_tfo->fabric_name,
@@ -769,17 +786,12 @@ static ssize_t core_tpg_rtpi_store(struct config_item *item,
 		}
 	}
 
-	if (se_tpg->tpg_rtpi != val) {
-		se_tpg->tpg_rtpi = val;
-		rtpi_changed = true;
-	}
+	se_tpg->tpg_rtpi = val;
 	spin_unlock(&g_tpg_lock);
 
-	if (rtpi_changed)
-		core_tpg_ua(se_tpg, 0x3f, ASCQ_3FH_INQUIRY_DATA_HAS_CHANGED);
-	ret = count;
+	core_tpg_ua(se_tpg, 0x3f, ASCQ_3FH_INQUIRY_DATA_HAS_CHANGED);
 
-	return ret;
+	return count;
 }
 
 CONFIGFS_ATTR(core_tpg_, rtpi);
-- 
2.25.1

Re: [PATCH 2/7] scsi: target: core: Add RTPI field to target port

[Mike Christie]

On 9/6/22 10:45 AM, Dmitry Bogdanov wrote:
> From: Roman Bolshakov <r.bolshakov@yadro.com>
> 
> SAM-5 4.6.5.2 (Relative Port Identifier attribute) defines the attribute
> as unique across SCSI target ports.
> 
> The change introduces RTPI attribute to se_portal group. The value is
> auto-incremented and unique across all SCSI target ports. It also limits
> number of SCSI target ports to 65535.
> 
> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
> ---
>  drivers/target/target_core_tpg.c  | 78 +++++++++++++++++++++++++++++--
>  include/target/target_core_base.h |  4 ++
>  2 files changed, 77 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
> index f0d38d77edcc..325ef439fb42 100644
> --- a/drivers/target/target_core_tpg.c
> +++ b/drivers/target/target_core_tpg.c
> @@ -31,6 +31,10 @@
>  #include "target_core_ua.h"
>  
>  extern struct se_device *g_lun0_dev;
> +static u16 g_tpg_count;
> +static u16 g_tpg_rtpi_counter = 1;
> +static LIST_HEAD(g_tpg_list);
> +static DEFINE_SPINLOCK(g_tpg_lock);
>  
>  /*	__core_tpg_get_initiator_node_acl():
>   *
> @@ -439,6 +443,57 @@ static void core_tpg_lun_ref_release(struct percpu_ref *ref)
>  	complete(&lun->lun_shutdown_comp);
>  }
>  
> +static int core_tpg_register_rtpi(struct se_portal_group *se_tpg)
> +{
> +	struct se_portal_group *tpg;
> +
> +	/*
> +	 * Allocate the next RELATIVE TARGET PORT IDENTIFIER.
> +	 * Here is the table from SPC-4 4.3.4:
> +	 *
> +	 *    Table 34 -- Relative target port identifier values
> +	 *
> +	 * Value		Description
> +	 * 0h			Reserved
> +	 * 1h			Relative port 1, historically known as port A
> +	 * 2h			Relative port 2, historically known as port B
> +	 * 3h to FFFFh		Relative port 3 through 65 535
> +	 */
> +	spin_lock(&g_tpg_lock);
> +
> +	if (g_tpg_count == 0xffff) {
> +		spin_unlock(&g_tpg_lock);
> +		pr_warn("Reached g_tpg_count == 0xffff\n");
> +		return -ENOSPC;
> +	}
> +again:
> +	se_tpg->tpg_rtpi = g_tpg_rtpi_counter++;
> +	if (!se_tpg->tpg_rtpi)
> +		goto again;
> +
> +	list_for_each_entry(tpg, &g_tpg_list, tpg_list) {
> +		/*
> +		 * Make sure RELATIVE TARGET PORT IDENTIFIER is unique
> +		 * for 16-bit wrap..
> +		 */
> +		if (se_tpg->tpg_rtpi == tpg->tpg_rtpi)
> +			goto again;
> +	}
> +	list_add(&se_tpg->tpg_list, &g_tpg_list);
> +	g_tpg_count++;
> +	spin_unlock(&g_tpg_lock);
> +

I think you could just use an ida.

Re: [PATCH 2/7] scsi: target: core: Add RTPI field to target port

[Mike Christie]

On 9/29/22 5:26 PM, Mike Christie wrote:
> 
> I think you could just use an ida.
>

After seeing the other patches I guess you need a idr.

Re: [PATCH 7/7] target: core: check RTPI uniquity for enabled TPG

[Mike Christie]

On 9/6/22 10:45 AM, Dmitry Bogdanov wrote:
> Garantee uniquity of RTPI only for enabled target port groups.
> Allow any RPTI for disabled tpg until it is enabled.
> 
> Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
> ---
>  drivers/target/target_core_fabric_configfs.c | 29 +++++++++++++-
>  drivers/target/target_core_internal.h        |  2 +
>  drivers/target/target_core_tpg.c             | 40 +++++++++++++-------
>  3 files changed, 56 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/target/target_core_fabric_configfs.c
> index a34b5db4eec5..fc1b8f54fb54 100644
> --- a/drivers/target/target_core_fabric_configfs.c
> +++ b/drivers/target/target_core_fabric_configfs.c
> @@ -857,6 +857,7 @@ static ssize_t target_fabric_tpg_base_enable_store(struct config_item *item,
>  						   size_t count)
>  {
>  	struct se_portal_group *se_tpg = to_tpg(item);
> +	struct se_portal_group *tpg;
>  	int ret;
>  	bool op;
>  
> @@ -867,11 +868,37 @@ static ssize_t target_fabric_tpg_base_enable_store(struct config_item *item,
>  	if (se_tpg->enabled == op)
>  		return count;
>  
> +	spin_lock(&g_tpg_lock);
> +
> +	if (op) {
> +		tpg = core_get_tpg_by_rtpi(se_tpg->tpg_rtpi);
> +		if (tpg) {
> +			spin_unlock(&g_tpg_lock);
> +
> +			pr_err("TARGET_CORE[%s]->TPG[%u] - RTPI %#x conflicts with TARGET_CORE[%s]->TPG[%u]\n",
> +			       se_tpg->se_tpg_tfo->fabric_name,
> +			       se_tpg->se_tpg_tfo->tpg_get_tag(tpg),
> +			       se_tpg->tpg_rtpi,
> +			       tpg->se_tpg_tfo->fabric_name,
> +			       tpg->se_tpg_tfo->tpg_get_tag(tpg));
> +			return -EINVAL;
> +		}
> +	}
> +
> +	se_tpg->enabled |= 0x10; /* transient state */

Just use a mutex and hold it the entire time if you can and
drop this. Or add a proper enum/define for the states and make a real
API since this is exported to userspace, and it's going to be difficult
to explain to users that state.


> +	spin_unlock(&g_tpg_lock);
> +
>  	ret = se_tpg->se_tpg_tfo->fabric_enable_tpg(se_tpg, op);
> -	if (ret)
> +
> +	spin_lock(&g_tpg_lock);
> +	if (ret < 0) {
> +		se_tpg->enabled	&= ~0x10; /* clear transient state */
> +		spin_unlock(&g_tpg_lock);
>  		return ret;
> +	}
>  
>  	se_tpg->enabled = op;
> +	spin_unlock(&g_tpg_lock);
>  
>  	return count;
>  }
> diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h
> index d662cdc9a04c..d4ace697edb0 100644
> --- a/drivers/target/target_core_internal.h
> +++ b/drivers/target/target_core_internal.h
> @@ -116,6 +116,7 @@ int	core_tmr_lun_reset(struct se_device *, struct se_tmr_req *,
>  		struct list_head *, struct se_cmd *);
>  
>  /* target_core_tpg.c */
> +extern struct spinlock g_tpg_lock;
>  extern struct se_device *g_lun0_dev;
>  extern struct configfs_attribute *core_tpg_attrib_attrs[];
>  
> @@ -131,6 +132,7 @@ void core_tpg_remove_lun(struct se_portal_group *, struct se_lun *);
>  struct se_node_acl *core_tpg_add_initiator_node_acl(struct se_portal_group *tpg,
>  		const char *initiatorname);
>  void core_tpg_del_initiator_node_acl(struct se_node_acl *acl);
> +struct se_portal_group *core_get_tpg_by_rtpi(u16 rtpi);
>  
>  /* target_core_transport.c */
>  extern struct kmem_cache *se_tmr_req_cache;
> diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
> index 85c9473a38fd..ef2adbe628e0 100644
> --- a/drivers/target/target_core_tpg.c
> +++ b/drivers/target/target_core_tpg.c
> @@ -34,7 +34,7 @@ extern struct se_device *g_lun0_dev;
>  static u16 g_tpg_count;
>  static u16 g_tpg_rtpi_counter = 1;
>  static LIST_HEAD(g_tpg_list);
> -static DEFINE_SPINLOCK(g_tpg_lock);
> +DEFINE_SPINLOCK(g_tpg_lock);
>  
>  /*	__core_tpg_get_initiator_node_acl():
>   *
> @@ -476,7 +476,7 @@ static int core_tpg_register_rtpi(struct se_portal_group *se_tpg)
>  		 * Make sure RELATIVE TARGET PORT IDENTIFIER is unique
>  		 * for 16-bit wrap..
>  		 */
> -		if (se_tpg->tpg_rtpi == tpg->tpg_rtpi)
> +		if (tpg->enabled && se_tpg->tpg_rtpi == tpg->tpg_rtpi)
>  			goto again;
>  	}
>  	list_add(&se_tpg->tpg_list, &g_tpg_list);
> @@ -738,12 +738,26 @@ static ssize_t core_tpg_rtpi_show(struct config_item *item, char *page)
>  	return sprintf(page, "%#x\n", se_tpg->tpg_rtpi);
>  }
>  
> +struct se_portal_group *
> +core_get_tpg_by_rtpi(u16 rtpi)
> +{
> +	struct se_portal_group *tpg;
> +
> +	lockdep_assert_held(&g_tpg_lock);
> +
> +	list_for_each_entry(tpg, &g_tpg_list, tpg_list) {
> +		if (tpg->enabled && rtpi == tpg->tpg_rtpi)
> +			return tpg;
> +	}
> +
> +	return NULL;
> +}
> +
>  static ssize_t core_tpg_rtpi_store(struct config_item *item,
>  				   const char *page, size_t count)
>  {
>  	struct se_portal_group *se_tpg = attrib_to_tpg(item);
>  	struct se_portal_group *tpg;
> -	bool rtpi_changed = false;
>  	u16 val;
>  	int ret;
>  
> @@ -753,11 +767,14 @@ static ssize_t core_tpg_rtpi_store(struct config_item *item,
>  	if (val == 0)
>  		return -EINVAL;
>  
> -	/* RTPI shouldn't conflict with values of existing ports */
> +	if (se_tpg->tpg_rtpi == val)
> +		return count;

This test above and the chunk at the end looks like it should have been
in patch 6.


> +
>  	spin_lock(&g_tpg_lock);
>  
> -	list_for_each_entry(tpg, &g_tpg_list, tpg_list) {
> -		if (se_tpg != tpg && val == tpg->tpg_rtpi) {
> +	if (se_tpg->enabled) {
> +		tpg = core_get_tpg_by_rtpi(val);
> +		if (tpg) {
>  			spin_unlock(&g_tpg_lock);
>  			pr_err("TARGET_CORE[%s]->TPG[%u] - RTPI %#x conflicts with TARGET_CORE[%s]->TPG[%u]\n",
>  			       se_tpg->se_tpg_tfo->fabric_name,
> @@ -769,17 +786,12 @@ static ssize_t core_tpg_rtpi_store(struct config_item *item,
>  		}
>  	}
>  
> -	if (se_tpg->tpg_rtpi != val) {
> -		se_tpg->tpg_rtpi = val;
> -		rtpi_changed = true;
> -	}
> +	se_tpg->tpg_rtpi = val;
>  	spin_unlock(&g_tpg_lock);
>  
> -	if (rtpi_changed)
> -		core_tpg_ua(se_tpg, 0x3f, ASCQ_3FH_INQUIRY_DATA_HAS_CHANGED);
> -	ret = count;
> +	core_tpg_ua(se_tpg, 0x3f, ASCQ_3FH_INQUIRY_DATA_HAS_CHANGED);
>  
> -	return ret;
> +	return count;
>  }
>  
>  CONFIGFS_ATTR(core_tpg_, rtpi);

Re: [PATCH 6/7] scsi: target: core: Add RTPI attribute for target port

[Mike Christie]

On 9/6/22 10:45 AM, Dmitry Bogdanov wrote:
> +
> +static ssize_t core_tpg_rtpi_store(struct config_item *item,
> +				   const char *page, size_t count)
> +{
> +	struct se_portal_group *se_tpg = attrib_to_tpg(item);
> +	struct se_portal_group *tpg;
> +	bool rtpi_changed = false;
> +	u16 val;
> +	int ret;
> +
> +	ret = kstrtou16(page, 0, &val);
> +	if (ret < 0)
> +		return ret;
> +	if (val == 0)
> +		return -EINVAL;
> +
> +	/* RTPI shouldn't conflict with values of existing ports */
> +	spin_lock(&g_tpg_lock);
> +
> +	list_for_each_entry(tpg, &g_tpg_list, tpg_list) {
> +		if (se_tpg != tpg && val == tpg->tpg_rtpi) {
> +			spin_unlock(&g_tpg_lock);
> +			pr_err("TARGET_CORE[%s]->TPG[%u] - RTPI %#x conflicts with TARGET_CORE[%s]->TPG[%u]\n",
> +			       se_tpg->se_tpg_tfo->fabric_name,
> +			       se_tpg->se_tpg_tfo->tpg_get_tag(tpg),
> +			       val,
> +			       tpg->se_tpg_tfo->fabric_name,
> +			       tpg->se_tpg_tfo->tpg_get_tag(tpg));
> +			return -EINVAL;
> +		}
> +	}
> +
> +	if (se_tpg->tpg_rtpi != val) {
> +		se_tpg->tpg_rtpi = val;
> +		rtpi_changed = true;
> +	}
> +	spin_unlock(&g_tpg_lock);
> +
> +	if (rtpi_changed)
> +		core_tpg_ua(se_tpg, 0x3f, ASCQ_3FH_INQUIRY_DATA_HAS_CHANGED);
> +	ret = count;
> +
> +	return ret;

Just return count.

Re: [PATCH 7/7] target: core: check RTPI uniquity for enabled TPG

[michael.christie]

On 9/29/22 7:02 PM, Mike Christie wrote:
> On 9/6/22 10:45 AM, Dmitry Bogdanov wrote:
>> Garantee uniquity of RTPI only for enabled target port groups.
>> Allow any RPTI for disabled tpg until it is enabled.
>>
>> Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
>> ---
>>  drivers/target/target_core_fabric_configfs.c | 29 +++++++++++++-
>>  drivers/target/target_core_internal.h        |  2 +
>>  drivers/target/target_core_tpg.c             | 40 +++++++++++++-------
>>  3 files changed, 56 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/target/target_core_fabric_configfs.c
>> index a34b5db4eec5..fc1b8f54fb54 100644
>> --- a/drivers/target/target_core_fabric_configfs.c
>> +++ b/drivers/target/target_core_fabric_configfs.c
>> @@ -857,6 +857,7 @@ static ssize_t target_fabric_tpg_base_enable_store(struct config_item *item,
>>  						   size_t count)
>>  {
>>  	struct se_portal_group *se_tpg = to_tpg(item);
>> +	struct se_portal_group *tpg;
>>  	int ret;
>>  	bool op;
>>  
>> @@ -867,11 +868,37 @@ static ssize_t target_fabric_tpg_base_enable_store(struct config_item *item,
>>  	if (se_tpg->enabled == op)
>>  		return count;
>>  
>> +	spin_lock(&g_tpg_lock);
>> +
>> +	if (op) {
>> +		tpg = core_get_tpg_by_rtpi(se_tpg->tpg_rtpi);
>> +		if (tpg) {
>> +			spin_unlock(&g_tpg_lock);
>> +
>> +			pr_err("TARGET_CORE[%s]->TPG[%u] - RTPI %#x conflicts with TARGET_CORE[%s]->TPG[%u]\n",
>> +			       se_tpg->se_tpg_tfo->fabric_name,
>> +			       se_tpg->se_tpg_tfo->tpg_get_tag(tpg),
>> +			       se_tpg->tpg_rtpi,
>> +			       tpg->se_tpg_tfo->fabric_name,
>> +			       tpg->se_tpg_tfo->tpg_get_tag(tpg));
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>> +	se_tpg->enabled |= 0x10; /* transient state */
> 
> Just use a mutex and hold it the entire time if 
I was looking at the configfs code and am now not sure what the transient state
is for. It looks like when doing a read or write configfs holds the buffer->mutex,
so I don't think userspace would ever see the transient state.

Can you just drop it?

Re: [PATCH 2/7] scsi: target: core: Add RTPI field to target port

[Dmitry Bogdanov]

On Thu, Sep 29, 2022 at 06:57:03PM -0500, Mike Christie wrote:
> 
> On 9/29/22 5:26 PM, Mike Christie wrote:
> >
> > I think you could just use an ida.
> >
> 
> After seeing the other patches I guess you need a idr.
> 

idr is deprecated, I looked at xarray. Looks good, I will rewrite
the patches using it. 

Thanks!

Re: [PATCH 6/7] scsi: target: core: Add RTPI attribute for target port

[Dmitry Bogdanov]

On Thu, Sep 29, 2022 at 07:03:52PM -0500, Mike Christie wrote:
> 
> On 9/6/22 10:45 AM, Dmitry Bogdanov wrote:
> > +
> > +static ssize_t core_tpg_rtpi_store(struct config_item *item,
> > +                                const char *page, size_t count)
> > +{
> > +     struct se_portal_group *se_tpg = attrib_to_tpg(item);
> > +     struct se_portal_group *tpg;
> > +     bool rtpi_changed = false;
> > +     u16 val;
> > +     int ret;
> > +
> > +     ret = kstrtou16(page, 0, &val);
> > +     if (ret < 0)
> > +             return ret;
> > +     if (val == 0)
> > +             return -EINVAL;
> > +
> > +     /* RTPI shouldn't conflict with values of existing ports */
> > +     spin_lock(&g_tpg_lock);
> > +
> > +     list_for_each_entry(tpg, &g_tpg_list, tpg_list) {
> > +             if (se_tpg != tpg && val == tpg->tpg_rtpi) {
> > +                     spin_unlock(&g_tpg_lock);
> > +                     pr_err("TARGET_CORE[%s]->TPG[%u] - RTPI %#x conflicts with TARGET_CORE[%s]->TPG[%u]\n",
> > +                            se_tpg->se_tpg_tfo->fabric_name,
> > +                            se_tpg->se_tpg_tfo->tpg_get_tag(tpg),
> > +                            val,
> > +                            tpg->se_tpg_tfo->fabric_name,
> > +                            tpg->se_tpg_tfo->tpg_get_tag(tpg));
> > +                     return -EINVAL;
> > +             }
> > +     }
> > +
> > +     if (se_tpg->tpg_rtpi != val) {
> > +             se_tpg->tpg_rtpi = val;
> > +             rtpi_changed = true;
> > +     }
> > +     spin_unlock(&g_tpg_lock);
> > +
> > +     if (rtpi_changed)
> > +             core_tpg_ua(se_tpg, 0x3f, ASCQ_3FH_INQUIRY_DATA_HAS_CHANGED);
> > +     ret = count;
> > +
> > +     return ret;
> 
> Just return count.

Yes, will do.

Re: [PATCH 7/7] target: core: check RTPI uniquity for enabled TPG

[Dmitry Bogdanov]

On Thu, Sep 29, 2022 at 07:02:12PM -0500, Mike Christie wrote:
> 
> On 9/6/22 10:45 AM, Dmitry Bogdanov wrote:
> > Garantee uniquity of RTPI only for enabled target port groups.
> > Allow any RPTI for disabled tpg until it is enabled.
> >
> > Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
> > ---
> >  drivers/target/target_core_fabric_configfs.c | 29 +++++++++++++-
> >  drivers/target/target_core_internal.h        |  2 +
> >  drivers/target/target_core_tpg.c             | 40 +++++++++++++-------
> >  3 files changed, 56 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/target/target_core_fabric_configfs.c
> > index a34b5db4eec5..fc1b8f54fb54 100644
> > --- a/drivers/target/target_core_fabric_configfs.c
> > +++ b/drivers/target/target_core_fabric_configfs.c
> > @@ -857,6 +857,7 @@ static ssize_t target_fabric_tpg_base_enable_store(struct config_item *item,
> >                                                  size_t count)
> >  {
> >       struct se_portal_group *se_tpg = to_tpg(item);
> > +     struct se_portal_group *tpg;
> >       int ret;
> >       bool op;
> >
> > @@ -867,11 +868,37 @@ static ssize_t target_fabric_tpg_base_enable_store(struct config_item *item,
> >       if (se_tpg->enabled == op)
> >               return count;
> >
> > +     spin_lock(&g_tpg_lock);
> > +
> > +     if (op) {
> > +             tpg = core_get_tpg_by_rtpi(se_tpg->tpg_rtpi);
> > +             if (tpg) {
> > +                     spin_unlock(&g_tpg_lock);
> > +
> > +                     pr_err("TARGET_CORE[%s]->TPG[%u] - RTPI %#x conflicts with TARGET_CORE[%s]->TPG[%u]\n",
> > +                            se_tpg->se_tpg_tfo->fabric_name,
> > +                            se_tpg->se_tpg_tfo->tpg_get_tag(tpg),
> > +                            se_tpg->tpg_rtpi,
> > +                            tpg->se_tpg_tfo->fabric_name,
> > +                            tpg->se_tpg_tfo->tpg_get_tag(tpg));
> > +                     return -EINVAL;
> > +             }
> > +     }
> > +
> > +     se_tpg->enabled |= 0x10; /* transient state */
> 
> Just use a mutex and hold it the entire time if you can and
> drop this. Or add a proper enum/define for the states and make a real
> API since this is exported to userspace, and it's going to be difficult
> to explain to users that state.

Yes, it looks wierd.
After rewriting to xarray I decided (according to SAM-5) to not allow
to change RTPI on the enabled TPG. And that part will be dropped.
4.6.5.2 Relative Port Identifier attribute
  The relative port identifier for a SCSI port shall not change once
  assigned unless reconfiguration of the SCSI target device occurs.
> 
> > +     spin_unlock(&g_tpg_lock);
> > +
> >       ret = se_tpg->se_tpg_tfo->fabric_enable_tpg(se_tpg, op);
> > -     if (ret)
> > +
> > +     spin_lock(&g_tpg_lock);
> > +     if (ret < 0) {
> > +             se_tpg->enabled &= ~0x10; /* clear transient state */
> > +             spin_unlock(&g_tpg_lock);
> >               return ret;
> > +     }
> >
> >       se_tpg->enabled = op;
> > +     spin_unlock(&g_tpg_lock);
> >
> >       return count;
> >  }
> > diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h
> > index d662cdc9a04c..d4ace697edb0 100644
> > --- a/drivers/target/target_core_internal.h
> > +++ b/drivers/target/target_core_internal.h
> > @@ -116,6 +116,7 @@ int       core_tmr_lun_reset(struct se_device *, struct se_tmr_req *,
> >               struct list_head *, struct se_cmd *);
> >
> >  /* target_core_tpg.c */
> > +extern struct spinlock g_tpg_lock;
> >  extern struct se_device *g_lun0_dev;
> >  extern struct configfs_attribute *core_tpg_attrib_attrs[];
> >
> > @@ -131,6 +132,7 @@ void core_tpg_remove_lun(struct se_portal_group *, struct se_lun *);
> >  struct se_node_acl *core_tpg_add_initiator_node_acl(struct se_portal_group *tpg,
> >               const char *initiatorname);
> >  void core_tpg_del_initiator_node_acl(struct se_node_acl *acl);
> > +struct se_portal_group *core_get_tpg_by_rtpi(u16 rtpi);
> >
> >  /* target_core_transport.c */
> >  extern struct kmem_cache *se_tmr_req_cache;
> > diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
> > index 85c9473a38fd..ef2adbe628e0 100644
> > --- a/drivers/target/target_core_tpg.c
> > +++ b/drivers/target/target_core_tpg.c
> > @@ -34,7 +34,7 @@ extern struct se_device *g_lun0_dev;
> >  static u16 g_tpg_count;
> >  static u16 g_tpg_rtpi_counter = 1;
> >  static LIST_HEAD(g_tpg_list);
> > -static DEFINE_SPINLOCK(g_tpg_lock);
> > +DEFINE_SPINLOCK(g_tpg_lock);
> >
> >  /*   __core_tpg_get_initiator_node_acl():
> >   *
> > @@ -476,7 +476,7 @@ static int core_tpg_register_rtpi(struct se_portal_group *se_tpg)
> >                * Make sure RELATIVE TARGET PORT IDENTIFIER is unique
> >                * for 16-bit wrap..
> >                */
> > -             if (se_tpg->tpg_rtpi == tpg->tpg_rtpi)
> > +             if (tpg->enabled && se_tpg->tpg_rtpi == tpg->tpg_rtpi)
> >                       goto again;
> >       }
> >       list_add(&se_tpg->tpg_list, &g_tpg_list);
> > @@ -738,12 +738,26 @@ static ssize_t core_tpg_rtpi_show(struct config_item *item, char *page)
> >       return sprintf(page, "%#x\n", se_tpg->tpg_rtpi);
> >  }
> >
> > +struct se_portal_group *
> > +core_get_tpg_by_rtpi(u16 rtpi)
> > +{
> > +     struct se_portal_group *tpg;
> > +
> > +     lockdep_assert_held(&g_tpg_lock);
> > +
> > +     list_for_each_entry(tpg, &g_tpg_list, tpg_list) {
> > +             if (tpg->enabled && rtpi == tpg->tpg_rtpi)
> > +                     return tpg;
> > +     }
> > +
> > +     return NULL;
> > +}
> > +
> >  static ssize_t core_tpg_rtpi_store(struct config_item *item,
> >                                  const char *page, size_t count)
> >  {
> >       struct se_portal_group *se_tpg = attrib_to_tpg(item);
> >       struct se_portal_group *tpg;
> > -     bool rtpi_changed = false;
> >       u16 val;
> >       int ret;
> >
> > @@ -753,11 +767,14 @@ static ssize_t core_tpg_rtpi_store(struct config_item *item,
> >       if (val == 0)
> >               return -EINVAL;
> >
> > -     /* RTPI shouldn't conflict with values of existing ports */
> > +     if (se_tpg->tpg_rtpi == val)
> > +             return count;
> 
> This test above and the chunk at the end looks like it should have been
> in patch 6.
> 
This patch will be completely rewritten.

> > +
> >       spin_lock(&g_tpg_lock);
> >
> > -     list_for_each_entry(tpg, &g_tpg_list, tpg_list) {
> > -             if (se_tpg != tpg && val == tpg->tpg_rtpi) {
> > +     if (se_tpg->enabled) {
> > +             tpg = core_get_tpg_by_rtpi(val);
> > +             if (tpg) {
> >                       spin_unlock(&g_tpg_lock);
> >                       pr_err("TARGET_CORE[%s]->TPG[%u] - RTPI %#x conflicts with TARGET_CORE[%s]->TPG[%u]\n",
> >                              se_tpg->se_tpg_tfo->fabric_name,
> > @@ -769,17 +786,12 @@ static ssize_t core_tpg_rtpi_store(struct config_item *item,
> >               }
> >       }
> >
> > -     if (se_tpg->tpg_rtpi != val) {
> > -             se_tpg->tpg_rtpi = val;
> > -             rtpi_changed = true;
> > -     }
> > +     se_tpg->tpg_rtpi = val;
> >       spin_unlock(&g_tpg_lock);
> >
> > -     if (rtpi_changed)
> > -             core_tpg_ua(se_tpg, 0x3f, ASCQ_3FH_INQUIRY_DATA_HAS_CHANGED);
> > -     ret = count;
> > +     core_tpg_ua(se_tpg, 0x3f, ASCQ_3FH_INQUIRY_DATA_HAS_CHANGED);
> >
> > -     return ret;
> > +     return count;
> >  }
> >
> >  CONFIGFS_ATTR(core_tpg_, rtpi);
> 

Re: [PATCH 7/7] target: core: check RTPI uniquity for enabled TPG

[Dmitry Bogdanov]

On Sat, Oct 01, 2022 at 11:19:45AM -0500, michael.christie@oracle.com wrote:
> 
> On 9/29/22 7:02 PM, Mike Christie wrote:
> > On 9/6/22 10:45 AM, Dmitry Bogdanov wrote:
> >> Garantee uniquity of RTPI only for enabled target port groups.
> >> Allow any RPTI for disabled tpg until it is enabled.
> >>
> >> Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
> >> ---
> >>  drivers/target/target_core_fabric_configfs.c | 29 +++++++++++++-
> >>  drivers/target/target_core_internal.h        |  2 +
> >>  drivers/target/target_core_tpg.c             | 40 +++++++++++++-------
> >>  3 files changed, 56 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/target/target_core_fabric_configfs.c
> >> index a34b5db4eec5..fc1b8f54fb54 100644
> >> --- a/drivers/target/target_core_fabric_configfs.c
> >> +++ b/drivers/target/target_core_fabric_configfs.c
> >> @@ -857,6 +857,7 @@ static ssize_t target_fabric_tpg_base_enable_store(struct config_item *item,
> >>                                                 size_t count)
> >>  {
> >>      struct se_portal_group *se_tpg = to_tpg(item);
> >> +    struct se_portal_group *tpg;
> >>      int ret;
> >>      bool op;
> >>
> >> @@ -867,11 +868,37 @@ static ssize_t target_fabric_tpg_base_enable_store(struct config_item *item,
> >>      if (se_tpg->enabled == op)
> >>              return count;
> >>
> >> +    spin_lock(&g_tpg_lock);
> >> +
> >> +    if (op) {
> >> +            tpg = core_get_tpg_by_rtpi(se_tpg->tpg_rtpi);
> >> +            if (tpg) {
> >> +                    spin_unlock(&g_tpg_lock);
> >> +
> >> +                    pr_err("TARGET_CORE[%s]->TPG[%u] - RTPI %#x conflicts with TARGET_CORE[%s]->TPG[%u]\n",
> >> +                           se_tpg->se_tpg_tfo->fabric_name,
> >> +                           se_tpg->se_tpg_tfo->tpg_get_tag(tpg),
> >> +                           se_tpg->tpg_rtpi,
> >> +                           tpg->se_tpg_tfo->fabric_name,
> >> +                           tpg->se_tpg_tfo->tpg_get_tag(tpg));
> >> +                    return -EINVAL;
> >> +            }
> >> +    }
> >> +
> >> +    se_tpg->enabled |= 0x10; /* transient state */
> >
> > Just use a mutex and hold it the entire time if
> I was looking at the configfs code and am now not sure what the transient state
> is for. It looks like when doing a read or write configfs holds the buffer->mutex,
> so I don't think userspace would ever see the transient state.
> 
> Can you just drop it?

sysfs lock is per file, so 'enable' and 'rtpi' files can be written in
parallel. But after rewriting of the patchset, this part will be
dropped.