From: Matias Bjørling <matias.bjorling@wdc.com>

Enable support for NVMe devices that identifies as rotational.

Thanks to Keith, Damien, and Niklas for their feedback on the patchset.

Matias Bjørling (2):
  nvme: make independent ns identify default
  nvme: add rotational support

 drivers/nvme/host/core.c | 12 ++++++++----
 include/linux/nvme.h     |  1 +
 2 files changed, 9 insertions(+), 4 deletions(-)

-- 
2.46.0

On Tue, Oct 08, 2024 at 04:55:01PM +0200, Matias Bjørling wrote:
> From: Matias Bjørling <matias.bjorling@wdc.com>
> 
> Enable support for NVMe devices that identifies as rotational.
> 
> Thanks to Keith, Damien, and Niklas for their feedback on the patchset.

Hmm, the only previous version I've seen was the the RFCs from
Wang Yugui, last seen in August.

What the improvement over that version?  Note that it also came
with basic nvmet support which is kinda nice.

On Wed, Oct 09, 2024 at 09:43:55AM +0200, Christoph Hellwig wrote:
> On Tue, Oct 08, 2024 at 04:55:01PM +0200, Matias Bjørling wrote:
> > From: Matias Bjørling <matias.bjorling@wdc.com>
> > 
> > Enable support for NVMe devices that identifies as rotational.
> > 
> > Thanks to Keith, Damien, and Niklas for their feedback on the patchset.
> 
> Hmm, the only previous version I've seen was the the RFCs from
> Wang Yugui, last seen in August.

Oops, that slipped by me as well. I think the right thing to do is bring
that one forward and retain the original credit. I agree with Matias
that we ought to be able to query the independent identification without
relying on CRWMS, though.

On 09-10-2024 17:39, Keith Busch wrote:
> On Wed, Oct 09, 2024 at 09:43:55AM +0200, Christoph Hellwig wrote:
>> On Tue, Oct 08, 2024 at 04:55:01PM +0200, Matias Bjørling wrote:
>>> From: Matias Bjørling <matias.bjorling@wdc.com>
>>>
>>> Enable support for NVMe devices that identifies as rotational.
>>>
>>> Thanks to Keith, Damien, and Niklas for their feedback on the patchset.
>>
>> Hmm, the only previous version I've seen was the the RFCs from
>> Wang Yugui, last seen in August.
> 
> Oops, that slipped by me as well. I think the right thing to do is bring
> that one forward and retain the original credit. I agree with Matias
> that we ought to be able to query the independent identification without
> relying on CRWMS, though.

Works for me. Yugui, are you okay with me posting the updated patch 
serie with your patch?

On 09-10-2024 09:43, Christoph Hellwig wrote:
> On Tue, Oct 08, 2024 at 04:55:01PM +0200, Matias Bjørling wrote:
>> From: Matias Bjørling <matias.bjorling@wdc.com>
>>
>> Enable support for NVMe devices that identifies as rotational.
>>
>> Thanks to Keith, Damien, and Niklas for their feedback on the patchset.
> 
> Hmm, the only previous version I've seen was the the RFCs from
> Wang Yugui, last seen in August.
> 
> What the improvement over that version?  Note that it also came
> with basic nvmet support which is kinda nice.

Ah, I made the patches without awareness of the earlier efforts.

This version, together with Keith's nvmet patch, does not rely on 
setting/checking the CRIMS flag.

Matias,

> Enable support for NVMe devices that identifies as rotational.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

On Tue, Oct 08, 2024 at 04:55:01PM +0200, Matias Bjørling wrote:
> From: Matias Bjørling <matias.bjorling@wdc.com>
> 
> Enable support for NVMe devices that identifies as rotational.

Thanks, this looks good to me.

I still hope to see nvmet report this. It would be great to test this
with HDD backed nvme-loop target.

On Tue, Oct 08, 2024 at 09:13:48AM -0600, Keith Busch wrote:
> I still hope to see nvmet report this. It would be great to test this
> with HDD backed nvme-loop target.

I took the liberty to write one up. Looks like everything is reporting
as expected.

---
diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 954d4c0747704..e167c9a2ff995 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -685,6 +685,35 @@ static void nvmet_execute_identify_ctrl_nvm(struct nvmet_req *req)
 		   nvmet_zero_sgl(req, 0, sizeof(struct nvme_id_ctrl_nvm)));
 }
 
+static void nvmet_execute_id_cs_indep(struct nvmet_req *req)
+{
+	struct nvme_id_ns_cs_indep *id;
+	u16 status;
+
+	status = nvmet_req_find_ns(req);
+	if (status)
+		goto out;
+
+	id = kzalloc(sizeof(*id), GFP_KERNEL);
+	if (!id) {
+		status = NVME_SC_INTERNAL;
+		goto out;
+	}
+
+	id->nstat = NVME_NSTAT_NRDY;
+	id->anagrpid = req->ns->anagrpid;
+	id->nmic = NVME_NS_NMIC_SHARED;
+	if (req->ns->readonly)
+		id->nsattr |= NVME_NS_ATTR_RO;
+	if (req->ns->bdev && !bdev_nonrot(req->ns->bdev))
+		id->nsfeat |= NVME_NS_ROTATIONAL;
+
+	status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id));
+	kfree(id);
+out:
+	nvmet_req_complete(req, status);
+}
+
 static void nvmet_execute_identify(struct nvmet_req *req)
 {
 	if (!nvmet_check_transfer_len(req, NVME_IDENTIFY_DATA_SIZE))
@@ -729,6 +758,9 @@ static void nvmet_execute_identify(struct nvmet_req *req)
 			break;
 		}
 		break;
+	case NVME_ID_CNS_NS_CS_INDEP:
+		nvmet_execute_id_cs_indep(req);
+		return;
 	}
 
 	pr_debug("unhandled identify cns %d on qid %d\n",
--

On 09-10-2024 00:04, Keith Busch wrote:
> On Tue, Oct 08, 2024 at 09:13:48AM -0600, Keith Busch wrote:
>> I still hope to see nvmet report this. It would be great to test this
>> with HDD backed nvme-loop target.
> 
> I took the liberty to write one up. Looks like everything is reporting
> as expected.
> 
> ---
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index 954d4c0747704..e167c9a2ff995 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -685,6 +685,35 @@ static void nvmet_execute_identify_ctrl_nvm(struct nvmet_req *req)
>   		   nvmet_zero_sgl(req, 0, sizeof(struct nvme_id_ctrl_nvm)));
>   }
>   
> +static void nvmet_execute_id_cs_indep(struct nvmet_req *req)
> +{
> +	struct nvme_id_ns_cs_indep *id;
> +	u16 status;
> +
> +	status = nvmet_req_find_ns(req);
> +	if (status)
> +		goto out;
> +
> +	id = kzalloc(sizeof(*id), GFP_KERNEL);
> +	if (!id) {
> +		status = NVME_SC_INTERNAL;
> +		goto out;
> +	}
> +
> +	id->nstat = NVME_NSTAT_NRDY;
> +	id->anagrpid = req->ns->anagrpid;
> +	id->nmic = NVME_NS_NMIC_SHARED;
> +	if (req->ns->readonly)
> +		id->nsattr |= NVME_NS_ATTR_RO;
> +	if (req->ns->bdev && !bdev_nonrot(req->ns->bdev))
> +		id->nsfeat |= NVME_NS_ROTATIONAL;
> +
> +	status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id));
> +	kfree(id);
> +out:
> +	nvmet_req_complete(req, status);
> +}
> +
>   static void nvmet_execute_identify(struct nvmet_req *req)
>   {
>   	if (!nvmet_check_transfer_len(req, NVME_IDENTIFY_DATA_SIZE))
> @@ -729,6 +758,9 @@ static void nvmet_execute_identify(struct nvmet_req *req)
>   			break;
>   		}
>   		break;
> +	case NVME_ID_CNS_NS_CS_INDEP:
> +		nvmet_execute_id_cs_indep(req);
> +		return;
>   	}
>   
>   	pr_debug("unhandled identify cns %d on qid %d\n",
> --

That was quick! Nice. Would you like me to pack it up in the serie and 
resend?