[PATCH v1 for-rc 0/8] vmw_pvrdma fixes for 4.15

[PATCH v1 for-rc 0/8] vmw_pvrdma fixes for 4.15

[Bryan Tan]

Do some cleanup and fixes related to the comments received during SRQ
support upstreaming that applied to existing code. Also add in two
missed macros used by the userlevel library for SRQs, a missed
ib_umem_release, an incorrect usage of the new refcount_t type,
and switched from use of wait queues to completions.

v0 -> v1 changelog:
- Removed use of BIT() in UAPI header
- Make setting/usage of is_kernel consistent between QP/CQ/SRQ
- Use completions instead of wait queues for resource destroy
- Cleaned up commit messages

Bryan Tan (8):
  RDMA/vmw_pvrdma: Clarify QP and CQ is_kernel logic
  RDMA/vmw_pvrdma: Call ib_umem_release on destroy QP path
  RDMA/vmw_pvrdma: Use more specific sizeof in kcalloc
  RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning
  RDMA/vmw_pvrdma: Use refcount_t instead of atomic_t
  RDMA/vmw_pvrdma: Remove usage of BIT() from UAPI header
  RDMA/vmw_pvrdma: Add UAR SRQ macros in ABI header file
  RDMA/vmw_pvrdma: Use completion instead of wait queue

 drivers/infiniband/hw/vmw_pvrdma/pvrdma.h      | 10 +++++-----
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c   | 18 ++++++++---------
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c | 27 ++++++++++++--------------
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c   | 23 ++++++++++++++--------
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c  |  7 ++++---
 include/uapi/rdma/vmw_pvrdma-abi.h             | 12 +++++++-----
 6 files changed, 52 insertions(+), 45 deletions(-)

-- 
1.8.5.6

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 for-rc 1/8] RDMA/vmw_pvrdma: Clarify QP and CQ is_kernel logic

[Bryan Tan]

Be more consistent in setting and checking is_kernel
flag for QPs and CQs.

Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c | 9 ++++-----
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++----
 2 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c
index 3562c0c..ea8db5e6 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c
@@ -132,8 +132,9 @@ struct ib_cq *pvrdma_create_cq(struct ib_device *ibdev,
 	}
 
 	cq->ibcq.cqe = entries;
+	cq->is_kernel = !context;
 
-	if (context) {
+	if (!cq->is_kernel) {
 		if (ib_copy_from_udata(&ucmd, udata, sizeof(ucmd))) {
 			ret = -EFAULT;
 			goto err_cq;
@@ -148,8 +149,6 @@ struct ib_cq *pvrdma_create_cq(struct ib_device *ibdev,
 
 		npages = ib_umem_page_count(cq->umem);
 	} else {
-		cq->is_kernel = true;
-
 		/* One extra page for shared ring state */
 		npages = 1 + (entries * sizeof(struct pvrdma_cqe) +
 			      PAGE_SIZE - 1) / PAGE_SIZE;
@@ -202,7 +201,7 @@ struct ib_cq *pvrdma_create_cq(struct ib_device *ibdev,
 	dev->cq_tbl[cq->cq_handle % dev->dsr->caps.max_cq] = cq;
 	spin_unlock_irqrestore(&dev->cq_tbl_lock, flags);
 
-	if (context) {
+	if (!cq->is_kernel) {
 		cq->uar = &(to_vucontext(context)->uar);
 
 		/* Copy udata back. */
@@ -219,7 +218,7 @@ struct ib_cq *pvrdma_create_cq(struct ib_device *ibdev,
 err_page_dir:
 	pvrdma_page_dir_cleanup(dev, &cq->pdir);
 err_umem:
-	if (context)
+	if (!cq->is_kernel)
 		ib_umem_release(cq->umem);
 err_cq:
 	atomic_dec(&dev->num_cqs);
diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
index 10420a1..b932b7e 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
@@ -249,8 +249,9 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
 		init_waitqueue_head(&qp->wait);
 
 		qp->state = IB_QPS_RESET;
+		qp->is_kernel = !(pd->uobject && udata);
 
-		if (pd->uobject && udata) {
+		if (!qp->is_kernel) {
 			dev_dbg(&dev->pdev->dev,
 				"create queuepair from user space\n");
 
@@ -291,8 +292,6 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
 				qp->npages_recv = 0;
 			qp->npages = qp->npages_send + qp->npages_recv;
 		} else {
-			qp->is_kernel = true;
-
 			ret = pvrdma_set_sq_size(to_vdev(pd->device),
 						 &init_attr->cap, qp);
 			if (ret)
@@ -394,7 +393,7 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
 err_pdir:
 	pvrdma_page_dir_cleanup(dev, &qp->pdir);
 err_umem:
-	if (pd->uobject && udata) {
+	if (!qp->is_kernel) {
 		if (qp->rumem)
 			ib_umem_release(qp->rumem);
 		if (qp->sumem)
-- 
1.8.5.6

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 for-rc 2/8] RDMA/vmw_pvrdma: Call ib_umem_release on destroy QP path

[Bryan Tan]

The QP cleanup did not previously call ib_umem_release. Fix this.

Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver")
Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
index b932b7e..77e7e57 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
@@ -430,6 +430,13 @@ static void pvrdma_free_qp(struct pvrdma_qp *qp)
 	atomic_dec(&qp->refcnt);
 	wait_event(qp->wait, !atomic_read(&qp->refcnt));
 
+	if (!qp->is_kernel) {
+		if (qp->rumem)
+			ib_umem_release(qp->rumem);
+		if (qp->sumem)
+			ib_umem_release(qp->sumem);
+	}
+
 	pvrdma_page_dir_cleanup(dev, &qp->pdir);
 
 	kfree(qp);
-- 
1.8.5.6

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 for-rc 3/8] RDMA/vmw_pvrdma: Use more specific sizeof in kcalloc

[Bryan Tan]

Convert the sizeof(void *) in two kcalloc calls to be more
specific for the arrays that are being allocated.

Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
index 1f4e187..941e324 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
@@ -243,13 +243,13 @@ static int pvrdma_register_device(struct pvrdma_dev *dev)
 	mutex_init(&dev->port_mutex);
 	spin_lock_init(&dev->desc_lock);
 
-	dev->cq_tbl = kcalloc(dev->dsr->caps.max_cq, sizeof(void *),
+	dev->cq_tbl = kcalloc(dev->dsr->caps.max_cq, sizeof(struct pvrdma_cq *),
 			      GFP_KERNEL);
 	if (!dev->cq_tbl)
 		return ret;
 	spin_lock_init(&dev->cq_tbl_lock);
 
-	dev->qp_tbl = kcalloc(dev->dsr->caps.max_qp, sizeof(void *),
+	dev->qp_tbl = kcalloc(dev->dsr->caps.max_qp, sizeof(struct pvrdma_qp *),
 			      GFP_KERNEL);
 	if (!dev->qp_tbl)
 		goto err_cq_free;
-- 
1.8.5.6

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 for-rc 4/8] RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning

[Bryan Tan]

refcount_dec generates a warning when the operation
causes the refcount to hit zero. Avoid this by using
refcount_dec_and_test.

Fixes: 8b10ba783c9d ("RDMA/vmw_pvrdma: Add shared receive queue support")
Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c
index 826ccb8..a2b1a3c 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c
@@ -236,8 +236,8 @@ static void pvrdma_free_srq(struct pvrdma_dev *dev, struct pvrdma_srq *srq)
 	dev->srq_tbl[srq->srq_handle] = NULL;
 	spin_unlock_irqrestore(&dev->srq_tbl_lock, flags);
 
-	refcount_dec(&srq->refcnt);
-	wait_event(srq->wait, !refcount_read(&srq->refcnt));
+	if (!refcount_dec_and_test(&srq->refcnt))
+		wait_event(srq->wait, !refcount_read(&srq->refcnt));
 
 	/* There is no support for kernel clients, so this is safe. */
 	ib_umem_release(srq->umem);
-- 
1.8.5.6

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 for-rc 5/8] RDMA/vmw_pvrdma: Use refcount_t instead of atomic_t

[Bryan Tan]

refcount_t is the preferred type for refcounts. Change the
QP and CQ refcnt fields to use refcount_t.

Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/hw/vmw_pvrdma/pvrdma.h      |  4 ++--
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c   |  6 +++---
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c | 15 ++++++---------
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c   |  6 +++---
 4 files changed, 14 insertions(+), 17 deletions(-)

diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h b/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
index 63bc2ef..07d287e 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
@@ -93,7 +93,7 @@ struct pvrdma_cq {
 	struct pvrdma_page_dir pdir;
 	u32 cq_handle;
 	bool is_kernel;
-	atomic_t refcnt;
+	refcount_t refcnt;
 	wait_queue_head_t wait;
 };
 
@@ -196,7 +196,7 @@ struct pvrdma_qp {
 	u8 state;
 	bool is_kernel;
 	struct mutex mutex; /* QP state mutex. */
-	atomic_t refcnt;
+	refcount_t refcnt;
 	wait_queue_head_t wait;
 };
 
diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c
index ea8db5e6..9dba949 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c
@@ -177,7 +177,7 @@ struct ib_cq *pvrdma_create_cq(struct ib_device *ibdev,
 	else
 		pvrdma_page_dir_insert_umem(&cq->pdir, cq->umem, 0);
 
-	atomic_set(&cq->refcnt, 1);
+	refcount_set(&cq->refcnt, 1);
 	init_waitqueue_head(&cq->wait);
 	spin_lock_init(&cq->cq_lock);
 
@@ -229,8 +229,8 @@ struct ib_cq *pvrdma_create_cq(struct ib_device *ibdev,
 
 static void pvrdma_free_cq(struct pvrdma_dev *dev, struct pvrdma_cq *cq)
 {
-	atomic_dec(&cq->refcnt);
-	wait_event(cq->wait, !atomic_read(&cq->refcnt));
+	if (!refcount_dec_and_test(&cq->refcnt))
+		wait_event(cq->wait, !refcount_read(&cq->refcnt));
 
 	if (!cq->is_kernel)
 		ib_umem_release(cq->umem);
diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
index 941e324..5cff9fa 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
@@ -333,7 +333,7 @@ static void pvrdma_qp_event(struct pvrdma_dev *dev, u32 qpn, int type)
 	spin_lock_irqsave(&dev->qp_tbl_lock, flags);
 	qp = dev->qp_tbl[qpn % dev->dsr->caps.max_qp];
 	if (qp)
-		atomic_inc(&qp->refcnt);
+		refcount_inc(&qp->refcnt);
 	spin_unlock_irqrestore(&dev->qp_tbl_lock, flags);
 
 	if (qp && qp->ibqp.event_handler) {
@@ -346,8 +346,7 @@ static void pvrdma_qp_event(struct pvrdma_dev *dev, u32 qpn, int type)
 		ibqp->event_handler(&e, ibqp->qp_context);
 	}
 	if (qp) {
-		atomic_dec(&qp->refcnt);
-		if (atomic_read(&qp->refcnt) == 0)
+		if (refcount_dec_and_test(&qp->refcnt))
 			wake_up(&qp->wait);
 	}
 }
@@ -360,7 +359,7 @@ static void pvrdma_cq_event(struct pvrdma_dev *dev, u32 cqn, int type)
 	spin_lock_irqsave(&dev->cq_tbl_lock, flags);
 	cq = dev->cq_tbl[cqn % dev->dsr->caps.max_cq];
 	if (cq)
-		atomic_inc(&cq->refcnt);
+		refcount_inc(&cq->refcnt);
 	spin_unlock_irqrestore(&dev->cq_tbl_lock, flags);
 
 	if (cq && cq->ibcq.event_handler) {
@@ -373,8 +372,7 @@ static void pvrdma_cq_event(struct pvrdma_dev *dev, u32 cqn, int type)
 		ibcq->event_handler(&e, ibcq->cq_context);
 	}
 	if (cq) {
-		atomic_dec(&cq->refcnt);
-		if (atomic_read(&cq->refcnt) == 0)
+		if (refcount_dec_and_test(&cq->refcnt))
 			wake_up(&cq->wait);
 	}
 }
@@ -533,14 +531,13 @@ static irqreturn_t pvrdma_intrx_handler(int irq, void *dev_id)
 		spin_lock_irqsave(&dev->cq_tbl_lock, flags);
 		cq = dev->cq_tbl[cqne->info % dev->dsr->caps.max_cq];
 		if (cq)
-			atomic_inc(&cq->refcnt);
+			refcount_inc(&cq->refcnt);
 		spin_unlock_irqrestore(&dev->cq_tbl_lock, flags);
 
 		if (cq && cq->ibcq.comp_handler)
 			cq->ibcq.comp_handler(&cq->ibcq, cq->ibcq.cq_context);
 		if (cq) {
-			atomic_dec(&cq->refcnt);
-			if (atomic_read(&cq->refcnt))
+			if (refcount_dec_and_test(&cq->refcnt))
 				wake_up(&cq->wait);
 		}
 		pvrdma_idx_ring_inc(&ring->cons_head, ring_slots);
diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
index 77e7e57..9745cb1 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
@@ -245,7 +245,7 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
 		spin_lock_init(&qp->sq.lock);
 		spin_lock_init(&qp->rq.lock);
 		mutex_init(&qp->mutex);
-		atomic_set(&qp->refcnt, 1);
+		refcount_set(&qp->refcnt, 1);
 		init_waitqueue_head(&qp->wait);
 
 		qp->state = IB_QPS_RESET;
@@ -427,8 +427,8 @@ static void pvrdma_free_qp(struct pvrdma_qp *qp)
 
 	pvrdma_unlock_cqs(scq, rcq, &scq_flags, &rcq_flags);
 
-	atomic_dec(&qp->refcnt);
-	wait_event(qp->wait, !atomic_read(&qp->refcnt));
+	if (!refcount_dec_and_test(&qp->refcnt))
+		wait_event(qp->wait, !refcount_read(&qp->refcnt));
 
 	if (!qp->is_kernel) {
 		if (qp->rumem)
-- 
1.8.5.6

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 for-rc 6/8] RDMA/vmw_pvrdma: Remove usage of BIT() from UAPI header

[Bryan Tan]

BIT() should not be used in the UAPI header. Remove it.

Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
---
 include/uapi/rdma/vmw_pvrdma-abi.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/uapi/rdma/vmw_pvrdma-abi.h b/include/uapi/rdma/vmw_pvrdma-abi.h
index aaa352f..8d37af5 100644
--- a/include/uapi/rdma/vmw_pvrdma-abi.h
+++ b/include/uapi/rdma/vmw_pvrdma-abi.h
@@ -52,12 +52,12 @@
 #define PVRDMA_UVERBS_ABI_VERSION	3		/* ABI Version. */
 #define PVRDMA_UAR_HANDLE_MASK		0x00FFFFFF	/* Bottom 24 bits. */
 #define PVRDMA_UAR_QP_OFFSET		0		/* QP doorbell. */
-#define PVRDMA_UAR_QP_SEND		BIT(30)		/* Send bit. */
-#define PVRDMA_UAR_QP_RECV		BIT(31)		/* Recv bit. */
+#define PVRDMA_UAR_QP_SEND		(1 << 30)	/* Send bit. */
+#define PVRDMA_UAR_QP_RECV		(1 << 31)	/* Recv bit. */
 #define PVRDMA_UAR_CQ_OFFSET		4		/* CQ doorbell. */
-#define PVRDMA_UAR_CQ_ARM_SOL		BIT(29)		/* Arm solicited bit. */
-#define PVRDMA_UAR_CQ_ARM		BIT(30)		/* Arm bit. */
-#define PVRDMA_UAR_CQ_POLL		BIT(31)		/* Poll bit. */
+#define PVRDMA_UAR_CQ_ARM_SOL		(1 << 29)	/* Arm solicited bit. */
+#define PVRDMA_UAR_CQ_ARM		(1 << 30)	/* Arm bit. */
+#define PVRDMA_UAR_CQ_POLL		(1 << 31)	/* Poll bit. */
 
 enum pvrdma_wr_opcode {
 	PVRDMA_WR_RDMA_WRITE,
-- 
1.8.5.6

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 for-rc 7/8] RDMA/vmw_pvrdma: Add UAR SRQ macros in ABI header file

[Bryan Tan]

Add the two UAR SRQ macros that are required by the userlevel
library.

Fixes: 8b10ba783c9d ("RDMA/vmw_pvrdma: Add shared receive queue support")
Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
---
 include/uapi/rdma/vmw_pvrdma-abi.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/uapi/rdma/vmw_pvrdma-abi.h b/include/uapi/rdma/vmw_pvrdma-abi.h
index 8d37af5..02ca0d0 100644
--- a/include/uapi/rdma/vmw_pvrdma-abi.h
+++ b/include/uapi/rdma/vmw_pvrdma-abi.h
@@ -58,6 +58,8 @@
 #define PVRDMA_UAR_CQ_ARM_SOL		(1 << 29)	/* Arm solicited bit. */
 #define PVRDMA_UAR_CQ_ARM		(1 << 30)	/* Arm bit. */
 #define PVRDMA_UAR_CQ_POLL		(1 << 31)	/* Poll bit. */
+#define PVRDMA_UAR_SRQ_OFFSET		8		/* SRQ doorbell. */
+#define PVRDMA_UAR_SRQ_RECV		(1 << 30)	/* Recv bit. */
 
 enum pvrdma_wr_opcode {
 	PVRDMA_WR_RDMA_WRITE,
-- 
1.8.5.6

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 for-rc 8/8] RDMA/vmw_pvrdma: Use completion instead of wait queue

[Bryan Tan]

The use of wait queues in vmw_pvrdma for handling concurrent
access to a resource leaves a possible use after free bug.
Fix this by using completions instead.

Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver")
Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/hw/vmw_pvrdma/pvrdma.h      | 6 +++---
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c   | 7 ++++---
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c | 8 ++++----
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c   | 7 ++++---
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c  | 7 ++++---
 5 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h b/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
index 07d287e..44cb1cf 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
@@ -94,7 +94,7 @@ struct pvrdma_cq {
 	u32 cq_handle;
 	bool is_kernel;
 	refcount_t refcnt;
-	wait_queue_head_t wait;
+	struct completion free;
 };
 
 struct pvrdma_id_table {
@@ -175,7 +175,7 @@ struct pvrdma_srq {
 	u32 srq_handle;
 	int npages;
 	refcount_t refcnt;
-	wait_queue_head_t wait;
+	struct completion free;
 };
 
 struct pvrdma_qp {
@@ -197,7 +197,7 @@ struct pvrdma_qp {
 	bool is_kernel;
 	struct mutex mutex; /* QP state mutex. */
 	refcount_t refcnt;
-	wait_queue_head_t wait;
+	struct completion free;
 };
 
 struct pvrdma_dev {
diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c
index 9dba949..faa9478 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c
@@ -178,7 +178,7 @@ struct ib_cq *pvrdma_create_cq(struct ib_device *ibdev,
 		pvrdma_page_dir_insert_umem(&cq->pdir, cq->umem, 0);
 
 	refcount_set(&cq->refcnt, 1);
-	init_waitqueue_head(&cq->wait);
+	init_completion(&cq->free);
 	spin_lock_init(&cq->cq_lock);
 
 	memset(cmd, 0, sizeof(*cmd));
@@ -229,8 +229,9 @@ struct ib_cq *pvrdma_create_cq(struct ib_device *ibdev,
 
 static void pvrdma_free_cq(struct pvrdma_dev *dev, struct pvrdma_cq *cq)
 {
-	if (!refcount_dec_and_test(&cq->refcnt))
-		wait_event(cq->wait, !refcount_read(&cq->refcnt));
+	if (refcount_dec_and_test(&cq->refcnt))
+		complete(&cq->free);
+	wait_for_completion(&cq->free);
 
 	if (!cq->is_kernel)
 		ib_umem_release(cq->umem);
diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
index 5cff9fa..939ac2f 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
@@ -347,7 +347,7 @@ static void pvrdma_qp_event(struct pvrdma_dev *dev, u32 qpn, int type)
 	}
 	if (qp) {
 		if (refcount_dec_and_test(&qp->refcnt))
-			wake_up(&qp->wait);
+			complete(&qp->free);
 	}
 }
 
@@ -373,7 +373,7 @@ static void pvrdma_cq_event(struct pvrdma_dev *dev, u32 cqn, int type)
 	}
 	if (cq) {
 		if (refcount_dec_and_test(&cq->refcnt))
-			wake_up(&cq->wait);
+			complete(&cq->free);
 	}
 }
 
@@ -402,7 +402,7 @@ static void pvrdma_srq_event(struct pvrdma_dev *dev, u32 srqn, int type)
 	}
 	if (srq) {
 		if (refcount_dec_and_test(&srq->refcnt))
-			wake_up(&srq->wait);
+			complete(&srq->free);
 	}
 }
 
@@ -538,7 +538,7 @@ static irqreturn_t pvrdma_intrx_handler(int irq, void *dev_id)
 			cq->ibcq.comp_handler(&cq->ibcq, cq->ibcq.cq_context);
 		if (cq) {
 			if (refcount_dec_and_test(&cq->refcnt))
-				wake_up(&cq->wait);
+				complete(&cq->free);
 		}
 		pvrdma_idx_ring_inc(&ring->cons_head, ring_slots);
 	}
diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
index 9745cb1..7bf518b 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
@@ -246,7 +246,7 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
 		spin_lock_init(&qp->rq.lock);
 		mutex_init(&qp->mutex);
 		refcount_set(&qp->refcnt, 1);
-		init_waitqueue_head(&qp->wait);
+		init_completion(&qp->free);
 
 		qp->state = IB_QPS_RESET;
 		qp->is_kernel = !(pd->uobject && udata);
@@ -427,8 +427,9 @@ static void pvrdma_free_qp(struct pvrdma_qp *qp)
 
 	pvrdma_unlock_cqs(scq, rcq, &scq_flags, &rcq_flags);
 
-	if (!refcount_dec_and_test(&qp->refcnt))
-		wait_event(qp->wait, !refcount_read(&qp->refcnt));
+	if (refcount_dec_and_test(&qp->refcnt))
+		complete(&qp->free);
+	wait_for_completion(&qp->free);
 
 	if (!qp->is_kernel) {
 		if (qp->rumem)
diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c
index a2b1a3c..5acebb1 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c
@@ -149,7 +149,7 @@ struct ib_srq *pvrdma_create_srq(struct ib_pd *pd,
 
 	spin_lock_init(&srq->lock);
 	refcount_set(&srq->refcnt, 1);
-	init_waitqueue_head(&srq->wait);
+	init_completion(&srq->free);
 
 	dev_dbg(&dev->pdev->dev,
 		"create shared receive queue from user space\n");
@@ -236,8 +236,9 @@ static void pvrdma_free_srq(struct pvrdma_dev *dev, struct pvrdma_srq *srq)
 	dev->srq_tbl[srq->srq_handle] = NULL;
 	spin_unlock_irqrestore(&dev->srq_tbl_lock, flags);
 
-	if (!refcount_dec_and_test(&srq->refcnt))
-		wait_event(srq->wait, !refcount_read(&srq->refcnt));
+	if (refcount_dec_and_test(&srq->refcnt))
+		complete(&srq->free);
+	wait_for_completion(&srq->free);
 
 	/* There is no support for kernel clients, so this is safe. */
 	ib_umem_release(srq->umem);
-- 
1.8.5.6

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 for-rc 2/8] RDMA/vmw_pvrdma: Call ib_umem_release on destroy QP path

[Yuval Shaia]

On Wed, Dec 13, 2017 at 04:21:33PM -0800, Bryan Tan wrote:
> The QP cleanup did not previously call ib_umem_release. Fix this.
> 
> Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver")
> Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> index b932b7e..77e7e57 100644
> --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> @@ -430,6 +430,13 @@ static void pvrdma_free_qp(struct pvrdma_qp *qp)
>  	atomic_dec(&qp->refcnt);
>  	wait_event(qp->wait, !atomic_read(&qp->refcnt));
>  
> +	if (!qp->is_kernel) {
> +		if (qp->rumem)
> +			ib_umem_release(qp->rumem);
> +		if (qp->sumem)
> +			ib_umem_release(qp->sumem);
> +	}
> +

Reviewed-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>

>  	pvrdma_page_dir_cleanup(dev, &qp->pdir);
>  
>  	kfree(qp);
> -- 
> 1.8.5.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 for-rc 1/8] RDMA/vmw_pvrdma: Clarify QP and CQ is_kernel logic

[Yuval Shaia]

On Wed, Dec 13, 2017 at 04:19:43PM -0800, Bryan Tan wrote:
> Be more consistent in setting and checking is_kernel
> flag for QPs and CQs.
> 
> Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c | 9 ++++-----
>  drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++----
>  2 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c
> index 3562c0c..ea8db5e6 100644
> --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c
> +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c
> @@ -132,8 +132,9 @@ struct ib_cq *pvrdma_create_cq(struct ib_device *ibdev,
>  	}
>  
>  	cq->ibcq.cqe = entries;
> +	cq->is_kernel = !context;
>  
> -	if (context) {
> +	if (!cq->is_kernel) {
>  		if (ib_copy_from_udata(&ucmd, udata, sizeof(ucmd))) {
>  			ret = -EFAULT;
>  			goto err_cq;
> @@ -148,8 +149,6 @@ struct ib_cq *pvrdma_create_cq(struct ib_device *ibdev,
>  
>  		npages = ib_umem_page_count(cq->umem);
>  	} else {
> -		cq->is_kernel = true;
> -
>  		/* One extra page for shared ring state */
>  		npages = 1 + (entries * sizeof(struct pvrdma_cqe) +
>  			      PAGE_SIZE - 1) / PAGE_SIZE;
> @@ -202,7 +201,7 @@ struct ib_cq *pvrdma_create_cq(struct ib_device *ibdev,
>  	dev->cq_tbl[cq->cq_handle % dev->dsr->caps.max_cq] = cq;
>  	spin_unlock_irqrestore(&dev->cq_tbl_lock, flags);
>  
> -	if (context) {
> +	if (!cq->is_kernel) {
>  		cq->uar = &(to_vucontext(context)->uar);
>  
>  		/* Copy udata back. */
> @@ -219,7 +218,7 @@ struct ib_cq *pvrdma_create_cq(struct ib_device *ibdev,
>  err_page_dir:
>  	pvrdma_page_dir_cleanup(dev, &cq->pdir);
>  err_umem:
> -	if (context)
> +	if (!cq->is_kernel)
>  		ib_umem_release(cq->umem);
>  err_cq:
>  	atomic_dec(&dev->num_cqs);
> diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> index 10420a1..b932b7e 100644
> --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> @@ -249,8 +249,9 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
>  		init_waitqueue_head(&qp->wait);
>  
>  		qp->state = IB_QPS_RESET;
> +		qp->is_kernel = !(pd->uobject && udata);
>  
> -		if (pd->uobject && udata) {
> +		if (!qp->is_kernel) {
>  			dev_dbg(&dev->pdev->dev,
>  				"create queuepair from user space\n");
>  
> @@ -291,8 +292,6 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
>  				qp->npages_recv = 0;
>  			qp->npages = qp->npages_send + qp->npages_recv;
>  		} else {
> -			qp->is_kernel = true;
> -
>  			ret = pvrdma_set_sq_size(to_vdev(pd->device),
>  						 &init_attr->cap, qp);
>  			if (ret)
> @@ -394,7 +393,7 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
>  err_pdir:
>  	pvrdma_page_dir_cleanup(dev, &qp->pdir);
>  err_umem:
> -	if (pd->uobject && udata) {
> +	if (!qp->is_kernel) {
>  		if (qp->rumem)
>  			ib_umem_release(qp->rumem);
>  		if (qp->sumem)
> -- 
> 1.8.5.6

Reviewed-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>

> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 for-rc 0/8] vmw_pvrdma fixes for 4.15

[Jason Gunthorpe]

On Wed, Dec 13, 2017 at 04:17:59PM -0800, Bryan Tan wrote:
> 
> Do some cleanup and fixes related to the comments received during SRQ
> support upstreaming that applied to existing code. Also add in two
> missed macros used by the userlevel library for SRQs, a missed
> ib_umem_release, an incorrect usage of the new refcount_t type,
> and switched from use of wait queues to completions.
> 
> v0 -> v1 changelog:
> - Removed use of BIT() in UAPI header
> - Make setting/usage of is_kernel consistent between QP/CQ/SRQ
> - Use completions instead of wait queues for resource destroy
> - Cleaned up commit messages

This series would be fine for -next, the patches look OK to me now.

But there are too many non-rc things to be for-rc.

If you want this in -rc for some reason then you need to send a series
with just those commits, otherwise it will go to -next.

These fix actual bugs and could be for-rc

>   RDMA/vmw_pvrdma: Call ib_umem_release on destroy QP path
>   RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning
>   RDMA/vmw_pvrdma: Use completion instead of wait queue

These are clearly not for-rc:

>   RDMA/vmw_pvrdma: Use more specific sizeof in kcalloc
>   RDMA/vmw_pvrdma: Use refcount_t instead of atomic_t

Does this fix a bug or is it just a style change? Can't tell:

>   RDMA/vmw_pvrdma: Clarify QP and CQ is_kernel logic

These two are boarder line. Would need a better commit message explaining
why these need to be in -rc:

>   RDMA/vmw_pvrdma: Remove usage of BIT() from UAPI header
>   RDMA/vmw_pvrdma: Add UAR SRQ macros in ABI header file

Let me know what you want.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 for-rc 0/8] vmw_pvrdma fixes for 4.15

[Bryan Tan]

On Mon, Dec 18, 2017 at 11:25:11AM -0700, Jason Gunthorpe wrote:
> On Wed, Dec 13, 2017 at 04:17:59PM -0800, Bryan Tan wrote:
> > 
> > Do some cleanup and fixes related to the comments received during SRQ
> > support upstreaming that applied to existing code. Also add in two
> > missed macros used by the userlevel library for SRQs, a missed
> > ib_umem_release, an incorrect usage of the new refcount_t type,
> > and switched from use of wait queues to completions.
> > 
> > v0 -> v1 changelog:
> > - Removed use of BIT() in UAPI header
> > - Make setting/usage of is_kernel consistent between QP/CQ/SRQ
> > - Use completions instead of wait queues for resource destroy
> > - Cleaned up commit messages
> 
> This series would be fine for -next, the patches look OK to me now.
> 
> But there are too many non-rc things to be for-rc.
> 
> If you want this in -rc for some reason then you need to send a series
> with just those commits, otherwise it will go to -next.

I will send out a series with the following commits:

>   RDMA/vmw_pvrdma: Call ib_umem_release on destroy QP path
>   RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning
>   RDMA/vmw_pvrdma: Use completion instead of wait queue
>   RDMA/vmw_pvrdma: Add UAR SRQ macros in ABI header file

And I will clarify the commit message in the last commit there.

Thanks!
Bryan
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 for-rc 0/8] vmw_pvrdma fixes for 4.15

[Jason Gunthorpe]

On Mon, Dec 18, 2017 at 01:42:44PM -0800, Bryan Tan wrote:
> > If you want this in -rc for some reason then you need to send a series
> > with just those commits, otherwise it will go to -next.
> 
> I will send out a series with the following commits:
> 
> >   RDMA/vmw_pvrdma: Call ib_umem_release on destroy QP path
> >   RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning
> >   RDMA/vmw_pvrdma: Use completion instead of wait queue
> >   RDMA/vmw_pvrdma: Add UAR SRQ macros in ABI header file
> 
> And I will clarify the commit message in the last commit there.

Okay, you'll need to rebase the reminaing patches on to the for-rc
patches and send them again.. I would have cherry-picked them
but the refcounter change created conflicts I didn't want to deal
with.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 for-rc 0/8] vmw_pvrdma fixes for 4.15

[Bryan Tan]

On Mon, Dec 18, 2017 at 03:45:19PM -0700, Jason Gunthorpe wrote:
> On Mon, Dec 18, 2017 at 01:42:44PM -0800, Bryan Tan wrote:
> > > If you want this in -rc for some reason then you need to send a series
> > > with just those commits, otherwise it will go to -next.
> > 
> > I will send out a series with the following commits:
> > 
> > >   RDMA/vmw_pvrdma: Call ib_umem_release on destroy QP path
> > >   RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning
> > >   RDMA/vmw_pvrdma: Use completion instead of wait queue
> > >   RDMA/vmw_pvrdma: Add UAR SRQ macros in ABI header file
> > 
> > And I will clarify the commit message in the last commit there.
> 
> Okay, you'll need to rebase the reminaing patches on to the for-rc
> patches and send them again.. I would have cherry-picked them
> but the refcounter change created conflicts I didn't want to deal
> with.

Ah okay. Should I send them together now? I was going to wait until
the for-rc bits make it to for-next before sending out the remaining
patches as they wouldn't apply cleanly to for-next right now.

Bryan
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 for-rc 5/8] RDMA/vmw_pvrdma: Use refcount_t instead of atomic_t

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 5867 bytes --]

On Wed, Dec 13, 2017 at 04:24:12PM -0800, Bryan Tan wrote:
> refcount_t is the preferred type for refcounts. Change the
> QP and CQ refcnt fields to use refcount_t.
>
> Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/infiniband/hw/vmw_pvrdma/pvrdma.h      |  4 ++--
>  drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c   |  6 +++---
>  drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c | 15 ++++++---------
>  drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c   |  6 +++---
>  4 files changed, 14 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h b/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
> index 63bc2ef..07d287e 100644
> --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
> +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
> @@ -93,7 +93,7 @@ struct pvrdma_cq {
>  	struct pvrdma_page_dir pdir;
>  	u32 cq_handle;
>  	bool is_kernel;
> -	atomic_t refcnt;
> +	refcount_t refcnt;
>  	wait_queue_head_t wait;
>  };
>
> @@ -196,7 +196,7 @@ struct pvrdma_qp {
>  	u8 state;
>  	bool is_kernel;
>  	struct mutex mutex; /* QP state mutex. */
> -	atomic_t refcnt;
> +	refcount_t refcnt;
>  	wait_queue_head_t wait;
>  };
>
> diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c
> index ea8db5e6..9dba949 100644
> --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c
> +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c
> @@ -177,7 +177,7 @@ struct ib_cq *pvrdma_create_cq(struct ib_device *ibdev,
>  	else
>  		pvrdma_page_dir_insert_umem(&cq->pdir, cq->umem, 0);
>
> -	atomic_set(&cq->refcnt, 1);
> +	refcount_set(&cq->refcnt, 1);
>  	init_waitqueue_head(&cq->wait);
>  	spin_lock_init(&cq->cq_lock);
>
> @@ -229,8 +229,8 @@ struct ib_cq *pvrdma_create_cq(struct ib_device *ibdev,
>
>  static void pvrdma_free_cq(struct pvrdma_dev *dev, struct pvrdma_cq *cq)
>  {
> -	atomic_dec(&cq->refcnt);
> -	wait_event(cq->wait, !atomic_read(&cq->refcnt));
> +	if (!refcount_dec_and_test(&cq->refcnt))
> +		wait_event(cq->wait, !refcount_read(&cq->refcnt));

Don't you suppose to call to wait_event without condition on refcnt and
sleep till refcnt == 0??

>
>  	if (!cq->is_kernel)
>  		ib_umem_release(cq->umem);
> diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
> index 941e324..5cff9fa 100644
> --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
> +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
> @@ -333,7 +333,7 @@ static void pvrdma_qp_event(struct pvrdma_dev *dev, u32 qpn, int type)
>  	spin_lock_irqsave(&dev->qp_tbl_lock, flags);
>  	qp = dev->qp_tbl[qpn % dev->dsr->caps.max_qp];
>  	if (qp)
> -		atomic_inc(&qp->refcnt);
> +		refcount_inc(&qp->refcnt);
>  	spin_unlock_irqrestore(&dev->qp_tbl_lock, flags);
>
>  	if (qp && qp->ibqp.event_handler) {
> @@ -346,8 +346,7 @@ static void pvrdma_qp_event(struct pvrdma_dev *dev, u32 qpn, int type)
>  		ibqp->event_handler(&e, ibqp->qp_context);
>  	}
>  	if (qp) {
> -		atomic_dec(&qp->refcnt);
> -		if (atomic_read(&qp->refcnt) == 0)
> +		if (refcount_dec_and_test(&qp->refcnt))
>  			wake_up(&qp->wait);
>  	}
>  }
> @@ -360,7 +359,7 @@ static void pvrdma_cq_event(struct pvrdma_dev *dev, u32 cqn, int type)
>  	spin_lock_irqsave(&dev->cq_tbl_lock, flags);
>  	cq = dev->cq_tbl[cqn % dev->dsr->caps.max_cq];
>  	if (cq)
> -		atomic_inc(&cq->refcnt);
> +		refcount_inc(&cq->refcnt);
>  	spin_unlock_irqrestore(&dev->cq_tbl_lock, flags);
>
>  	if (cq && cq->ibcq.event_handler) {
> @@ -373,8 +372,7 @@ static void pvrdma_cq_event(struct pvrdma_dev *dev, u32 cqn, int type)
>  		ibcq->event_handler(&e, ibcq->cq_context);
>  	}
>  	if (cq) {
> -		atomic_dec(&cq->refcnt);
> -		if (atomic_read(&cq->refcnt) == 0)
> +		if (refcount_dec_and_test(&cq->refcnt))
>  			wake_up(&cq->wait);
>  	}
>  }
> @@ -533,14 +531,13 @@ static irqreturn_t pvrdma_intrx_handler(int irq, void *dev_id)
>  		spin_lock_irqsave(&dev->cq_tbl_lock, flags);
>  		cq = dev->cq_tbl[cqne->info % dev->dsr->caps.max_cq];
>  		if (cq)
> -			atomic_inc(&cq->refcnt);
> +			refcount_inc(&cq->refcnt);
>  		spin_unlock_irqrestore(&dev->cq_tbl_lock, flags);
>
>  		if (cq && cq->ibcq.comp_handler)
>  			cq->ibcq.comp_handler(&cq->ibcq, cq->ibcq.cq_context);
>  		if (cq) {
> -			atomic_dec(&cq->refcnt);
> -			if (atomic_read(&cq->refcnt))
> +			if (refcount_dec_and_test(&cq->refcnt))
>  				wake_up(&cq->wait);
>  		}
>  		pvrdma_idx_ring_inc(&ring->cons_head, ring_slots);
> diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> index 77e7e57..9745cb1 100644
> --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> @@ -245,7 +245,7 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
>  		spin_lock_init(&qp->sq.lock);
>  		spin_lock_init(&qp->rq.lock);
>  		mutex_init(&qp->mutex);
> -		atomic_set(&qp->refcnt, 1);
> +		refcount_set(&qp->refcnt, 1);
>  		init_waitqueue_head(&qp->wait);
>
>  		qp->state = IB_QPS_RESET;
> @@ -427,8 +427,8 @@ static void pvrdma_free_qp(struct pvrdma_qp *qp)
>
>  	pvrdma_unlock_cqs(scq, rcq, &scq_flags, &rcq_flags);
>
> -	atomic_dec(&qp->refcnt);
> -	wait_event(qp->wait, !atomic_read(&qp->refcnt));
> +	if (!refcount_dec_and_test(&qp->refcnt))
> +		wait_event(qp->wait, !refcount_read(&qp->refcnt));
>
>  	if (!qp->is_kernel) {
>  		if (qp->rumem)
> --
> 1.8.5.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 for-rc 5/8] RDMA/vmw_pvrdma: Use refcount_t instead of atomic_t

[Bryan Tan]

On Tue, Dec 19, 2017 at 07:32:01AM +0200, Leon Romanovsky wrote:
> On Wed, Dec 13, 2017 at 04:24:12PM -0800, Bryan Tan wrote:
> > @@ -229,8 +229,8 @@ struct ib_cq *pvrdma_create_cq(struct ib_device *ibdev,
> >
> >  static void pvrdma_free_cq(struct pvrdma_dev *dev, struct pvrdma_cq *cq)
> >  {
> > -	atomic_dec(&cq->refcnt);
> > -	wait_event(cq->wait, !atomic_read(&cq->refcnt));
> > +	if (!refcount_dec_and_test(&cq->refcnt))
> > +		wait_event(cq->wait, !refcount_read(&cq->refcnt));
> 
> Don't you suppose to call to wait_event without condition on refcnt and
> sleep till refcnt == 0??

By the time we check this condition, there cannot be any new references
to this CQ. If refcnt is zero, there isn't any reason to call
wait_event.

However, we've changed this to use completions instead of wait queues
because there is a possibility of use after free here still. For SRQs we
use this pattern first to fix the problem of using refcount_dec, and
then a few commits later in the new patch series wait queues are
switched to completions. Please take a look at the v2 patch series.

Thanks!
Bryan
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html