[PATCH] nvmet: allow setting ctrl to ready without iosqes/iocqes set

[PATCH] nvmet: allow setting ctrl to ready without iosqes/iocqes set

[ran.anner]

From: Ran Anner <Ran.Anner@dell.com>

According to the spec we should fail creation of IO queues
if iosqes or iocqes properties are not set but does not mention
preventing from changing ctrl status to ready.
We have seen host implementation which sets the property to 0x1
and waits endlessly until ctrl status changes to ready.

Signed-off-by: Ran Anner <Ran.Anner@dell.com>
---
 drivers/nvme/target/core.c        | 39 +------------------------------
 drivers/nvme/target/fabrics-cmd.c |  9 +++++++
 drivers/nvme/target/nvmet.h       | 34 +++++++++++++++++++++++++++
 3 files changed, 44 insertions(+), 38 deletions(-)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index adbede9ab7f3..559dc7354adb 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1078,48 +1078,11 @@ void nvmet_req_free_sgls(struct nvmet_req *req)
 }
 EXPORT_SYMBOL_GPL(nvmet_req_free_sgls);
 
-static inline bool nvmet_cc_en(u32 cc)
-{
-	return (cc >> NVME_CC_EN_SHIFT) & 0x1;
-}
-
-static inline u8 nvmet_cc_css(u32 cc)
-{
-	return (cc >> NVME_CC_CSS_SHIFT) & 0x7;
-}
-
-static inline u8 nvmet_cc_mps(u32 cc)
-{
-	return (cc >> NVME_CC_MPS_SHIFT) & 0xf;
-}
-
-static inline u8 nvmet_cc_ams(u32 cc)
-{
-	return (cc >> NVME_CC_AMS_SHIFT) & 0x7;
-}
-
-static inline u8 nvmet_cc_shn(u32 cc)
-{
-	return (cc >> NVME_CC_SHN_SHIFT) & 0x3;
-}
-
-static inline u8 nvmet_cc_iosqes(u32 cc)
-{
-	return (cc >> NVME_CC_IOSQES_SHIFT) & 0xf;
-}
-
-static inline u8 nvmet_cc_iocqes(u32 cc)
-{
-	return (cc >> NVME_CC_IOCQES_SHIFT) & 0xf;
-}
-
 static void nvmet_start_ctrl(struct nvmet_ctrl *ctrl)
 {
 	lockdep_assert_held(&ctrl->lock);
 
-	if (nvmet_cc_iosqes(ctrl->cc) != NVME_NVM_IOSQES ||
-	    nvmet_cc_iocqes(ctrl->cc) != NVME_NVM_IOCQES ||
-	    nvmet_cc_mps(ctrl->cc) != 0 ||
+	if (nvmet_cc_mps(ctrl->cc) != 0 ||
 	    nvmet_cc_ams(ctrl->cc) != 0 ||
 	    nvmet_cc_css(ctrl->cc) != 0) {
 		ctrl->csts = NVME_CSTS_CFS;
diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
index 1420a8e3e0b1..421ab564ed5c 100644
--- a/drivers/nvme/target/fabrics-cmd.c
+++ b/drivers/nvme/target/fabrics-cmd.c
@@ -259,6 +259,15 @@ static void nvmet_execute_io_connect(struct nvmet_req *req)
 		goto out_ctrl_put;
 	}
 
+	if (nvmet_cc_iosqes(ctrl->cc) != NVME_NVM_IOSQES ||
+			nvmet_cc_iocqes(ctrl->cc) != NVME_NVM_IOCQES) {
+		pr_warn("reject connect cmd (IOSQES %u IOCSES %u)",
+				nvmet_cc_iosqes(ctrl->cc),
+				nvmet_cc_iocqes(ctrl->cc));
+		status = NVME_SC_QUEUE_SIZE;
+		goto out_ctrl_put;
+	}
+
 	status = nvmet_install_queue(ctrl, req);
 	if (status) {
 		/* pass back cntlid that had the issue of installing queue */
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 5566ed403576..18213edb8a07 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -616,4 +616,38 @@ static inline sector_t nvmet_lba_to_sect(struct nvmet_ns *ns, __le64 lba)
 	return le64_to_cpu(lba) << (ns->blksize_shift - SECTOR_SHIFT);
 }
 
+static inline bool nvmet_cc_en(u32 cc)
+{
+	return (cc >> NVME_CC_EN_SHIFT) & 0x1;
+}
+
+static inline u8 nvmet_cc_css(u32 cc)
+{
+	return (cc >> NVME_CC_CSS_SHIFT) & 0x7;
+}
+
+static inline u8 nvmet_cc_mps(u32 cc)
+{
+	return (cc >> NVME_CC_MPS_SHIFT) & 0xf;
+}
+
+static inline u8 nvmet_cc_ams(u32 cc)
+{
+	return (cc >> NVME_CC_AMS_SHIFT) & 0x7;
+}
+
+static inline u8 nvmet_cc_shn(u32 cc)
+{
+	return (cc >> NVME_CC_SHN_SHIFT) & 0x3;
+}
+
+static inline u8 nvmet_cc_iosqes(u32 cc)
+{
+	return (cc >> NVME_CC_IOSQES_SHIFT) & 0xf;
+}
+
+static inline u8 nvmet_cc_iocqes(u32 cc)
+{
+	return (cc >> NVME_CC_IOCQES_SHIFT) & 0xf;
+}
 #endif /* _NVMET_H */
-- 
2.18.2


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvmet: allow setting ctrl to ready without iosqes/iocqes set

[Chaitanya Kulkarni]

On 5/3/21 15:17, ran.anner@dell.com wrote:
>  
> +static inline bool nvmet_cc_en(u32 cc)
> +{
> +	return (cc >> NVME_CC_EN_SHIFT) & 0x1;
> +}
> +
> +static inline u8 nvmet_cc_css(u32 cc)
> +{
> +	return (cc >> NVME_CC_CSS_SHIFT) & 0x7;
> +}
> +
> +static inline u8 nvmet_cc_mps(u32 cc)
> +{
> +	return (cc >> NVME_CC_MPS_SHIFT) & 0xf;
> +}
> +
> +static inline u8 nvmet_cc_ams(u32 cc)
> +{
> +	return (cc >> NVME_CC_AMS_SHIFT) & 0x7;
> +}
> +
> +static inline u8 nvmet_cc_shn(u32 cc)
> +{
> +	return (cc >> NVME_CC_SHN_SHIFT) & 0x3;
> +}
> +
> +static inline u8 nvmet_cc_iosqes(u32 cc)
> +{
> +	return (cc >> NVME_CC_IOSQES_SHIFT) & 0xf;
> +}
> +
> +static inline u8 nvmet_cc_iocqes(u32 cc)
> +{
> +	return (cc >> NVME_CC_IOCQES_SHIFT) & 0xf;
> +}

Why are we bloating header file ?

If I remember the code correctly some of these functions are
only used in the target/core.c so there is no point in moving
those unless we have callers in the second file.

CC all the maintainers for NVMeOF target to get the better
response.



_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvmet: allow setting ctrl to ready without iosqes/iocqes set

[Christoph Hellwig]

On Sun, May 02, 2021 at 06:31:06PM +0300, ran.anner@dell.com wrote:
> From: Ran Anner <Ran.Anner@dell.com>
> 
> According to the spec we should fail creation of IO queues
> if iosqes or iocqes properties are not set but does not mention
> preventing from changing ctrl status to ready.
> We have seen host implementation which sets the property to 0x1
> and waits endlessly until ctrl status changes to ready.

So is that for a discovery controller?  The spec says the field shall
be read-only for them, although we don't reject writes to the property
right now.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

RE: [PATCH] nvmet: allow setting ctrl to ready without iosqes/iocqes set

[Anner, Ran]

Reviving this thread.

-----Original Message-----
From: Christoph Hellwig <hch@lst.de> 
Sent: Tuesday, May 11, 2021 19:34
To: Anner, Ran
Cc: Christoph Hellwig
Subject: Re: [PATCH] nvmet: allow setting ctrl to ready without iosqes/iocqes set


[EXTERNAL EMAIL] 

Please resend the mail with the mainling list in the loop.

On Mon, May 10, 2021 at 12:27:36PM +0000, Anner, Ran wrote:
> Hi Christoph.
> 
> This is for IO controller.
> We saw a host behavior where only the cc.en bit was set and the host was waiting for csts to indicate ready state.
> >From what I saw in the nvme specification we should only fail connect commands that create IO queues in case IOSQES/IOCQES is not set and csts should not be effected.
> 
> Thanks
> 
> -----Original Message-----
> From: Christoph Hellwig <hch@lst.de>
> Sent: Tuesday, May 4, 2021 12:13
> To: Anner, Ran
> Cc: hch@lst.de; linux-nvme@lists.infradead.org
> Subject: Re: [PATCH] nvmet: allow setting ctrl to ready without 
> iosqes/iocqes set
> 
> 
> [EXTERNAL EMAIL]
> 
> On Sun, May 02, 2021 at 06:31:06PM +0300, ran.anner@dell.com wrote:
> > From: Ran Anner <Ran.Anner@dell.com>
> > 
> > According to the spec we should fail creation of IO queues if iosqes 
> > or iocqes properties are not set but does not mention preventing 
> > from changing ctrl status to ready.
> > We have seen host implementation which sets the property to 0x1 and 
> > waits endlessly until ctrl status changes to ready.
> 
> So is that for a discovery controller?  The spec says the field shall be read-only for them, although we don't reject writes to the property right now.
---end quoted text---

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvmet: allow setting ctrl to ready without iosqes/iocqes set

[Sagi Grimberg]

> From: Ran Anner <Ran.Anner@dell.com>
> 
> According to the spec we should fail creation of IO queues
> if iosqes or iocqes properties are not set

Would be good to add the specific section in the spec.

  but does not mention
> preventing from changing ctrl status to ready.
> We have seen host implementation which sets the property to 0x1
> and waits endlessly until ctrl status changes to ready.
> 
> Signed-off-by: Ran Anner <Ran.Anner@dell.com>
> ---
>   drivers/nvme/target/core.c        | 39 +------------------------------
>   drivers/nvme/target/fabrics-cmd.c |  9 +++++++
>   drivers/nvme/target/nvmet.h       | 34 +++++++++++++++++++++++++++
>   3 files changed, 44 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index adbede9ab7f3..559dc7354adb 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -1078,48 +1078,11 @@ void nvmet_req_free_sgls(struct nvmet_req *req)
>   }
>   EXPORT_SYMBOL_GPL(nvmet_req_free_sgls);
>   
> -static inline bool nvmet_cc_en(u32 cc)
> -{
> -	return (cc >> NVME_CC_EN_SHIFT) & 0x1;
> -}
> -
> -static inline u8 nvmet_cc_css(u32 cc)
> -{
> -	return (cc >> NVME_CC_CSS_SHIFT) & 0x7;
> -}
> -
> -static inline u8 nvmet_cc_mps(u32 cc)
> -{
> -	return (cc >> NVME_CC_MPS_SHIFT) & 0xf;
> -}
> -
> -static inline u8 nvmet_cc_ams(u32 cc)
> -{
> -	return (cc >> NVME_CC_AMS_SHIFT) & 0x7;
> -}
> -
> -static inline u8 nvmet_cc_shn(u32 cc)
> -{
> -	return (cc >> NVME_CC_SHN_SHIFT) & 0x3;
> -}
> -
> -static inline u8 nvmet_cc_iosqes(u32 cc)
> -{
> -	return (cc >> NVME_CC_IOSQES_SHIFT) & 0xf;
> -}
> -
> -static inline u8 nvmet_cc_iocqes(u32 cc)
> -{
> -	return (cc >> NVME_CC_IOCQES_SHIFT) & 0xf;
> -}
> -
>   static void nvmet_start_ctrl(struct nvmet_ctrl *ctrl)
>   {
>   	lockdep_assert_held(&ctrl->lock);
>   
> -	if (nvmet_cc_iosqes(ctrl->cc) != NVME_NVM_IOSQES ||
> -	    nvmet_cc_iocqes(ctrl->cc) != NVME_NVM_IOCQES ||
> -	    nvmet_cc_mps(ctrl->cc) != 0 ||
> +	if (nvmet_cc_mps(ctrl->cc) != 0 ||
>   	    nvmet_cc_ams(ctrl->cc) != 0 ||
>   	    nvmet_cc_css(ctrl->cc) != 0) {
>   		ctrl->csts = NVME_CSTS_CFS;
> diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
> index 1420a8e3e0b1..421ab564ed5c 100644
> --- a/drivers/nvme/target/fabrics-cmd.c
> +++ b/drivers/nvme/target/fabrics-cmd.c
> @@ -259,6 +259,15 @@ static void nvmet_execute_io_connect(struct nvmet_req *req)
>   		goto out_ctrl_put;
>   	}
>   
> +	if (nvmet_cc_iosqes(ctrl->cc) != NVME_NVM_IOSQES ||
> +			nvmet_cc_iocqes(ctrl->cc) != NVME_NVM_IOCQES) {

Can you indent to the start of the parenthesis?

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme