[PATCH 0/2] Cleanup controller configuration register handling

[PATCH 0/2] Cleanup controller configuration register handling

[Damien Le Moal]

A couple of patches to cleanup the definition and handling of the CC
register. No functional changes.

Damien Le Moal (2):
  nvme: Cleanup the definition of the controller config register fields
  nvmet: Use enum definitions instead of hardcoded values

 drivers/nvme/target/nvmet.h | 14 +++++++-------
 include/linux/nvme.h        | 25 ++++++++++++++++++-------
 2 files changed, 25 insertions(+), 14 deletions(-)

-- 
2.48.1

[PATCH 1/2] nvme: Cleanup the definition of the controller config register fields

[Damien Le Moal]

Reorganized the enum used to define the fields of the contrller
configuration (CC) register in include/linux/nvme.h to:
1) Group together all the values defined for each field.
2) Add the missing field masks definitions.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 include/linux/nvme.h | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index fe3b60818fdc..a1cf63144b1b 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -202,24 +202,35 @@ enum {
 enum {
 	NVME_CC_ENABLE		= 1 << 0,
 	NVME_CC_EN_SHIFT	= 0,
+
 	NVME_CC_CSS_SHIFT	= 4,
-	NVME_CC_MPS_SHIFT	= 7,
-	NVME_CC_AMS_SHIFT	= 11,
-	NVME_CC_SHN_SHIFT	= 14,
-	NVME_CC_IOSQES_SHIFT	= 16,
-	NVME_CC_IOCQES_SHIFT	= 20,
+	NVME_CC_CSS_MASK	= 7 << NVME_CC_CSS_SHIFT,
 	NVME_CC_CSS_NVM		= 0 << NVME_CC_CSS_SHIFT,
 	NVME_CC_CSS_CSI		= 6 << NVME_CC_CSS_SHIFT,
-	NVME_CC_CSS_MASK	= 7 << NVME_CC_CSS_SHIFT,
+
+	NVME_CC_MPS_SHIFT	= 7,
+	NVME_CC_MPS_MASK	= 0xf << NVME_CC_MPS_SHIFT,
+
+	NVME_CC_AMS_SHIFT	= 11,
+	NVME_CC_AMS_MASK	= 7 << NVME_CC_AMS_SHIFT,
 	NVME_CC_AMS_RR		= 0 << NVME_CC_AMS_SHIFT,
 	NVME_CC_AMS_WRRU	= 1 << NVME_CC_AMS_SHIFT,
 	NVME_CC_AMS_VS		= 7 << NVME_CC_AMS_SHIFT,
+
+	NVME_CC_SHN_SHIFT	= 14,
+	NVME_CC_SHN_MASK	= 3 << NVME_CC_SHN_SHIFT,
 	NVME_CC_SHN_NONE	= 0 << NVME_CC_SHN_SHIFT,
 	NVME_CC_SHN_NORMAL	= 1 << NVME_CC_SHN_SHIFT,
 	NVME_CC_SHN_ABRUPT	= 2 << NVME_CC_SHN_SHIFT,
-	NVME_CC_SHN_MASK	= 3 << NVME_CC_SHN_SHIFT,
+
+	NVME_CC_IOSQES_SHIFT	= 16,
+	NVME_CC_IOSQES_MASK	= 0xf << NVME_CC_IOSQES_SHIFT,
 	NVME_CC_IOSQES		= NVME_NVM_IOSQES << NVME_CC_IOSQES_SHIFT,
+
+	NVME_CC_IOCQES_SHIFT	= 20,
+	NVME_CC_IOCQES_MASK	= 0xf << NVME_CC_IOCQES_SHIFT,
 	NVME_CC_IOCQES		= NVME_NVM_IOCQES << NVME_CC_IOCQES_SHIFT,
+
 	NVME_CC_CRIME		= 1 << 24,
 };
 
-- 
2.48.1

[PATCH 2/2] nvmet: Use enum definitions instead of hardcoded values

[Damien Le Moal]

Change the definition of the inline functions nvmet_cc_en(),
nvmet_cc_css(), nvmet_cc_mps(), nvmet_cc_ams(), nvmet_cc_shn(),
nvmet_cc_iosqes(), and nvmet_cc_iocqes() to use the enum difinitions in
include/linux/nvme.h instead of hardcoded values.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/nvme/target/nvmet.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 4be8d22d2d8d..d2c1233981e1 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -784,37 +784,37 @@ u16 nvmet_report_invalid_opcode(struct nvmet_req *req);
 
 static inline bool nvmet_cc_en(u32 cc)
 {
-	return (cc >> NVME_CC_EN_SHIFT) & 0x1;
+	return (cc & NVME_CC_ENABLE) >> NVME_CC_EN_SHIFT;
 }
 
 static inline u8 nvmet_cc_css(u32 cc)
 {
-	return (cc >> NVME_CC_CSS_SHIFT) & 0x7;
+	return (cc & NVME_CC_CSS_MASK) >> NVME_CC_CSS_SHIFT;
 }
 
 static inline u8 nvmet_cc_mps(u32 cc)
 {
-	return (cc >> NVME_CC_MPS_SHIFT) & 0xf;
+	return (cc & NVME_CC_MPS_MASK) >> NVME_CC_MPS_SHIFT;
 }
 
 static inline u8 nvmet_cc_ams(u32 cc)
 {
-	return (cc >> NVME_CC_AMS_SHIFT) & 0x7;
+	return (cc & NVME_CC_AMS_MASK) >> NVME_CC_AMS_SHIFT;
 }
 
 static inline u8 nvmet_cc_shn(u32 cc)
 {
-	return (cc >> NVME_CC_SHN_SHIFT) & 0x3;
+	return (cc & NVME_CC_SHN_MASK) >> NVME_CC_SHN_SHIFT;
 }
 
 static inline u8 nvmet_cc_iosqes(u32 cc)
 {
-	return (cc >> NVME_CC_IOSQES_SHIFT) & 0xf;
+	return (cc & NVME_CC_IOSQES_MASK) >> NVME_CC_IOSQES_SHIFT;
 }
 
 static inline u8 nvmet_cc_iocqes(u32 cc)
 {
-	return (cc >> NVME_CC_IOCQES_SHIFT) & 0xf;
+	return (cc & NVME_CC_IOCQES_MASK) >> NVME_CC_IOCQES_SHIFT;
 }
 
 /* Convert a 32-bit number to a 16-bit 0's based number */
-- 
2.48.1

Re: [PATCH 1/2] nvme: Cleanup the definition of the controller config register fields

[Chaitanya Kulkarni]

On 2/9/25 16:29, Damien Le Moal wrote:
> Reorganized the enum used to define the fields of the contrller
> configuration (CC) register in include/linux/nvme.h to:
> 1) Group together all the values defined for each field.
> 2) Add the missing field masks definitions.
>
> Signed-off-by: Damien Le Moal<dlemoal@kernel.org>
> ---


Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH 2/2] nvmet: Use enum definitions instead of hardcoded values

[Chaitanya Kulkarni]

On 2/9/25 16:29, Damien Le Moal wrote:
> Change the definition of the inline functions nvmet_cc_en(),
> nvmet_cc_css(), nvmet_cc_mps(), nvmet_cc_ams(), nvmet_cc_shn(),
> nvmet_cc_iosqes(), and nvmet_cc_iocqes() to use the enum difinitions in
> include/linux/nvme.h instead of hardcoded values.
>
> Signed-off-by: Damien Le Moal<dlemoal@kernel.org>


Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH 1/2] nvme: Cleanup the definition of the controller config register fields

[Christoph Hellwig]

> +++ b/include/linux/nvme.h
> @@ -202,24 +202,35 @@ enum {
>  enum {
>  	NVME_CC_ENABLE		= 1 << 0,
>  	NVME_CC_EN_SHIFT	= 0,
> +
>  	NVME_CC_CSS_SHIFT	= 4,
> -	NVME_CC_MPS_SHIFT	= 7,
> -	NVME_CC_AMS_SHIFT	= 11,
> -	NVME_CC_SHN_SHIFT	= 14,
> -	NVME_CC_IOSQES_SHIFT	= 16,
> -	NVME_CC_IOCQES_SHIFT	= 20,
> +	NVME_CC_CSS_MASK	= 7 << NVME_CC_CSS_SHIFT,

While you're at it, maybe use a separate anonymous enum for each
field and a comment about which field it describes?

Re: [PATCH 2/2] nvmet: Use enum definitions instead of hardcoded values

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 1/2] nvme: Cleanup the definition of the controller config register fields

[Damien Le Moal]

On 2025/02/13 15:00, Christoph Hellwig wrote:
>> +++ b/include/linux/nvme.h
>> @@ -202,24 +202,35 @@ enum {
>>  enum {
>>  	NVME_CC_ENABLE		= 1 << 0,
>>  	NVME_CC_EN_SHIFT	= 0,
>> +
>>  	NVME_CC_CSS_SHIFT	= 4,
>> -	NVME_CC_MPS_SHIFT	= 7,
>> -	NVME_CC_AMS_SHIFT	= 11,
>> -	NVME_CC_SHN_SHIFT	= 14,
>> -	NVME_CC_IOSQES_SHIFT	= 16,
>> -	NVME_CC_IOCQES_SHIFT	= 20,
>> +	NVME_CC_CSS_MASK	= 7 << NVME_CC_CSS_SHIFT,
> 
> While you're at it, maybe use a separate anonymous enum for each
> field and a comment about which field it describes?

I find the single enum for all the fields of the same u32 register to be a nice
grouping of things. But I agree that more comments would indeed be nice.
I can add that. But do we really need separate enums for each field ?


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/2] nvme: Cleanup the definition of the controller config register fields

[Christoph Hellwig]

On Thu, Feb 13, 2025 at 03:07:51PM +0900, Damien Le Moal wrote:
> I find the single enum for all the fields of the same u32 register to be a nice
> grouping of things. But I agree that more comments would indeed be nice.
> I can add that. But do we really need separate enums for each field ?

Ah, not really for each sub-field.  But the nvme code traditionally
also used a single enum for multiple register or struct fields.  If
I read this to quickly and that's not the case here all is fine.

Re: [PATCH 1/2] nvme: Cleanup the definition of the controller config register fields

[Damien Le Moal]

On 2025/02/13 15:23, Christoph Hellwig wrote:
> On Thu, Feb 13, 2025 at 03:07:51PM +0900, Damien Le Moal wrote:
>> I find the single enum for all the fields of the same u32 register to be a nice
>> grouping of things. But I agree that more comments would indeed be nice.
>> I can add that. But do we really need separate enums for each field ?
> 
> Ah, not really for each sub-field.  But the nvme code traditionally
> also used a single enum for multiple register or struct fields.  If
> I read this to quickly and that's not the case here all is fine.

Yep, the enum I changed is only for the CC register. Nothing else in there.
So I will just add comments.


-- 
Damien Le Moal
Western Digital Research