diff mbox series

[v2,4/5] scsi: target: core: Add common port attributes

Message ID 20221006105057.30184-5-d.bogdanov@yadro.com
State Superseded
Headers show
Series scsi: target: make RTPI an TPG identifier | expand

Commit Message

Dmitry Bogdanov Oct. 6, 2022, 10:50 a.m. UTC 
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(-)

Comments

'Christoph Hellwig' Oct. 17, 2022, 7:42 a.m. UTC | #1 
On Thu, Oct 06, 2022 at 01:50:56PM +0300, Dmitry Bogdanov wrote:
> 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.

But does it really buy you much?  There is quite a lot of boilerplate
code vs just adding an attribute to an array in the drivers, which could
be further simplified by having a macro for all common ones.
Dmitry Bogdanov Oct. 19, 2022, 3:13 p.m. UTC | #2 
On Mon, Oct 17, 2022 at 12:42:16AM -0700, Christoph Hellwig wrote:
> 
> On Thu, Oct 06, 2022 at 01:50:56PM +0300, Dmitry Bogdanov wrote:
> > 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.
> 
> But does it really buy you much?  There is quite a lot of boilerplate
> code vs just adding an attribute to an array in the drivers, which could
> be further simplified by having a macro for all common ones.

Yes, of course. Mostly it is not about deduplication, but for just a
logical place of the attribute. Without this patch the 5th patch would
be cloned for every fabric driver. Moreover it would look strange to
have a some private core logic (check enableness) in fabric driver.

BR,
 Dmitry
diff mbox series

Patch

diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index 416514c5c7ac..ca728852e2bb 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 c3a41d4d646b..810e8abc0b00 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);
 
@@ -1110,7 +1141,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 4699c078f6f4..06d87008615f 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 5838cd2d8a96..25ffc3d7a741 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -681,3 +681,7 @@  void core_tpg_remove_lun(
 
 	percpu_ref_exit(&lun->lun_ref);
 }
+
+struct configfs_attribute *core_tpg_attrib_attrs[] = {
+	NULL,
+};