[PATCH 0/5] myri10ge: IRQ and pci state cleanups

[PATCH 0/5] myri10ge: IRQ and pci state cleanups

[Brice Goglin]

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

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

[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 <brice@myri.com>
---
 drivers/net/myri10ge/myri10ge.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Index: linux-rc/drivers/net/myri10ge/myri10ge.c
===================================================================
--- linux-rc.orig/drivers/net/myri10ge/myri10ge.c	2006-12-17 22:12:20.000000000 +0100
+++ linux-rc/drivers/net/myri10ge/myri10ge.c	2006-12-17 22:13:50.000000000 +0100
@@ -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);

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

[Jeff Garzik]

Brice Goglin wrote:
> 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 <brice@myri.com>

applied 1-5

[PATCH 2/5] myri10ge: move request_irq to myri10ge_open

[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 <brice@myri.com>
---
 drivers/net/myri10ge/myri10ge.c |   98 ++++++++++++++++++++++------------------
 1 file changed, 54 insertions(+), 44 deletions(-)

Index: linux-rc/drivers/net/myri10ge/myri10ge.c
===================================================================
--- linux-rc.orig/drivers/net/myri10ge/myri10ge.c	2006-12-17 22:14:10.000000000 +0100
+++ linux-rc/drivers/net/myri10ge/myri10ge.c	2006-12-17 22:14:43.000000000 +0100
@@ -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);
 

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

[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 <brice@myri.com>
---
 drivers/net/myri10ge/myri10ge.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-rc/drivers/net/myri10ge/myri10ge.c
===================================================================
--- linux-rc.orig/drivers/net/myri10ge/myri10ge.c	2006-12-17 22:15:52.000000000 +0100
+++ linux-rc/drivers/net/myri10ge/myri10ge.c	2006-12-17 22:16:47.000000000 +0100
@@ -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;

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

[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 <brice@myri.com>
Signed-off-by: Stephen Hemminger <shemminger@osdl.org>
---
 drivers/net/myri10ge/myri10ge.c |   47 +++++++---------------------------------
 1 file changed, 9 insertions(+), 38 deletions(-)

Index: linux-rc/drivers/net/myri10ge/myri10ge.c
===================================================================
--- linux-rc.orig/drivers/net/myri10ge/myri10ge.c	2006-12-18 10:28:30.000000000 +0100
+++ linux-rc/drivers/net/myri10ge/myri10ge.c	2006-12-18 10:32:05.000000000 +0100
@@ -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,

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

[Brice Goglin]

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

Signed-off-by: Brice Goglin <brice@myri.com>
Signed-off-by: Stephen Hemminger <shemminger@osdl.org>
---
 drivers/net/myri10ge/myri10ge.c |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Index: linux-rc/drivers/net/myri10ge/myri10ge.c
===================================================================
--- linux-rc.orig/drivers/net/myri10ge/myri10ge.c	2006-12-18 10:32:05.000000000 +0100
+++ linux-rc/drivers/net/myri10ge/myri10ge.c	2006-12-18 10:35:13.000000000 +0100
@@ -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);