Re: [PATCH v6 03/18] nvmet: Implement CCR nvme command

[Mohamed Khalfella <mkhalfella@purestorage.com>]

On Mon 2026-10-19 18:00:01 +0200, Daniel Wagner wrote:
> > +void nvmet_execute_cross_ctrl_reset(struct nvmet_req *req)
> > +{
> > +	struct nvmet_ctrl *ictrl, *sctrl = req->sq->ctrl;
> > +	struct nvme_command *cmd = req->cmd;
> > +	struct nvmet_ccr *ccr, *new_ccr;
> > +	int ccr_active, ccr_total;
> > +	u16 cntlid, status = NVME_SC_SUCCESS;
> > +
> > +	if (!nvmet_check_transfer_len(req, 0))
> > +		return;
> > +
> > +	cntlid = le16_to_cpu(cmd->ccr.icid);
> > +	if (sctrl->cntlid == cntlid) {
> > +		req->error_loc =
> > +			offsetof(struct nvme_cross_ctrl_reset_cmd, icid);
> > +		status = NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
> > +		goto out;
> > +	}
> > +
> > +	/* Find and get impacted controller */
> > +	ictrl = nvmet_ctrl_find_get_ccr(sctrl->subsys, sctrl->hostnqn,
> > +					cmd->ccr.ciu, cntlid,
> > +					le64_to_cpu(cmd->ccr.cirn));
> > +	if (!ictrl) {
> > +		/* Immediate Reset Successful */
> > +		nvmet_set_result(req, 1);
> > +		status = NVME_SC_SUCCESS;
> > +		goto out;
> > +	}
> > +
> > +	ccr_total = ccr_active = 0;
> > +	mutex_lock(&sctrl->lock);
> > +	list_for_each_entry(ccr, &sctrl->ccr_list, entry) {
> > +		if (ccr->ctrl == ictrl) {
> > +			status = NVME_SC_CCR_IN_PROGRESS | NVME_STATUS_DNR;
> > +			goto out_unlock;
> > +		}
> > +
> > +		ccr_total++;
> > +		if (ccr->ctrl)
> > +			ccr_active++;
> > +	}
> > +
> > +	if (ccr_active >= NVMF_CCR_LIMIT) {
> > +		status = NVME_SC_CCR_LIMIT_EXCEEDED;
> > +		goto out_unlock;
> > +	}
> > +	if (ccr_total >= NVMF_CCR_PER_PAGE) {
> > +		status = NVME_SC_CCR_LOGPAGE_FULL;
> > +		goto out_unlock;
> > +	}
> > +
> > +	new_ccr = kmalloc_obj(*new_ccr, GFP_KERNEL);
> > +	if (!new_ccr) {
> > +		status = NVME_SC_INTERNAL;
> > +		goto out_unlock;
> > +	}
> > +
> > +	new_ccr->ciu = cmd->ccr.ciu;
> > +	new_ccr->icid = cntlid;
> > +	new_ccr->ctrl = ictrl;
> > +	list_add_tail(&new_ccr->entry, &sctrl->ccr_list);
> > +
> > +out_unlock:
> > +	mutex_unlock(&sctrl->lock);
> > +	if (status == NVME_SC_SUCCESS)
> > +		nvmet_ctrl_fatal_error(ictrl);
> 
> nvmet_ctrl_fatal_error will log an error, is this desired?

I think it is good to have the error logged. The fatal error might not
have been observed by the impacted controller we are failing yet, but it
is still okay to consider it as fatal error IMO.

> 
> > +struct nvmet_ctrl *nvmet_ctrl_find_get_ccr(struct nvmet_subsys *subsys,
> > +					   const char *hostnqn, u8 ciu,
> > +					   u16 cntlid, u64 cirn)
> > +{
> > +	struct nvmet_ctrl *ctrl, *ictrl = NULL;
> > +	bool found = false;
> > +
> > +	mutex_lock(&subsys->lock);
> > +	list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry) {
> > +		if (ctrl->cntlid != cntlid)
> > +			continue;
> > +		if (strncmp(ctrl->hostnqn, hostnqn, NVMF_NQN_SIZE))
> > +			continue;
> > +
> > +		/* Avoid racing with a controller that is becoming ready */
> 
> I think this comment could a bit more eleborate, it took me a while
> to figure out the impact of it. What about something like:
> 
>                 /*
>                  * Enabling the controller (nvmet_start_ctrl()) regenerates
>                  * ciu and cirn under ctrl->lock. Hold the lock so that a
>                  * controller becoming ready concurrently is matched against
>                  * either the old or the new instance, never a mix of both.
>                  *
>                  * CSTS.RDY is not checked on purpose: a disabled controller
>                  * can still have commands in flight, so a matching instance
>                  * must go through the CCR path rather than being reported as
>                  * already reset.
>                  */

Okay, I will take the first paragraph and remove the mention to
nvmet_start_ctrl() because comments with function names do not
usually age well. The second paragraph is not directly related
to the lock IMO.

> 
> -- 
> Daniel Wagner <wagi@kernel.org>