Re: [PATCH] nvme: fix cns check

[Christoph Hellwig <hch@lst.de>]

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);
	}
}