[PATCH v4 0/2] nvme: Enable generic interface (/dev/ngX) for unknown command sets

[PATCH v4 0/2] nvme: Enable generic interface (/dev/ngX) for unknown command sets

[Joel Granados]

Currently block and char interface do not show up for any command set other than
NVM and ZNS. This series enables char interface to come up for unknown command sets.

Patch 1: Is prep. It allows the re-use of nvme_mpath_add_disk for
supported as well as independent command sets.

Patch 2: The following functions become aware of whether the command set is
independent or not: nvme_alloc_ns and nvme_validate_ns. A switch statement
is introduced in each of them to call the relevant helper to identify and
update the namespaces.  nvme_update_ns_info_cs_indep is added to update
command set independent devices.

Changes since V3:
- Stop ignoring the NVM or ZNS controllers that do not support
  command-set-independent namespace identification and allow them to come
  up through the usual command-set-dependent path.
- Only devices that are not NVM nor ZNS and do not support the
  command-set-indep NS identification will be "abandoned"
- New function nvme_update_ns_mpath_info_general that is called from both
  nmve_update_ns_info and nvme_update_ns_info_cs_indep to "update" the
  mpath device. This removes repeated code.
- Warn when when command-set-indep NS is not supported or it returns
  error.
- Only assign the independent id in nvme_identify_ns_cs_indep when there
  are no errors. This is to keel NULL value on failure.

Changes since V2:
- Removed unnecessary include to types.h
- Call nvme_identify_ns_cs_indep only once and reuse the created
  independent id in the nvme_alloc_ns call. We call
  nvme_identify_ns_cs_indep regardless of ctrl->cap & NVME_CAP_CRMS_CRIMS
  value
- Replace 'if' with 'switch' statements making it explicit that the command
  set independent functions are used when the command set is not NVM nor
  ZNS.
- Set the hidden flag in the multipath case so the block device does not
  come up when multipath is enabled.

Changes since V1:
- Use command-set independent id-ns to enable unknown command-sets

Joel Granados (2):
  nvme-multipath: refactor nvme_mpath_add_disk
  nvme: enable generic interface (/dev/ngXnY) for unknown command sets

 drivers/nvme/host/core.c      | 164 ++++++++++++++++++++++++----------
 drivers/nvme/host/multipath.c |   6 +-
 drivers/nvme/host/nvme.h      |   5 +-
 3 files changed, 120 insertions(+), 55 deletions(-)

-- 
2.30.2

[PATCH v4 1/2] nvme-multipath: refactor nvme_mpath_add_disk

[Joel Granados]

Pass anagrpid as second argument. This is prep patch that allows reusing
this function for supporting unknown command sets.

Signed-off-by: Joel Granados <j.granados@samsung.com>
Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
---
 drivers/nvme/host/core.c      | 2 +-
 drivers/nvme/host/multipath.c | 6 +++---
 drivers/nvme/host/nvme.h      | 5 ++---
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 57480075550c..0fd6127da8d1 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4024,7 +4024,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
 	if (!nvme_ns_head_multipath(ns->head))
 		nvme_add_ns_cdev(ns);
 
-	nvme_mpath_add_disk(ns, id);
+	nvme_mpath_add_disk(ns, id->anagrpid);
 	nvme_fault_inject_init(&ns->fault_inject, ns->disk->disk_name);
 	kfree(id);
 
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index d3e2440d8abb..94dba4eab4fb 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -800,16 +800,16 @@ static int nvme_lookup_ana_group_desc(struct nvme_ctrl *ctrl,
 	return -ENXIO; /* just break out of the loop */
 }
 
-void nvme_mpath_add_disk(struct nvme_ns *ns, struct nvme_id_ns *id)
+void nvme_mpath_add_disk(struct nvme_ns *ns, __le32 anagrpid)
 {
 	if (nvme_ctrl_use_ana(ns->ctrl)) {
 		struct nvme_ana_group_desc desc = {
-			.grpid = id->anagrpid,
+			.grpid = anagrpid,
 			.state = 0,
 		};
 
 		mutex_lock(&ns->ctrl->ana_lock);
-		ns->ana_grpid = le32_to_cpu(id->anagrpid);
+		ns->ana_grpid = le32_to_cpu(anagrpid);
 		nvme_parse_ana_log(ns->ctrl, &desc, nvme_lookup_ana_group_desc);
 		mutex_unlock(&ns->ctrl->ana_lock);
 		if (desc.state) {
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index e4612dd0b420..5dac319f9b7c 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -836,7 +836,7 @@ void nvme_mpath_default_iopolicy(struct nvme_subsystem *subsys);
 void nvme_failover_req(struct request *req);
 void nvme_kick_requeue_lists(struct nvme_ctrl *ctrl);
 int nvme_mpath_alloc_disk(struct nvme_ctrl *ctrl,struct nvme_ns_head *head);
-void nvme_mpath_add_disk(struct nvme_ns *ns, struct nvme_id_ns *id);
+void nvme_mpath_add_disk(struct nvme_ns *ns, __le32 anagrpid);
 void nvme_mpath_remove_disk(struct nvme_ns_head *head);
 int nvme_mpath_init_identify(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id);
 void nvme_mpath_init_ctrl(struct nvme_ctrl *ctrl);
@@ -878,8 +878,7 @@ static inline int nvme_mpath_alloc_disk(struct nvme_ctrl *ctrl,
 {
 	return 0;
 }
-static inline void nvme_mpath_add_disk(struct nvme_ns *ns,
-		struct nvme_id_ns *id)
+static inline void nvme_mpath_add_disk(struct nvme_ns *ns, __le32 anagrpid)
 {
 }
 static inline void nvme_mpath_remove_disk(struct nvme_ns_head *head)
-- 
2.30.2

[PATCH v4 2/2] nvme: enable generic interface (/dev/ngXnY) for unknown command sets

[Joel Granados]

Extend nvme_alloc_ns() and nvme_validate_ns() for unknown command-set as
well. Both are made to use a new helper (nvme_update_ns_info_cs_indep)
which is similar to nvme_update_ns_info but performs fewer operations
to get the generic interface up.

Suggested-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Joel Granados <j.granados@samsung.com>
Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
---
 drivers/nvme/host/core.c | 164 +++++++++++++++++++++++++++------------
 1 file changed, 115 insertions(+), 49 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 0fd6127da8d1..45ad1bbe0c33 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1425,28 +1425,30 @@ static int nvme_identify_ns(struct nvme_ctrl *ctrl, unsigned nsid,
 }
 
 static int nvme_identify_ns_cs_indep(struct nvme_ctrl *ctrl, unsigned nsid,
-			struct nvme_id_ns_cs_indep **id)
+			struct nvme_id_ns_cs_indep **indep_id)
 {
 	struct nvme_command c = {
 		.identify.opcode	= nvme_admin_identify,
 		.identify.nsid		= cpu_to_le32(nsid),
 		.identify.cns		= NVME_ID_CNS_NS_CS_INDEP,
 	};
+	struct nvme_id_ns_cs_indep *id;
 	int ret;
 
-	*id = kmalloc(sizeof(**id), GFP_KERNEL);
-	if (!*id)
+	id = kmalloc(sizeof(**indep_id), GFP_KERNEL);
+	if (!id)
 		return -ENOMEM;
 
-	ret = nvme_submit_sync_cmd(ctrl->admin_q, &c, *id, sizeof(**id));
+	ret = nvme_submit_sync_cmd(ctrl->admin_q, &c, id, sizeof(**indep_id));
 	if (ret) {
 		dev_warn(ctrl->device,
 			 "Identify namespace (CS independent) failed (%d)\n",
 			 ret);
-		kfree(*id);
+		kfree(id);
 		return ret;
 	}
 
+	*indep_id = id;
 	return 0;
 }
 
@@ -1908,6 +1910,39 @@ static void nvme_set_chunk_sectors(struct nvme_ns *ns, struct nvme_id_ns *id)
 	blk_queue_chunk_sectors(ns->queue, iob);
 }
 
+static void nvme_update_ns_mpath_info_general(struct nvme_ns *ns, __u8 nsattr)
+{
+	set_disk_ro(ns->head->disk,
+		    (nsattr & NVME_NS_ATTR_RO) ||
+			    test_bit(NVME_NS_FORCE_RO, &ns->flags));
+	nvme_mpath_revalidate_paths(ns);
+	blk_stack_limits(&ns->head->disk->queue->limits,
+			 &ns->queue->limits, 0);
+}
+
+static int nvme_update_ns_info_cs_indep(struct nvme_ns *ns,
+					struct nvme_id_ns_cs_indep *id)
+{
+	blk_mq_freeze_queue(ns->disk->queue);
+	nvme_set_queue_limits(ns->ctrl, ns->queue);
+	set_disk_ro(ns->disk, (id->nsattr & NVME_NS_ATTR_RO) ||
+		test_bit(NVME_NS_FORCE_RO, &ns->flags));
+	blk_mq_unfreeze_queue(ns->disk->queue);
+
+	if (nvme_ns_head_multipath(ns->head)) {
+		blk_mq_freeze_queue(ns->head->disk->queue);
+		nvme_update_ns_mpath_info_general(ns, id->nsattr);
+		ns->head->disk->flags |= GENHD_FL_HIDDEN;
+		blk_mq_unfreeze_queue(ns->head->disk->queue);
+	}
+
+	/* Hide the block-interface for these devices */
+	ns->disk->flags |= GENHD_FL_HIDDEN;
+	set_bit(NVME_NS_READY, &ns->flags);
+
+	return 0;
+}
+
 static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id)
 {
 	unsigned lbaf = nvme_lbaf_index(id->flbas);
@@ -1941,12 +1976,7 @@ static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id)
 	if (nvme_ns_head_multipath(ns->head)) {
 		blk_mq_freeze_queue(ns->head->disk->queue);
 		nvme_update_disk_info(ns->head->disk, ns, id);
-		set_disk_ro(ns->head->disk,
-			    (id->nsattr & NVME_NS_ATTR_RO) ||
-				    test_bit(NVME_NS_FORCE_RO, &ns->flags));
-		nvme_mpath_revalidate_paths(ns);
-		blk_stack_limits(&ns->head->disk->queue->limits,
-				 &ns->queue->limits, 0);
+		nvme_update_ns_mpath_info_general(ns, id->nsattr);
 		disk_update_readahead(ns->head->disk);
 		blk_mq_unfreeze_queue(ns->head->disk->queue);
 	}
@@ -3951,15 +3981,31 @@ static void nvme_ns_add_to_ctrl_list(struct nvme_ns *ns)
 }
 
 static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
-		struct nvme_ns_ids *ids)
+		struct nvme_ns_ids *ids, struct nvme_id_ns_cs_indep *indep_id)
 {
+	int ret = 0;
 	struct nvme_ns *ns;
 	struct gendisk *disk;
-	struct nvme_id_ns *id;
+	struct nvme_id_ns *id = NULL;
 	int node = ctrl->numa_node;
+	__u8 nmic;
+	__le32 anagrpid;
 
-	if (nvme_identify_ns(ctrl, nsid, ids, &id))
-		return;
+	switch (ids->csi) {
+	case NVME_CSI_NVM:
+	case NVME_CSI_ZNS:
+		if (nvme_identify_ns(ctrl, nsid, ids, &id))
+			return;
+		nmic = id->nmic;
+		anagrpid = id->anagrpid;
+		break;
+	default:
+		if (!indep_id)
+			return;
+		nmic = indep_id->nmic;
+		anagrpid = indep_id->anagrpid;
+		break;
+	}
 
 	ns = kzalloc_node(sizeof(*ns), GFP_KERNEL, node);
 	if (!ns)
@@ -3984,7 +4030,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
 	ns->ctrl = ctrl;
 	kref_init(&ns->kref);
 
-	if (nvme_init_ns_head(ns, nsid, ids, id->nmic & NVME_NS_NMIC_SHARED))
+	if (nvme_init_ns_head(ns, nsid, ids, nmic & NVME_NS_NMIC_SHARED))
 		goto out_cleanup_disk;
 
 	/*
@@ -4010,7 +4056,16 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
 			ns->head->instance);
 	}
 
-	if (nvme_update_ns_info(ns, id))
+	switch (ids->csi) {
+	case NVME_CSI_NVM:
+	case NVME_CSI_ZNS:
+		ret = nvme_update_ns_info(ns, id);
+		break;
+	default:
+		ret = nvme_update_ns_info_cs_indep(ns, indep_id);
+		break;
+	}
+	if (ret)
 		goto out_unlink_ns;
 
 	down_write(&ctrl->namespaces_rwsem);
@@ -4024,11 +4079,10 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
 	if (!nvme_ns_head_multipath(ns->head))
 		nvme_add_ns_cdev(ns);
 
-	nvme_mpath_add_disk(ns, id->anagrpid);
+	nvme_mpath_add_disk(ns, anagrpid);
 	nvme_fault_inject_init(&ns->fault_inject, ns->disk->disk_name);
-	kfree(id);
 
-	return;
+	goto out_free_id;
 
  out_cleanup_ns_from_list:
 	nvme_put_ctrl(ctrl);
@@ -4106,29 +4160,34 @@ static void nvme_ns_remove_by_nsid(struct nvme_ctrl *ctrl, u32 nsid)
 	}
 }
 
-static void nvme_validate_ns(struct nvme_ns *ns, struct nvme_ns_ids *ids)
+static void nvme_validate_ns(struct nvme_ns *ns, struct nvme_ns_ids *ids,
+			     struct nvme_id_ns_cs_indep *indep_id)
 {
-	struct nvme_id_ns *id;
+	struct nvme_id_ns *id = NULL;
 	int ret = NVME_SC_INVALID_NS | NVME_SC_DNR;
 
 	if (test_bit(NVME_NS_DEAD, &ns->flags))
 		goto out;
 
-	ret = nvme_identify_ns(ns->ctrl, ns->head->ns_id, ids, &id);
-	if (ret)
-		goto out;
-
-	ret = NVME_SC_INVALID_NS | NVME_SC_DNR;
-	if (!nvme_ns_ids_equal(&ns->head->ids, ids)) {
-		dev_err(ns->ctrl->device,
-			"identifiers changed for nsid %d\n", ns->head->ns_id);
-		goto out_free_id;
+	switch (ids->csi) {
+	case NVME_CSI_NVM:
+	case NVME_CSI_ZNS:
+		if (nvme_identify_ns(ns->ctrl, ns->head->ns_id, ids, &id))
+			goto out;
+		if (!nvme_ns_ids_equal(&ns->head->ids, ids))
+			dev_err(ns->ctrl->device,
+				"identifiers changed for nsid %d\n",
+				ns->head->ns_id);
+		else
+			ret = nvme_update_ns_info(ns, id);
+		kfree(id);
+		break;
+	default:
+		if (indep_id)
+			ret = nvme_update_ns_info_cs_indep(ns, indep_id);
+		break;
 	}
 
-	ret = nvme_update_ns_info(ns, id);
-
-out_free_id:
-	kfree(id);
 out:
 	/*
 	 * Only remove the namespace if we got a fatal error back from the
@@ -4143,36 +4202,34 @@ static void nvme_validate_ns(struct nvme_ns *ns, struct nvme_ns_ids *ids)
 static void nvme_validate_or_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 {
 	struct nvme_ns_ids ids = { };
-	struct nvme_id_ns_cs_indep *id;
+	struct nvme_id_ns_cs_indep *indep_id = NULL;
 	struct nvme_ns *ns;
-	bool ready = true;
+	int ret;
 
 	if (nvme_identify_ns_descs(ctrl, nsid, &ids))
 		return;
 
+	ret = nvme_identify_ns_cs_indep(ctrl, nsid, &indep_id);
+
 	/*
 	 * Check if the namespace is ready.  If not ignore it, we will get an
 	 * AEN once it becomes ready and restart the scan.
 	 */
-	if ((ctrl->cap & NVME_CAP_CRMS_CRIMS) &&
-	    !nvme_identify_ns_cs_indep(ctrl, nsid, &id)) {
-		ready = id->nstat & NVME_NSTAT_NRDY;
-		kfree(id);
+	if ((ctrl->cap & NVME_CAP_CRMS_CRIMS) && !ret) {
+		if (!(indep_id->nstat & NVME_NSTAT_NRDY))
+			goto free_indep_id;
 	}
 
-	if (!ready)
-		return;
-
 	ns = nvme_find_get_ns(ctrl, nsid);
 	if (ns) {
-		nvme_validate_ns(ns, &ids);
+		nvme_validate_ns(ns, &ids, indep_id);
 		nvme_put_ns(ns);
-		return;
+		goto free_indep_id;
 	}
 
 	switch (ids.csi) {
 	case NVME_CSI_NVM:
-		nvme_alloc_ns(ctrl, nsid, &ids);
+		nvme_alloc_ns(ctrl, nsid, &ids, indep_id);
 		break;
 	case NVME_CSI_ZNS:
 		if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED)) {
@@ -4187,13 +4244,22 @@ static void nvme_validate_or_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 				nsid);
 			break;
 		}
-		nvme_alloc_ns(ctrl, nsid, &ids);
+		nvme_alloc_ns(ctrl, nsid, &ids, indep_id);
 		break;
 	default:
-		dev_warn(ctrl->device, "unknown csi %u for nsid %u\n",
+		/* enable char-interface for unknown command sets*/
+		if (!ret) {
+			nvme_alloc_ns(ctrl, nsid, &ids, indep_id);
+			break;
+		}
+		dev_warn(ctrl->device,
+			"unknown csi %u for nsid %u not supported\n",
 			ids.csi, nsid);
 		break;
 	}
+
+free_indep_id:
+	kfree(indep_id);
 }
 
 static void nvme_remove_invalid_namespaces(struct nvme_ctrl *ctrl,
-- 
2.30.2

Re: [PATCH v4 1/2] nvme-multipath: refactor nvme_mpath_add_disk

[Christoph Hellwig]

Thanks,

applied to nvme-5.20.