On 3/22/21 3:05 AM, Dmitry Bogdanov wrote:
> Many fabric modules provide their own implementation of enable
> attribute in tpg.
> The change provides a way to remove code duplication in the fabric
> modules and automatically add "enable" attribute if a fabric module has
> an implementation of fabric_enable_tpg() ops.
>
> Reviewed-by: Roman Bolshakov <r.bolshakov@xxxxxxxxx>
> Signed-off-by: Dmitry Bogdanov <d.bogdanov@xxxxxxxxx>
> ---
> v2:
> create enable atribute only for modules with enable_tpg ops
>
> drivers/target/target_core_configfs.c | 1 +
> drivers/target/target_core_fabric_configfs.c | 40 ++++++++++++++++++-
> drivers/target/target_core_internal.h | 1 +
> drivers/target/target_core_tpg.c | 41 ++++++++++++++++++++
> include/target/target_core_base.h | 1 +
> include/target/target_core_fabric.h | 1 +
> 6 files changed, 83 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
> index f04352285155..fc3949e91f9c 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -490,6 +490,7 @@ void target_unregister_template(const struct target_core_fabric_ops *fo)
> * fabric driver unload of TFO->module to proceed.
> */
> rcu_barrier();
> + kfree(t->tf_tpg_base_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 ee85602213f7..c5e1b0d54330 100644
> --- a/drivers/target/target_core_fabric_configfs.c
> +++ b/drivers/target/target_core_fabric_configfs.c
> @@ -815,8 +815,40 @@ static struct configfs_item_operations target_fabric_tpg_base_item_ops = {
> .release = target_fabric_tpg_release,
> };
>
> -TF_CIT_SETUP_DRV(tpg_base, &target_fabric_tpg_base_item_ops, NULL);
> +static int
> +target_fabric_setup_tpg_base_cit(struct target_fabric_configfs *tf)
> +{
> + int i, k, len = 0;
> + struct config_item_type *cit = &tf->tf_tpg_base_cit;
> + struct configfs_attribute **attrs;
> +
> + if (tf->tf_ops->fabric_enable_tpg)
> + for (i = 0; core_tpg_base_enable_attrs[i]; i++)
> + len += sizeof(struct configfs_attribute *);
> + if (tf->tf_ops->tfc_tpg_base_attrs)
> + for (i = 0; tf->tf_ops->tfc_tpg_base_attrs[i]; i++)
> + len += sizeof(struct configfs_attribute *);
> + len += sizeof(struct configfs_attribute *);
> +
> + attrs = kzalloc(len, GFP_KERNEL);
> + if (!attrs)
> + return -ENOMEM;
> +
> + if (tf->tf_ops->fabric_enable_tpg)
> + for (i = 0; core_tpg_base_enable_attrs[i]; i++)
> + attrs[i] = core_tpg_base_enable_attrs[i];
> + if (tf->tf_ops->tfc_tpg_base_attrs)
> + for (k = 0; tf->tf_ops->tfc_tpg_base_attrs[k]; k++, i++)
If fabric_enable_tpg is not set then I think i is the value from above
when we did the tfc_tpg_base_attrs loop to calculate the total len needed
for the kzalloc.
> + attrs[i] = tf->tf_ops->tfc_tpg_base_attrs[k];
> + attrs[i] = NULL;
If fabric_enable_tpg and tfc_tpg_base_attrs attrs is not set then i is
not initialized.
I'm guessing you guys do more testing with tcm_qla. Maybe also do a quick
target creation test with iscsi and loop to hit some of the different combos.
> +
> + cit->ct_item_ops = &target_fabric_tpg_base_item_ops;
> + cit->ct_attrs = attrs;
> + cit->ct_owner = tf->tf_ops->module;
> + pr_debug("Setup generic tpg_base\n");
>
> + return 0;
> +}
> /* End of tfc_tpg_base_cit */
>
> /* Start of tfc_tpg_cit */
> @@ -971,11 +1003,15 @@ TF_CIT_SETUP_DRV(discovery, NULL, NULL);
>
> int target_fabric_setup_cits(struct target_fabric_configfs *tf)
> {
> + int ret;
> +
> target_fabric_setup_discovery_cit(tf);
> target_fabric_setup_wwn_cit(tf);
> target_fabric_setup_wwn_fabric_stats_cit(tf);
> target_fabric_setup_tpg_cit(tf);
> - target_fabric_setup_tpg_base_cit(tf);
Add newline
> + ret = target_fabric_setup_tpg_base_cit(tf);
> + if (ret)
> + return ret;
Add newline
> target_fabric_setup_tpg_port_cit(tf);
> target_fabric_setup_tpg_port_stat_cit(tf);
> target_fabric_setup_tpg_lun_cit(tf);
> diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h
> index e7b3c6e5d574..274fd670bf9c 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_base_enable_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 736847c933e5..d06b3fbd8511 100644
> --- a/drivers/target/target_core_tpg.c
> +++ b/drivers/target/target_core_tpg.c
> @@ -650,3 +650,44 @@ void core_tpg_remove_lun(
>
> percpu_ref_exit(&lun->lun_ref);
> }
> +
Delete extra newline.
> +
> +static ssize_t core_tpg_base_enable_show(struct config_item *item, char *page)
> +{
> + return sysfs_emit(page, "%d\n", to_tpg(item)->enabled);
> +}
> +
> +static ssize_t core_tpg_base_enable_store(struct config_item *item,
> + const char *page, size_t count)
> +{
> + struct se_portal_group *se_tpg = to_tpg(item);
> + int ret;
> + u32 op;
> +
> + ret = kstrtou32(page, 0, &op);
> + if (ret)
> + return ret;
Add a newline.
> + if ((op != 1) && (op != 0)) {
> + pr_err("Illegal value for tpg_enable: %u\n", op);
> + return -EINVAL;
> + }
> +
> + if (se_tpg->enabled == op)
> + return count;
> +
> + ret = se_tpg->se_tpg_tfo->fabric_enable_tpg(se_tpg, op);
> +
Delete extra newline.
> + if (ret)
> + return ret;
> +
> + se_tpg->enabled = op;
> +
> + return count;
> +}
