The reference counting code can be simplified. Instead taking a tgtport
refrerence at the beginning of nvmet_fc_alloc_hostport and put it back
if not a new hostport object is allocated, only take it when a new
hostport object is allocated.

Signed-off-by: Daniel Wagner <wagi@kernel.org>
---
 drivers/nvme/target/fc.c | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
index b807b4c05cac7fe4764df3df76f8fa50f4bab6ba..391917b4ce0115dbc0ad99d1fb363b1af6ee0685 100644
--- a/drivers/nvme/target/fc.c
+++ b/drivers/nvme/target/fc.c
@@ -1046,29 +1046,16 @@ nvmet_fc_alloc_hostport(struct nvmet_fc_tgtport *tgtport, void *hosthandle)
 	if (!hosthandle)
 		return NULL;
 
-	/*
-	 * take reference for what will be the newly allocated hostport if
-	 * we end up using a new allocation
-	 */
-	if (!nvmet_fc_tgtport_get(tgtport))
-		return ERR_PTR(-EINVAL);
-
 	spin_lock_irqsave(&tgtport->lock, flags);
 	match = nvmet_fc_match_hostport(tgtport, hosthandle);
 	spin_unlock_irqrestore(&tgtport->lock, flags);
 
-	if (match) {
-		/* no new allocation - release reference */
-		nvmet_fc_tgtport_put(tgtport);
+	if (match)
 		return match;
-	}
 
 	newhost = kzalloc(sizeof(*newhost), GFP_KERNEL);
-	if (!newhost) {
-		/* no new allocation - release reference */
-		nvmet_fc_tgtport_put(tgtport);
+	if (!newhost)
 		return ERR_PTR(-ENOMEM);
-	}
 
 	spin_lock_irqsave(&tgtport->lock, flags);
 	match = nvmet_fc_match_hostport(tgtport, hosthandle);
@@ -1077,6 +1064,7 @@ nvmet_fc_alloc_hostport(struct nvmet_fc_tgtport *tgtport, void *hosthandle)
 		kfree(newhost);
 		newhost = match;
 	} else {
+		nvmet_fc_tgtport_get(tgtport);
 		newhost->tgtport = tgtport;
 		newhost->hosthandle = hosthandle;
 		INIT_LIST_HEAD(&newhost->host_list);

-- 
2.48.1

On 2/26/25 19:46, Daniel Wagner wrote:
> The reference counting code can be simplified. Instead taking a tgtport
> refrerence at the beginning of nvmet_fc_alloc_hostport and put it back
> if not a new hostport object is allocated, only take it when a new
> hostport object is allocated.
> 
Can it really?
Main point of this operation is that 'tgtport' isn't going away during 
while we're figuring out whether we need it.

With this patch it means that

> Signed-off-by: Daniel Wagner <wagi@kernel.org>
> ---
>   drivers/nvme/target/fc.c | 18 +++---------------
>   1 file changed, 3 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
> index b807b4c05cac7fe4764df3df76f8fa50f4bab6ba..391917b4ce0115dbc0ad99d1fb363b1af6ee0685 100644
> --- a/drivers/nvme/target/fc.c
> +++ b/drivers/nvme/target/fc.c
> @@ -1046,29 +1046,16 @@ nvmet_fc_alloc_hostport(struct nvmet_fc_tgtport *tgtport, void *hosthandle)
>   	if (!hosthandle)
>   		return NULL;
>   
> -	/*
> -	 * take reference for what will be the newly allocated hostport if
> -	 * we end up using a new allocation
> -	 */
> -	if (!nvmet_fc_tgtport_get(tgtport))
> -		return ERR_PTR(-EINVAL);
> -
 >   	spin_lock_irqsave(&tgtport->lock, flags);>   	match = 
nvmet_fc_match_hostport(tgtport, hosthandle);
>   	spin_unlock_irqrestore(&tgtport->lock, flags);
>   

'tgtport' might be invalid here, causing a crash when taking the lock.

> -	if (match) {
> -		/* no new allocation - release reference */
> -		nvmet_fc_tgtport_put(tgtport);
> +	if (match)
>   		return match;
> -	}
>   
>   	newhost = kzalloc(sizeof(*newhost), GFP_KERNEL);
> -	if (!newhost) {
> -		/* no new allocation - release reference */
> -		nvmet_fc_tgtport_put(tgtport);
> +	if (!newhost)
>   		return ERR_PTR(-ENOMEM);
> -	}
>   
>   	spin_lock_irqsave(&tgtport->lock, flags);
>   	match = nvmet_fc_match_hostport(tgtport, hosthandle);
> @@ -1077,6 +1064,7 @@ nvmet_fc_alloc_hostport(struct nvmet_fc_tgtport *tgtport, void *hosthandle)
>   		kfree(newhost);
>   		newhost = match;
>   	} else {
> +		nvmet_fc_tgtport_get(tgtport);
>   		newhost->tgtport = tgtport;
>   		newhost->hosthandle = hosthandle;
>   		INIT_LIST_HEAD(&newhost->host_list);
> 

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

On Fri, Feb 28, 2025 at 08:34:51AM +0100, Hannes Reinecke wrote:
> On 2/26/25 19:46, Daniel Wagner wrote:
> > The reference counting code can be simplified. Instead taking a tgtport
> > refrerence at the beginning of nvmet_fc_alloc_hostport and put it back
> > if not a new hostport object is allocated, only take it when a new
> > hostport object is allocated.
> > 
> Can it really?
> Main point of this operation is that 'tgtport' isn't going away during while
> we're figuring out whether we need it.
> 
> With this patch it means that

The tgtport is not going away. nvmet_fc_alloc_hostport can only be
called with reference on tgtport being hold:

nvmet_fc_rcv_ls_req
  nvmet_fc_tgtport_get(tgtport)
  nvmet_fc_handle_ls_rqst_work
    nvmet_fc_handle_ls_rqst
      nvmet_fc_ls_create_association
        nvmet_fc_alloc_target_assoc
          nvmet_fc_alloc_hostport

The goal with this patch here is to make it simpler to read where we
take a ref. IMO, there is not really anything gained by the existing
logic, though I agree it's not obvious that the tgtport is not going
away. Would it be okay to add a comment?