diff mbox series

[v3,09/19] nvme: Add pr_ops read_keys support

Message ID 20221026231945.6609-10-michael.christie@oracle.com
State New
Headers show
Series Use block pr_ops in LIO | expand

Commit Message

Mike Christie Oct. 26, 2022, 11:19 p.m. UTC 
This patch adds support for the pr_ops read_keys callout by calling the
NVMe Reservation Report helper, then parsing that info to get the
controller's registered keys. Because the callout is only used in the
kernel where the callers do not know about controller/host IDs, the
callout just returns the registered keys which is required by the SCSI PR
in READ KEYS command.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/nvme/host/pr.c | 73 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/nvme.h   |  4 +++
 2 files changed, 77 insertions(+)

Comments

Christoph Hellwig Oct. 30, 2022, 8:17 a.m. UTC | #1 
On Wed, Oct 26, 2022 at 06:19:35PM -0500, Mike Christie wrote:
> This patch adds support for the pr_ops read_keys callout by calling the
> NVMe Reservation Report helper, then parsing that info to get the
> controller's registered keys. Because the callout is only used in the
> kernel where the callers do not know about controller/host IDs, the
> callout just returns the registered keys which is required by the SCSI PR
> in READ KEYS command.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>  drivers/nvme/host/pr.c | 73 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/nvme.h   |  4 +++
>  2 files changed, 77 insertions(+)
> 
> diff --git a/drivers/nvme/host/pr.c b/drivers/nvme/host/pr.c
> index aa88c55879b2..df7eb2440c67 100644
> --- a/drivers/nvme/host/pr.c
> +++ b/drivers/nvme/host/pr.c
> @@ -118,10 +118,83 @@ static int nvme_pr_release(struct block_device *bdev, u64 key, enum pr_type type
>  	return nvme_pr_command(bdev, cdw10, key, 0, nvme_cmd_resv_release);
>  }
>  
> +static int nvme_pr_resv_report(struct block_device *bdev, u8 *data,
> +		u32 data_len, bool *eds)

Is there any good reason this method seems to take a u8 instead of a void
pointer?  As that seems to make a few things a bit messy.

> +	if (IS_ENABLED(CONFIG_NVME_MULTIPATH) &&
> +	    bdev->bd_disk->fops == &nvme_ns_head_ops)
> +		ret = nvme_send_ns_head_pr_command(bdev, &c, data, data_len);
> +	else
> +		ret = nvme_send_ns_pr_command(bdev->bd_disk->private_data, &c,
> +					      data, data_len);

Can you please add a hlper for this logic?

> +	for (i = 0; i < num_ret_keys; i++) {
> +		if (eds) {
> +			keys_info->keys[i] =
> +					le64_to_cpu(status->regctl_eds[i].rkey);
> +		} else {
> +			keys_info->keys[i] =
> +					le64_to_cpu(status->regctl_ds[i].rkey);
> +		}

If you shorten the status variable name to something like rs this becomes
much easier to follow :)
Mike Christie Oct. 30, 2022, 8:47 p.m. UTC | #2 
On 10/30/22 3:17 AM, Christoph Hellwig wrote:
> On Wed, Oct 26, 2022 at 06:19:35PM -0500, Mike Christie wrote:
>> This patch adds support for the pr_ops read_keys callout by calling the
>> NVMe Reservation Report helper, then parsing that info to get the
>> controller's registered keys. Because the callout is only used in the
>> kernel where the callers do not know about controller/host IDs, the
>> callout just returns the registered keys which is required by the SCSI PR
>> in READ KEYS command.
>>
>> Signed-off-by: Mike Christie <michael.christie@oracle.com>
>> ---
>>  drivers/nvme/host/pr.c | 73 ++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/nvme.h   |  4 +++
>>  2 files changed, 77 insertions(+)
>>
>> diff --git a/drivers/nvme/host/pr.c b/drivers/nvme/host/pr.c
>> index aa88c55879b2..df7eb2440c67 100644
>> --- a/drivers/nvme/host/pr.c
>> +++ b/drivers/nvme/host/pr.c
>> @@ -118,10 +118,83 @@ static int nvme_pr_release(struct block_device *bdev, u64 key, enum pr_type type
>>  	return nvme_pr_command(bdev, cdw10, key, 0, nvme_cmd_resv_release);
>>  }
>>  
>> +static int nvme_pr_resv_report(struct block_device *bdev, u8 *data,
>> +		u32 data_len, bool *eds)
> 
> Is there any good reason this method seems to take a u8 instead of a void
> pointer?  As that seems to make a few things a bit messy.

I did it because the helper functions nvme_send_ns_head_pr_command/
nvme_send_ns_pr_command took a u8.

Looking at it some more I see those functions use nvme_submit_sync_cmd
which then takes a avoid pointer.

To handle your comment about the helper below I'll fix all that up to
take a void pointer.


> 
>> +	if (IS_ENABLED(CONFIG_NVME_MULTIPATH) &&
>> +	    bdev->bd_disk->fops == &nvme_ns_head_ops)
>> +		ret = nvme_send_ns_head_pr_command(bdev, &c, data, data_len);
>> +	else
>> +		ret = nvme_send_ns_pr_command(bdev->bd_disk->private_data, &c,
>> +					      data, data_len);
> 
> Can you please add a hlper for this logic?

Will do.

> 
>> +	for (i = 0; i < num_ret_keys; i++) {
>> +		if (eds) {
>> +			keys_info->keys[i] =
>> +					le64_to_cpu(status->regctl_eds[i].rkey);
>> +		} else {
>> +			keys_info->keys[i] =
>> +					le64_to_cpu(status->regctl_ds[i].rkey);
>> +		}
> 
> If you shorten the status variable name to something like rs this becomes
> much easier to follow :)


Will do.
diff mbox series

Patch

diff --git a/drivers/nvme/host/pr.c b/drivers/nvme/host/pr.c
index aa88c55879b2..df7eb2440c67 100644
--- a/drivers/nvme/host/pr.c
+++ b/drivers/nvme/host/pr.c
@@ -118,10 +118,83 @@  static int nvme_pr_release(struct block_device *bdev, u64 key, enum pr_type type
 	return nvme_pr_command(bdev, cdw10, key, 0, nvme_cmd_resv_release);
 }
 
+static int nvme_pr_resv_report(struct block_device *bdev, u8 *data,
+		u32 data_len, bool *eds)
+{
+	struct nvme_command c = { };
+	int ret;
+
+	c.common.opcode = nvme_cmd_resv_report;
+	c.common.cdw10 = cpu_to_le32(nvme_bytes_to_numd(data_len));
+	c.common.cdw11 = NVME_EXTENDED_DATA_STRUCT;
+	*eds = true;
+
+retry:
+	if (IS_ENABLED(CONFIG_NVME_MULTIPATH) &&
+	    bdev->bd_disk->fops == &nvme_ns_head_ops)
+		ret = nvme_send_ns_head_pr_command(bdev, &c, data, data_len);
+	else
+		ret = nvme_send_ns_pr_command(bdev->bd_disk->private_data, &c,
+					      data, data_len);
+	if (ret == NVME_SC_HOST_ID_INCONSIST &&
+	    c.common.cdw11 == NVME_EXTENDED_DATA_STRUCT) {
+		c.common.cdw11 = 0;
+		*eds = false;
+		goto retry;
+	}
+
+	return ret;
+}
+
+static int nvme_pr_read_keys(struct block_device *bdev,
+		struct pr_keys *keys_info, u32 keys_len)
+{
+	struct nvme_reservation_status *status;
+	u32 data_len, num_ret_keys;
+	int ret, i;
+	bool eds;
+	u8 *data;
+
+	/*
+	 * Assume we are using 128-bit host IDs and allocate a buffer large
+	 * enough to get enough keys to fill the return keys buffer.
+	 */
+	num_ret_keys = keys_len / 8;
+	data_len = sizeof(*status) +
+			num_ret_keys * sizeof(struct nvme_registered_ctrl_ext);
+	data = kzalloc(data_len, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	ret = nvme_pr_resv_report(bdev, data, data_len, &eds);
+	if (ret)
+		goto free_data;
+
+	status = (struct nvme_reservation_status *)data;
+	keys_info->generation = le32_to_cpu(status->gen);
+	keys_info->num_keys = get_unaligned_le16(&status->regctl);
+
+	num_ret_keys = min(num_ret_keys, keys_info->num_keys);
+	for (i = 0; i < num_ret_keys; i++) {
+		if (eds) {
+			keys_info->keys[i] =
+					le64_to_cpu(status->regctl_eds[i].rkey);
+		} else {
+			keys_info->keys[i] =
+					le64_to_cpu(status->regctl_ds[i].rkey);
+		}
+	}
+
+free_data:
+	kfree(data);
+	return ret;
+}
+
 const struct pr_ops nvme_pr_ops = {
 	.pr_register	= nvme_pr_register,
 	.pr_reserve	= nvme_pr_reserve,
 	.pr_release	= nvme_pr_release,
 	.pr_preempt	= nvme_pr_preempt,
 	.pr_clear	= nvme_pr_clear,
+	.pr_read_keys	= nvme_pr_read_keys,
 };
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 3ab141d982d1..5bc9c84dc216 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -757,6 +757,10 @@  enum {
 	NVME_LBART_ATTRIB_HIDE	= 1 << 1,
 };
 
+enum nvme_eds {
+	NVME_EXTENDED_DATA_STRUCT	= 0x1,
+};
+
 struct nvme_registered_ctrl {
 	__le16	cntlid;
 	__u8	rcsts;