Skip to contentSkip to conversation

sky2 1.7 non-critical bug fixes

PATCH In review [email protected] 2 participants 13 messages Aug 28, 2006 – Aug 29, 2006

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover sky2 1.7 non-critical bug fixes

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

#1 lore raw

PATCH 1/9 sky2: remove cloned/pskb_expand_head check

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.

drivers/net/sky2.c +0 -8
@@ -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 <[email protected]>

#2 lore raw

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

applied 1-9

#3 lore raw

PATCH 2/9 sky2: use netdev_alloc_skb

In reply to [email protected]

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.

drivers/net/sky2.c +16 -13
@@ -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 <[email protected]>

#4 lore raw

PATCH 3/9 sky2: dont use force status bit

In reply to [email protected]

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

drivers/net/sky2.c +2 -7
@@ -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);
 
drivers/net/sky2.h +0 -1
@@ -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 <[email protected]>

#5 lore raw

PATCH 4/9 sky2: MSI test timing

In reply to [email protected]

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.

drivers/net/sky2.c +3 -3
@@ -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 <[email protected]>

#6 lore raw

PATCH 5/9 sky2: TSO mss optimization

In reply to [email protected]

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.

drivers/net/sky2.c +9 -9
@@ -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 <[email protected]>

#7 lore raw

PATCH 6/9 sky2: optimize checksum offload information

In reply to [email protected]

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.

drivers/net/sky2.c +11 -6
@@ -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);
drivers/net/sky2.h +2 -0
@@ -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 <[email protected]>

#8 lore raw

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

In reply to [email protected]

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

drivers/net/sky2.c +31 -26
@@ -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 <[email protected]>

#9 lore raw

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

Quoted 3 lines · [email protected] wrote:

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

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

#10 lore raw

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

Quoted 9 lines · On Tue, 29 Aug 2006 17:04:59 -0400 Jeff Garzik <[email protected]> wrote:
Quoted 3 lines · [email protected] wrote:

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

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

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

Signature · 1 line
Stephen Hemminger <[email protected]>
#11 lore raw

PATCH 8/9 sky2: pci post bug

In reply to [email protected]

Make sure that PCI write occurs before the delay.

drivers/net/sky2.c +4 -2
@@ -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 <[email protected]>