[PATCH v2 0/2] nvme: some cleanup

[PATCH v2 0/2] nvme: some cleanup

[Edmund Nadolski]

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

v2 changes:
 - Move typo fixes into separate patch.
 - Drop struct member comments.
 - Clarify changelog remarks.

Edmund Nadolski (2):
  nvme: nvme_set_queue_count should use descriptive macros
  nvme: fix some typos

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

-- 
2.20.1

[PATCH v2 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 and reflects the field names used in the spec.

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 v2 2/2] nvme: fix some typos

[Edmund Nadolski]

Corrects a few trivial typos.

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

diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 527d64545023..01dca6c18d7b 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -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 9b02c4576591..5b69147a7250 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 v2 1/2] nvme: nvme_set_queue_count should use descriptive macros

[James Smart]

On 4/30/2019 1:09 PM, 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 and reflects the field names used in the spec.
>
> 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);
>   	}
>   

I agree with Christoph. I think it actually hurts understanding.

-- james