Skip to contentSkip to conversation

IB/mthca: comment fix

PATCH In review Michael S. Tsirkin 3 participants 8 messages Jul 10, 2006

Subsystems (3)
Networking Network drivers InfiniBand
lore
Share & tools
lore

Michael S. Tsirkin

author
#0 lore raw

PATCH 1/1 IB/mthca: comment fix

Hi Andrew,
Here's a cosmetic patch for IB/mthca. Pls drop it into -mm and on.

---

comment in mthca_qp.c makes it seem lockdep is the only reason WQ locks should be initialized separately, but as Zach Brown and Roland pointed out, there are other reasons, e.g. that mthca_wq_init is called from modify qp as well.

Signed-off-by Michael S. Tsirkin <[email protected]>
drivers/infiniband/hw/mthca/mthca_qp.c +1 -1
index 490fc78..2f3917e 100644
@@ -1114,7 +1114,7 @@ static int mthca_alloc_qp_common(struct 
 	qp->sq_policy    = send_policy;
 	mthca_wq_init(&qp->sq);
 	mthca_wq_init(&qp->rq);
-	/* these are initialized separately so lockdep can tell them apart */
+
 	spin_lock_init(&qp->sq.lock);
 	spin_lock_init(&qp->rq.lock);
Signature · 1 line
MST
#1 lore raw
Quoted 8 lines · Michael S. Tsirkin

Hi Andrew,
Here's a cosmetic patch for IB/mthca. Pls drop it into -mm and on.

---

comment in mthca_qp.c makes it seem lockdep is the only reason WQ locks should be initialized separately, but as Zach Brown and Roland pointed out, there are other reasons, e.g. that mthca_wq_init is called from modify qp as well.

ehh.. shouldn't the comment say that instead then? that's one tricky thing and might as well have that documented in the code!

author
#2 lore raw

Quoting r. Arjan van de Ven <[email protected]>:

Quoted 6 lines

Subject: Re: [PATCH] IB/mthca: comment fix

Quoted 3 lines · Michael S. Tsirkin

Hi Andrew,
Here's a cosmetic patch for IB/mthca. Pls drop it into -mm and on.

Quoted 8 lines
Quoted 5 lines

---

comment in mthca_qp.c makes it seem lockdep is the only reason WQ locks should be initialized separately, but as Zach Brown and Roland pointed out, there are other reasons, e.g. that mthca_wq_init is called from modify qp as well.

ehh.. shouldn't the comment say that instead then? that's one tricky thing and might as well have that documented in the code!

Hmm. Okay. Maybe we should rename mthca_wq_init to mthca_wq_reset? This would make it clear that it does not init the spinlocks, but just resets the rest of the fields, would not it?

How does this sound?

Signature · 1 line
MST
#3 lore raw
Quoted 13 lines · Michael S. Tsirkin

Quoting r. Arjan van de Ven <[email protected]>:

Quoted 12 lines

Subject: Re: [PATCH] IB/mthca: comment fix

Quoted 8 lines · Michael S. Tsirkin

Hi Andrew,
Here's a cosmetic patch for IB/mthca. Pls drop it into -mm and on.

---

comment in mthca_qp.c makes it seem lockdep is the only reason WQ locks should be initialized separately, but as Zach Brown and Roland pointed out, there are other reasons, e.g. that mthca_wq_init is called from modify qp as well.

Quoted 6 lines
Quoted 2 lines

ehh.. shouldn't the comment say that instead then? that's one tricky thing and might as well have that documented in the code!

Hmm. Okay. Maybe we should rename mthca_wq_init to mthca_wq_reset? This would make it clear that it does not init the spinlocks, but just resets the rest of the fields, would not it?

makes sense to me; my main concern is that we document the bug that was there; unless you document such things.. these bugs tend to have a habit of resurfacing later ;)

author
#4 lore raw

Quoting r. Arjan van de Ven <[email protected]>:

Quoted 3 lines

makes sense to me; my main concern is that we document the bug that was there; unless you document such things.. these bugs tend to have a habit of resurfacing later ;)

Right. Although lockdep will catch this one quickly :)

Signature · 1 line
MST
author
#5 lore raw

PATCH 1/1 IB/mthca: comment fix

In reply to Michael S. Tsirkin

OK, the following makes it clear what the function does, from its name. More comments?

---

After recent changes, mthca_wq_init does not actually initialize the WQ as it used to - it simply resets all index fields to their initial values. So, let's rename it to mthca_wq_reset.

Signed-off-by Michael S. Tsirkin <[email protected]>
drivers/infiniband/hw/mthca/mthca_qp.c +6 -7
index 490fc78..cd8b672 100644
@@ -222,9 +222,8 @@ static void *get_send_wqe(struct mthca_q
 			 (PAGE_SIZE - 1));
 }
 
-static void mthca_wq_init(struct mthca_wq *wq)
+static void mthca_wq_reset(struct mthca_wq *wq)
 {
-	/* mthca_alloc_qp_common() initializes the locks */
 	wq->next_ind  = 0;
 	wq->last_comp = wq->max - 1;
 	wq->head      = 0;

@@ -845,10 +844,10 @@ int mthca_modify_qp(struct ib_qp *ibqp, 
 			mthca_cq_clean(dev, to_mcq(qp->ibqp.recv_cq), qp->qpn,
 				       qp->ibqp.srq ? to_msrq(qp->ibqp.srq) : NULL);
 
-		mthca_wq_init(&qp->sq);
+		mthca_wq_reset(&qp->sq);
 		qp->sq.last = get_send_wqe(qp, qp->sq.max - 1);
 
-		mthca_wq_init(&qp->rq);
+		mthca_wq_reset(&qp->rq);
 		qp->rq.last = get_recv_wqe(qp, qp->rq.max - 1);
 
 		if (mthca_is_memfree(dev)) {

@@ -1112,9 +1111,9 @@ static int mthca_alloc_qp_common(struct 
 	qp->atomic_rd_en = 0;
 	qp->resp_depth   = 0;
 	qp->sq_policy    = send_policy;
-	mthca_wq_init(&qp->sq);
-	mthca_wq_init(&qp->rq);
-	/* these are initialized separately so lockdep can tell them apart */
+	mthca_wq_reset(&qp->sq);
+	mthca_wq_reset(&qp->rq);
+
 	spin_lock_init(&qp->sq.lock);
 	spin_lock_init(&qp->rq.lock);
Signature · 1 line
MST
author
#6 lore raw

PATCH v2 1/1 IB/mthca: comment fix

OK, this is fine with both Arjan van de Ven and Roland Dreier, so - Andrew, could you take this into -mm please?

---

After recent changes, mthca_wq_init does not actually initialize the WQ as it used to - it simply resets all index fields to their initial values. So, let's rename it to mthca_wq_reset.

Signed-off-by Michael S. Tsirkin <[email protected]>
drivers/infiniband/hw/mthca/mthca_qp.c +6 -7
index 490fc78..cd8b672 100644
@@ -222,9 +222,8 @@ static void *get_send_wqe(struct mthca_q
 			 (PAGE_SIZE - 1));
 }
 
-static void mthca_wq_init(struct mthca_wq *wq)
+static void mthca_wq_reset(struct mthca_wq *wq)
 {
-	/* mthca_alloc_qp_common() initializes the locks */
 	wq->next_ind  = 0;
 	wq->last_comp = wq->max - 1;
 	wq->head      = 0;

@@ -845,10 +844,10 @@ int mthca_modify_qp(struct ib_qp *ibqp, 
 			mthca_cq_clean(dev, to_mcq(qp->ibqp.recv_cq), qp->qpn,
 				       qp->ibqp.srq ? to_msrq(qp->ibqp.srq) : NULL);
 
-		mthca_wq_init(&qp->sq);
+		mthca_wq_reset(&qp->sq);
 		qp->sq.last = get_send_wqe(qp, qp->sq.max - 1);
 
-		mthca_wq_init(&qp->rq);
+		mthca_wq_reset(&qp->rq);
 		qp->rq.last = get_recv_wqe(qp, qp->rq.max - 1);
 
 		if (mthca_is_memfree(dev)) {

@@ -1112,9 +1111,9 @@ static int mthca_alloc_qp_common(struct 
 	qp->atomic_rd_en = 0;
 	qp->resp_depth   = 0;
 	qp->sq_policy    = send_policy;
-	mthca_wq_init(&qp->sq);
-	mthca_wq_init(&qp->rq);
-	/* these are initialized separately so lockdep can tell them apart */
+	mthca_wq_reset(&qp->sq);
+	mthca_wq_reset(&qp->rq);
+
 	spin_lock_init(&qp->sq.lock);
 	spin_lock_init(&qp->rq.lock);
Signature · 1 line
MST