enable generic interface (/dev/ngX) for unknown command sets

enable generic interface (/dev/ngX) for unknown command sets

[Christoph Hellwig]

Hi all,

this is a version of the patch from Joel rebased on a bunch of cleanups
from me that I think make the layering a lot cleaner.  Let me know what
everyone things.

[PATCH 1/5] nvme: rename nvme_validate_or_alloc_ns to nvme_scan_ns

[Christoph Hellwig]

This shorter name much better fits what this function does in
the scanning process.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/host/core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index eabffbc708cd9..88b14fbb7a5a8 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4283,7 +4283,7 @@ static void nvme_validate_ns(struct nvme_ns *ns, struct nvme_ns_ids *ids)
 		nvme_ns_remove(ns);
 }
 
-static void nvme_validate_or_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
+static void nvme_scan_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 {
 	struct nvme_ns_ids ids = { };
 	struct nvme_id_ns_cs_indep *id;
@@ -4391,7 +4391,7 @@ static int nvme_scan_ns_list(struct nvme_ctrl *ctrl)
 
 			if (!nsid)	/* end of the list? */
 				goto out;
-			nvme_validate_or_alloc_ns(ctrl, nsid);
+			nvme_scan_ns(ctrl, nsid);
 			while (++prev < nsid)
 				nvme_ns_remove_by_nsid(ctrl, prev);
 		}
@@ -4414,7 +4414,7 @@ static void nvme_scan_ns_sequential(struct nvme_ctrl *ctrl)
 	kfree(id);
 
 	for (i = 1; i <= nn; i++)
-		nvme_validate_or_alloc_ns(ctrl, i);
+		nvme_scan_ns(ctrl, i);
 
 	nvme_remove_invalid_namespaces(ctrl, nn);
 }
-- 
2.30.2

Re: [PATCH 1/5] nvme: rename nvme_validate_or_alloc_ns to nvme_scan_ns

[Chaitanya Kulkarni]

On 7/12/22 22:49, Christoph Hellwig wrote:
> This shorter name much better fits what this function does in
> the scanning process.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

I'm fine either way, looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH 1/5] nvme: rename nvme_validate_or_alloc_ns to nvme_scan_ns

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH 1/5] nvme: rename nvme_validate_or_alloc_ns to nvme_scan_ns

[Kanchan Joshi]

[-- Attachment #1: Type: text/plain, Size: 252 bytes --]

On Wed, Jul 13, 2022 at 07:49:10AM +0200, Christoph Hellwig wrote:
>This shorter name much better fits what this function does in
>the scanning process.
>
>Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 2/5] nvme: generalize the nvme_multi_css check in nvme_scan_ns

[Christoph Hellwig]

Check for multiple command set support early on an error out if is
not supported when a !NVM command set namespace is found.  This
prepares for adding command set independent passthrough support.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/host/core.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 88b14fbb7a5a8..4769c49141913 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4293,6 +4293,12 @@ static void nvme_scan_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 	if (nvme_identify_ns_descs(ctrl, nsid, &ids))
 		return;
 
+	if (ids.csi != NVME_CSI_NVM && !nvme_multi_css(ctrl)) {
+		dev_warn(ctrl->device,
+			"command set not reported for nsid: %d\n", nsid);
+		return;
+	}
+
 	/*
 	 * Check if the namespace is ready.  If not ignore it, we will get an
 	 * AEN once it becomes ready and restart the scan.
@@ -4324,12 +4330,6 @@ static void nvme_scan_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 				nsid);
 			break;
 		}
-		if (!nvme_multi_css(ctrl)) {
-			dev_warn(ctrl->device,
-				"command set not reported for nsid: %d\n",
-				nsid);
-			break;
-		}
 		nvme_alloc_ns(ctrl, nsid, &ids);
 		break;
 	default:
-- 
2.30.2

Re: [PATCH 2/5] nvme: generalize the nvme_multi_css check in nvme_scan_ns

[Chaitanya Kulkarni]

On 7/12/22 22:49, Christoph Hellwig wrote:
> Check for multiple command set support early on an error out if is
> not supported when a !NVM command set namespace is found.  This
> prepares for adding command set independent passthrough support.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH 2/5] nvme: generalize the nvme_multi_css check in nvme_scan_ns

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH 2/5] nvme: generalize the nvme_multi_css check in nvme_scan_ns

[Kanchan Joshi]

[-- Attachment #1: Type: text/plain, Size: 365 bytes --]

On Wed, Jul 13, 2022 at 07:49:11AM +0200, Christoph Hellwig wrote:
>Check for multiple command set support early on an error out if is
>not supported when a !NVM command set namespace is found.  This
>prepares for adding command set independent passthrough support.
>
>Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 3/5] nvme: refactor namespace probing

[Christoph Hellwig]

Change nvme_ns_scan to gather all information needed for generic
namespace setup into a nvme_ns_info structure.  This structure is filled
from the Command Set Idependent Identify Namespace data structure if
it is available or else the legacy Identify namespace structure.

With that everything related to the NVM command set (and the ZNS command
set derived from it) can be encapsulated in the nvme_update_ns_info_block
function while keeping the rest of the namespace probing generic.

The downside is that we now always issue two Identify Namespace calls for
each probed namespace instead of usually just a single one previously.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/host/core.c | 231 +++++++++++++++++++++------------------
 1 file changed, 126 insertions(+), 105 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 4769c49141913..659068f90165c 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -31,6 +31,14 @@
 
 #define NVME_MINORS		(1U << MINORBITS)
 
+struct nvme_ns_info {
+	struct nvme_ns_ids ids;
+	u32 nsid;
+	__le32 anagrpid;
+	bool is_shared;
+	bool is_ready;
+};
+
 unsigned int admin_timeout = 60;
 module_param(admin_timeout, uint, 0644);
 MODULE_PARM_DESC(admin_timeout, "timeout in seconds for admin commands");
@@ -1342,8 +1350,8 @@ static int nvme_process_ns_desc(struct nvme_ctrl *ctrl, struct nvme_ns_ids *ids,
 	}
 }
 
-static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl, unsigned nsid,
-		struct nvme_ns_ids *ids)
+static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl,
+		struct nvme_ns_info *info)
 {
 	struct nvme_command c = { };
 	bool csi_seen = false;
@@ -1356,7 +1364,7 @@ static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl, unsigned nsid,
 		return 0;
 
 	c.identify.opcode = nvme_admin_identify;
-	c.identify.nsid = cpu_to_le32(nsid);
+	c.identify.nsid = cpu_to_le32(info->nsid);
 	c.identify.cns = NVME_ID_CNS_NS_DESC_LIST;
 
 	data = kzalloc(NVME_IDENTIFY_DATA_SIZE, GFP_KERNEL);
@@ -1368,7 +1376,7 @@ static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl, unsigned nsid,
 	if (status) {
 		dev_warn(ctrl->device,
 			"Identify Descriptors failed (nsid=%u, status=0x%x)\n",
-			nsid, status);
+			info->nsid, status);
 		goto free_data;
 	}
 
@@ -1378,7 +1386,7 @@ static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl, unsigned nsid,
 		if (cur->nidl == 0)
 			break;
 
-		len = nvme_process_ns_desc(ctrl, ids, cur, &csi_seen);
+		len = nvme_process_ns_desc(ctrl, &info->ids, cur, &csi_seen);
 		if (len < 0)
 			break;
 
@@ -1387,7 +1395,7 @@ static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl, unsigned nsid,
 
 	if (nvme_multi_css(ctrl) && !csi_seen) {
 		dev_warn(ctrl->device, "Command set not reported for nsid:%d\n",
-			 nsid);
+			 info->nsid);
 		status = -EINVAL;
 	}
 
@@ -1397,7 +1405,7 @@ static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl, unsigned nsid,
 }
 
 static int nvme_identify_ns(struct nvme_ctrl *ctrl, unsigned nsid,
-			struct nvme_ns_ids *ids, struct nvme_id_ns **id)
+			struct nvme_id_ns **id)
 {
 	struct nvme_command c = { };
 	int error;
@@ -1420,51 +1428,64 @@ static int nvme_identify_ns(struct nvme_ctrl *ctrl, unsigned nsid,
 	error = NVME_SC_INVALID_NS | NVME_SC_DNR;
 	if ((*id)->ncap == 0) /* namespace not allocated or attached */
 		goto out_free_id;
+	return 0;
 
+out_free_id:
+	kfree(*id);
+	return error;
+}
 
+static int nvme_ns_info_from_identify(struct nvme_ctrl *ctrl,
+		struct nvme_ns_info *info)
+{
+	struct nvme_ns_ids *ids = &info->ids;
+	struct nvme_id_ns *id;
+	int ret;
+
+	ret = nvme_identify_ns(ctrl, info->nsid, &id);
+	if (ret)
+		return ret;
+	info->anagrpid = id->anagrpid;
+	info->is_shared = id->nmic & NVME_NS_NMIC_SHARED;
+	info->is_ready = true;
 	if (ctrl->quirks & NVME_QUIRK_BOGUS_NID) {
 		dev_info(ctrl->device,
 			 "Ignoring bogus Namespace Identifiers\n");
 	} else {
 		if (ctrl->vs >= NVME_VS(1, 1, 0) &&
 		    !memchr_inv(ids->eui64, 0, sizeof(ids->eui64)))
-			memcpy(ids->eui64, (*id)->eui64, sizeof(ids->eui64));
+			memcpy(ids->eui64, id->eui64, sizeof(ids->eui64));
 		if (ctrl->vs >= NVME_VS(1, 2, 0) &&
 		    !memchr_inv(ids->nguid, 0, sizeof(ids->nguid)))
-			memcpy(ids->nguid, (*id)->nguid, sizeof(ids->nguid));
+			memcpy(ids->nguid, id->nguid, sizeof(ids->nguid));
 	}
-
+	kfree(id);
 	return 0;
-
-out_free_id:
-	kfree(*id);
-	return error;
 }
 
-static int nvme_identify_ns_cs_indep(struct nvme_ctrl *ctrl, unsigned nsid,
-			struct nvme_id_ns_cs_indep **id)
+static int nvme_ns_info_from_id_cs_indep(struct nvme_ctrl *ctrl,
+		struct nvme_ns_info *info)
 {
+	struct nvme_id_ns_cs_indep *id;
 	struct nvme_command c = {
 		.identify.opcode	= nvme_admin_identify,
-		.identify.nsid		= cpu_to_le32(nsid),
+		.identify.nsid		= cpu_to_le32(info->nsid),
 		.identify.cns		= NVME_ID_CNS_NS_CS_INDEP,
 	};
 	int ret;
 
-	*id = kmalloc(sizeof(**id), GFP_KERNEL);
-	if (!*id)
+	id = kmalloc(sizeof(*id), GFP_KERNEL);
+	if (!id)
 		return -ENOMEM;
 
-	ret = nvme_submit_sync_cmd(ctrl->admin_q, &c, *id, sizeof(**id));
-	if (ret) {
-		dev_warn(ctrl->device,
-			 "Identify namespace (CS independent) failed (%d)\n",
-			 ret);
-		kfree(*id);
-		return ret;
+	ret = nvme_submit_sync_cmd(ctrl->admin_q, &c, id, sizeof(*id));
+	if (!ret) {
+		info->anagrpid = id->anagrpid;
+		info->is_shared = id->nmic & NVME_NS_NMIC_SHARED;
+		info->is_ready = id->nstat & NVME_NSTAT_NRDY;
 	}
-
-	return 0;
+	kfree(id);
+	return ret;
 }
 
 static int nvme_features(struct nvme_ctrl *dev, u8 op, unsigned int fid,
@@ -1925,12 +1946,19 @@ static void nvme_set_chunk_sectors(struct nvme_ns *ns, struct nvme_id_ns *id)
 	blk_queue_chunk_sectors(ns->queue, iob);
 }
 
-static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id)
+static int nvme_update_ns_info_block(struct nvme_ns *ns,
+		struct nvme_ns_info *info)
 {
-	unsigned lbaf = nvme_lbaf_index(id->flbas);
+	struct nvme_id_ns *id;
+	unsigned lbaf;
 	int ret;
 
+	ret = nvme_identify_ns(ns->ctrl, info->nsid, &id);
+	if (ret)
+		return ret;
+
 	blk_mq_freeze_queue(ns->disk->queue);
+	lbaf = nvme_lbaf_index(id->flbas);
 	ns->lba_shift = id->lbaf[lbaf].ds;
 	nvme_set_queue_limits(ns->ctrl, ns->queue);
 
@@ -1967,6 +1995,7 @@ static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id)
 		disk_update_readahead(ns->head->disk);
 		blk_mq_unfreeze_queue(ns->head->disk->queue);
 	}
+	kfree(id);
 	return 0;
 
 out_unfreeze:
@@ -1980,9 +2009,30 @@ static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id)
 		ret = 0;
 	}
 	blk_mq_unfreeze_queue(ns->disk->queue);
+	kfree(id);
 	return ret;
 }
 
+static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_ns_info *info)
+{
+	switch (info->ids.csi) {
+	case NVME_CSI_ZNS:
+		if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED)) {
+			dev_warn(ns->ctrl->device,
+	"nsid %u not supported without CONFIG_BLK_DEV_ZONED\n",
+				info->nsid);
+			return -ENODEV;
+		}
+		return nvme_update_ns_info_block(ns, info);
+	case NVME_CSI_NVM:
+		return nvme_update_ns_info_block(ns, info);
+	default:
+		dev_warn(ns->ctrl->device, "unknown csi %u for nsid %u\n",
+			info->ids.csi, info->nsid);
+		return -ENODEV;
+	}
+}
+
 static char nvme_pr_type(enum pr_type type)
 {
 	switch (type) {
@@ -3911,7 +3961,7 @@ static int nvme_add_ns_cdev(struct nvme_ns *ns)
 }
 
 static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
-		unsigned nsid, struct nvme_ns_ids *ids)
+		struct nvme_ns_info *info)
 {
 	struct nvme_ns_head *head;
 	size_t size = sizeof(*head);
@@ -3933,8 +3983,9 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
 	if (ret)
 		goto out_ida_remove;
 	head->subsys = ctrl->subsys;
-	head->ns_id = nsid;
-	head->ids = *ids;
+	head->ns_id = info->nsid;
+	head->ids = info->ids;
+	head->shared = info->is_shared;
 	kref_init(&head->ref);
 
 	if (head->ids.csi) {
@@ -3991,55 +4042,54 @@ static int nvme_global_check_duplicate_ids(struct nvme_subsystem *this,
 	return ret;
 }
 
-static int nvme_init_ns_head(struct nvme_ns *ns, unsigned nsid,
-		struct nvme_ns_ids *ids, bool is_shared)
+static int nvme_init_ns_head(struct nvme_ns *ns, struct nvme_ns_info *info)
 {
 	struct nvme_ctrl *ctrl = ns->ctrl;
 	struct nvme_ns_head *head = NULL;
 	int ret;
 
-	ret = nvme_global_check_duplicate_ids(ctrl->subsys, ids);
+	ret = nvme_global_check_duplicate_ids(ctrl->subsys, &info->ids);
 	if (ret) {
 		dev_err(ctrl->device,
-			"globally duplicate IDs for nsid %d\n", nsid);
+			"globally duplicate IDs for nsid %d\n", info->nsid);
 		nvme_print_device_info(ctrl);
 		return ret;
 	}
 
 	mutex_lock(&ctrl->subsys->lock);
-	head = nvme_find_ns_head(ctrl, nsid);
+	head = nvme_find_ns_head(ctrl, info->nsid);
 	if (!head) {
-		ret = nvme_subsys_check_duplicate_ids(ctrl->subsys, ids);
+		ret = nvme_subsys_check_duplicate_ids(ctrl->subsys, &info->ids);
 		if (ret) {
 			dev_err(ctrl->device,
 				"duplicate IDs in subsystem for nsid %d\n",
-				nsid);
+				info->nsid);
 			goto out_unlock;
 		}
-		head = nvme_alloc_ns_head(ctrl, nsid, ids);
+		head = nvme_alloc_ns_head(ctrl, info);
 		if (IS_ERR(head)) {
 			ret = PTR_ERR(head);
 			goto out_unlock;
 		}
-		head->shared = is_shared;
 	} else {
 		ret = -EINVAL;
-		if (!is_shared || !head->shared) {
+		if (!info->is_shared || !head->shared) {
 			dev_err(ctrl->device,
-				"Duplicate unshared namespace %d\n", nsid);
+				"Duplicate unshared namespace %d\n",
+				info->nsid);
 			goto out_put_ns_head;
 		}
-		if (!nvme_ns_ids_equal(&head->ids, ids)) {
+		if (!nvme_ns_ids_equal(&head->ids, &info->ids)) {
 			dev_err(ctrl->device,
 				"IDs don't match for shared namespace %d\n",
-					nsid);
+					info->nsid);
 			goto out_put_ns_head;
 		}
 
 		if (!multipath && !list_empty(&head->list)) {
 			dev_warn(ctrl->device,
 				"Found shared namespace %d, but multipathing not supported.\n",
-				nsid);
+				info->nsid);
 			dev_warn_once(ctrl->device,
 				"Support for shared namespaces without CONFIG_NVME_MULTIPATH is deprecated and will be removed in Linux 6.0\n.");
 		}
@@ -4093,20 +4143,15 @@ static void nvme_ns_add_to_ctrl_list(struct nvme_ns *ns)
 	list_add(&ns->list, &ns->ctrl->namespaces);
 }
 
-static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
-		struct nvme_ns_ids *ids)
+static void nvme_alloc_ns(struct nvme_ctrl *ctrl, struct nvme_ns_info *info)
 {
 	struct nvme_ns *ns;
 	struct gendisk *disk;
-	struct nvme_id_ns *id;
 	int node = ctrl->numa_node;
 
-	if (nvme_identify_ns(ctrl, nsid, ids, &id))
-		return;
-
 	ns = kzalloc_node(sizeof(*ns), GFP_KERNEL, node);
 	if (!ns)
-		goto out_free_id;
+		return;
 
 	disk = blk_mq_alloc_disk(ctrl->tagset, ns);
 	if (IS_ERR(disk))
@@ -4127,7 +4172,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, info))
 		goto out_cleanup_disk;
 
 	/*
@@ -4153,7 +4198,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
 			ns->head->instance);
 	}
 
-	if (nvme_update_ns_info(ns, id))
+	if (nvme_update_ns_info(ns, info))
 		goto out_unlink_ns;
 
 	down_write(&ctrl->namespaces_rwsem);
@@ -4167,9 +4212,8 @@ 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, info->anagrpid);
 	nvme_fault_inject_init(&ns->fault_inject, ns->disk->disk_name);
-	kfree(id);
 
 	return;
 
@@ -4189,8 +4233,6 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
 	blk_cleanup_disk(disk);
  out_free_ns:
 	kfree(ns);
- out_free_id:
-	kfree(id);
 }
 
 static void nvme_ns_remove(struct nvme_ns *ns)
@@ -4249,29 +4291,21 @@ 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_info *info)
 {
-	struct nvme_id_ns *id;
 	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)) {
+	if (!nvme_ns_ids_equal(&ns->head->ids, &info->ids)) {
 		dev_err(ns->ctrl->device,
 			"identifiers changed for nsid %d\n", ns->head->ns_id);
-		goto out_free_id;
+		goto out;
 	}
 
-	ret = nvme_update_ns_info(ns, id);
-
-out_free_id:
-	kfree(id);
+	ret = nvme_update_ns_info(ns, info);
 out:
 	/*
 	 * Only remove the namespace if we got a fatal error back from the
@@ -4285,57 +4319,44 @@ static void nvme_validate_ns(struct nvme_ns *ns, struct nvme_ns_ids *ids)
 
 static void nvme_scan_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 {
-	struct nvme_ns_ids ids = { };
-	struct nvme_id_ns_cs_indep *id;
+	struct nvme_ns_info info = { .nsid = nsid };
 	struct nvme_ns *ns;
-	bool ready = true;
 
-	if (nvme_identify_ns_descs(ctrl, nsid, &ids))
+	if (nvme_identify_ns_descs(ctrl, &info))
 		return;
 
-	if (ids.csi != NVME_CSI_NVM && !nvme_multi_css(ctrl)) {
+	if (info.ids.csi != NVME_CSI_NVM && !nvme_multi_css(ctrl)) {
 		dev_warn(ctrl->device,
 			"command set not reported for nsid: %d\n", nsid);
 		return;
 	}
 
 	/*
-	 * Check if the namespace is ready.  If not ignore it, we will get an
-	 * AEN once it becomes ready and restart the scan.
+	 * If available try to use the Command Set Idependent Identify Namespace
+	 * data structure to find all the generic information that is needed to
+	 * set up a namespace.  If not fall back to the legacy version.
 	 */
-	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) {
+		if (nvme_ns_info_from_id_cs_indep(ctrl, &info))
+			return;
+	} else {
+		if (nvme_ns_info_from_identify(ctrl, &info))
+			return;
 	}
 
-	if (!ready)
+	/*
+	 * Ignore the namespace if it is not ready. We will get an AEN once it
+	 * becomes ready and restart the scan.
+	 */
+	if (!info.is_ready)
 		return;
 
 	ns = nvme_find_get_ns(ctrl, nsid);
 	if (ns) {
-		nvme_validate_ns(ns, &ids);
+		nvme_validate_ns(ns, &info);
 		nvme_put_ns(ns);
-		return;
-	}
-
-	switch (ids.csi) {
-	case NVME_CSI_NVM:
-		nvme_alloc_ns(ctrl, nsid, &ids);
-		break;
-	case NVME_CSI_ZNS:
-		if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED)) {
-			dev_warn(ctrl->device,
-				"nsid %u not supported without CONFIG_BLK_DEV_ZONED\n",
-				nsid);
-			break;
-		}
-		nvme_alloc_ns(ctrl, nsid, &ids);
-		break;
-	default:
-		dev_warn(ctrl->device, "unknown csi %u for nsid %u\n",
-			ids.csi, nsid);
-		break;
+	} else {
+		nvme_alloc_ns(ctrl, &info);
 	}
 }
 
-- 
2.30.2

Re: [PATCH 3/5] nvme: refactor namespace probing

[Sagi Grimberg]

> Change nvme_ns_scan to gather all information needed for generic
> namespace setup into a nvme_ns_info structure.  This structure is filled
> from the Command Set Idependent Identify Namespace data structure if
> it is available or else the legacy Identify namespace structure.
> 
> With that everything related to the NVM command set (and the ZNS command
> set derived from it) can be encapsulated in the nvme_update_ns_info_block
> function while keeping the rest of the namespace probing generic.
> 
> The downside is that we now always issue two Identify Namespace calls for
> each probed namespace instead of usually just a single one previously.

Maybe nvme_ns_info can have a id reference such that 
nvme_update_ns_info_block can reuse it?

Not that I care all that much, but it does seem redundant.

Re: [PATCH 3/5] nvme: refactor namespace probing

[Christoph Hellwig]

On Wed, Jul 13, 2022 at 12:21:51PM +0300, Sagi Grimberg wrote:
> Maybe nvme_ns_info can have a id reference such that 
> nvme_update_ns_info_block can reuse it?

I thought about it and, and it does seem doable.  But it just seems
like an awful lack of abstraction, all for avoiding a single command
on some setups.  True, those are the absolute majority right now, but
with more and more information going to go into the comman set
independent identify page it will become less and less so going
forward.

Re: [PATCH 3/5] nvme: refactor namespace probing

[Sagi Grimberg]

>> Maybe nvme_ns_info can have a id reference such that
>> nvme_update_ns_info_block can reuse it?
> 
> I thought about it and, and it does seem doable.  But it just seems
> like an awful lack of abstraction, all for avoiding a single command
> on some setups.  True, those are the absolute majority right now, but
> with more and more information going to go into the comman set
> independent identify page it will become less and less so going
> forward.

I don't think its that bad, and it saves fetching the same data
twice. But I don't really care either way...

Re: [PATCH 3/5] nvme: refactor namespace probing

[Kanchan Joshi]

[-- Attachment #1: Type: text/plain, Size: 1346 bytes --]

On Wed, Jul 13, 2022 at 07:49:12AM +0200, Christoph Hellwig wrote:

> static void nvme_scan_ns(struct nvme_ctrl *ctrl, unsigned nsid)
> {
>-	struct nvme_ns_ids ids = { };
>-	struct nvme_id_ns_cs_indep *id;
>+	struct nvme_ns_info info = { .nsid = nsid };
> 	struct nvme_ns *ns;
>-	bool ready = true;
>
>-	if (nvme_identify_ns_descs(ctrl, nsid, &ids))
>+	if (nvme_identify_ns_descs(ctrl, &info))
> 		return;
>
>-	if (ids.csi != NVME_CSI_NVM && !nvme_multi_css(ctrl)) {
>+	if (info.ids.csi != NVME_CSI_NVM && !nvme_multi_css(ctrl)) {
> 		dev_warn(ctrl->device,
> 			"command set not reported for nsid: %d\n", nsid);
> 		return;
> 	}
>
> 	/*
>-	 * Check if the namespace is ready.  If not ignore it, we will get an
>-	 * AEN once it becomes ready and restart the scan.
>+	 * If available try to use the Command Set Idependent Identify Namespace
>+	 * data structure to find all the generic information that is needed to
>+	 * set up a namespace.  If not fall back to the legacy version.
> 	 */
>-	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) {
>+		if (nvme_ns_info_from_id_cs_indep(ctrl, &info))
>+			return;

Is this return fine if independent id-ns fails? Earlier behavior was to
ingore such failure.


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 3/5] nvme: refactor namespace probing

[Christoph Hellwig]

On Wed, Jul 13, 2022 at 03:47:00PM +0530, Kanchan Joshi wrote:
>> -	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) {
>> +		if (nvme_ns_info_from_id_cs_indep(ctrl, &info))
>> +			return;
>
> Is this return fine if independent id-ns fails? Earlier behavior was to
> ingore such failure.

Good point.  If NVME_CAP_CRMS_CRIMS is supported we really need to
support this log page, and later on for the unknown command sets
we need as well.  So I don't think we can do without it here, but
maybe that is worth mentioning in the commit log

[PATCH 4/5] nvme: factor out a nvme_ns_is_readonly helper

[Christoph Hellwig]

Add a little helper to check if a namespace should be marked read-only
that uses a new is_readonly flag in the nvme_ns_info structure.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/host/core.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 659068f90165c..dca7580f785a8 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -36,6 +36,7 @@ struct nvme_ns_info {
 	u32 nsid;
 	__le32 anagrpid;
 	bool is_shared;
+	bool is_readonly;
 	bool is_ready;
 };
 
@@ -1447,6 +1448,7 @@ static int nvme_ns_info_from_identify(struct nvme_ctrl *ctrl,
 		return ret;
 	info->anagrpid = id->anagrpid;
 	info->is_shared = id->nmic & NVME_NS_NMIC_SHARED;
+	info->is_readonly = id->nsattr & NVME_NS_ATTR_RO;
 	info->is_ready = true;
 	if (ctrl->quirks & NVME_QUIRK_BOGUS_NID) {
 		dev_info(ctrl->device,
@@ -1482,6 +1484,7 @@ static int nvme_ns_info_from_id_cs_indep(struct nvme_ctrl *ctrl,
 	if (!ret) {
 		info->anagrpid = id->anagrpid;
 		info->is_shared = id->nmic & NVME_NS_NMIC_SHARED;
+		info->is_readonly = id->nsattr & NVME_NS_ATTR_RO;
 		info->is_ready = id->nstat & NVME_NSTAT_NRDY;
 	}
 	kfree(id);
@@ -1909,6 +1912,11 @@ static void nvme_update_disk_info(struct gendisk *disk,
 					   ns->ctrl->max_zeroes_sectors);
 }
 
+static bool nvme_ns_is_readonly(struct nvme_ns *ns, struct nvme_ns_info *info)
+{
+	return info->is_readonly || test_bit(NVME_NS_FORCE_RO, &ns->flags);
+}
+
 static inline bool nvme_first_scan(struct gendisk *disk)
 {
 	/* nvme_alloc_ns() scans the disk prior to adding it */
@@ -1972,8 +1980,7 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
 			goto out_unfreeze;
 	}
 
-	set_disk_ro(ns->disk, (id->nsattr & NVME_NS_ATTR_RO) ||
-		test_bit(NVME_NS_FORCE_RO, &ns->flags));
+	set_disk_ro(ns->disk, nvme_ns_is_readonly(ns, info));
 	set_bit(NVME_NS_READY, &ns->flags);
 	blk_mq_unfreeze_queue(ns->disk->queue);
 
@@ -1986,9 +1993,7 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
 	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));
+		set_disk_ro(ns->head->disk, nvme_ns_is_readonly(ns, info));
 		nvme_mpath_revalidate_paths(ns);
 		blk_stack_limits(&ns->head->disk->queue->limits,
 				 &ns->queue->limits, 0);
-- 
2.30.2

Re: [PATCH 4/5] nvme: factor out a nvme_ns_is_readonly helper

[Chaitanya Kulkarni]

On 7/12/22 22:49, Christoph Hellwig wrote:
> Add a little helper to check if a namespace should be marked read-only
> that uses a new is_readonly flag in the nvme_ns_info structure.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH 4/5] nvme: factor out a nvme_ns_is_readonly helper

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH 4/5] nvme: factor out a nvme_ns_is_readonly helper

[Kanchan Joshi]

[-- Attachment #1: Type: text/plain, Size: 303 bytes --]

On Wed, Jul 13, 2022 at 07:49:13AM +0200, Christoph Hellwig wrote:
>Add a little helper to check if a namespace should be marked read-only
>that uses a new is_readonly flag in the nvme_ns_info structure.
>
>Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Christoph Hellwig]

From: Joel Granados <j.granados@samsung.com>

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>
[hch: rebased on other refactoring patches]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/host/core.c | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index dca7580f785a8..f865d0b185c32 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1954,6 +1954,31 @@ static void nvme_set_chunk_sectors(struct nvme_ns *ns, struct nvme_id_ns *id)
 	blk_queue_chunk_sectors(ns->queue, iob);
 }
 
+static int nvme_update_ns_info_generic(struct nvme_ns *ns,
+		struct nvme_ns_info *info)
+{
+	blk_mq_freeze_queue(ns->disk->queue);
+	nvme_set_queue_limits(ns->ctrl, ns->queue);
+	set_disk_ro(ns->disk, nvme_ns_is_readonly(ns, info));
+	blk_mq_unfreeze_queue(ns->disk->queue);
+
+	if (nvme_ns_head_multipath(ns->head)) {
+		blk_mq_freeze_queue(ns->head->disk->queue);
+		set_disk_ro(ns->head->disk, nvme_ns_is_readonly(ns, info));
+		nvme_mpath_revalidate_paths(ns);
+		blk_stack_limits(&ns->head->disk->queue->limits,
+				 &ns->queue->limits, 0);
+		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_block(struct nvme_ns *ns,
 		struct nvme_ns_info *info)
 {
@@ -2024,17 +2049,15 @@ static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_ns_info *info)
 	case NVME_CSI_ZNS:
 		if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED)) {
 			dev_warn(ns->ctrl->device,
-	"nsid %u not supported without CONFIG_BLK_DEV_ZONED\n",
+	"block device for nsid %u not supported without CONFIG_BLK_DEV_ZONED\n",
 				info->nsid);
-			return -ENODEV;
+			return nvme_update_ns_info_generic(ns, info);
 		}
 		return nvme_update_ns_info_block(ns, info);
 	case NVME_CSI_NVM:
 		return nvme_update_ns_info_block(ns, info);
 	default:
-		dev_warn(ns->ctrl->device, "unknown csi %u for nsid %u\n",
-			info->ids.csi, info->nsid);
-		return -ENODEV;
+		return nvme_update_ns_info_generic(ns, info);
 	}
 }
 
@@ -4341,7 +4364,8 @@ static void nvme_scan_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 	 * data structure to find all the generic information that is needed to
 	 * set up a namespace.  If not fall back to the legacy version.
 	 */
-	if (ctrl->cap & NVME_CAP_CRMS_CRIMS) {
+	if ((ctrl->cap & NVME_CAP_CRMS_CRIMS) ||
+	    (info.ids.csi != NVME_CSI_NVM && info.ids.csi != NVME_CSI_ZNS)) {
 		if (nvme_ns_info_from_id_cs_indep(ctrl, &info))
 			return;
 	} else {
-- 
2.30.2

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

[Sagi Grimberg]

On 7/13/22 08:49, Christoph Hellwig wrote:
> From: Joel Granados <j.granados@samsung.com>
> 
> 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>
> [hch: rebased on other refactoring patches]
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/nvme/host/core.c | 36 ++++++++++++++++++++++++++++++------
>   1 file changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index dca7580f785a8..f865d0b185c32 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1954,6 +1954,31 @@ static void nvme_set_chunk_sectors(struct nvme_ns *ns, struct nvme_id_ns *id)
>   	blk_queue_chunk_sectors(ns->queue, iob);
>   }
>   
> +static int nvme_update_ns_info_generic(struct nvme_ns *ns,
> +		struct nvme_ns_info *info)
> +{
> +	blk_mq_freeze_queue(ns->disk->queue);
> +	nvme_set_queue_limits(ns->ctrl, ns->queue);
> +	set_disk_ro(ns->disk, nvme_ns_is_readonly(ns, info));
> +	blk_mq_unfreeze_queue(ns->disk->queue);
> +
> +	if (nvme_ns_head_multipath(ns->head)) {
> +		blk_mq_freeze_queue(ns->head->disk->queue);
> +		set_disk_ro(ns->head->disk, nvme_ns_is_readonly(ns, info));
> +		nvme_mpath_revalidate_paths(ns);
> +		blk_stack_limits(&ns->head->disk->queue->limits,
> +				 &ns->queue->limits, 0);
> +		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_block(struct nvme_ns *ns,
>   		struct nvme_ns_info *info)
>   {
> @@ -2024,17 +2049,15 @@ static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_ns_info *info)
>   	case NVME_CSI_ZNS:
>   		if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED)) {
>   			dev_warn(ns->ctrl->device,
> -	"nsid %u not supported without CONFIG_BLK_DEV_ZONED\n",
> +	"block device for nsid %u not supported without CONFIG_BLK_DEV_ZONED\n",
>   				info->nsid);
> -			return -ENODEV;
> +			return nvme_update_ns_info_generic(ns, info);
>   		}
>   		return nvme_update_ns_info_block(ns, info);
>   	case NVME_CSI_NVM:
>   		return nvme_update_ns_info_block(ns, info);
>   	default:
> -		dev_warn(ns->ctrl->device, "unknown csi %u for nsid %u\n",
> -			info->ids.csi, info->nsid);
> -		return -ENODEV;

Maybe we want to keep the log? can be converted to dev_info/dev_dbg, but
seems useful to have.

> +		return nvme_update_ns_info_generic(ns, info);
>   	}
>   }
>   
> @@ -4341,7 +4364,8 @@ static void nvme_scan_ns(struct nvme_ctrl *ctrl, unsigned nsid)
>   	 * data structure to find all the generic information that is needed to
>   	 * set up a namespace.  If not fall back to the legacy version.
>   	 */
> -	if (ctrl->cap & NVME_CAP_CRMS_CRIMS) {
> +	if ((ctrl->cap & NVME_CAP_CRMS_CRIMS) ||
> +	    (info.ids.csi != NVME_CSI_NVM && info.ids.csi != NVME_CSI_ZNS)) {
>   		if (nvme_ns_info_from_id_cs_indep(ctrl, &info))
>   			return;
>   	} else {

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

[Christoph Hellwig]

On Wed, Jul 13, 2022 at 12:25:38PM +0300, Sagi Grimberg wrote:
>>   	default:
>> -		dev_warn(ns->ctrl->device, "unknown csi %u for nsid %u\n",
>> -			info->ids.csi, info->nsid);
>> -		return -ENODEV;
>
> Maybe we want to keep the log? can be converted to dev_info/dev_dbg, but
> seems useful to have.

As-is I think it is more suitable for a dev_dbg as it sound like
gibberish to a user..

Re: enable generic interface (/dev/ngX) for unknown command sets

[Javier González]

On 13.07.2022 07:49, Christoph Hellwig wrote:
>Hi all,
>
>this is a version of the patch from Joel rebased on a bunch of cleanups
>from me that I think make the layering a lot cleaner.  Let me know what
>everyone things.

Series look good. Thanks for doing the rebase Christoph.

Reviewed-by: Javier González <javier.gonz@samsung.com>

enable generic interface (/dev/ngX) for unknown command sets v2

[Christoph Hellwig]

Hi all,

this is a version of the patch from Joel rebased on a bunch of cleanups
from me that I think make the layering a lot cleaner.  Let me know what
everyone things.

Changes since v1:
 - log consistent messages when only passthrough is supported for
   namespaces

[PATCH 3/5] nvme: refactor namespace probing

[Christoph Hellwig]

Change nvme_ns_scan to gather all information needed for generic
namespace setup into a nvme_ns_info structure.  This structure is filled
from the Command Set Idependent Identify Namespace data structure if
it is available or else the legacy Identify namespace structure.

With that everything related to the NVM command set (and the ZNS command
set derived from it) can be encapsulated in the nvme_update_ns_info_block
function while keeping the rest of the namespace probing generic.

The downside is that we now always issue two Identify Namespace calls for
each probed namespace instead of usually just a single one previously.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Javier González <javier.gonz@samsung.com>
---
 drivers/nvme/host/core.c | 231 +++++++++++++++++++++------------------
 1 file changed, 126 insertions(+), 105 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 4769c49141913..659068f90165c 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -31,6 +31,14 @@
 
 #define NVME_MINORS		(1U << MINORBITS)
 
+struct nvme_ns_info {
+	struct nvme_ns_ids ids;
+	u32 nsid;
+	__le32 anagrpid;
+	bool is_shared;
+	bool is_ready;
+};
+
 unsigned int admin_timeout = 60;
 module_param(admin_timeout, uint, 0644);
 MODULE_PARM_DESC(admin_timeout, "timeout in seconds for admin commands");
@@ -1342,8 +1350,8 @@ static int nvme_process_ns_desc(struct nvme_ctrl *ctrl, struct nvme_ns_ids *ids,
 	}
 }
 
-static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl, unsigned nsid,
-		struct nvme_ns_ids *ids)
+static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl,
+		struct nvme_ns_info *info)
 {
 	struct nvme_command c = { };
 	bool csi_seen = false;
@@ -1356,7 +1364,7 @@ static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl, unsigned nsid,
 		return 0;
 
 	c.identify.opcode = nvme_admin_identify;
-	c.identify.nsid = cpu_to_le32(nsid);
+	c.identify.nsid = cpu_to_le32(info->nsid);
 	c.identify.cns = NVME_ID_CNS_NS_DESC_LIST;
 
 	data = kzalloc(NVME_IDENTIFY_DATA_SIZE, GFP_KERNEL);
@@ -1368,7 +1376,7 @@ static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl, unsigned nsid,
 	if (status) {
 		dev_warn(ctrl->device,
 			"Identify Descriptors failed (nsid=%u, status=0x%x)\n",
-			nsid, status);
+			info->nsid, status);
 		goto free_data;
 	}
 
@@ -1378,7 +1386,7 @@ static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl, unsigned nsid,
 		if (cur->nidl == 0)
 			break;
 
-		len = nvme_process_ns_desc(ctrl, ids, cur, &csi_seen);
+		len = nvme_process_ns_desc(ctrl, &info->ids, cur, &csi_seen);
 		if (len < 0)
 			break;
 
@@ -1387,7 +1395,7 @@ static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl, unsigned nsid,
 
 	if (nvme_multi_css(ctrl) && !csi_seen) {
 		dev_warn(ctrl->device, "Command set not reported for nsid:%d\n",
-			 nsid);
+			 info->nsid);
 		status = -EINVAL;
 	}
 
@@ -1397,7 +1405,7 @@ static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl, unsigned nsid,
 }
 
 static int nvme_identify_ns(struct nvme_ctrl *ctrl, unsigned nsid,
-			struct nvme_ns_ids *ids, struct nvme_id_ns **id)
+			struct nvme_id_ns **id)
 {
 	struct nvme_command c = { };
 	int error;
@@ -1420,51 +1428,64 @@ static int nvme_identify_ns(struct nvme_ctrl *ctrl, unsigned nsid,
 	error = NVME_SC_INVALID_NS | NVME_SC_DNR;
 	if ((*id)->ncap == 0) /* namespace not allocated or attached */
 		goto out_free_id;
+	return 0;
 
+out_free_id:
+	kfree(*id);
+	return error;
+}
 
+static int nvme_ns_info_from_identify(struct nvme_ctrl *ctrl,
+		struct nvme_ns_info *info)
+{
+	struct nvme_ns_ids *ids = &info->ids;
+	struct nvme_id_ns *id;
+	int ret;
+
+	ret = nvme_identify_ns(ctrl, info->nsid, &id);
+	if (ret)
+		return ret;
+	info->anagrpid = id->anagrpid;
+	info->is_shared = id->nmic & NVME_NS_NMIC_SHARED;
+	info->is_ready = true;
 	if (ctrl->quirks & NVME_QUIRK_BOGUS_NID) {
 		dev_info(ctrl->device,
 			 "Ignoring bogus Namespace Identifiers\n");
 	} else {
 		if (ctrl->vs >= NVME_VS(1, 1, 0) &&
 		    !memchr_inv(ids->eui64, 0, sizeof(ids->eui64)))
-			memcpy(ids->eui64, (*id)->eui64, sizeof(ids->eui64));
+			memcpy(ids->eui64, id->eui64, sizeof(ids->eui64));
 		if (ctrl->vs >= NVME_VS(1, 2, 0) &&
 		    !memchr_inv(ids->nguid, 0, sizeof(ids->nguid)))
-			memcpy(ids->nguid, (*id)->nguid, sizeof(ids->nguid));
+			memcpy(ids->nguid, id->nguid, sizeof(ids->nguid));
 	}
-
+	kfree(id);
 	return 0;
-
-out_free_id:
-	kfree(*id);
-	return error;
 }
 
-static int nvme_identify_ns_cs_indep(struct nvme_ctrl *ctrl, unsigned nsid,
-			struct nvme_id_ns_cs_indep **id)
+static int nvme_ns_info_from_id_cs_indep(struct nvme_ctrl *ctrl,
+		struct nvme_ns_info *info)
 {
+	struct nvme_id_ns_cs_indep *id;
 	struct nvme_command c = {
 		.identify.opcode	= nvme_admin_identify,
-		.identify.nsid		= cpu_to_le32(nsid),
+		.identify.nsid		= cpu_to_le32(info->nsid),
 		.identify.cns		= NVME_ID_CNS_NS_CS_INDEP,
 	};
 	int ret;
 
-	*id = kmalloc(sizeof(**id), GFP_KERNEL);
-	if (!*id)
+	id = kmalloc(sizeof(*id), GFP_KERNEL);
+	if (!id)
 		return -ENOMEM;
 
-	ret = nvme_submit_sync_cmd(ctrl->admin_q, &c, *id, sizeof(**id));
-	if (ret) {
-		dev_warn(ctrl->device,
-			 "Identify namespace (CS independent) failed (%d)\n",
-			 ret);
-		kfree(*id);
-		return ret;
+	ret = nvme_submit_sync_cmd(ctrl->admin_q, &c, id, sizeof(*id));
+	if (!ret) {
+		info->anagrpid = id->anagrpid;
+		info->is_shared = id->nmic & NVME_NS_NMIC_SHARED;
+		info->is_ready = id->nstat & NVME_NSTAT_NRDY;
 	}
-
-	return 0;
+	kfree(id);
+	return ret;
 }
 
 static int nvme_features(struct nvme_ctrl *dev, u8 op, unsigned int fid,
@@ -1925,12 +1946,19 @@ static void nvme_set_chunk_sectors(struct nvme_ns *ns, struct nvme_id_ns *id)
 	blk_queue_chunk_sectors(ns->queue, iob);
 }
 
-static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id)
+static int nvme_update_ns_info_block(struct nvme_ns *ns,
+		struct nvme_ns_info *info)
 {
-	unsigned lbaf = nvme_lbaf_index(id->flbas);
+	struct nvme_id_ns *id;
+	unsigned lbaf;
 	int ret;
 
+	ret = nvme_identify_ns(ns->ctrl, info->nsid, &id);
+	if (ret)
+		return ret;
+
 	blk_mq_freeze_queue(ns->disk->queue);
+	lbaf = nvme_lbaf_index(id->flbas);
 	ns->lba_shift = id->lbaf[lbaf].ds;
 	nvme_set_queue_limits(ns->ctrl, ns->queue);
 
@@ -1967,6 +1995,7 @@ static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id)
 		disk_update_readahead(ns->head->disk);
 		blk_mq_unfreeze_queue(ns->head->disk->queue);
 	}
+	kfree(id);
 	return 0;
 
 out_unfreeze:
@@ -1980,9 +2009,30 @@ static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id)
 		ret = 0;
 	}
 	blk_mq_unfreeze_queue(ns->disk->queue);
+	kfree(id);
 	return ret;
 }
 
+static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_ns_info *info)
+{
+	switch (info->ids.csi) {
+	case NVME_CSI_ZNS:
+		if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED)) {
+			dev_warn(ns->ctrl->device,
+	"nsid %u not supported without CONFIG_BLK_DEV_ZONED\n",
+				info->nsid);
+			return -ENODEV;
+		}
+		return nvme_update_ns_info_block(ns, info);
+	case NVME_CSI_NVM:
+		return nvme_update_ns_info_block(ns, info);
+	default:
+		dev_warn(ns->ctrl->device, "unknown csi %u for nsid %u\n",
+			info->ids.csi, info->nsid);
+		return -ENODEV;
+	}
+}
+
 static char nvme_pr_type(enum pr_type type)
 {
 	switch (type) {
@@ -3911,7 +3961,7 @@ static int nvme_add_ns_cdev(struct nvme_ns *ns)
 }
 
 static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
-		unsigned nsid, struct nvme_ns_ids *ids)
+		struct nvme_ns_info *info)
 {
 	struct nvme_ns_head *head;
 	size_t size = sizeof(*head);
@@ -3933,8 +3983,9 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
 	if (ret)
 		goto out_ida_remove;
 	head->subsys = ctrl->subsys;
-	head->ns_id = nsid;
-	head->ids = *ids;
+	head->ns_id = info->nsid;
+	head->ids = info->ids;
+	head->shared = info->is_shared;
 	kref_init(&head->ref);
 
 	if (head->ids.csi) {
@@ -3991,55 +4042,54 @@ static int nvme_global_check_duplicate_ids(struct nvme_subsystem *this,
 	return ret;
 }
 
-static int nvme_init_ns_head(struct nvme_ns *ns, unsigned nsid,
-		struct nvme_ns_ids *ids, bool is_shared)
+static int nvme_init_ns_head(struct nvme_ns *ns, struct nvme_ns_info *info)
 {
 	struct nvme_ctrl *ctrl = ns->ctrl;
 	struct nvme_ns_head *head = NULL;
 	int ret;
 
-	ret = nvme_global_check_duplicate_ids(ctrl->subsys, ids);
+	ret = nvme_global_check_duplicate_ids(ctrl->subsys, &info->ids);
 	if (ret) {
 		dev_err(ctrl->device,
-			"globally duplicate IDs for nsid %d\n", nsid);
+			"globally duplicate IDs for nsid %d\n", info->nsid);
 		nvme_print_device_info(ctrl);
 		return ret;
 	}
 
 	mutex_lock(&ctrl->subsys->lock);
-	head = nvme_find_ns_head(ctrl, nsid);
+	head = nvme_find_ns_head(ctrl, info->nsid);
 	if (!head) {
-		ret = nvme_subsys_check_duplicate_ids(ctrl->subsys, ids);
+		ret = nvme_subsys_check_duplicate_ids(ctrl->subsys, &info->ids);
 		if (ret) {
 			dev_err(ctrl->device,
 				"duplicate IDs in subsystem for nsid %d\n",
-				nsid);
+				info->nsid);
 			goto out_unlock;
 		}
-		head = nvme_alloc_ns_head(ctrl, nsid, ids);
+		head = nvme_alloc_ns_head(ctrl, info);
 		if (IS_ERR(head)) {
 			ret = PTR_ERR(head);
 			goto out_unlock;
 		}
-		head->shared = is_shared;
 	} else {
 		ret = -EINVAL;
-		if (!is_shared || !head->shared) {
+		if (!info->is_shared || !head->shared) {
 			dev_err(ctrl->device,
-				"Duplicate unshared namespace %d\n", nsid);
+				"Duplicate unshared namespace %d\n",
+				info->nsid);
 			goto out_put_ns_head;
 		}
-		if (!nvme_ns_ids_equal(&head->ids, ids)) {
+		if (!nvme_ns_ids_equal(&head->ids, &info->ids)) {
 			dev_err(ctrl->device,
 				"IDs don't match for shared namespace %d\n",
-					nsid);
+					info->nsid);
 			goto out_put_ns_head;
 		}
 
 		if (!multipath && !list_empty(&head->list)) {
 			dev_warn(ctrl->device,
 				"Found shared namespace %d, but multipathing not supported.\n",
-				nsid);
+				info->nsid);
 			dev_warn_once(ctrl->device,
 				"Support for shared namespaces without CONFIG_NVME_MULTIPATH is deprecated and will be removed in Linux 6.0\n.");
 		}
@@ -4093,20 +4143,15 @@ static void nvme_ns_add_to_ctrl_list(struct nvme_ns *ns)
 	list_add(&ns->list, &ns->ctrl->namespaces);
 }
 
-static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
-		struct nvme_ns_ids *ids)
+static void nvme_alloc_ns(struct nvme_ctrl *ctrl, struct nvme_ns_info *info)
 {
 	struct nvme_ns *ns;
 	struct gendisk *disk;
-	struct nvme_id_ns *id;
 	int node = ctrl->numa_node;
 
-	if (nvme_identify_ns(ctrl, nsid, ids, &id))
-		return;
-
 	ns = kzalloc_node(sizeof(*ns), GFP_KERNEL, node);
 	if (!ns)
-		goto out_free_id;
+		return;
 
 	disk = blk_mq_alloc_disk(ctrl->tagset, ns);
 	if (IS_ERR(disk))
@@ -4127,7 +4172,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, info))
 		goto out_cleanup_disk;
 
 	/*
@@ -4153,7 +4198,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
 			ns->head->instance);
 	}
 
-	if (nvme_update_ns_info(ns, id))
+	if (nvme_update_ns_info(ns, info))
 		goto out_unlink_ns;
 
 	down_write(&ctrl->namespaces_rwsem);
@@ -4167,9 +4212,8 @@ 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, info->anagrpid);
 	nvme_fault_inject_init(&ns->fault_inject, ns->disk->disk_name);
-	kfree(id);
 
 	return;
 
@@ -4189,8 +4233,6 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
 	blk_cleanup_disk(disk);
  out_free_ns:
 	kfree(ns);
- out_free_id:
-	kfree(id);
 }
 
 static void nvme_ns_remove(struct nvme_ns *ns)
@@ -4249,29 +4291,21 @@ 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_info *info)
 {
-	struct nvme_id_ns *id;
 	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)) {
+	if (!nvme_ns_ids_equal(&ns->head->ids, &info->ids)) {
 		dev_err(ns->ctrl->device,
 			"identifiers changed for nsid %d\n", ns->head->ns_id);
-		goto out_free_id;
+		goto out;
 	}
 
-	ret = nvme_update_ns_info(ns, id);
-
-out_free_id:
-	kfree(id);
+	ret = nvme_update_ns_info(ns, info);
 out:
 	/*
 	 * Only remove the namespace if we got a fatal error back from the
@@ -4285,57 +4319,44 @@ static void nvme_validate_ns(struct nvme_ns *ns, struct nvme_ns_ids *ids)
 
 static void nvme_scan_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 {
-	struct nvme_ns_ids ids = { };
-	struct nvme_id_ns_cs_indep *id;
+	struct nvme_ns_info info = { .nsid = nsid };
 	struct nvme_ns *ns;
-	bool ready = true;
 
-	if (nvme_identify_ns_descs(ctrl, nsid, &ids))
+	if (nvme_identify_ns_descs(ctrl, &info))
 		return;
 
-	if (ids.csi != NVME_CSI_NVM && !nvme_multi_css(ctrl)) {
+	if (info.ids.csi != NVME_CSI_NVM && !nvme_multi_css(ctrl)) {
 		dev_warn(ctrl->device,
 			"command set not reported for nsid: %d\n", nsid);
 		return;
 	}
 
 	/*
-	 * Check if the namespace is ready.  If not ignore it, we will get an
-	 * AEN once it becomes ready and restart the scan.
+	 * If available try to use the Command Set Idependent Identify Namespace
+	 * data structure to find all the generic information that is needed to
+	 * set up a namespace.  If not fall back to the legacy version.
 	 */
-	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) {
+		if (nvme_ns_info_from_id_cs_indep(ctrl, &info))
+			return;
+	} else {
+		if (nvme_ns_info_from_identify(ctrl, &info))
+			return;
 	}
 
-	if (!ready)
+	/*
+	 * Ignore the namespace if it is not ready. We will get an AEN once it
+	 * becomes ready and restart the scan.
+	 */
+	if (!info.is_ready)
 		return;
 
 	ns = nvme_find_get_ns(ctrl, nsid);
 	if (ns) {
-		nvme_validate_ns(ns, &ids);
+		nvme_validate_ns(ns, &info);
 		nvme_put_ns(ns);
-		return;
-	}
-
-	switch (ids.csi) {
-	case NVME_CSI_NVM:
-		nvme_alloc_ns(ctrl, nsid, &ids);
-		break;
-	case NVME_CSI_ZNS:
-		if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED)) {
-			dev_warn(ctrl->device,
-				"nsid %u not supported without CONFIG_BLK_DEV_ZONED\n",
-				nsid);
-			break;
-		}
-		nvme_alloc_ns(ctrl, nsid, &ids);
-		break;
-	default:
-		dev_warn(ctrl->device, "unknown csi %u for nsid %u\n",
-			ids.csi, nsid);
-		break;
+	} else {
+		nvme_alloc_ns(ctrl, &info);
 	}
 }
 
-- 
2.30.2

Re: [PATCH 3/5] nvme: refactor namespace probing

[Kanchan Joshi]

[-- Attachment #1: Type: text/plain, Size: 1735 bytes --]

On Mon, Jul 18, 2022 at 07:25:01AM +0200, Christoph Hellwig wrote:

>-static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id)
>+static int nvme_update_ns_info_block(struct nvme_ns *ns,
>+		struct nvme_ns_info *info)
> {
>-	unsigned lbaf = nvme_lbaf_index(id->flbas);
>+	struct nvme_id_ns *id;
>+	unsigned lbaf;
> 	int ret;
>
>+	ret = nvme_identify_ns(ns->ctrl, info->nsid, &id);
>+	if (ret)
>+		return ret;
>+
> 	blk_mq_freeze_queue(ns->disk->queue);
>+	lbaf = nvme_lbaf_index(id->flbas);
> 	ns->lba_shift = id->lbaf[lbaf].ds;
> 	nvme_set_queue_limits(ns->ctrl, ns->queue);
>
>@@ -1967,6 +1995,7 @@ static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id)
> 		disk_update_readahead(ns->head->disk);
> 		blk_mq_unfreeze_queue(ns->head->disk->queue);
> 	}
>+	kfree(id);
> 	return 0;
>
> out_unfreeze:
>@@ -1980,9 +2009,30 @@ static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id)
> 		ret = 0;
> 	}
> 	blk_mq_unfreeze_queue(ns->disk->queue);
>+	kfree(id);
> 	return ret;

Seems this new function (nvme_update_ns_info_block) requires kfree at one more place?
This one:

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index bbdf0f95331f..5a917211e377 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2011,8 +2011,10 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,

        if (blk_queue_is_zoned(ns->queue)) {
                ret = nvme_revalidate_zones(ns);
-               if (ret && !nvme_first_scan(ns->disk))
+               if (ret && !nvme_first_scan(ns->disk)) {
+                       kfree(id);
                        return ret;
+               }
        }
	
With that-
Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 3/5] nvme: refactor namespace probing

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 2197 bytes --]

On Tue, Jul 19, 2022 at 12:38:45PM +0530, Kanchan Joshi wrote:
> On Mon, Jul 18, 2022 at 07:25:01AM +0200, Christoph Hellwig wrote:
> 
> > -static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id)
> > +static int nvme_update_ns_info_block(struct nvme_ns *ns,
> > +		struct nvme_ns_info *info)
> > {
> > -	unsigned lbaf = nvme_lbaf_index(id->flbas);
> > +	struct nvme_id_ns *id;
> > +	unsigned lbaf;
> > 	int ret;
> > 
> > +	ret = nvme_identify_ns(ns->ctrl, info->nsid, &id);
> > +	if (ret)
> > +		return ret;
> > +
> > 	blk_mq_freeze_queue(ns->disk->queue);
> > +	lbaf = nvme_lbaf_index(id->flbas);
> > 	ns->lba_shift = id->lbaf[lbaf].ds;
> > 	nvme_set_queue_limits(ns->ctrl, ns->queue);
> > 
> > @@ -1967,6 +1995,7 @@ static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id)
> > 		disk_update_readahead(ns->head->disk);
> > 		blk_mq_unfreeze_queue(ns->head->disk->queue);
> > 	}
> > +	kfree(id);
> > 	return 0;
> > 
> > out_unfreeze:
> > @@ -1980,9 +2009,30 @@ static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id)
> > 		ret = 0;
> > 	}
> > 	blk_mq_unfreeze_queue(ns->disk->queue);
> > +	kfree(id);
> > 	return ret;
> 
> Seems this new function (nvme_update_ns_info_block) requires kfree at one more place?
> This one:
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index bbdf0f95331f..5a917211e377 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2011,8 +2011,10 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
> 
>        if (blk_queue_is_zoned(ns->queue)) {
>                ret = nvme_revalidate_zones(ns);
> -               if (ret && !nvme_first_scan(ns->disk))
> +               if (ret && !nvme_first_scan(ns->disk)) {
> +                       kfree(id);
>                        return ret;
> +               }
>        }
> 	

Should we also take care to hide the device?
As I read it the call to nvme_revalidate_zones can return a -ENODEV
(through blk_revalidate_disk_zones) which is the original condition to
hide the block device.

> With that-
> Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 3/5] nvme: refactor namespace probing

[Christoph Hellwig]

On Tue, Jul 19, 2022 at 03:00:48PM +0200, Joel Granados wrote:
> > +++ b/drivers/nvme/host/core.c
> > @@ -2011,8 +2011,10 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
> > 
> >        if (blk_queue_is_zoned(ns->queue)) {
> >                ret = nvme_revalidate_zones(ns);
> > -               if (ret && !nvme_first_scan(ns->disk))
> > +               if (ret && !nvme_first_scan(ns->disk)) {
> > +                       kfree(id);
> >                        return ret;
> > +               }
> >        }
> > 	
> 
> Should we also take care to hide the device?
> As I read it the call to nvme_revalidate_zones can return a -ENODEV
> (through blk_revalidate_disk_zones) which is the original condition to
> hide the block device.

Yes, I think we need something like the patch below folded in.  This
unfreezes before setting the hidden flag and setting the ns ready,
but I can't see why we'd need it frozen for that.

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index bbdf0f95331f3..e6025f80744f6 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2012,7 +2012,7 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
 	if (blk_queue_is_zoned(ns->queue)) {
 		ret = nvme_revalidate_zones(ns);
 		if (ret && !nvme_first_scan(ns->disk))
-			return ret;
+			goto out_free_id;
 	}
 
 	if (nvme_ns_head_multipath(ns->head)) {
@@ -2029,6 +2029,8 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
 	return 0;
 
 out_unfreeze:
+	blk_mq_unfreeze_queue(ns->disk->queue);
+out_free_id:
 	/*
 	 * If probing fails due an unsupported feature, hide the block device,
 	 * but still allow other access.
@@ -2038,7 +2040,6 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
 		set_bit(NVME_NS_READY, &ns->flags);
 		ret = 0;
 	}
-	blk_mq_unfreeze_queue(ns->disk->queue);
 	kfree(id);
 	return ret;
 }

Re: [PATCH 3/5] nvme: refactor namespace probing

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 3606 bytes --]

On Tue, Jul 19, 2022 at 04:37:03PM +0200, Christoph Hellwig wrote:
> On Tue, Jul 19, 2022 at 03:00:48PM +0200, Joel Granados wrote:
> > > +++ b/drivers/nvme/host/core.c
> > > @@ -2011,8 +2011,10 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
> > > 
> > >        if (blk_queue_is_zoned(ns->queue)) {
> > >                ret = nvme_revalidate_zones(ns);
> > > -               if (ret && !nvme_first_scan(ns->disk))
> > > +               if (ret && !nvme_first_scan(ns->disk)) {
> > > +                       kfree(id);
> > >                        return ret;
> > > +               }
> > >        }
> > > 	
> > 
> > Should we also take care to hide the device?
> > As I read it the call to nvme_revalidate_zones can return a -ENODEV
> > (through blk_revalidate_disk_zones) which is the original condition to
> > hide the block device.
> 
> Yes, I think we need something like the patch below folded in.  This
> unfreezes before setting the hidden flag and setting the ns ready,
> but I can't see why we'd need it frozen for that.
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index bbdf0f95331f3..e6025f80744f6 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2012,7 +2012,7 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
>  	if (blk_queue_is_zoned(ns->queue)) {
>  		ret = nvme_revalidate_zones(ns);
>  		if (ret && !nvme_first_scan(ns->disk))
> -			return ret;
> +			goto out_free_id;
>  	}
>  
>  	if (nvme_ns_head_multipath(ns->head)) {
> @@ -2029,6 +2029,8 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
>  	return 0;
>  
>  out_unfreeze:
> +	blk_mq_unfreeze_queue(ns->disk->queue);
> +out_free_id:
>  	/*
>  	 * If probing fails due an unsupported feature, hide the block device,
>  	 * but still allow other access.
> @@ -2038,7 +2040,6 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
>  		set_bit(NVME_NS_READY, &ns->flags);
>  		ret = 0;
>  	}
> -	blk_mq_unfreeze_queue(ns->disk->queue);
>  	kfree(id);
>  	return ret;
>  }

Yes. And with unfreeze in its own goto, we can return with the
out_free_id on success. In this way we do not duplicate the 'kfree' line
and future uses of 'ret' will just work.
Something like this:

diff --git i/drivers/nvme/host/core.c w/drivers/nvme/host/core.c
index bbdf0f95331f..8359efbb1d29 100644
--- i/drivers/nvme/host/core.c
+++ w/drivers/nvme/host/core.c
@@ -2012,7 +2012,7 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
        if (blk_queue_is_zoned(ns->queue)) {
                ret = nvme_revalidate_zones(ns);
                if (ret && !nvme_first_scan(ns->disk))
-                       return ret;
+                       goto out_free_id;
        }
 
        if (nvme_ns_head_multipath(ns->head)) {
@@ -2025,10 +2025,11 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
                disk_update_readahead(ns->head->disk);
                blk_mq_unfreeze_queue(ns->head->disk->queue);
        }
-       kfree(id);
-       return 0;
+       goto out_free_id;
 
 out_unfreeze:
+       blk_mq_unfreeze_queue(ns->disk->queue);
+out_free_id:
        /*
         * If probing fails due an unsupported feature, hide the block device,
         * but still allow other access.
@@ -2038,7 +2039,6 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
                set_bit(NVME_NS_READY, &ns->flags);
                ret = 0;
        }
-       blk_mq_unfreeze_queue(ns->disk->queue);
        kfree(id);
        return ret;
 }


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 3/5] nvme: refactor namespace probing

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 16654 bytes --]

On Mon, Jul 18, 2022 at 07:25:01AM +0200, Christoph Hellwig wrote:
> Change nvme_ns_scan to gather all information needed for generic
> namespace setup into a nvme_ns_info structure.  This structure is filled
> from the Command Set Idependent Identify Namespace data structure if
> it is available or else the legacy Identify namespace structure.
> 
> With that everything related to the NVM command set (and the ZNS command
> set derived from it) can be encapsulated in the nvme_update_ns_info_block
> function while keeping the rest of the namespace probing generic.
> 
> The downside is that we now always issue two Identify Namespace calls for
> each probed namespace instead of usually just a single one previously.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Javier González <javier.gonz@samsung.com>
> ---
>  drivers/nvme/host/core.c | 231 +++++++++++++++++++++------------------
>  1 file changed, 126 insertions(+), 105 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 4769c49141913..659068f90165c 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -31,6 +31,14 @@
>  
>  #define NVME_MINORS		(1U << MINORBITS)
>  
> +struct nvme_ns_info {
> +	struct nvme_ns_ids ids;
> +	u32 nsid;
> +	__le32 anagrpid;
> +	bool is_shared;
> +	bool is_ready;
> +};
Nitpick after looking at this for the second time:
Should we use tabs instead of spaces to increase readability here?

> +
>  unsigned int admin_timeout = 60;
>  module_param(admin_timeout, uint, 0644);
>  MODULE_PARM_DESC(admin_timeout, "timeout in seconds for admin commands");
> @@ -1342,8 +1350,8 @@ static int nvme_process_ns_desc(struct nvme_ctrl *ctrl, struct nvme_ns_ids *ids,
>  	}
>  }
>  
> -static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl, unsigned nsid,
> -		struct nvme_ns_ids *ids)
> +static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl,
> +		struct nvme_ns_info *info)
>  {
>  	struct nvme_command c = { };
>  	bool csi_seen = false;
> @@ -1356,7 +1364,7 @@ static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl, unsigned nsid,
>  		return 0;
>  
>  	c.identify.opcode = nvme_admin_identify;
> -	c.identify.nsid = cpu_to_le32(nsid);
> +	c.identify.nsid = cpu_to_le32(info->nsid);
>  	c.identify.cns = NVME_ID_CNS_NS_DESC_LIST;
>  
>  	data = kzalloc(NVME_IDENTIFY_DATA_SIZE, GFP_KERNEL);
> @@ -1368,7 +1376,7 @@ static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl, unsigned nsid,
>  	if (status) {
>  		dev_warn(ctrl->device,
>  			"Identify Descriptors failed (nsid=%u, status=0x%x)\n",
> -			nsid, status);
> +			info->nsid, status);
>  		goto free_data;
>  	}
>  
> @@ -1378,7 +1386,7 @@ static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl, unsigned nsid,
>  		if (cur->nidl == 0)
>  			break;
>  
> -		len = nvme_process_ns_desc(ctrl, ids, cur, &csi_seen);
> +		len = nvme_process_ns_desc(ctrl, &info->ids, cur, &csi_seen);
>  		if (len < 0)
>  			break;
>  
> @@ -1387,7 +1395,7 @@ static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl, unsigned nsid,
>  
>  	if (nvme_multi_css(ctrl) && !csi_seen) {
>  		dev_warn(ctrl->device, "Command set not reported for nsid:%d\n",
> -			 nsid);
> +			 info->nsid);
>  		status = -EINVAL;
>  	}
>  
> @@ -1397,7 +1405,7 @@ static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl, unsigned nsid,
>  }
>  
>  static int nvme_identify_ns(struct nvme_ctrl *ctrl, unsigned nsid,
> -			struct nvme_ns_ids *ids, struct nvme_id_ns **id)
> +			struct nvme_id_ns **id)
>  {
>  	struct nvme_command c = { };
>  	int error;
> @@ -1420,51 +1428,64 @@ static int nvme_identify_ns(struct nvme_ctrl *ctrl, unsigned nsid,
>  	error = NVME_SC_INVALID_NS | NVME_SC_DNR;
>  	if ((*id)->ncap == 0) /* namespace not allocated or attached */
>  		goto out_free_id;
> +	return 0;
>  
> +out_free_id:
> +	kfree(*id);
> +	return error;
> +}
>  
> +static int nvme_ns_info_from_identify(struct nvme_ctrl *ctrl,
> +		struct nvme_ns_info *info)
> +{
> +	struct nvme_ns_ids *ids = &info->ids;
> +	struct nvme_id_ns *id;
> +	int ret;
> +
> +	ret = nvme_identify_ns(ctrl, info->nsid, &id);
> +	if (ret)
> +		return ret;
> +	info->anagrpid = id->anagrpid;
> +	info->is_shared = id->nmic & NVME_NS_NMIC_SHARED;
> +	info->is_ready = true;
>  	if (ctrl->quirks & NVME_QUIRK_BOGUS_NID) {
>  		dev_info(ctrl->device,
>  			 "Ignoring bogus Namespace Identifiers\n");
>  	} else {
>  		if (ctrl->vs >= NVME_VS(1, 1, 0) &&
>  		    !memchr_inv(ids->eui64, 0, sizeof(ids->eui64)))
> -			memcpy(ids->eui64, (*id)->eui64, sizeof(ids->eui64));
> +			memcpy(ids->eui64, id->eui64, sizeof(ids->eui64));
>  		if (ctrl->vs >= NVME_VS(1, 2, 0) &&
>  		    !memchr_inv(ids->nguid, 0, sizeof(ids->nguid)))
> -			memcpy(ids->nguid, (*id)->nguid, sizeof(ids->nguid));
> +			memcpy(ids->nguid, id->nguid, sizeof(ids->nguid));
>  	}
> -
> +	kfree(id);
>  	return 0;
> -
> -out_free_id:
> -	kfree(*id);
> -	return error;
>  }
>  
> -static int nvme_identify_ns_cs_indep(struct nvme_ctrl *ctrl, unsigned nsid,
> -			struct nvme_id_ns_cs_indep **id)
> +static int nvme_ns_info_from_id_cs_indep(struct nvme_ctrl *ctrl,
> +		struct nvme_ns_info *info)
>  {
> +	struct nvme_id_ns_cs_indep *id;
>  	struct nvme_command c = {
>  		.identify.opcode	= nvme_admin_identify,
> -		.identify.nsid		= cpu_to_le32(nsid),
> +		.identify.nsid		= cpu_to_le32(info->nsid),
>  		.identify.cns		= NVME_ID_CNS_NS_CS_INDEP,
>  	};
>  	int ret;
>  
> -	*id = kmalloc(sizeof(**id), GFP_KERNEL);
> -	if (!*id)
> +	id = kmalloc(sizeof(*id), GFP_KERNEL);
> +	if (!id)
>  		return -ENOMEM;
>  
> -	ret = nvme_submit_sync_cmd(ctrl->admin_q, &c, *id, sizeof(**id));
> -	if (ret) {
> -		dev_warn(ctrl->device,
> -			 "Identify namespace (CS independent) failed (%d)\n",
> -			 ret);
> -		kfree(*id);
> -		return ret;
> +	ret = nvme_submit_sync_cmd(ctrl->admin_q, &c, id, sizeof(*id));
> +	if (!ret) {
> +		info->anagrpid = id->anagrpid;
> +		info->is_shared = id->nmic & NVME_NS_NMIC_SHARED;
> +		info->is_ready = id->nstat & NVME_NSTAT_NRDY;
>  	}
> -
> -	return 0;
> +	kfree(id);
> +	return ret;
>  }
>  
>  static int nvme_features(struct nvme_ctrl *dev, u8 op, unsigned int fid,
> @@ -1925,12 +1946,19 @@ static void nvme_set_chunk_sectors(struct nvme_ns *ns, struct nvme_id_ns *id)
>  	blk_queue_chunk_sectors(ns->queue, iob);
>  }
>  
> -static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id)
> +static int nvme_update_ns_info_block(struct nvme_ns *ns,
> +		struct nvme_ns_info *info)
>  {
> -	unsigned lbaf = nvme_lbaf_index(id->flbas);
> +	struct nvme_id_ns *id;
> +	unsigned lbaf;
Checkpatch.pl has a warning here for use of 'unsigned int' instead of
just 'unsigned'

>  	int ret;
>  
> +	ret = nvme_identify_ns(ns->ctrl, info->nsid, &id);
> +	if (ret)
> +		return ret;
> +
>  	blk_mq_freeze_queue(ns->disk->queue);
> +	lbaf = nvme_lbaf_index(id->flbas);
>  	ns->lba_shift = id->lbaf[lbaf].ds;
>  	nvme_set_queue_limits(ns->ctrl, ns->queue);
>  
> @@ -1967,6 +1995,7 @@ static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id)
>  		disk_update_readahead(ns->head->disk);
>  		blk_mq_unfreeze_queue(ns->head->disk->queue);
>  	}
> +	kfree(id);
>  	return 0;
>  
>  out_unfreeze:
> @@ -1980,9 +2009,30 @@ static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id)
>  		ret = 0;
>  	}
>  	blk_mq_unfreeze_queue(ns->disk->queue);
> +	kfree(id);
>  	return ret;
>  }
>  
> +static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_ns_info *info)
> +{
> +	switch (info->ids.csi) {
> +	case NVME_CSI_ZNS:
> +		if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED)) {
> +			dev_warn(ns->ctrl->device,
> +	"nsid %u not supported without CONFIG_BLK_DEV_ZONED\n",
> +				info->nsid);
> +			return -ENODEV;
> +		}
> +		return nvme_update_ns_info_block(ns, info);
> +	case NVME_CSI_NVM:
> +		return nvme_update_ns_info_block(ns, info);
> +	default:
> +		dev_warn(ns->ctrl->device, "unknown csi %u for nsid %u\n",
> +			info->ids.csi, info->nsid);
> +		return -ENODEV;
> +	}
> +}
> +
>  static char nvme_pr_type(enum pr_type type)
>  {
>  	switch (type) {
> @@ -3911,7 +3961,7 @@ static int nvme_add_ns_cdev(struct nvme_ns *ns)
>  }
>  
>  static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
> -		unsigned nsid, struct nvme_ns_ids *ids)
> +		struct nvme_ns_info *info)
>  {
>  	struct nvme_ns_head *head;
>  	size_t size = sizeof(*head);
> @@ -3933,8 +3983,9 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
>  	if (ret)
>  		goto out_ida_remove;
>  	head->subsys = ctrl->subsys;
> -	head->ns_id = nsid;
> -	head->ids = *ids;
> +	head->ns_id = info->nsid;
> +	head->ids = info->ids;
> +	head->shared = info->is_shared;
>  	kref_init(&head->ref);
>  
>  	if (head->ids.csi) {
> @@ -3991,55 +4042,54 @@ static int nvme_global_check_duplicate_ids(struct nvme_subsystem *this,
>  	return ret;
>  }
>  
> -static int nvme_init_ns_head(struct nvme_ns *ns, unsigned nsid,
> -		struct nvme_ns_ids *ids, bool is_shared)
> +static int nvme_init_ns_head(struct nvme_ns *ns, struct nvme_ns_info *info)
>  {
>  	struct nvme_ctrl *ctrl = ns->ctrl;
>  	struct nvme_ns_head *head = NULL;
>  	int ret;
>  
> -	ret = nvme_global_check_duplicate_ids(ctrl->subsys, ids);
> +	ret = nvme_global_check_duplicate_ids(ctrl->subsys, &info->ids);
>  	if (ret) {
>  		dev_err(ctrl->device,
> -			"globally duplicate IDs for nsid %d\n", nsid);
> +			"globally duplicate IDs for nsid %d\n", info->nsid);
>  		nvme_print_device_info(ctrl);
>  		return ret;
>  	}
>  
>  	mutex_lock(&ctrl->subsys->lock);
> -	head = nvme_find_ns_head(ctrl, nsid);
> +	head = nvme_find_ns_head(ctrl, info->nsid);
>  	if (!head) {
> -		ret = nvme_subsys_check_duplicate_ids(ctrl->subsys, ids);
> +		ret = nvme_subsys_check_duplicate_ids(ctrl->subsys, &info->ids);
>  		if (ret) {
>  			dev_err(ctrl->device,
>  				"duplicate IDs in subsystem for nsid %d\n",
> -				nsid);
> +				info->nsid);
>  			goto out_unlock;
>  		}
> -		head = nvme_alloc_ns_head(ctrl, nsid, ids);
> +		head = nvme_alloc_ns_head(ctrl, info);
>  		if (IS_ERR(head)) {
>  			ret = PTR_ERR(head);
>  			goto out_unlock;
>  		}
> -		head->shared = is_shared;
>  	} else {
>  		ret = -EINVAL;
> -		if (!is_shared || !head->shared) {
> +		if (!info->is_shared || !head->shared) {
>  			dev_err(ctrl->device,
> -				"Duplicate unshared namespace %d\n", nsid);
> +				"Duplicate unshared namespace %d\n",
> +				info->nsid);
>  			goto out_put_ns_head;
>  		}
> -		if (!nvme_ns_ids_equal(&head->ids, ids)) {
> +		if (!nvme_ns_ids_equal(&head->ids, &info->ids)) {
>  			dev_err(ctrl->device,
>  				"IDs don't match for shared namespace %d\n",
> -					nsid);
> +					info->nsid);
>  			goto out_put_ns_head;
>  		}
>  
>  		if (!multipath && !list_empty(&head->list)) {
>  			dev_warn(ctrl->device,
>  				"Found shared namespace %d, but multipathing not supported.\n",
> -				nsid);
> +				info->nsid);
>  			dev_warn_once(ctrl->device,
>  				"Support for shared namespaces without CONFIG_NVME_MULTIPATH is deprecated and will be removed in Linux 6.0\n.");
>  		}
> @@ -4093,20 +4143,15 @@ static void nvme_ns_add_to_ctrl_list(struct nvme_ns *ns)
>  	list_add(&ns->list, &ns->ctrl->namespaces);
>  }
>  
> -static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
> -		struct nvme_ns_ids *ids)
> +static void nvme_alloc_ns(struct nvme_ctrl *ctrl, struct nvme_ns_info *info)
>  {
>  	struct nvme_ns *ns;
>  	struct gendisk *disk;
> -	struct nvme_id_ns *id;
>  	int node = ctrl->numa_node;
>  
> -	if (nvme_identify_ns(ctrl, nsid, ids, &id))
> -		return;
> -
>  	ns = kzalloc_node(sizeof(*ns), GFP_KERNEL, node);
>  	if (!ns)
> -		goto out_free_id;
> +		return;
>  
>  	disk = blk_mq_alloc_disk(ctrl->tagset, ns);
>  	if (IS_ERR(disk))
> @@ -4127,7 +4172,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, info))
>  		goto out_cleanup_disk;
>  
>  	/*
> @@ -4153,7 +4198,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
>  			ns->head->instance);
>  	}
>  
> -	if (nvme_update_ns_info(ns, id))
> +	if (nvme_update_ns_info(ns, info))
>  		goto out_unlink_ns;
>  
>  	down_write(&ctrl->namespaces_rwsem);
> @@ -4167,9 +4212,8 @@ 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, info->anagrpid);
>  	nvme_fault_inject_init(&ns->fault_inject, ns->disk->disk_name);
> -	kfree(id);
>  
>  	return;
>  
> @@ -4189,8 +4233,6 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
>  	blk_cleanup_disk(disk);
>   out_free_ns:
>  	kfree(ns);
> - out_free_id:
> -	kfree(id);
>  }
>  
>  static void nvme_ns_remove(struct nvme_ns *ns)
> @@ -4249,29 +4291,21 @@ 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_info *info)
>  {
> -	struct nvme_id_ns *id;
>  	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)) {
> +	if (!nvme_ns_ids_equal(&ns->head->ids, &info->ids)) {
>  		dev_err(ns->ctrl->device,
>  			"identifiers changed for nsid %d\n", ns->head->ns_id);
> -		goto out_free_id;
> +		goto out;
>  	}
>  
> -	ret = nvme_update_ns_info(ns, id);
> -
> -out_free_id:
> -	kfree(id);
> +	ret = nvme_update_ns_info(ns, info);
>  out:
>  	/*
>  	 * Only remove the namespace if we got a fatal error back from the
> @@ -4285,57 +4319,44 @@ static void nvme_validate_ns(struct nvme_ns *ns, struct nvme_ns_ids *ids)
>  
>  static void nvme_scan_ns(struct nvme_ctrl *ctrl, unsigned nsid)
>  {
> -	struct nvme_ns_ids ids = { };
> -	struct nvme_id_ns_cs_indep *id;
> +	struct nvme_ns_info info = { .nsid = nsid };
>  	struct nvme_ns *ns;
> -	bool ready = true;
>  
> -	if (nvme_identify_ns_descs(ctrl, nsid, &ids))
> +	if (nvme_identify_ns_descs(ctrl, &info))
>  		return;
>  
> -	if (ids.csi != NVME_CSI_NVM && !nvme_multi_css(ctrl)) {
> +	if (info.ids.csi != NVME_CSI_NVM && !nvme_multi_css(ctrl)) {
>  		dev_warn(ctrl->device,
>  			"command set not reported for nsid: %d\n", nsid);
>  		return;
>  	}
>  
>  	/*
> -	 * Check if the namespace is ready.  If not ignore it, we will get an
> -	 * AEN once it becomes ready and restart the scan.
> +	 * If available try to use the Command Set Idependent Identify Namespace
> +	 * data structure to find all the generic information that is needed to
> +	 * set up a namespace.  If not fall back to the legacy version.
>  	 */
> -	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) {
> +		if (nvme_ns_info_from_id_cs_indep(ctrl, &info))
> +			return;
> +	} else {
> +		if (nvme_ns_info_from_identify(ctrl, &info))
> +			return;
>  	}
>  
> -	if (!ready)
> +	/*
> +	 * Ignore the namespace if it is not ready. We will get an AEN once it
> +	 * becomes ready and restart the scan.
> +	 */
> +	if (!info.is_ready)
>  		return;
>  
>  	ns = nvme_find_get_ns(ctrl, nsid);
>  	if (ns) {
> -		nvme_validate_ns(ns, &ids);
> +		nvme_validate_ns(ns, &info);
>  		nvme_put_ns(ns);
> -		return;
> -	}
> -
> -	switch (ids.csi) {
> -	case NVME_CSI_NVM:
> -		nvme_alloc_ns(ctrl, nsid, &ids);
> -		break;
> -	case NVME_CSI_ZNS:
> -		if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED)) {
> -			dev_warn(ctrl->device,
> -				"nsid %u not supported without CONFIG_BLK_DEV_ZONED\n",
> -				nsid);
> -			break;
> -		}
> -		nvme_alloc_ns(ctrl, nsid, &ids);
> -		break;
> -	default:
> -		dev_warn(ctrl->device, "unknown csi %u for nsid %u\n",
> -			ids.csi, nsid);
> -		break;
> +	} else {
> +		nvme_alloc_ns(ctrl, &info);
>  	}
>  }
>  
> -- 
> 2.30.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 3/5] nvme: refactor namespace probing

[Christoph Hellwig]

On Wed, Jul 20, 2022 at 10:19:12AM +0200, Joel Granados wrote:
> > +struct nvme_ns_info {
> > +	struct nvme_ns_ids ids;
> > +	u32 nsid;
> > +	__le32 anagrpid;
> > +	bool is_shared;
> > +	bool is_ready;
> > +};
> Nitpick after looking at this for the second time:
> Should we use tabs instead of spaces to increase readability here?

Not sure why that would help.

> > -	unsigned lbaf = nvme_lbaf_index(id->flbas);
> > +	struct nvme_id_ns *id;
> > +	unsigned lbaf;
> Checkpatch.pl has a warning here for use of 'unsigned int' instead of
> just 'unsigned'

checkpatch is, as so often, rather misguided here.

Re: [PATCH 3/5] nvme: refactor namespace probing

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 859 bytes --]

On Wed, Jul 20, 2022 at 11:02:34AM +0200, Christoph Hellwig wrote:
> On Wed, Jul 20, 2022 at 10:19:12AM +0200, Joel Granados wrote:
> > > +struct nvme_ns_info {
> > > +	struct nvme_ns_ids ids;
> > > +	u32 nsid;
> > > +	__le32 anagrpid;
> > > +	bool is_shared;
> > > +	bool is_ready;
> > > +};
> > Nitpick after looking at this for the second time:
> > Should we use tabs instead of spaces to increase readability here?
> 
> Not sure why that would help.
Was unsure what the convention was, that is why I asked.

> 
> > > -	unsigned lbaf = nvme_lbaf_index(id->flbas);
> > > +	struct nvme_id_ns *id;
> > > +	unsigned lbaf;
> > Checkpatch.pl has a warning here for use of 'unsigned int' instead of
> > just 'unsigned'
> 
> checkpatch is, as so often, rather misguided here.
Don't kill the messenger ;). Should we just remove the warning?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]