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@xxxxxxxxx>
> ---
> 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);
