Skip to contentSkip to conversation

sky2: driver update

PATCH In review Stephen Hemminger 2 participants 12 messages Aug 6, 2009 – Aug 11, 2009

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

Stephen Hemminger

author
#0 lore raw

cover sky2: driver update

This undoes the prematurely committed sky2 transmit logic changes and applies Mike's later changes.
Here is a corrected patch stream please apply this to 2.6.31-rc5

author
#1 lore raw

PATCH 1/8 Revert "sky2: Avoid transmits during sky2_down()"

This reverts commit f6caa14aa0b126d4a2933907d1519611b2a8524a. Better solution to same problem was done in later patches.

Signed-off-by Stephen Hemminger <[email protected]>
---
 drivers/net/sky2.c |   14 +-------------
 drivers/net/sky2.h |    1 -
 2 files changed, 1 insertion(+), 14 deletions(-)
drivers/net/sky2.c +1 -13
@@ -1488,8 +1488,6 @@ static int sky2_up(struct net_device *de
 	sky2_set_vlan_mode(hw, port, sky2->vlgrp != NULL);
 #endif
 
-	sky2->restarting = 0;
-
 	err = sky2_rx_start(sky2);
 	if (err)
 		goto err_out;

@@ -1502,9 +1500,6 @@ static int sky2_up(struct net_device *de
 
 	sky2_set_multicast(dev);
 
-	/* wake queue incase we are restarting */
-	netif_wake_queue(dev);
-
 	if (netif_msg_ifup(sky2))
 		printk(KERN_INFO PFX "%s: enabling interface\n", dev->name);
 	return 0;

@@ -1538,8 +1533,6 @@ static inline int tx_dist(unsigned tail,
 /* Number of list elements available for next tx */
 static inline int tx_avail(const struct sky2_port *sky2)
 {
-	if (unlikely(sky2->restarting))
-		return 0;
 	return sky2->tx_pending - tx_dist(sky2->tx_cons, sky2->tx_prod);
 }
 

@@ -1825,10 +1818,6 @@ static int sky2_down(struct net_device *
 	if (netif_msg_ifdown(sky2))
 		printk(KERN_INFO PFX "%s: disabling interface\n", dev->name);
 
-	/* explicitly shut off tx incase we're restarting */
-	sky2->restarting = 1;
-	netif_tx_disable(dev);
-
 	/* Force flow control off */
 	sky2_write8(hw, SK_REG(port, GMAC_CTRL), GMC_PAUSE_OFF);
 

@@ -2370,7 +2359,7 @@ static inline void sky2_tx_done(struct n
 {
 	struct sky2_port *sky2 = netdev_priv(dev);
 
-	if (likely(netif_running(dev) && !sky2->restarting)) {
+	if (netif_running(dev)) {
 		netif_tx_lock(dev);
 		sky2_tx_complete(sky2, last);
 		netif_tx_unlock(dev);

@@ -4294,7 +4283,6 @@ static __devinit struct net_device *sky2
 	spin_lock_init(&sky2->phy_lock);
 	sky2->tx_pending = TX_DEF_PENDING;
 	sky2->rx_pending = RX_DEF_PENDING;
-	sky2->restarting = 0;
 
 	hw->dev[port] = dev;
drivers/net/sky2.h +0 -1
@@ -2051,7 +2051,6 @@ struct sky2_port {
 	u8		     duplex;	/* DUPLEX_HALF, DUPLEX_FULL */
 	u8		     rx_csum;
 	u8		     wol;
-	u8		     restarting;
  	enum flow_control    flow_mode;
  	enum flow_control    flow_status;
author
#2 lore raw

PATCH 2/8 sky2: Avoid rewinding sky2->tx_prod

In reply to Stephen Hemminger

Rather than moving the next used transmit ring index forward then backing it up in the case of DMA mapping failure, cleaner to use local variable.

Signed-off-by Mike McCormack <[email protected]>
Acked-by Stephen Hemminger <[email protected]>
---
 drivers/net/sky2.c |   34 ++++++++++++++++++----------------
 1 files changed, 18 insertions(+), 16 deletions(-)
drivers/net/sky2.c +18 -16
@@ -989,11 +989,11 @@ static void sky2_prefetch_init(struct sk
 	sky2_read32(hw, Y2_QADDR(qaddr, PREF_UNIT_CTRL));
 }
 
-static inline struct sky2_tx_le *get_tx_le(struct sky2_port *sky2)
+static inline struct sky2_tx_le *get_tx_le(struct sky2_port *sky2, u16 *slot)
 {
-	struct sky2_tx_le *le = sky2->tx_le + sky2->tx_prod;
+	struct sky2_tx_le *le = sky2->tx_le + *slot;
 
-	sky2->tx_prod = RING_NEXT(sky2->tx_prod, TX_RING_SIZE);
+	*slot = RING_NEXT(*slot, TX_RING_SIZE);
 	le->ctrl = 0;
 	return le;
 }

@@ -1006,7 +1006,7 @@ static void tx_init(struct sky2_port *sk
 	sky2->tx_tcpsum = 0;
 	sky2->tx_last_mss = 0;
 
-	le = get_tx_le(sky2);
+	le = get_tx_le(sky2, &sky2->tx_prod);
 	le->addr = 0;
 	le->opcode = OP_ADDR64 | HW_OWNER;
 }

@@ -1565,7 +1565,8 @@ static int sky2_xmit_frame(struct sk_buf
 	struct sky2_hw *hw = sky2->hw;
 	struct sky2_tx_le *le = NULL;
 	struct tx_ring_info *re;
-	unsigned i, len, first_slot;
+	unsigned i, len;
+	u16 slot;
 	dma_addr_t mapping;
 	u16 mss;
 	u8 ctrl;

@@ -1579,14 +1580,14 @@ static int sky2_xmit_frame(struct sk_buf
 	if (pci_dma_mapping_error(hw->pdev, mapping))
 		goto mapping_error;
 
-	first_slot = sky2->tx_prod;
+	slot = sky2->tx_prod;
 	if (unlikely(netif_msg_tx_queued(sky2)))
 		printk(KERN_DEBUG "%s: tx queued, slot %u, len %d\n",
-		       dev->name, first_slot, skb->len);
+		       dev->name, slot, skb->len);
 
 	/* Send high bits if needed */
 	if (sizeof(dma_addr_t) > sizeof(u32)) {
-		le = get_tx_le(sky2);
+		le = get_tx_le(sky2, &slot);
 		le->addr = cpu_to_le32(upper_32_bits(mapping));
 		le->opcode = OP_ADDR64 | HW_OWNER;
 	}

@@ -1599,7 +1600,7 @@ static int sky2_xmit_frame(struct sk_buf
 			mss += ETH_HLEN + ip_hdrlen(skb) + tcp_hdrlen(skb);
 
   		if (mss != sky2->tx_last_mss) {
-  			le = get_tx_le(sky2);
+			le = get_tx_le(sky2, &slot);
   			le->addr = cpu_to_le32(mss);
 
 			if (hw->flags & SKY2_HW_NEW_LE)

@@ -1615,7 +1616,7 @@ static int sky2_xmit_frame(struct sk_buf
 	/* Add VLAN tag, can piggyback on LRGLEN or ADDR64 */
 	if (sky2->vlgrp && vlan_tx_tag_present(skb)) {
 		if (!le) {
-			le = get_tx_le(sky2);
+			le = get_tx_le(sky2, &slot);
 			le->addr = 0;
 			le->opcode = OP_VLAN|HW_OWNER;
 		} else

@@ -1644,7 +1645,7 @@ static int sky2_xmit_frame(struct sk_buf
 			if (tcpsum != sky2->tx_tcpsum) {
 				sky2->tx_tcpsum = tcpsum;
 
-				le = get_tx_le(sky2);
+				le = get_tx_le(sky2, &slot);
 				le->addr = cpu_to_le32(tcpsum);
 				le->length = 0;	/* initial checksum value */
 				le->ctrl = 1;	/* one packet */

@@ -1653,7 +1654,7 @@ static int sky2_xmit_frame(struct sk_buf
 		}
 	}
 
-	le = get_tx_le(sky2);
+	le = get_tx_le(sky2, &slot);
 	le->addr = cpu_to_le32((u32) mapping);
 	le->length = cpu_to_le16(len);
 	le->ctrl = ctrl;

@@ -1674,13 +1675,13 @@ static int sky2_xmit_frame(struct sk_buf
 			goto mapping_unwind;
 
 		if (sizeof(dma_addr_t) > sizeof(u32)) {
-			le = get_tx_le(sky2);
+			le = get_tx_le(sky2, &slot);
 			le->addr = cpu_to_le32(upper_32_bits(mapping));
 			le->ctrl = 0;
 			le->opcode = OP_ADDR64 | HW_OWNER;
 		}
 
-		le = get_tx_le(sky2);
+		le = get_tx_le(sky2, &slot);
 		le->addr = cpu_to_le32((u32) mapping);
 		le->length = cpu_to_le16(frag->size);
 		le->ctrl = ctrl;

@@ -1694,6 +1695,8 @@ static int sky2_xmit_frame(struct sk_buf
 
 	le->ctrl |= EOP;
 
+	sky2->tx_prod = slot;
+
 	if (tx_avail(sky2) <= MAX_SKB_TX_LE)
 		netif_stop_queue(dev);
 

@@ -1702,7 +1705,7 @@ static int sky2_xmit_frame(struct sk_buf
 	return NETDEV_TX_OK;
 
 mapping_unwind:
-	for (i = first_slot; i != sky2->tx_prod; i = RING_NEXT(i, TX_RING_SIZE)) {
+	for (i = sky2->tx_prod; i != slot; i = RING_NEXT(i, TX_RING_SIZE)) {
 		le = sky2->tx_le + i;
 		re = sky2->tx_ring + i;
 

@@ -1722,7 +1725,6 @@ mapping_unwind:
 		}
 	}
 
-	sky2->tx_prod = first_slot;
 mapping_error:
 	if (net_ratelimit())
 		dev_warn(&hw->pdev->dev, "%s: tx mapping error\n", dev->name);
author
#3 lore raw

PATCH 3/8 sky2: Move tx reset functionality to sky2_tx_reset()

In reply to Stephen Hemminger

This is refactoring and should not affect functionality.

Signed-off-by Mike McCormack <[email protected]>
Acked-by Stephen Hemminger <[email protected]>
---
 drivers/net/sky2.c |   44 ++++++++++++++++++++++++++------------------
 1 files changed, 26 insertions(+), 18 deletions(-)
drivers/net/sky2.c +26 -18
index 16a359d..a9bb3fa 100644
@@ -1804,6 +1804,31 @@ static void sky2_tx_clean(struct net_device *dev)
 	netif_tx_unlock_bh(dev);
 }
 
+static void sky2_tx_reset(struct sky2_port* sky2)
+{
+	unsigned port = sky2->port;
+	struct sky2_hw *hw = sky2->hw;
+
+	/* Disable Force Sync bit and Enable Alloc bit */
+	sky2_write8(hw, SK_REG(port, TXA_CTRL),
+		    TXA_DIS_FSYNC | TXA_DIS_ALLOC | TXA_STOP_RC);
+
+	/* Stop Interval Timer and Limit Counter of Tx Arbiter */
+	sky2_write32(hw, SK_REG(port, TXA_ITI_INI), 0L);
+	sky2_write32(hw, SK_REG(port, TXA_LIM_INI), 0L);
+
+	/* Reset the PCI FIFO of the async Tx queue */
+	sky2_write32(hw, Q_ADDR(txqaddr[port], Q_CSR),
+		     BMU_RST_SET | BMU_FIFO_RST);
+
+	/* Reset the Tx prefetch units */
+	sky2_write32(hw, Y2_QADDR(txqaddr[port], PREF_UNIT_CTRL),
+		     PREF_UNIT_RST_SET);
+
+	sky2_write32(hw, RB_ADDR(txqaddr[port], RB_CTRL), RB_RST_SET);
+	sky2_write8(hw, SK_REG(port, TX_GMF_CTRL_T), GMF_RST_SET);
+}
+
 /* Network shutdown */
 static int sky2_down(struct net_device *dev)
 {

@@ -1841,26 +1866,9 @@ static int sky2_down(struct net_device *dev)
 	      && port == 0 && hw->dev[1] && netif_running(hw->dev[1])))
 		sky2_write8(hw, SK_REG(port, GMAC_CTRL), GMC_RST_SET);
 
-	/* Disable Force Sync bit and Enable Alloc bit */
-	sky2_write8(hw, SK_REG(port, TXA_CTRL),
-		    TXA_DIS_FSYNC | TXA_DIS_ALLOC | TXA_STOP_RC);
-
-	/* Stop Interval Timer and Limit Counter of Tx Arbiter */
-	sky2_write32(hw, SK_REG(port, TXA_ITI_INI), 0L);
-	sky2_write32(hw, SK_REG(port, TXA_LIM_INI), 0L);
-
-	/* Reset the PCI FIFO of the async Tx queue */
-	sky2_write32(hw, Q_ADDR(txqaddr[port], Q_CSR),
-		     BMU_RST_SET | BMU_FIFO_RST);
-
-	/* Reset the Tx prefetch units */
-	sky2_write32(hw, Y2_QADDR(txqaddr[port], PREF_UNIT_CTRL),
-		     PREF_UNIT_RST_SET);
-
-	sky2_write32(hw, RB_ADDR(txqaddr[port], RB_CTRL), RB_RST_SET);
+	sky2_tx_reset(sky2);
 
 	sky2_write8(hw, SK_REG(port, RX_GMF_CTRL_T), GMF_RST_SET);
-	sky2_write8(hw, SK_REG(port, TX_GMF_CTRL_T), GMF_RST_SET);
 
 	/* Force any delayed status interrrupt and NAPI */
 	sky2_write32(hw, STAT_LEV_TIMER_CNT, 0);
Signature · 3 lines
1.5.6.5

--
author
#4 lore raw

PATCH 4/8 sky2: Reset tx train after interrupts disabled.

In reply to Stephen Hemminger

Reseting the tx chain too soon results in invalid tx queue positions being delivered in the status queue. This also makes sure there's no overlap between the cleanup done by sky2_tx_clean() and sky2_tx_done().

Signed-off-by Mike McCormack <[email protected]>
Acked-by Stephen Hemminger <[email protected]>
---
 drivers/net/sky2.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
drivers/net/sky2.c +2 -2
index a9bb3fa..ac303cb 100644
@@ -1866,8 +1866,6 @@ static int sky2_down(struct net_device *dev)
 	      && port == 0 && hw->dev[1] && netif_running(hw->dev[1])))
 		sky2_write8(hw, SK_REG(port, GMAC_CTRL), GMC_RST_SET);
 
-	sky2_tx_reset(sky2);
-
 	sky2_write8(hw, SK_REG(port, RX_GMF_CTRL_T), GMF_RST_SET);
 
 	/* Force any delayed status interrrupt and NAPI */

@@ -1892,6 +1890,8 @@ static int sky2_down(struct net_device *dev)
 	/* turn off LED's */
 	sky2_write16(hw, B0_Y2LED, LED_STAT_OFF);
 
+	sky2_tx_reset(sky2);
+
 	sky2_tx_clean(dev);
 	sky2_rx_clean(sky2);
Signature · 3 lines
1.5.6.5

--
author
#5 lore raw

PATCH 5/8 sky2: Remove tx locks

In reply to Stephen Hemminger

Should no longer be necessary.

Signed-off-by Mike McCormack <[email protected]>
Acked-by Stephen Hemminger <[email protected]>
---
 drivers/net/sky2.c |    8 +-------
 1 files changed, 1 insertions(+), 7 deletions(-)
drivers/net/sky2.c +1 -7
index ac303cb..dbedee4 100644
@@ -1799,9 +1799,7 @@ static void sky2_tx_clean(struct net_device *dev)
 {
 	struct sky2_port *sky2 = netdev_priv(dev);
 
-	netif_tx_lock_bh(dev);
 	sky2_tx_complete(sky2, sky2->tx_prod);
-	netif_tx_unlock_bh(dev);
 }
 
 static void sky2_tx_reset(struct sky2_port* sky2)

@@ -2369,11 +2367,7 @@ static inline void sky2_tx_done(struct net_device *dev, u16 last)
 {
 	struct sky2_port *sky2 = netdev_priv(dev);
 
-	if (netif_running(dev)) {
-		netif_tx_lock(dev);
-		sky2_tx_complete(sky2, last);
-		netif_tx_unlock(dev);
-	}
+	sky2_tx_complete(sky2, last);
 }
 
 static inline void sky2_skb_rx(const struct sky2_port *sky2,
Signature · 3 lines
1.5.6.5

--
author
#6 lore raw

PATCH 6/8 sky2: Lock tx queue when calling sky2_down locally

In reply to Stephen Hemminger
Signed-off-by Mike McCormack <[email protected]>
Acked-by Stephen Hemminger <[email protected]>
---
 drivers/net/sky2.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)
drivers/net/sky2.c +11 -2
index dbedee4..972c716 100644
@@ -3078,10 +3078,13 @@ static void sky2_restart(struct work_struct *work)
 	int i, err;
 
 	rtnl_lock();
+
 	for (i = 0; i < hw->ports; i++) {
 		dev = hw->dev[i];
-		if (netif_running(dev))
+		if (netif_running(dev)) {
+			netif_tx_lock(dev);
 			sky2_down(dev);
+		}
 	}
 
 	napi_disable(&hw->napi);

@@ -3099,6 +3102,8 @@ static void sky2_restart(struct work_struct *work)
 				       dev->name, err);
 				dev_close(dev);
 			}
+			else
+				netif_tx_unlock(dev);
 		}
 	}
 

@@ -3697,8 +3702,10 @@ static int sky2_set_ringparam(struct net_device *dev,
 	    ering->tx_pending > TX_RING_SIZE - 1)
 		return -EINVAL;
 
-	if (netif_running(dev))
+	if (netif_running(dev)) {
+		netif_tx_lock(dev);
 		sky2_down(dev);
+	}
 
 	sky2->rx_pending = ering->rx_pending;
 	sky2->tx_pending = ering->tx_pending;

@@ -3707,6 +3714,8 @@ static int sky2_set_ringparam(struct net_device *dev,
 		err = sky2_up(dev);
 		if (err)
 			dev_close(dev);
+		else
+			netif_tx_unlock(dev);
 	}
 
 	return err;
Signature · 3 lines
1.5.6.5

--
author
#7 lore raw

PATCH 7/8 sky2: hold RTNL when doing suspend/shutdown operations

In reply to Stephen Hemminger

The suspend and shutdown code plays with shared state. Use consistent locking, for extra protection.

Signed-off-by Stephen Hemminger <[email protected]>
drivers/net/sky2.c +4 -0
@@ -4635,6 +4635,7 @@ static int sky2_suspend(struct pci_dev *
 	del_timer_sync(&hw->watchdog_timer);
 	cancel_work_sync(&hw->restart_work);
 
+	rtnl_lock();
 	for (i = 0; i < hw->ports; i++) {
 		struct net_device *dev = hw->dev[i];
 		struct sky2_port *sky2 = netdev_priv(dev);

@@ -4652,6 +4653,7 @@ static int sky2_suspend(struct pci_dev *
 	sky2_write32(hw, B0_IMSK, 0);
 	napi_disable(&hw->napi);
 	sky2_power_aux(hw);
+	rtnl_unlock();
 
 	pci_save_state(pdev);
 	pci_enable_wake(pdev, pci_choose_state(pdev, state), wol);

@@ -4721,6 +4723,7 @@ static void sky2_shutdown(struct pci_dev
 	if (!hw)
 		return;
 
+	rtnl_lock();
 	del_timer_sync(&hw->watchdog_timer);
 
 	for (i = 0; i < hw->ports; i++) {

@@ -4735,6 +4738,7 @@ static void sky2_shutdown(struct pci_dev
 
 	if (wol)
 		sky2_power_aux(hw);
+	rtnl_unlock();
 
 	pci_enable_wake(pdev, PCI_D3hot, wol);
 	pci_enable_wake(pdev, PCI_D3cold, wol);
author
#8 lore raw

PATCH 8/8 sky2: hold spinlock around phy_power_down

In reply to Stephen Hemminger

Avoid any possible problems with accessing PHY registers on shutdown. This is a purely theoretical issue and is not related to any of the outstanding bug reports. Since receiver and transmitter are already shutdown and phy interrupts for this device are already disabled, there should already be enough protection.

Suggested-by Mike McCormack <[email protected]>
Signed-off-by Stephen Hemminger <[email protected]>
drivers/net/sky2.c +2 -0
@@ -1883,7 +1883,9 @@ static int sky2_down(struct net_device *
 	synchronize_irq(hw->pdev->irq);
 	napi_synchronize(&hw->napi);
 
+	spin_lock_bh(&sky2->phy_lock);
 	sky2_phy_power_down(hw, port);
+	spin_unlock_bh(&sky2->phy_lock);
 
 	/* turn off LED's */
 	sky2_write16(hw, B0_Y2LED, LED_STAT_OFF);
maintainer
#9 lore raw
In reply to Stephen Hemminger

From: Stephen Hemminger <[email protected]>
Date: Thu, 06 Aug 2009 09:12:13 -0700

Quoted 3 lines

This undoes the prematurely committed sky2 transmit logic changes and applies Mike's later changes.
Here is a corrected patch stream please apply this to 2.6.31-rc5

I have to pass on this set, too many problems:

1) You didn't provide explicit "From: " lines for patches where you were not the author Stephen. I could guess from the Signoff lines but I'd rather not.

2) I can almost swallow the cleanups if they make the bug fix coming up afterwards to be understood more easily.

But I'm not going to apply patches right now that remove locking beause it "seems safe" or "should be ok". That's absolutely not appropriate right now unless it fixes a performance regression explicitly listed in the formal regression list. This is in reference to patch #5.

If this is a prerequisite for patch #6 in some way (that's my guess), please make this more explicit, or even better change words like "should" (that sounds like "I'm not sure") into "is" when the commit message describes how the locks are no longer needed at that spot.

Another idea is to combine #5 and #6 and explain the dependency in the commit message.

Also, to be honest, this kind of commit count is way too high at this late stage of the RC series. Nothing in here is super critical, and none of it (to my knowledge) is in the regression list either.

And I informed you in private that the main reason I kept asking you to be more responsive over the past month or so to sky2 patches and bug reports was so that patches trickled in gradually, instead of being dumped all at once way too late in the RC series.

author
#10 lore raw
Quoted 7 lines · On Sun, 09 Aug 2009 21:09:43 -0700 (PDT) David Miller <[email protected]>…

From: Stephen Hemminger <[email protected]>
Date: Thu, 06 Aug 2009 09:12:13 -0700

Quoted 3 lines

This undoes the prematurely committed sky2 transmit logic changes and applies Mike's later changes.
Here is a corrected patch stream please apply this to 2.6.31-rc5

Quoted 5 lines

I have to pass on this set, too many problems:

1) You didn't provide explicit "From: " lines for patches where you were not the author Stephen. I could guess from the Signoff lines but I'd rather not.

What is the mechanics for this, does patchwork accept From:
lines in message body?

maintainer
#11 lore raw

From: Stephen Hemminger <[email protected]>
Date: Mon, 10 Aug 2009 08:48:22 -0700

Quoted 10 lines
Quoted 7 lines · On Sun, 09 Aug 2009 21:09:43 -0700 (PDT) David Miller <[email protected]>…

From: Stephen Hemminger <[email protected]>
Date: Thu, 06 Aug 2009 09:12:13 -0700

Quoted 3 lines

This undoes the prematurely committed sky2 transmit logic changes and applies Mike's later changes.
Here is a corrected patch stream please apply this to 2.6.31-rc5

Quoted 8 lines
Quoted 5 lines

I have to pass on this set, too many problems:

1) You didn't provide explicit "From: " lines for patches where you were not the author Stephen. I could guess from the Signoff lines but I'd rather not.

What is the mechanics for this, does patchwork accept From:
lines in message body?

Yes, this is exactly how it works, git format-patch will even generate these for you :-)