[PATCHv2] nvme: define constants for identification values

[PATCHv2] nvme: define constants for identification values

[Keith Busch]

Improve code readability by defining the specification's constants that
the driver is using when decoding identification payloads.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
v1 -> v2: update multipath.c to use new constants

 drivers/nvme/host/core.c      | 10 +++++-----
 drivers/nvme/host/multipath.c |  5 +++--
 include/linux/nvme.h          |  6 ++++++
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 05aa3525ef06..75d12198833a 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1824,7 +1824,7 @@ static void nvme_update_disk_info(struct gendisk *disk,
 		 * and whether it should be used instead of AWUPF. If NAWUPF ==
 		 * 0 then AWUPF must be used instead.
 		 */
-		if (id->nsfeat & (1 << 1) && id->nawupf)
+		if (id->nsfeat & NVME_NS_FEAT_ATOMICS && id->nawupf)
 			atomic_bs = (1 + le16_to_cpu(id->nawupf)) * bs;
 		else
 			atomic_bs = (1 + ns->ctrl->subsys->awupf) * bs;
@@ -1833,7 +1833,7 @@ static void nvme_update_disk_info(struct gendisk *disk,
 	}
 	phys_bs = bs;
 	io_opt = bs;
-	if (id->nsfeat & (1 << 4)) {
+	if (id->nsfeat & NVME_NS_FEAT_IO_OPT) {
 		/* NPWG = Namespace Preferred Write Granularity */
 		phys_bs *= 1 + le16_to_cpu(id->npwg);
 		/* NOWS = Namespace Optimal Write Size */
@@ -1862,7 +1862,7 @@ static void nvme_update_disk_info(struct gendisk *disk,
 	nvme_config_discard(disk, ns);
 	nvme_config_write_zeroes(disk, ns);
 
-	if (id->nsattr & (1 << 0))
+	if (id->nsattr & NVME_NS_ATTR_RO)
 		set_disk_ro(disk, true);
 	else
 		set_disk_ro(disk, false);
@@ -2655,7 +2655,7 @@ static bool nvme_validate_cntlid(struct nvme_subsystem *subsys,
 			return false;
 		}
 
-		if ((id->cmic & (1 << 1)) ||
+		if ((id->cmic & NVME_CTRL_CMIC_MULTI_CTRL) ||
 		    (ctrl->opts && ctrl->opts->discovery_nqn))
 			continue;
 
@@ -3468,7 +3468,7 @@ static int nvme_init_ns_head(struct nvme_ns *ns, unsigned nsid,
 		struct nvme_id_ns *id)
 {
 	struct nvme_ctrl *ctrl = ns->ctrl;
-	bool is_shared = id->nmic & (1 << 0);
+	bool is_shared = id->nmic & NVME_NS_NMIC_SHARED;
 	struct nvme_ns_head *head = NULL;
 	struct nvme_ns_ids ids;
 	int ret = 0;
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 54603bd3e02d..a4b1fa48937c 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -371,7 +371,7 @@ int nvme_mpath_alloc_disk(struct nvme_ctrl *ctrl, struct nvme_ns_head *head)
 	 * We also do this for private namespaces as the namespace sharing data could
 	 * change after a rescan.
 	 */
-	if (!(ctrl->subsys->cmic & (1 << 1)) || !multipath)
+	if (!(ctrl->subsys->cmic & NVME_CTRL_CMIC_MULTI_CTRL) || !multipath)
 		return 0;
 
 	q = blk_alloc_queue(nvme_ns_head_make_request, ctrl->numa_node);
@@ -687,7 +687,8 @@ int nvme_mpath_init(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
 	int error;
 
 	/* check if multipath is enabled and we have the capability */
-	if (!multipath || !ctrl->subsys || !(ctrl->subsys->cmic & (1 << 3)))
+	if (!multipath || !ctrl->subsys ||
+	    !(ctrl->subsys->cmic & NVME_CTRL_CMIC_ANA))
 		return 0;
 
 	ctrl->anacap = id->anacap;
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 3d5189f46cb1..120f351c6e08 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -299,6 +299,8 @@ struct nvme_id_ctrl {
 };
 
 enum {
+	NVME_CTRL_CMIC_MULTI_CTRL		= 1 << 1,
+	NVME_CTRL_CMIC_ANA			= 1 << 3,
 	NVME_CTRL_ONCS_COMPARE			= 1 << 0,
 	NVME_CTRL_ONCS_WRITE_UNCORRECTABLE	= 1 << 1,
 	NVME_CTRL_ONCS_DSM			= 1 << 2,
@@ -394,8 +396,12 @@ enum {
 
 enum {
 	NVME_NS_FEAT_THIN	= 1 << 0,
+	NVME_NS_FEAT_ATOMICS	= 1 << 1,
+	NVME_NS_FEAT_IO_OPT	= 1 << 4,
+	NVME_NS_ATTR_RO		= 1 << 0,
 	NVME_NS_FLBAS_LBA_MASK	= 0xf,
 	NVME_NS_FLBAS_META_EXT	= 0x10,
+	NVME_NS_NMIC_SHARED	= 1 << 0,
 	NVME_LBAF_RP_BEST	= 0,
 	NVME_LBAF_RP_BETTER	= 1,
 	NVME_LBAF_RP_GOOD	= 2,
-- 
2.24.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCHv2] nvme: define constants for identification values

[Chaitanya Kulkarni]

On 04/03/2020 10:54 AM, Keith Busch wrote:
> Improve code readability by defining the specification's constants that
> the driver is using when decoding identification payloads.
>
> Signed-off-by: Keith Busch<kbusch@kernel.org>

This definitely improves the code readability.

Thanks for doing this Keith.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCHv2] nvme: define constants for identification values

[Christoph Hellwig]

On Fri, Apr 03, 2020 at 10:53:46AM -0700, Keith Busch wrote:
> Improve code readability by defining the specification's constants that
> the driver is using when decoding identification payloads.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>

I can't say I find this an improvement - the bit values allow you
to jump straight to the spec, while the names need a translation
(either to the actual bit or the spec name without underscores and co
first).

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCHv2] nvme: define constants for identification values

[Keith Busch]

On Sat, Apr 04, 2020 at 09:04:37AM +0200, Christoph Hellwig wrote:
> On Fri, Apr 03, 2020 at 10:53:46AM -0700, Keith Busch wrote:
> > Improve code readability by defining the specification's constants that
> > the driver is using when decoding identification payloads.
> > 
> > Signed-off-by: Keith Busch <kbusch@kernel.org>
> 
> I can't say I find this an improvement - the bit values allow you
> to jump straight to the spec, while the names need a translation
> (either to the actual bit or the spec name without underscores and co
> first).

Named constants tell us what they are inline with the code that's
using it. You can make sense of the code just by reading the code,
no additional docs or code comments required.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCHv2] nvme: define constants for identification values

[Christoph Hellwig]

On Sat, Apr 04, 2020 at 11:07:42AM -0600, Keith Busch wrote:
> On Sat, Apr 04, 2020 at 09:04:37AM +0200, Christoph Hellwig wrote:
> > On Fri, Apr 03, 2020 at 10:53:46AM -0700, Keith Busch wrote:
> > > Improve code readability by defining the specification's constants that
> > > the driver is using when decoding identification payloads.
> > > 
> > > Signed-off-by: Keith Busch <kbusch@kernel.org>
> > 
> > I can't say I find this an improvement - the bit values allow you
> > to jump straight to the spec, while the names need a translation
> > (either to the actual bit or the spec name without underscores and co
> > first).
> 
> Named constants tell us what they are inline with the code that's
> using it. You can make sense of the code just by reading the code,
> no additional docs or code comments required.

Sometimes they do, but especially for nvme they often don't.  Anyway,
I'd like to hear from Sagi and/or Jens to see if we have more people
who like this or don't instead of just us two having different
preferences.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCHv2] nvme: define constants for identification values

[Sagi Grimberg]

On 4/7/20 10:25 AM, Christoph Hellwig wrote:
> On Sat, Apr 04, 2020 at 11:07:42AM -0600, Keith Busch wrote:
>> On Sat, Apr 04, 2020 at 09:04:37AM +0200, Christoph Hellwig wrote:
>>> On Fri, Apr 03, 2020 at 10:53:46AM -0700, Keith Busch wrote:
>>>> Improve code readability by defining the specification's constants that
>>>> the driver is using when decoding identification payloads.
>>>>
>>>> Signed-off-by: Keith Busch <kbusch@kernel.org>
>>>
>>> I can't say I find this an improvement - the bit values allow you
>>> to jump straight to the spec, while the names need a translation
>>> (either to the actual bit or the spec name without underscores and co
>>> first).
>>
>> Named constants tell us what they are inline with the code that's
>> using it. You can make sense of the code just by reading the code,
>> no additional docs or code comments required.
> 
> Sometimes they do, but especially for nvme they often don't.  Anyway,
> I'd like to hear from Sagi and/or Jens to see if we have more people
> who like this or don't instead of just us two having different
> preferences.

Personally, I'm in Christoph's camp, especially when the names include
words like CMIC or NMIC etc... These are cryptic unless you are familiar
with the spec and the NVMe language. I often find it easier to have
the bits.

But, I'm also often asked about constants and their meaning, so I'd be
in favor of adding some descriptive names. But if the point here is to
save the code reader a trip to the spec, then we should probably add
some documentation explaining some of the somewhat cryptic acronyms we
have in NVMe.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCHv2] nvme: define constants for identification values

[Keith Busch]

On Tue, Apr 07, 2020 at 10:47:57AM -0700, Sagi Grimberg wrote:
> 
> 
> On 4/7/20 10:25 AM, Christoph Hellwig wrote:
> > On Sat, Apr 04, 2020 at 11:07:42AM -0600, Keith Busch wrote:
> > > On Sat, Apr 04, 2020 at 09:04:37AM +0200, Christoph Hellwig wrote:
> > > > On Fri, Apr 03, 2020 at 10:53:46AM -0700, Keith Busch wrote:
> > > > > Improve code readability by defining the specification's constants that
> > > > > the driver is using when decoding identification payloads.
> > > > > 
> > > > > Signed-off-by: Keith Busch <kbusch@kernel.org>
> > > > 
> > > > I can't say I find this an improvement - the bit values allow you
> > > > to jump straight to the spec, while the names need a translation
> > > > (either to the actual bit or the spec name without underscores and co
> > > > first).
> > > 
> > > Named constants tell us what they are inline with the code that's
> > > using it. You can make sense of the code just by reading the code,
> > > no additional docs or code comments required.
> > 
> > Sometimes they do, but especially for nvme they often don't.  Anyway,
> > I'd like to hear from Sagi and/or Jens to see if we have more people
> > who like this or don't instead of just us two having different
> > preferences.
> 
> Personally, I'm in Christoph's camp, especially when the names include
> words like CMIC or NMIC etc... These are cryptic unless you are familiar
> with the spec and the NVMe language. I often find it easier to have
> the bits.
> 
> But, I'm also often asked about constants and their meaning, so I'd be
> in favor of adding some descriptive names. But if the point here is to
> save the code reader a trip to the spec, then we should probably add
> some documentation explaining some of the somewhat cryptic acronyms we
> have in NVMe.

It's not just saving a trip to the spec. There's a benefit to naming
things when browsing the code: you can cscope named constants easier
grepping patterns to find all occurances.

The driver isn't so big that locating something is too difficult right
now, so I'm not really concerned either way.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCHv2] nvme: define constants for identification values

[Sagi Grimberg]

>>>>>> Improve code readability by defining the specification's constants that
>>>>>> the driver is using when decoding identification payloads.
>>>>>>
>>>>>> Signed-off-by: Keith Busch <kbusch@kernel.org>
>>>>>
>>>>> I can't say I find this an improvement - the bit values allow you
>>>>> to jump straight to the spec, while the names need a translation
>>>>> (either to the actual bit or the spec name without underscores and co
>>>>> first).
>>>>
>>>> Named constants tell us what they are inline with the code that's
>>>> using it. You can make sense of the code just by reading the code,
>>>> no additional docs or code comments required.
>>>
>>> Sometimes they do, but especially for nvme they often don't.  Anyway,
>>> I'd like to hear from Sagi and/or Jens to see if we have more people
>>> who like this or don't instead of just us two having different
>>> preferences.
>>
>> Personally, I'm in Christoph's camp, especially when the names include
>> words like CMIC or NMIC etc... These are cryptic unless you are familiar
>> with the spec and the NVMe language. I often find it easier to have
>> the bits.
>>
>> But, I'm also often asked about constants and their meaning, so I'd be
>> in favor of adding some descriptive names. But if the point here is to
>> save the code reader a trip to the spec, then we should probably add
>> some documentation explaining some of the somewhat cryptic acronyms we
>> have in NVMe.
> 
> It's not just saving a trip to the spec. There's a benefit to naming
> things when browsing the code: you can cscope named constants easier
> grepping patterns to find all occurances.
> 
> The driver isn't so big that locating something is too difficult right
> now, so I'm not really concerned either way.

I'm fine with this change if it makes it more convenient for people.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCHv2] nvme: define constants for identification values

[Bart Van Assche]

On 2020-04-03 10:53, Keith Busch wrote:
> -		if ((id->cmic & (1 << 1)) ||
> +		if ((id->cmic & NVME_CTRL_CMIC_MULTI_CTRL) ||
>  		    (ctrl->opts && ctrl->opts->discovery_nqn))
>  			continue;

If this patch is reposted, please remove the parentheses that are
superfluous from the above expression. Anyway:

Reviewed-by: Bart van Assche <bvanassche@acm.org>

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCHv2] nvme: define constants for identification values

[Keith Busch]

On Thu, Apr 09, 2020 at 07:34:07PM -0700, Bart Van Assche wrote:
> On 2020-04-03 10:53, Keith Busch wrote:
> > -		if ((id->cmic & (1 << 1)) ||
> > +		if ((id->cmic & NVME_CTRL_CMIC_MULTI_CTRL) ||
> >  		    (ctrl->opts && ctrl->opts->discovery_nqn))
> >  			continue;
> 
> If this patch is reposted, please remove the parentheses that are
> superfluous from the above expression.

Indeed, thanks for pointing that out. I'll send v3 if the other
maintainers have been swayed to ack.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCHv2] nvme: define constants for identification values

[Sagi Grimberg]

>> On 2020-04-03 10:53, Keith Busch wrote:
>>> -		if ((id->cmic & (1 << 1)) ||
>>> +		if ((id->cmic & NVME_CTRL_CMIC_MULTI_CTRL) ||
>>>   		    (ctrl->opts && ctrl->opts->discovery_nqn))
>>>   			continue;
>>
>> If this patch is reposted, please remove the parentheses that are
>> superfluous from the above expression.
> 
> Indeed, thanks for pointing that out. I'll send v3 if the other
> maintainers have been swayed to ack.

I'm fine with the changem

Acked-by: Sagi Grimberg <sagi@grimberg.me>

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCHv2] nvme: define constants for identification values

[Christoph Hellwig]

Thanks,

applied to nvme-5.8.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme