diff mbox series

[v3,10/19] nvme: Move NVMe and Block PR types to an array

Message ID 20221026231945.6609-11-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 
For Reservation Report support we need to also convert from the NVMe spec
PR type back to the block PR definition. This moves us to an array, so in
the next patch we can add another helper to do the conversion without
having to manage 2 switches.

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

Comments

Keith Busch Oct. 27, 2022, 3:18 p.m. UTC | #1 
On Wed, Oct 26, 2022 at 06:19:36PM -0500, Mike Christie wrote:
> For Reservation Report support we need to also convert from the NVMe spec
> PR type back to the block PR definition. This moves us to an array, so in
> the next patch we can add another helper to do the conversion without
> having to manage 2 switches.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>  drivers/nvme/host/pr.c | 42 +++++++++++++++++++++++-------------------
>  include/linux/nvme.h   |  9 +++++++++
>  2 files changed, 32 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/nvme/host/pr.c b/drivers/nvme/host/pr.c
> index df7eb2440c67..5c4611d15d9c 100644
> --- a/drivers/nvme/host/pr.c
> +++ b/drivers/nvme/host/pr.c
> @@ -6,24 +6,28 @@
>  
>  #include "nvme.h"
>  
> -static char nvme_pr_type(enum pr_type type)
> +static const struct {
> +	enum nvme_pr_type	nvme_type;
> +	enum pr_type		blk_type;
> +} nvme_pr_types[] = {
> +	{ NVME_PR_WRITE_EXCLUSIVE, PR_WRITE_EXCLUSIVE },
> +	{ NVME_PR_EXCLUSIVE_ACCESS, PR_EXCLUSIVE_ACCESS },
> +	{ NVME_PR_WRITE_EXCLUSIVE_REG_ONLY, PR_WRITE_EXCLUSIVE_REG_ONLY },
> +	{ NVME_PR_EXCLUSIVE_ACCESS_REG_ONLY, PR_EXCLUSIVE_ACCESS_REG_ONLY },
> +	{ NVME_PR_WRITE_EXCLUSIVE_ALL_REGS, PR_WRITE_EXCLUSIVE_ALL_REGS },
> +	{ NVME_PR_EXCLUSIVE_ACCESS_ALL_REGS, PR_EXCLUSIVE_ACCESS_ALL_REGS },
> +};

Wouldn't it be easier to use the block type as the array index to avoid
the whole looped lookup?

  enum nvme_pr_type types[] = {
	.PR_WRITE_EXCLUSIVE = NVME_PR_WRITE_EXCLUSIVE,
	.PR_EXCLUSIVE_ACCESS  = NVME_PR_EXCLUSIVE_ACCESS,
        ...
  };

?
Mike Christie Oct. 27, 2022, 5:13 p.m. UTC | #2 
On 10/27/22 12:06 PM, Mike Christie wrote:
> On 10/27/22 10:18 AM, Keith Busch wrote:
>> On Wed, Oct 26, 2022 at 06:19:36PM -0500, Mike Christie wrote:
>>> For Reservation Report support we need to also convert from the NVMe spec
>>> PR type back to the block PR definition. This moves us to an array, so in
>>> the next patch we can add another helper to do the conversion without
>>> having to manage 2 switches.
>>>
>>> Signed-off-by: Mike Christie <michael.christie@oracle.com>
>>> ---
>>>  drivers/nvme/host/pr.c | 42 +++++++++++++++++++++++-------------------
>>>  include/linux/nvme.h   |  9 +++++++++
>>>  2 files changed, 32 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/nvme/host/pr.c b/drivers/nvme/host/pr.c
>>> index df7eb2440c67..5c4611d15d9c 100644
>>> --- a/drivers/nvme/host/pr.c
>>> +++ b/drivers/nvme/host/pr.c
>>> @@ -6,24 +6,28 @@
>>>  
>>>  #include "nvme.h"
>>>  
>>> -static char nvme_pr_type(enum pr_type type)
>>> +static const struct {
>>> +	enum nvme_pr_type	nvme_type;
>>> +	enum pr_type		blk_type;
>>> +} nvme_pr_types[] = {
>>> +	{ NVME_PR_WRITE_EXCLUSIVE, PR_WRITE_EXCLUSIVE },
>>> +	{ NVME_PR_EXCLUSIVE_ACCESS, PR_EXCLUSIVE_ACCESS },
>>> +	{ NVME_PR_WRITE_EXCLUSIVE_REG_ONLY, PR_WRITE_EXCLUSIVE_REG_ONLY },
>>> +	{ NVME_PR_EXCLUSIVE_ACCESS_REG_ONLY, PR_EXCLUSIVE_ACCESS_REG_ONLY },
>>> +	{ NVME_PR_WRITE_EXCLUSIVE_ALL_REGS, PR_WRITE_EXCLUSIVE_ALL_REGS },
>>> +	{ NVME_PR_EXCLUSIVE_ACCESS_ALL_REGS, PR_EXCLUSIVE_ACCESS_ALL_REGS },
>>> +};
>>
>> Wouldn't it be easier to use the block type as the array index to avoid
>> the whole looped lookup?
>>
>>   enum nvme_pr_type types[] = {
>> 	.PR_WRITE_EXCLUSIVE = NVME_PR_WRITE_EXCLUSIVE,
>> 	.PR_EXCLUSIVE_ACCESS  = NVME_PR_EXCLUSIVE_ACCESS,
>>         ...
>>   };
> 
> It would be. However,
> 
> 1. I wasn't sure how future proof we wanted it and I might have
> misinterpreted Chaitanya's original review comment. The part of
> the comment about handling "every new nvme_type" made me think that
> we were worried there would be new types in the future. So I thought
> we wanted it to be really generic and be able to handle cases where
> the values could be funky like -1 in the future.
> 
> 2. I also need to go from NVME_PR type to PR type, so we need a
> second array. So we can either have 2 arrays or 1 array and 2
> loops (the next patch in this set added the second loop).
> If we don't care about #1 then I can I see 2 arrays is nicer.

Oh wait there was also a

3. The pr_types come from userspace so if it passes us 10
and we just do:

types[pr_type]

then we would crash due an out of bounds error.

Similarly I thought there could be a bad target that does the
same thing.
Mike Christie Oct. 28, 2022, 4:05 p.m. UTC | #3 
On 10/27/22 12:16 PM, Keith Busch wrote:
> On Thu, Oct 27, 2022 at 12:13:06PM -0500, michael.christie@oracle.com wrote:
>> Oh wait there was also a
>>
>> 3. The pr_types come from userspace so if it passes us 10
>> and we just do:
>>
>> types[pr_type]
>>
>> then we would crash due an out of bounds error.
>>
>> Similarly I thought there could be a bad target that does the
>> same thing.
> 
> Well, you'd of course have to check the boundaries before accessing if
> you were to implement this scheme. :)

Yeah :) Sorry I didn't write that reply well. I was more trying to say
that we would still need a wrapper function to check the bounds, so either
we have 2 arrays and 2 helper functions or 1 array and 2 helper functions.

I'll see what other people's opinions are and just do what you guys end up
preferring.
diff mbox series

Patch

diff --git a/drivers/nvme/host/pr.c b/drivers/nvme/host/pr.c
index df7eb2440c67..5c4611d15d9c 100644
--- a/drivers/nvme/host/pr.c
+++ b/drivers/nvme/host/pr.c
@@ -6,24 +6,28 @@ 
 
 #include "nvme.h"
 
-static char nvme_pr_type(enum pr_type type)
+static const struct {
+	enum nvme_pr_type	nvme_type;
+	enum pr_type		blk_type;
+} nvme_pr_types[] = {
+	{ NVME_PR_WRITE_EXCLUSIVE, PR_WRITE_EXCLUSIVE },
+	{ NVME_PR_EXCLUSIVE_ACCESS, PR_EXCLUSIVE_ACCESS },
+	{ NVME_PR_WRITE_EXCLUSIVE_REG_ONLY, PR_WRITE_EXCLUSIVE_REG_ONLY },
+	{ NVME_PR_EXCLUSIVE_ACCESS_REG_ONLY, PR_EXCLUSIVE_ACCESS_REG_ONLY },
+	{ NVME_PR_WRITE_EXCLUSIVE_ALL_REGS, PR_WRITE_EXCLUSIVE_ALL_REGS },
+	{ NVME_PR_EXCLUSIVE_ACCESS_ALL_REGS, PR_EXCLUSIVE_ACCESS_ALL_REGS },
+};
+
+static enum nvme_pr_type nvme_pr_type_from_blk(enum pr_type type)
 {
-	switch (type) {
-	case PR_WRITE_EXCLUSIVE:
-		return 1;
-	case PR_EXCLUSIVE_ACCESS:
-		return 2;
-	case PR_WRITE_EXCLUSIVE_REG_ONLY:
-		return 3;
-	case PR_EXCLUSIVE_ACCESS_REG_ONLY:
-		return 4;
-	case PR_WRITE_EXCLUSIVE_ALL_REGS:
-		return 5;
-	case PR_EXCLUSIVE_ACCESS_ALL_REGS:
-		return 6;
-	default:
-		return 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(nvme_pr_types); i++) {
+		if (nvme_pr_types[i].blk_type == type)
+			return nvme_pr_types[i].nvme_type;
 	}
+
+	return 0;
 }
 
 static int nvme_send_ns_head_pr_command(struct block_device *bdev,
@@ -91,7 +95,7 @@  static int nvme_pr_reserve(struct block_device *bdev, u64 key,
 	if (flags & ~PR_FL_IGNORE_KEY)
 		return -EOPNOTSUPP;
 
-	cdw10 = nvme_pr_type(type) << 8;
+	cdw10 = nvme_pr_type_from_blk(type) << 8;
 	cdw10 |= ((flags & PR_FL_IGNORE_KEY) ? 1 << 3 : 0);
 	return nvme_pr_command(bdev, cdw10, key, 0, nvme_cmd_resv_acquire);
 }
@@ -99,7 +103,7 @@  static int nvme_pr_reserve(struct block_device *bdev, u64 key,
 static int nvme_pr_preempt(struct block_device *bdev, u64 old, u64 new,
 		enum pr_type type, bool abort)
 {
-	u32 cdw10 = nvme_pr_type(type) << 8 | (abort ? 2 : 1);
+	u32 cdw10 = nvme_pr_type_from_blk(type) << 8 | (abort ? 2 : 1);
 
 	return nvme_pr_command(bdev, cdw10, old, new, nvme_cmd_resv_acquire);
 }
@@ -113,7 +117,7 @@  static int nvme_pr_clear(struct block_device *bdev, u64 key)
 
 static int nvme_pr_release(struct block_device *bdev, u64 key, enum pr_type type)
 {
-	u32 cdw10 = nvme_pr_type(type) << 8 | (key ? 0 : 1 << 3);
+	u32 cdw10 = nvme_pr_type_from_blk(type) << 8 | (key ? 0 : 1 << 3);
 
 	return nvme_pr_command(bdev, cdw10, key, 0, nvme_cmd_resv_release);
 }
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 5bc9c84dc216..d0bd15f527fc 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -757,6 +757,15 @@  enum {
 	NVME_LBART_ATTRIB_HIDE	= 1 << 1,
 };
 
+enum nvme_pr_type {
+	NVME_PR_WRITE_EXCLUSIVE			= 1,
+	NVME_PR_EXCLUSIVE_ACCESS		= 2,
+	NVME_PR_WRITE_EXCLUSIVE_REG_ONLY	= 3,
+	NVME_PR_EXCLUSIVE_ACCESS_REG_ONLY	= 4,
+	NVME_PR_WRITE_EXCLUSIVE_ALL_REGS	= 5,
+	NVME_PR_EXCLUSIVE_ACCESS_ALL_REGS	= 6,
+};
+
 enum nvme_eds {
 	NVME_EXTENDED_DATA_STRUCT	= 0x1,
 };