Skip to contentSkip to conversation

3c59x: Locking fixes

PATCH Applied Ben Hutchings 3 participants 13 messages Jun 23, 2010 – Jun 30, 2010

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

Ben Hutchings

author
#0 lore raw

cover 3c59x: Locking fixes

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(-)
author
#1 lore raw

PATCH 1/2 3c59x: Specify window explicitly for access to windowed registers

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 <[email protected]>
Tested-by Arne Nordmark <[email protected]> [against 2.6.32]
---
 drivers/net/3c59x.c |  288 +++++++++++++++++++++++++--------------------------
 1 files changed, 140 insertions(+), 148 deletions(-)
drivers/net/3c59x.c +140 -148
index d75803e..beddef9 100644
@@ -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);
Signature · 1 line
1.7.1
maintainer Applied
#2 lore raw

Re: [PATCH net-next-2.6 1/2] 3c59x: Specify window explicitly for access to windowed registers

From: Ben Hutchings <[email protected]>
Date: Thu, 24 Jun 2010 00:54:31 +0100

Quoted 10 lines

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

Quoted 5 lines

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

Signed-off-by Ben Hutchings <[email protected]>
Tested-by Arne Nordmark <[email protected]> [against 2.6.32]

Applied.

author
#3 lore raw

PATCH 2/2 3c59x: Use fine-grained locks for MII and windowed register access

In reply to Ben Hutchings

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 <[email protected]>
Signed-off-by Ben Hutchings <[email protected]>
Tested-by Arne Nordmark <[email protected]> [against 2.6.32]
---
 drivers/net/3c59x.c |   66 ++++++++++++++++++++++++++++++---------------------
 1 files changed, 39 insertions(+), 27 deletions(-)
drivers/net/3c59x.c +39 -27
index beddef9..f4a3fb1 100644
@@ -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. */
Signature · 1 line
1.7.1
#4 lore raw

Re: [PATCH net-next-2.6 2/2] 3c59x: Use fine-grained locks for MII and windowed register access

Hi.

Quoted 22 lines · Ben Hutchings

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 <[email protected]>
Signed-off-by Ben Hutchings <[email protected]>
Tested-by Arne Nordmark <[email protected]> [against 2.6.32]
---
 drivers/net/3c59x.c |   66 ++++++++++++++++++++++++++++++---------------------
 1 files changed, 39 insertions(+), 27 deletions(-)
drivers/net/3c59x.c +0 -1
index beddef9..f4a3fb1 100644
@@ -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 */

drivers/net/3c59x.c struct vortex_private { · line 654

Quoted 10 lines
drivers/net/3c59x.c +8 -2
-	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().

Quoted 17 lines
drivers/net/3c59x.c +7 -1
+	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;							\
 }									\

drivers/net/3c59x.c static void window_set(struct vortex_private *vp, int window) · line 687

Quoted 10 lines
drivers/net/3c59x.c +3 -0
 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.

Quoted 22 lines
drivers/net/3c59x.c +0 -4
 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. */

drivers/net/3c59x.c vortex_timer(unsigned long data) · line 1840

Quoted 4 lines
drivers/net/3c59x.c +1 -0
@@ -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

author
#5 lore raw

Re: [PATCH net-next-2.6 2/2] 3c59x: Use fine-grained locks for MII and windowed register access

Quoted 29 lines · Steffen Klassert

Hi.

Quoted 26 lines · Ben Hutchings

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 <[email protected]>
Signed-off-by Ben Hutchings <[email protected]>
Tested-by Arne Nordmark <[email protected]> [against 2.6.32]
---
 drivers/net/3c59x.c |   66 ++++++++++++++++++++++++++++++---------------------
 1 files changed, 39 insertions(+), 27 deletions(-)
drivers/net/3c59x.c +2 -3
index beddef9..f4a3fb1 100644
@@ -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.
Quoted 8 lines

drivers/net/3c59x.c struct vortex_private { · line 654

Quoted 6 lines
drivers/net/3c59x.c +6 -0
+	 * 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.

Quoted 25 lines
Quoted 25 lines
drivers/net/3c59x.c +9 -1
+	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);			\
Quoted 8 lines

drivers/net/3c59x.c static void window_set(struct vortex_private *vp, int window) · line 687

Quoted 2 lines
drivers/net/3c59x.c +1 -0
+	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.

Quoted 25 lines
Quoted 25 lines
drivers/net/3c59x.c +0 -4
 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;
Quoted 7 lines

drivers/net/3c59x.c vortex_timer(unsigned long data) · line 1840

Quoted 1 line
drivers/net/3c59x.c +1 -0
+		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.

Quoted 6 lines

The rest looks quite good to me.

Thanks,

Steffen

Signature · 2 lines
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.
#6 lore raw

Re: [PATCH net-next-2.6 2/2] 3c59x: Use fine-grained locks for MII and windowed register access

Quoted 10 lines · Ben Hutchings
Quoted 6 lines

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);
}
Quoted 6 lines

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

author
#7 lore raw

Re: [PATCH net-next-2.6 2/2] 3c59x: Use fine-grained locks for MII and windowed register access

Quoted 25 lines · Steffen Klassert
Quoted 10 lines · Ben Hutchings
Quoted 6 lines

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:

Quoted 8 lines
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.

Signature · 2 lines
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.
#8 lore raw

Re: [PATCH net-next-2.6 2/2] 3c59x: Use fine-grained locks for MII and windowed register access

Quoted 22 lines · Ben Hutchings
Quoted 14 lines

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:

Quoted 8 lines
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;
}
Quoted 4 lines

[...]

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.

Quoted 4 lines

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

maintainer
#9 lore raw

Re: [PATCH net-next-2.6 2/2] 3c59x: Use fine-grained locks for MII and windowed register access

From: Steffen Klassert <[email protected]>
Date: Fri, 25 Jun 2010 10:24:47 +0200

Quoted 1 line

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.

#10 lore raw

Re: [PATCH net-next-2.6 2/2] 3c59x: Use fine-grained locks for MII and windowed register access

Quoted 4 lines · David Miller

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

author
#11 lore raw

PATCH v2 1/1 3c59x: Use fine-grained locks for MII and windowed register access

In reply to David Miller

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 <[email protected]>
Signed-off-by Ben Hutchings <[email protected]>

---
Added the missing spin_lock_init() calls.

Ben.

 drivers/net/3c59x.c |   68 ++++++++++++++++++++++++++++++--------------------
 1 files changed, 41 insertions(+), 27 deletions(-)
drivers/net/3c59x.c +41 -27
index beddef9..069a03f 100644
@@ -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. */
Signature · 6 lines
1.7.1


-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.
maintainer Applied
#12 lore raw

Re: [PATCHv2 net-next-2.6] 3c59x: Use fine-grained locks for MII and windowed register access

From: Ben Hutchings <[email protected]>
Date: Wed, 30 Jun 2010 02:26:56 +0100

Quoted 8 lines

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 <[email protected]>
Signed-off-by Ben Hutchings <[email protected]>

Applied.