| Message ID | 20221002082851.13222-1-mgurtovoy@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/1] nvme: use macro definitions for setting reservation values | expand |
> 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
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.
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.
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.
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>
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.
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.
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;
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(-)