public inbox for linux-nvme@lists.infradead.org
 help / color / mirror / Atom feed
* [PATCH v2 0/2] nvme: Enable generic interface (/dev/ngX) for unknown command sets
       [not found] <CGME20220607134309eucas1p1724cee80a5bc116c652e91422d474ccd@eucas1p1.samsung.com>
@ 2022-06-07 13:40 ` Joel Granados
  2022-06-07 13:40   ` [PATCH v2 1/2] nvme-multipath: refactor nvme_mpath_add_disk Joel Granados
  2022-06-07 13:40   ` [PATCH v2 2/2] nvme: enable generic interface (/dev/ngXnY) for unknown command sets Joel Granados
  0 siblings, 2 replies; 9+ messages in thread
From: Joel Granados @ 2022-06-07 13:40 UTC (permalink / raw)
  To: hch, kbusch; +Cc: joshi.k, linux-nvme, gost.dev, k.jensen

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 boolean 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 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      | 83 ++++++++++++++++++++++++++++++-----
 drivers/nvme/host/multipath.c |  7 +--
 drivers/nvme/host/nvme.h      |  5 +--
 3 files changed, 78 insertions(+), 17 deletions(-)

-- 
2.30.2



^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 1/2] nvme-multipath: refactor nvme_mpath_add_disk
  2022-06-07 13:40 ` [PATCH v2 0/2] nvme: Enable generic interface (/dev/ngX) for unknown command sets Joel Granados
@ 2022-06-07 13:40   ` Joel Granados
  2022-06-13 18:16     ` Christoph Hellwig
  2022-06-07 13:40   ` [PATCH v2 2/2] nvme: enable generic interface (/dev/ngXnY) for unknown command sets Joel Granados
  1 sibling, 1 reply; 9+ messages in thread
From: Joel Granados @ 2022-06-07 13:40 UTC (permalink / raw)
  To: hch, kbusch; +Cc: joshi.k, linux-nvme, gost.dev, k.jensen

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 | 7 ++++---
 drivers/nvme/host/nvme.h      | 5 ++---
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 24165daee3c8..06b9718b0540 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4027,7 +4027,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..6ae7c042b1dc 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -3,6 +3,7 @@
  * Copyright (c) 2017-2018 Christoph Hellwig.
  */
 
+#include "linux/types.h"
 #include <linux/backing-dev.h>
 #include <linux/moduleparam.h>
 #include <linux/vmalloc.h>
@@ -800,16 +801,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 9b72b6ecf33c..14b097dfa7fa 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -808,7 +808,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);
@@ -850,8 +850,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



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 2/2] nvme: enable generic interface (/dev/ngXnY) for unknown command sets
  2022-06-07 13:40 ` [PATCH v2 0/2] nvme: Enable generic interface (/dev/ngX) for unknown command sets Joel Granados
  2022-06-07 13:40   ` [PATCH v2 1/2] nvme-multipath: refactor nvme_mpath_add_disk Joel Granados
@ 2022-06-07 13:40   ` Joel Granados
  2022-06-13 18:24     ` Christoph Hellwig
  1 sibling, 1 reply; 9+ messages in thread
From: Joel Granados @ 2022-06-07 13:40 UTC (permalink / raw)
  To: hch, kbusch; +Cc: joshi.k, linux-nvme, gost.dev, k.jensen

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 | 83 ++++++++++++++++++++++++++++++++++------
 1 file changed, 72 insertions(+), 11 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 06b9718b0540..c7eb92480fad 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1912,6 +1912,33 @@ 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);
+		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);
@@ -3953,15 +3980,27 @@ static void nvme_ns_add_to_ctrl_list(struct nvme_ns *ns)
 	list_add(&ns->list, &ns->ctrl->namespaces);
 }
 
+static inline bool nvme_ns_indep(struct nvme_ns_ids *ids)
+{
+	return ids->csi != NVME_CSI_NVM && ids->csi != NVME_CSI_ZNS;
+}
+
 static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
 		struct nvme_ns_ids *ids)
 {
+	int ret = 0;
 	struct nvme_ns *ns;
 	struct gendisk *disk;
 	struct nvme_id_ns *id;
+	struct nvme_id_ns_cs_indep *indep_id;
 	int node = ctrl->numa_node;
+	bool cs_indep = nvme_ns_indep(ids);
 
-	if (nvme_identify_ns(ctrl, nsid, ids, &id))
+	if (cs_indep)
+		ret = nvme_identify_ns_cs_indep(ctrl, nsid, &indep_id);
+	else
+		ret = nvme_identify_ns(ctrl, nsid, ids, &id);
+	if (ret)
 		return;
 
 	ns = kzalloc_node(sizeof(*ns), GFP_KERNEL, node);
@@ -3987,7 +4026,8 @@ 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,
+			      (cs_indep ? indep_id->nmic : id->nmic) & NVME_NS_NMIC_SHARED))
 		goto out_cleanup_disk;
 
 	/*
@@ -4013,7 +4053,11 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
 			ns->head->instance);
 	}
 
-	if (nvme_update_ns_info(ns, id))
+	if (cs_indep)
+		ret = nvme_update_ns_info_cs_indep(ns, indep_id);
+	else
+		ret = nvme_update_ns_info(ns, id);
+	if (ret)
 		goto out_unlink_ns;
 
 	down_write(&ctrl->namespaces_rwsem);
@@ -4027,9 +4071,12 @@ 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, cs_indep ? indep_id->anagrpid : id->anagrpid);
 	nvme_fault_inject_init(&ns->fault_inject, ns->disk->disk_name);
-	kfree(id);
+	if (cs_indep)
+		kfree(indep_id);
+	else
+		kfree(id);
 
 	return;
 
@@ -4050,7 +4097,10 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
  out_free_ns:
 	kfree(ns);
  out_free_id:
-	kfree(id);
+	if (cs_indep)
+		kfree(indep_id);
+	else
+		kfree(id);
 }
 
 static void nvme_ns_remove(struct nvme_ns *ns)
@@ -4112,12 +4162,17 @@ 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)
 {
 	struct nvme_id_ns *id;
+	struct nvme_id_ns_cs_indep *indep_id;
 	int ret = NVME_SC_INVALID_NS | NVME_SC_DNR;
+	bool cs_indep = nvme_ns_indep(ids);
 
 	if (test_bit(NVME_NS_DEAD, &ns->flags))
 		goto out;
 
-	ret = nvme_identify_ns(ns->ctrl, ns->head->ns_id, ids, &id);
+	if (cs_indep)
+		ret = nvme_identify_ns_cs_indep(ns->ctrl, ns->head->ns_id, &indep_id);
+	else
+		ret = nvme_identify_ns(ns->ctrl, ns->head->ns_id, ids, &id);
 	if (ret)
 		goto out;
 
@@ -4128,10 +4183,16 @@ static void nvme_validate_ns(struct nvme_ns *ns, struct nvme_ns_ids *ids)
 		goto out_free_id;
 	}
 
-	ret = nvme_update_ns_info(ns, id);
+	if (cs_indep)
+		ret = nvme_update_ns_info_cs_indep(ns, indep_id);
+	else
+		ret = nvme_update_ns_info(ns, id);
 
 out_free_id:
-	kfree(id);
+	if (cs_indep)
+		kfree(indep_id);
+	else
+		kfree(id);
 out:
 	/*
 	 * Only remove the namespace if we got a fatal error back from the
@@ -4193,8 +4254,8 @@ static void nvme_validate_or_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 		nvme_alloc_ns(ctrl, nsid, &ids);
 		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);
 		break;
 	}
 }
-- 
2.30.2



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/2] nvme-multipath: refactor nvme_mpath_add_disk
  2022-06-07 13:40   ` [PATCH v2 1/2] nvme-multipath: refactor nvme_mpath_add_disk Joel Granados
@ 2022-06-13 18:16     ` Christoph Hellwig
  2022-06-15  7:50       ` Joel Granados
  0 siblings, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2022-06-13 18:16 UTC (permalink / raw)
  To: Joel Granados; +Cc: hch, kbusch, joshi.k, linux-nvme, gost.dev, k.jensen

> +++ b/drivers/nvme/host/multipath.c
> @@ -3,6 +3,7 @@
>   * Copyright (c) 2017-2018 Christoph Hellwig.
>   */
>  
> +#include "linux/types.h"

Please always use <> style includes for system headers.  But I don't
think we even need this include.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] nvme: enable generic interface (/dev/ngXnY) for unknown command sets
  2022-06-07 13:40   ` [PATCH v2 2/2] nvme: enable generic interface (/dev/ngXnY) for unknown command sets Joel Granados
@ 2022-06-13 18:24     ` Christoph Hellwig
  2022-06-15 11:20       ` Joel Granados
  0 siblings, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2022-06-13 18:24 UTC (permalink / raw)
  To: Joel Granados; +Cc: hch, kbusch, joshi.k, linux-nvme, gost.dev, k.jensen

On Tue, Jun 07, 2022 at 03:40:55PM +0200, Joel Granados wrote:

>  static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
>  		struct nvme_ns_ids *ids)
>  {
> +	int ret = 0;
>  	struct nvme_ns *ns;
>  	struct gendisk *disk;
>  	struct nvme_id_ns *id;
> +	struct nvme_id_ns_cs_indep *indep_id;
>  	int node = ctrl->numa_node;
> +	bool cs_indep = nvme_ns_indep(ids);
>  
> -	if (nvme_identify_ns(ctrl, nsid, ids, &id))
> +	if (cs_indep)
> +		ret = nvme_identify_ns_cs_indep(ctrl, nsid, &indep_id);
> +	else
> +		ret = nvme_identify_ns(ctrl, nsid, ids, &id);

nvme_validate_or_alloc_ns already does the nvme_identify_ns_cs_indep, so
we should be able to reuse it or move it here.

> -	if (nvme_init_ns_head(ns, nsid, ids, id->nmic & NVME_NS_NMIC_SHARED))
> +	if (nvme_init_ns_head(ns, nsid, ids,
> +			      (cs_indep ? indep_id->nmic : id->nmic) & NVME_NS_NMIC_SHARED))

This is pretty ugly.  I think if there is no command set idepdent
structure supported, we should probably just fake one up based on the
legacy Identify Namespace.

> +	if (cs_indep)
> +		ret = nvme_update_ns_info_cs_indep(ns, indep_id);
> +	else
> +		ret = nvme_update_ns_info(ns, id);

And here we should do a switch on ids->csi ala:

	 switch (ids.csi) {
	 case NVME_CSI_NVM:
	 case NVME_CSI_ZNS:
	 	ret = nvme_update_ns_info_block(ns, id);
		break
	default:
		nvme_update_ns_info_generic(ns, iid);
		break;
	}


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/2] nvme-multipath: refactor nvme_mpath_add_disk
  2022-06-13 18:16     ` Christoph Hellwig
@ 2022-06-15  7:50       ` Joel Granados
  0 siblings, 0 replies; 9+ messages in thread
From: Joel Granados @ 2022-06-15  7:50 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: kbusch, joshi.k, linux-nvme, gost.dev, k.jensen

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

On Mon, Jun 13, 2022 at 08:16:20PM +0200, Christoph Hellwig wrote:
> > +++ b/drivers/nvme/host/multipath.c
> > @@ -3,6 +3,7 @@
> >   * Copyright (c) 2017-2018 Christoph Hellwig.
> >   */
> >  
> > +#include "linux/types.h"
> 
> Please always use <> style includes for system headers.  But I don't
> think we even need this include.

I'll just remove this line for the next version. Thx.

Joel

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] nvme: enable generic interface (/dev/ngXnY) for unknown command sets
  2022-06-13 18:24     ` Christoph Hellwig
@ 2022-06-15 11:20       ` Joel Granados
  2022-06-15 14:50         ` Joel Granados
  2022-06-22 14:11         ` Joel Granados
  0 siblings, 2 replies; 9+ messages in thread
From: Joel Granados @ 2022-06-15 11:20 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: kbusch, joshi.k, linux-nvme, gost.dev, k.jensen

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

On Mon, Jun 13, 2022 at 08:24:39PM +0200, Christoph Hellwig wrote:
> On Tue, Jun 07, 2022 at 03:40:55PM +0200, Joel Granados wrote:
> 
> >  static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
> >  		struct nvme_ns_ids *ids)
> >  {
> > +	int ret = 0;
> >  	struct nvme_ns *ns;
> >  	struct gendisk *disk;
> >  	struct nvme_id_ns *id;
> > +	struct nvme_id_ns_cs_indep *indep_id;
> >  	int node = ctrl->numa_node;
> > +	bool cs_indep = nvme_ns_indep(ids);
> >  
> > -	if (nvme_identify_ns(ctrl, nsid, ids, &id))
> > +	if (cs_indep)
> > +		ret = nvme_identify_ns_cs_indep(ctrl, nsid, &indep_id);
> > +	else
> > +		ret = nvme_identify_ns(ctrl, nsid, ids, &id);
> 
> nvme_validate_or_alloc_ns already does the nvme_identify_ns_cs_indep, so
> we should be able to reuse it or move it here.
One way we can do this is to take the nvme_id_ns_cs_indep id that we got
from calling nvme_identify_ns_cs_indep and pass it along to
nvme_alloc_ns. Let me adjust this for V3.

> 
> > -	if (nvme_init_ns_head(ns, nsid, ids, id->nmic & NVME_NS_NMIC_SHARED))
> > +	if (nvme_init_ns_head(ns, nsid, ids,
> > +			      (cs_indep ? indep_id->nmic : id->nmic) & NVME_NS_NMIC_SHARED))
> 
> This is pretty ugly.  I think if there is no command set idepdent
> structure supported, we should probably just fake one up based on the
> legacy Identify Namespace.

Do you mean that ns->head should be "faked" with a call different
than nvme_init_ns_head which is based on the legacy Identify Namespace?

> 
> > +	if (cs_indep)
> > +		ret = nvme_update_ns_info_cs_indep(ns, indep_id);
> > +	else
> > +		ret = nvme_update_ns_info(ns, id);
> 
> And here we should do a switch on ids->csi ala:
> 
> 	 switch (ids.csi) {
> 	 case NVME_CSI_NVM:
> 	 case NVME_CSI_ZNS:
> 	 	ret = nvme_update_ns_info_block(ns, id);
> 		break
> 	default:
> 		nvme_update_ns_info_generic(ns, iid);
> 		break;
> 	}
> 
Will adjust for V3

Joel

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] nvme: enable generic interface (/dev/ngXnY) for unknown command sets
  2022-06-15 11:20       ` Joel Granados
@ 2022-06-15 14:50         ` Joel Granados
  2022-06-22 14:11         ` Joel Granados
  1 sibling, 0 replies; 9+ messages in thread
From: Joel Granados @ 2022-06-15 14:50 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: kbusch, joshi.k, linux-nvme, gost.dev, k.jensen

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

On Wed, Jun 15, 2022 at 01:20:19PM +0200, Joel Granados wrote:
> On Mon, Jun 13, 2022 at 08:24:39PM +0200, Christoph Hellwig wrote:
> > On Tue, Jun 07, 2022 at 03:40:55PM +0200, Joel Granados wrote:
> > 
> > >  static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
> > >  		struct nvme_ns_ids *ids)
> > >  {
> > > +	int ret = 0;
> > >  	struct nvme_ns *ns;
> > >  	struct gendisk *disk;
> > >  	struct nvme_id_ns *id;
> > > +	struct nvme_id_ns_cs_indep *indep_id;
> > >  	int node = ctrl->numa_node;
> > > +	bool cs_indep = nvme_ns_indep(ids);
> > >  
> > > -	if (nvme_identify_ns(ctrl, nsid, ids, &id))
> > > +	if (cs_indep)
> > > +		ret = nvme_identify_ns_cs_indep(ctrl, nsid, &indep_id);
> > > +	else
> > > +		ret = nvme_identify_ns(ctrl, nsid, ids, &id);
> > 
> > nvme_validate_or_alloc_ns already does the nvme_identify_ns_cs_indep, so
> > we should be able to reuse it or move it here.
> One way we can do this is to take the nvme_id_ns_cs_indep id that we got
> from calling nvme_identify_ns_cs_indep and pass it along to
> nvme_alloc_ns. Let me adjust this for V3.
> 
> > 
> > > -	if (nvme_init_ns_head(ns, nsid, ids, id->nmic & NVME_NS_NMIC_SHARED))
> > > +	if (nvme_init_ns_head(ns, nsid, ids,
> > > +			      (cs_indep ? indep_id->nmic : id->nmic) & NVME_NS_NMIC_SHARED))
> > 
> > This is pretty ugly.  I think if there is no command set idepdent
> > structure supported, we should probably just fake one up based on the
> > legacy Identify Namespace.
> 
> Do you mean that ns->head should be "faked" with a call different
> than nvme_init_ns_head which is based on the legacy Identify Namespace?
> 
> > 
> > > +	if (cs_indep)
> > > +		ret = nvme_update_ns_info_cs_indep(ns, indep_id);
> > > +	else
> > > +		ret = nvme_update_ns_info(ns, id);
> > 
> > And here we should do a switch on ids->csi ala:
> > 
> > 	 switch (ids.csi) {
> > 	 case NVME_CSI_NVM:
> > 	 case NVME_CSI_ZNS:
> > 	 	ret = nvme_update_ns_info_block(ns, id);
> > 		break
> > 	default:
> > 		nvme_update_ns_info_generic(ns, iid);
> > 		break;
> > 	}
> > 
> Will adjust for V3

I see the point of making the command sets more explicit and for
consistency I will also change the other conditionals. Its a bit more
lines, but we gain clarity.

> 
> Joel



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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] nvme: enable generic interface (/dev/ngXnY) for unknown command sets
  2022-06-15 11:20       ` Joel Granados
  2022-06-15 14:50         ` Joel Granados
@ 2022-06-22 14:11         ` Joel Granados
  1 sibling, 0 replies; 9+ messages in thread
From: Joel Granados @ 2022-06-22 14:11 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: kbusch, joshi.k, linux-nvme, gost.dev, k.jensen

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

On Wed, Jun 15, 2022 at 01:20:19PM +0200, Joel Granados wrote:
> On Mon, Jun 13, 2022 at 08:24:39PM +0200, Christoph Hellwig wrote:
> > On Tue, Jun 07, 2022 at 03:40:55PM +0200, Joel Granados wrote:
> > 
> > >  static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
> > >  		struct nvme_ns_ids *ids)
> > >  {
> > > +	int ret = 0;
> > >  	struct nvme_ns *ns;
> > >  	struct gendisk *disk;
> > >  	struct nvme_id_ns *id;
> > > +	struct nvme_id_ns_cs_indep *indep_id;
> > >  	int node = ctrl->numa_node;
> > > +	bool cs_indep = nvme_ns_indep(ids);
> > >  
> > > -	if (nvme_identify_ns(ctrl, nsid, ids, &id))
> > > +	if (cs_indep)
> > > +		ret = nvme_identify_ns_cs_indep(ctrl, nsid, &indep_id);
> > > +	else
> > > +		ret = nvme_identify_ns(ctrl, nsid, ids, &id);
> > 
> > nvme_validate_or_alloc_ns already does the nvme_identify_ns_cs_indep, so
> > we should be able to reuse it or move it here.
> One way we can do this is to take the nvme_id_ns_cs_indep id that we got
> from calling nvme_identify_ns_cs_indep and pass it along to
> nvme_alloc_ns. Let me adjust this for V3.
> 
> > 
> > > -	if (nvme_init_ns_head(ns, nsid, ids, id->nmic & NVME_NS_NMIC_SHARED))
> > > +	if (nvme_init_ns_head(ns, nsid, ids,
> > > +			      (cs_indep ? indep_id->nmic : id->nmic) & NVME_NS_NMIC_SHARED))
> > 
> > This is pretty ugly.  I think if there is no command set idepdent
> > structure supported, we should probably just fake one up based on the
> > legacy Identify Namespace.
> 
> Do you mean that ns->head should be "faked" with a call different
> than nvme_init_ns_head which is based on the legacy Identify Namespace?

I have posted V3 for this patch
(https://lore.kernel.org/all/20220622135556.2185991-1-j.granados@samsung.com/),
but am still unsure if I have addressed this concern. We can continue
discussing in the V3 thread if you still see that something needs to be
changed

Best

Joel

> 
> > 
> > > +	if (cs_indep)
> > > +		ret = nvme_update_ns_info_cs_indep(ns, indep_id);
> > > +	else
> > > +		ret = nvme_update_ns_info(ns, id);
> > 
> > And here we should do a switch on ids->csi ala:
> > 
> > 	 switch (ids.csi) {
> > 	 case NVME_CSI_NVM:
> > 	 case NVME_CSI_ZNS:
> > 	 	ret = nvme_update_ns_info_block(ns, id);
> > 		break
> > 	default:
> > 		nvme_update_ns_info_generic(ns, iid);
> > 		break;
> > 	}
> > 
> Will adjust for V3
> 
> Joel



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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-06-22 14:11 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <CGME20220607134309eucas1p1724cee80a5bc116c652e91422d474ccd@eucas1p1.samsung.com>
2022-06-07 13:40 ` [PATCH v2 0/2] nvme: Enable generic interface (/dev/ngX) for unknown command sets Joel Granados
2022-06-07 13:40   ` [PATCH v2 1/2] nvme-multipath: refactor nvme_mpath_add_disk Joel Granados
2022-06-13 18:16     ` Christoph Hellwig
2022-06-15  7:50       ` Joel Granados
2022-06-07 13:40   ` [PATCH v2 2/2] nvme: enable generic interface (/dev/ngXnY) for unknown command sets Joel Granados
2022-06-13 18:24     ` Christoph Hellwig
2022-06-15 11:20       ` Joel Granados
2022-06-15 14:50         ` Joel Granados
2022-06-22 14:11         ` Joel Granados

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox