diff mbox series

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

Message ID 20220906154519.27487-8-d.bogdanov@yadro.com
State New
Headers show
Series scsi: target: make RTPI an TPG identifier | expand

Commit Message

Dmitry Bogdanov Sept. 6, 2022, 3:45 p.m. UTC
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(-)

Comments

Mike Christie Sept. 30, 2022, 12:02 a.m. UTC | #1
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);
Mike Christie Oct. 1, 2022, 4:19 p.m. UTC | #2
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?
Dmitry Bogdanov Oct. 4, 2022, 4:37 p.m. UTC | #3
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);
>
Dmitry Bogdanov Oct. 4, 2022, 4:41 p.m. UTC | #4
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.
diff mbox series

Patch

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