[PATCH] nvme-fabrics: handle zero MAXCMD without closing the connection

[PATCH] nvme-fabrics: handle zero MAXCMD without closing the connection

[Maurizio Lombardi]

The NVMe specification states that MAXCMD is mandatory
for NVMe-over-Fabrics implementations. However, some NVMe/TCP
and NVMe/FC arrays from major vendors have buggy firmware
that reports MAXCMD as zero in the Identify Controller data structure.

Currently, the implementation closes the connection in such cases,
completely preventing the host from connecting to the target.

Fix the issue by printing a clear error message about the firmware bug
and allowing the connection to proceed. It assumes that the
target supports a MAXCMD value of SQSIZE + 1. If any issues arise,
the user can manually adjust SQSIZE to mitigate them.

Fixes: 4999568184e5 ("nvme-fabrics: check max outstanding commands")
Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
---
 drivers/nvme/host/core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 1a8d32a4a5c3..91ee2c3aa95e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3257,8 +3257,9 @@ static int nvme_check_ctrl_fabric_info(struct nvme_ctrl *ctrl, struct nvme_id_ct
 	}
 
 	if (!ctrl->maxcmd) {
-		dev_err(ctrl->device, "Maximum outstanding commands is 0\n");
-		return -EINVAL;
+		dev_err(ctrl->device,
+			"Firmware bug: maximum outstanding commands is 0\n");
+		ctrl->maxcmd = ctrl->sqsize + 1;
 	}
 
 	return 0;
-- 
2.43.5

Re: [PATCH] nvme-fabrics: handle zero MAXCMD without closing the connection

[Laurence Oberman]

On Fri, 2024-11-29 at 15:17 +0100, Maurizio Lombardi wrote:
> The NVMe specification states that MAXCMD is mandatory
> for NVMe-over-Fabrics implementations. However, some NVMe/TCP
> and NVMe/FC arrays from major vendors have buggy firmware
> that reports MAXCMD as zero in the Identify Controller data
> structure.
> 
> Currently, the implementation closes the connection in such cases,
> completely preventing the host from connecting to the target.
> 
> Fix the issue by printing a clear error message about the firmware
> bug
> and allowing the connection to proceed. It assumes that the
> target supports a MAXCMD value of SQSIZE + 1. If any issues arise,
> the user can manually adjust SQSIZE to mitigate them.
> 
> Fixes: 4999568184e5 ("nvme-fabrics: check max outstanding commands")
> Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
> ---
>  drivers/nvme/host/core.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 1a8d32a4a5c3..91ee2c3aa95e 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3257,8 +3257,9 @@ static int nvme_check_ctrl_fabric_info(struct
> nvme_ctrl *ctrl, struct nvme_id_ct
>         }
>  
>         if (!ctrl->maxcmd) {
> -               dev_err(ctrl->device, "Maximum outstanding commands
> is 0\n");
> -               return -EINVAL;
> +               dev_err(ctrl->device,
> +                       "Firmware bug: maximum outstanding commands
> is 0\n");
> +               ctrl->maxcmd = ctrl->sqsize + 1;
>         }
>  
>         return 0;

A similar fix was tested by a customer seeing this issue and they can
now connect to their array again.
Looks good.

Reviewed-by: Laurence Oberman <loberman@redhat.com>

Re: [PATCH] nvme-fabrics: handle zero MAXCMD without closing the connection

[Christoph Hellwig]

On Fri, Nov 29, 2024 at 03:17:06PM +0100, Maurizio Lombardi wrote:
> The NVMe specification states that MAXCMD is mandatory
> for NVMe-over-Fabrics implementations. However, some NVMe/TCP
> and NVMe/FC arrays from major vendors have buggy firmware
> that reports MAXCMD as zero in the Identify Controller data structure.

Sigh.

>  	if (!ctrl->maxcmd) {
> -		dev_err(ctrl->device, "Maximum outstanding commands is 0\n");
> -		return -EINVAL;
> +		dev_err(ctrl->device,
> +			"Firmware bug: maximum outstanding commands is 0\n");
> +		ctrl->maxcmd = ctrl->sqsize + 1;

Maybe warn instead of err as made it not an error now?  Otherwise
looks fine.