Skip to contentSkip to conversation

sky2: version 1.24

PATCH In review Stephen Hemminger 2 participants 13 messages Aug 14, 2009

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

Stephen Hemminger

author
#0 lore raw

cover sky2: version 1.24

This is rework of some of Mike's patches, and additional improvement in the restart logic. Patch against current net-next-2.6.

author
#1 lore raw

PATCH 1/9 sky2: Avoid rewinding sky2->tx_prod

From: Mike McCormack <[email protected]>

Keep sky2->tx_prod consistent since int might be examined by an softirq poll or restart.

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
#2 lore raw

PATCH 2/9 sky2: Move tx reset functionality to sky2_tx_reset()

In reply to Stephen Hemminger

From: Mike McCormack <[email protected]>

This is pure refactoring.

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
@@ -1804,6 +1804,31 @@ static void sky2_tx_clean(struct net_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 *
 	      && 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);
author
#3 lore raw

PATCH 3/9 sky2: Reset tx train after interrupts disabled.

In reply to Stephen Hemminger

From: Mike McCormack <[email protected]>

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 +3 -6
@@ -1804,11 +1804,8 @@ static void sky2_tx_clean(struct net_dev
 	netif_tx_unlock_bh(dev);
 }
 
-static void sky2_tx_reset(struct sky2_port* sky2)
+static void sky2_tx_reset(struct sky2_hw *hw, unsigned port)
 {
-	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);

@@ -1866,8 +1863,6 @@ static int sky2_down(struct net_device *
 	      && 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 +1887,8 @@ static int sky2_down(struct net_device *
 	/* turn off LED's */
 	sky2_write16(hw, B0_Y2LED, LED_STAT_OFF);
 
+	sky2_tx_reset(hw, port);
+
 	sky2_tx_clean(dev);
 	sky2_rx_clean(sky2);
author
#4 lore raw

PATCH 4/9 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.

Signed-off-by Stephen Hemminger <[email protected]>
drivers/net/sky2.c +2 -0
@@ -1882,7 +1882,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);
author
#5 lore raw

PATCH 5/9 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
@@ -4631,6 +4631,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);

@@ -4648,6 +4649,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);

@@ -4717,6 +4719,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++) {

@@ -4731,6 +4734,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
#6 lore raw

PATCH 6/9 sky2: cleanup restart operations

In reply to Stephen Hemminger

This unifies the places that bounce the device (suspend/resume and restart). And makes the operations have the same semantics as normal dev_open/dev_stop.

This also avoids setting the multicast addresses twice when device is brought up.

Signed-off-by Stephen Hemminger <[email protected]>
---
 drivers/net/sky2.c |  120 +++++++++++++++++++++++++----------------------------
 1 file changed, 57 insertions(+), 63 deletions(-)
drivers/net/sky2.c +48 -47
@@ -1498,10 +1498,9 @@ static int sky2_up(struct net_device *de
 	sky2_write32(hw, B0_IMSK, imask);
 	sky2_read32(hw, B0_IMSK);
 
-	sky2_set_multicast(dev);
-
 	if (netif_msg_ifup(sky2))
 		printk(KERN_INFO PFX "%s: enabling interface\n", dev->name);
+
 	return 0;
 
 err_out:

@@ -3076,18 +3075,46 @@ static void sky2_reset(struct sky2_hw *h
 	sky2_write8(hw, STAT_ISR_TIMER_CTRL, TIM_START);
 }
 
+/* Take device down (offline).
+ * Equivalent to doing dev_stop() but this does not
+ * inform upper layers of the transistion.
+ */
+static void sky2_detach(struct net_device *dev)
+{
+	if (netif_running(dev)) {
+		netif_device_detach(dev);	/* stop txq */
+		sky2_down(dev);
+	}
+}
+
+/* Bring device back after doing sky2_detach */
+static int sky2_reattach(struct net_device *dev)
+{
+	int err = 0;
+
+	if (netif_running(dev)) {
+		err = sky2_up(dev);
+		if (err) {
+			printk(KERN_INFO PFX "%s: could not restart %d\n",
+			       dev->name, err);
+			dev_close(dev);
+		} else {
+			netif_device_attach(dev);
+			sky2_set_multicast(dev);
+		}
+	}
+
+	return err;
+}
+
 static void sky2_restart(struct work_struct *work)
 {
 	struct sky2_hw *hw = container_of(work, struct sky2_hw, restart_work);
-	struct net_device *dev;
-	int i, err;
+	int i;
 
 	rtnl_lock();
-	for (i = 0; i < hw->ports; i++) {
-		dev = hw->dev[i];
-		if (netif_running(dev))
-			sky2_down(dev);
-	}
+	for (i = 0; i < hw->ports; i++)
+		sky2_detach(hw->dev[i]);
 
 	napi_disable(&hw->napi);
 	sky2_write32(hw, B0_IMSK, 0);

@@ -3095,17 +3122,8 @@ static void sky2_restart(struct work_str
 	sky2_write32(hw, B0_IMSK, Y2_IS_BASE);
 	napi_enable(&hw->napi);
 
-	for (i = 0; i < hw->ports; i++) {
-		dev = hw->dev[i];
-		if (netif_running(dev)) {
-			err = sky2_up(dev);
-			if (err) {
-				printk(KERN_INFO PFX "%s: could not restart %d\n",
-				       dev->name, err);
-				dev_close(dev);
-			}
-		}
-	}
+	for (i = 0; i < hw->ports; i++)
+		sky2_reattach(hw->dev[i]);
 
 	rtnl_unlock();
 }

@@ -3694,7 +3712,6 @@ static int sky2_set_ringparam(struct net
 			      struct ethtool_ringparam *ering)
 {
 	struct sky2_port *sky2 = netdev_priv(dev);
-	int err = 0;
 
 	if (ering->rx_pending > RX_MAX_PENDING ||
 	    ering->rx_pending < 8 ||

@@ -3702,19 +3719,12 @@ static int sky2_set_ringparam(struct net
 	    ering->tx_pending > TX_RING_SIZE - 1)
 		return -EINVAL;
 
-	if (netif_running(dev))
-		sky2_down(dev);
+	sky2_detach(dev);
 
 	sky2->rx_pending = ering->rx_pending;
 	sky2->tx_pending = ering->tx_pending;
 
-	if (netif_running(dev)) {
-		err = sky2_up(dev);
-		if (err)
-			dev_close(dev);
-	}
-
-	return err;
+	return sky2_reattach(dev);
 }
 
 static int sky2_get_regs_len(struct net_device *dev)

@@ -4636,9 +4646,7 @@ static int sky2_suspend(struct pci_dev *
 		struct net_device *dev = hw->dev[i];
 		struct sky2_port *sky2 = netdev_priv(dev);
 
-		netif_device_detach(dev);
-		if (netif_running(dev))
-			sky2_down(dev);
+		sky2_detach(dev);
 
 		if (sky2->wol)
 			sky2_wol_init(sky2);

@@ -4686,25 +4694,18 @@ static int sky2_resume(struct pci_dev *p
 	sky2_write32(hw, B0_IMSK, Y2_IS_BASE);
 	napi_enable(&hw->napi);
 
+	rtnl_lock();
 	for (i = 0; i < hw->ports; i++) {
-		struct net_device *dev = hw->dev[i];
-
-		netif_device_attach(dev);
-		if (netif_running(dev)) {
-			err = sky2_up(dev);
-			if (err) {
-				printk(KERN_ERR PFX "%s: could not up: %d\n",
-				       dev->name, err);
-				rtnl_lock();
-				dev_close(dev);
-				rtnl_unlock();
-				goto out;
-			}
-		}
+		err = sky2_reattach(hw->dev[i]);
+		if (err)
+			goto out;
 	}
+	rtnl_unlock();
 
 	return 0;
 out:
+	rtnl_unlock();
+
 	dev_err(&pdev->dev, "resume failed (%d)\n", err);
 	pci_disable_device(pdev);
 	return err;
author
#7 lore raw

PATCH 7/9 sky2: lock less transmit completion

In reply to Stephen Hemminger

Transmit completion can safely run lockless against transmit start. In the normal case, completion is done from NAPI and only looks at elements that are at the tail of the ring. When doing shutdown or reset, the transmiter should be completely block by NAPI disable and blocking of transmit queue.

Based on earlier work by Mike McCormack.

Signed-off-by Stephen Hemminger <[email protected]>
---
 drivers/net/sky2.c |  120 +++++++++++++++++++++++++----------------------------
 1 file changed, 57 insertions(+), 63 deletions(-)
drivers/net/sky2.c +9 -16
@@ -1734,8 +1734,12 @@ mapping_error:
 /*
  * Free ring elements from starting at tx_cons until "done"
  *
- * NB: the hardware will tell us about partial completion of multi-part
+ * NB:
+ *  1. The hardware will tell us about partial completion of multi-part
  *     buffers so make sure not to free skb to early.
+ *  2. This may run in parallel start_xmit because the it only
+ *     looks at the tail of the queue of FIFO (tx_cons), not
+ *     the head (tx_prod)
  */
 static void sky2_tx_complete(struct sky2_port *sky2, u16 done)
 {

@@ -1793,16 +1797,6 @@ static void sky2_tx_complete(struct sky2
 		netif_wake_queue(dev);
 }
 
-/* Cleanup all untransmitted buffers, assume transmitter not running */
-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_hw *hw, unsigned port)
 {
 	/* Disable Force Sync bit and Enable Alloc bit */

@@ -1890,7 +1884,9 @@ static int sky2_down(struct net_device *
 
 	sky2_tx_reset(hw, port);
 
-	sky2_tx_clean(dev);
+	/* Free any pending frames stuck in HW queue */
+	sky2_tx_complete(sky2, sky2->tx_prod);
+
 	sky2_rx_clean(sky2);
 
 	pci_free_consistent(hw->pdev, RX_LE_BYTES,

@@ -2367,11 +2363,8 @@ static inline void sky2_tx_done(struct n
 {
 	struct sky2_port *sky2 = netdev_priv(dev);
 
-	if (netif_running(dev)) {
-		netif_tx_lock(dev);
+	if (netif_running(dev))
 		sky2_tx_complete(sky2, last);
-		netif_tx_unlock(dev);
-	}
 }
 
 static inline void sky2_skb_rx(const struct sky2_port *sky2,
author
#8 lore raw

PATCH 8/9 sky2: fix pause negotiation

In reply to Stephen Hemminger

The sky2 driver combines auto speed negotiation with automatic negotiation of pause parameters; but the ethtool interface expects them to be split. This patch allows autonegotiation to be used for speed, but manually disable flow control.

Signed-off-by Stephen Hemminger <[email protected]>
drivers/net/sky2.c +47 -26
@@ -321,7 +321,7 @@ static void sky2_phy_init(struct sky2_hw
 	struct sky2_port *sky2 = netdev_priv(hw->dev[port]);
 	u16 ctrl, ct1000, adv, pg, ledctrl, ledover, reg;
 
-	if (sky2->autoneg == AUTONEG_ENABLE &&
+	if ( (sky2->flags & SKY2_FLAG_AUTO_SPEED) &&
 	    !(hw->flags & SKY2_HW_NEWER_PHY)) {
 		u16 ectrl = gm_phy_read(hw, port, PHY_MARV_EXT_CTRL);
 

@@ -363,7 +363,7 @@ static void sky2_phy_init(struct sky2_hw
 			ctrl |= PHY_M_PC_MDI_XMODE(PHY_M_PC_ENA_AUTO);
 
 			/* downshift on PHY 88E1112 and 88E1149 is changed */
-			if (sky2->autoneg == AUTONEG_ENABLE
+			if ( (sky2->flags & SKY2_FLAG_AUTO_SPEED)
 			    && (hw->flags & SKY2_HW_NEWER_PHY)) {
 				/* set downshift counter to 3x and enable downshift */
 				ctrl &= ~PHY_M_PC_DSC_MSK;

@@ -408,7 +408,7 @@ static void sky2_phy_init(struct sky2_hw
 	adv = PHY_AN_CSMA;
 	reg = 0;
 
-	if (sky2->autoneg == AUTONEG_ENABLE) {
+	if (sky2->flags & SKY2_FLAG_AUTO_SPEED) {
 		if (sky2_is_copper(hw)) {
 			if (sky2->advertising & ADVERTISED_1000baseT_Full)
 				ct1000 |= PHY_M_1000C_AFD;

@@ -423,14 +423,11 @@ static void sky2_phy_init(struct sky2_hw
 			if (sky2->advertising & ADVERTISED_10baseT_Half)
 				adv |= PHY_M_AN_10_HD;
 
-			adv |= copper_fc_adv[sky2->flow_mode];
 		} else {	/* special defines for FIBER (88E1040S only) */
 			if (sky2->advertising & ADVERTISED_1000baseT_Full)
 				adv |= PHY_M_AN_1000X_AFD;
 			if (sky2->advertising & ADVERTISED_1000baseT_Half)
 				adv |= PHY_M_AN_1000X_AHD;
-
-			adv |= fiber_fc_adv[sky2->flow_mode];
 		}
 
 		/* Restart Auto-negotiation */

@@ -439,8 +436,8 @@ static void sky2_phy_init(struct sky2_hw
 		/* forced speed/duplex settings */
 		ct1000 = PHY_M_1000C_MSE;
 
-		/* Disable auto update for duplex flow control and speed */
-		reg |= GM_GPCR_AU_ALL_DIS;
+		/* Disable auto update for duplex flow control and duplex */
+		reg |= GM_GPCR_AU_DUP_DIS | GM_GPCR_AU_SPD_DIS;
 
 		switch (sky2->speed) {
 		case SPEED_1000:

@@ -458,8 +455,15 @@ static void sky2_phy_init(struct sky2_hw
 			ctrl |= PHY_CT_DUP_MD;
 		} else if (sky2->speed < SPEED_1000)
 			sky2->flow_mode = FC_NONE;
+	}
 
-
+	if (sky2->flags & SKY2_FLAG_AUTO_PAUSE) {
+		if (sky2_is_copper(hw))
+			adv |= copper_fc_adv[sky2->flow_mode];
+		else
+			adv |= fiber_fc_adv[sky2->flow_mode];
+	} else {
+		reg |= GM_GPCR_AU_FCT_DIS;
  		reg |= gm_fc_disable[sky2->flow_mode];
 
 		/* Forward pause packets to GMAC? */

@@ -594,7 +598,8 @@ static void sky2_phy_init(struct sky2_hw
 		/* no effect on Yukon-XL */
 		gm_phy_write(hw, port, PHY_MARV_LED_CTRL, ledctrl);
 
-		if (sky2->autoneg == AUTONEG_DISABLE || sky2->speed == SPEED_100) {
+		if ( !(sky2->flags & SKY2_FLAG_AUTO_SPEED)
+		     || sky2->speed == SPEED_100) {
 			/* turn on 100 Mbps LED (LED_LINK100) */
 			ledover |= PHY_M_LED_MO_100(MO_LED_ON);
 		}

@@ -605,7 +610,7 @@ static void sky2_phy_init(struct sky2_hw
 	}
 
 	/* Enable phy interrupt on auto-negotiation complete (or link up) */
-	if (sky2->autoneg == AUTONEG_ENABLE)
+	if (sky2->flags & SKY2_FLAG_AUTO_SPEED)
 		gm_phy_write(hw, port, PHY_MARV_INT_MASK, PHY_M_IS_AN_COMPL);
 	else
 		gm_phy_write(hw, port, PHY_MARV_INT_MASK, PHY_M_DEF_MSK);

@@ -661,7 +666,9 @@ static void sky2_phy_power_down(struct s
 
 	/* setup General Purpose Control Register */
 	gma_write16(hw, port, GM_GP_CTRL,
-		    GM_GPCR_FL_PASS | GM_GPCR_SPEED_100 | GM_GPCR_AU_ALL_DIS);
+		    GM_GPCR_FL_PASS | GM_GPCR_SPEED_100 |
+		    GM_GPCR_AU_DUP_DIS | GM_GPCR_AU_FCT_DIS |
+		    GM_GPCR_AU_SPD_DIS);
 
 	if (hw->chip_id != CHIP_ID_YUKON_EC) {
 		if (hw->chip_id == CHIP_ID_YUKON_EC_U) {

@@ -1117,7 +1124,8 @@ static void rx_set_checksum(struct sky2_
 
 	sky2_write32(sky2->hw,
 		     Q_ADDR(rxqaddr[sky2->port], Q_CSR),
-		     sky2->rx_csum ? BMU_ENA_RX_CHKSUM : BMU_DIS_RX_CHKSUM);
+		     (sky2->flags & SKY2_FLAG_RX_CHECKSUM)
+		     ? BMU_ENA_RX_CHKSUM : BMU_DIS_RX_CHKSUM);
 }
 
 /*

@@ -2076,7 +2084,7 @@ static void sky2_phy_intr(struct sky2_hw
 		printk(KERN_INFO PFX "%s: phy interrupt status 0x%x 0x%x\n",
 		       sky2->netdev->name, istatus, phystat);
 
-	if (sky2->autoneg == AUTONEG_ENABLE && (istatus & PHY_M_IS_AN_COMPL)) {
+	if (istatus & PHY_M_IS_AN_COMPL) {
 		if (sky2_autoneg_done(sky2, phystat) == 0)
 			sky2_link_up(sky2);
 		goto out;

@@ -2442,7 +2450,7 @@ static int sky2_status_intr(struct sky2_
 
 			/* This chip reports checksum status differently */
 			if (hw->flags & SKY2_HW_NEW_LE) {
-				if (sky2->rx_csum &&
+				if ((sky2->flags & SKY2_FLAG_RX_CHECKSUM) &&
 				    (le->css & (CSS_ISIPV4 | CSS_ISIPV6)) &&
 				    (le->css & CSS_TCPUDPCSOK))
 					skb->ip_summed = CHECKSUM_UNNECESSARY;

@@ -2469,7 +2477,7 @@ static int sky2_status_intr(struct sky2_
 			/* fall through */
 #endif
 		case OP_RXCHKS:
-			if (!sky2->rx_csum)
+			if (!(sky2->flags & SKY2_FLAG_RX_CHECKSUM))
 				break;
 
 			/* If this happens then driver assuming wrong format */

@@ -2494,7 +2502,8 @@ static int sky2_status_intr(struct sky2_
 				printk(KERN_NOTICE PFX "%s: hardware receive "
 				       "checksum problem (status = %#x)\n",
 				       dev->name, status);
-				sky2->rx_csum = 0;
+				sky2->flags &= ~SKY2_FLAG_RX_CHECKSUM;
+
 				sky2_write32(sky2->hw,
 					     Q_ADDR(rxqaddr[port], Q_CSR),
 					     BMU_DIS_RX_CHKSUM);

@@ -3195,7 +3204,8 @@ static int sky2_get_settings(struct net_
 	}
 
 	ecmd->advertising = sky2->advertising;
-	ecmd->autoneg = sky2->autoneg;
+	ecmd->autoneg = (sky2->flags & SKY2_FLAG_AUTO_SPEED)
+		? AUTONEG_ENABLE : AUTONEG_DISABLE;
 	ecmd->duplex = sky2->duplex;
 	return 0;
 }

@@ -3207,6 +3217,7 @@ static int sky2_set_settings(struct net_
 	u32 supported = sky2_supported_modes(hw);
 
 	if (ecmd->autoneg == AUTONEG_ENABLE) {
+		sky2->flags |= SKY2_FLAG_AUTO_SPEED;
 		ecmd->advertising = supported;
 		sky2->duplex = -1;
 		sky2->speed = -1;

@@ -3248,9 +3259,9 @@ static int sky2_set_settings(struct net_
 
 		sky2->speed = ecmd->speed;
 		sky2->duplex = ecmd->duplex;
+		sky2->flags &= ~SKY2_FLAG_AUTO_SPEED;
 	}
 
-	sky2->autoneg = ecmd->autoneg;
 	sky2->advertising = ecmd->advertising;
 
 	if (netif_running(dev)) {

@@ -3320,14 +3331,17 @@ static u32 sky2_get_rx_csum(struct net_d
 {
 	struct sky2_port *sky2 = netdev_priv(dev);
 
-	return sky2->rx_csum;
+	return !!(sky2->flags & SKY2_FLAG_RX_CHECKSUM);
 }
 
 static int sky2_set_rx_csum(struct net_device *dev, u32 data)
 {
 	struct sky2_port *sky2 = netdev_priv(dev);
 
-	sky2->rx_csum = data;
+	if (data)
+		sky2->flags |= SKY2_FLAG_RX_CHECKSUM;
+	else
+		sky2->flags &= ~SKY2_FLAG_RX_CHECKSUM;
 
 	sky2_write32(sky2->hw, Q_ADDR(rxqaddr[sky2->port], Q_CSR),
 		     data ? BMU_ENA_RX_CHKSUM : BMU_DIS_RX_CHKSUM);

@@ -3345,7 +3359,7 @@ static int sky2_nway_reset(struct net_de
 {
 	struct sky2_port *sky2 = netdev_priv(dev);
 
-	if (!netif_running(dev) || sky2->autoneg != AUTONEG_ENABLE)
+	if (!netif_running(dev) || !(sky2->flags & SKY2_FLAG_AUTO_SPEED))
 		return -EINVAL;
 
 	sky2_phy_reinit(sky2);

@@ -3585,7 +3599,8 @@ static void sky2_get_pauseparam(struct n
 		ecmd->tx_pause = ecmd->rx_pause = 1;
 	}
 
-	ecmd->autoneg = sky2->autoneg;
+	ecmd->autoneg = (sky2->flags & SKY2_FLAG_AUTO_PAUSE)
+		? AUTONEG_ENABLE : AUTONEG_DISABLE;
 }
 
 static int sky2_set_pauseparam(struct net_device *dev,

@@ -3593,7 +3608,11 @@ static int sky2_set_pauseparam(struct ne
 {
 	struct sky2_port *sky2 = netdev_priv(dev);
 
-	sky2->autoneg = ecmd->autoneg;
+	if (ecmd->autoneg == AUTONEG_ENABLE)
+		sky2->flags |= SKY2_FLAG_AUTO_PAUSE;
+	else
+		sky2->flags &= ~SKY2_FLAG_AUTO_PAUSE;
+
 	sky2->flow_mode = sky2_flow(ecmd->rx_pause, ecmd->tx_pause);
 
 	if (netif_running(dev))

@@ -4283,13 +4302,15 @@ static __devinit struct net_device *sky2
 	sky2->msg_enable = netif_msg_init(debug, default_msg);
 
 	/* Auto speed and flow control */
-	sky2->autoneg = AUTONEG_ENABLE;
+	sky2->flags = SKY2_FLAG_AUTO_SPEED | SKY2_FLAG_AUTO_PAUSE;
+	if (hw->chip_id != CHIP_ID_YUKON_XL)
+		sky2->flags |= SKY2_FLAG_RX_CHECKSUM;
+
 	sky2->flow_mode = FC_BOTH;
 
 	sky2->duplex = -1;
 	sky2->speed = -1;
 	sky2->advertising = sky2_supported_modes(hw);
-	sky2->rx_csum = (hw->chip_id != CHIP_ID_YUKON_XL);
 	sky2->wol = wol;
 
 	spin_lock_init(&sky2->phy_lock);
drivers/net/sky2.h +9 -7
@@ -1583,7 +1583,6 @@ enum {
 };
 
 #define GM_GPCR_SPEED_1000	(GM_GPCR_GIGS_ENA | GM_GPCR_SPEED_100)
-#define GM_GPCR_AU_ALL_DIS	(GM_GPCR_AU_DUP_DIS | GM_GPCR_AU_FCT_DIS|GM_GPCR_AU_SPD_DIS)
 
 /*	GM_TX_CTRL			16 bit r/w	Transmit Control Register */
 enum {

@@ -2042,15 +2041,18 @@ struct sky2_port {
 		u8	fifo_lev;
 	} check;
 
-
 	dma_addr_t	     rx_le_map;
 	dma_addr_t	     tx_le_map;
+
 	u16		     advertising;	/* ADVERTISED_ bits */
-	u16		     speed;	/* SPEED_1000, SPEED_100, ... */
-	u8		     autoneg;	/* AUTONEG_ENABLE, AUTONEG_DISABLE */
-	u8		     duplex;	/* DUPLEX_HALF, DUPLEX_FULL */
-	u8		     rx_csum;
-	u8		     wol;
+	u16		     speed;		/* SPEED_1000, SPEED_100, ... */
+	u8		     wol;		/* WAKE_ bits */
+	u8		     duplex;		/* DUPLEX_HALF, DUPLEX_FULL */
+	u16		     flags;
+#define SKY2_FLAG_RX_CHECKSUM		0x0001
+#define SKY2_FLAG_AUTO_SPEED		0x0002
+#define SKY2_FLAG_AUTO_PAUSE		0x0004
+
  	enum flow_control    flow_mode;
  	enum flow_control    flow_status;
maintainer
#10 lore raw
In reply to Stephen Hemminger

From: Stephen Hemminger <[email protected]>
Date: Fri, 14 Aug 2009 08:15:11 -0700

Quoted 2 lines

This is rework of some of Mike's patches, and additional improvement in the restart logic.

Patches themselves look good.

Quoted 1 line

Patch against current net-next-2.6.

Ummm... no, it isn't.

I merged net-2.6 into net-next-2.6 yesterday, which brought in various sky2 fixes. This caused conflicts which I had to hand merge starting in patch #6.

For example, in patch #6:

@@ -1498,10 +1498,9 @@ static int sky2_up(struct net_device *de
sky2_write32(hw, B0_IMSK, imask); sky2_read32(hw, B0_IMSK);

- sky2_set_multicast(dev);
-
if (netif_msg_ifup(sky2))
printk(KERN_INFO PFX "%s: enabling interface\n", dev->name);
+
return 0;

err_out:

There is a netif_wake_queue() call right before the if (netif_msg_ifup)

Then, in patch #7:

@@ -2367,11 +2363,8 @@ static inline void sky2_tx_done(struct n
{
struct sky2_port *sky2 = netdev_priv(dev);

- if (netif_running(dev)) {
- netif_tx_lock(dev);
+ if (netif_running(dev))
sky2_tx_complete(sky2, last);
- netif_tx_unlock(dev);
- }
}

static inline void sky2_skb_rx(const struct sky2_port *sky2,

That if () test has an extra condition "&& !sky2->restarting", I retained in when applying that patch.

Then, in patch #8:

@@ -2042,15 +2041,18 @@ struct sky2_port {
u8 fifo_lev;
} check;

-
dma_addr_t rx_le_map;
dma_addr_t tx_le_map;
+
u16 advertising; /* ADVERTISED_ bits */
- u16 speed; /* SPEED_1000, SPEED_100, ... */

-	u8		     autoneg;	/* AUTONEG_ENABLE, AUTONEG_DISABLE */
-	u8		     duplex;	/* DUPLEX_HALF, DUPLEX_FULL */
-	u8		     rx_csum;
-	u8		     wol;

+ u16 speed; /* SPEED_1000, SPEED_100, ... */
+ u8 wol; /* WAKE_ bits */
+ u8 duplex; /* DUPLEX_HALF, DUPLEX_FULL */
+ u16 flags;
+#define SKY2_FLAG_RX_CHECKSUM 0x0001
+#define SKY2_FLAG_AUTO_SPEED 0x0002
+#define SKY2_FLAG_AUTO_PAUSE 0x0004
+
enum flow_control flow_mode;
enum flow_control flow_status;

This is missing the "->restarting" member, I retained it while applying this patch.

Even if whatever net-next-2.6 tree you patches against didn't have those fixes, you should have anticipated them and known that I'd have to end up sorting out the merge mess afterwards.

Anyways I pushed it all out, send me fixup patches if necessary.

author
#11 lore raw

PATCH 1/1 sky2: remove restarting workaround flag

The whole restarting flag was introduced by Mike McCormack and was a temporary duct tape patch around issues with transmits inflight during restart. The problems it was covering are now fixed and the code should have been reverted.

Signed-off-by Stephen Hemminger <[email protected]>
drivers/net/sky2.c +1 -13
@@ -1496,8 +1496,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;

@@ -1508,9 +1506,6 @@ static int sky2_up(struct net_device *de
 	sky2_write32(hw, B0_IMSK, imask);
 	sky2_read32(hw, B0_IMSK);
 
-	/* 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);
 

@@ -1545,8 +1540,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);
 }
 

@@ -1850,10 +1843,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);
 

@@ -2382,7 +2371,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))
 		sky2_tx_complete(sky2, last);
 }
 

@@ -4327,7 +4316,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
@@ -2053,7 +2053,6 @@ struct sky2_port {
 #define SKY2_FLAG_AUTO_SPEED		0x0002
 #define SKY2_FLAG_AUTO_PAUSE		0x0004
 
-	u8		     restarting;
  	enum flow_control    flow_mode;
  	enum flow_control    flow_status;
maintainer Applied
#12 lore raw

Re: [PATCH] sky2: remove restarting workaround flag

From: Stephen Hemminger <[email protected]>
Date: Fri, 14 Aug 2009 16:33:17 -0700

Quoted 6 lines

The whole restarting flag was introduced by Mike McCormack and was a temporary duct tape patch around issues with transmits inflight during restart. The problems it was covering are now fixed and the code should have been reverted.

Signed-off-by Stephen Hemminger <[email protected]>

Applied, thanks.