[PATCH 0/9] sky2 1.7 non-critical bug fixes

[PATCH 0/9] sky2 1.7 non-critical bug fixes

[shemminger]

These are a set of non-critical fixes to the sky2 driver.
  1. cloned skb tso bug fix
  2. netdev_alloc_skb
  3. don't use force status on transmit
  4. MSI pci posting bug
  5. TSO segment size optimization
  6. checksum offload optimization
  7. power up PHY only on network open
  8. pci post bug before delay
  9. version 1.7

[PATCH 1/9] sky2: remove cloned/pskb_expand_head check

[shemminger]

[-- Attachment #1: sky2-no-overwrite.patch --]
[-- Type: text/plain, Size: 1173 bytes --]

The code to handle cloned skb overwriting is unnecessary in the
sky2 driver and is buggy. The bug is that pskb_expand_head can change the
skb but the driver has already mapped in the header.

Since the sky2 hardware doesn't need to overwrite memory, the buggy
code can just be removed; it was mistakenly copied from the tg3
driver.

Signed-off-by: Stephen Hemminger <shemminger@osdl.org>
--- sky2.orig/drivers/net/sky2.c	2006-08-25 16:00:28.000000000 -0700
+++ sky2/drivers/net/sky2.c	2006-08-25 16:01:33.000000000 -0700
@@ -1239,13 +1239,6 @@
 	/* Check for TCP Segmentation Offload */
 	mss = skb_shinfo(skb)->gso_size;
 	if (mss != 0) {
-		/* just drop the packet if non-linear expansion fails */
-		if (skb_header_cloned(skb) &&
-		    pskb_expand_head(skb, 0, 0, GFP_ATOMIC)) {
-			dev_kfree_skb(skb);
-			goto out_unlock;
-		}
-
 		mss += ((skb->h.th->doff - 5) * 4);	/* TCP options */
 		mss += (skb->nh.iph->ihl * 4) + sizeof(struct tcphdr);
 		mss += ETH_HLEN;
@@ -1341,7 +1334,6 @@
 
 	sky2_put_idx(hw, txqaddr[sky2->port], sky2->tx_prod);
 
-out_unlock:
 	spin_unlock(&sky2->tx_lock);
 
 	dev->trans_start = jiffies;

--
Stephen Hemminger <shemminger@osdl.org>

[PATCH 2/9] sky2: use netdev_alloc_skb

[shemminger]

[-- Attachment #1: sky2-netdev.patch --]
[-- Type: text/plain, Size: 3440 bytes --]

Use netdev_alloc_skb for buffer allocation to allow for headroom.
This prevents bugs in code paths that assume extra space at the
front and makes sky2 behave like other drivers.

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

--- sky2.orig/drivers/net/sky2.c	2006-08-25 16:01:33.000000000 -0700
+++ sky2/drivers/net/sky2.c	2006-08-25 16:02:27.000000000 -0700
@@ -954,14 +954,16 @@
 /*
  * It appears the hardware has a bug in the FIFO logic that
  * cause it to hang if the FIFO gets overrun and the receive buffer
- * is not aligned. ALso alloc_skb() won't align properly if slab
- * debugging is enabled.
+ * is not 64 byte aligned. The buffer returned from netdev_alloc_skb is
+ * aligned except if slab debugging is enabled.
  */
-static inline struct sk_buff *sky2_alloc_skb(unsigned int size, gfp_t gfp_mask)
+static inline struct sk_buff *sky2_alloc_skb(struct net_device *dev,
+					     unsigned int length,
+					     gfp_t gfp_mask)
 {
 	struct sk_buff *skb;
 
-	skb = alloc_skb(size + RX_SKB_ALIGN, gfp_mask);
+	skb = __netdev_alloc_skb(dev, length + RX_SKB_ALIGN, gfp_mask);
 	if (likely(skb)) {
 		unsigned long p	= (unsigned long) skb->data;
 		skb_reserve(skb, ALIGN(p, RX_SKB_ALIGN) - p);
@@ -997,7 +999,8 @@
 	for (i = 0; i < sky2->rx_pending; i++) {
 		struct ring_info *re = sky2->rx_ring + i;
 
-		re->skb = sky2_alloc_skb(sky2->rx_bufsize, GFP_KERNEL);
+		re->skb = sky2_alloc_skb(sky2->netdev, sky2->rx_bufsize,
+					 GFP_KERNEL);
 		if (!re->skb)
 			goto nomem;
 
@@ -1829,15 +1832,16 @@
  * For small packets or errors, just reuse existing skb.
  * For larger packets, get new buffer.
  */
-static struct sk_buff *sky2_receive(struct sky2_port *sky2,
+static struct sk_buff *sky2_receive(struct net_device *dev,
 				    u16 length, u32 status)
 {
+ 	struct sky2_port *sky2 = netdev_priv(dev);
 	struct ring_info *re = sky2->rx_ring + sky2->rx_next;
 	struct sk_buff *skb = NULL;
 
 	if (unlikely(netif_msg_rx_status(sky2)))
 		printk(KERN_DEBUG PFX "%s: rx slot %u status 0x%x len %d\n",
-		       sky2->netdev->name, sky2->rx_next, status, length);
+		       dev->name, sky2->rx_next, status, length);
 
 	sky2->rx_next = (sky2->rx_next + 1) % sky2->rx_pending;
 	prefetch(sky2->rx_ring + sky2->rx_next);
@@ -1848,11 +1852,11 @@
 	if (!(status & GMR_FS_RX_OK))
 		goto resubmit;
 
-	if (length > sky2->netdev->mtu + ETH_HLEN)
+	if (length > dev->mtu + ETH_HLEN)
 		goto oversize;
 
 	if (length < copybreak) {
-		skb = alloc_skb(length + 2, GFP_ATOMIC);
+		skb = netdev_alloc_skb(dev, length + 2);
 		if (!skb)
 			goto resubmit;
 
@@ -1867,7 +1871,7 @@
 	} else {
 		struct sk_buff *nskb;
 
-		nskb = sky2_alloc_skb(sky2->rx_bufsize, GFP_ATOMIC);
+		nskb = sky2_alloc_skb(dev, sky2->rx_bufsize, GFP_ATOMIC);
 		if (!nskb)
 			goto resubmit;
 
@@ -1897,7 +1901,7 @@
 
 	if (netif_msg_rx_err(sky2) && net_ratelimit())
 		printk(KERN_INFO PFX "%s: rx error, status 0x%x length %d\n",
-		       sky2->netdev->name, status, length);
+		       dev->name, status, length);
 
 	if (status & (GMR_FS_LONG_ERR | GMR_FS_UN_SIZE))
 		sky2->net_stats.rx_length_errors++;
@@ -1951,11 +1955,10 @@
 
 		switch (le->opcode & ~HW_OWNER) {
 		case OP_RXSTAT:
-			skb = sky2_receive(sky2, length, status);
+			skb = sky2_receive(dev, length, status);
 			if (!skb)
 				break;
 
-			skb->dev = dev;
 			skb->protocol = eth_type_trans(skb, dev);
 			dev->last_rx = jiffies;
 

--
Stephen Hemminger <shemminger@osdl.org>

[PATCH 3/9] sky2: dont use force status bit

[shemminger]

[-- Attachment #1: sky2-ctl-bits.patch --]
[-- Type: text/plain, Size: 1081 bytes --]

Don't use force status bit. It was never implemented on all chips, or has
no impact.

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

--- sky2.orig/drivers/net/sky2.c	2006-08-25 16:02:27.000000000 -0700
+++ sky2/drivers/net/sky2.c	2006-08-25 16:05:10.000000000 -0700
@@ -1192,7 +1192,6 @@
 	struct sky2_tx_le *le = NULL;
 	struct tx_ring_info *re;
 	unsigned i, len;
-	int avail;
 	dma_addr_t mapping;
 	u32 addr64;
 	u16 mss;
@@ -1328,12 +1327,8 @@
 	re->idx = sky2->tx_prod;
 	le->ctrl |= EOP;
 
-	avail = tx_avail(sky2);
-	if (mss != 0 || avail < TX_MIN_PENDING) {
- 		le->ctrl |= FRC_STAT;
-		if (avail <= MAX_SKB_TX_LE)
-			netif_stop_queue(dev);
-	}
+	if (tx_avail(sky2) <= MAX_SKB_TX_LE)
+		netif_stop_queue(dev);
 
 	sky2_put_idx(hw, txqaddr[sky2->port], sky2->tx_prod);
 
--- sky2.orig/drivers/net/sky2.h	2006-08-25 16:00:28.000000000 -0700
+++ sky2/drivers/net/sky2.h	2006-08-25 16:05:10.000000000 -0700
@@ -1748,7 +1748,6 @@
 	INIT_SUM= 1<<3,
 	LOCK_SUM= 1<<4,
 	INS_VLAN= 1<<5,
-	FRC_STAT= 1<<6,
 	EOP	= 1<<7,
 };
 

--
Stephen Hemminger <shemminger@osdl.org>

[PATCH 4/9] sky2: MSI test timing

[shemminger]

[-- Attachment #1: sky2-post-bug.patch --]
[-- Type: text/plain, Size: 852 bytes --]

The test for MSI IRQ could have timing issues. The PCI write needs to be 
pushed out before waiting, and the wait queue should be initialized before
the IRQ.

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

--- sky2.orig/drivers/net/sky2.c	2006-08-25 16:05:10.000000000 -0700
+++ sky2/drivers/net/sky2.c	2006-08-25 16:05:14.000000000 -0700
@@ -3189,6 +3189,8 @@
 	struct pci_dev *pdev = hw->pdev;
 	int err;
 
+	init_waitqueue_head (&hw->msi_wait);
+
 	sky2_write32(hw, B0_IMSK, Y2_IS_IRQ_SW);
 
 	err = request_irq(pdev->irq, sky2_test_intr, IRQF_SHARED, DRV_NAME, hw);
@@ -3198,10 +3200,8 @@
 		return err;
 	}
 
-	init_waitqueue_head (&hw->msi_wait);
-
 	sky2_write8(hw, B0_CTST, CS_ST_SW_IRQ);
-	wmb();
+	sky2_read8(hw, B0_CTST);
 
 	wait_event_timeout(hw->msi_wait, hw->msi_detected, HZ/10);
 

--
Stephen Hemminger <shemminger@osdl.org>

[PATCH 5/9] sky2: TSO mss optimization

[shemminger]

[-- Attachment #1: sky2-only-change.patch --]
[-- Type: text/plain, Size: 1234 bytes --]

The MSS in the transmit engine only has to change if TSO mtu changes. This
means less commands to the chip when mixing TSO and regular data.

Signed-off-by: Stephen Hemminger <shemminger@osdl.org>
--- sky2.orig/drivers/net/sky2.c	2006-08-28 10:00:07.000000000 -0700
+++ sky2/drivers/net/sky2.c	2006-08-28 10:00:08.000000000 -0700
@@ -1244,15 +1244,15 @@
 		mss += ((skb->h.th->doff - 5) * 4);	/* TCP options */
 		mss += (skb->nh.iph->ihl * 4) + sizeof(struct tcphdr);
 		mss += ETH_HLEN;
-	}
 
-	if (mss != sky2->tx_last_mss) {
-		le = get_tx_le(sky2);
-		le->tx.tso.size = cpu_to_le16(mss);
-		le->tx.tso.rsvd = 0;
-		le->opcode = OP_LRGLEN | HW_OWNER;
-		le->ctrl = 0;
-		sky2->tx_last_mss = mss;
+		if (mss != sky2->tx_last_mss) {
+			le = get_tx_le(sky2);
+			le->tx.tso.size = cpu_to_le16(mss);
+			le->tx.tso.rsvd = 0;
+			le->opcode = OP_LRGLEN | HW_OWNER;
+			le->ctrl = 0;
+			sky2->tx_last_mss = mss;
+		}
 	}
 
 	ctrl = 0;
@@ -1320,7 +1320,7 @@
 		le->opcode = OP_BUFFER | HW_OWNER;
 
 		fre = sky2->tx_ring
-		    + RING_NEXT((re - sky2->tx_ring) + i, TX_RING_SIZE);
+			+ RING_NEXT((re - sky2->tx_ring) + i, TX_RING_SIZE);
 		pci_unmap_addr_set(fre, mapaddr, mapping);
 	}
 

--
Stephen Hemminger <shemminger@osdl.org>

[PATCH 6/9] sky2: optimize checksum offload information

[shemminger]

[-- Attachment #1: sky2-csum-off-cache.patch --]
[-- Type: text/plain, Size: 1510 bytes --]

Since many packets have the same checksum starting offset and insertion
location; the driver can save the last information and only tell hardware
when it changes.

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

--- sky2.orig/drivers/net/sky2.c	2006-08-28 10:00:08.000000000 -0700
+++ sky2/drivers/net/sky2.c	2006-08-28 10:00:13.000000000 -0700
@@ -1280,12 +1280,17 @@
 		if (skb->nh.iph->protocol == IPPROTO_UDP)
 			ctrl |= UDPTCP;
 
-		le = get_tx_le(sky2);
-		le->tx.csum.start = cpu_to_le16(hdr);
-		le->tx.csum.offset = cpu_to_le16(offset);
-		le->length = 0;	/* initial checksum value */
-		le->ctrl = 1;	/* one packet */
-		le->opcode = OP_TCPLISW | HW_OWNER;
+		if (hdr != sky2->tx_csum_start || offset != sky2->tx_csum_offset) {
+			sky2->tx_csum_start = hdr;
+			sky2->tx_csum_offset = offset;
+
+			le = get_tx_le(sky2);
+			le->tx.csum.start = cpu_to_le16(hdr);
+			le->tx.csum.offset = cpu_to_le16(offset);
+			le->length = 0;	/* initial checksum value */
+			le->ctrl = 1;	/* one packet */
+			le->opcode = OP_TCPLISW | HW_OWNER;
+		}
 	}
 
 	le = get_tx_le(sky2);
--- sky2.orig/drivers/net/sky2.h	2006-08-28 09:59:36.000000000 -0700
+++ sky2/drivers/net/sky2.h	2006-08-28 10:00:13.000000000 -0700
@@ -1843,6 +1843,8 @@
 	u32		     tx_addr64;
 	u16		     tx_pending;
 	u16		     tx_last_mss;
+	u16		     tx_csum_start;
+	u16		     tx_csum_offset;
 
 	struct ring_info     *rx_ring ____cacheline_aligned_in_smp;
 	struct sky2_rx_le    *rx_le;

--
Stephen Hemminger <shemminger@osdl.org>

[PATCH 7/9] sky2: power down PHY when not up

[shemminger]

[-- Attachment #1: sky2-phy-power.patch --]
[-- Type: text/plain, Size: 3372 bytes --]

To save power, don't enable power to the PHY until device is brought up.

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

--- sky2.orig/drivers/net/sky2.c	2006-08-28 10:00:13.000000000 -0700
+++ sky2/drivers/net/sky2.c	2006-08-28 10:00:17.000000000 -0700
@@ -195,7 +195,6 @@
 static void sky2_set_power_state(struct sky2_hw *hw, pci_power_t state)
 {
 	u16 power_control;
-	u32 reg1;
 	int vaux;
 
 	pr_debug("sky2_set_power_state %d\n", state);
@@ -228,20 +227,9 @@
 		else
 			sky2_write8(hw, B2_Y2_CLK_GATE, 0);
 
-		/* Turn off phy power saving */
-		reg1 = sky2_pci_read32(hw, PCI_DEV_REG1);
-		reg1 &= ~(PCI_Y2_PHY1_POWD | PCI_Y2_PHY2_POWD);
-
-		/* looks like this XL is back asswards .. */
-		if (hw->chip_id == CHIP_ID_YUKON_XL && hw->chip_rev > 1) {
-			reg1 |= PCI_Y2_PHY1_COMA;
-			if (hw->ports > 1)
-				reg1 |= PCI_Y2_PHY2_COMA;
-		}
-		sky2_pci_write32(hw, PCI_DEV_REG1, reg1);
-		udelay(100);
-
 		if (hw->chip_id == CHIP_ID_YUKON_EC_U) {
+			u32 reg1;
+
 			sky2_pci_write32(hw, PCI_DEV_REG3, 0);
 			reg1 = sky2_pci_read32(hw, PCI_DEV_REG4);
 			reg1 &= P_ASPM_CONTROL_MSK;
@@ -253,15 +241,6 @@
 
 	case PCI_D3hot:
 	case PCI_D3cold:
-		/* Turn on phy power saving */
-		reg1 = sky2_pci_read32(hw, PCI_DEV_REG1);
-		if (hw->chip_id == CHIP_ID_YUKON_XL && hw->chip_rev > 1)
-			reg1 &= ~(PCI_Y2_PHY1_POWD | PCI_Y2_PHY2_POWD);
-		else
-			reg1 |= (PCI_Y2_PHY1_POWD | PCI_Y2_PHY2_POWD);
-		sky2_pci_write32(hw, PCI_DEV_REG1, reg1);
-		udelay(100);
-
 		if (hw->chip_id == CHIP_ID_YUKON_XL && hw->chip_rev > 1)
 			sky2_write8(hw, B2_Y2_CLK_GATE, 0);
 		else
@@ -285,7 +264,7 @@
 	sky2_write8(hw, B2_TST_CTRL1, TST_CFG_WRITE_OFF);
 }
 
-static void sky2_phy_reset(struct sky2_hw *hw, unsigned port)
+static void sky2_gmac_reset(struct sky2_hw *hw, unsigned port)
 {
 	u16 reg;
 
@@ -533,6 +512,28 @@
 		gm_phy_write(hw, port, PHY_MARV_INT_MASK, PHY_M_DEF_MSK);
 }
 
+static void sky2_phy_power(struct sky2_hw *hw, unsigned port, int onoff)
+{
+	u32 reg1;
+	static const u32 phy_power[]
+		= { PCI_Y2_PHY1_POWD, PCI_Y2_PHY2_POWD };
+
+	/* looks like this XL is back asswards .. */
+	if (hw->chip_id == CHIP_ID_YUKON_XL && hw->chip_rev > 1)
+		onoff = !onoff;
+
+	reg1 = sky2_pci_read32(hw, PCI_DEV_REG1);
+
+	if (onoff)
+		/* Turn off phy power saving */
+		reg1 &= ~phy_power[port];
+	else
+		reg1 |= phy_power[port];
+
+	sky2_pci_write32(hw, PCI_DEV_REG1, reg1);
+	udelay(100);
+}
+
 /* Force a renegotiation */
 static void sky2_phy_reinit(struct sky2_port *sky2)
 {
@@ -1088,6 +1089,8 @@
 	if (!sky2->rx_ring)
 		goto err_out;
 
+	sky2_phy_power(hw, port, 1);
+
 	sky2_mac_init(hw, port);
 
 	/* Determine available ram buffer space (in 4K blocks).
@@ -1421,7 +1424,7 @@
 	/* Stop more packets from being queued */
 	netif_stop_queue(dev);
 
-	sky2_phy_reset(hw, port);
+	sky2_gmac_reset(hw, port);
 
 	/* Stop transmitter */
 	sky2_write32(hw, Q_ADDR(txqaddr[port], Q_CSR), BMU_STOP);
@@ -1469,6 +1472,8 @@
 	imask &= ~portirq_msk[port];
 	sky2_write32(hw, B0_IMSK, imask);
 
+	sky2_phy_power(hw, port, 0);
+
 	/* turn off LED's */
 	sky2_write16(hw, B0_Y2LED, LED_STAT_OFF);
 
@@ -2403,7 +2408,7 @@
 	sky2_write32(hw, B0_HWE_IMSK, Y2_HWE_ALL_MASK);
 
 	for (i = 0; i < hw->ports; i++)
-		sky2_phy_reset(hw, i);
+		sky2_gmac_reset(hw, i);
 
 	memset(hw->st_le, 0, STATUS_LE_BYTES);
 	hw->st_idx = 0;

--
Stephen Hemminger <shemminger@osdl.org>

[PATCH 8/9] sky2: pci post bug

[shemminger]

[-- Attachment #1: sky2-pci-post.patch --]
[-- Type: text/plain, Size: 742 bytes --]

Make sure that PCI write occurs before the delay.

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


--- sky2.orig/drivers/net/sky2.c	2006-08-28 10:00:17.000000000 -0700
+++ sky2/drivers/net/sky2.c	2006-08-28 10:00:20.000000000 -0700
@@ -531,6 +531,7 @@
 		reg1 |= phy_power[port];
 
 	sky2_pci_write32(hw, PCI_DEV_REG1, reg1);
+	sky2_pci_read32(hw, PCI_DEV_REG1);
 	udelay(100);
 }
 
@@ -766,9 +767,10 @@
 /* Update chip's next pointer */
 static inline void sky2_put_idx(struct sky2_hw *hw, unsigned q, u16 idx)
 {
+	q = Y2_QADDR(q, PREF_UNIT_PUT_IDX);
 	wmb();
-	sky2_write16(hw, Y2_QADDR(q, PREF_UNIT_PUT_IDX), idx);
-	mmiowb();
+	sky2_write16(hw, q, idx);
+	sky2_read16(hw, q);
 }
 
 

--
Stephen Hemminger <shemminger@osdl.org>

[PATCH 9/9] sky2: version 1.7

[shemminger]

[-- Attachment #1: sky2-1.7.patch --]
[-- Type: text/plain, Size: 430 bytes --]

Change version number for this bundle.

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

--- sky2.orig/drivers/net/sky2.c	2006-08-22 14:55:42.000000000 -0700
+++ sky2/drivers/net/sky2.c	2006-08-22 14:55:46.000000000 -0700
@@ -50,7 +50,7 @@
 #include "sky2.h"
 
 #define DRV_NAME		"sky2"
-#define DRV_VERSION		"1.6"
+#define DRV_VERSION		"1.7"
 #define PFX			DRV_NAME " "
 
 /*

--
Stephen Hemminger <shemminger@osdl.org>

Re: [PATCH 7/9] sky2: power down PHY when not up

[Jeff Garzik]

shemminger@osdl.org wrote:
> To save power, don't enable power to the PHY until device is brought up.
> 
> Signed-off-by: Stephen Hemminger <shemminger@osdl.org>

Strongly agree, and ACK, but note that users have complained in the past 
when they could not configure their MAC when the interface was down. 
Users want to eliminate the

	ifconfig ethX up
	ethtool ...

race window.

	Jeff

Re: [PATCH 7/9] sky2: power down PHY when not up

[Stephen Hemminger]

On Tue, 29 Aug 2006 17:04:59 -0400
Jeff Garzik <jgarzik@pobox.com> wrote:

> shemminger@osdl.org wrote:
> > To save power, don't enable power to the PHY until device is brought up.
> > 
> > Signed-off-by: Stephen Hemminger <shemminger@osdl.org>
> 
> Strongly agree, and ACK, but note that users have complained in the past 
> when they could not configure their MAC when the interface was down. 
> Users want to eliminate the
> 
> 	ifconfig ethX up
> 	ethtool ...
> 
> race window.
> 
> 	Jeff

This driver should already handle this because the settings are stored
in the device structure and applied when device is brought up.

-- 
Stephen Hemminger <shemminger@osdl.org>

Re: [PATCH 1/9] sky2: remove cloned/pskb_expand_head check

[Jeff Garzik]

applied 1-9