[PATCH] IB/mthca: comment fix

[PATCH] IB/mthca: comment fix

[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.

Signed-off-by: Michael S. Tsirkin <mst@mellanox.co.il>

diff --git a/drivers/infiniband/hw/mthca/mthca_qp.c b/drivers/infiniband/hw/mthca/mthca_qp.c
index 490fc78..2f3917e 100644
--- a/drivers/infiniband/hw/mthca/mthca_qp.c
+++ b/drivers/infiniband/hw/mthca/mthca_qp.c
@@ -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);
 

-- 
MST

Re: [PATCH] IB/mthca: comment fix

[Arjan van de Ven]

On Mon, 2006-07-10 at 14:14 +0300, Michael S. Tsirkin wrote:
> 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!

Re: [PATCH] IB/mthca: comment fix

[Michael S. Tsirkin]

Quoting r. Arjan van de Ven <arjan@infradead.org>:
> Subject: Re: [PATCH] IB/mthca: comment fix
> 
> On Mon, 2006-07-10 at 14:14 +0300, Michael S. Tsirkin wrote:
> > 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!

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?

-- 
MST

Re: [PATCH] IB/mthca: comment fix

[Arjan van de Ven]

On Mon, 2006-07-10 at 14:31 +0300, Michael S. Tsirkin wrote:
> Quoting r. Arjan van de Ven <arjan@infradead.org>:
> > Subject: Re: [PATCH] IB/mthca: comment fix
> > 
> > On Mon, 2006-07-10 at 14:14 +0300, Michael S. Tsirkin wrote:
> > > 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!
> 
> 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 ;)

Re: [PATCH] IB/mthca: comment fix

[Michael S. Tsirkin]

Quoting r. Arjan van de Ven <arjan@infradead.org>:
> 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 :)

-- 
MST

[PATCH updated] IB/mthca: comment fix

[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 <mst@mellanox.co.il>

diff --git a/drivers/infiniband/hw/mthca/mthca_qp.c b/drivers/infiniband/hw/mthca/mthca_qp.c
index 490fc78..cd8b672 100644
--- a/drivers/infiniband/hw/mthca/mthca_qp.c
+++ b/drivers/infiniband/hw/mthca/mthca_qp.c
@@ -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);
 
-- 
MST

[PATCHv2] IB/mthca: comment fix

[Michael S. Tsirkin]

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 <mst@mellanox.co.il>

diff --git a/drivers/infiniband/hw/mthca/mthca_qp.c b/drivers/infiniband/hw/mthca/mthca_qp.c
index 490fc78..cd8b672 100644
--- a/drivers/infiniband/hw/mthca/mthca_qp.c
+++ b/drivers/infiniband/hw/mthca/mthca_qp.c
@@ -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);
 
-- 
MST

Re: [PATCHv2] IB/mthca: comment fix

[Zach Brown]

Michael S. Tsirkin wrote:
> OK, this is fine with both Arjan van de Ven and Roland Dreier, so -
> Andrew, could you take this into -mm please?

> Signed-off-by: Michael S. Tsirkin <mst@mellanox.co.il>

Acked-by: Zach Brown <zach.brown@oracle.com>