[PATCH v2 1/1] nvme: use macro definitions for setting reservation values

[PATCH v2 1/1] nvme: use macro definitions for setting reservation values

[Max Gurtovoy]

This makes the code more readable.

Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
Changes from v1:
 - remove comments (Sagi/Keith)
 - use tabs for constants alignment, similar to 'enum pr_type' (Keith)
---
 drivers/nvme/host/core.c | 12 ++++++------
 include/linux/nvme.h     |  9 +++++++++
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 3f1a7dc2a2a3..50668e1bd9f1 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2068,17 +2068,17 @@ static char nvme_pr_type(enum pr_type type)
 {
 	switch (type) {
 	case PR_WRITE_EXCLUSIVE:
-		return 1;
+		return NVME_PR_WRITE_EXCLUSIVE;
 	case PR_EXCLUSIVE_ACCESS:
-		return 2;
+		return NVME_PR_EXCLUSIVE_ACCESS;
 	case PR_WRITE_EXCLUSIVE_REG_ONLY:
-		return 3;
+		return NVME_PR_WRITE_EXCLUSIVE_REG_ONLY;
 	case PR_EXCLUSIVE_ACCESS_REG_ONLY:
-		return 4;
+		return NVME_PR_EXCLUSIVE_ACCESS_REG_ONLY;
 	case PR_WRITE_EXCLUSIVE_ALL_REGS:
-		return 5;
+		return NVME_PR_WRITE_EXCLUSIVE_ALL_REGS;
 	case PR_EXCLUSIVE_ACCESS_ALL_REGS:
-		return 6;
+		return NVME_PR_EXCLUSIVE_ACCESS_ALL_REGS;
 	default:
 		return 0;
 	}
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index ae53d74f3696..4f777d8621a7 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -238,6 +238,15 @@ enum {
 	NVME_CAP_CRMS_CRIMS	= 1ULL << 60,
 };
 
+enum {
+	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,
+};
+
 struct nvme_id_power_state {
 	__le16			max_power;	/* centiwatts */
 	__u8			rsvd2;
-- 
2.18.1

Re: [PATCH v2 1/1] nvme: use macro definitions for setting reservation values

[Sagi Grimberg]

> This makes the code more readable.
> 
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> ---
> Changes from v1:
>   - remove comments (Sagi/Keith)
>   - use tabs for constants alignment, similar to 'enum pr_type' (Keith)
> ---
>   drivers/nvme/host/core.c | 12 ++++++------
>   include/linux/nvme.h     |  9 +++++++++
>   2 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 3f1a7dc2a2a3..50668e1bd9f1 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2068,17 +2068,17 @@ static char nvme_pr_type(enum pr_type type)
>   {
>   	switch (type) {
>   	case PR_WRITE_EXCLUSIVE:
> -		return 1;
> +		return NVME_PR_WRITE_EXCLUSIVE;
>   	case PR_EXCLUSIVE_ACCESS:
> -		return 2;
> +		return NVME_PR_EXCLUSIVE_ACCESS;
>   	case PR_WRITE_EXCLUSIVE_REG_ONLY:
> -		return 3;
> +		return NVME_PR_WRITE_EXCLUSIVE_REG_ONLY;
>   	case PR_EXCLUSIVE_ACCESS_REG_ONLY:
> -		return 4;
> +		return NVME_PR_EXCLUSIVE_ACCESS_REG_ONLY;
>   	case PR_WRITE_EXCLUSIVE_ALL_REGS:
> -		return 5;
> +		return NVME_PR_WRITE_EXCLUSIVE_ALL_REGS;
>   	case PR_EXCLUSIVE_ACCESS_ALL_REGS:
> -		return 6;
> +		return NVME_PR_EXCLUSIVE_ACCESS_ALL_REGS;
>   	default:
>   		return 0;
>   	}
> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index ae53d74f3696..4f777d8621a7 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -238,6 +238,15 @@ enum {
>   	NVME_CAP_CRMS_CRIMS	= 1ULL << 60,
>   };
>   
> +enum {

I'd make this named nvme_pr_type

Re: [PATCH v2 1/1] nvme: use macro definitions for setting reservation values

[Max Gurtovoy]

On 10/3/2022 1:03 PM, Sagi Grimberg wrote:
>
>> This makes the code more readable.
>>
>> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
>> ---
>> Changes from v1:
>>   - remove comments (Sagi/Keith)
>>   - use tabs for constants alignment, similar to 'enum pr_type' (Keith)
>> ---
>>   drivers/nvme/host/core.c | 12 ++++++------
>>   include/linux/nvme.h     |  9 +++++++++
>>   2 files changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index 3f1a7dc2a2a3..50668e1bd9f1 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -2068,17 +2068,17 @@ static char nvme_pr_type(enum pr_type type)
>>   {
>>       switch (type) {
>>       case PR_WRITE_EXCLUSIVE:
>> -        return 1;
>> +        return NVME_PR_WRITE_EXCLUSIVE;
>>       case PR_EXCLUSIVE_ACCESS:
>> -        return 2;
>> +        return NVME_PR_EXCLUSIVE_ACCESS;
>>       case PR_WRITE_EXCLUSIVE_REG_ONLY:
>> -        return 3;
>> +        return NVME_PR_WRITE_EXCLUSIVE_REG_ONLY;
>>       case PR_EXCLUSIVE_ACCESS_REG_ONLY:
>> -        return 4;
>> +        return NVME_PR_EXCLUSIVE_ACCESS_REG_ONLY;
>>       case PR_WRITE_EXCLUSIVE_ALL_REGS:
>> -        return 5;
>> +        return NVME_PR_WRITE_EXCLUSIVE_ALL_REGS;
>>       case PR_EXCLUSIVE_ACCESS_ALL_REGS:
>> -        return 6;
>> +        return NVME_PR_EXCLUSIVE_ACCESS_ALL_REGS;
>>       default:
>>           return 0;
>>       }
>> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
>> index ae53d74f3696..4f777d8621a7 100644
>> --- a/include/linux/nvme.h
>> +++ b/include/linux/nvme.h
>> @@ -238,6 +238,15 @@ enum {
>>       NVME_CAP_CRMS_CRIMS    = 1ULL << 60,
>>   };
>>   +enum {
>
> I'd make this named nvme_pr_type

Most of the enums of this header are not named.

I don't understand what is the convention here.

Usually, if it's not a new header file I'm trying to keep the file 
convention and this is what I did.

If all agree on named, I'll send another version.

Re: [PATCH v2 1/1] nvme: use macro definitions for setting reservation values

[Christoph Hellwig]

On Mon, Oct 03, 2022 at 01:16:36PM +0300, Max Gurtovoy wrote:
>> I'd make this named nvme_pr_type
>
> Most of the enums of this header are not named.
>
> I don't understand what is the convention here.
>
> Usually, if it's not a new header file I'm trying to keep the file 
> convention and this is what I did.
>
> If all agree on named, I'll send another version.

I think named is better, nvme has some weird unnamed enums that
even combine totally unrelated fields which I always found confusing.

Re: [PATCH v2 1/1] nvme: use macro definitions for setting reservation values

[Keith Busch]

On Tue, Oct 25, 2022 at 05:19:14PM +0200, Christoph Hellwig wrote:
> On Mon, Oct 03, 2022 at 01:16:36PM +0300, Max Gurtovoy wrote:
> >> I'd make this named nvme_pr_type
> >
> > Most of the enums of this header are not named.
> >
> > I don't understand what is the convention here.
> >
> > Usually, if it's not a new header file I'm trying to keep the file 
> > convention and this is what I did.
> >
> > If all agree on named, I'll send another version.
> 
> I think named is better, nvme has some weird unnamed enums that
> even combine totally unrelated fields which I always found confusing.

It's carry-over from the early days when structures had so few constants
that giving each field values their own enum looked a bit silly, and
enums were considered better than a bunch of #define's.

As far as naming them goes, if the only usage is its definition, then
what's the point? If there's a use for a named enum somewhere else, then
yah, that improves readability.

Re: [PATCH v2 1/1] nvme: use macro definitions for setting reservation values

[Christoph Hellwig]

On Tue, Oct 25, 2022 at 05:09:21PM -0600, Keith Busch wrote:
> As far as naming them goes, if the only usage is its definition, then
> what's the point? If there's a use for a named enum somewhere else, then
> yah, that improves readability.

I think it is nice bit of documentation.  E.g. if the enum is for the
bits or value in a nvme filed naming it after that field can be handy.

Re: [PATCH v2 1/1] nvme: use macro definitions for setting reservation values

[Max Gurtovoy]

On 10/28/2022 11:31 AM, Christoph Hellwig wrote:
> On Tue, Oct 25, 2022 at 05:09:21PM -0600, Keith Busch wrote:
>> As far as naming them goes, if the only usage is its definition, then
>> what's the point? If there's a use for a named enum somewhere else, then
>> yah, that improves readability.
> I think it is nice bit of documentation.  E.g. if the enum is for the
> bits or value in a nvme filed naming it after that field can be handy.

I also preferred my V1 with more documentation but was asked to remove it:

+/*
+ * Reservation Type Encoding
+ */
+enum {
+       NVME_PR_WRITE_EXCLUSIVE = 1, /* Write Exclusive Reservation */
+       NVME_PR_EXCLUSIVE_ACCESS = 2, /* Exclusive Access Reservation */
+       NVME_PR_WRITE_EXCLUSIVE_REG_ONLY = 3, /* Write Exclusive - 
Registrants Only Reservation */
+       NVME_PR_EXCLUSIVE_ACCESS_REG_ONLY = 4, /* Exclusive Access - 
Registrants Only Reservation */
+       NVME_PR_WRITE_EXCLUSIVE_ALL_REGS = 5, /* Write Exclusive - All 
Registrants Reservation */
+       NVME_PR_EXCLUSIVE_ACCESS_ALL_REGS = 6, /* Exclusive Access - All 
Registrants Reservation */
+};

regarding the naming of the enum I tend to agree that this is not a must 
here since this is currently the style of this header and since we use 
only its internal definitions.

So I'm ok we both v1 and v2.

We can do some work to improve the confusing enums in nvme.h but we need 
to agree on the strategy before we go implement.

Re: [PATCH v2 1/1] nvme: use macro definitions for setting reservation values

[Keith Busch]

On Sun, Oct 02, 2022 at 11:28:51AM +0300, Max Gurtovoy wrote:
> This makes the code more readable.
> 
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>

Looks good to me.

Reviewed-by: Keith Busch <kbusch@kernel.org>