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

[PATCH v3 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 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      | 132 ++++++++++++++++++++++++----------
 drivers/nvme/host/multipath.c |   6 +-
 drivers/nvme/host/nvme.h      |   5 +-
 3 files changed, 98 insertions(+), 45 deletions(-)

-- 
2.30.2

[PATCH v3 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 v3 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 | 132 +++++++++++++++++++++++++++------------
 1 file changed, 93 insertions(+), 39 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 0fd6127da8d1..03ba3d0f4d3a 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1908,6 +1908,34 @@ 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_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);
+		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);
+		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);
@@ -3951,15 +3979,29 @@ 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:
+		nmic = &indep_id->nmic;
+		anagrpid = &indep_id->anagrpid;
+		break;
+	}
 
 	ns = kzalloc_node(sizeof(*ns), GFP_KERNEL, node);
 	if (!ns)
@@ -3984,7 +4026,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 +4052,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 +4075,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 +4156,32 @@ 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:
+		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 +4196,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 *id_indep;
 	struct nvme_ns *ns;
-	bool ready = true;
 
 	if (nvme_identify_ns_descs(ctrl, nsid, &ids))
 		return;
 
+	if (nvme_identify_ns_cs_indep(ctrl, nsid, &id_indep))
+		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 ((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 (!(id_indep->nstat & NVME_NSTAT_NRDY))
+			goto free_id_indep;
 	}
 
-	if (!ready)
-		return;
-
 	ns = nvme_find_get_ns(ctrl, nsid);
 	if (ns) {
-		nvme_validate_ns(ns, &ids);
+		nvme_validate_ns(ns, &ids, id_indep);
 		nvme_put_ns(ns);
-		return;
+		goto free_id_indep;
 	}
 
 	switch (ids.csi) {
 	case NVME_CSI_NVM:
-		nvme_alloc_ns(ctrl, nsid, &ids);
+		nvme_alloc_ns(ctrl, nsid, &ids, id_indep);
 		break;
 	case NVME_CSI_ZNS:
 		if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED)) {
@@ -4187,13 +4238,16 @@ 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, id_indep);
 		break;
 	default:
-		dev_warn(ctrl->device, "unknown csi %u for nsid %u\n",
-			ids.csi, nsid);
+		/* required to enable char-interface for unknown command sets*/
+		nvme_alloc_ns(ctrl, nsid, &ids, id_indep);
 		break;
 	}
+
+free_id_indep:
+	kfree(id_indep);
 }
 
 static void nvme_remove_invalid_namespaces(struct nvme_ctrl *ctrl,
-- 
2.30.2

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

[Keith Busch]

On Wed, Jun 22, 2022 at 03:55:56PM +0200, Joel Granados wrote:
>  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 *id_indep;
>  	struct nvme_ns *ns;
> -	bool ready = true;
>  
>  	if (nvme_identify_ns_descs(ctrl, nsid, &ids))
>  		return;
>  
> +	if (nvme_identify_ns_cs_indep(ctrl, nsid, &id_indep))
> +		return;
> +

There's no check that the command-set-independent namespace identification is
supported, so an error should be expected for many controllers instead of
abandoning the namespace.

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

[Joel Granados]

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

On Wed, Jun 22, 2022 at 02:20:05PM -0600, Keith Busch wrote:
> On Wed, Jun 22, 2022 at 03:55:56PM +0200, Joel Granados wrote:
> >  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 *id_indep;
> >  	struct nvme_ns *ns;
> > -	bool ready = true;
> >  
> >  	if (nvme_identify_ns_descs(ctrl, nsid, &ids))
> >  		return;
> >  
> > +	if (nvme_identify_ns_cs_indep(ctrl, nsid, &id_indep))
> > +		return;
> > +
> 
> There's no check that the command-set-independent namespace identification is
> supported, so an error should be expected for many controllers instead of
> abandoning the namespace.
good catch. But is the error really necessary?
If the controller supports NVM or ZNS then we can continue with the
normal (command-set-dependent) namespace identification.

I see two situations:

1. When we are dealing with NVM or ZNS command sets:
1.1 Should bring up a block device regardless of supporting command-set-
    independent namespace identification
1.2 Time-to-ready will be used only if command-set-independent is
    supported

2. When we are dealing with something that is not NVM nor ZNS:
2.1 Should bring up a char device iff the command-set-independent
    namespace identification is supported.
2.2 Should abandon the namespace with a dev_warn if the
    command-set-independent namespace identification is not supported.

Am I missing something?

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

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

[Keith Busch]

On Thu, Jun 23, 2022 at 10:39:18AM +0200, Joel Granados wrote:
> On Wed, Jun 22, 2022 at 02:20:05PM -0600, Keith Busch wrote:
> > On Wed, Jun 22, 2022 at 03:55:56PM +0200, Joel Granados wrote:
> > >  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 *id_indep;
> > >  	struct nvme_ns *ns;
> > > -	bool ready = true;
> > >  
> > >  	if (nvme_identify_ns_descs(ctrl, nsid, &ids))
> > >  		return;
> > >  
> > > +	if (nvme_identify_ns_cs_indep(ctrl, nsid, &id_indep))
> > > +		return;
> > > +
> > 
> > There's no check that the command-set-independent namespace identification is
> > supported, so an error should be expected for many controllers instead of
> > abandoning the namespace.
> good catch. But is the error really necessary?
> If the controller supports NVM or ZNS then we can continue with the
> normal (command-set-dependent) namespace identification.
> 
> I see two situations:
> 
> 1. When we are dealing with NVM or ZNS command sets:
> 1.1 Should bring up a block device regardless of supporting command-set-
>     independent namespace identification
> 1.2 Time-to-ready will be used only if command-set-independent is
>     supported
> 
> 2. When we are dealing with something that is not NVM nor ZNS:
> 2.1 Should bring up a char device iff the command-set-independent
>     namespace identification is supported.
> 2.2 Should abandon the namespace with a dev_warn if the
>     command-set-independent namespace identification is not supported.
> 
> Am I missing something?

I think your description sounds like what we want to happen, but that's not
what the code does. When validating NVM or ZNS namespace, but the controller
doesn't support CNS 8h, the code returns early instead of allocating the
namespace.

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

[Joel Granados]

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

On Thu, Jun 23, 2022 at 08:57:29AM -0600, Keith Busch wrote:
> On Thu, Jun 23, 2022 at 10:39:18AM +0200, Joel Granados wrote:
> > On Wed, Jun 22, 2022 at 02:20:05PM -0600, Keith Busch wrote:
> > > On Wed, Jun 22, 2022 at 03:55:56PM +0200, Joel Granados wrote:
> > > >  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 *id_indep;
> > > >  	struct nvme_ns *ns;
> > > > -	bool ready = true;
> > > >  
> > > >  	if (nvme_identify_ns_descs(ctrl, nsid, &ids))
> > > >  		return;
> > > >  
> > > > +	if (nvme_identify_ns_cs_indep(ctrl, nsid, &id_indep))
> > > > +		return;
> > > > +
> > > 
> > > There's no check that the command-set-independent namespace identification is
> > > supported, so an error should be expected for many controllers instead of
> > > abandoning the namespace.
> > good catch. But is the error really necessary?
> > If the controller supports NVM or ZNS then we can continue with the
> > normal (command-set-dependent) namespace identification.
> > 
> > I see two situations:
> > 
> > 1. When we are dealing with NVM or ZNS command sets:
> > 1.1 Should bring up a block device regardless of supporting command-set-
> >     independent namespace identification
> > 1.2 Time-to-ready will be used only if command-set-independent is
> >     supported
> > 
> > 2. When we are dealing with something that is not NVM nor ZNS:
> > 2.1 Should bring up a char device iff the command-set-independent
> >     namespace identification is supported.
> > 2.2 Should abandon the namespace with a dev_warn if the
> >     command-set-independent namespace identification is not supported.
> > 
> > Am I missing something?
> 
> I think your description sounds like what we want to happen, but that's not
> what the code does. When validating NVM or ZNS namespace, but the controller
> doesn't support CNS 8h, the code returns early instead of allocating the
> namespace.

Will update in V4 so it reflects required behavior. Just wanted to make
sure that the error was not necessary.

Thx


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

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

[Sagi Grimberg]

On 6/22/22 16:55, Joel Granados wrote:
> 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 | 132 +++++++++++++++++++++++++++------------
>   1 file changed, 93 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 0fd6127da8d1..03ba3d0f4d3a 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1908,6 +1908,34 @@ 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_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);
> +		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);
> +		ns->head->disk->flags |= GENHD_FL_HIDDEN;
> +		blk_mq_unfreeze_queue(ns->head->disk->queue);
> +	}

We can probably share this code with the existing update_ns_info ?

> +
> +	/* 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);
> @@ -3951,15 +3979,29 @@ 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:
> +		nmic = &indep_id->nmic;
> +		anagrpid = &indep_id->anagrpid;
> +		break;
> +	}
>   
>   	ns = kzalloc_node(sizeof(*ns), GFP_KERNEL, node);
>   	if (!ns)
> @@ -3984,7 +4026,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 +4052,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 +4075,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 +4156,32 @@ 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:
> +		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 +4196,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 *id_indep;
>   	struct nvme_ns *ns;
> -	bool ready = true;
>   
>   	if (nvme_identify_ns_descs(ctrl, nsid, &ids))
>   		return;
>   
> +	if (nvme_identify_ns_cs_indep(ctrl, nsid, &id_indep))
> +		return;
> +

Is it ok to issue this without checking that the namespace is ready?

>   	/*
>   	 * 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) {
> +		if (!(id_indep->nstat & NVME_NSTAT_NRDY))
> +			goto free_id_indep;
>   	}
>   
> -	if (!ready)
> -		return;
> -
>   	ns = nvme_find_get_ns(ctrl, nsid);
>   	if (ns) {
> -		nvme_validate_ns(ns, &ids);
> +		nvme_validate_ns(ns, &ids, id_indep);
>   		nvme_put_ns(ns);
> -		return;
> +		goto free_id_indep;
>   	}
>   
>   	switch (ids.csi) {
>   	case NVME_CSI_NVM:
> -		nvme_alloc_ns(ctrl, nsid, &ids);
> +		nvme_alloc_ns(ctrl, nsid, &ids, id_indep);
>   		break;
>   	case NVME_CSI_ZNS:
>   		if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED)) {
> @@ -4187,13 +4238,16 @@ 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, id_indep);
>   		break;
>   	default:
> -		dev_warn(ctrl->device, "unknown csi %u for nsid %u\n",
> -			ids.csi, nsid);
> +		/* required to enable char-interface for unknown command sets*/
> +		nvme_alloc_ns(ctrl, nsid, &ids, id_indep);
>   		break;
>   	}
> +
> +free_id_indep:
> +	kfree(id_indep);
>   }
>   
>   static void nvme_remove_invalid_namespaces(struct nvme_ctrl *ctrl,

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

[Joel Granados]

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

On Sun, Jun 26, 2022 at 04:53:32PM +0300, Sagi Grimberg wrote:
> 
> 
> On 6/22/22 16:55, Joel Granados wrote:
> > 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 | 132 +++++++++++++++++++++++++++------------
> >   1 file changed, 93 insertions(+), 39 deletions(-)
> > 
> > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > index 0fd6127da8d1..03ba3d0f4d3a 100644
> > --- a/drivers/nvme/host/core.c
> > +++ b/drivers/nvme/host/core.c
> > @@ -1908,6 +1908,34 @@ 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_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);
> > +		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);
> > +		ns->head->disk->flags |= GENHD_FL_HIDDEN;
> > +		blk_mq_unfreeze_queue(ns->head->disk->queue);
> > +	}
> 
> We can probably share this code with the existing update_ns_info ?

There are three differences in the multipath section of these two functions:
1. The command-set-indep does not call nvme_update_disk_info
2. The command-set-indep does not call disk_update_readahead
3. The command-set-indep hides the device "GENHD_FL_HIDDEN" at the end

I'll add a function that gathers the three common lines.
Something like this:

static inline void nvme_update_ns_mpath_info_general(struct nvme_ns *ns, __u8 nsattr)
{
  set_disk_ro(....)
  nvme_mpath_revalidate_paths(...)
  blk_stack_limits(...)
}

> 
> > +
> > +	/* 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);
> > @@ -3951,15 +3979,29 @@ 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:
> > +		nmic = &indep_id->nmic;
> > +		anagrpid = &indep_id->anagrpid;
> > +		break;
> > +	}
> >   	ns = kzalloc_node(sizeof(*ns), GFP_KERNEL, node);
> >   	if (!ns)
> > @@ -3984,7 +4026,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 +4052,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 +4075,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 +4156,32 @@ 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:
> > +		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 +4196,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 *id_indep;
> >   	struct nvme_ns *ns;
> > -	bool ready = true;
> >   	if (nvme_identify_ns_descs(ctrl, nsid, &ids))
> >   		return;
> > +	if (nvme_identify_ns_cs_indep(ctrl, nsid, &id_indep))
> > +		return;
> > +
> 
> Is it ok to issue this without checking that the namespace is ready?

This is a great question. Gave TP4084 a read and this is what I
understand (please correct me if I'm wrong):
* For the first controller ready mode (Controller ready with media), once
  the controller is ready the namespaces are required to process admin
  commands. Which basically means commands are not allowed to be aborted
  with "namespace not ready" status code. In this case it is OK to issue
  without checking NS ready.
* For the second controller ready mode (Controller ready independent of
  media), the identify command is not allowed to abort with
  "namespace not ready" status code  In this case it is OK to issue
  without checking NS ready.
* In the case that the command set indep identify NS is NOT supported we
  ignore the returned error and continue with validation in the
  expectation that the controller just forgot to implement the indep
  identify (08h) but actually supports the normal identify namespace
  command (00h).

All this is contained in my V4 which I'll post as soon as I have tested

Thx for the review

> 
> >   	/*
> >   	 * 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) {
> > +		if (!(id_indep->nstat & NVME_NSTAT_NRDY))
> > +			goto free_id_indep;
> >   	}
> > -	if (!ready)
> > -		return;
> > -
> >   	ns = nvme_find_get_ns(ctrl, nsid);
> >   	if (ns) {
> > -		nvme_validate_ns(ns, &ids);
> > +		nvme_validate_ns(ns, &ids, id_indep);
> >   		nvme_put_ns(ns);
> > -		return;
> > +		goto free_id_indep;
> >   	}
> >   	switch (ids.csi) {
> >   	case NVME_CSI_NVM:
> > -		nvme_alloc_ns(ctrl, nsid, &ids);
> > +		nvme_alloc_ns(ctrl, nsid, &ids, id_indep);
> >   		break;
> >   	case NVME_CSI_ZNS:
> >   		if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED)) {
> > @@ -4187,13 +4238,16 @@ 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, id_indep);
> >   		break;
> >   	default:
> > -		dev_warn(ctrl->device, "unknown csi %u for nsid %u\n",
> > -			ids.csi, nsid);
> > +		/* required to enable char-interface for unknown command sets*/
> > +		nvme_alloc_ns(ctrl, nsid, &ids, id_indep);
> >   		break;
> >   	}
> > +
> > +free_id_indep:
> > +	kfree(id_indep);
> >   }
> >   static void nvme_remove_invalid_namespaces(struct nvme_ctrl *ctrl,

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