[PATCH] nvme: fix cns check

[PATCH] nvme: fix cns check

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

The 1.1 spec defined two bits for the Identify CNS field, so such an
implementation wouldn't be expected to check the full byte for a
decoding it. Provide a different helper function to make these
decisions as the existing check only separates the single-bit from the
two-bit spec versions.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/core.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 10eca7660ca7c..0f7ff5f417f17 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1412,6 +1412,11 @@ static bool nvme_ctrl_limited_cns(struct nvme_ctrl *ctrl)
 	return ctrl->vs < NVME_VS(1, 1, 0);
 }
 
+static bool nvme_ctrl_byte_cns(struct nvme_ctrl *ctrl)
+{
+	return ctrl->vs >= NVME_VS(1, 2, 0);
+}
+
 static int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id)
 {
 	struct nvme_command c = { };
@@ -3113,7 +3118,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_ctrl_byte_cns(ctrl) ||
 	    test_bit(NVME_CTRL_SKIP_ID_CNS_CS, &ctrl->flags))
 		return 0;
 
-- 
2.43.5

Re: [PATCH] nvme: fix cns check

[Christoph Hellwig]

On Wed, Oct 09, 2024 at 02:40:35PM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The 1.1 spec defined two bits for the Identify CNS field, so such an
> implementation wouldn't be expected to check the full byte for a
> decoding it. Provide a different helper function to make these
> decisions as the existing check only separates the single-bit from the
> two-bit spec versions.

I don't really understand what this is trying to solve.  Is there a
controller in the wild reporting 1.1 compliance but reporting useful
information in the NVM Command Set specific Identify Controller data
structure?

Re: [PATCH] nvme: fix cns check

[Keith Busch]

On Thu, Oct 10, 2024 at 09:56:33AM +0200, Christoph Hellwig wrote:
> On Wed, Oct 09, 2024 at 02:40:35PM -0700, Keith Busch wrote:
> > From: Keith Busch <kbusch@kernel.org>
> > 
> > The 1.1 spec defined two bits for the Identify CNS field, so such an
> > implementation wouldn't be expected to check the full byte for a
> > decoding it. Provide a different helper function to make these
> > decisions as the existing check only separates the single-bit from the
> > two-bit spec versions.
> 
> I don't really understand what this is trying to solve.  Is there a
> controller in the wild reporting 1.1 compliance but reporting useful
> information in the NVM Command Set specific Identify Controller data
> structure?

It's not going to return anything useful. The 1.1 compliant controller
truncates Command Set Specific ID Ctrl (CNS 06h) to just two bits (2h),
believing the host requested the a namespace list, and the host will
think the success means it has different identification data.

I was only thinking about this because of the rotational media patches
were using the same criteria that would let a 1.1 complaint controller
see another Identify that will have a truncated CNS.

Re: [PATCH] nvme: fix cns check

[Christoph Hellwig]

On Thu, Oct 10, 2024 at 09:11:37AM -0600, Keith Busch wrote:
> It's not going to return anything useful. The 1.1 compliant controller
> truncates Command Set Specific ID Ctrl (CNS 06h) to just two bits (2h),
> believing the host requested the a namespace list, and the host will
> think the success means it has different identification data.
> 
> I was only thinking about this because of the rotational media patches
> were using the same criteria that would let a 1.1 complaint controller
> see another Identify that will have a truncated CNS.

Ok, I finally understand what you are doing now, I as just beeing
stupid before (and maybe the commit log didn't help enough, but I don't
want to shift blame :)).

So yes, something like this is probably fine.  But maybe we'll want to
struture it in a way that is easier to read instead of having two
helpers to decide which CNS value is supported.  Maybe something
like:

static bool nvme_identify_cns_allowed(struct nvme_ctrl *ctrl, u8 cns)
{
	switch (ctrl->vs) {
	default:
		/*
		 * Starting with NVMe 1.2 the CNS field occupies a full
		 * byte.
		 */
		return true;
	case NVME_VS(1, 1, 0):
		/*
		 * 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->quirks & NVME_QUIRK_IDENTIFY_CNS))
			return !(cns & ~0x02);
		fallthrough;
	case NVME_VS(1, 0, 0):
		/*
		 * NVMe 1.0 only used a single for the CNS value.
		 * (That's where the name comes from:
		 *  Controller or Namespace Structure)
		 */
		return !(cns & ~0x01);
	}
}

Re: [PATCH] nvme: fix cns check

[Keith Busch]

On Fri, Oct 11, 2024 at 10:09:25AM +0200, Christoph Hellwig wrote:
> So yes, something like this is probably fine.  But maybe we'll want to
> struture it in a way that is easier to read instead of having two
> helpers to decide which CNS value is supported.  Maybe something
> like:
> 
> static bool nvme_identify_cns_allowed(struct nvme_ctrl *ctrl, u8 cns)
> {
> 	switch (ctrl->vs) {
> 	default:
> 		/*
> 		 * Starting with NVMe 1.2 the CNS field occupies a full
> 		 * byte.
> 		 */
> 		return true;
> 	case NVME_VS(1, 1, 0):
> 		/*
> 		 * 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->quirks & NVME_QUIRK_IDENTIFY_CNS))
> 			return !(cns & ~0x02);
> 		fallthrough;
> 	case NVME_VS(1, 0, 0):
> 		/*
> 		 * NVMe 1.0 only used a single for the CNS value.
> 		 * (That's where the name comes from:
> 		 *  Controller or Namespace Structure)
> 		 */
> 		return !(cns & ~0x01);
> 	}
> }

I like this idea, and the spec makes it seem safe enough to assume the
tertiary version number is always 0 for 1.0 and 1.1 controllers. So
yeah, this looks good to me.

Re: [PATCH] nvme: fix cns check

[Christoph Hellwig]

On Tue, Oct 15, 2024 at 08:36:15AM -0600, Keith Busch wrote:
> I like this idea, and the spec makes it seem safe enough to assume the
> tertiary version number is always 0 for 1.0 and 1.1 controllers. So
> yeah, this looks good to me.

Do you want to pick this up, or do you want me to send a patch?

Re: [PATCH] nvme: fix cns check

[Keith Busch]

On Tue, Oct 15, 2024 at 05:16:52PM +0200, Christoph Hellwig wrote:
> On Tue, Oct 15, 2024 at 08:36:15AM -0600, Keith Busch wrote:
> > I like this idea, and the spec makes it seem safe enough to assume the
> > tertiary version number is always 0 for 1.0 and 1.1 controllers. So
> > yeah, this looks good to me.
> 
> Do you want to pick this up, or do you want me to send a patch?

I'll respin a patch with this idea. I'm just catching up from a short
vacation, so might need one more day to get a v2 out.