[PATCH 1/2] napi_synchronize: waiting for NAPI

[PATCH 1/2] napi_synchronize: waiting for NAPI

[Stephen Hemminger]

[-- Attachment #1: napi-synchronize.patch --]
[-- Type: text/plain, Size: 1039 bytes --]

Some drivers with shared NAPI need a synchronization barrier.
Also suggested by Benjamin Herrenschmidt for EMAC.

Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>

--- a/include/linux/netdevice.h	2007-10-17 08:45:32.000000000 -0700
+++ b/include/linux/netdevice.h	2007-10-17 08:47:54.000000000 -0700
@@ -407,6 +407,24 @@ static inline void napi_enable(struct na
 	clear_bit(NAPI_STATE_SCHED, &n->state);
 }
 
+#ifdef CONFIG_SMP
+/**
+ *	napi_synchronize - wait until NAPI is not running
+ *	@n: napi context
+ *
+ * Wait until NAPI is done being scheduled on this context.
+ * Waits till any outstanding processing completes but
+ * does not disable future activations.
+ */
+static inline void napi_synchronize(const struct napi_struct *n)
+{
+	while (test_bit(NAPI_STATE_SCHED, &n->state))
+		msleep(1);
+}
+#else
+# define napi_synchronize(n)	barrier()
+#endif
+
 /*
  *	The DEVICE structure.
  *	Actually, this whole structure is a big mistake.  It mixes I/O

-- 
Stephen Hemminger <shemminger@linux-foundation.org>

Re: [PATCH 1/2] napi_synchronize: waiting for NAPI

[Jeff Garzik]

applied 1-2

Re: [PATCH 1/2] napi_synchronize: waiting for NAPI

[Benjamin Herrenschmidt]

On Wed, 2007-10-17 at 13:26 -0700, Stephen Hemminger wrote:
> plain text document attachment (napi-synchronize.patch)
> Some drivers with shared NAPI need a synchronization barrier.
> Also suggested by Benjamin Herrenschmidt for EMAC.
> 
> Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>

Just saw this patch, sorry, missed it. Same as you sent me on another
reply, I still think it needs the smp barrier. Can you have a look at
the one I posted today ?

Cheers,
Ben.

Re: [PATCH 1/2] napi_synchronize: waiting for NAPI

[Stephen Hemminger]

On Thu, 18 Oct 2007 10:28:09 +1000
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> 
> On Wed, 2007-10-17 at 13:26 -0700, Stephen Hemminger wrote:
> > plain text document attachment (napi-synchronize.patch)
> > Some drivers with shared NAPI need a synchronization barrier.
> > Also suggested by Benjamin Herrenschmidt for EMAC.
> > 
> > Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>
> 
> Just saw this patch, sorry, missed it. Same as you sent me on another
> reply, I still think it needs the smp barrier. Can you have a look at
> the one I posted today ?
> 
> Cheers,
> Ben.

Could you give a concrete example as to why it needs a smp barrier?
The test_bit is going to get a consistent result. Are you worried about
operations ordering before the test_bit()? In that case, one of those
smp_mb__before_xxx functions is probably what is needed.


-- 
Stephen Hemminger <shemminger@linux-foundation.org>

Re: [PATCH 1/2] napi_synchronize: waiting for NAPI

[Benjamin Herrenschmidt]

On Wed, 2007-10-17 at 18:26 -0700, Stephen Hemminger wrote:
> 
> Could you give a concrete example as to why it needs a smp barrier?
> The test_bit is going to get a consistent result. Are you worried about
> operations ordering before the test_bit()? In that case, one of those
> smp_mb__before_xxx functions is probably what is needed.

test_bit is just a read, without any ordering constraint. It can thus be
totally mixed with anything before.

we want to synchronize. Basically, make sure that whatever we did before
the call is visible to whatever we are synchronizing with (NAPI poll in
our case). That is, in our case, that a previous/concurrent NAPI poll
(that hasn't seen the effect of what we did before) is complete or any
new one will have seen that effect.

thus we need a barrier. At least a read barrier semantic to avoid the
test_bit to dance around and be speculated but I think a full barrier
also making sure that any previous store is visible before we preform
the read is also necessary in pretty much any usage scenario of this
function. I believe this is also a bug in synchronize_irq and possibly
even in napi_disable(). I sent a patch for synchronize_irq() a minute
ago.

Besides, it's not like this was a hot path :-)

Cheers,
Ben.

[PATCH 2/2] sky2: shutdown cleanup

[Stephen Hemminger]

[-- Attachment #1: sky2-shutdown-cleanup.patch --]
[-- Type: text/plain, Size: 4485 bytes --]

Solve issues with dual port devices due to shared NAPI.
 * shutting down one device shouldn't kill other one.
 * suspend shouldn't hang.
Also fix potential race between restart and shutdown.

Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>

--- a/drivers/net/sky2.c	2007-10-17 10:17:56.000000000 -0700
+++ b/drivers/net/sky2.c	2007-10-17 13:12:06.000000000 -0700
@@ -1384,13 +1384,9 @@ static int sky2_up(struct net_device *de
 	sky2_prefetch_init(hw, txqaddr[port], sky2->tx_le_map,
 			   TX_RING_SIZE - 1);
 
-	napi_enable(&hw->napi);
-
 	err = sky2_rx_start(sky2);
-	if (err) {
-		napi_disable(&hw->napi);
+	if (err)
 		goto err_out;
-	}
 
 	/* Enable interrupts from phy/mac for port */
 	imask = sky2_read32(hw, B0_IMSK);
@@ -1679,13 +1675,13 @@ static int sky2_down(struct net_device *
 	/* Stop more packets from being queued */
 	netif_stop_queue(dev);
 
-	napi_disable(&hw->napi);
-
 	/* Disable port IRQ */
 	imask = sky2_read32(hw, B0_IMSK);
 	imask &= ~portirq_msk[port];
 	sky2_write32(hw, B0_IMSK, imask);
 
+	synchronize_irq(hw->pdev->irq);
+
 	sky2_gmac_reset(hw, port);
 
 	/* Stop transmitter */
@@ -1699,6 +1695,9 @@ static int sky2_down(struct net_device *
 	ctrl &= ~(GM_GPCR_TX_ENA | GM_GPCR_RX_ENA);
 	gma_write16(hw, port, GM_GP_CTRL, ctrl);
 
+	/* Make sure no packets are pending */
+	napi_synchronize(&hw->napi);
+
 	sky2_write8(hw, SK_REG(port, GPHY_CTRL), GPC_RST_SET);
 
 	/* Workaround shared GMAC reset */
@@ -1736,8 +1735,6 @@ static int sky2_down(struct net_device *
 	/* turn off LED's */
 	sky2_write16(hw, B0_Y2LED, LED_STAT_OFF);
 
-	synchronize_irq(hw->pdev->irq);
-
 	sky2_tx_clean(dev);
 	sky2_rx_clean(sky2);
 
@@ -2048,9 +2045,6 @@ static int sky2_change_mtu(struct net_de
 	err = sky2_rx_start(sky2);
 	sky2_write32(hw, B0_IMSK, imask);
 
-	/* Unconditionally re-enable NAPI because even if we
-	 * call dev_close() that will do a napi_disable().
-	 */
 	napi_enable(&hw->napi);
 
 	if (err)
@@ -2915,6 +2909,7 @@ static void sky2_restart(struct work_str
 	rtnl_lock();
 	sky2_write32(hw, B0_IMSK, 0);
 	sky2_read32(hw, B0_IMSK);
+	napi_disable(&hw->napi);
 
 	for (i = 0; i < hw->ports; i++) {
 		dev = hw->dev[i];
@@ -2924,6 +2919,7 @@ static void sky2_restart(struct work_str
 
 	sky2_reset(hw);
 	sky2_write32(hw, B0_IMSK, Y2_IS_BASE);
+	napi_enable(&hw->napi);
 
 	for (i = 0; i < hw->ports; i++) {
 		dev = hw->dev[i];
@@ -4191,7 +4187,6 @@ static int __devinit sky2_probe(struct p
 		err = -ENOMEM;
 		goto err_out_free_pci;
 	}
-	netif_napi_add(dev, &hw->napi, sky2_poll, NAPI_WEIGHT);
 
 	if (!disable_msi && pci_enable_msi(pdev) == 0) {
 		err = sky2_test_msi(hw);
@@ -4207,6 +4202,8 @@ static int __devinit sky2_probe(struct p
 		goto err_out_free_netdev;
 	}
 
+	netif_napi_add(dev, &hw->napi, sky2_poll, NAPI_WEIGHT);
+
 	err = request_irq(pdev->irq, sky2_intr,
 			  (hw->flags & SKY2_HW_USE_MSI) ? 0 : IRQF_SHARED,
 			  dev->name, hw);
@@ -4215,6 +4212,7 @@ static int __devinit sky2_probe(struct p
 		goto err_out_unregister;
 	}
 	sky2_write32(hw, B0_IMSK, Y2_IS_BASE);
+	napi_enable(&hw->napi);
 
 	sky2_show_addr(dev);
 
@@ -4265,23 +4263,18 @@ err_out:
 static void __devexit sky2_remove(struct pci_dev *pdev)
 {
 	struct sky2_hw *hw = pci_get_drvdata(pdev);
-	struct net_device *dev0, *dev1;
+	int i;
 
 	if (!hw)
 		return;
 
 	del_timer_sync(&hw->watchdog_timer);
+	cancel_work_sync(&hw->restart_work);
 
-	flush_scheduled_work();
+	for (i = hw->ports; i >= 0; --i)
+		unregister_netdev(hw->dev[i]);
 
 	sky2_write32(hw, B0_IMSK, 0);
-	synchronize_irq(hw->pdev->irq);
-
-	dev0 = hw->dev[0];
-	dev1 = hw->dev[1];
-	if (dev1)
-		unregister_netdev(dev1);
-	unregister_netdev(dev0);
 
 	sky2_power_aux(hw);
 
@@ -4296,9 +4289,9 @@ static void __devexit sky2_remove(struct
 	pci_release_regions(pdev);
 	pci_disable_device(pdev);
 
-	if (dev1)
-		free_netdev(dev1);
-	free_netdev(dev0);
+	for (i = hw->ports; i >= 0; --i)
+		free_netdev(hw->dev[i]);
+
 	iounmap(hw->regs);
 	kfree(hw);
 
@@ -4328,6 +4321,7 @@ static int sky2_suspend(struct pci_dev *
 	}
 
 	sky2_write32(hw, B0_IMSK, 0);
+	napi_disable(&hw->napi);
 	sky2_power_aux(hw);
 
 	pci_save_state(pdev);
@@ -4362,8 +4356,8 @@ static int sky2_resume(struct pci_dev *p
 		pci_write_config_dword(pdev, PCI_DEV_REG3, 0);
 
 	sky2_reset(hw);
-
 	sky2_write32(hw, B0_IMSK, Y2_IS_BASE);
+	napi_enable(&hw->napi);
 
 	for (i = 0; i < hw->ports; i++) {
 		struct net_device *dev = hw->dev[i];

-- 
Stephen Hemminger <shemminger@linux-foundation.org>