[PATCHv2] nvme: enhance cns version checking

[PATCHv2] nvme: enhance cns version checking

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

The number of CNS bits in the command is specific to the nvme spec
version compliance. The existing check is not sufficient for possible
CNS values the driver uses, so enhance the check to consider the version
and desired CNS value.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
v1->v2:

  Replaced the existing helper with a more complex version suggested by
  Christoph. Implemented a little differently here without a switch
  statement: I just don't trust vendors to not have mucked with the
  tertiary field.

 drivers/nvme/host/core.c | 37 +++++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 9e7e63e10e5a8..f4a26199e695f 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1399,17 +1399,30 @@ static void nvme_update_keep_alive(struct nvme_ctrl *ctrl,
 	nvme_start_keep_alive(ctrl);
 }
 
-/*
- * In NVMe 1.0 the CNS field was just a binary controller or namespace
- * flag, thus sending any new CNS opcodes has a big chance of not working.
- * Qemu unfortunately had that bug after reporting a 1.1 version compliance
- * (but not for any later version).
- */
-static bool nvme_ctrl_limited_cns(struct nvme_ctrl *ctrl)
+static bool nvme_id_cns_ok(struct nvme_ctrl *ctrl, u8 cns)
 {
-	if (ctrl->quirks & NVME_QUIRK_IDENTIFY_CNS)
-		return ctrl->vs < NVME_VS(1, 2, 0);
-	return ctrl->vs < NVME_VS(1, 1, 0);
+	/*
+	 * The CNS field occupies a full byte starting with NVMe 1.2
+	 */
+	if (ctrl->vs >= NVME_VS(1, 2, 0))
+		return true;
+
+	/*
+	 * NVMe 1.1 expanded the CNS value to two bytes, which means values
+	 * larger than that could get truncated and treated as an incorrect
+	 * value.
+	 *
+	 * Qemu implemented 1.0 behavior for controllers claiming 1.1
+	 * compliance, so they need to be quirked here.
+	 */
+	if (ctrl->vs >= NVME_VS(1, 1, 0) &&
+	    (!(ctrl->quirks & NVME_QUIRK_IDENTIFY_CNS)))
+		return cns <= 3;
+
+	/*
+	 * NVMe 1.0 used a single bit for the CNS value.
+         */
+	return cns <= 1;
 }
 
 static int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id)
@@ -3113,7 +3126,7 @@ static int nvme_init_non_mdts_limits(struct nvme_ctrl *ctrl)
 		ctrl->max_zeroes_sectors = 0;
 
 	if (ctrl->subsys->subtype != NVME_NQN_NVME ||
-	    nvme_ctrl_limited_cns(ctrl) ||
+	    !nvme_id_cns_ok(ctrl, NVME_ID_CNS_CS_CTRL) ||
 	    test_bit(NVME_CTRL_SKIP_ID_CNS_CS, &ctrl->flags))
 		return 0;
 
@@ -4209,7 +4222,7 @@ static void nvme_scan_work(struct work_struct *work)
 	}
 
 	mutex_lock(&ctrl->scan_lock);
-	if (nvme_ctrl_limited_cns(ctrl)) {
+	if (!nvme_id_cns_ok(ctrl, NVME_ID_CNS_NS_ACTIVE_LIST)) {
 		nvme_scan_ns_sequential(ctrl);
 	} else {
 		/*
-- 
2.43.5

Re: [PATCHv2] nvme: enhance cns version checking

[Christoph Hellwig]

On Thu, Oct 17, 2024 at 11:12:31AM -0700, Keith Busch wrote:
> +	if (ctrl->vs >= NVME_VS(1, 1, 0) &&
> +	    (!(ctrl->quirks & NVME_QUIRK_IDENTIFY_CNS)))

No need for the braces around the ngation here.

> +		return cns <= 3;
> +
> +	/*
> +	 * NVMe 1.0 used a single bit for the CNS value.
> +         */
> +	return cns <= 1;

Slightly messed up indentation.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCHv2] nvme: enhance cns version checking

[Sagi Grimberg]

> Reviewed-by: Christoph Hellwig <hch@lst.de>
>

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCHv2] nvme: enhance cns version checking

[Keith Busch]

On Fri, Oct 18, 2024 at 07:12:01AM +0200, Christoph Hellwig wrote:
> On Thu, Oct 17, 2024 at 11:12:31AM -0700, Keith Busch wrote:
> > +	if (ctrl->vs >= NVME_VS(1, 1, 0) &&
> > +	    (!(ctrl->quirks & NVME_QUIRK_IDENTIFY_CNS)))
> 
> No need for the braces around the ngation here.
> 
> > +		return cns <= 3;
> > +
> > +	/*
> > +	 * NVMe 1.0 used a single bit for the CNS value.
> > +         */
> > +	return cns <= 1;
> 
> Slightly messed up indentation.

Fixed up, and applied to nvme-6.12.