The lifetime of the controller is managed by the upper layers. This
ensures that the controller does not go away as long as there are
commands in flight. Thus, there is no need to take references in the
command handling code.
Currently, the refcounting code is partially open-coded for handling
error paths. By moving the cleanup code fully under refcounting and
releasing references when necessary, the error handling can be
simplified.
Signed-off-by: Daniel Wagner <wagi@kernel.org>
---
drivers/nvme/host/fc.c | 102 ++++++++++++++++---------------------------------
1 file changed, 32 insertions(+), 70 deletions(-)
diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index db5dbbf6aee21db45e91ea6f0c122681b9bdaf6f..3e6c79bf0bfd3884867c9ebeb24771323a619934 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -228,6 +228,9 @@ static struct device *fc_udev_device;
static void nvme_fc_complete_rq(struct request *rq);
+static void nvme_fc_ctrl_put(struct nvme_fc_ctrl *);
+static int nvme_fc_ctrl_get(struct nvme_fc_ctrl *);
+
/* *********************** FC-NVME Port Management ************************ */
static void __nvme_fc_delete_hw_queue(struct nvme_fc_ctrl *,
@@ -826,7 +829,7 @@ nvme_fc_unregister_remoteport(struct nvme_fc_remote_port *portptr)
dev_warn(ctrl->ctrl.device,
"NVME-FC{%d}: controller connectivity lost.\n",
ctrl->cnum);
- nvme_delete_ctrl(&ctrl->ctrl);
+ nvme_fc_ctrl_put(ctrl);
} else
nvme_fc_ctrl_connectivity_loss(ctrl);
}
@@ -980,9 +983,6 @@ fc_dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
/* *********************** FC-NVME LS Handling **************************** */
-static void nvme_fc_ctrl_put(struct nvme_fc_ctrl *);
-static int nvme_fc_ctrl_get(struct nvme_fc_ctrl *);
-
static void nvme_fc_error_recovery(struct nvme_fc_ctrl *ctrl, char *errmsg);
static void
@@ -1476,8 +1476,6 @@ nvme_fc_match_disconn_ls(struct nvme_fc_rport *rport,
spin_lock_irqsave(&rport->lock, flags);
list_for_each_entry(ctrl, &rport->ctrl_list, ctrl_list) {
- if (!nvme_fc_ctrl_get(ctrl))
- continue;
spin_lock(&ctrl->lock);
if (association_id == ctrl->association_id) {
oldls = ctrl->rcv_disconn;
@@ -1485,10 +1483,6 @@ nvme_fc_match_disconn_ls(struct nvme_fc_rport *rport,
ret = ctrl;
}
spin_unlock(&ctrl->lock);
- if (ret)
- /* leave the ctrl get reference */
- break;
- nvme_fc_ctrl_put(ctrl);
}
spin_unlock_irqrestore(&rport->lock, flags);
@@ -1567,9 +1561,6 @@ nvme_fc_ls_disconnect_assoc(struct nvmefc_ls_rcv_op *lsop)
/* fail the association */
nvme_fc_error_recovery(ctrl, "Disconnect Association LS received");
- /* release the reference taken by nvme_fc_match_disconn_ls() */
- nvme_fc_ctrl_put(ctrl);
-
return false;
}
@@ -2036,7 +2027,6 @@ nvme_fc_fcpio_done(struct nvmefc_fcp_req *req)
__nvme_fc_fcpop_chk_teardowns(ctrl, op, opstate);
atomic_set(&op->state, FCPOP_STATE_IDLE);
op->flags = FCOP_FLAGS_AEN; /* clear other flags */
- nvme_fc_ctrl_put(ctrl);
goto check_error;
}
@@ -2349,37 +2339,18 @@ nvme_fc_init_io_queues(struct nvme_fc_ctrl *ctrl)
}
static void
-nvme_fc_ctrl_free(struct kref *ref)
+nvme_fc_ctrl_delete(struct kref *ref)
{
struct nvme_fc_ctrl *ctrl =
container_of(ref, struct nvme_fc_ctrl, ref);
- unsigned long flags;
-
- if (ctrl->ctrl.tagset)
- nvme_remove_io_tag_set(&ctrl->ctrl);
- /* remove from rport list */
- spin_lock_irqsave(&ctrl->rport->lock, flags);
- list_del(&ctrl->ctrl_list);
- spin_unlock_irqrestore(&ctrl->rport->lock, flags);
-
- nvme_unquiesce_admin_queue(&ctrl->ctrl);
- nvme_remove_admin_tag_set(&ctrl->ctrl);
-
- kfree(ctrl->queues);
-
- put_device(ctrl->dev);
- nvme_fc_rport_put(ctrl->rport);
-
- ida_free(&nvme_fc_ctrl_cnt, ctrl->cnum);
- nvmf_ctrl_options_put(ctrl->ctrl.opts);
- kfree(ctrl);
+ nvme_delete_ctrl(&ctrl->ctrl);
}
static void
nvme_fc_ctrl_put(struct nvme_fc_ctrl *ctrl)
{
- kref_put(&ctrl->ref, nvme_fc_ctrl_free);
+ kref_put(&ctrl->ref, nvme_fc_ctrl_delete);
}
static int
@@ -2397,9 +2368,20 @@ nvme_fc_free_ctrl(struct nvme_ctrl *nctrl)
{
struct nvme_fc_ctrl *ctrl = to_fc_ctrl(nctrl);
- WARN_ON(nctrl != &ctrl->ctrl);
+ if (ctrl->ctrl.tagset)
+ nvme_remove_io_tag_set(&ctrl->ctrl);
+
+ nvme_unquiesce_admin_queue(&ctrl->ctrl);
+ nvme_remove_admin_tag_set(&ctrl->ctrl);
- nvme_fc_ctrl_put(ctrl);
+ kfree(ctrl->queues);
+
+ put_device(ctrl->dev);
+ nvme_fc_rport_put(ctrl->rport);
+
+ ida_free(&nvme_fc_ctrl_cnt, ctrl->cnum);
+ nvmf_ctrl_options_put(ctrl->ctrl.opts);
+ kfree(ctrl);
}
/*
@@ -2649,9 +2631,6 @@ nvme_fc_start_fcp_op(struct nvme_fc_ctrl *ctrl, struct nvme_fc_queue *queue,
if (ctrl->rport->remoteport.port_state != FC_OBJSTATE_ONLINE)
return BLK_STS_RESOURCE;
- if (!nvme_fc_ctrl_get(ctrl))
- return BLK_STS_IOERR;
-
/* format the FC-NVME CMD IU and fcp_req */
cmdiu->connection_id = cpu_to_be64(queue->connection_id);
cmdiu->data_len = cpu_to_be32(data_len);
@@ -2696,7 +2675,6 @@ nvme_fc_start_fcp_op(struct nvme_fc_ctrl *ctrl, struct nvme_fc_queue *queue,
ret = nvme_fc_map_data(ctrl, op->rq, op);
if (ret < 0) {
nvme_cleanup_cmd(op->rq);
- nvme_fc_ctrl_put(ctrl);
if (ret == -ENOMEM || ret == -EAGAIN)
return BLK_STS_RESOURCE;
return BLK_STS_IOERR;
@@ -2737,8 +2715,6 @@ nvme_fc_start_fcp_op(struct nvme_fc_ctrl *ctrl, struct nvme_fc_queue *queue,
nvme_cleanup_cmd(op->rq);
}
- nvme_fc_ctrl_put(ctrl);
-
if (ctrl->rport->remoteport.port_state == FC_OBJSTATE_ONLINE &&
ret != -EBUSY)
return BLK_STS_IOERR;
@@ -2822,7 +2798,6 @@ nvme_fc_complete_rq(struct request *rq)
nvme_fc_unmap_data(ctrl, rq, op);
nvme_complete_rq(rq);
- nvme_fc_ctrl_put(ctrl);
}
static void nvme_fc_map_queues(struct blk_mq_tag_set *set)
@@ -3251,9 +3226,16 @@ static void
nvme_fc_delete_ctrl(struct nvme_ctrl *nctrl)
{
struct nvme_fc_ctrl *ctrl = to_fc_ctrl(nctrl);
+ unsigned long flags;
cancel_work_sync(&ctrl->ioerr_work);
cancel_delayed_work_sync(&ctrl->connect_work);
+
+ /* remove from rport list */
+ spin_lock_irqsave(&ctrl->rport->lock, flags);
+ list_del(&ctrl->ctrl_list);
+ spin_unlock_irqrestore(&ctrl->rport->lock, flags);
+
/*
* kill the association on the link side. this will block
* waiting for io to terminate
@@ -3307,7 +3289,7 @@ nvme_fc_reconnect_or_delete(struct nvme_fc_ctrl *ctrl, int status)
ctrl->cnum, min_t(int, portptr->dev_loss_tmo,
(ctrl->ctrl.opts->max_reconnects *
ctrl->ctrl.opts->reconnect_delay)));
- WARN_ON(nvme_delete_ctrl(&ctrl->ctrl));
+ nvme_fc_ctrl_put(ctrl);
}
}
@@ -3521,6 +3503,7 @@ nvme_fc_alloc_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
out_free_ctrl:
kfree(ctrl);
out_fail:
+ nvme_fc_rport_put(rport);
/* exit via here doesn't follow ctlr ref points */
return ERR_PTR(ret);
}
@@ -3539,7 +3522,7 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
ret = nvme_add_ctrl(&ctrl->ctrl);
if (ret)
- goto out_put_ctrl;
+ goto fail_ctrl;
ret = nvme_alloc_admin_tag_set(&ctrl->ctrl, &ctrl->admin_tag_set,
&nvme_fc_admin_mq_ops,
@@ -3574,26 +3557,7 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
return &ctrl->ctrl;
fail_ctrl:
- nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_DELETING);
- cancel_work_sync(&ctrl->ioerr_work);
- cancel_work_sync(&ctrl->ctrl.reset_work);
- cancel_delayed_work_sync(&ctrl->connect_work);
-
- /* initiate nvme ctrl ref counting teardown */
- nvme_uninit_ctrl(&ctrl->ctrl);
-
-out_put_ctrl:
- /* Remove core ctrl ref. */
- nvme_put_ctrl(&ctrl->ctrl);
-
- /* as we're past the point where we transition to the ref
- * counting teardown path, if we return a bad pointer here,
- * the calling routine, thinking it's prior to the
- * transition, will do an rport put. Since the teardown
- * path also does a rport put, we do an extra get here to
- * so proper order/teardown happens.
- */
- nvme_fc_rport_get(rport);
+ nvme_fc_ctrl_put(ctrl);
return ERR_PTR(-EIO);
}
@@ -3703,8 +3667,6 @@ nvme_fc_create_ctrl(struct device *dev, struct nvmf_ctrl_options *opts)
spin_unlock_irqrestore(&nvme_fc_lock, flags);
ctrl = nvme_fc_init_ctrl(dev, opts, lport, rport);
- if (IS_ERR(ctrl))
- nvme_fc_rport_put(rport);
return ctrl;
}
}
@@ -3929,7 +3891,7 @@ nvme_fc_delete_controllers(struct nvme_fc_rport *rport)
dev_warn(ctrl->ctrl.device,
"NVME-FC{%d}: transport unloading: deleting ctrl\n",
ctrl->cnum);
- nvme_delete_ctrl(&ctrl->ctrl);
+ nvme_fc_ctrl_put(ctrl);
}
spin_unlock(&rport->lock);
}
--
2.51.0On 8/29/25 17:37, Daniel Wagner wrote: > The lifetime of the controller is managed by the upper layers. This > ensures that the controller does not go away as long as there are > commands in flight. Thus, there is no need to take references in the > command handling code. > > Currently, the refcounting code is partially open-coded for handling > error paths. By moving the cleanup code fully under refcounting and > releasing references when necessary, the error handling can be > simplified. > > Signed-off-by: Daniel Wagner <wagi@kernel.org> > --- > drivers/nvme/host/fc.c | 102 ++++++++++++++++--------------------------------- > 1 file changed, 32 insertions(+), 70 deletions(-) > Reviewed-by: Hannes Reinecke <hare@suse.de> 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
Hi Daniel,
kernel test robot noticed the following build warnings:
url: https://github.com/intel-lab-lkp/linux/commits/Daniel-Wagner/nvme-fabrics-introduce-ref-counting-for-nvmf_ctrl_options/20250829-234513
base: c17b750b3ad9f45f2b6f7e6f7f4679844244f0b9
patch link: https://lore.kernel.org/r/20250829-nvme-fc-sync-v3-2-d69c87e63aee%40kernel.org
patch subject: [PATCH v3 2/4] nvme-fc: reorganize ctrl ref-counting and cleanup code
config: x86_64-randconfig-161-20250830 (https://download.01.org/0day-ci/archive/20250831/202508310442.lBWBZ5AC-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202508310442.lBWBZ5AC-lkp@intel.com/
New smatch warnings:
drivers/nvme/host/fc.c:1492 nvme_fc_match_disconn_ls() warn: iterator used outside loop: 'ctrl'
Old smatch warnings:
drivers/nvme/host/fc.c:3180 nvme_fc_delete_association() warn: mixing irqsave and irq
vim +/ctrl +1492 drivers/nvme/host/fc.c
14fd1e98afafc0 James Smart 2020-03-31 1465 static struct nvme_fc_ctrl *
14fd1e98afafc0 James Smart 2020-03-31 1466 nvme_fc_match_disconn_ls(struct nvme_fc_rport *rport,
14fd1e98afafc0 James Smart 2020-03-31 1467 struct nvmefc_ls_rcv_op *lsop)
14fd1e98afafc0 James Smart 2020-03-31 1468 {
14fd1e98afafc0 James Smart 2020-03-31 1469 struct fcnvme_ls_disconnect_assoc_rqst *rqst =
14fd1e98afafc0 James Smart 2020-03-31 1470 &lsop->rqstbuf->rq_dis_assoc;
14fd1e98afafc0 James Smart 2020-03-31 1471 struct nvme_fc_ctrl *ctrl, *ret = NULL;
14fd1e98afafc0 James Smart 2020-03-31 1472 struct nvmefc_ls_rcv_op *oldls = NULL;
14fd1e98afafc0 James Smart 2020-03-31 1473 u64 association_id = be64_to_cpu(rqst->associd.association_id);
14fd1e98afafc0 James Smart 2020-03-31 1474 unsigned long flags;
14fd1e98afafc0 James Smart 2020-03-31 1475
14fd1e98afafc0 James Smart 2020-03-31 1476 spin_lock_irqsave(&rport->lock, flags);
14fd1e98afafc0 James Smart 2020-03-31 1477
14fd1e98afafc0 James Smart 2020-03-31 1478 list_for_each_entry(ctrl, &rport->ctrl_list, ctrl_list) {
14fd1e98afafc0 James Smart 2020-03-31 1479 spin_lock(&ctrl->lock);
14fd1e98afafc0 James Smart 2020-03-31 1480 if (association_id == ctrl->association_id) {
14fd1e98afafc0 James Smart 2020-03-31 1481 oldls = ctrl->rcv_disconn;
14fd1e98afafc0 James Smart 2020-03-31 1482 ctrl->rcv_disconn = lsop;
14fd1e98afafc0 James Smart 2020-03-31 1483 ret = ctrl;
There should maybe be a break statement here?
14fd1e98afafc0 James Smart 2020-03-31 1484 }
14fd1e98afafc0 James Smart 2020-03-31 1485 spin_unlock(&ctrl->lock);
14fd1e98afafc0 James Smart 2020-03-31 1486 }
14fd1e98afafc0 James Smart 2020-03-31 1487
14fd1e98afafc0 James Smart 2020-03-31 1488 spin_unlock_irqrestore(&rport->lock, flags);
14fd1e98afafc0 James Smart 2020-03-31 1489
14fd1e98afafc0 James Smart 2020-03-31 1490 /* transmit a response for anything that was pending */
14fd1e98afafc0 James Smart 2020-03-31 1491 if (oldls) {
14fd1e98afafc0 James Smart 2020-03-31 @1492 dev_info(rport->lport->dev,
14fd1e98afafc0 James Smart 2020-03-31 1493 "NVME-FC{%d}: Multiple Disconnect Association "
14fd1e98afafc0 James Smart 2020-03-31 1494 "LS's received\n", ctrl->cnum);
ctrl->cnum is always an invalid pointer on this line. Another
option would be to print ret->cnum instead ctrl->nmum.
14fd1e98afafc0 James Smart 2020-03-31 1495 /* overwrite good response with bogus failure */
14fd1e98afafc0 James Smart 2020-03-31 1496 oldls->lsrsp->rsplen = nvme_fc_format_rjt(oldls->rspbuf,
14fd1e98afafc0 James Smart 2020-03-31 1497 sizeof(*oldls->rspbuf),
14fd1e98afafc0 James Smart 2020-03-31 1498 rqst->w0.ls_cmd,
14fd1e98afafc0 James Smart 2020-03-31 1499 FCNVME_RJT_RC_UNAB,
14fd1e98afafc0 James Smart 2020-03-31 1500 FCNVME_RJT_EXP_NONE, 0);
14fd1e98afafc0 James Smart 2020-03-31 1501 nvme_fc_xmt_ls_rsp(oldls);
14fd1e98afafc0 James Smart 2020-03-31 1502 }
14fd1e98afafc0 James Smart 2020-03-31 1503
14fd1e98afafc0 James Smart 2020-03-31 1504 return ret;
14fd1e98afafc0 James Smart 2020-03-31 1505 }
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Dan,
On Mon, Sep 01, 2025 at 02:46:07PM +0300, Dan Carpenter wrote:
> Hi Daniel,
>
> kernel test robot noticed the following build warnings:
>
> url: https://github.com/intel-lab-lkp/linux/commits/Daniel-Wagner/nvme-fabrics-introduce-ref-counting-for-nvmf_ctrl_options/20250829-234513
> base: c17b750b3ad9f45f2b6f7e6f7f4679844244f0b9
> patch link: https://lore.kernel.org/r/20250829-nvme-fc-sync-v3-2-d69c87e63aee%40kernel.org
> patch subject: [PATCH v3 2/4] nvme-fc: reorganize ctrl ref-counting and cleanup code
> config: x86_64-randconfig-161-20250830 (https://download.01.org/0day-ci/archive/20250831/202508310442.lBWBZ5AC-lkp@intel.com/config)
> compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> | Closes: https://lore.kernel.org/r/202508310442.lBWBZ5AC-lkp@intel.com/
>
> New smatch warnings:
> drivers/nvme/host/fc.c:1492 nvme_fc_match_disconn_ls() warn: iterator used outside loop: 'ctrl'
>
> Old smatch warnings:
> drivers/nvme/host/fc.c:3180 nvme_fc_delete_association() warn: mixing irqsave and irq
>
> vim +/ctrl +1492 drivers/nvme/host/fc.c
>
> 14fd1e98afafc0 James Smart 2020-03-31 1465 static struct nvme_fc_ctrl *
> 14fd1e98afafc0 James Smart 2020-03-31 1466 nvme_fc_match_disconn_ls(struct nvme_fc_rport *rport,
> 14fd1e98afafc0 James Smart 2020-03-31 1467 struct nvmefc_ls_rcv_op *lsop)
> 14fd1e98afafc0 James Smart 2020-03-31 1468 {
> 14fd1e98afafc0 James Smart 2020-03-31 1469 struct fcnvme_ls_disconnect_assoc_rqst *rqst =
> 14fd1e98afafc0 James Smart 2020-03-31 1470 &lsop->rqstbuf->rq_dis_assoc;
> 14fd1e98afafc0 James Smart 2020-03-31 1471 struct nvme_fc_ctrl *ctrl, *ret = NULL;
> 14fd1e98afafc0 James Smart 2020-03-31 1472 struct nvmefc_ls_rcv_op *oldls = NULL;
> 14fd1e98afafc0 James Smart 2020-03-31 1473 u64 association_id = be64_to_cpu(rqst->associd.association_id);
> 14fd1e98afafc0 James Smart 2020-03-31 1474 unsigned long flags;
> 14fd1e98afafc0 James Smart 2020-03-31 1475
> 14fd1e98afafc0 James Smart 2020-03-31 1476 spin_lock_irqsave(&rport->lock, flags);
> 14fd1e98afafc0 James Smart 2020-03-31 1477
> 14fd1e98afafc0 James Smart 2020-03-31 1478 list_for_each_entry(ctrl, &rport->ctrl_list, ctrl_list) {
> 14fd1e98afafc0 James Smart 2020-03-31 1479 spin_lock(&ctrl->lock);
> 14fd1e98afafc0 James Smart 2020-03-31 1480 if (association_id == ctrl->association_id) {
> 14fd1e98afafc0 James Smart 2020-03-31 1481 oldls = ctrl->rcv_disconn;
> 14fd1e98afafc0 James Smart 2020-03-31 1482 ctrl->rcv_disconn = lsop;
> 14fd1e98afafc0 James Smart 2020-03-31 1483 ret = ctrl;
>
> There should maybe be a break statement here?
Hmm, the commit 14fd1e98afafc0 has a break after the spin_unlock.
+ spin_unlock(&ctrl->lock);
+ if (ret)
+ /* leave the ctrl get reference */
+ break;
And since then there aren't any changes applied.
> 14fd1e98afafc0 James Smart 2020-03-31 1484 }
> 14fd1e98afafc0 James Smart 2020-03-31 1485 spin_unlock(&ctrl->lock);
> 14fd1e98afafc0 James Smart 2020-03-31 1486 }
> 14fd1e98afafc0 James Smart 2020-03-31 1487
> 14fd1e98afafc0 James Smart 2020-03-31 1488 spin_unlock_irqrestore(&rport->lock, flags);
> 14fd1e98afafc0 James Smart 2020-03-31 1489
> 14fd1e98afafc0 James Smart 2020-03-31 1490 /* transmit a response for anything that was pending */
> 14fd1e98afafc0 James Smart 2020-03-31 1491 if (oldls) {
> 14fd1e98afafc0 James Smart 2020-03-31 @1492 dev_info(rport->lport->dev,
> 14fd1e98afafc0 James Smart 2020-03-31 1493 "NVME-FC{%d}: Multiple Disconnect Association "
> 14fd1e98afafc0 James Smart 2020-03-31 1494 "LS's received\n", ctrl->cnum);
>
> ctrl->cnum is always an invalid pointer on this line. Another
> option would be to print ret->cnum instead ctrl->nmum.
Yes, this makes sense. I'll send a patch for this. Thanks!
Thanks,
Daniel
© 2016 - 2025 Red Hat, Inc.