[PATCH 1/2] nvme-tcp: check sgl supported by target

[PATCH 1/2] nvme-tcp: check sgl supported by target

[Max Gurtovoy]

SGLs support is mandatory for NVMe/tcp, make sure that the target is
aligned to the specification.

Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 drivers/nvme/host/tcp.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index a0f00cb8f9f3..e5fbb921165e 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1973,6 +1973,11 @@ static int nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl, bool new)
 		goto destroy_admin;
 	}
 
+	if (!(ctrl->sgls & ((1 << 0) | (1 << 1)))) {
+		dev_err(ctrl->device, "Mandatory sgls are not supported!\n");
+		goto destroy_admin;
+	}
+
 	if (opts->queue_size > ctrl->sqsize + 1)
 		dev_warn(ctrl->device,
 			"queue_size %zu > ctrl sqsize %u, clamping down\n",
-- 
2.25.4


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

[PATCH 2/2] nvme-fc: check sgl supported by target

[Max Gurtovoy]

SGLs support is mandatory for NVMe/tcp, make sure that the target is
aligned to the specification.

Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 drivers/nvme/host/fc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 6ffa8de2a0d7..03032d9b0573 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -3100,6 +3100,11 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
 	}
 
 	/* FC-NVME supports normal SGL Data Block Descriptors */
+	if (!(ctrl->ctrl.sgls & ((1 << 0) | (1 << 1)))) {
+		dev_err(ctrl->ctrl.device,
+			"Mandatory sgls are not supported!\n");
+		goto out_disconnect_admin_queue;
+	}
 
 	if (opts->queue_size > ctrl->ctrl.maxcmd) {
 		/* warn if maxcmd is lower than queue_size */
-- 
2.25.4


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

Re: [PATCH 1/2] nvme-tcp: check sgl supported by target

[Chaitanya Kulkarni]

On 3/30/21 16:01, Max Gurtovoy wrote:
> SGLs support is mandatory for NVMe/tcp, make sure that the target is
> aligned to the specification.
>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>


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

Re: [PATCH 2/2] nvme-fc: check sgl supported by target

[Chaitanya Kulkarni]

On 3/30/21 16:01, Max Gurtovoy wrote:
> SGLs support is mandatory for NVMe/tcp, make sure that the target is
> aligned to the specification.
>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>

s/"NVMe/tcp"/"NVMe-FC"/ ? if so that can be done by at the time of
applying the patch, if not just ignore this comment.

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>



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

Re: [PATCH 1/2] nvme-tcp: check sgl supported by target

[Keith Busch]

On Tue, Mar 30, 2021 at 11:01:19PM +0000, Max Gurtovoy wrote:
> SGLs support is mandatory for NVMe/tcp, make sure that the target is
> aligned to the specification.

Logging an error seems fine. Are there existing controllers that do
this? If so, what currently happens? If they just have a broken Identify
Controller structure but still work with this driver, should we proceed
with trying to use it anyway with the existing assumption?
 
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> ---
>  drivers/nvme/host/tcp.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index a0f00cb8f9f3..e5fbb921165e 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -1973,6 +1973,11 @@ static int nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl, bool new)
>  		goto destroy_admin;
>  	}
>  
> +	if (!(ctrl->sgls & ((1 << 0) | (1 << 1)))) {
> +		dev_err(ctrl->device, "Mandatory sgls are not supported!\n");
> +		goto destroy_admin;
> +	}
> +
>  	if (opts->queue_size > ctrl->sqsize + 1)
>  		dev_warn(ctrl->device,
>  			"queue_size %zu > ctrl sqsize %u, clamping down\n",
> -- 

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

Re: [PATCH 1/2] nvme-tcp: check sgl supported by target

[Chaitanya Kulkarni]

On 3/30/21 17:30, Keith Busch wrote:
> On Tue, Mar 30, 2021 at 11:01:19PM +0000, Max Gurtovoy wrote:
>> SGLs support is mandatory for NVMe/tcp, make sure that the target is
>> aligned to the specification.
> Logging an error seems fine. Are there existing controllers that do
> this? If so, what currently happens? If they just have a broken Identify
> Controller structure but still work with this driver, should we proceed
> with trying to use it anyway with the existing assumption?
>  

At least I've not seen any, in case they do exists should we consider adding
a quirk if at all any of those controller's start showing up ?



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

Re: [PATCH 2/2] nvme-fc: check sgl supported by target

[Max Gurtovoy]

On 3/31/2021 2:42 AM, Chaitanya Kulkarni wrote:
> On 3/30/21 16:01, Max Gurtovoy wrote:
>> SGLs support is mandatory for NVMe/tcp, make sure that the target is
>> aligned to the specification.
>>
>> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> s/"NVMe/tcp"/"NVMe-FC"/ ? if so that can be done by at the time of
> applying the patch, if not just ignore this comment.

Good catch. Yes, this should be updated.

Thanks.

>
> Looks good.
>
> Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
>
>

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

Re: [PATCH 1/2] nvme-tcp: check sgl supported by target

[Max Gurtovoy]

On 3/31/2021 4:12 AM, Chaitanya Kulkarni wrote:
> On 3/30/21 17:30, Keith Busch wrote:
>> On Tue, Mar 30, 2021 at 11:01:19PM +0000, Max Gurtovoy wrote:
>>> SGLs support is mandatory for NVMe/tcp, make sure that the target is
>>> aligned to the specification.
>> Logging an error seems fine. Are there existing controllers that do
>> this? If so, what currently happens? If they just have a broken Identify
>> Controller structure but still work with this driver, should we proceed
>> with trying to use it anyway with the existing assumption?
>>   
> At least I've not seen any, in case they do exists should we consider adding
> a quirk if at all any of those controller's start showing up ?
>
I also haven't seen controllers with broken ID-CTRL, and adding a quirk 
in the future is better than ignoring the error and hope for good 
behavior from the controller.



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

Re: [PATCH 1/2] nvme-tcp: check sgl supported by target

[Christoph Hellwig]

Thanks,

applied both patches to nvme-5.13.

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