All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next-2.6 0/2] 3c59x: Locking fixes
@ 2010-06-23 23:52 Ben Hutchings
  2010-06-23 23:54 ` [PATCH net-next-2.6 1/2] 3c59x: Specify window explicitly for access to windowed registers Ben Hutchings
  2010-06-23 23:55 ` [PATCH net-next-2.6 2/2] 3c59x: Use fine-grained locks for MII and windowed register access Ben Hutchings
  0 siblings, 2 replies; 13+ messages in thread
From: Ben Hutchings @ 2010-06-23 23:52 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, Chase Douglas, Arne Nordmark

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

disable_irq() is no longer safe to use in atomic context.  This should
remove the need to use disable_irq() or to hold locks for long periods
in 3c59x.

Ben.

Ben Hutchings (2):
  3c59x: Specify window explicitly for access to windowed registers
  3c59x: Use fine-grained locks for MII and windowed register access

 drivers/net/3c59x.c |  350 ++++++++++++++++++++++++++-------------------------
 1 files changed, 177 insertions(+), 173 deletions(-)



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

^ permalink raw reply	[flat|nested] 13+ messages in thread
* [PATCH net-next-2.6 1/2] 3c59x: Specify window explicitly for access to windowed registers
  2010-06-23 23:52 [PATCH net-next-2.6 0/2] 3c59x: Locking fixes Ben Hutchings
@ 2010-06-23 23:54 ` Ben Hutchings
  2010-06-29  6:20   ` David Miller
  2010-06-23 23:55 ` [PATCH net-next-2.6 2/2] 3c59x: Use fine-grained locks for MII and windowed register access Ben Hutchings
  1 sibling, 1 reply; 13+ messages in thread
From: Ben Hutchings @ 2010-06-23 23:54 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, Chase Douglas, Arne Nordmark

Currently much of the code assumes that a specific window has been
selected, while a few functions save and restore the window.  This
makes it impossible to introduce fine-grained locking.

Make those assumptions explicit by introducing wrapper functions
to set the window and read/write a register.  Use these everywhere
except vortex_interrupt(), vortex_start_xmit() and vortex_rx().
These set the window just once, or not at all in the case of
vortex_rx() as it should always be called from vortex_interrupt().

Cache the current window in struct vortex_private to avoid
unnecessary hardware writes.

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
Tested-by: Arne Nordmark <nordmark@mech.kth.se> [against 2.6.32]
---
 drivers/net/3c59x.c |  288 +++++++++++++++++++++++++--------------------------
 1 files changed, 140 insertions(+), 148 deletions(-)

diff --git a/drivers/net/3c59x.c b/drivers/net/3c59x.c
index d75803e..beddef9 100644
--- a/drivers/net/3c59x.c
+++ b/drivers/net/3c59x.c
@@ -435,7 +435,6 @@ MODULE_DEVICE_TABLE(pci, vortex_pci_tbl);
    First the windows.  There are eight register windows, with the command
    and status registers available in each.
    */
-#define EL3WINDOW(win_num) iowrite16(SelectWindow + (win_num), ioaddr + EL3_CMD)
 #define EL3_CMD 0x0e
 #define EL3_STATUS 0x0e
 
@@ -647,8 +646,35 @@ struct vortex_private {
 	u16 io_size;						/* Size of PCI region (for release_region) */
 	spinlock_t lock;					/* Serialise access to device & its vortex_private */
 	struct mii_if_info mii;				/* MII lib hooks/info */
+	int window;					/* Register window */
 };
 
+static void window_set(struct vortex_private *vp, int window)
+{
+	if (window != vp->window) {
+		iowrite16(SelectWindow + window, vp->ioaddr + EL3_CMD);
+		vp->window = window;
+	}
+}
+
+#define DEFINE_WINDOW_IO(size)						\
+static u ## size							\
+window_read ## size(struct vortex_private *vp, int window, int addr)	\
+{									\
+	window_set(vp, window);						\
+	return ioread ## size(vp->ioaddr + addr);			\
+}									\
+static void								\
+window_write ## size(struct vortex_private *vp, u ## size value,	\
+		     int window, int addr)				\
+{									\
+	window_set(vp, window);						\
+	iowrite ## size(value, vp->ioaddr + addr);			\
+}
+DEFINE_WINDOW_IO(8)
+DEFINE_WINDOW_IO(16)
+DEFINE_WINDOW_IO(32)
+
 #ifdef CONFIG_PCI
 #define DEVICE_PCI(dev) (((dev)->bus == &pci_bus_type) ? to_pci_dev((dev)) : NULL)
 #else
@@ -711,7 +737,7 @@ static int vortex_probe1(struct device *gendev, void __iomem *ioaddr, int irq,
 static int vortex_up(struct net_device *dev);
 static void vortex_down(struct net_device *dev, int final);
 static int vortex_open(struct net_device *dev);
-static void mdio_sync(void __iomem *ioaddr, int bits);
+static void mdio_sync(struct vortex_private *vp, int bits);
 static int mdio_read(struct net_device *dev, int phy_id, int location);
 static void mdio_write(struct net_device *vp, int phy_id, int location, int value);
 static void vortex_timer(unsigned long arg);
@@ -1119,6 +1145,7 @@ static int __devinit vortex_probe1(struct device *gendev,
 	vp->has_nway = (vci->drv_flags & HAS_NWAY) ? 1 : 0;
 	vp->io_size = vci->io_size;
 	vp->card_idx = card_idx;
+	vp->window = -1;
 
 	/* module list only for Compaq device */
 	if (gendev == NULL) {
@@ -1205,7 +1232,6 @@ static int __devinit vortex_probe1(struct device *gendev,
 	vp->mii.force_media = vp->full_duplex;
 	vp->options = option;
 	/* Read the station address from the EEPROM. */
-	EL3WINDOW(0);
 	{
 		int base;
 
@@ -1218,14 +1244,15 @@ static int __devinit vortex_probe1(struct device *gendev,
 
 		for (i = 0; i < 0x40; i++) {
 			int timer;
-			iowrite16(base + i, ioaddr + Wn0EepromCmd);
+			window_write16(vp, base + i, 0, Wn0EepromCmd);
 			/* Pause for at least 162 us. for the read to take place. */
 			for (timer = 10; timer >= 0; timer--) {
 				udelay(162);
-				if ((ioread16(ioaddr + Wn0EepromCmd) & 0x8000) == 0)
+				if ((window_read16(vp, 0, Wn0EepromCmd) &
+				     0x8000) == 0)
 					break;
 			}
-			eeprom[i] = ioread16(ioaddr + Wn0EepromData);
+			eeprom[i] = window_read16(vp, 0, Wn0EepromData);
 		}
 	}
 	for (i = 0; i < 0x18; i++)
@@ -1250,9 +1277,8 @@ static int __devinit vortex_probe1(struct device *gendev,
 		pr_err("*** EEPROM MAC address is invalid.\n");
 		goto free_ring;	/* With every pack */
 	}
-	EL3WINDOW(2);
 	for (i = 0; i < 6; i++)
-		iowrite8(dev->dev_addr[i], ioaddr + i);
+		window_write8(vp, dev->dev_addr[i], 2, i);
 
 	if (print_info)
 		pr_cont(", IRQ %d\n", dev->irq);
@@ -1261,8 +1287,7 @@ static int __devinit vortex_probe1(struct device *gendev,
 		pr_warning(" *** Warning: IRQ %d is unlikely to work! ***\n",
 			   dev->irq);
 
-	EL3WINDOW(4);
-	step = (ioread8(ioaddr + Wn4_NetDiag) & 0x1e) >> 1;
+	step = (window_read8(vp, 4, Wn4_NetDiag) & 0x1e) >> 1;
 	if (print_info) {
 		pr_info("  product code %02x%02x rev %02x.%d date %02d-%02d-%02d\n",
 			eeprom[6]&0xff, eeprom[6]>>8, eeprom[0x14],
@@ -1285,17 +1310,15 @@ static int __devinit vortex_probe1(struct device *gendev,
 				(unsigned long long)pci_resource_start(pdev, 2),
 				vp->cb_fn_base);
 		}
-		EL3WINDOW(2);
 
-		n = ioread16(ioaddr + Wn2_ResetOptions) & ~0x4010;
+		n = window_read16(vp, 2, Wn2_ResetOptions) & ~0x4010;
 		if (vp->drv_flags & INVERT_LED_PWR)
 			n |= 0x10;
 		if (vp->drv_flags & INVERT_MII_PWR)
 			n |= 0x4000;
-		iowrite16(n, ioaddr + Wn2_ResetOptions);
+		window_write16(vp, n, 2, Wn2_ResetOptions);
 		if (vp->drv_flags & WNO_XCVR_PWR) {
-			EL3WINDOW(0);
-			iowrite16(0x0800, ioaddr);
+			window_write16(vp, 0x0800, 0, 0);
 		}
 	}
 
@@ -1313,14 +1336,13 @@ static int __devinit vortex_probe1(struct device *gendev,
 	{
 		static const char * const ram_split[] = {"5:3", "3:1", "1:1", "3:5"};
 		unsigned int config;
-		EL3WINDOW(3);
-		vp->available_media = ioread16(ioaddr + Wn3_Options);
+		vp->available_media = window_read16(vp, 3, Wn3_Options);
 		if ((vp->available_media & 0xff) == 0)		/* Broken 3c916 */
 			vp->available_media = 0x40;
-		config = ioread32(ioaddr + Wn3_Config);
+		config = window_read32(vp, 3, Wn3_Config);
 		if (print_info) {
 			pr_debug("  Internal config register is %4.4x, transceivers %#x.\n",
-				config, ioread16(ioaddr + Wn3_Options));
+				config, window_read16(vp, 3, Wn3_Options));
 			pr_info("  %dK %s-wide RAM %s Rx:Tx split, %s%s interface.\n",
 				   8 << RAM_SIZE(config),
 				   RAM_WIDTH(config) ? "word" : "byte",
@@ -1346,7 +1368,6 @@ static int __devinit vortex_probe1(struct device *gendev,
 	if ((vp->available_media & 0x40) || (vci->drv_flags & HAS_NWAY) ||
 		dev->if_port == XCVR_MII || dev->if_port == XCVR_NWAY) {
 		int phy, phy_idx = 0;
-		EL3WINDOW(4);
 		mii_preamble_required++;
 		if (vp->drv_flags & EXTRA_PREAMBLE)
 			mii_preamble_required++;
@@ -1478,18 +1499,17 @@ static void
 vortex_set_duplex(struct net_device *dev)
 {
 	struct vortex_private *vp = netdev_priv(dev);
-	void __iomem *ioaddr = vp->ioaddr;
 
 	pr_info("%s:  setting %s-duplex.\n",
 		dev->name, (vp->full_duplex) ? "full" : "half");
 
-	EL3WINDOW(3);
 	/* Set the full-duplex bit. */
-	iowrite16(((vp->info1 & 0x8000) || vp->full_duplex ? 0x20 : 0) |
-		 	(vp->large_frames ? 0x40 : 0) |
-			((vp->full_duplex && vp->flow_ctrl && vp->partner_flow_ctrl) ?
-					0x100 : 0),
-			ioaddr + Wn3_MAC_Ctrl);
+	window_write16(vp,
+		       ((vp->info1 & 0x8000) || vp->full_duplex ? 0x20 : 0) |
+		       (vp->large_frames ? 0x40 : 0) |
+		       ((vp->full_duplex && vp->flow_ctrl && vp->partner_flow_ctrl) ?
+			0x100 : 0),
+		       3, Wn3_MAC_Ctrl);
 }
 
 static void vortex_check_media(struct net_device *dev, unsigned int init)
@@ -1529,8 +1549,7 @@ vortex_up(struct net_device *dev)
 	}
 
 	/* Before initializing select the active media port. */
-	EL3WINDOW(3);
-	config = ioread32(ioaddr + Wn3_Config);
+	config = window_read32(vp, 3, Wn3_Config);
 
 	if (vp->media_override != 7) {
 		pr_info("%s: Media override to transceiver %d (%s).\n",
@@ -1577,10 +1596,9 @@ vortex_up(struct net_device *dev)
 	config = BFINS(config, dev->if_port, 20, 4);
 	if (vortex_debug > 6)
 		pr_debug("vortex_up(): writing 0x%x to InternalConfig\n", config);
-	iowrite32(config, ioaddr + Wn3_Config);
+	window_write32(vp, config, 3, Wn3_Config);
 
 	if (dev->if_port == XCVR_MII || dev->if_port == XCVR_NWAY) {
-		EL3WINDOW(4);
 		mii_reg1 = mdio_read(dev, vp->phys[0], MII_BMSR);
 		mii_reg5 = mdio_read(dev, vp->phys[0], MII_LPA);
 		vp->partner_flow_ctrl = ((mii_reg5 & 0x0400) != 0);
@@ -1601,51 +1619,46 @@ vortex_up(struct net_device *dev)
 	iowrite16(SetStatusEnb | 0x00, ioaddr + EL3_CMD);
 
 	if (vortex_debug > 1) {
-		EL3WINDOW(4);
 		pr_debug("%s: vortex_up() irq %d media status %4.4x.\n",
-			   dev->name, dev->irq, ioread16(ioaddr + Wn4_Media));
+			   dev->name, dev->irq, window_read16(vp, 4, Wn4_Media));
 	}
 
 	/* Set the station address and mask in window 2 each time opened. */
-	EL3WINDOW(2);
 	for (i = 0; i < 6; i++)
-		iowrite8(dev->dev_addr[i], ioaddr + i);
+		window_write8(vp, dev->dev_addr[i], 2, i);
 	for (; i < 12; i+=2)
-		iowrite16(0, ioaddr + i);
+		window_write16(vp, 0, 2, i);
 
 	if (vp->cb_fn_base) {
-		unsigned short n = ioread16(ioaddr + Wn2_ResetOptions) & ~0x4010;
+		unsigned short n = window_read16(vp, 2, Wn2_ResetOptions) & ~0x4010;
 		if (vp->drv_flags & INVERT_LED_PWR)
 			n |= 0x10;
 		if (vp->drv_flags & INVERT_MII_PWR)
 			n |= 0x4000;
-		iowrite16(n, ioaddr + Wn2_ResetOptions);
+		window_write16(vp, n, 2, Wn2_ResetOptions);
 	}
 
 	if (dev->if_port == XCVR_10base2)
 		/* Start the thinnet transceiver. We should really wait 50ms...*/
 		iowrite16(StartCoax, ioaddr + EL3_CMD);
 	if (dev->if_port != XCVR_NWAY) {
-		EL3WINDOW(4);
-		iowrite16((ioread16(ioaddr + Wn4_Media) & ~(Media_10TP|Media_SQE)) |
-			 media_tbl[dev->if_port].media_bits, ioaddr + Wn4_Media);
+		window_write16(vp,
+			       (window_read16(vp, 4, Wn4_Media) &
+				~(Media_10TP|Media_SQE)) |
+			       media_tbl[dev->if_port].media_bits,
+			       4, Wn4_Media);
 	}
 
 	/* Switch to the stats window, and clear all stats by reading. */
 	iowrite16(StatsDisable, ioaddr + EL3_CMD);
-	EL3WINDOW(6);
 	for (i = 0; i < 10; i++)
-		ioread8(ioaddr + i);
-	ioread16(ioaddr + 10);
-	ioread16(ioaddr + 12);
+		window_read8(vp, 6, i);
+	window_read16(vp, 6, 10);
+	window_read16(vp, 6, 12);
 	/* New: On the Vortex we must also clear the BadSSD counter. */
-	EL3WINDOW(4);
-	ioread8(ioaddr + 12);
+	window_read8(vp, 4, 12);
 	/* ..and on the Boomerang we enable the extra statistics bits. */
-	iowrite16(0x0040, ioaddr + Wn4_NetDiag);
-
-	/* Switch to register set 7 for normal use. */
-	EL3WINDOW(7);
+	window_write16(vp, 0x0040, 4, Wn4_NetDiag);
 
 	if (vp->full_bus_master_rx) { /* Boomerang bus master. */
 		vp->cur_rx = vp->dirty_rx = 0;
@@ -1763,7 +1776,7 @@ vortex_timer(unsigned long data)
 	void __iomem *ioaddr = vp->ioaddr;
 	int next_tick = 60*HZ;
 	int ok = 0;
-	int media_status, old_window;
+	int media_status;
 
 	if (vortex_debug > 2) {
 		pr_debug("%s: Media selection timer tick happened, %s.\n",
@@ -1772,9 +1785,7 @@ vortex_timer(unsigned long data)
 	}
 
 	disable_irq_lockdep(dev->irq);
-	old_window = ioread16(ioaddr + EL3_CMD) >> 13;
-	EL3WINDOW(4);
-	media_status = ioread16(ioaddr + Wn4_Media);
+	media_status = window_read16(vp, 4, Wn4_Media);
 	switch (dev->if_port) {
 	case XCVR_10baseT:  case XCVR_100baseTx:  case XCVR_100baseFx:
 		if (media_status & Media_LnkBeat) {
@@ -1830,13 +1841,14 @@ vortex_timer(unsigned long data)
 					   dev->name, media_tbl[dev->if_port].name);
 			next_tick = media_tbl[dev->if_port].wait;
 		}
-		iowrite16((media_status & ~(Media_10TP|Media_SQE)) |
-			 media_tbl[dev->if_port].media_bits, ioaddr + Wn4_Media);
+		window_write16(vp,
+			       (media_status & ~(Media_10TP|Media_SQE)) |
+			       media_tbl[dev->if_port].media_bits,
+			       4, Wn4_Media);
 
-		EL3WINDOW(3);
-		config = ioread32(ioaddr + Wn3_Config);
+		config = window_read32(vp, 3, Wn3_Config);
 		config = BFINS(config, dev->if_port, 20, 4);
-		iowrite32(config, ioaddr + Wn3_Config);
+		window_write32(vp, config, 3, Wn3_Config);
 
 		iowrite16(dev->if_port == XCVR_10base2 ? StartCoax : StopCoax,
 			 ioaddr + EL3_CMD);
@@ -1850,7 +1862,6 @@ leave_media_alone:
 	  pr_debug("%s: Media selection timer finished, %s.\n",
 			 dev->name, media_tbl[dev->if_port].name);
 
-	EL3WINDOW(old_window);
 	enable_irq_lockdep(dev->irq);
 	mod_timer(&vp->timer, RUN_AT(next_tick));
 	if (vp->deferred)
@@ -1865,12 +1876,11 @@ static void vortex_tx_timeout(struct net_device *dev)
 	pr_err("%s: transmit timed out, tx_status %2.2x status %4.4x.\n",
 		   dev->name, ioread8(ioaddr + TxStatus),
 		   ioread16(ioaddr + EL3_STATUS));
-	EL3WINDOW(4);
 	pr_err("  diagnostics: net %04x media %04x dma %08x fifo %04x\n",
-			ioread16(ioaddr + Wn4_NetDiag),
-			ioread16(ioaddr + Wn4_Media),
+			window_read16(vp, 4, Wn4_NetDiag),
+			window_read16(vp, 4, Wn4_Media),
 			ioread32(ioaddr + PktStatus),
-			ioread16(ioaddr + Wn4_FIFODiag));
+			window_read16(vp, 4, Wn4_FIFODiag));
 	/* Slight code bloat to be user friendly. */
 	if ((ioread8(ioaddr + TxStatus) & 0x88) == 0x88)
 		pr_err("%s: Transmitter encountered 16 collisions --"
@@ -1917,9 +1927,6 @@ static void vortex_tx_timeout(struct net_device *dev)
 	/* Issue Tx Enable */
 	iowrite16(TxEnable, ioaddr + EL3_CMD);
 	dev->trans_start = jiffies; /* prevent tx timeout */
-
-	/* Switch to register set 7 for normal use. */
-	EL3WINDOW(7);
 }
 
 /*
@@ -1980,10 +1987,10 @@ vortex_error(struct net_device *dev, int status)
 			ioread16(ioaddr + EL3_STATUS) & StatsFull) {
 			pr_warning("%s: Updating statistics failed, disabling "
 				   "stats as an interrupt source.\n", dev->name);
-			EL3WINDOW(5);
-			iowrite16(SetIntrEnb | (ioread16(ioaddr + 10) & ~StatsFull), ioaddr + EL3_CMD);
+			iowrite16(SetIntrEnb |
+				  (window_read16(vp, 5, 10) & ~StatsFull),
+				  ioaddr + EL3_CMD);
 			vp->intr_enable &= ~StatsFull;
-			EL3WINDOW(7);
 			DoneDidThat++;
 		}
 	}
@@ -1993,8 +2000,7 @@ vortex_error(struct net_device *dev, int status)
 	}
 	if (status & HostError) {
 		u16 fifo_diag;
-		EL3WINDOW(4);
-		fifo_diag = ioread16(ioaddr + Wn4_FIFODiag);
+		fifo_diag = window_read16(vp, 4, Wn4_FIFODiag);
 		pr_err("%s: Host error, FIFO diagnostic register %4.4x.\n",
 			   dev->name, fifo_diag);
 		/* Adapter failure requires Tx/Rx reset and reinit. */
@@ -2043,8 +2049,10 @@ vortex_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	if (vp->bus_master) {
 		/* Set the bus-master controller to transfer the packet. */
 		int len = (skb->len + 3) & ~3;
-		iowrite32(vp->tx_skb_dma = pci_map_single(VORTEX_PCI(vp), skb->data, len, PCI_DMA_TODEVICE),
-				ioaddr + Wn7_MasterAddr);
+		vp->tx_skb_dma = pci_map_single(VORTEX_PCI(vp), skb->data, len,
+						PCI_DMA_TODEVICE);
+		window_set(vp, 7);
+		iowrite32(vp->tx_skb_dma, ioaddr + Wn7_MasterAddr);
 		iowrite16(len, ioaddr + Wn7_MasterLen);
 		vp->tx_skb = skb;
 		iowrite16(StartDMADown, ioaddr + EL3_CMD);
@@ -2217,6 +2225,8 @@ vortex_interrupt(int irq, void *dev_id)
 		pr_debug("%s: interrupt, status %4.4x, latency %d ticks.\n",
 			   dev->name, status, ioread8(ioaddr + Timer));
 
+	window_set(vp, 7);
+
 	do {
 		if (vortex_debug > 5)
 				pr_debug("%s: In interrupt loop, status %4.4x.\n",
@@ -2760,54 +2770,46 @@ static struct net_device_stats *vortex_get_stats(struct net_device *dev)
 static void update_stats(void __iomem *ioaddr, struct net_device *dev)
 {
 	struct vortex_private *vp = netdev_priv(dev);
-	int old_window = ioread16(ioaddr + EL3_CMD);
 
-	if (old_window == 0xffff)	/* Chip suspended or ejected. */
-		return;
 	/* Unlike the 3c5x9 we need not turn off stats updates while reading. */
 	/* Switch to the stats window, and read everything. */
-	EL3WINDOW(6);
-	dev->stats.tx_carrier_errors		+= ioread8(ioaddr + 0);
-	dev->stats.tx_heartbeat_errors		+= ioread8(ioaddr + 1);
-	dev->stats.tx_window_errors		+= ioread8(ioaddr + 4);
-	dev->stats.rx_fifo_errors		+= ioread8(ioaddr + 5);
-	dev->stats.tx_packets			+= ioread8(ioaddr + 6);
-	dev->stats.tx_packets			+= (ioread8(ioaddr + 9)&0x30) << 4;
-	/* Rx packets	*/			ioread8(ioaddr + 7);   /* Must read to clear */
+	dev->stats.tx_carrier_errors		+= window_read8(vp, 6, 0);
+	dev->stats.tx_heartbeat_errors		+= window_read8(vp, 6, 1);
+	dev->stats.tx_window_errors		+= window_read8(vp, 6, 4);
+	dev->stats.rx_fifo_errors		+= window_read8(vp, 6, 5);
+	dev->stats.tx_packets			+= window_read8(vp, 6, 6);
+	dev->stats.tx_packets			+= (window_read8(vp, 6, 9) &
+						    0x30) << 4;
+	/* Rx packets	*/			window_read8(vp, 6, 7);   /* Must read to clear */
 	/* Don't bother with register 9, an extension of registers 6&7.
 	   If we do use the 6&7 values the atomic update assumption above
 	   is invalid. */
-	dev->stats.rx_bytes 			+= ioread16(ioaddr + 10);
-	dev->stats.tx_bytes 			+= ioread16(ioaddr + 12);
+	dev->stats.rx_bytes 			+= window_read16(vp, 6, 10);
+	dev->stats.tx_bytes 			+= window_read16(vp, 6, 12);
 	/* Extra stats for get_ethtool_stats() */
-	vp->xstats.tx_multiple_collisions	+= ioread8(ioaddr + 2);
-	vp->xstats.tx_single_collisions         += ioread8(ioaddr + 3);
-	vp->xstats.tx_deferred			+= ioread8(ioaddr + 8);
-	EL3WINDOW(4);
-	vp->xstats.rx_bad_ssd			+= ioread8(ioaddr + 12);
+	vp->xstats.tx_multiple_collisions	+= window_read8(vp, 6, 2);
+	vp->xstats.tx_single_collisions         += window_read8(vp, 6, 3);
+	vp->xstats.tx_deferred			+= window_read8(vp, 6, 8);
+	vp->xstats.rx_bad_ssd			+= window_read8(vp, 4, 12);
 
 	dev->stats.collisions = vp->xstats.tx_multiple_collisions
 		+ vp->xstats.tx_single_collisions
 		+ vp->xstats.tx_max_collisions;
 
 	{
-		u8 up = ioread8(ioaddr + 13);
+		u8 up = window_read8(vp, 4, 13);
 		dev->stats.rx_bytes += (up & 0x0f) << 16;
 		dev->stats.tx_bytes += (up & 0xf0) << 12;
 	}
-
-	EL3WINDOW(old_window >> 13);
 }
 
 static int vortex_nway_reset(struct net_device *dev)
 {
 	struct vortex_private *vp = netdev_priv(dev);
-	void __iomem *ioaddr = vp->ioaddr;
 	unsigned long flags;
 	int rc;
 
 	spin_lock_irqsave(&vp->lock, flags);
-	EL3WINDOW(4);
 	rc = mii_nway_restart(&vp->mii);
 	spin_unlock_irqrestore(&vp->lock, flags);
 	return rc;
@@ -2816,12 +2818,10 @@ static int vortex_nway_reset(struct net_device *dev)
 static int vortex_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 {
 	struct vortex_private *vp = netdev_priv(dev);
-	void __iomem *ioaddr = vp->ioaddr;
 	unsigned long flags;
 	int rc;
 
 	spin_lock_irqsave(&vp->lock, flags);
-	EL3WINDOW(4);
 	rc = mii_ethtool_gset(&vp->mii, cmd);
 	spin_unlock_irqrestore(&vp->lock, flags);
 	return rc;
@@ -2830,12 +2830,10 @@ static int vortex_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 static int vortex_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 {
 	struct vortex_private *vp = netdev_priv(dev);
-	void __iomem *ioaddr = vp->ioaddr;
 	unsigned long flags;
 	int rc;
 
 	spin_lock_irqsave(&vp->lock, flags);
-	EL3WINDOW(4);
 	rc = mii_ethtool_sset(&vp->mii, cmd);
 	spin_unlock_irqrestore(&vp->lock, flags);
 	return rc;
@@ -2930,7 +2928,6 @@ static int vortex_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 {
 	int err;
 	struct vortex_private *vp = netdev_priv(dev);
-	void __iomem *ioaddr = vp->ioaddr;
 	unsigned long flags;
 	pci_power_t state = 0;
 
@@ -2942,7 +2939,6 @@ static int vortex_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 	if(state != 0)
 		pci_set_power_state(VORTEX_PCI(vp), PCI_D0);
 	spin_lock_irqsave(&vp->lock, flags);
-	EL3WINDOW(4);
 	err = generic_mii_ioctl(&vp->mii, if_mii(rq), cmd, NULL);
 	spin_unlock_irqrestore(&vp->lock, flags);
 	if(state != 0)
@@ -2985,8 +2981,6 @@ static void set_rx_mode(struct net_device *dev)
 static void set_8021q_mode(struct net_device *dev, int enable)
 {
 	struct vortex_private *vp = netdev_priv(dev);
-	void __iomem *ioaddr = vp->ioaddr;
-	int old_window = ioread16(ioaddr + EL3_CMD);
 	int mac_ctrl;
 
 	if ((vp->drv_flags&IS_CYCLONE) || (vp->drv_flags&IS_TORNADO)) {
@@ -2997,28 +2991,23 @@ static void set_8021q_mode(struct net_device *dev, int enable)
 		if (enable)
 			max_pkt_size += 4;	/* 802.1Q VLAN tag */
 
-		EL3WINDOW(3);
-		iowrite16(max_pkt_size, ioaddr+Wn3_MaxPktSize);
+		window_write16(vp, max_pkt_size, 3, Wn3_MaxPktSize);
 
 		/* set VlanEtherType to let the hardware checksumming
 		   treat tagged frames correctly */
-		EL3WINDOW(7);
-		iowrite16(VLAN_ETHER_TYPE, ioaddr+Wn7_VlanEtherType);
+		window_write16(vp, VLAN_ETHER_TYPE, 7, Wn7_VlanEtherType);
 	} else {
 		/* on older cards we have to enable large frames */
 
 		vp->large_frames = dev->mtu > 1500 || enable;
 
-		EL3WINDOW(3);
-		mac_ctrl = ioread16(ioaddr+Wn3_MAC_Ctrl);
+		mac_ctrl = window_read16(vp, 3, Wn3_MAC_Ctrl);
 		if (vp->large_frames)
 			mac_ctrl |= 0x40;
 		else
 			mac_ctrl &= ~0x40;
-		iowrite16(mac_ctrl, ioaddr+Wn3_MAC_Ctrl);
+		window_write16(vp, mac_ctrl, 3, Wn3_MAC_Ctrl);
 	}
-
-	EL3WINDOW(old_window);
 }
 #else
 
@@ -3037,7 +3026,10 @@ static void set_8021q_mode(struct net_device *dev, int enable)
 /* The maximum data clock rate is 2.5 Mhz.  The minimum timing is usually
    met by back-to-back PCI I/O cycles, but we insert a delay to avoid
    "overclocking" issues. */
-#define mdio_delay() ioread32(mdio_addr)
+static void mdio_delay(struct vortex_private *vp)
+{
+	window_read32(vp, 4, Wn4_PhysicalMgmt);
+}
 
 #define MDIO_SHIFT_CLK	0x01
 #define MDIO_DIR_WRITE	0x04
@@ -3048,16 +3040,15 @@ static void set_8021q_mode(struct net_device *dev, int enable)
 
 /* Generate the preamble required for initial synchronization and
    a few older transceivers. */
-static void mdio_sync(void __iomem *ioaddr, int bits)
+static void mdio_sync(struct vortex_private *vp, int bits)
 {
-	void __iomem *mdio_addr = ioaddr + Wn4_PhysicalMgmt;
-
 	/* Establish sync by sending at least 32 logic ones. */
 	while (-- bits >= 0) {
-		iowrite16(MDIO_DATA_WRITE1, mdio_addr);
-		mdio_delay();
-		iowrite16(MDIO_DATA_WRITE1 | MDIO_SHIFT_CLK, mdio_addr);
-		mdio_delay();
+		window_write16(vp, MDIO_DATA_WRITE1, 4, Wn4_PhysicalMgmt);
+		mdio_delay(vp);
+		window_write16(vp, MDIO_DATA_WRITE1 | MDIO_SHIFT_CLK,
+			       4, Wn4_PhysicalMgmt);
+		mdio_delay(vp);
 	}
 }
 
@@ -3065,29 +3056,31 @@ static int mdio_read(struct net_device *dev, int phy_id, int location)
 {
 	int i;
 	struct vortex_private *vp = netdev_priv(dev);
-	void __iomem *ioaddr = vp->ioaddr;
 	int read_cmd = (0xf6 << 10) | (phy_id << 5) | location;
 	unsigned int retval = 0;
-	void __iomem *mdio_addr = ioaddr + Wn4_PhysicalMgmt;
 
 	if (mii_preamble_required)
-		mdio_sync(ioaddr, 32);
+		mdio_sync(vp, 32);
 
 	/* Shift the read command bits out. */
 	for (i = 14; i >= 0; i--) {
 		int dataval = (read_cmd&(1<<i)) ? MDIO_DATA_WRITE1 : MDIO_DATA_WRITE0;
-		iowrite16(dataval, mdio_addr);
-		mdio_delay();
-		iowrite16(dataval | MDIO_SHIFT_CLK, mdio_addr);
-		mdio_delay();
+		window_write16(vp, dataval, 4, Wn4_PhysicalMgmt);
+		mdio_delay(vp);
+		window_write16(vp, dataval | MDIO_SHIFT_CLK,
+			       4, Wn4_PhysicalMgmt);
+		mdio_delay(vp);
 	}
 	/* Read the two transition, 16 data, and wire-idle bits. */
 	for (i = 19; i > 0; i--) {
-		iowrite16(MDIO_ENB_IN, mdio_addr);
-		mdio_delay();
-		retval = (retval << 1) | ((ioread16(mdio_addr) & MDIO_DATA_READ) ? 1 : 0);
-		iowrite16(MDIO_ENB_IN | MDIO_SHIFT_CLK, mdio_addr);
-		mdio_delay();
+		window_write16(vp, MDIO_ENB_IN, 4, Wn4_PhysicalMgmt);
+		mdio_delay(vp);
+		retval = (retval << 1) |
+			((window_read16(vp, 4, Wn4_PhysicalMgmt) &
+			  MDIO_DATA_READ) ? 1 : 0);
+		window_write16(vp, MDIO_ENB_IN | MDIO_SHIFT_CLK,
+			       4, Wn4_PhysicalMgmt);
+		mdio_delay(vp);
 	}
 	return retval & 0x20000 ? 0xffff : retval>>1 & 0xffff;
 }
@@ -3095,28 +3088,28 @@ static int mdio_read(struct net_device *dev, int phy_id, int location)
 static void mdio_write(struct net_device *dev, int phy_id, int location, int value)
 {
 	struct vortex_private *vp = netdev_priv(dev);
-	void __iomem *ioaddr = vp->ioaddr;
 	int write_cmd = 0x50020000 | (phy_id << 23) | (location << 18) | value;
-	void __iomem *mdio_addr = ioaddr + Wn4_PhysicalMgmt;
 	int i;
 
 	if (mii_preamble_required)
-		mdio_sync(ioaddr, 32);
+		mdio_sync(vp, 32);
 
 	/* Shift the command bits out. */
 	for (i = 31; i >= 0; i--) {
 		int dataval = (write_cmd&(1<<i)) ? MDIO_DATA_WRITE1 : MDIO_DATA_WRITE0;
-		iowrite16(dataval, mdio_addr);
-		mdio_delay();
-		iowrite16(dataval | MDIO_SHIFT_CLK, mdio_addr);
-		mdio_delay();
+		window_write16(vp, dataval, 4, Wn4_PhysicalMgmt);
+		mdio_delay(vp);
+		window_write16(vp, dataval | MDIO_SHIFT_CLK,
+			       4, Wn4_PhysicalMgmt);
+		mdio_delay(vp);
 	}
 	/* Leave the interface idle. */
 	for (i = 1; i >= 0; i--) {
-		iowrite16(MDIO_ENB_IN, mdio_addr);
-		mdio_delay();
-		iowrite16(MDIO_ENB_IN | MDIO_SHIFT_CLK, mdio_addr);
-		mdio_delay();
+		window_write16(vp, MDIO_ENB_IN, 4, Wn4_PhysicalMgmt);
+		mdio_delay(vp);
+		window_write16(vp, MDIO_ENB_IN | MDIO_SHIFT_CLK,
+			       4, Wn4_PhysicalMgmt);
+		mdio_delay(vp);
 	}
 }
 
@@ -3131,8 +3124,7 @@ static void acpi_set_WOL(struct net_device *dev)
 
 	if (vp->enable_wol) {
 		/* Power up on: 1==Downloaded Filter, 2==Magic Packets, 4==Link Status. */
-		EL3WINDOW(7);
-		iowrite16(2, ioaddr + 0x0c);
+		window_write16(vp, 2, 7, 0x0c);
 		/* The RxFilter must accept the WOL frames. */
 		iowrite16(SetRxFilter|RxStation|RxMulticast|RxBroadcast, ioaddr + EL3_CMD);
 		iowrite16(RxEnable, ioaddr + EL3_CMD);
-- 
1.7.1




^ permalink raw reply related	[flat|nested] 13+ messages in thread
* [PATCH net-next-2.6 2/2] 3c59x: Use fine-grained locks for MII and windowed register access
  2010-06-23 23:52 [PATCH net-next-2.6 0/2] 3c59x: Locking fixes Ben Hutchings
  2010-06-23 23:54 ` [PATCH net-next-2.6 1/2] 3c59x: Specify window explicitly for access to windowed registers Ben Hutchings
@ 2010-06-23 23:55 ` Ben Hutchings
  2010-06-24 12:05   ` Steffen Klassert
  1 sibling, 1 reply; 13+ messages in thread
From: Ben Hutchings @ 2010-06-23 23:55 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, Chase Douglas, Arne Nordmark

This avoids scheduling in atomic context and also means that IRQs
will only be deferred for relatively short periods of time.

Previously discussed in:
http://article.gmane.org/gmane.linux.network/155024

Reported-by: Arne Nordmark <nordmark@mech.kth.se>
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
Tested-by: Arne Nordmark <nordmark@mech.kth.se> [against 2.6.32]
---
 drivers/net/3c59x.c |   66 ++++++++++++++++++++++++++++++---------------------
 1 files changed, 39 insertions(+), 27 deletions(-)

diff --git a/drivers/net/3c59x.c b/drivers/net/3c59x.c
index beddef9..f4a3fb1 100644
--- a/drivers/net/3c59x.c
+++ b/drivers/net/3c59x.c
@@ -644,9 +644,15 @@ struct vortex_private {
 	u16 deferred;						/* Resend these interrupts when we
 										 * bale from the ISR */
 	u16 io_size;						/* Size of PCI region (for release_region) */
-	spinlock_t lock;					/* Serialise access to device & its vortex_private */
-	struct mii_if_info mii;				/* MII lib hooks/info */
-	int window;					/* Register window */
+
+	/* Serialises access to hardware other than MII and variables below.
+	 * The lock hierarchy is rtnl_lock > lock > mii_lock > window_lock. */
+	spinlock_t lock;
+
+	spinlock_t mii_lock;		/* Serialises access to MII */
+	struct mii_if_info mii;		/* MII lib hooks/info */
+	spinlock_t window_lock;		/* Serialises access to windowed regs */
+	int window;			/* Register window */
 };
 
 static void window_set(struct vortex_private *vp, int window)
@@ -661,15 +667,23 @@ static void window_set(struct vortex_private *vp, int window)
 static u ## size							\
 window_read ## size(struct vortex_private *vp, int window, int addr)	\
 {									\
+	unsigned long flags;						\
+	u ## size ret;							\
+	spin_lock_irqsave(&vp->window_lock, flags);			\
 	window_set(vp, window);						\
-	return ioread ## size(vp->ioaddr + addr);			\
+	ret = ioread ## size(vp->ioaddr + addr);			\
+	spin_unlock_irqrestore(&vp->window_lock, flags);		\
+	return ret;							\
 }									\
 static void								\
 window_write ## size(struct vortex_private *vp, u ## size value,	\
 		     int window, int addr)				\
 {									\
+	unsigned long flags;						\
+	spin_lock_irqsave(&vp->window_lock, flags);			\
 	window_set(vp, window);						\
 	iowrite ## size(value, vp->ioaddr + addr);			\
+	spin_unlock_irqrestore(&vp->window_lock, flags);		\
 }
 DEFINE_WINDOW_IO(8)
 DEFINE_WINDOW_IO(16)
@@ -1784,7 +1798,6 @@ vortex_timer(unsigned long data)
 		pr_debug("dev->watchdog_timeo=%d\n", dev->watchdog_timeo);
 	}
 
-	disable_irq_lockdep(dev->irq);
 	media_status = window_read16(vp, 4, Wn4_Media);
 	switch (dev->if_port) {
 	case XCVR_10baseT:  case XCVR_100baseTx:  case XCVR_100baseFx:
@@ -1805,10 +1818,7 @@ vortex_timer(unsigned long data)
 	case XCVR_MII: case XCVR_NWAY:
 		{
 			ok = 1;
-			/* Interrupts are already disabled */
-			spin_lock(&vp->lock);
 			vortex_check_media(dev, 0);
-			spin_unlock(&vp->lock);
 		}
 		break;
 	  default:					/* Other media types handled by Tx timeouts. */
@@ -1827,6 +1837,8 @@ vortex_timer(unsigned long data)
 	if (!ok) {
 		unsigned int config;
 
+		spin_lock_irq(&vp->lock);
+
 		do {
 			dev->if_port = media_tbl[dev->if_port].next;
 		} while ( ! (vp->available_media & media_tbl[dev->if_port].mask));
@@ -1855,6 +1867,8 @@ vortex_timer(unsigned long data)
 		if (vortex_debug > 1)
 			pr_debug("wrote 0x%08x to Wn3_Config\n", config);
 		/* AKPM: FIXME: Should reset Rx & Tx here.  P60 of 3c90xc.pdf */
+
+		spin_unlock_irq(&vp->lock);
 	}
 
 leave_media_alone:
@@ -1862,7 +1876,6 @@ leave_media_alone:
 	  pr_debug("%s: Media selection timer finished, %s.\n",
 			 dev->name, media_tbl[dev->if_port].name);
 
-	enable_irq_lockdep(dev->irq);
 	mod_timer(&vp->timer, RUN_AT(next_tick));
 	if (vp->deferred)
 		iowrite16(FakeIntr, ioaddr + EL3_CMD);
@@ -2051,9 +2064,11 @@ vortex_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		int len = (skb->len + 3) & ~3;
 		vp->tx_skb_dma = pci_map_single(VORTEX_PCI(vp), skb->data, len,
 						PCI_DMA_TODEVICE);
+		spin_lock_irq(&vp->window_lock);
 		window_set(vp, 7);
 		iowrite32(vp->tx_skb_dma, ioaddr + Wn7_MasterAddr);
 		iowrite16(len, ioaddr + Wn7_MasterLen);
+		spin_unlock_irq(&vp->window_lock);
 		vp->tx_skb = skb;
 		iowrite16(StartDMADown, ioaddr + EL3_CMD);
 		/* netif_wake_queue() will be called at the DMADone interrupt. */
@@ -2225,6 +2240,7 @@ vortex_interrupt(int irq, void *dev_id)
 		pr_debug("%s: interrupt, status %4.4x, latency %d ticks.\n",
 			   dev->name, status, ioread8(ioaddr + Timer));
 
+	spin_lock(&vp->window_lock);
 	window_set(vp, 7);
 
 	do {
@@ -2285,6 +2301,8 @@ vortex_interrupt(int irq, void *dev_id)
 		iowrite16(AckIntr | IntReq | IntLatch, ioaddr + EL3_CMD);
 	} while ((status = ioread16(ioaddr + EL3_STATUS)) & (IntLatch | RxComplete));
 
+	spin_unlock(&vp->window_lock);
+
 	if (vortex_debug > 4)
 		pr_debug("%s: exiting interrupt, status %4.4x.\n",
 			   dev->name, status);
@@ -2806,37 +2824,22 @@ static void update_stats(void __iomem *ioaddr, struct net_device *dev)
 static int vortex_nway_reset(struct net_device *dev)
 {
 	struct vortex_private *vp = netdev_priv(dev);
-	unsigned long flags;
-	int rc;
 
-	spin_lock_irqsave(&vp->lock, flags);
-	rc = mii_nway_restart(&vp->mii);
-	spin_unlock_irqrestore(&vp->lock, flags);
-	return rc;
+	return mii_nway_restart(&vp->mii);
 }
 
 static int vortex_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 {
 	struct vortex_private *vp = netdev_priv(dev);
-	unsigned long flags;
-	int rc;
 
-	spin_lock_irqsave(&vp->lock, flags);
-	rc = mii_ethtool_gset(&vp->mii, cmd);
-	spin_unlock_irqrestore(&vp->lock, flags);
-	return rc;
+	return mii_ethtool_gset(&vp->mii, cmd);
 }
 
 static int vortex_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 {
 	struct vortex_private *vp = netdev_priv(dev);
-	unsigned long flags;
-	int rc;
 
-	spin_lock_irqsave(&vp->lock, flags);
-	rc = mii_ethtool_sset(&vp->mii, cmd);
-	spin_unlock_irqrestore(&vp->lock, flags);
-	return rc;
+	return mii_ethtool_sset(&vp->mii, cmd);
 }
 
 static u32 vortex_get_msglevel(struct net_device *dev)
@@ -3059,6 +3062,8 @@ static int mdio_read(struct net_device *dev, int phy_id, int location)
 	int read_cmd = (0xf6 << 10) | (phy_id << 5) | location;
 	unsigned int retval = 0;
 
+	spin_lock_bh(&vp->mii_lock);
+
 	if (mii_preamble_required)
 		mdio_sync(vp, 32);
 
@@ -3082,6 +3087,9 @@ static int mdio_read(struct net_device *dev, int phy_id, int location)
 			       4, Wn4_PhysicalMgmt);
 		mdio_delay(vp);
 	}
+
+	spin_unlock_bh(&vp->mii_lock);
+
 	return retval & 0x20000 ? 0xffff : retval>>1 & 0xffff;
 }
 
@@ -3091,6 +3099,8 @@ static void mdio_write(struct net_device *dev, int phy_id, int location, int val
 	int write_cmd = 0x50020000 | (phy_id << 23) | (location << 18) | value;
 	int i;
 
+	spin_lock_bh(&vp->mii_lock);
+
 	if (mii_preamble_required)
 		mdio_sync(vp, 32);
 
@@ -3111,6 +3121,8 @@ static void mdio_write(struct net_device *dev, int phy_id, int location, int val
 			       4, Wn4_PhysicalMgmt);
 		mdio_delay(vp);
 	}
+
+	spin_unlock_bh(&vp->mii_lock);
 }
 
 /* ACPI: Advanced Configuration and Power Interface. */
-- 
1.7.1



^ permalink raw reply related	[flat|nested] 13+ messages in thread
* Re: [PATCH net-next-2.6 2/2] 3c59x: Use fine-grained locks for MII and windowed register access
  2010-06-23 23:55 ` [PATCH net-next-2.6 2/2] 3c59x: Use fine-grained locks for MII and windowed register access Ben Hutchings
@ 2010-06-24 12:05   ` Steffen Klassert
  2010-06-24 12:57     ` Ben Hutchings
  0 siblings, 1 reply; 13+ messages in thread
From: Steffen Klassert @ 2010-06-24 12:05 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: David Miller, netdev, Chase Douglas, Arne Nordmark

Hi.

On Thu, Jun 24, 2010 at 12:55:41AM +0100, Ben Hutchings wrote:
> This avoids scheduling in atomic context and also means that IRQs
> will only be deferred for relatively short periods of time.
> 
> Previously discussed in:
> http://article.gmane.org/gmane.linux.network/155024
> 
> Reported-by: Arne Nordmark <nordmark@mech.kth.se>
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> Tested-by: Arne Nordmark <nordmark@mech.kth.se> [against 2.6.32]
> ---
>  drivers/net/3c59x.c |   66 ++++++++++++++++++++++++++++++---------------------
>  1 files changed, 39 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/net/3c59x.c b/drivers/net/3c59x.c
> index beddef9..f4a3fb1 100644
> --- a/drivers/net/3c59x.c
> +++ b/drivers/net/3c59x.c
> @@ -644,9 +644,15 @@ struct vortex_private {
>  	u16 deferred;						/* Resend these interrupts when we
>  										 * bale from the ISR */
>  	u16 io_size;						/* Size of PCI region (for release_region) */
> -	spinlock_t lock;					/* Serialise access to device & its vortex_private */
> -	struct mii_if_info mii;				/* MII lib hooks/info */
> -	int window;					/* Register window */
> +
> +	/* Serialises access to hardware other than MII and variables below.
> +	 * The lock hierarchy is rtnl_lock > lock > mii_lock > window_lock. */
> +	spinlock_t lock;
> +
> +	spinlock_t mii_lock;		/* Serialises access to MII */
> +	struct mii_if_info mii;		/* MII lib hooks/info */
> +	spinlock_t window_lock;		/* Serialises access to windowed regs */

You should initialize the new locks properly with spin_lock_init().

> +	int window;			/* Register window */
>  };
>  
>  static void window_set(struct vortex_private *vp, int window)
> @@ -661,15 +667,23 @@ static void window_set(struct vortex_private *vp, int window)
>  static u ## size							\
>  window_read ## size(struct vortex_private *vp, int window, int addr)	\
>  {									\
> +	unsigned long flags;						\
> +	u ## size ret;							\
> +	spin_lock_irqsave(&vp->window_lock, flags);			\
>  	window_set(vp, window);						\
> -	return ioread ## size(vp->ioaddr + addr);			\
> +	ret = ioread ## size(vp->ioaddr + addr);			\
> +	spin_unlock_irqrestore(&vp->window_lock, flags);		\
> +	return ret;							\
>  }									\
>  static void								\
>  window_write ## size(struct vortex_private *vp, u ## size value,	\
>  		     int window, int addr)				\
>  {									\
> +	unsigned long flags;						\
> +	spin_lock_irqsave(&vp->window_lock, flags);			\
>  	window_set(vp, window);						\
>  	iowrite ## size(value, vp->ioaddr + addr);			\
> +	spin_unlock_irqrestore(&vp->window_lock, flags);		\
>  }

This adds a lot of calls to spin_lock_irqsave/spin_unlock_irqrestore to many
places where this is not necessary at all. For example during device probe and
device open, window_read/window_write are called multiple times, each time
disabling the interrupts. I'd suggest to have unlocked, locked and irqsave
versions of window_read/window_write and use them in appropriate places.

>  DEFINE_WINDOW_IO(8)
>  DEFINE_WINDOW_IO(16)
> @@ -1784,7 +1798,6 @@ vortex_timer(unsigned long data)
>  		pr_debug("dev->watchdog_timeo=%d\n", dev->watchdog_timeo);
>  	}
>  
> -	disable_irq_lockdep(dev->irq);
>  	media_status = window_read16(vp, 4, Wn4_Media);
>  	switch (dev->if_port) {
>  	case XCVR_10baseT:  case XCVR_100baseTx:  case XCVR_100baseFx:
> @@ -1805,10 +1818,7 @@ vortex_timer(unsigned long data)
>  	case XCVR_MII: case XCVR_NWAY:
>  		{
>  			ok = 1;
> -			/* Interrupts are already disabled */
> -			spin_lock(&vp->lock);
>  			vortex_check_media(dev, 0);
> -			spin_unlock(&vp->lock);
>  		}
>  		break;
>  	  default:					/* Other media types handled by Tx timeouts. */
> @@ -1827,6 +1837,8 @@ vortex_timer(unsigned long data)
>  	if (!ok) {
>  		unsigned int config;
>  
> +		spin_lock_irq(&vp->lock);

This can still happen every 5 seconds if the NIC has no link beat and
medialock is not set. So what about defering this locked codepath to
a workqueue, or moving the whole vortex_timer to a delayed workqueue?
In this case we don't need to disable all the interrups on the cpu, we
could still use disable_irq then.

The rest looks quite good to me.

Thanks,

Steffen

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH net-next-2.6 2/2] 3c59x: Use fine-grained locks for MII and windowed register access
  2010-06-24 12:05   ` Steffen Klassert
@ 2010-06-24 12:57     ` Ben Hutchings
  2010-06-24 14:00       ` Steffen Klassert
  0 siblings, 1 reply; 13+ messages in thread
From: Ben Hutchings @ 2010-06-24 12:57 UTC (permalink / raw)
  To: Steffen Klassert; +Cc: David Miller, netdev, Chase Douglas, Arne Nordmark

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

On Thu, 2010-06-24 at 14:05 +0200, Steffen Klassert wrote:
> Hi.
> 
> On Thu, Jun 24, 2010 at 12:55:41AM +0100, Ben Hutchings wrote:
> > This avoids scheduling in atomic context and also means that IRQs
> > will only be deferred for relatively short periods of time.
> > 
> > Previously discussed in:
> > http://article.gmane.org/gmane.linux.network/155024
> > 
> > Reported-by: Arne Nordmark <nordmark@mech.kth.se>
> > Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> > Tested-by: Arne Nordmark <nordmark@mech.kth.se> [against 2.6.32]
> > ---
> >  drivers/net/3c59x.c |   66 ++++++++++++++++++++++++++++++---------------------
> >  1 files changed, 39 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/net/3c59x.c b/drivers/net/3c59x.c
> > index beddef9..f4a3fb1 100644
> > --- a/drivers/net/3c59x.c
> > +++ b/drivers/net/3c59x.c
> > @@ -644,9 +644,15 @@ struct vortex_private {
> >  	u16 deferred;						/* Resend these interrupts when we
> >  										 * bale from the ISR */
> >  	u16 io_size;						/* Size of PCI region (for release_region) */
> > -	spinlock_t lock;					/* Serialise access to device & its vortex_private */
> > -	struct mii_if_info mii;				/* MII lib hooks/info */
> > -	int window;					/* Register window */
> > +
> > +	/* Serialises access to hardware other than MII and variables below.
> > +	 * The lock hierarchy is rtnl_lock > lock > mii_lock > window_lock. */
> > +	spinlock_t lock;
> > +
> > +	spinlock_t mii_lock;		/* Serialises access to MII */
> > +	struct mii_if_info mii;		/* MII lib hooks/info */
> > +	spinlock_t window_lock;		/* Serialises access to windowed regs */
> 
> You should initialize the new locks properly with spin_lock_init().

Oops, yes, obviously.

> > +	int window;			/* Register window */
> >  };
> >  
> >  static void window_set(struct vortex_private *vp, int window)
> > @@ -661,15 +667,23 @@ static void window_set(struct vortex_private *vp, int window)
> >  static u ## size							\
> >  window_read ## size(struct vortex_private *vp, int window, int addr)	\
> >  {									\
> > +	unsigned long flags;						\
> > +	u ## size ret;							\
> > +	spin_lock_irqsave(&vp->window_lock, flags);			\
> >  	window_set(vp, window);						\
> > -	return ioread ## size(vp->ioaddr + addr);			\
> > +	ret = ioread ## size(vp->ioaddr + addr);			\
> > +	spin_unlock_irqrestore(&vp->window_lock, flags);		\
> > +	return ret;							\
> >  }									\
> >  static void								\
> >  window_write ## size(struct vortex_private *vp, u ## size value,	\
> >  		     int window, int addr)				\
> >  {									\
> > +	unsigned long flags;						\
> > +	spin_lock_irqsave(&vp->window_lock, flags);			\
> >  	window_set(vp, window);						\
> >  	iowrite ## size(value, vp->ioaddr + addr);			\
> > +	spin_unlock_irqrestore(&vp->window_lock, flags);		\
> >  }
> 
> This adds a lot of calls to spin_lock_irqsave/spin_unlock_irqrestore to many
> places where this is not necessary at all. For example during device probe and
> device open, window_read/window_write are called multiple times, each time
> disabling the interrupts. I'd suggest to have unlocked, locked and irqsave
> versions of window_read/window_write and use them in appropriate places.

So what?  These are not speed-critical.  The fast-path functions do
acquire the lock just once.

> >  DEFINE_WINDOW_IO(8)
> >  DEFINE_WINDOW_IO(16)
> > @@ -1784,7 +1798,6 @@ vortex_timer(unsigned long data)
> >  		pr_debug("dev->watchdog_timeo=%d\n", dev->watchdog_timeo);
> >  	}
> >  
> > -	disable_irq_lockdep(dev->irq);
> >  	media_status = window_read16(vp, 4, Wn4_Media);
> >  	switch (dev->if_port) {
> >  	case XCVR_10baseT:  case XCVR_100baseTx:  case XCVR_100baseFx:
> > @@ -1805,10 +1818,7 @@ vortex_timer(unsigned long data)
> >  	case XCVR_MII: case XCVR_NWAY:
> >  		{
> >  			ok = 1;
> > -			/* Interrupts are already disabled */
> > -			spin_lock(&vp->lock);
> >  			vortex_check_media(dev, 0);
> > -			spin_unlock(&vp->lock);
> >  		}
> >  		break;
> >  	  default:					/* Other media types handled by Tx timeouts. */
> > @@ -1827,6 +1837,8 @@ vortex_timer(unsigned long data)
> >  	if (!ok) {
> >  		unsigned int config;
> >  
> > +		spin_lock_irq(&vp->lock);
> 
> This can still happen every 5 seconds if the NIC has no link beat and
> medialock is not set. So what about defering this locked codepath to
> a workqueue, or moving the whole vortex_timer to a delayed workqueue?
> In this case we don't need to disable all the interrups on the cpu, we
> could still use disable_irq then.

This locked section is now very short - 5 or 6 register read/writes and
no delays.  We might even be able to get away without locking here as
the only software state this accesses is dev->if_port and I don't think
it can race with anything except SIOCGIFMAP (which seems harmless).

Ben.

> The rest looks quite good to me.
> 
> Thanks,
> 
> Steffen
> 

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH net-next-2.6 2/2] 3c59x: Use fine-grained locks for MII and windowed register access
  2010-06-24 12:57     ` Ben Hutchings
@ 2010-06-24 14:00       ` Steffen Klassert
  2010-06-25  0:45         ` Ben Hutchings
  0 siblings, 1 reply; 13+ messages in thread
From: Steffen Klassert @ 2010-06-24 14:00 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: David Miller, netdev, Chase Douglas, Arne Nordmark

On Thu, Jun 24, 2010 at 01:57:19PM +0100, Ben Hutchings wrote:
> > 
> > This adds a lot of calls to spin_lock_irqsave/spin_unlock_irqrestore to many
> > places where this is not necessary at all. For example during device probe and
> > device open, window_read/window_write are called multiple times, each time
> > disabling the interrupts. I'd suggest to have unlocked, locked and irqsave
> > versions of window_read/window_write and use them in appropriate places.
> 
> So what?  These are not speed-critical.  The fast-path functions do
> acquire the lock just once.
> 

The point is that we should not disable the interrupts if we don't need to
do so. It is not speed critical for the 3c59x driver but disabling the
interrupts should be avoided whenever possible. For example during device
probe and device open we can't race against an interrupt handler because
the device is not yet running.

An example from vortex_probe1() is:

for (i = 0; i < 6; i++)
	window_write8(vp, dev->dev_addr[i], 2, i);

which expands to someting like:

for (i = 0; i < 6; i++) {
	unsigned long flags;
	spin_lock_irqsave(&vp->window_lock, flags);
	window_set(vp, window);
	iowrite8(dev->dev_addr[i], vp->ioaddr  + i);
	spin_unlock_irqrestore(&vp->window_lock, flags);
	return ret;
}

which is quite odd in a codepath that could simply do:

for (i = 0; i < 6; i++) {
	window_set(vp, window);
	iowrite8(dev->dev_addr[i], vp->ioaddr + i);
}

> 
> This locked section is now very short - 5 or 6 register read/writes and
> no delays.  We might even be able to get away without locking here as
> the only software state this accesses is dev->if_port and I don't think
> it can race with anything except SIOCGIFMAP (which seems harmless).
> 

Best would be, if we don't need to disable the interrupts on this cpu
at all. But then we probaply need to disable the interupt line with
disable_irq. That's why I suggested to move the timer to thread context.

Steffen

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH net-next-2.6 2/2] 3c59x: Use fine-grained locks for MII and windowed register access
  2010-06-24 14:00       ` Steffen Klassert
@ 2010-06-25  0:45         ` Ben Hutchings
  2010-06-25  8:24           ` Steffen Klassert
  0 siblings, 1 reply; 13+ messages in thread
From: Ben Hutchings @ 2010-06-25  0:45 UTC (permalink / raw)
  To: Steffen Klassert; +Cc: David Miller, netdev, Chase Douglas, Arne Nordmark

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

On Thu, 2010-06-24 at 16:00 +0200, Steffen Klassert wrote:
> On Thu, Jun 24, 2010 at 01:57:19PM +0100, Ben Hutchings wrote:
> > > 
> > > This adds a lot of calls to spin_lock_irqsave/spin_unlock_irqrestore to many
> > > places where this is not necessary at all. For example during device probe and
> > > device open, window_read/window_write are called multiple times, each time
> > > disabling the interrupts. I'd suggest to have unlocked, locked and irqsave
> > > versions of window_read/window_write and use them in appropriate places.
> > 
> > So what?  These are not speed-critical.  The fast-path functions do
> > acquire the lock just once.
> > 
> 
> The point is that we should not disable the interrupts if we don't need to
> do so. It is not speed critical for the 3c59x driver but disabling the
> interrupts should be avoided whenever possible. For example during device
> probe and device open we can't race against an interrupt handler because
> the device is not yet running.
> 
> An example from vortex_probe1() is:
> 
> for (i = 0; i < 6; i++)
> 	window_write8(vp, dev->dev_addr[i], 2, i);
> 
> which expands to someting like:
> 
> for (i = 0; i < 6; i++) {
> 	unsigned long flags;
> 	spin_lock_irqsave(&vp->window_lock, flags);
> 	window_set(vp, window);
> 	iowrite8(dev->dev_addr[i], vp->ioaddr  + i);
> 	spin_unlock_irqrestore(&vp->window_lock, flags);
> 	return ret;
> }
[...]

I still fail to see why this matters.

The sfc driver which I look after in my day job also uses
spin_lock_irqsave() for each CSR update, when this could be avoided in
the initialisation path.  None of the many customers using rt kernels
has ever complained about this.  It's just not an important issue.

Ben.

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH net-next-2.6 2/2] 3c59x: Use fine-grained locks for MII and windowed register access
  2010-06-25  0:45         ` Ben Hutchings
@ 2010-06-25  8:24           ` Steffen Klassert
  2010-06-29  6:18             ` David Miller
  0 siblings, 1 reply; 13+ messages in thread
From: Steffen Klassert @ 2010-06-25  8:24 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: David Miller, netdev, Chase Douglas, Arne Nordmark

On Fri, Jun 25, 2010 at 01:45:59AM +0100, Ben Hutchings wrote:
> > 
> > The point is that we should not disable the interrupts if we don't need to
> > do so. It is not speed critical for the 3c59x driver but disabling the
> > interrupts should be avoided whenever possible. For example during device
> > probe and device open we can't race against an interrupt handler because
> > the device is not yet running.
> > 
> > An example from vortex_probe1() is:
> > 
> > for (i = 0; i < 6; i++)
> > 	window_write8(vp, dev->dev_addr[i], 2, i);
> > 
> > which expands to someting like:
> > 
> > for (i = 0; i < 6; i++) {
> > 	unsigned long flags;
> > 	spin_lock_irqsave(&vp->window_lock, flags);
> > 	window_set(vp, window);
> > 	iowrite8(dev->dev_addr[i], vp->ioaddr  + i);
> > 	spin_unlock_irqrestore(&vp->window_lock, flags);
> > 	return ret;
> > }
> [...]
> 
> I still fail to see why this matters.
> 

These locks are not needed, why you want to have them arround?
It adds overhead, even if they are not in a fast-path function.
And much more important, this gives a reader of the code the
impression that these locks are needed. For somebody who tries
to understand the code, it will be probaply not that easy to
figure out which of these locks are needed and for what reason.

> The sfc driver which I look after in my day job also uses
> spin_lock_irqsave() for each CSR update, when this could be avoided in
> the initialisation path.  None of the many customers using rt kernels
> has ever complained about this.  It's just not an important issue.

Perhaps I'm fussy, personally I prefer to use the appropriate
lock in any case. But that's just my opinion, other people might
think different about that.

Steffen

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH net-next-2.6 2/2] 3c59x: Use fine-grained locks for MII and windowed register access
  2010-06-25  8:24           ` Steffen Klassert
@ 2010-06-29  6:18             ` David Miller
  2010-06-29  6:39               ` Steffen Klassert
  2010-06-30  1:26               ` [PATCHv2 net-next-2.6] " Ben Hutchings
  0 siblings, 2 replies; 13+ messages in thread
From: David Miller @ 2010-06-29  6:18 UTC (permalink / raw)
  To: steffen.klassert; +Cc: ben, netdev, chase.douglas, nordmark

From: Steffen Klassert <steffen.klassert@secunet.com>
Date: Fri, 25 Jun 2010 10:24:47 +0200

> These locks are not needed, why you want to have them arround?

Steffen I think you are being overly picky of Ben's changes.

I'd rather have too much locking during device probe and
initialization than a subtle bug that occurs because later on someone
decides to move IRQ enabling earlier in the chip init path and now
we get strange hangs that take forever to diagnose.

I mean, extra locking in probe/init paths... ugh, there are so many
more important things to worry about!

Once Ben posts a new version of this second patch with the
proper spin_lock_init() calls added I am going to apply both
of his changes.

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH net-next-2.6 1/2] 3c59x: Specify window explicitly for access to windowed registers
  2010-06-23 23:54 ` [PATCH net-next-2.6 1/2] 3c59x: Specify window explicitly for access to windowed registers Ben Hutchings
@ 2010-06-29  6:20   ` David Miller
  0 siblings, 0 replies; 13+ messages in thread
From: David Miller @ 2010-06-29  6:20 UTC (permalink / raw)
  To: ben; +Cc: netdev, chase.douglas, nordmark

From: Ben Hutchings <ben@decadent.org.uk>
Date: Thu, 24 Jun 2010 00:54:31 +0100

> Currently much of the code assumes that a specific window has been
> selected, while a few functions save and restore the window.  This
> makes it impossible to introduce fine-grained locking.
> 
> Make those assumptions explicit by introducing wrapper functions
> to set the window and read/write a register.  Use these everywhere
> except vortex_interrupt(), vortex_start_xmit() and vortex_rx().
> These set the window just once, or not at all in the case of
> vortex_rx() as it should always be called from vortex_interrupt().
> 
> Cache the current window in struct vortex_private to avoid
> unnecessary hardware writes.
> 
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> Tested-by: Arne Nordmark <nordmark@mech.kth.se> [against 2.6.32]

Applied.

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH net-next-2.6 2/2] 3c59x: Use fine-grained locks for MII and windowed register access
  2010-06-29  6:18             ` David Miller
@ 2010-06-29  6:39               ` Steffen Klassert
  2010-06-30  1:26               ` [PATCHv2 net-next-2.6] " Ben Hutchings
  1 sibling, 0 replies; 13+ messages in thread
From: Steffen Klassert @ 2010-06-29  6:39 UTC (permalink / raw)
  To: David Miller; +Cc: ben, netdev, chase.douglas, nordmark

On Mon, Jun 28, 2010 at 11:18:12PM -0700, David Miller wrote:
> 
> Once Ben posts a new version of this second patch with the
> proper spin_lock_init() calls added I am going to apply both
> of his changes.

Yes, of course apply them. It was just a recommendation to avoid the locks
in the cases they are not needed. These patches are a real improvement,
so I'm fine with them.

Steffen

^ permalink raw reply	[flat|nested] 13+ messages in thread
* [PATCHv2 net-next-2.6] 3c59x: Use fine-grained locks for MII and windowed register access
  2010-06-29  6:18             ` David Miller
  2010-06-29  6:39               ` Steffen Klassert
@ 2010-06-30  1:26               ` Ben Hutchings
  2010-06-30  6:15                 ` David Miller
  1 sibling, 1 reply; 13+ messages in thread
From: Ben Hutchings @ 2010-06-30  1:26 UTC (permalink / raw)
  To: David Miller; +Cc: steffen.klassert, netdev, chase.douglas, nordmark

This avoids scheduling in atomic context and also means that IRQs
will only be deferred for relatively short periods of time.

Previously discussed in:
http://article.gmane.org/gmane.linux.network/155024

Reported-by: Arne Nordmark <nordmark@mech.kth.se>
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
Added the missing spin_lock_init() calls.

Ben.

 drivers/net/3c59x.c |   68 ++++++++++++++++++++++++++++++--------------------
 1 files changed, 41 insertions(+), 27 deletions(-)

diff --git a/drivers/net/3c59x.c b/drivers/net/3c59x.c
index beddef9..069a03f 100644
--- a/drivers/net/3c59x.c
+++ b/drivers/net/3c59x.c
@@ -644,9 +644,15 @@ struct vortex_private {
 	u16 deferred;						/* Resend these interrupts when we
 										 * bale from the ISR */
 	u16 io_size;						/* Size of PCI region (for release_region) */
-	spinlock_t lock;					/* Serialise access to device & its vortex_private */
-	struct mii_if_info mii;				/* MII lib hooks/info */
-	int window;					/* Register window */
+
+	/* Serialises access to hardware other than MII and variables below.
+	 * The lock hierarchy is rtnl_lock > lock > mii_lock > window_lock. */
+	spinlock_t lock;
+
+	spinlock_t mii_lock;		/* Serialises access to MII */
+	struct mii_if_info mii;		/* MII lib hooks/info */
+	spinlock_t window_lock;		/* Serialises access to windowed regs */
+	int window;			/* Register window */
 };
 
 static void window_set(struct vortex_private *vp, int window)
@@ -661,15 +667,23 @@ static void window_set(struct vortex_private *vp, int window)
 static u ## size							\
 window_read ## size(struct vortex_private *vp, int window, int addr)	\
 {									\
+	unsigned long flags;						\
+	u ## size ret;							\
+	spin_lock_irqsave(&vp->window_lock, flags);			\
 	window_set(vp, window);						\
-	return ioread ## size(vp->ioaddr + addr);			\
+	ret = ioread ## size(vp->ioaddr + addr);			\
+	spin_unlock_irqrestore(&vp->window_lock, flags);		\
+	return ret;							\
 }									\
 static void								\
 window_write ## size(struct vortex_private *vp, u ## size value,	\
 		     int window, int addr)				\
 {									\
+	unsigned long flags;						\
+	spin_lock_irqsave(&vp->window_lock, flags);			\
 	window_set(vp, window);						\
 	iowrite ## size(value, vp->ioaddr + addr);			\
+	spin_unlock_irqrestore(&vp->window_lock, flags);		\
 }
 DEFINE_WINDOW_IO(8)
 DEFINE_WINDOW_IO(16)
@@ -1181,6 +1195,8 @@ static int __devinit vortex_probe1(struct device *gendev,
 	}
 
 	spin_lock_init(&vp->lock);
+	spin_lock_init(&vp->mii_lock);
+	spin_lock_init(&vp->window_lock);
 	vp->gendev = gendev;
 	vp->mii.dev = dev;
 	vp->mii.mdio_read = mdio_read;
@@ -1784,7 +1800,6 @@ vortex_timer(unsigned long data)
 		pr_debug("dev->watchdog_timeo=%d\n", dev->watchdog_timeo);
 	}
 
-	disable_irq_lockdep(dev->irq);
 	media_status = window_read16(vp, 4, Wn4_Media);
 	switch (dev->if_port) {
 	case XCVR_10baseT:  case XCVR_100baseTx:  case XCVR_100baseFx:
@@ -1805,10 +1820,7 @@ vortex_timer(unsigned long data)
 	case XCVR_MII: case XCVR_NWAY:
 		{
 			ok = 1;
-			/* Interrupts are already disabled */
-			spin_lock(&vp->lock);
 			vortex_check_media(dev, 0);
-			spin_unlock(&vp->lock);
 		}
 		break;
 	  default:					/* Other media types handled by Tx timeouts. */
@@ -1827,6 +1839,8 @@ vortex_timer(unsigned long data)
 	if (!ok) {
 		unsigned int config;
 
+		spin_lock_irq(&vp->lock);
+
 		do {
 			dev->if_port = media_tbl[dev->if_port].next;
 		} while ( ! (vp->available_media & media_tbl[dev->if_port].mask));
@@ -1855,6 +1869,8 @@ vortex_timer(unsigned long data)
 		if (vortex_debug > 1)
 			pr_debug("wrote 0x%08x to Wn3_Config\n", config);
 		/* AKPM: FIXME: Should reset Rx & Tx here.  P60 of 3c90xc.pdf */
+
+		spin_unlock_irq(&vp->lock);
 	}
 
 leave_media_alone:
@@ -1862,7 +1878,6 @@ leave_media_alone:
 	  pr_debug("%s: Media selection timer finished, %s.\n",
 			 dev->name, media_tbl[dev->if_port].name);
 
-	enable_irq_lockdep(dev->irq);
 	mod_timer(&vp->timer, RUN_AT(next_tick));
 	if (vp->deferred)
 		iowrite16(FakeIntr, ioaddr + EL3_CMD);
@@ -2051,9 +2066,11 @@ vortex_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		int len = (skb->len + 3) & ~3;
 		vp->tx_skb_dma = pci_map_single(VORTEX_PCI(vp), skb->data, len,
 						PCI_DMA_TODEVICE);
+		spin_lock_irq(&vp->window_lock);
 		window_set(vp, 7);
 		iowrite32(vp->tx_skb_dma, ioaddr + Wn7_MasterAddr);
 		iowrite16(len, ioaddr + Wn7_MasterLen);
+		spin_unlock_irq(&vp->window_lock);
 		vp->tx_skb = skb;
 		iowrite16(StartDMADown, ioaddr + EL3_CMD);
 		/* netif_wake_queue() will be called at the DMADone interrupt. */
@@ -2225,6 +2242,7 @@ vortex_interrupt(int irq, void *dev_id)
 		pr_debug("%s: interrupt, status %4.4x, latency %d ticks.\n",
 			   dev->name, status, ioread8(ioaddr + Timer));
 
+	spin_lock(&vp->window_lock);
 	window_set(vp, 7);
 
 	do {
@@ -2285,6 +2303,8 @@ vortex_interrupt(int irq, void *dev_id)
 		iowrite16(AckIntr | IntReq | IntLatch, ioaddr + EL3_CMD);
 	} while ((status = ioread16(ioaddr + EL3_STATUS)) & (IntLatch | RxComplete));
 
+	spin_unlock(&vp->window_lock);
+
 	if (vortex_debug > 4)
 		pr_debug("%s: exiting interrupt, status %4.4x.\n",
 			   dev->name, status);
@@ -2806,37 +2826,22 @@ static void update_stats(void __iomem *ioaddr, struct net_device *dev)
 static int vortex_nway_reset(struct net_device *dev)
 {
 	struct vortex_private *vp = netdev_priv(dev);
-	unsigned long flags;
-	int rc;
 
-	spin_lock_irqsave(&vp->lock, flags);
-	rc = mii_nway_restart(&vp->mii);
-	spin_unlock_irqrestore(&vp->lock, flags);
-	return rc;
+	return mii_nway_restart(&vp->mii);
 }
 
 static int vortex_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 {
 	struct vortex_private *vp = netdev_priv(dev);
-	unsigned long flags;
-	int rc;
 
-	spin_lock_irqsave(&vp->lock, flags);
-	rc = mii_ethtool_gset(&vp->mii, cmd);
-	spin_unlock_irqrestore(&vp->lock, flags);
-	return rc;
+	return mii_ethtool_gset(&vp->mii, cmd);
 }
 
 static int vortex_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 {
 	struct vortex_private *vp = netdev_priv(dev);
-	unsigned long flags;
-	int rc;
 
-	spin_lock_irqsave(&vp->lock, flags);
-	rc = mii_ethtool_sset(&vp->mii, cmd);
-	spin_unlock_irqrestore(&vp->lock, flags);
-	return rc;
+	return mii_ethtool_sset(&vp->mii, cmd);
 }
 
 static u32 vortex_get_msglevel(struct net_device *dev)
@@ -3059,6 +3064,8 @@ static int mdio_read(struct net_device *dev, int phy_id, int location)
 	int read_cmd = (0xf6 << 10) | (phy_id << 5) | location;
 	unsigned int retval = 0;
 
+	spin_lock_bh(&vp->mii_lock);
+
 	if (mii_preamble_required)
 		mdio_sync(vp, 32);
 
@@ -3082,6 +3089,9 @@ static int mdio_read(struct net_device *dev, int phy_id, int location)
 			       4, Wn4_PhysicalMgmt);
 		mdio_delay(vp);
 	}
+
+	spin_unlock_bh(&vp->mii_lock);
+
 	return retval & 0x20000 ? 0xffff : retval>>1 & 0xffff;
 }
 
@@ -3091,6 +3101,8 @@ static void mdio_write(struct net_device *dev, int phy_id, int location, int val
 	int write_cmd = 0x50020000 | (phy_id << 23) | (location << 18) | value;
 	int i;
 
+	spin_lock_bh(&vp->mii_lock);
+
 	if (mii_preamble_required)
 		mdio_sync(vp, 32);
 
@@ -3111,6 +3123,8 @@ static void mdio_write(struct net_device *dev, int phy_id, int location, int val
 			       4, Wn4_PhysicalMgmt);
 		mdio_delay(vp);
 	}
+
+	spin_unlock_bh(&vp->mii_lock);
 }
 
 /* ACPI: Advanced Configuration and Power Interface. */
-- 
1.7.1


-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

^ permalink raw reply related	[flat|nested] 13+ messages in thread
* Re: [PATCHv2 net-next-2.6] 3c59x: Use fine-grained locks for MII and windowed register access
  2010-06-30  1:26               ` [PATCHv2 net-next-2.6] " Ben Hutchings
@ 2010-06-30  6:15                 ` David Miller
  0 siblings, 0 replies; 13+ messages in thread
From: David Miller @ 2010-06-30  6:15 UTC (permalink / raw)
  To: ben; +Cc: steffen.klassert, netdev, chase.douglas, nordmark

From: Ben Hutchings <ben@decadent.org.uk>
Date: Wed, 30 Jun 2010 02:26:56 +0100

> This avoids scheduling in atomic context and also means that IRQs
> will only be deferred for relatively short periods of time.
> 
> Previously discussed in:
> http://article.gmane.org/gmane.linux.network/155024
> 
> Reported-by: Arne Nordmark <nordmark@mech.kth.se>
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>

Applied.

^ permalink raw reply	[flat|nested] 13+ messages in thread
end of thread, other threads:[~2010-06-30  6:15 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-06-23 23:52 [PATCH net-next-2.6 0/2] 3c59x: Locking fixes Ben Hutchings
2010-06-23 23:54 ` [PATCH net-next-2.6 1/2] 3c59x: Specify window explicitly for access to windowed registers Ben Hutchings
2010-06-29  6:20   ` David Miller
2010-06-23 23:55 ` [PATCH net-next-2.6 2/2] 3c59x: Use fine-grained locks for MII and windowed register access Ben Hutchings
2010-06-24 12:05   ` Steffen Klassert
2010-06-24 12:57     ` Ben Hutchings
2010-06-24 14:00       ` Steffen Klassert
2010-06-25  0:45         ` Ben Hutchings
2010-06-25  8:24           ` Steffen Klassert
2010-06-29  6:18             ` David Miller
2010-06-29  6:39               ` Steffen Klassert
2010-06-30  1:26               ` [PATCHv2 net-next-2.6] " Ben Hutchings
2010-06-30  6:15                 ` David Miller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.