[PATCH 0/3] IB/vmw_pvrdma patches for 4.10

[PATCH 0/3] IB/vmw_pvrdma patches for 4.10

[Adit Ranadive]

Hi Doug,

These are a small set of patches for the VMware paravirtual rdma driver.
I hope these can be added to the set of fixes for 4.10. I based these on
Linus' tree but they apply cleanly on your k.o/for-4.10-topics/vmw_pvrdma
branch too.

Thanks,
Adit

Adit Ranadive (3):
  IB/vmw_pvrdma: Defer activating device until vmxnet3 link is up
  IB/vmw_pvrdma: Cleanup unused variables
  IB/vmw_pvrdma: Dont hardcode QP header page

 drivers/infiniband/hw/vmw_pvrdma/pvrdma.h       |   4 +
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c  | 103 +++++++++++++++---------
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c    |  42 +++++-----
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.c |   2 +-
 4 files changed, 89 insertions(+), 62 deletions(-)

-- 
2.7.4

--
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 1/3] IB/vmw_pvrdma: Defer activating device until vmxnet3 link is up

[Adit Ranadive]

From: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>

Currently on bootup, ethernet drivers seem to load before RDMA ones
in Linux. So while the vmxnet3 module is loaded before vmw_pvrdma,
the vmxnet3 link state may not necessarily be enabled by the stack
before pvrdma is loaded. This is a problem because if the pvrdma
module is loaded on bootup (by installing it in /lib/modules/*),
the pvrdma device comes up in a port down state.

Since this is the most common use case scenario, defer the activation
of the device till the paired vmxnet3 link actually comes up. One
downside of doing this is, if a user doesn't have the vmxnet3 link
up when the pvrdma driver is loaded, they may not see any output
for ibv_devinfo until the paired vmxnet3 link is enabled too. The
users somehow need to be aware of this.

This only changes how the device is activated the first time. Once
enabled if the link goes down, a pvrdma driver reload is still required
after link up.

Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver")
Signed-off-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/hw/vmw_pvrdma/pvrdma.h      |  1 +
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c | 99 +++++++++++++++++---------
 2 files changed, 65 insertions(+), 35 deletions(-)

diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h b/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
index 71e1d55..540a54b 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
@@ -221,6 +221,7 @@ struct pvrdma_dev {
 	u32 port_cap_mask;
 	struct mutex port_mutex; /* Port modification mutex. */
 	bool ib_active;
+	bool enabled;
 	atomic_t num_qps;
 	atomic_t num_cqs;
 	atomic_t num_pds;
diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
index 231a1ce..b57132f 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
@@ -74,6 +74,8 @@ static int pvrdma_del_gid(struct ib_device *ibdev,
 			  void **context);
 
 
+static int pvrdma_enable_dev(struct pvrdma_dev *dev);
+
 static ssize_t show_hca(struct device *device, struct device_attribute *attr,
 			char *buf)
 {
@@ -755,6 +757,10 @@ static void pvrdma_netdevice_event_handle(struct pvrdma_dev *dev,
 		pvrdma_dispatch_event(dev, 1, IB_EVENT_PORT_ERR);
 		break;
 	case NETDEV_UP:
+		if (!dev->enabled && pvrdma_enable_dev(dev)) {
+			dev_err(&dev->pdev->dev, "failed to enable device\n");
+			break;
+		}
 		pvrdma_dispatch_event(dev, 1, IB_EVENT_PORT_ACTIVE);
 		break;
 	default:
@@ -801,6 +807,48 @@ static int pvrdma_netdevice_event(struct notifier_block *this,
 	return NOTIFY_DONE;
 }
 
+static void pvrdma_disable_dev(struct pvrdma_dev *dev)
+{
+	if (dev->enabled) {
+		ib_unregister_device(&dev->ib_dev);
+		dev->enabled = false;
+	}
+}
+
+static int pvrdma_enable_dev(struct pvrdma_dev *dev)
+{
+	int ret;
+	pvrdma_enable_intrs(dev);
+
+	/* Activate pvrdma device */
+	pvrdma_write_reg(dev, PVRDMA_REG_CTL, PVRDMA_DEVICE_CTL_ACTIVATE);
+
+	/* Make sure the write is complete before reading status. */
+	mb();
+
+	/* Check if device was successfully activated */
+	ret = pvrdma_read_reg(dev, PVRDMA_REG_ERR);
+	if (ret) {
+		dev_err(&dev->pdev->dev, "failed to activate device\n");
+		ret = -EFAULT;
+		goto err_disable_intrs;
+	}
+
+	/* Register IB device */
+	ret = pvrdma_register_device(dev);
+	if (ret) {
+		dev_err(&dev->pdev->dev, "failed to register IB device\n");
+		goto err_disable_intrs;
+	}
+
+	dev->enabled = true;
+	return 0;
+
+err_disable_intrs:
+	pvrdma_disable_intrs(dev);
+	return ret;
+}
+
 static int pvrdma_pci_probe(struct pci_dev *pdev,
 			    const struct pci_device_id *id)
 {
@@ -867,14 +915,14 @@ static int pvrdma_pci_probe(struct pci_dev *pdev,
 	/* Enable 64-Bit DMA */
 	if (pci_set_dma_mask(pdev, DMA_BIT_MASK(64)) == 0) {
 		ret = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(64));
-		if (ret != 0) {
+		if (ret) {
 			dev_err(&pdev->dev,
 				"pci_set_consistent_dma_mask failed\n");
 			goto err_free_resource;
 		}
 	} else {
 		ret = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
-		if (ret != 0) {
+		if (ret) {
 			dev_err(&pdev->dev,
 				"pci_set_dma_mask failed\n");
 			goto err_free_resource;
@@ -1029,7 +1077,7 @@ static int pvrdma_pci_probe(struct pci_dev *pdev,
 	if (ret) {
 		dev_err(&pdev->dev, "failed to allocate interrupts\n");
 		ret = -ENOMEM;
-		goto err_netdevice;
+		goto err_free_cq_ring;
 	}
 
 	/* Allocate UAR table. */
@@ -1049,51 +1097,35 @@ static int pvrdma_pci_probe(struct pci_dev *pdev,
 	}
 	dev_dbg(&pdev->dev, "gid table len %d\n", dev->dsr->caps.gid_tbl_len);
 
-	pvrdma_enable_intrs(dev);
-
-	/* Activate pvrdma device */
-	pvrdma_write_reg(dev, PVRDMA_REG_CTL, PVRDMA_DEVICE_CTL_ACTIVATE);
-
-	/* Make sure the write is complete before reading status. */
-	mb();
-
-	/* Check if device was successfully activated */
-	ret = pvrdma_read_reg(dev, PVRDMA_REG_ERR);
-	if (ret != 0) {
-		dev_err(&pdev->dev, "failed to activate device\n");
-		ret = -EFAULT;
-		goto err_disable_intr;
-	}
-
-	/* Register IB device */
-	ret = pvrdma_register_device(dev);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to register IB device\n");
-		goto err_disable_intr;
+	if (netif_running(dev->netdev) && netif_carrier_ok(dev->netdev)) {
+		ret = pvrdma_enable_dev(dev);
+		if (ret) {
+			dev_err(&pdev->dev, "failed to enable device\n");
+			goto err_free_sgid_tbl;
+		}
+	} else {
+		dev_info(&pdev->dev, "pvrdma netdev link is down\n");
 	}
 
 	dev->nb_netdev.notifier_call = pvrdma_netdevice_event;
 	ret = register_netdevice_notifier(&dev->nb_netdev);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to register netdevice events\n");
-		goto err_unreg_ibdev;
+		goto err_disable_dev;
 	}
 
 	dev_info(&pdev->dev, "attached to device\n");
 	return 0;
 
-err_unreg_ibdev:
-	ib_unregister_device(&dev->ib_dev);
-err_disable_intr:
-	pvrdma_disable_intrs(dev);
+err_disable_dev:
+	pvrdma_disable_dev(dev);
+err_free_sgid_tbl:
 	kfree(dev->sgid_tbl);
 err_free_uar_table:
 	pvrdma_uar_table_cleanup(dev);
 err_free_intrs:
 	pvrdma_free_irq(dev);
 	pvrdma_disable_msi_all(dev);
-err_netdevice:
-	unregister_netdevice_notifier(&dev->nb_netdev);
 err_free_cq_ring:
 	pvrdma_page_dir_cleanup(dev, &dev->cq_pdir);
 err_free_async_ring:
@@ -1132,10 +1164,7 @@ static void pvrdma_pci_remove(struct pci_dev *pdev)
 	unregister_netdevice_notifier(&dev->nb_netdev);
 	dev->nb_netdev.notifier_call = NULL;
 
-	flush_workqueue(event_wq);
-
-	/* Unregister ib device */
-	ib_unregister_device(&dev->ib_dev);
+	pvrdma_disable_dev(dev);
 
 	mutex_lock(&pvrdma_device_list_lock);
 	list_del(&dev->device_link);
-- 
2.7.4

--
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 2/3] IB/vmw_pvrdma: Cleanup unused variables

[Adit Ranadive]

Removed the unused nreq and redundant index variables.
Moved hardcoded async and cq ring pages number to macro.

Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver")
Reported-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/hw/vmw_pvrdma/pvrdma.h      |  2 ++
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c |  4 ++--
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c   | 33 ++++++++++----------------
 3 files changed, 17 insertions(+), 22 deletions(-)

diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h b/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
index 540a54b..ee6a941 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
@@ -69,6 +69,8 @@
  */
 #define PCI_DEVICE_ID_VMWARE_PVRDMA	0x0820
 
+#define PVRDMA_NUM_RING_PAGES		4
+
 struct pvrdma_dev;
 
 struct pvrdma_page_dir {
diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
index b57132f..8e21a86 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
@@ -1001,7 +1001,7 @@ static int pvrdma_pci_probe(struct pci_dev *pdev,
 	dev->dsr->resp_slot_dma = (u64)slot_dma;
 
 	/* Async event ring */
-	dev->dsr->async_ring_pages.num_pages = 4;
+	dev->dsr->async_ring_pages.num_pages = PVRDMA_NUM_RING_PAGES;
 	ret = pvrdma_page_dir_init(dev, &dev->async_pdir,
 				   dev->dsr->async_ring_pages.num_pages, true);
 	if (ret)
@@ -1010,7 +1010,7 @@ static int pvrdma_pci_probe(struct pci_dev *pdev,
 	dev->dsr->async_ring_pages.pdir_dma = dev->async_pdir.dir_dma;
 
 	/* CQ notification ring */
-	dev->dsr->cq_ring_pages.num_pages = 4;
+	dev->dsr->cq_ring_pages.num_pages = PVRDMA_NUM_RING_PAGES;
 	ret = pvrdma_page_dir_init(dev, &dev->cq_pdir,
 				   dev->dsr->cq_ring_pages.num_pages, true);
 	if (ret)
diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
index c8c01e5..765bd32 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
@@ -555,13 +555,13 @@ int pvrdma_modify_qp(struct ib_qp *ibqp, struct ib_qp_attr *attr,
 	return ret;
 }
 
-static inline void *get_sq_wqe(struct pvrdma_qp *qp, int n)
+static inline void *get_sq_wqe(struct pvrdma_qp *qp, unsigned int n)
 {
 	return pvrdma_page_dir_get_ptr(&qp->pdir,
 				       qp->sq.offset + n * qp->sq.wqe_size);
 }
 
-static inline void *get_rq_wqe(struct pvrdma_qp *qp, int n)
+static inline void *get_rq_wqe(struct pvrdma_qp *qp, unsigned int n)
 {
 	return pvrdma_page_dir_get_ptr(&qp->pdir,
 				       qp->rq.offset + n * qp->rq.wqe_size);
@@ -599,9 +599,7 @@ int pvrdma_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
 	unsigned long flags;
 	struct pvrdma_sq_wqe_hdr *wqe_hdr;
 	struct pvrdma_sge *sge;
-	int i, index;
-	int nreq;
-	int ret;
+	int i, ret;
 
 	/*
 	 * In states lower than RTS, we can fail immediately. In other states,
@@ -614,9 +612,8 @@ int pvrdma_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
 
 	spin_lock_irqsave(&qp->sq.lock, flags);
 
-	index = pvrdma_idx(&qp->sq.ring->prod_tail, qp->sq.wqe_cnt);
-	for (nreq = 0; wr; nreq++, wr = wr->next) {
-		unsigned int tail;
+	while (wr) {
+		unsigned int tail = 0;
 
 		if (unlikely(!pvrdma_idx_ring_has_space(
 				qp->sq.ring, qp->sq.wqe_cnt, &tail))) {
@@ -681,7 +678,7 @@ int pvrdma_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
 			}
 		}
 
-		wqe_hdr = (struct pvrdma_sq_wqe_hdr *)get_sq_wqe(qp, index);
+		wqe_hdr = (struct pvrdma_sq_wqe_hdr *)get_sq_wqe(qp, tail);
 		memset(wqe_hdr, 0, sizeof(*wqe_hdr));
 		wqe_hdr->wr_id = wr->wr_id;
 		wqe_hdr->num_sge = wr->num_sge;
@@ -772,12 +769,11 @@ int pvrdma_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
 		/* Make sure wqe is written before index update */
 		smp_wmb();
 
-		index++;
-		if (unlikely(index >= qp->sq.wqe_cnt))
-			index = 0;
 		/* Update shared sq ring */
 		pvrdma_idx_ring_inc(&qp->sq.ring->prod_tail,
 				    qp->sq.wqe_cnt);
+
+		wr = wr->next;
 	}
 
 	ret = 0;
@@ -807,7 +803,6 @@ int pvrdma_post_recv(struct ib_qp *ibqp, struct ib_recv_wr *wr,
 	struct pvrdma_qp *qp = to_vqp(ibqp);
 	struct pvrdma_rq_wqe_hdr *wqe_hdr;
 	struct pvrdma_sge *sge;
-	int index, nreq;
 	int ret = 0;
 	int i;
 
@@ -822,9 +817,8 @@ int pvrdma_post_recv(struct ib_qp *ibqp, struct ib_recv_wr *wr,
 
 	spin_lock_irqsave(&qp->rq.lock, flags);
 
-	index = pvrdma_idx(&qp->rq.ring->prod_tail, qp->rq.wqe_cnt);
-	for (nreq = 0; wr; nreq++, wr = wr->next) {
-		unsigned int tail;
+	while (wr) {
+		unsigned int tail = 0;
 
 		if (unlikely(wr->num_sge > qp->rq.max_sg ||
 			     wr->num_sge < 0)) {
@@ -844,7 +838,7 @@ int pvrdma_post_recv(struct ib_qp *ibqp, struct ib_recv_wr *wr,
 			goto out;
 		}
 
-		wqe_hdr = (struct pvrdma_rq_wqe_hdr *)get_rq_wqe(qp, index);
+		wqe_hdr = (struct pvrdma_rq_wqe_hdr *)get_rq_wqe(qp, tail);
 		wqe_hdr->wr_id = wr->wr_id;
 		wqe_hdr->num_sge = wr->num_sge;
 		wqe_hdr->total_len = 0;
@@ -860,12 +854,11 @@ int pvrdma_post_recv(struct ib_qp *ibqp, struct ib_recv_wr *wr,
 		/* Make sure wqe is written before index update */
 		smp_wmb();
 
-		index++;
-		if (unlikely(index >= qp->rq.wqe_cnt))
-			index = 0;
 		/* Update shared rq ring */
 		pvrdma_idx_ring_inc(&qp->rq.ring->prod_tail,
 				    qp->rq.wqe_cnt);
+
+		wr = wr->next;
 	}
 
 	spin_unlock_irqrestore(&qp->rq.lock, flags);
-- 
2.7.4

--
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 3/3] IB/vmw_pvrdma: Dont hardcode QP header page

[Adit Ranadive]

Moved the header page count to a macro [1]. In a future patch we will
separate out the allocation for the header page. Thanks Yuval.
Also, clear out the alloc_ucontext user response [2]. Thanks Dan.

[1] - http://marc.info/?l=linux-rdma&m=148101146228847&w=2
[2] - http://marc.info/?l=linux-rdma&m=148351229926333&w=2

Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver")
Reported-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Reported-by: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/hw/vmw_pvrdma/pvrdma.h       | 1 +
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c    | 9 +++++----
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.c | 2 +-
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h b/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
index ee6a941..5dada5a 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
@@ -70,6 +70,7 @@
 #define PCI_DEVICE_ID_VMWARE_PVRDMA	0x0820
 
 #define PVRDMA_NUM_RING_PAGES		4
+#define PVRDMA_QP_NUM_HEADER_PAGES	1
 
 struct pvrdma_dev;
 
diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
index 765bd32..3e23425 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
@@ -170,8 +170,9 @@ static int pvrdma_set_sq_size(struct pvrdma_dev *dev, struct ib_qp_cap *req_cap,
 					     sizeof(struct pvrdma_sge) *
 					     qp->sq.max_sg);
 	/* Note: one extra page for the header. */
-	qp->npages_send = 1 + (qp->sq.wqe_cnt * qp->sq.wqe_size +
-			       PAGE_SIZE - 1) / PAGE_SIZE;
+	qp->npages_send = PVRDMA_QP_NUM_HEADER_PAGES +
+			  (qp->sq.wqe_cnt * qp->sq.wqe_size + PAGE_SIZE - 1) /
+								PAGE_SIZE;
 
 	return 0;
 }
@@ -289,7 +290,7 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
 			qp->npages = qp->npages_send + qp->npages_recv;
 
 			/* Skip header page. */
-			qp->sq.offset = PAGE_SIZE;
+			qp->sq.offset = PVRDMA_QP_NUM_HEADER_PAGES * PAGE_SIZE;
 
 			/* Recv queue pages are after send pages. */
 			qp->rq.offset = qp->npages_send * PAGE_SIZE;
@@ -342,7 +343,7 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
 	cmd->qp_type = ib_qp_type_to_pvrdma(init_attr->qp_type);
 	cmd->access_flags = IB_ACCESS_LOCAL_WRITE;
 	cmd->total_chunks = qp->npages;
-	cmd->send_chunks = qp->npages_send - 1;
+	cmd->send_chunks = qp->npages_send - PVRDMA_QP_NUM_HEADER_PAGES;
 	cmd->pdir_dma = qp->pdir.dir_dma;
 
 	dev_dbg(&dev->pdev->dev, "create queuepair with %d, %d, %d, %d\n",
diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.c
index 5489137..c2aa526 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.c
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.c
@@ -306,7 +306,7 @@ struct ib_ucontext *pvrdma_alloc_ucontext(struct ib_device *ibdev,
 	union pvrdma_cmd_resp rsp;
 	struct pvrdma_cmd_create_uc *cmd = &req.create_uc;
 	struct pvrdma_cmd_create_uc_resp *resp = &rsp.create_uc_resp;
-	struct pvrdma_alloc_ucontext_resp uresp;
+	struct pvrdma_alloc_ucontext_resp uresp = {0};
 	int ret;
 	void *ptr;
 
-- 
2.7.4

--
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 3/3] IB/vmw_pvrdma: Dont hardcode QP header page

[Yuval Shaia]

On Tue, Jan 10, 2017 at 11:15:41AM -0800, Adit Ranadive wrote:
> Moved the header page count to a macro [1]. In a future patch we will
> separate out the allocation for the header page. Thanks Yuval.
> Also, clear out the alloc_ucontext user response [2]. Thanks Dan.

Though i appreciate the above acknowledgment i'm not sure it can be part of
the commit message.

> 
> [1] - http://marc.info/?l=linux-rdma&m=148101146228847&w=2
> [2] - http://marc.info/?l=linux-rdma&m=148351229926333&w=2

Same here.

> 
> Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver")
> Reported-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> Reported-by: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/infiniband/hw/vmw_pvrdma/pvrdma.h       | 1 +
>  drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c    | 9 +++++----
>  drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.c | 2 +-
>  3 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h b/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
> index ee6a941..5dada5a 100644
> --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
> +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
> @@ -70,6 +70,7 @@
>  #define PCI_DEVICE_ID_VMWARE_PVRDMA	0x0820
>  
>  #define PVRDMA_NUM_RING_PAGES		4
> +#define PVRDMA_QP_NUM_HEADER_PAGES	1
>  
>  struct pvrdma_dev;
>  
> diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> index 765bd32..3e23425 100644
> --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> @@ -170,8 +170,9 @@ static int pvrdma_set_sq_size(struct pvrdma_dev *dev, struct ib_qp_cap *req_cap,
>  					     sizeof(struct pvrdma_sge) *
>  					     qp->sq.max_sg);
>  	/* Note: one extra page for the header. */
> -	qp->npages_send = 1 + (qp->sq.wqe_cnt * qp->sq.wqe_size +
> -			       PAGE_SIZE - 1) / PAGE_SIZE;
> +	qp->npages_send = PVRDMA_QP_NUM_HEADER_PAGES +
> +			  (qp->sq.wqe_cnt * qp->sq.wqe_size + PAGE_SIZE - 1) /
> +								PAGE_SIZE;
>  
>  	return 0;
>  }
> @@ -289,7 +290,7 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
>  			qp->npages = qp->npages_send + qp->npages_recv;
>  
>  			/* Skip header page. */
> -			qp->sq.offset = PAGE_SIZE;
> +			qp->sq.offset = PVRDMA_QP_NUM_HEADER_PAGES * PAGE_SIZE;
>  
>  			/* Recv queue pages are after send pages. */
>  			qp->rq.offset = qp->npages_send * PAGE_SIZE;
> @@ -342,7 +343,7 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
>  	cmd->qp_type = ib_qp_type_to_pvrdma(init_attr->qp_type);
>  	cmd->access_flags = IB_ACCESS_LOCAL_WRITE;
>  	cmd->total_chunks = qp->npages;
> -	cmd->send_chunks = qp->npages_send - 1;
> +	cmd->send_chunks = qp->npages_send - PVRDMA_QP_NUM_HEADER_PAGES;
>  	cmd->pdir_dma = qp->pdir.dir_dma;
>  
>  	dev_dbg(&dev->pdev->dev, "create queuepair with %d, %d, %d, %d\n",
> diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.c
> index 5489137..c2aa526 100644
> --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.c
> +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.c
> @@ -306,7 +306,7 @@ struct ib_ucontext *pvrdma_alloc_ucontext(struct ib_device *ibdev,
>  	union pvrdma_cmd_resp rsp;
>  	struct pvrdma_cmd_create_uc *cmd = &req.create_uc;
>  	struct pvrdma_cmd_create_uc_resp *resp = &rsp.create_uc_resp;
> -	struct pvrdma_alloc_ucontext_resp uresp;
> +	struct pvrdma_alloc_ucontext_resp uresp = {0};
>  	int ret;
>  	void *ptr;

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

>  
> -- 
> 2.7.4
> 
> --
> 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 2/3] IB/vmw_pvrdma: Cleanup unused variables

[Yuval Shaia]

On Tue, Jan 10, 2017 at 11:15:40AM -0800, Adit Ranadive wrote:
> Removed the unused nreq and redundant index variables.
> Moved hardcoded async and cq ring pages number to macro.
> 
> Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver")
> Reported-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/infiniband/hw/vmw_pvrdma/pvrdma.h      |  2 ++
>  drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c |  4 ++--
>  drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c   | 33 ++++++++++----------------
>  3 files changed, 17 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h b/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
> index 540a54b..ee6a941 100644
> --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
> +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
> @@ -69,6 +69,8 @@
>   */
>  #define PCI_DEVICE_ID_VMWARE_PVRDMA	0x0820
>  
> +#define PVRDMA_NUM_RING_PAGES		4
> +
>  struct pvrdma_dev;
>  
>  struct pvrdma_page_dir {
> diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
> index b57132f..8e21a86 100644
> --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
> +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
> @@ -1001,7 +1001,7 @@ static int pvrdma_pci_probe(struct pci_dev *pdev,
>  	dev->dsr->resp_slot_dma = (u64)slot_dma;
>  
>  	/* Async event ring */
> -	dev->dsr->async_ring_pages.num_pages = 4;
> +	dev->dsr->async_ring_pages.num_pages = PVRDMA_NUM_RING_PAGES;
>  	ret = pvrdma_page_dir_init(dev, &dev->async_pdir,
>  				   dev->dsr->async_ring_pages.num_pages, true);
>  	if (ret)
> @@ -1010,7 +1010,7 @@ static int pvrdma_pci_probe(struct pci_dev *pdev,
>  	dev->dsr->async_ring_pages.pdir_dma = dev->async_pdir.dir_dma;
>  
>  	/* CQ notification ring */
> -	dev->dsr->cq_ring_pages.num_pages = 4;
> +	dev->dsr->cq_ring_pages.num_pages = PVRDMA_NUM_RING_PAGES;
>  	ret = pvrdma_page_dir_init(dev, &dev->cq_pdir,
>  				   dev->dsr->cq_ring_pages.num_pages, true);
>  	if (ret)
> diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> index c8c01e5..765bd32 100644
> --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> @@ -555,13 +555,13 @@ int pvrdma_modify_qp(struct ib_qp *ibqp, struct ib_qp_attr *attr,
>  	return ret;
>  }
>  
> -static inline void *get_sq_wqe(struct pvrdma_qp *qp, int n)
> +static inline void *get_sq_wqe(struct pvrdma_qp *qp, unsigned int n)
>  {
>  	return pvrdma_page_dir_get_ptr(&qp->pdir,
>  				       qp->sq.offset + n * qp->sq.wqe_size);
>  }
>  
> -static inline void *get_rq_wqe(struct pvrdma_qp *qp, int n)
> +static inline void *get_rq_wqe(struct pvrdma_qp *qp, unsigned int n)
>  {
>  	return pvrdma_page_dir_get_ptr(&qp->pdir,
>  				       qp->rq.offset + n * qp->rq.wqe_size);
> @@ -599,9 +599,7 @@ int pvrdma_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
>  	unsigned long flags;
>  	struct pvrdma_sq_wqe_hdr *wqe_hdr;
>  	struct pvrdma_sge *sge;
> -	int i, index;
> -	int nreq;
> -	int ret;
> +	int i, ret;
>  
>  	/*
>  	 * In states lower than RTS, we can fail immediately. In other states,
> @@ -614,9 +612,8 @@ int pvrdma_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
>  
>  	spin_lock_irqsave(&qp->sq.lock, flags);
>  
> -	index = pvrdma_idx(&qp->sq.ring->prod_tail, qp->sq.wqe_cnt);
> -	for (nreq = 0; wr; nreq++, wr = wr->next) {
> -		unsigned int tail;
> +	while (wr) {
> +		unsigned int tail = 0;
>  
>  		if (unlikely(!pvrdma_idx_ring_has_space(
>  				qp->sq.ring, qp->sq.wqe_cnt, &tail))) {
> @@ -681,7 +678,7 @@ int pvrdma_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
>  			}
>  		}
>  
> -		wqe_hdr = (struct pvrdma_sq_wqe_hdr *)get_sq_wqe(qp, index);
> +		wqe_hdr = (struct pvrdma_sq_wqe_hdr *)get_sq_wqe(qp, tail);
>  		memset(wqe_hdr, 0, sizeof(*wqe_hdr));
>  		wqe_hdr->wr_id = wr->wr_id;
>  		wqe_hdr->num_sge = wr->num_sge;
> @@ -772,12 +769,11 @@ int pvrdma_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
>  		/* Make sure wqe is written before index update */
>  		smp_wmb();
>  
> -		index++;
> -		if (unlikely(index >= qp->sq.wqe_cnt))
> -			index = 0;
>  		/* Update shared sq ring */
>  		pvrdma_idx_ring_inc(&qp->sq.ring->prod_tail,
>  				    qp->sq.wqe_cnt);
> +
> +		wr = wr->next;
>  	}
>  
>  	ret = 0;
> @@ -807,7 +803,6 @@ int pvrdma_post_recv(struct ib_qp *ibqp, struct ib_recv_wr *wr,
>  	struct pvrdma_qp *qp = to_vqp(ibqp);
>  	struct pvrdma_rq_wqe_hdr *wqe_hdr;
>  	struct pvrdma_sge *sge;
> -	int index, nreq;
>  	int ret = 0;
>  	int i;
>  
> @@ -822,9 +817,8 @@ int pvrdma_post_recv(struct ib_qp *ibqp, struct ib_recv_wr *wr,
>  
>  	spin_lock_irqsave(&qp->rq.lock, flags);
>  
> -	index = pvrdma_idx(&qp->rq.ring->prod_tail, qp->rq.wqe_cnt);
> -	for (nreq = 0; wr; nreq++, wr = wr->next) {
> -		unsigned int tail;
> +	while (wr) {
> +		unsigned int tail = 0;
>  
>  		if (unlikely(wr->num_sge > qp->rq.max_sg ||
>  			     wr->num_sge < 0)) {
> @@ -844,7 +838,7 @@ int pvrdma_post_recv(struct ib_qp *ibqp, struct ib_recv_wr *wr,
>  			goto out;
>  		}
>  
> -		wqe_hdr = (struct pvrdma_rq_wqe_hdr *)get_rq_wqe(qp, index);
> +		wqe_hdr = (struct pvrdma_rq_wqe_hdr *)get_rq_wqe(qp, tail);
>  		wqe_hdr->wr_id = wr->wr_id;
>  		wqe_hdr->num_sge = wr->num_sge;
>  		wqe_hdr->total_len = 0;
> @@ -860,12 +854,11 @@ int pvrdma_post_recv(struct ib_qp *ibqp, struct ib_recv_wr *wr,
>  		/* Make sure wqe is written before index update */
>  		smp_wmb();
>  
> -		index++;
> -		if (unlikely(index >= qp->rq.wqe_cnt))
> -			index = 0;
>  		/* Update shared rq ring */
>  		pvrdma_idx_ring_inc(&qp->rq.ring->prod_tail,
>  				    qp->rq.wqe_cnt);
> +
> +		wr = wr->next;
>  	}

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

>  
>  	spin_unlock_irqrestore(&qp->rq.lock, flags);
> -- 
> 2.7.4
> 
> --
> 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 1/3] IB/vmw_pvrdma: Defer activating device until vmxnet3 link is up

[Yuval Shaia]

On Tue, Jan 10, 2017 at 11:15:39AM -0800, Adit Ranadive wrote:
> From: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> 
> Currently on bootup, ethernet drivers seem to load before RDMA ones
> in Linux. So while the vmxnet3 module is loaded before vmw_pvrdma,
> the vmxnet3 link state may not necessarily be enabled by the stack
> before pvrdma is loaded. This is a problem because if the pvrdma
> module is loaded on bootup (by installing it in /lib/modules/*),
> the pvrdma device comes up in a port down state.
> 
> Since this is the most common use case scenario, defer the activation
> of the device till the paired vmxnet3 link actually comes up. One
> downside of doing this is, if a user doesn't have the vmxnet3 link
> up when the pvrdma driver is loaded, they may not see any output
> for ibv_devinfo until the paired vmxnet3 link is enabled too. The
> users somehow need to be aware of this.
> 
> This only changes how the device is activated the first time. Once
> enabled if the link goes down, a pvrdma driver reload is still required
> after link up.
> 
> Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver")
> Signed-off-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/infiniband/hw/vmw_pvrdma/pvrdma.h      |  1 +
>  drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c | 99 +++++++++++++++++---------
>  2 files changed, 65 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h b/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
> index 71e1d55..540a54b 100644
> --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
> +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
> @@ -221,6 +221,7 @@ struct pvrdma_dev {
>  	u32 port_cap_mask;
>  	struct mutex port_mutex; /* Port modification mutex. */
>  	bool ib_active;
> +	bool enabled;
>  	atomic_t num_qps;
>  	atomic_t num_cqs;
>  	atomic_t num_pds;
> diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
> index 231a1ce..b57132f 100644
> --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
> +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
> @@ -74,6 +74,8 @@ static int pvrdma_del_gid(struct ib_device *ibdev,
>  			  void **context);
>  
>  
> +static int pvrdma_enable_dev(struct pvrdma_dev *dev);
> +
>  static ssize_t show_hca(struct device *device, struct device_attribute *attr,
>  			char *buf)
>  {
> @@ -755,6 +757,10 @@ static void pvrdma_netdevice_event_handle(struct pvrdma_dev *dev,
>  		pvrdma_dispatch_event(dev, 1, IB_EVENT_PORT_ERR);
>  		break;
>  	case NETDEV_UP:
> +		if (!dev->enabled && pvrdma_enable_dev(dev)) {
> +			dev_err(&dev->pdev->dev, "failed to enable device\n");
> +			break;
> +		}
>  		pvrdma_dispatch_event(dev, 1, IB_EVENT_PORT_ACTIVE);
>  		break;
>  	default:
> @@ -801,6 +807,48 @@ static int pvrdma_netdevice_event(struct notifier_block *this,
>  	return NOTIFY_DONE;
>  }
>  
> +static void pvrdma_disable_dev(struct pvrdma_dev *dev)
> +{
> +	if (dev->enabled) {
> +		ib_unregister_device(&dev->ib_dev);
> +		dev->enabled = false;
> +	}
> +}
> +
> +static int pvrdma_enable_dev(struct pvrdma_dev *dev)
> +{
> +	int ret;
> +	pvrdma_enable_intrs(dev);
> +
> +	/* Activate pvrdma device */
> +	pvrdma_write_reg(dev, PVRDMA_REG_CTL, PVRDMA_DEVICE_CTL_ACTIVATE);
> +
> +	/* Make sure the write is complete before reading status. */
> +	mb();
> +
> +	/* Check if device was successfully activated */
> +	ret = pvrdma_read_reg(dev, PVRDMA_REG_ERR);
> +	if (ret) {
> +		dev_err(&dev->pdev->dev, "failed to activate device\n");
> +		ret = -EFAULT;
> +		goto err_disable_intrs;
> +	}
> +
> +	/* Register IB device */
> +	ret = pvrdma_register_device(dev);
> +	if (ret) {
> +		dev_err(&dev->pdev->dev, "failed to register IB device\n");
> +		goto err_disable_intrs;
> +	}
> +
> +	dev->enabled = true;
> +	return 0;
> +
> +err_disable_intrs:
> +	pvrdma_disable_intrs(dev);
> +	return ret;
> +}
> +
>  static int pvrdma_pci_probe(struct pci_dev *pdev,
>  			    const struct pci_device_id *id)
>  {
> @@ -867,14 +915,14 @@ static int pvrdma_pci_probe(struct pci_dev *pdev,
>  	/* Enable 64-Bit DMA */
>  	if (pci_set_dma_mask(pdev, DMA_BIT_MASK(64)) == 0) {
>  		ret = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(64));
> -		if (ret != 0) {
> +		if (ret) {
>  			dev_err(&pdev->dev,
>  				"pci_set_consistent_dma_mask failed\n");
>  			goto err_free_resource;
>  		}
>  	} else {
>  		ret = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
> -		if (ret != 0) {
> +		if (ret) {
>  			dev_err(&pdev->dev,
>  				"pci_set_dma_mask failed\n");
>  			goto err_free_resource;
> @@ -1029,7 +1077,7 @@ static int pvrdma_pci_probe(struct pci_dev *pdev,
>  	if (ret) {
>  		dev_err(&pdev->dev, "failed to allocate interrupts\n");
>  		ret = -ENOMEM;
> -		goto err_netdevice;
> +		goto err_free_cq_ring;

This fix seems to be true regardless of $subject, right?

>  	}
>  
>  	/* Allocate UAR table. */
> @@ -1049,51 +1097,35 @@ static int pvrdma_pci_probe(struct pci_dev *pdev,
>  	}
>  	dev_dbg(&pdev->dev, "gid table len %d\n", dev->dsr->caps.gid_tbl_len);
>  
> -	pvrdma_enable_intrs(dev);
> -
> -	/* Activate pvrdma device */
> -	pvrdma_write_reg(dev, PVRDMA_REG_CTL, PVRDMA_DEVICE_CTL_ACTIVATE);
> -
> -	/* Make sure the write is complete before reading status. */
> -	mb();
> -
> -	/* Check if device was successfully activated */
> -	ret = pvrdma_read_reg(dev, PVRDMA_REG_ERR);
> -	if (ret != 0) {
> -		dev_err(&pdev->dev, "failed to activate device\n");
> -		ret = -EFAULT;
> -		goto err_disable_intr;
> -	}
> -
> -	/* Register IB device */
> -	ret = pvrdma_register_device(dev);
> -	if (ret) {
> -		dev_err(&pdev->dev, "failed to register IB device\n");
> -		goto err_disable_intr;
> +	if (netif_running(dev->netdev) && netif_carrier_ok(dev->netdev)) {
> +		ret = pvrdma_enable_dev(dev);
> +		if (ret) {
> +			dev_err(&pdev->dev, "failed to enable device\n");
> +			goto err_free_sgid_tbl;
> +		}
> +	} else {
> +		dev_info(&pdev->dev, "pvrdma netdev link is down\n");
>  	}
>  
>  	dev->nb_netdev.notifier_call = pvrdma_netdevice_event;
>  	ret = register_netdevice_notifier(&dev->nb_netdev);
>  	if (ret) {
>  		dev_err(&pdev->dev, "failed to register netdevice events\n");
> -		goto err_unreg_ibdev;
> +		goto err_disable_dev;
>  	}
>  
>  	dev_info(&pdev->dev, "attached to device\n");
>  	return 0;
>  
> -err_unreg_ibdev:
> -	ib_unregister_device(&dev->ib_dev);
> -err_disable_intr:
> -	pvrdma_disable_intrs(dev);
> +err_disable_dev:
> +	pvrdma_disable_dev(dev);
> +err_free_sgid_tbl:
>  	kfree(dev->sgid_tbl);
>  err_free_uar_table:
>  	pvrdma_uar_table_cleanup(dev);
>  err_free_intrs:
>  	pvrdma_free_irq(dev);
>  	pvrdma_disable_msi_all(dev);
> -err_netdevice:
> -	unregister_netdevice_notifier(&dev->nb_netdev);
>  err_free_cq_ring:
>  	pvrdma_page_dir_cleanup(dev, &dev->cq_pdir);
>  err_free_async_ring:
> @@ -1132,10 +1164,7 @@ static void pvrdma_pci_remove(struct pci_dev *pdev)
>  	unregister_netdevice_notifier(&dev->nb_netdev);
>  	dev->nb_netdev.notifier_call = NULL;
>  
> -	flush_workqueue(event_wq);
> -
> -	/* Unregister ib device */
> -	ib_unregister_device(&dev->ib_dev);
> +	pvrdma_disable_dev(dev);
>  
>  	mutex_lock(&pvrdma_device_list_lock);
>  	list_del(&dev->device_link);

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

> -- 
> 2.7.4
> 
> --
> 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 1/3] IB/vmw_pvrdma: Defer activating device until vmxnet3 link is up

[Adit Ranadive]

On Wed, Jan 11, 2017 at 10:31:32AM +0200, Yuval Shaia wrote:
> On Tue, Jan 10, 2017 at 11:15:39AM -0800, Adit Ranadive wrote:
> > From: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > 
> > Currently on bootup, ethernet drivers seem to load before RDMA ones
> > in Linux. So while the vmxnet3 module is loaded before vmw_pvrdma,
> > the vmxnet3 link state may not necessarily be enabled by the stack
> > before pvrdma is loaded. This is a problem because if the pvrdma
> > module is loaded on bootup (by installing it in /lib/modules/*),
> > the pvrdma device comes up in a port down state.
> > 
> > Since this is the most common use case scenario, defer the activation
> > of the device till the paired vmxnet3 link actually comes up. One
> > downside of doing this is, if a user doesn't have the vmxnet3 link
> > up when the pvrdma driver is loaded, they may not see any output
> > for ibv_devinfo until the paired vmxnet3 link is enabled too. The
> > users somehow need to be aware of this.
> > 
> > This only changes how the device is activated the first time. Once
> > enabled if the link goes down, a pvrdma driver reload is still required
> > after link up.
> > 
> > Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver")
> > Signed-off-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > Reviewed-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > ---
> >  drivers/infiniband/hw/vmw_pvrdma/pvrdma.h      |  1 +
> >  drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c | 99 +++++++++++++++++---------
> >  2 files changed, 65 insertions(+), 35 deletions(-)
> > 

<...>

> >  static int pvrdma_pci_probe(struct pci_dev *pdev,
> >  			    const struct pci_device_id *id)
> >  {
> > @@ -867,14 +915,14 @@ static int pvrdma_pci_probe(struct pci_dev *pdev,
> >  	/* Enable 64-Bit DMA */
> >  	if (pci_set_dma_mask(pdev, DMA_BIT_MASK(64)) == 0) {
> >  		ret = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(64));
> > -		if (ret != 0) {
> > +		if (ret) {
> >  			dev_err(&pdev->dev,
> >  				"pci_set_consistent_dma_mask failed\n");
> >  			goto err_free_resource;
> >  		}
> >  	} else {
> >  		ret = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
> > -		if (ret != 0) {
> > +		if (ret) {
> >  			dev_err(&pdev->dev,
> >  				"pci_set_dma_mask failed\n");
> >  			goto err_free_resource;
> > @@ -1029,7 +1077,7 @@ static int pvrdma_pci_probe(struct pci_dev *pdev,
> >  	if (ret) {
> >  		dev_err(&pdev->dev, "failed to allocate interrupts\n");
> >  		ret = -ENOMEM;
> > -		goto err_netdevice;
> > +		goto err_free_cq_ring;
> 
> This fix seems to be true regardless of $subject, right?

Correct. Thanks for the review!
--
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 3/3] IB/vmw_pvrdma: Dont hardcode QP header page

[Adit Ranadive]

On Wed, Jan 11, 2017 at 09:09:53AM +0200, Yuval Shaia wrote:
> On Tue, Jan 10, 2017 at 11:15:41AM -0800, Adit Ranadive wrote:
> > Moved the header page count to a macro [1]. In a future patch we will
> > separate out the allocation for the header page. Thanks Yuval.
> > Also, clear out the alloc_ucontext user response [2]. Thanks Dan.
> 
> Though i appreciate the above acknowledgment i'm not sure it can be part of
> the commit message.

Okay.

Doug, if possible please update the commit message as follows:
--
Moved the header page count to a macro. Also, clear out the alloc_ucontext
user response.
--

If you prefer I can send a new series with the updated message.

Thanks,
Adit
--
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 3/3] IB/vmw_pvrdma: Dont hardcode QP header page

[Yuval Shaia]

On Wed, Jan 11, 2017 at 02:27:31PM -0800, Adit Ranadive wrote:
> On Wed, Jan 11, 2017 at 09:09:53AM +0200, Yuval Shaia wrote:
> > On Tue, Jan 10, 2017 at 11:15:41AM -0800, Adit Ranadive wrote:
> > > Moved the header page count to a macro [1]. In a future patch we will
> > > separate out the allocation for the header page. Thanks Yuval.
> > > Also, clear out the alloc_ucontext user response [2]. Thanks Dan.
> > 
> > Though i appreciate the above acknowledgment i'm not sure it can be part of
> > the commit message.
> 
> Okay.
> 
> Doug, if possible please update the commit message as follows:
> --
> Moved the header page count to a macro. Also, clear out the alloc_ucontext
> user response.
> --

With this commit message fix:

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

> 
> If you prefer I can send a new series with the updated message.
> 
> Thanks,
> Adit
--
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 1/3] IB/vmw_pvrdma: Defer activating device until vmxnet3 link is up

[Leon Romanovsky]

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

On Tue, Jan 10, 2017 at 11:15:39AM -0800, Adit Ranadive wrote:
> From: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
>
> Currently on bootup, ethernet drivers seem to load before RDMA ones
> in Linux. So while the vmxnet3 module is loaded before vmw_pvrdma,
> the vmxnet3 link state may not necessarily be enabled by the stack
> before pvrdma is loaded. This is a problem because if the pvrdma
> module is loaded on bootup (by installing it in /lib/modules/*),
> the pvrdma device comes up in a port down state.
>
> Since this is the most common use case scenario, defer the activation
> of the device till the paired vmxnet3 link actually comes up. One
> downside of doing this is, if a user doesn't have the vmxnet3 link
> up when the pvrdma driver is loaded, they may not see any output
> for ibv_devinfo until the paired vmxnet3 link is enabled too. The
> users somehow need to be aware of this.
>

I wouldn't call it "fix", the more appropriate name is "hack".
It doesn't look right that users should be aware of such non-intuitive
driver behavior.


> This only changes how the device is activated the first time. Once
> enabled if the link goes down, a pvrdma driver reload is still required
> after link up.
>
> Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver")
> Signed-off-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/infiniband/hw/vmw_pvrdma/pvrdma.h      |  1 +
>  drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c | 99 +++++++++++++++++---------
>  2 files changed, 65 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h b/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
> index 71e1d55..540a54b 100644
> --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
> +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
> @@ -221,6 +221,7 @@ struct pvrdma_dev {
>  	u32 port_cap_mask;
>  	struct mutex port_mutex; /* Port modification mutex. */
>  	bool ib_active;
> +	bool enabled;
>  	atomic_t num_qps;
>  	atomic_t num_cqs;
>  	atomic_t num_pds;
> diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
> index 231a1ce..b57132f 100644
> --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
> +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
> @@ -74,6 +74,8 @@ static int pvrdma_del_gid(struct ib_device *ibdev,
>  			  void **context);
>
>
> +static int pvrdma_enable_dev(struct pvrdma_dev *dev);
> +
>  static ssize_t show_hca(struct device *device, struct device_attribute *attr,
>  			char *buf)
>  {
> @@ -755,6 +757,10 @@ static void pvrdma_netdevice_event_handle(struct pvrdma_dev *dev,
>  		pvrdma_dispatch_event(dev, 1, IB_EVENT_PORT_ERR);
>  		break;
>  	case NETDEV_UP:
> +		if (!dev->enabled && pvrdma_enable_dev(dev)) {
> +			dev_err(&dev->pdev->dev, "failed to enable device\n");
> +			break;
> +		}
>  		pvrdma_dispatch_event(dev, 1, IB_EVENT_PORT_ACTIVE);
>  		break;
>  	default:
> @@ -801,6 +807,48 @@ static int pvrdma_netdevice_event(struct notifier_block *this,
>  	return NOTIFY_DONE;
>  }
>
> +static void pvrdma_disable_dev(struct pvrdma_dev *dev)
> +{
> +	if (dev->enabled) {
> +		ib_unregister_device(&dev->ib_dev);
> +		dev->enabled = false;
> +	}
> +}
> +
> +static int pvrdma_enable_dev(struct pvrdma_dev *dev)
> +{
> +	int ret;
> +	pvrdma_enable_intrs(dev);
> +
> +	/* Activate pvrdma device */
> +	pvrdma_write_reg(dev, PVRDMA_REG_CTL, PVRDMA_DEVICE_CTL_ACTIVATE);
> +
> +	/* Make sure the write is complete before reading status. */
> +	mb();
> +
> +	/* Check if device was successfully activated */
> +	ret = pvrdma_read_reg(dev, PVRDMA_REG_ERR);
> +	if (ret) {
> +		dev_err(&dev->pdev->dev, "failed to activate device\n");
> +		ret = -EFAULT;
> +		goto err_disable_intrs;
> +	}
> +
> +	/* Register IB device */
> +	ret = pvrdma_register_device(dev);
> +	if (ret) {
> +		dev_err(&dev->pdev->dev, "failed to register IB device\n");
> +		goto err_disable_intrs;
> +	}
> +
> +	dev->enabled = true;
> +	return 0;
> +
> +err_disable_intrs:
> +	pvrdma_disable_intrs(dev);
> +	return ret;
> +}
> +
>  static int pvrdma_pci_probe(struct pci_dev *pdev,
>  			    const struct pci_device_id *id)
>  {
> @@ -867,14 +915,14 @@ static int pvrdma_pci_probe(struct pci_dev *pdev,
>  	/* Enable 64-Bit DMA */
>  	if (pci_set_dma_mask(pdev, DMA_BIT_MASK(64)) == 0) {
>  		ret = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(64));
> -		if (ret != 0) {
> +		if (ret) {
>  			dev_err(&pdev->dev,
>  				"pci_set_consistent_dma_mask failed\n");
>  			goto err_free_resource;
>  		}
>  	} else {
>  		ret = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
> -		if (ret != 0) {
> +		if (ret) {
>  			dev_err(&pdev->dev,
>  				"pci_set_dma_mask failed\n");
>  			goto err_free_resource;
> @@ -1029,7 +1077,7 @@ static int pvrdma_pci_probe(struct pci_dev *pdev,
>  	if (ret) {
>  		dev_err(&pdev->dev, "failed to allocate interrupts\n");
>  		ret = -ENOMEM;
> -		goto err_netdevice;
> +		goto err_free_cq_ring;
>  	}
>
>  	/* Allocate UAR table. */
> @@ -1049,51 +1097,35 @@ static int pvrdma_pci_probe(struct pci_dev *pdev,
>  	}
>  	dev_dbg(&pdev->dev, "gid table len %d\n", dev->dsr->caps.gid_tbl_len);
>
> -	pvrdma_enable_intrs(dev);
> -
> -	/* Activate pvrdma device */
> -	pvrdma_write_reg(dev, PVRDMA_REG_CTL, PVRDMA_DEVICE_CTL_ACTIVATE);
> -
> -	/* Make sure the write is complete before reading status. */
> -	mb();
> -
> -	/* Check if device was successfully activated */
> -	ret = pvrdma_read_reg(dev, PVRDMA_REG_ERR);
> -	if (ret != 0) {
> -		dev_err(&pdev->dev, "failed to activate device\n");
> -		ret = -EFAULT;
> -		goto err_disable_intr;
> -	}
> -
> -	/* Register IB device */
> -	ret = pvrdma_register_device(dev);
> -	if (ret) {
> -		dev_err(&pdev->dev, "failed to register IB device\n");
> -		goto err_disable_intr;
> +	if (netif_running(dev->netdev) && netif_carrier_ok(dev->netdev)) {
> +		ret = pvrdma_enable_dev(dev);
> +		if (ret) {
> +			dev_err(&pdev->dev, "failed to enable device\n");
> +			goto err_free_sgid_tbl;
> +		}
> +	} else {
> +		dev_info(&pdev->dev, "pvrdma netdev link is down\n");
>  	}
>
>  	dev->nb_netdev.notifier_call = pvrdma_netdevice_event;
>  	ret = register_netdevice_notifier(&dev->nb_netdev);
>  	if (ret) {
>  		dev_err(&pdev->dev, "failed to register netdevice events\n");
> -		goto err_unreg_ibdev;
> +		goto err_disable_dev;
>  	}
>
>  	dev_info(&pdev->dev, "attached to device\n");
>  	return 0;
>
> -err_unreg_ibdev:
> -	ib_unregister_device(&dev->ib_dev);
> -err_disable_intr:
> -	pvrdma_disable_intrs(dev);
> +err_disable_dev:
> +	pvrdma_disable_dev(dev);
> +err_free_sgid_tbl:
>  	kfree(dev->sgid_tbl);
>  err_free_uar_table:
>  	pvrdma_uar_table_cleanup(dev);
>  err_free_intrs:
>  	pvrdma_free_irq(dev);
>  	pvrdma_disable_msi_all(dev);
> -err_netdevice:
> -	unregister_netdevice_notifier(&dev->nb_netdev);
>  err_free_cq_ring:
>  	pvrdma_page_dir_cleanup(dev, &dev->cq_pdir);
>  err_free_async_ring:
> @@ -1132,10 +1164,7 @@ static void pvrdma_pci_remove(struct pci_dev *pdev)
>  	unregister_netdevice_notifier(&dev->nb_netdev);
>  	dev->nb_netdev.notifier_call = NULL;
>
> -	flush_workqueue(event_wq);
> -
> -	/* Unregister ib device */
> -	ib_unregister_device(&dev->ib_dev);
> +	pvrdma_disable_dev(dev);
>
>  	mutex_lock(&pvrdma_device_list_lock);
>  	list_del(&dev->device_link);
> --
> 2.7.4
>
> --
> 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 1/3] IB/vmw_pvrdma: Defer activating device until vmxnet3 link is up

[Adit Ranadive]

On Mon, Jan 16, 2017 at 09:38:58AM +0200, Leon Romanovsky wrote:
> On Tue, Jan 10, 2017 at 11:15:39AM -0800, Adit Ranadive wrote:
> > From: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> >
> > Currently on bootup, ethernet drivers seem to load before RDMA ones
> > in Linux. So while the vmxnet3 module is loaded before vmw_pvrdma,
> > the vmxnet3 link state may not necessarily be enabled by the stack
> > before pvrdma is loaded. This is a problem because if the pvrdma
> > module is loaded on bootup (by installing it in /lib/modules/*),
> > the pvrdma device comes up in a port down state.
> >
> > Since this is the most common use case scenario, defer the activation
> > of the device till the paired vmxnet3 link actually comes up. One
> > downside of doing this is, if a user doesn't have the vmxnet3 link
> > up when the pvrdma driver is loaded, they may not see any output
> > for ibv_devinfo until the paired vmxnet3 link is enabled too. The
> > users somehow need to be aware of this.
> >
> 
> I wouldn't call it "fix", the more appropriate name is "hack".
> It doesn't look right that users should be aware of such non-intuitive
> driver behavior.

Well its a partial fix for 4.10 to let the RDMA device come up correctly
Since this is a RoCE device we want to mimic to eth port status for the
RDMA link. Currently, for us there isnt an easy way to do this if the
vmxnet3 device comes up after we have already registered the IB device.
This patch moves the IB device registration to after the ethernet link is
up.
I wanted to get this in so that in the common case of VM power on, the 
PVRDMA device is in the port active state along with the vmxnet3 link.

Doug, any thoughts?

- Adit
--
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 1/3] IB/vmw_pvrdma: Defer activating device until vmxnet3 link is up

[Parav Pandit]

Hi Adit,

> -----Original Message-----
> From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-
> owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Adit Ranadive
> Sent: Tuesday, January 17, 2017 6:31 PM
> To: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; pv-
> drivers-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org; Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Subject: Re: [PATCH 1/3] IB/vmw_pvrdma: Defer activating device until
> vmxnet3 link is up
> 
> On Mon, Jan 16, 2017 at 09:38:58AM +0200, Leon Romanovsky wrote:
> > On Tue, Jan 10, 2017 at 11:15:39AM -0800, Adit Ranadive wrote:
> > > From: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > >
> > > Currently on bootup, ethernet drivers seem to load before RDMA ones
> > > in Linux. So while the vmxnet3 module is loaded before vmw_pvrdma,
> > > the vmxnet3 link state may not necessarily be enabled by the stack
> > > before pvrdma is loaded. This is a problem because if the pvrdma
> > > module is loaded on bootup (by installing it in /lib/modules/*), the
> > > pvrdma device comes up in a port down state.
> > >
> > > Since this is the most common use case scenario, defer the
> > > activation of the device till the paired vmxnet3 link actually comes
> > > up. One downside of doing this is, if a user doesn't have the
> > > vmxnet3 link up when the pvrdma driver is loaded, they may not see
> > > any output for ibv_devinfo until the paired vmxnet3 link is enabled
> > > too. The users somehow need to be aware of this.
> > >
> >
> > I wouldn't call it "fix", the more appropriate name is "hack".
> > It doesn't look right that users should be aware of such non-intuitive
> > driver behavior.
> 
> Well its a partial fix for 4.10 to let the RDMA device come up correctly Since
> this is a RoCE device we want to mimic to eth port status for the RDMA link.
> Currently, for us there isnt an easy way to do this if the
> vmxnet3 device comes up after we have already registered the IB device.
vmxnet3 device come up and its link come are two different events.
I haven't reviewed the patches or code either.
So asking dumb question.
Can't you load RDMA driver when vmxnet3 device come up?
You should bring up the link of RDMA device when vmxnet3 device's link is up.
(Similar to what other RDMA drivers do which depend on their underlying NIC drivers).

> This patch moves the IB device registration to after the ethernet link is up.
> I wanted to get this in so that in the common case of VM power on, the
> PVRDMA device is in the port active state along with the vmxnet3 link.
>
What happens when link goes down? Does it call ib_unregister()?

 
> Doug, any thoughts?
> 
> - Adit
> --
> 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 1/3] IB/vmw_pvrdma: Defer activating device until vmxnet3 link is up

[Adit Ranadive]

On Wed, Jan 18, 2017 at 04:21:13AM +0000, Parav Pandit wrote:
> Hi Adit,
> 
> > -----Original Message-----
> > From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-
> > owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Adit Ranadive
> > Sent: Tuesday, January 17, 2017 6:31 PM
> > To: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > Cc: dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; pv-
> > drivers-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org; Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > Subject: Re: [PATCH 1/3] IB/vmw_pvrdma: Defer activating device until
> > vmxnet3 link is up
> > 
> > On Mon, Jan 16, 2017 at 09:38:58AM +0200, Leon Romanovsky wrote:
> > > On Tue, Jan 10, 2017 at 11:15:39AM -0800, Adit Ranadive wrote:
> > > > From: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > >
> > > > Currently on bootup, ethernet drivers seem to load before RDMA ones
> > > > in Linux. So while the vmxnet3 module is loaded before vmw_pvrdma,
> > > > the vmxnet3 link state may not necessarily be enabled by the stack
> > > > before pvrdma is loaded. This is a problem because if the pvrdma
> > > > module is loaded on bootup (by installing it in /lib/modules/*), the
> > > > pvrdma device comes up in a port down state.
> > > >
> > > > Since this is the most common use case scenario, defer the
> > > > activation of the device till the paired vmxnet3 link actually comes
> > > > up. One downside of doing this is, if a user doesn't have the
> > > > vmxnet3 link up when the pvrdma driver is loaded, they may not see
> > > > any output for ibv_devinfo until the paired vmxnet3 link is enabled
> > > > too. The users somehow need to be aware of this.
> > > >
> > >
> > > I wouldn't call it "fix", the more appropriate name is "hack".
> > > It doesn't look right that users should be aware of such non-intuitive
> > > driver behavior.
> > 
> > Well its a partial fix for 4.10 to let the RDMA device come up correctly Since
> > this is a RoCE device we want to mimic to eth port status for the RDMA link.
> > Currently, for us there isnt an easy way to do this if the
> > vmxnet3 device comes up after we have already registered the IB device.
> vmxnet3 device come up and its link come are two different events.

You're right that they are two separate events - I was referring to the
vmxnet3 link being up. 

> I haven't reviewed the patches or code either.
> So asking dumb question.
> Can't you load RDMA driver when vmxnet3 device come up?

Are you referring to loading the pvrdma module or just calling ib_register
after the vmxnet3 link is up? The latter is waht we do in this patch anyways.

> You should bring up the link of RDMA device when vmxnet3 device's link is up.
> (Similar to what other RDMA drivers do which depend on their underlying NIC drivers).

We do register for netdev events so we get callbacks for the
link state. The problem happens when we call ib_register before the vmxnet3
link is up. At this point the ib_core stack already has created the QP1, etc.
Once the vmxnet3 link is up we would have to call ib_unregister/ib_register
to recreate QP1. This might fail/stuck if the there is a user app thats created
QPs and we're just waiting to free those resources.

> > This patch moves the IB device registration to after the ethernet link is up.
> > I wanted to get this in so that in the common case of VM power on, the
> > PVRDMA device is in the port active state along with the vmxnet3 link.
> >
> What happens when link goes down? Does it call ib_unregister()?
> 
>  
> > Doug, any thoughts?
> > 
> > - Adit
--
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 1/3] IB/vmw_pvrdma: Defer activating device until vmxnet3 link is up

[Jason Gunthorpe]

On Wed, Jan 18, 2017 at 10:30:20AM -0800, Adit Ranadive wrote:

> We do register for netdev events so we get callbacks for the
> link state. The problem happens when we call ib_register before the vmxnet3
> link is up. At this point the ib_core stack already has created the QP1, etc.
> Once the vmxnet3 link is up we would have to call ib_unregister/ib_register
> to recreate QP1. This might fail/stuck if the there is a user app thats created
> QPs and we're just waiting to free those resources.

Having to register/unregister on every link transition sounds like a
horrible design, you should fix that :)

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 1/3] IB/vmw_pvrdma: Defer activating device until vmxnet3 link is up

[Doug Ledford]

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

On Tue, 2017-01-17 at 16:31 -0800, Adit Ranadive wrote:
> On Mon, Jan 16, 2017 at 09:38:58AM +0200, Leon Romanovsky wrote:
> > 
> > On Tue, Jan 10, 2017 at 11:15:39AM -0800, Adit Ranadive wrote:
> > > 
> > > From: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > 
> > > Currently on bootup, ethernet drivers seem to load before RDMA
> > > ones
> > > in Linux. So while the vmxnet3 module is loaded before
> > > vmw_pvrdma,
> > > the vmxnet3 link state may not necessarily be enabled by the
> > > stack
> > > before pvrdma is loaded. This is a problem because if the pvrdma
> > > module is loaded on bootup (by installing it in /lib/modules/*),
> > > the pvrdma device comes up in a port down state.
> > > 
> > > Since this is the most common use case scenario, defer the
> > > activation
> > > of the device till the paired vmxnet3 link actually comes up. One
> > > downside of doing this is, if a user doesn't have the vmxnet3
> > > link
> > > up when the pvrdma driver is loaded, they may not see any output
> > > for ibv_devinfo until the paired vmxnet3 link is enabled too. The
> > > users somehow need to be aware of this.
> > > 
> > 
> > I wouldn't call it "fix", the more appropriate name is "hack".
> > It doesn't look right that users should be aware of such non-
> > intuitive
> > driver behavior.
> 
> Well its a partial fix for 4.10 to let the RDMA device come up
> correctly
> Since this is a RoCE device we want to mimic to eth port status for
> the
> RDMA link. Currently, for us there isnt an easy way to do this if the
> vmxnet3 device comes up after we have already registered the IB
> device.
> This patch moves the IB device registration to after the ethernet
> link is
> up.
> I wanted to get this in so that in the common case of VM power on,
> the 
> PVRDMA device is in the port active state along with the vmxnet3
> link.
> 
> Doug, any thoughts?

Yes, I'm wondering why the vmw_pvrdma driver doesn't detect link state
changes and act appropriately?

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG KeyID: B826A3330E572FDD
   
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/3] IB/vmw_pvrdma: Defer activating device until vmxnet3 link is up

[Doug Ledford]

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

On Wed, 2017-01-18 at 11:42 -0700, Jason Gunthorpe wrote:
> On Wed, Jan 18, 2017 at 10:30:20AM -0800, Adit Ranadive wrote:
> 
> > 
> > We do register for netdev events so we get callbacks for the
> > link state. The problem happens when we call ib_register before the
> > vmxnet3
> > link is up. At this point the ib_core stack already has created the
> > QP1, etc.
> > Once the vmxnet3 link is up we would have to call
> > ib_unregister/ib_register
> > to recreate QP1. This might fail/stuck if the there is a user app
> > thats created
> > QPs and we're just waiting to free those resources.
> 
> Having to register/unregister on every link transition sounds like a
> horrible design, you should fix that :)

Indeed.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG KeyID: B826A3330E572FDD
   
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/3] IB/vmw_pvrdma: Defer activating device until vmxnet3 link is up

[Adit Ranadive]

On Wed, Jan 18, 2017 at 11:42:36AM -0700, Jason Gunthorpe wrote:
> On Wed, Jan 18, 2017 at 10:30:20AM -0800, Adit Ranadive wrote:
> 
>> We do register for netdev events so we get callbacks for the
>> link state. The problem happens when we call ib_register before the vmxnet3
>> link is up. At this point the ib_core stack already has created the QP1, etc.
>> Once the vmxnet3 link is up we would have to call ib_unregister/ib_register
>> to recreate QP1. This might fail/stuck if the there is a user app thats created
>> QPs and we're just waiting to free those resources.
> 
> Having to register/unregister on every link transition sounds like a
> horrible design, you should fix that :)

We try to avoid the register if the link is down with this patch but the
issue boils down to the handling of QP1. This gets us a step closer.

In order to recover QP1 correctly after a link transition we had to go through a
register/unregister since it is placed in error state when the port is down.
In this case can the MAD layer re-initialize QP1 if link transitions are performed?







--
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 1/3] IB/vmw_pvrdma: Defer activating device until vmxnet3 link is up

[Adit Ranadive]

On Wed, Jan 18, 2017 at 3:23:10PM -0500, Doug Ledford wrote:
> On Tue, 2017-01-17 at 16:31 -0800, Adit Ranadive wrote:
>> On Mon, Jan 16, 2017 at 09:38:58AM +0200, Leon Romanovsky wrote:
>> > 
>> > On Tue, Jan 10, 2017 at 11:15:39AM -0800, Adit Ranadive wrote:
>> > > 
>> > > From: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
>> > > 
>> > > Currently on bootup, ethernet drivers seem to load before RDMA
>> > > ones
>> > > in Linux. So while the vmxnet3 module is loaded before
>> > > vmw_pvrdma,
>> > > the vmxnet3 link state may not necessarily be enabled by the
>> > > stack
>> > > before pvrdma is loaded. This is a problem because if the pvrdma
>> > > module is loaded on bootup (by installing it in /lib/modules/*),
>> > > the pvrdma device comes up in a port down state.
>> > > 
>> > > Since this is the most common use case scenario, defer the
>> > > activation
>> > > of the device till the paired vmxnet3 link actually comes up. One
>> > > downside of doing this is, if a user doesn't have the vmxnet3
>> > > link
>> > > up when the pvrdma driver is loaded, they may not see any output
>> > > for ibv_devinfo until the paired vmxnet3 link is enabled too. The
>> > > users somehow need to be aware of this.
>> > > 
>> > 
>> > I wouldn't call it "fix", the more appropriate name is "hack".
>> > It doesn't look right that users should be aware of such non-
>> > intuitive
>> > driver behavior.
>> 
>> Well its a partial fix for 4.10 to let the RDMA device come up
>> correctly
>> Since this is a RoCE device we want to mimic to eth port status for
>> the
>> RDMA link. Currently, for us there isnt an easy way to do this if the
>> vmxnet3 device comes up after we have already registered the IB
>> device.
>> This patch moves the IB device registration to after the ethernet
>> link is
>> up.
>> I wanted to get this in so that in the common case of VM power on,
>> the 
>> PVRDMA device is in the port active state along with the vmxnet3
>> link.
>> 
>> Doug, any thoughts?
> 
> Yes, I'm wondering why the vmw_pvrdma driver doesn't detect link state
> changes and act appropriately?

We do through netdev events. Unfortunately, currently we are limited to
registering/unregistering to recover everything correctly.
--
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 1/3] IB/vmw_pvrdma: Defer activating device until vmxnet3 link is up

[Jason Gunthorpe]

On Wed, Jan 18, 2017 at 12:30:07PM -0800, Adit Ranadive wrote:

> In order to recover QP1 correctly after a link transition we had to go through a
> register/unregister since it is placed in error state when the port is down.
> In this case can the MAD layer re-initialize QP1 if link transitions are performed?

Your driver needs to deal with this. Do whatever it takes to recover
your QP1 without re-registring and do not bother the rest of the
system with it.

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 1/3] IB/vmw_pvrdma: Defer activating device until vmxnet3 link is up

[Adit Ranadive]

On Wed, Jan 18, 2017 at 1:41:30PM -0700, Jason Gunthorpe wrote:
> On Wed, Jan 18, 2017 at 12:30:07PM -0800, Adit Ranadive wrote:
> 
>> In order to recover QP1 correctly after a link transition we had to go through a
>> register/unregister since it is placed in error state when the port is down.
>> In this case can the MAD layer re-initialize QP1 if link transitions are performed?
> 
> Your driver needs to deal with this. Do whatever it takes to recover
> your QP1 without re-registring and do not bother the rest of the
> system with it.

Ok. We'll investigate internally on for a better recovery process for QP1.

--
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 1/3] IB/vmw_pvrdma: Defer activating device until vmxnet3 link is up

[Parav Pandit]

Hi Adit,

> -----Original Message-----
> From: Adit Ranadive [mailto:aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org]
> Sent: Wednesday, January 18, 2017 2:53 PM
> To: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Cc: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; Leon Romanovsky
> <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>; dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; pv-
> drivers-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org; Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Subject: Re: [PATCH 1/3] IB/vmw_pvrdma: Defer activating device until
> vmxnet3 link is up
> 
> On Wed, Jan 18, 2017 at 1:41:30PM -0700, Jason Gunthorpe wrote:
> > On Wed, Jan 18, 2017 at 12:30:07PM -0800, Adit Ranadive wrote:
> >
> >> In order to recover QP1 correctly after a link transition we had to
> >> go through a register/unregister since it is placed in error state when the
> port is down.
> >> In this case can the MAD layer re-initialize QP1 if link transitions are
> performed?
> >
> > Your driver needs to deal with this. Do whatever it takes to recover
> > your QP1 without re-registring and do not bother the rest of the
> > system with it.
> 
> Ok. We'll investigate internally on for a better recovery process for QP1.

By the time I reply, most others requested same.
Thanks for addressing it.
--
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 1/3] IB/vmw_pvrdma: Defer activating device until vmxnet3 link is up

[Adit Ranadive]

On Wed, Jan 18, 2017 at 10:50:54PM +0000, Parav Pandit wrote:
> Hi Adit,
> 
>> -----Original Message-----
>> From: Adit Ranadive [mailto:aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org]
>> Sent: Wednesday, January 18, 2017 2:53 PM
>> To: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
>> Cc: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; Leon Romanovsky
>> <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>; dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; pv-
>> drivers-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org; Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
>> Subject: Re: [PATCH 1/3] IB/vmw_pvrdma: Defer activating device until
>> vmxnet3 link is up
>> 
>> On Wed, Jan 18, 2017 at 1:41:30PM -0700, Jason Gunthorpe wrote:
>> > On Wed, Jan 18, 2017 at 12:30:07PM -0800, Adit Ranadive wrote:
>> >
>> >> In order to recover QP1 correctly after a link transition we had to
>> >> go through a register/unregister since it is placed in error state when the
>> port is down.
>> >> In this case can the MAD layer re-initialize QP1 if link transitions are
>> performed?
>> >
>> > Your driver needs to deal with this. Do whatever it takes to recover
>> > your QP1 without re-registring and do not bother the rest of the
>> > system with it.
>> 
>> Ok. We'll investigate internally on for a better recovery process for QP1.
> 
> By the time I reply, most others requested same.
> Thanks for addressing it.

Hi Doug,

Please consider taking the other two patches in this series if possible
for your next set of rc fixes.

Thanks,
Adit

--
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 1/3] IB/vmw_pvrdma: Defer activating device until vmxnet3 link is up

[Doug Ledford]

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

On Wed, 2017-01-18 at 16:31 -0800, Adit Ranadive wrote:
> On Wed, Jan 18, 2017 at 10:50:54PM +0000, Parav Pandit wrote:
> > 
> > Hi Adit,
> > 
> > > 
> > > -----Original Message-----
> > > From: Adit Ranadive [mailto:aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org]
> > > Sent: Wednesday, January 18, 2017 2:53 PM
> > > To: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> > > Cc: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; Leon Romanovsky
> > > <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>; dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org; linux-rdma-u79uwXL29TaqPxH82wqD4g@public.gmane.org
> > > g; pv-
> > > drivers-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org; Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > Subject: Re: [PATCH 1/3] IB/vmw_pvrdma: Defer activating device
> > > until
> > > vmxnet3 link is up
> > > 
> > > On Wed, Jan 18, 2017 at 1:41:30PM -0700, Jason Gunthorpe wrote:
> > > > 
> > > > On Wed, Jan 18, 2017 at 12:30:07PM -0800, Adit Ranadive wrote:
> > > > 
> > > > > 
> > > > > In order to recover QP1 correctly after a link transition we
> > > > > had to
> > > > > go through a register/unregister since it is placed in error
> > > > > state when the
> > > port is down.
> > > > 
> > > > > 
> > > > > In this case can the MAD layer re-initialize QP1 if link
> > > > > transitions are
> > > performed?
> > > > 
> > > > 
> > > > Your driver needs to deal with this. Do whatever it takes to
> > > > recover
> > > > your QP1 without re-registring and do not bother the rest of
> > > > the
> > > > system with it.
> > > 
> > > Ok. We'll investigate internally on for a better recovery process
> > > for QP1.
> > 
> > By the time I reply, most others requested same.
> > Thanks for addressing it.
> 
> Hi Doug,
> 
> Please consider taking the other two patches in this series if
> possible
> for your next set of rc fixes.

If you want me to take these for the -rc cycle, then they need to be
presented appropriately.  This patch (1/3) is gone just because it was
the wrong solution (but it at least sounded like a fix).  Patch 2/3
starts its subject with "Cleanup unused...", that doesn't sound
remotely like a fix.  Patch 3/3 starts with "Don't hardcode...", which
also doesn't sound remotely like a fix.  If you want these in the -rc
cycle, then please repost the two other patches with suitable subjects
and commit logs that make it clear these are real fixes needed in an
-rc and not simple patches that should get applied in the for-next
area.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG KeyID: B826A3330E572FDD
   
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/3] IB/vmw_pvrdma: Defer activating device until vmxnet3 link is up

[Adit Ranadive]

On Thu, Jan 19, 2017 at 01:45:17AM -0500, Doug Ledford wrote:
> On Wed, 2017-01-18 at 16:31 -0800, Adit Ranadive wrote:
> > On Wed, Jan 18, 2017 at 10:50:54PM +0000, Parav Pandit wrote:
> > > 
> > > Hi Adit,
> > > 
> > > > 
> > > > -----Original Message-----
> > > > From: Adit Ranadive [mailto:aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org]
> > > > Sent: Wednesday, January 18, 2017 2:53 PM
> > > > To: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> > > > Cc: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; Leon Romanovsky
> > > > <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>; dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org; linux-rdma-u79uwXL29TaqPxH82wqD4g@public.gmane.org
> > > > g; pv-
> > > > drivers-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org; Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > > Subject: Re: [PATCH 1/3] IB/vmw_pvrdma: Defer activating device
> > > > until
> > > > vmxnet3 link is up
> > > > 
> > > > On Wed, Jan 18, 2017 at 1:41:30PM -0700, Jason Gunthorpe wrote:
> > > > > 
> > > > > On Wed, Jan 18, 2017 at 12:30:07PM -0800, Adit Ranadive wrote:
> > > > > 
> > > > > > 
> > > > > > In order to recover QP1 correctly after a link transition we
> > > > > > had to
> > > > > > go through a register/unregister since it is placed in error
> > > > > > state when the
> > > > port is down.
> > > > > 
> > > > > > 
> > > > > > In this case can the MAD layer re-initialize QP1 if link
> > > > > > transitions are
> > > > performed?
> > > > > 
> > > > > 
> > > > > Your driver needs to deal with this. Do whatever it takes to
> > > > > recover
> > > > > your QP1 without re-registring and do not bother the rest of
> > > > > the
> > > > > system with it.
> > > > 
> > > > Ok. We'll investigate internally on for a better recovery process
> > > > for QP1.
> > > 
> > > By the time I reply, most others requested same.
> > > Thanks for addressing it.
> > 
> > Hi Doug,
> > 
> > Please consider taking the other two patches in this series if
> > possible
> > for your next set of rc fixes.
> 
> If you want me to take these for the -rc cycle, then they need to be
> presented appropriately.  This patch (1/3) is gone just because it was
> the wrong solution (but it at least sounded like a fix).  Patch 2/3
> starts its subject with "Cleanup unused...", that doesn't sound
> remotely like a fix.  Patch 3/3 starts with "Don't hardcode...", which
> also doesn't sound remotely like a fix.  If you want these in the -rc
> cycle, then please repost the two other patches with suitable subjects
> and commit logs that make it clear these are real fixes needed in an
> -rc and not simple patches that should get applied in the for-next
> area.

Ah ok. Sorry for my misunderstanding. In that case there are two oneline
fixes - info leaked to user and an incorrect cleanup on the pci probe error
path. I'll separate them out from these patches and post separately.
Please, drop this series and I'll repost the patches 2,3 separately meant
for for-next.

Thanks,
Adit
--
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