Re: [PATCH] nvme: fix cns check

[Keith Busch <kbusch@kernel.org>]

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.