linux-nvme.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/1] nvme: use macro definitions for setting reservation values
@ 2022-10-02  8:28 Max Gurtovoy
  2022-10-03 10:03 ` Sagi Grimberg
  2022-10-25 23:20 ` Keith Busch
  0 siblings, 2 replies; 8+ messages in thread
From: Max Gurtovoy @ 2022-10-02  8:28 UTC (permalink / raw)
  To: linux-nvme, sagi, hch, kbusch; +Cc: oren, chaitanyak, linux-block, 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



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/1] nvme: use macro definitions for setting reservation values
  2022-10-02  8:28 [PATCH v2 1/1] nvme: use macro definitions for setting reservation values Max Gurtovoy
@ 2022-10-03 10:03 ` Sagi Grimberg
  2022-10-03 10:16   ` Max Gurtovoy
  2022-10-25 23:20 ` Keith Busch
  1 sibling, 1 reply; 8+ messages in thread
From: Sagi Grimberg @ 2022-10-03 10:03 UTC (permalink / raw)
  To: Max Gurtovoy, linux-nvme, hch, kbusch; +Cc: oren, chaitanyak, linux-block


> 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/1] nvme: use macro definitions for setting reservation values
  2022-10-03 10:03 ` Sagi Grimberg
@ 2022-10-03 10:16   ` Max Gurtovoy
  2022-10-25 15:19     ` Christoph Hellwig
  0 siblings, 1 reply; 8+ messages in thread
From: Max Gurtovoy @ 2022-10-03 10:16 UTC (permalink / raw)
  To: Sagi Grimberg, linux-nvme, hch, kbusch; +Cc: oren, chaitanyak, linux-block


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.




^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/1] nvme: use macro definitions for setting reservation values
  2022-10-03 10:16   ` Max Gurtovoy
@ 2022-10-25 15:19     ` Christoph Hellwig
  2022-10-25 23:09       ` Keith Busch
  0 siblings, 1 reply; 8+ messages in thread
From: Christoph Hellwig @ 2022-10-25 15:19 UTC (permalink / raw)
  To: Max Gurtovoy
  Cc: Sagi Grimberg, linux-nvme, hch, kbusch, oren, chaitanyak, linux-block

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.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/1] nvme: use macro definitions for setting reservation values
  2022-10-25 15:19     ` Christoph Hellwig
@ 2022-10-25 23:09       ` Keith Busch
  2022-10-28  8:31         ` Christoph Hellwig
  0 siblings, 1 reply; 8+ messages in thread
From: Keith Busch @ 2022-10-25 23:09 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Max Gurtovoy, Sagi Grimberg, linux-nvme, oren, chaitanyak, linux-block

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.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/1] nvme: use macro definitions for setting reservation values
  2022-10-02  8:28 [PATCH v2 1/1] nvme: use macro definitions for setting reservation values Max Gurtovoy
  2022-10-03 10:03 ` Sagi Grimberg
@ 2022-10-25 23:20 ` Keith Busch
  1 sibling, 0 replies; 8+ messages in thread
From: Keith Busch @ 2022-10-25 23:20 UTC (permalink / raw)
  To: Max Gurtovoy; +Cc: linux-nvme, sagi, hch, oren, chaitanyak, linux-block

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>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/1] nvme: use macro definitions for setting reservation values
  2022-10-25 23:09       ` Keith Busch
@ 2022-10-28  8:31         ` Christoph Hellwig
  2022-10-30  0:35           ` Max Gurtovoy
  0 siblings, 1 reply; 8+ messages in thread
From: Christoph Hellwig @ 2022-10-28  8:31 UTC (permalink / raw)
  To: Keith Busch
  Cc: Christoph Hellwig, Max Gurtovoy, Sagi Grimberg, linux-nvme, oren,
	chaitanyak, linux-block

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.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/1] nvme: use macro definitions for setting reservation values
  2022-10-28  8:31         ` Christoph Hellwig
@ 2022-10-30  0:35           ` Max Gurtovoy
  0 siblings, 0 replies; 8+ messages in thread
From: Max Gurtovoy @ 2022-10-30  0:35 UTC (permalink / raw)
  To: Christoph Hellwig, Keith Busch
  Cc: Sagi Grimberg, linux-nvme, oren, chaitanyak, linux-block


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.



^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-10-30  0:35 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-02  8:28 [PATCH v2 1/1] nvme: use macro definitions for setting reservation values Max Gurtovoy
2022-10-03 10:03 ` Sagi Grimberg
2022-10-03 10:16   ` Max Gurtovoy
2022-10-25 15:19     ` Christoph Hellwig
2022-10-25 23:09       ` Keith Busch
2022-10-28  8:31         ` Christoph Hellwig
2022-10-30  0:35           ` Max Gurtovoy
2022-10-25 23:20 ` Keith Busch

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).