Skip to contentSkip to conversation

myri10ge: IRQ and pci state cleanups

PATCH In review Brice Goglin 2 participants 7 messages Dec 18, 2006 – Dec 26, 2006

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Brice Goglin

author
#0 lore raw

cover myri10ge: IRQ and pci state cleanups

Hi Jeff,

While testing Stephen Hemminger's patch to cleanup the save/restore_state in myri10ge, we discovered some problems in our code. First, since the MSI and PCIe state is saved on a kind of stack, we leak some memory by saving the state both for parity recovery and on suspend. Additionaly, some race between the watchdog and the interrupt handler could lead to similar problems.

We fix all this by first matching the number of pci_save_state() and pci_restore_state() (#1), then moving request_irq() to myri10ge_open() (#2) instead of probe() to eliminate some race.
Apart from cleaning the code, this also enables switching from/to MSI at runtime (#3).
Finally (Stephen's patch), we stop saving the MSI and PCIe states in the driver since the PCI layer does it for us (#4), and handle some failure cases in the suspend and resume routines (#5).

1. match number of save_state and restore
2. move request_irq to myri10ge_open
3. make msi configurable at runtime through sysfs
4. no need to save MSI and PCIe state in the driver
5. handle failures in suspend and resume

Thanks,
Brice

author
#1 lore raw

PATCH 1/5 myri10ge: match number of save_state and restore

Since pci_save_state() pushes MSI and PCIe states on a kind of stack, myri10ge saving the state in advance for parity recovery will push the state again on the stack on suspend. This leads to some memory leak. We add a couple additional calls to save_state and restore_state so that we don't leak anymore.

For the future, we are thinking of a better way to recover from parity error without using pci_save_state().

Signed-off-by Brice Goglin <[email protected]>
---
 drivers/net/myri10ge/myri10ge.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
drivers/net/myri10ge/myri10ge.c +10 -2
Index: linux-rc/drivers/net/myri10ge/myri10ge.c
===================================================================
@@ -2641,6 +2641,10 @@
 		 * nic was resumed from power saving mode.
 		 */
 		myri10ge_restore_state(mgp);
+
+		/* save state again for accounting reasons */
+		myri10ge_save_state(mgp);
+
 	} else {
 		/* if we get back -1's from our slot, perhaps somebody
 		 * powered off our card.  Don't try to reset it in

@@ -2907,7 +2911,7 @@
 	status = register_netdev(netdev);
 	if (status != 0) {
 		dev_err(&pdev->dev, "register_netdev failed: %d\n", status);
-		goto abort_with_irq;
+		goto abort_with_state;
 	}
 	dev_info(dev, "%s IRQ %d, tx bndry %d, fw %s, WC %s\n",
 		 (mgp->msi_enabled ? "MSI" : "xPIC"),

@@ -2916,7 +2920,8 @@
 
 	return 0;
 
-abort_with_irq:
+abort_with_state:
+	myri10ge_restore_state(mgp);
 	free_irq(pdev->irq, mgp);
 	if (mgp->msi_enabled)
 		pci_disable_msi(pdev);

@@ -2976,6 +2981,9 @@
 
 	myri10ge_dummy_rdma(mgp, 0);
 
+	/* avoid a memory leak */
+	myri10ge_restore_state(mgp);
+
 	bytes = myri10ge_max_intr_slots * sizeof(*mgp->rx_done.entry);
 	dma_free_coherent(&pdev->dev, bytes,
 			  mgp->rx_done.entry, mgp->rx_done.bus);
#2 lore raw

Re: [PATCH 1/5] myri10ge: match number of save_state and restore

Quoted 10 lines · Brice Goglin

Since pci_save_state() pushes MSI and PCIe states on a kind of stack, myri10ge saving the state in advance for parity recovery will push the state again on the stack on suspend. This leads to some memory leak. We add a couple additional calls to save_state and restore_state so that we don't leak anymore.

For the future, we are thinking of a better way to recover from parity error without using pci_save_state().

Signed-off-by Brice Goglin <[email protected]>

applied 1-5

author
#3 lore raw

PATCH 2/5 myri10ge: move request_irq to myri10ge_open

In reply to Brice Goglin

Request IRQ in myri10ge_open() and free in close() instead of probe() and remove() to eliminate potential race between the watchdog and the interrupt handler. Additionaly, the interrupt handler won't get called on shared irq anymore when the interface is down.

Signed-off-by Brice Goglin <[email protected]>
---
 drivers/net/myri10ge/myri10ge.c |   98 ++++++++++++++++++++++------------------
 1 file changed, 54 insertions(+), 44 deletions(-)
drivers/net/myri10ge/myri10ge.c +54 -44
Index: linux-rc/drivers/net/myri10ge/myri10ge.c
===================================================================
@@ -721,12 +721,10 @@
 	status |=
 	    myri10ge_send_cmd(mgp, MXGEFW_CMD_GET_IRQ_ACK_OFFSET, &cmd, 0);
 	mgp->irq_claim = (__iomem __be32 *) (mgp->sram + cmd.data0);
-	if (!mgp->msi_enabled) {
-		status |= myri10ge_send_cmd
-		    (mgp, MXGEFW_CMD_GET_IRQ_DEASSERT_OFFSET, &cmd, 0);
-		mgp->irq_deassert = (__iomem __be32 *) (mgp->sram + cmd.data0);
+	status |= myri10ge_send_cmd(mgp, MXGEFW_CMD_GET_IRQ_DEASSERT_OFFSET,
+				    &cmd, 0);
+	mgp->irq_deassert = (__iomem __be32 *) (mgp->sram + cmd.data0);
 
-	}
 	status |= myri10ge_send_cmd
 	    (mgp, MXGEFW_CMD_GET_INTR_COAL_DELAY_OFFSET, &cmd, 0);
 	mgp->intr_coal_delay_ptr = (__iomem __be32 *) (mgp->sram + cmd.data0);

@@ -1619,6 +1617,41 @@
 	mgp->tx.req_list = NULL;
 }
 
+static int myri10ge_request_irq(struct myri10ge_priv *mgp)
+{
+	struct pci_dev *pdev = mgp->pdev;
+	int status;
+
+	if (myri10ge_msi) {
+		status = pci_enable_msi(pdev);
+		if (status != 0)
+			dev_err(&pdev->dev,
+				"Error %d setting up MSI; falling back to xPIC\n",
+				status);
+		else
+			mgp->msi_enabled = 1;
+	} else {
+		mgp->msi_enabled = 0;
+	}
+	status = request_irq(pdev->irq, myri10ge_intr, IRQF_SHARED,
+			     mgp->dev->name, mgp);
+	if (status != 0) {
+		dev_err(&pdev->dev, "failed to allocate IRQ\n");
+		if (mgp->msi_enabled)
+			pci_disable_msi(pdev);
+	}
+	return status;
+}
+
+static void myri10ge_free_irq(struct myri10ge_priv *mgp)
+{
+	struct pci_dev *pdev = mgp->pdev;
+
+	free_irq(pdev->irq, mgp);
+	if (mgp->msi_enabled)
+		pci_disable_msi(pdev);
+}
+
 static int myri10ge_open(struct net_device *dev)
 {
 	struct myri10ge_priv *mgp;

@@ -1634,10 +1667,13 @@
 	status = myri10ge_reset(mgp);
 	if (status != 0) {
 		printk(KERN_ERR "myri10ge: %s: failed reset\n", dev->name);
-		mgp->running = MYRI10GE_ETH_STOPPED;
-		return -ENXIO;
+		goto abort_with_nothing;
 	}
 
+	status = myri10ge_request_irq(mgp);
+	if (status != 0)
+		goto abort_with_nothing;
+
 	/* decide what small buffer size to use.  For good TCP rx
 	 * performance, it is important to not receive 1514 byte
 	 * frames into jumbo buffers, as it confuses the socket buffer

@@ -1677,7 +1713,7 @@
 		       "myri10ge: %s: failed to get ring sizes or locations\n",
 		       dev->name);
 		mgp->running = MYRI10GE_ETH_STOPPED;
-		return -ENXIO;
+		goto abort_with_irq;
 	}
 
 	if (mgp->mtrr >= 0) {

@@ -1708,7 +1744,7 @@
 
 	status = myri10ge_allocate_rings(dev);
 	if (status != 0)
-		goto abort_with_nothing;
+		goto abort_with_irq;
 
 	/* now give firmware buffers sizes, and MTU */
 	cmd.data0 = dev->mtu + ETH_HLEN + VLAN_HLEN;

@@ -1771,6 +1807,9 @@
 abort_with_rings:
 	myri10ge_free_rings(dev);
 
+abort_with_irq:
+	myri10ge_free_irq(mgp);
+
 abort_with_nothing:
 	mgp->running = MYRI10GE_ETH_STOPPED;
 	return -ENOMEM;

@@ -1807,7 +1846,7 @@
 		printk(KERN_ERR "myri10ge: %s never got down irq\n", dev->name);
 
 	netif_tx_disable(dev);
-
+	myri10ge_free_irq(mgp);
 	myri10ge_free_rings(dev);
 
 	mgp->running = MYRI10GE_ETH_STOPPED;

@@ -2529,7 +2568,6 @@
 		rtnl_unlock();
 	}
 	myri10ge_dummy_rdma(mgp, 0);
-	free_irq(pdev->irq, mgp);
 	myri10ge_save_state(mgp);
 	pci_disable_device(pdev);
 	pci_set_power_state(pdev, pci_choose_state(pdev, state));

@@ -2565,13 +2603,6 @@
 
 	pci_set_master(pdev);
 
-	status = request_irq(pdev->irq, myri10ge_intr, IRQF_SHARED,
-			     netdev->name, mgp);
-	if (status != 0) {
-		dev_err(&pdev->dev, "failed to allocate IRQ\n");
-		goto abort_with_enabled;
-	}
-
 	myri10ge_reset(mgp);
 	myri10ge_dummy_rdma(mgp, 1);
 

@@ -2581,8 +2612,11 @@
 
 	if (netif_running(netdev)) {
 		rtnl_lock();
-		myri10ge_open(netdev);
+		status = myri10ge_open(netdev);
 		rtnl_unlock();
+		if (status != 0)
+			goto abort_with_enabled;
+
 	}
 	netif_device_attach(netdev);
 

@@ -2860,23 +2894,6 @@
 		goto abort_with_firmware;
 	}
 
-	if (myri10ge_msi) {
-		status = pci_enable_msi(pdev);
-		if (status != 0)
-			dev_err(&pdev->dev,
-				"Error %d setting up MSI; falling back to xPIC\n",
-				status);
-		else
-			mgp->msi_enabled = 1;
-	}
-
-	status = request_irq(pdev->irq, myri10ge_intr, IRQF_SHARED,
-			     netdev->name, mgp);
-	if (status != 0) {
-		dev_err(&pdev->dev, "failed to allocate IRQ\n");
-		goto abort_with_firmware;
-	}
-
 	pci_set_drvdata(pdev, mgp);
 	if ((myri10ge_initial_mtu + ETH_HLEN) > MYRI10GE_MAX_ETHER_MTU)
 		myri10ge_initial_mtu = MYRI10GE_MAX_ETHER_MTU - ETH_HLEN;

@@ -2913,8 +2930,7 @@
 		dev_err(&pdev->dev, "register_netdev failed: %d\n", status);
 		goto abort_with_state;
 	}
-	dev_info(dev, "%s IRQ %d, tx bndry %d, fw %s, WC %s\n",
-		 (mgp->msi_enabled ? "MSI" : "xPIC"),
+	dev_info(dev, "%d, tx bndry %d, fw %s, WC %s\n",
 		 pdev->irq, mgp->tx.boundary, mgp->fw_name,
 		 (mgp->mtrr >= 0 ? "Enabled" : "Disabled"));
 

@@ -2922,9 +2938,6 @@
 
 abort_with_state:
 	myri10ge_restore_state(mgp);
-	free_irq(pdev->irq, mgp);
-	if (mgp->msi_enabled)
-		pci_disable_msi(pdev);
 
 abort_with_firmware:
 	myri10ge_dummy_rdma(mgp, 0);

@@ -2975,9 +2988,6 @@
 	flush_scheduled_work();
 	netdev = mgp->dev;
 	unregister_netdev(netdev);
-	free_irq(pdev->irq, mgp);
-	if (mgp->msi_enabled)
-		pci_disable_msi(pdev);
 
 	myri10ge_dummy_rdma(mgp, 0);
author
#4 lore raw

PATCH 3/5 myri10ge: make msi configurable at runtime through sysfs

In reply to Brice Goglin

Now that IRQ are requested is called on open() and freed on close(), we can safely switch from/to MSI without unloading the module.

We are guaranteed to correctly free IRQ even if the sysfs file got written in the meantime since the MSI initialization is stored in mgp->msi_enabled.

Signed-off-by Brice Goglin <[email protected]>
---
 drivers/net/myri10ge/myri10ge.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/myri10ge/myri10ge.c +1 -1
Index: linux-rc/drivers/net/myri10ge/myri10ge.c
===================================================================
@@ -228,7 +228,7 @@
 MODULE_PARM_DESC(myri10ge_small_bytes, "Threshold of small packets\n");
 
 static int myri10ge_msi = 1;	/* enable msi by default */
-module_param(myri10ge_msi, int, S_IRUGO);
+module_param(myri10ge_msi, int, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(myri10ge_msi, "Enable Message Signalled Interrupts\n");
 
 static int myri10ge_intr_coal_delay = 25;
author
#5 lore raw

PATCH 4/5 myri10ge: no need to save MSI and PCIe state in the driver

In reply to Brice Goglin

The PCI MSI and express state are already saved and restored by the current versions of pci_save_state/pci_restore_state. Therefore it is no longer necessary for the driver to do it.

Signed-off-by Brice Goglin <[email protected]>
Signed-off-by Stephen Hemminger <[email protected]>
---
 drivers/net/myri10ge/myri10ge.c |   47 +++++++---------------------------------
 1 file changed, 9 insertions(+), 38 deletions(-)
drivers/net/myri10ge/myri10ge.c +9 -38
Index: linux-rc/drivers/net/myri10ge/myri10ge.c
===================================================================
@@ -199,8 +199,6 @@
 	unsigned long serial_number;
 	int vendor_specific_offset;
 	int fw_multicast_support;
-	u32 devctl;
-	u16 msi_flags;
 	u32 read_dma;
 	u32 write_dma;
 	u32 read_write_dma;

@@ -2520,34 +2518,6 @@
 	}
 }
 
-static void myri10ge_save_state(struct myri10ge_priv *mgp)
-{
-	struct pci_dev *pdev = mgp->pdev;
-	int cap;
-
-	pci_save_state(pdev);
-	/* now save PCIe and MSI state that Linux will not
-	 * save for us */
-	cap = pci_find_capability(pdev, PCI_CAP_ID_EXP);
-	pci_read_config_dword(pdev, cap + PCI_EXP_DEVCTL, &mgp->devctl);
-	cap = pci_find_capability(pdev, PCI_CAP_ID_MSI);
-	pci_read_config_word(pdev, cap + PCI_MSI_FLAGS, &mgp->msi_flags);
-}
-
-static void myri10ge_restore_state(struct myri10ge_priv *mgp)
-{
-	struct pci_dev *pdev = mgp->pdev;
-	int cap;
-
-	/* restore PCIe and MSI state that linux will not */
-	cap = pci_find_capability(pdev, PCI_CAP_ID_EXP);
-	pci_write_config_dword(pdev, cap + PCI_CAP_ID_EXP, mgp->devctl);
-	cap = pci_find_capability(pdev, PCI_CAP_ID_MSI);
-	pci_write_config_word(pdev, cap + PCI_MSI_FLAGS, mgp->msi_flags);
-
-	pci_restore_state(pdev);
-}
-
 #ifdef CONFIG_PM
 
 static int myri10ge_suspend(struct pci_dev *pdev, pm_message_t state)

@@ -2568,7 +2538,7 @@
 		rtnl_unlock();
 	}
 	myri10ge_dummy_rdma(mgp, 0);
-	myri10ge_save_state(mgp);
+	pci_save_state(pdev);
 	pci_disable_device(pdev);
 	pci_set_power_state(pdev, pci_choose_state(pdev, state));
 	return 0;

@@ -2593,7 +2563,8 @@
 		       mgp->dev->name);
 		return -EIO;
 	}
-	myri10ge_restore_state(mgp);
+
+	pci_restore_state(pdev);
 
 	status = pci_enable_device(pdev);
 	if (status < 0) {

@@ -2608,7 +2579,7 @@
 
 	/* Save configuration space to be restored if the
 	 * nic resets due to a parity error */
-	myri10ge_save_state(mgp);
+	pci_save_state(pdev);
 
 	if (netif_running(netdev)) {
 		rtnl_lock();

@@ -2674,10 +2645,10 @@
 		 * when the driver was loaded, or the last time the
 		 * nic was resumed from power saving mode.
 		 */
-		myri10ge_restore_state(mgp);
+		pci_restore_state(mgp->pdev);
 
 		/* save state again for accounting reasons */
-		myri10ge_save_state(mgp);
+		pci_save_state(mgp->pdev);
 
 	} else {
 		/* if we get back -1's from our slot, perhaps somebody

@@ -2917,7 +2888,7 @@
 
 	/* Save configuration space to be restored if the
 	 * nic resets due to a parity error */
-	myri10ge_save_state(mgp);
+	pci_save_state(pdev);
 
 	/* Setup the watchdog timer */
 	setup_timer(&mgp->watchdog_timer, myri10ge_watchdog_timer,

@@ -2937,7 +2908,7 @@
 	return 0;
 
 abort_with_state:
-	myri10ge_restore_state(mgp);
+	pci_restore_state(pdev);
 
 abort_with_firmware:
 	myri10ge_dummy_rdma(mgp, 0);

@@ -2992,7 +2963,7 @@
 	myri10ge_dummy_rdma(mgp, 0);
 
 	/* avoid a memory leak */
-	myri10ge_restore_state(mgp);
+	pci_restore_state(pdev);
 
 	bytes = myri10ge_max_intr_slots * sizeof(*mgp->rx_done.entry);
 	dma_free_coherent(&pdev->dev, bytes,
author
#6 lore raw

PATCH 5/5 myri10ge: handle failures in suspend and resume

In reply to Brice Goglin

On suspend, handle pci_set_power_state errors, and on resume handle failures in pci_resume_state().

Signed-off-by Brice Goglin <[email protected]>
Signed-off-by Stephen Hemminger <[email protected]>
---
 drivers/net/myri10ge/myri10ge.c |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)
drivers/net/myri10ge/myri10ge.c +7 -5
Index: linux-rc/drivers/net/myri10ge/myri10ge.c
===================================================================
@@ -2540,8 +2540,8 @@
 	myri10ge_dummy_rdma(mgp, 0);
 	pci_save_state(pdev);
 	pci_disable_device(pdev);
-	pci_set_power_state(pdev, pci_choose_state(pdev, state));
-	return 0;
+
+	return pci_set_power_state(pdev, pci_choose_state(pdev, state));
 }
 
 static int myri10ge_resume(struct pci_dev *pdev)

@@ -2564,12 +2564,14 @@
 		return -EIO;
 	}
 
-	pci_restore_state(pdev);
+	status = pci_restore_state(pdev);
+	if (status)
+		return status;
 
 	status = pci_enable_device(pdev);
-	if (status < 0) {
+	if (status) {
 		dev_err(&pdev->dev, "failed to enable device\n");
-		return -EIO;
+		return status;
 	}
 
 	pci_set_master(pdev);