IB/vmw_pvrdma patches for 4.10

From: Aditya Sarwade <[email protected]>

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 <[email protected]>

Reviewed-by Bryan Tan <[email protected]>

Signed-off-by Adit Ranadive <[email protected]>

---
 drivers/infiniband/hw/vmw_pvrdma/pvrdma.h      |  1 +
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c | 99 +++++++++++++++++---------
 2 files changed, 65 insertions(+), 35 deletions(-)

drivers/infiniband/hw/vmw_pvrdma/pvrdma.h +1 -0

index 71e1d55..540a54b 100644
@@ -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;

drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c +50 -2

index 231a1ce..b57132f 100644
@@ -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;

drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c +12 -28

 	}
 
 	/* 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);

Signature · 6 lines

2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 29 lines · Adit Ranadive

From: Aditya Sarwade <[email protected]>

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 <[email protected]>

Reviewed-by Bryan Tan <[email protected]>

Signed-off-by Adit Ranadive <[email protected]>

---
 drivers/infiniband/hw/vmw_pvrdma/pvrdma.h      |  1 +
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c | 99 +++++++++++++++++---------
 2 files changed, 65 insertions(+), 35 deletions(-)

Quoted 20 lines

static int pvrdma_pci_probe(struct pci_dev *pdev,

drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c +2 -2

 			    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;

drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c static int pvrdma_pci_probe(struct pci_dev *pdev, · line 1080

Quoted 6 lines

drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c +1 -1

@@ -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?

From: Aditya Sarwade <[email protected]>

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 <[email protected]>

Reviewed-by Bryan Tan <[email protected]>

Signed-off-by Adit Ranadive <[email protected]>

---
 drivers/infiniband/hw/vmw_pvrdma/pvrdma.h      |  1 +
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c | 99 +++++++++++++++++---------
 2 files changed, 65 insertions(+), 35 deletions(-)

drivers/infiniband/hw/vmw_pvrdma/pvrdma.h +1 -0

index 71e1d55..540a54b 100644
@@ -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;

drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c +64 -36

index 231a1ce..b57132f 100644
@@ -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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 9 lines · Adit Ranadive

From: Aditya Sarwade <[email protected]>

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.

Quoted 7 lines

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.

-----Original Message-----
From: [email protected] [mailto:linux-rdma- [email protected]] On Behalf Of Adit Ranadive
Sent: Tuesday, January 17, 2017 6:31 PM
To: Leon Romanovsky <[email protected]>
Cc: [email protected]; [email protected]; pv- [email protected]; Aditya Sarwade <[email protected]>
Subject: Re: [PATCH 1/3] IB/vmw_pvrdma: Defer activating device until
vmxnet3 link is up

Quoted 18 lines · Leon Romanovsky

Quoted 16 lines · Adit Ranadive

From: Aditya Sarwade <[email protected]>

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.

Quoted 3 lines

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 [email protected] More majordomo info at
http://vger.kernel.org/majordomo-info.html

Hi Adit,

Quoted 33 lines

-----Original Message-----
From: [email protected] [mailto:linux-rdma- [email protected]] On Behalf Of Adit Ranadive
Sent: Tuesday, January 17, 2017 6:31 PM
To: Leon Romanovsky <[email protected]>
Cc: [email protected]; [email protected]; pv- [email protected]; Aditya Sarwade <[email protected]>
Subject: Re: [PATCH 1/3] IB/vmw_pvrdma: Defer activating device until
vmxnet3 link is up

Quoted 21 lines · Leon Romanovsky

Quoted 9 lines · Adit Ranadive

From: Aditya Sarwade <[email protected]>

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.

Quoted 7 lines

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.

Quoted 4 lines

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

Quoted 4 lines

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()?

Quoted 3 lines

Doug, any thoughts?

- Adit

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.

Quoted 11 lines · Adit Ranadive

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 :)

Quoted 6 lines · Adit Ranadive

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 :)

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?

Quoted 3 lines · Adit Ranadive

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.

-----Original Message-----
From: Adit Ranadive [mailto:[email protected]]
Sent: Wednesday, January 18, 2017 2:53 PM
To: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Cc: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>; Leon Romanovsky <[email protected]>; [email protected]; [email protected]; pv- [email protected]; Aditya Sarwade <[email protected]>
Subject: Re: [PATCH 1/3] IB/vmw_pvrdma: Defer activating device until
vmxnet3 link is up

Quoted 4 lines · Jason Gunthorpe

Quoted 2 lines · Adit Ranadive

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.

Quoted 1 line

Quoted 1 line

In this case can the MAD layer re-initialize QP1 if link transitions are

performed?

Quoted 4 lines

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.

Hi Adit,

Quoted 19 lines

-----Original Message-----
From: Adit Ranadive [mailto:[email protected]]
Sent: Wednesday, January 18, 2017 2:53 PM
To: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Cc: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>; Leon Romanovsky <[email protected]>; [email protected]; [email protected]; pv- [email protected]; Aditya Sarwade <[email protected]>
Subject: Re: [PATCH 1/3] IB/vmw_pvrdma: Defer activating device until
vmxnet3 link is up

Quoted 4 lines · Jason Gunthorpe

Quoted 2 lines · Adit Ranadive

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.

Quoted 1 line

Quoted 1 line

In this case can the MAD layer re-initialize QP1 if link transitions are

performed?

Quoted 1 line

Quoted 5 lines

Quoted 3 lines

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.

Quoted 42 lines · Parav Pandit

Hi Adit,

Quoted 38 lines

-----Original Message-----
From: Adit Ranadive [mailto:[email protected]]
Sent: Wednesday, January 18, 2017 2:53 PM
To: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Cc: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>; Leon Romanovsky <[email protected]>; [email protected]; [email protected] g; pv-
[email protected]; Aditya Sarwade <[email protected]>
Subject: Re: [PATCH 1/3] IB/vmw_pvrdma: Defer activating device
until
vmxnet3 link is up

Quoted 8 lines · Jason Gunthorpe

Quoted 5 lines · Adit Ranadive

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.

Quoted 4 lines

Quoted 4 lines

In this case can the MAD layer re-initialize QP1 if link transitions are

performed?

Quoted 7 lines

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.

Quoted 2 lines

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.

Quoted 43 lines · Adit Ranadive

Quoted 42 lines · Parav Pandit

Hi Adit,

Quoted 38 lines

-----Original Message-----
From: Adit Ranadive [mailto:[email protected]]
Sent: Wednesday, January 18, 2017 2:53 PM
To: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Cc: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>; Leon Romanovsky <[email protected]>; [email protected]; [email protected] g; pv-
[email protected]; Aditya Sarwade <[email protected]>
Subject: Re: [PATCH 1/3] IB/vmw_pvrdma: Defer activating device
until
vmxnet3 link is up

Quoted 8 lines · Jason Gunthorpe

Quoted 5 lines · Adit Ranadive

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.

Quoted 4 lines

Quoted 4 lines

In this case can the MAD layer re-initialize QP1 if link transitions are

performed?

Quoted 7 lines

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.

Quoted 8 lines

Quoted 2 lines

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.

Quoted 25 lines · Leon Romanovsky

Quoted 13 lines · Adit Ranadive

From: Aditya Sarwade <[email protected]>

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.

Quoted 9 lines

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.

Quoted 4 lines

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?

Quoted 46 lines · Adit Ranadive

Quoted 25 lines · Leon Romanovsky

Quoted 13 lines · Adit Ranadive

From: Aditya Sarwade <[email protected]>

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.

Quoted 9 lines

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.

Quoted 4 lines

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.

Quoted 1 line

Doug, any thoughts?

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

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 <[email protected]>

Signed-off-by Adit Ranadive <[email protected]>

Reviewed-by Aditya Sarwade <[email protected]>

---
 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(-)

drivers/infiniband/hw/vmw_pvrdma/pvrdma.h +2 -0

index 540a54b..ee6a941 100644
@@ -69,6 +69,8 @@
  */
 #define PCI_DEVICE_ID_VMWARE_PVRDMA	0x0820
 
+#define PVRDMA_NUM_RING_PAGES		4
+
 struct pvrdma_dev;
 
 struct pvrdma_page_dir {

drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c +2 -2

index b57132f..8e21a86 100644
@@ -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)

drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c +11 -17

index c8c01e5..765bd32 100644
@@ -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();

drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c +0 -0

 	spin_unlock_irqrestore(&qp->rq.lock, flags);

Signature · 6 lines

2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 <[email protected]>

Reported-by Dan Carpenter <[email protected]>

Signed-off-by Adit Ranadive <[email protected]>

Reviewed-by Aditya Sarwade <[email protected]>

---
 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(-)

drivers/infiniband/hw/vmw_pvrdma/pvrdma.h +1 -0

index ee6a941..5dada5a 100644
@@ -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;
 

drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c +5 -4

index 765bd32..3e23425 100644
@@ -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",

drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.c +0 -0

index 5489137..c2aa526 100644

Signature · 7 lines

-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 3 lines · 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.

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

Quoted 7 lines · Yuval Shaia

Quoted 3 lines · 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.

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