[PATCH 0/2] nvme: some cleanup

[PATCH 0/2] nvme: some cleanup

[Edmund Nadolski]

This series clarifies the code by adding some macros, comments,
and fixing a few miscellaneous typos.

Edmund Nadolski (2):
  nvme: nvme_set_queue_count should use descriptive macros
  nvme: add clarifying comments and fix some typos

 drivers/nvme/host/core.c |  8 ++++++--
 drivers/nvme/host/nvme.h | 32 ++++++++++++++++----------------
 drivers/nvme/host/pci.c  |  2 +-
 3 files changed, 23 insertions(+), 19 deletions(-)

-- 
2.20.1

[PATCH 1/2] nvme: nvme_set_queue_count should use descriptive macros

[Edmund Nadolski]

Implement macros to set/get the number of submission and/or completion
queues requested by the Set Features command. This replaces the bit
masking/shifting code which is harder to read and more error prone to
maintain.

Signed-off-by: Edmund Nadolski <ednadols at linux.microsoft.com>
---
 drivers/nvme/host/core.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 3dd043aa6d1f..b3804dbdcc30 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1132,9 +1132,13 @@ static int nvme_set_features(struct nvme_ctrl *dev, unsigned fid, unsigned dword
 	return ret;
 }
 
+#define SET_NUMQ(nsqr, ncqr)	(((nsqr) - 1) | (((ncqr) - 1) << 16))
+#define GET_NSQA(dw)		(((dw) & 0xffff) + 1)
+#define GET_NCQA(dw)		(((dw) >> 16) + 1)
+
 int nvme_set_queue_count(struct nvme_ctrl *ctrl, int *count)
 {
-	u32 q_count = (*count - 1) | ((*count - 1) << 16);
+	u32 q_count = SET_NUMQ(*count, *count);
 	u32 result;
 	int status, nr_io_queues;
 
@@ -1152,7 +1156,7 @@ int nvme_set_queue_count(struct nvme_ctrl *ctrl, int *count)
 		dev_err(ctrl->device, "Could not set queue count (%d)\n", status);
 		*count = 0;
 	} else {
-		nr_io_queues = min(result & 0xffff, result >> 16) + 1;
+		nr_io_queues = min(GET_NSQA(result), GET_NCQA(result));
 		*count = min(*count, nr_io_queues);
 	}
 
-- 
2.20.1

[PATCH 1/2] nvme: nvme_set_queue_count should use descriptive macros

[Sagi Grimberg]

Looks fine,

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

[PATCH 1/2] nvme: nvme_set_queue_count should use descriptive macros

[Christoph Hellwig]

On Mon, Apr 29, 2019@10:35:32AM -0700, Edmund Nadolski wrote:
> Implement macros to set/get the number of submission and/or completion
> queues requested by the Set Features command. This replaces the bit
> masking/shifting code which is harder to read and more error prone to
> maintain.
> 
> Signed-off-by: Edmund Nadolski <ednadols at linux.microsoft.com>

I have to say I actually find this much harder to read than the old
version.

[PATCH 2/2] nvme: add clarifying comments and fix some typos

[Edmund Nadolski]

Add comment descriptions for struct fields per NVMe spec.

Signed-off-by: Edmund Nadolski <ednadols at linux.microsoft.com>
---
 drivers/nvme/host/nvme.h | 32 ++++++++++++++++----------------
 drivers/nvme/host/pci.c  |  2 +-
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 527d64545023..61736d1ef15e 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -177,7 +177,7 @@ struct nvme_ctrl {
 	u16 cntlid;
 
 	u32 ctrl_config;
-	u16 mtfa;
+	u16 mtfa;		/* maximum time for firmware activation */
 	u32 queue_count;
 
 	u64 cap;
@@ -185,23 +185,23 @@ struct nvme_ctrl {
 	u32 max_hw_sectors;
 	u32 max_segments;
 	u16 crdt[3];
-	u16 oncs;
-	u16 oacs;
-	u16 nssa;
+	u16 oncs;		/* optional nvm command support */
+	u16 oacs;		/* optional admin command support */
+	u16 nssa;		/* nvme subsystem streams available */
 	u16 nr_streams;
 	u32 max_namespaces;
 	atomic_t abort_limit;
-	u8 vwc;
-	u32 vs;
-	u32 sgls;
-	u16 kas;
-	u8 npss;
-	u8 apsta;
-	u32 oaes;
+	u8 vwc;			/* volatile write cache */
+	u32 vs;			/* version */
+	u32 sgls;		/* scatter gather list support */
+	u16 kas;		/* keep alive support */
+	u8 npss;		/* number of power states supported */
+	u8 apsta;		/* autonomous power state transition */
+	u32 oaes;		/* optional asynchronous events supported */
 	u32 aen_result;
-	u32 ctratt;
+	u32 ctratt;		/* controller attribute */
 	unsigned int shutdown_timeout;
-	unsigned int kato;
+	unsigned int kato;	/* keep alive timeout */
 	bool subsystem;
 	unsigned long quirks;
 	struct nvme_id_power_state psd[32];
@@ -231,8 +231,8 @@ struct nvme_ctrl {
 	bool apst_enabled;
 
 	/* PCIe only: */
-	u32 hmpre;
-	u32 hmmin;
+	u32 hmpre;	/* host memory buffer preferred size */
+	u32 hmmin;	/* host memory buffer minimum size */
 	u32 hmminds;
 	u16 hmmaxd;
 
@@ -290,7 +290,7 @@ struct nvme_ns_ids {
 /*
  * Anchor structure for namespaces.  There is one for each namespace in a
  * NVMe subsystem that any of our controllers can see, and the namespace
- * structure for each controller is chained of it.  For private namespaces
+ * structure for each controller is chained off it.  For private namespaces
  * there is a 1:1 relation to our namespace structures, that is ->list
  * only ever has a single entry for private namespaces.
  */
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index c1eecde6b853..368371c5cafc 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -227,7 +227,7 @@ struct nvme_iod {
 };
 
 /*
- * Check we didin't inadvertently grow the command struct
+ * Check we didn't inadvertently grow the command struct
  */
 static inline void _nvme_check_size(void)
 {
-- 
2.20.1

[PATCH 2/2] nvme: add clarifying comments and fix some typos

[Sagi Grimberg]

> Add comment descriptions for struct fields per NVMe spec.

Lets split typos from this patch. Not sure that the comments are all
that useful, as we usually have a spec/tp at hand...

[PATCH 2/2] nvme: add clarifying comments and fix some typos

[Edmund Nadolski]

On 4/29/19 4:23 PM, Sagi Grimberg wrote:
> 
>> Add comment descriptions for struct fields per NVMe spec.
> 
> Lets split typos from this patch. Not sure that the comments are all
> that useful, as we usually have a spec/tp at hand...

They could be useful to save a lookup (or when a spec isn't handy). I had
also thought of doing the nvm_id_ctrl and  nvme_id_ns structs, but I'll hold
off as it seems folks see this as extraneous.

I will split out the typos out and re-send.

Thanks,
Ed

[PATCH 2/2] nvme: add clarifying comments and fix some typos

[Minwoo Im]

> -----Original Message-----
> From: Linux-nvme [mailto:linux-nvme-bounces at lists.infradead.org] On
> Behalf Of Edmund Nadolski
> Sent: Tuesday, April 30, 2019 2:36 AM
> To: linux-nvme at lists.infradead.org; ednadols at linux.microsoft.com
> Subject: [PATCH 2/2] nvme: add clarifying comments and fix some typos
> 
> Add comment descriptions for struct fields per NVMe spec.
> 
> Signed-off-by: Edmund Nadolski <ednadols at linux.microsoft.com>

IMHO, if you really want to provide description for structure attributes,
why don't we have all of them instead describing few fields only.

Also It would be better if typo-related commit is separted from it.

Thanks,

[PATCH 2/2] nvme: add clarifying comments and fix some typos

[Chaitanya Kulkarni]

These fields are taken from the spec, so I don't see the need for
adding comments as most of the fields are combination of the bits and 
for that you have to refer to the spec anyways.

On 4/29/19 10:36 AM, Edmund Nadolski wrote:
> Add comment descriptions for struct fields per NVMe spec.
> 
> Signed-off-by: Edmund Nadolski <ednadols at linux.microsoft.com>
> ---
>   drivers/nvme/host/nvme.h | 32 ++++++++++++++++----------------
>   drivers/nvme/host/pci.c  |  2 +-
>   2 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 527d64545023..61736d1ef15e 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -177,7 +177,7 @@ struct nvme_ctrl {
>   	u16 cntlid;
>   
>   	u32 ctrl_config;
> -	u16 mtfa;
> +	u16 mtfa;		/* maximum time for firmware activation */
>   	u32 queue_count;
>   
>   	u64 cap;
> @@ -185,23 +185,23 @@ struct nvme_ctrl {
>   	u32 max_hw_sectors;
>   	u32 max_segments;
>   	u16 crdt[3];
> -	u16 oncs;
> -	u16 oacs;
> -	u16 nssa;
> +	u16 oncs;		/* optional nvm command support */
> +	u16 oacs;		/* optional admin command support */
> +	u16 nssa;		/* nvme subsystem streams available */
>   	u16 nr_streams;
>   	u32 max_namespaces;
>   	atomic_t abort_limit;
> -	u8 vwc;
> -	u32 vs;
> -	u32 sgls;
> -	u16 kas;
> -	u8 npss;
> -	u8 apsta;
> -	u32 oaes;
> +	u8 vwc;			/* volatile write cache */
> +	u32 vs;			/* version */
> +	u32 sgls;		/* scatter gather list support */
> +	u16 kas;		/* keep alive support */
> +	u8 npss;		/* number of power states supported */
> +	u8 apsta;		/* autonomous power state transition */
> +	u32 oaes;		/* optional asynchronous events supported */
>   	u32 aen_result;
> -	u32 ctratt;
> +	u32 ctratt;		/* controller attribute */
>   	unsigned int shutdown_timeout;
> -	unsigned int kato;
> +	unsigned int kato;	/* keep alive timeout */
>   	bool subsystem;
>   	unsigned long quirks;
>   	struct nvme_id_power_state psd[32];
> @@ -231,8 +231,8 @@ struct nvme_ctrl {
>   	bool apst_enabled;
>   
>   	/* PCIe only: */
> -	u32 hmpre;
> -	u32 hmmin;
> +	u32 hmpre;	/* host memory buffer preferred size */
> +	u32 hmmin;	/* host memory buffer minimum size */
>   	u32 hmminds;
>   	u16 hmmaxd;
>   
> @@ -290,7 +290,7 @@ struct nvme_ns_ids {
>   /*
>    * Anchor structure for namespaces.  There is one for each namespace in a
>    * NVMe subsystem that any of our controllers can see, and the namespace
> - * structure for each controller is chained of it.  For private namespaces
> + * structure for each controller is chained off it.  For private namespaces
>    * there is a 1:1 relation to our namespace structures, that is ->list
>    * only ever has a single entry for private namespaces.
>    */
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index c1eecde6b853..368371c5cafc 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -227,7 +227,7 @@ struct nvme_iod {
>   };
>   
>   /*
> - * Check we didin't inadvertently grow the command struct
> + * Check we didn't inadvertently grow the command struct
>    */
>   static inline void _nvme_check_size(void)
>   {
>