diff mbox series

[for-next,2/9] RDMA/hns: Add CQ flag instead of independent enable flag

Message ID 1589982799-28728-3-git-send-email-liweihang@huawei.com (mailing list archive)
State Mainlined
Commit 05e6a5a63579d4c55cc996e5148bd6da9ed48860
Delegated to: Jason Gunthorpe
Headers show
Series RDMA/hns: Cleanups for 5.8 | expand

Commit Message

Weihang Li May 20, 2020, 1:53 p.m. UTC 
From: Lang Cheng <chenglang@huawei.com>

It's easier to understand and maintain enable flags of cq using a single
field in type of u32 than defining a field for every flags in the structure
hns_roce_cq, and we can add new flags for features more conveniently in the
future.

Signed-off-by: Lang Cheng <chenglang@huawei.com>
Signed-off-by: Weihang Li <liweihang@huawei.com>
---
 drivers/infiniband/hw/hns/hns_roce_cq.c     | 10 +++++-----
 drivers/infiniband/hw/hns/hns_roce_device.h |  6 +++---
 drivers/infiniband/hw/hns/hns_roce_hw_v2.c  |  6 +++---
 3 files changed, 11 insertions(+), 11 deletions(-)

Comments

Jason Gunthorpe May 25, 2020, 5:06 p.m. UTC | #1 
On Wed, May 20, 2020 at 09:53:12PM +0800, Weihang Li wrote:
> +	roce_set_bit(cq_context->byte_44_db_record,
> +		     V2_CQC_BYTE_44_DB_RECORD_EN_S,
> +		     (hr_cq->flags & HNS_ROCE_CQ_FLAG_RECORD_DB) ? 1 : 0);

It seems like the if expression should be inside the roce_set_bit
macro (just cast to bool) as something called 'bit' should have that
safety built in.

Also, if someone wants a project, all this endless stuff should be
using genmask and field_prep instead of this home grown stuff.

Jason
Weihang Li May 26, 2020, 2:57 a.m. UTC | #2 
On 2020/5/26 1:06, Jason Gunthorpe wrote:
> On Wed, May 20, 2020 at 09:53:12PM +0800, Weihang Li wrote:
>> +	roce_set_bit(cq_context->byte_44_db_record,
>> +		     V2_CQC_BYTE_44_DB_RECORD_EN_S,
>> +		     (hr_cq->flags & HNS_ROCE_CQ_FLAG_RECORD_DB) ? 1 : 0);
> 
> It seems like the if expression should be inside the roce_set_bit
> macro (just cast to bool) as something called 'bit' should have that
> safety built in.
> 

Hi Jason

Thanks for your comments, will prepare a patch to modify it.


> Also, if someone wants a project, all this endless stuff should be
> using genmask and field_prep instead of this home grown stuff.
> 
> Jason
> 

I took a look at this macro, FILED_PREP() can indeed simplify lots of
similar codes in the hns driver. I will take a try and maybe prepare a
patch/series to use it in v5.9.

Weihang
Jason Gunthorpe May 26, 2020, 12:08 p.m. UTC | #3 
On Tue, May 26, 2020 at 02:57:39AM +0000, liweihang wrote:
> > Also, if someone wants a project, all this endless stuff should be
> > using genmask and field_prep instead of this home grown stuff.
> > 
> I took a look at this macro, FILED_PREP() can indeed simplify lots of
> similar codes in the hns driver. I will take a try and maybe prepare a
> patch/series to use it in v5.9.

If you look in the git history you can find some Coccinelle spatch
stuff that makes work like this not too hard

eg 91b60a7128d96244794beb9b324eb39273872da2 did something similar for
the IBA CM MADs

Jason
Weihang Li May 28, 2020, 1:15 a.m. UTC | #4 
On 2020/5/26 20:08, Jason Gunthorpe wrote:
> On Tue, May 26, 2020 at 02:57:39AM +0000, liweihang wrote:
>>> Also, if someone wants a project, all this endless stuff should be
>>> using genmask and field_prep instead of this home grown stuff.
>>>
>> I took a look at this macro, FILED_PREP() can indeed simplify lots of
>> similar codes in the hns driver. I will take a try and maybe prepare a
>> patch/series to use it in v5.9.
> 
> If you look in the git history you can find some Coccinelle spatch
> stuff that makes work like this not too hard
> 
> eg 91b60a7128d96244794beb9b324eb39273872da2 did something similar for
> the IBA CM MADs
> 
> Jason
> 

I see, thanks for the commit-id.

Weihang
diff mbox series

Patch

diff --git a/drivers/infiniband/hw/hns/hns_roce_cq.c b/drivers/infiniband/hw/hns/hns_roce_cq.c
index d2d7074..925fb77 100644
--- a/drivers/infiniband/hw/hns/hns_roce_cq.c
+++ b/drivers/infiniband/hw/hns/hns_roce_cq.c
@@ -186,8 +186,8 @@  static int alloc_cq_db(struct hns_roce_dev *hr_dev, struct hns_roce_cq *hr_cq,
 						   &hr_cq->db);
 			if (err)
 				return err;
-			hr_cq->db_en = 1;
-			resp->cap_flags |= HNS_ROCE_SUPPORT_CQ_RECORD_DB;
+			hr_cq->flags |= HNS_ROCE_CQ_FLAG_RECORD_DB;
+			resp->cap_flags |= HNS_ROCE_CQ_FLAG_RECORD_DB;
 		}
 	} else {
 		if (has_db) {
@@ -196,7 +196,7 @@  static int alloc_cq_db(struct hns_roce_dev *hr_dev, struct hns_roce_cq *hr_cq,
 				return err;
 			hr_cq->set_ci_db = hr_cq->db.db_record;
 			*hr_cq->set_ci_db = 0;
-			hr_cq->db_en = 1;
+			hr_cq->flags |= HNS_ROCE_CQ_FLAG_RECORD_DB;
 		}
 		hr_cq->cq_db_l = hr_dev->reg_base + hr_dev->odb_offset +
 				 DB_REG_OFFSET * hr_dev->priv_uar.index;
@@ -210,10 +210,10 @@  static void free_cq_db(struct hns_roce_dev *hr_dev, struct hns_roce_cq *hr_cq,
 {
 	struct hns_roce_ucontext *uctx;
 
-	if (!hr_cq->db_en)
+	if (!(hr_cq->flags & HNS_ROCE_CQ_FLAG_RECORD_DB))
 		return;
 
-	hr_cq->db_en = 0;
+	hr_cq->flags &= ~HNS_ROCE_CQ_FLAG_RECORD_DB;
 	if (udata) {
 		uctx = rdma_udata_to_drv_context(udata,
 						 struct hns_roce_ucontext,
diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h
index 4fcd608e..06bafa1 100644
--- a/drivers/infiniband/hw/hns/hns_roce_device.h
+++ b/drivers/infiniband/hw/hns/hns_roce_device.h
@@ -135,8 +135,8 @@  enum {
 	HNS_ROCE_QP_CAP_SQ_RECORD_DB = BIT(1),
 };
 
-enum {
-	HNS_ROCE_SUPPORT_CQ_RECORD_DB = 1 << 0,
+enum hns_roce_cq_flags {
+	HNS_ROCE_CQ_FLAG_RECORD_DB = BIT(0),
 };
 
 enum hns_roce_qp_state {
@@ -454,7 +454,7 @@  struct hns_roce_cq {
 	struct ib_cq			ib_cq;
 	struct hns_roce_mtr		mtr;
 	struct hns_roce_db		db;
-	u8				db_en;
+	u32				flags;
 	spinlock_t			lock;
 	u32				cq_depth;
 	u32				cons_index;
diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
index 7d0556ef..36a9871 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
+++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
@@ -2905,9 +2905,9 @@  static void hns_roce_v2_write_cqc(struct hns_roce_dev *hr_dev,
 	roce_set_field(cq_context->byte_40_cqe_ba, V2_CQC_BYTE_40_CQE_BA_M,
 		       V2_CQC_BYTE_40_CQE_BA_S, (dma_handle >> (32 + 3)));
 
-	if (hr_cq->db_en)
-		roce_set_bit(cq_context->byte_44_db_record,
-			     V2_CQC_BYTE_44_DB_RECORD_EN_S, 1);
+	roce_set_bit(cq_context->byte_44_db_record,
+		     V2_CQC_BYTE_44_DB_RECORD_EN_S,
+		     (hr_cq->flags & HNS_ROCE_CQ_FLAG_RECORD_DB) ? 1 : 0);
 
 	roce_set_field(cq_context->byte_44_db_record,
 		       V2_CQC_BYTE_44_DB_RECORD_ADDR_M,