Re: [PATCH 25/26] [REVISED] atl1: add NAPI support

Re: [PATCH 25/26] [REVISED] atl1: add NAPI support

[Jay Cliburn]

Thanks for your comments Stephen and Joonwoo.  Here's the revised
version of the atl1 NAPI patch.



>From 9c3a8944220287671f983557099bc329f02fda9b Mon Sep 17 00:00:00 2001
From: Jay Cliburn <jacliburn@bellsouth.net>
Date: Tue, 1 Jan 2008 11:55:24 -0600
Subject: [PATCH 25/26] atl1: add NAPI support

Add support for NAPI.

Signed-off-by: Jay Cliburn <jacliburn@bellsouth.net>
---
 drivers/net/Kconfig     |   14 +++++
 drivers/net/atlx/atl1.c |  125 +++++++++++++++++++++++++++++++++++------------
 drivers/net/atlx/atl1.h |   19 +++++++
 drivers/net/atlx/atlx.h |    7 ++-
 4 files changed, 132 insertions(+), 33 deletions(-)

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index d9107e5..095629f 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -2371,6 +2371,20 @@ config ATL1
 	  To compile this driver as a module, choose M here.  The module
 	  will be called atl1.
 
+config ATL1_NAPI
+	bool "Use Rx Polling (NAPI) (EXPERIMENTAL)"
+	depends on ATL1 && EXPERIMENTAL
+	help
+	  NAPI is a new driver API designed to reduce CPU and interrupt load
+	  when the driver is receiving lots of packets from the card. It is
+	  still somewhat experimental and thus not yet enabled by default.
+
+	  If your estimated Rx load is 10kpps or more, or if the card will be
+	  deployed on potentially unfriendly networks (e.g. in a firewall),
+	  then say Y here.
+
+	  If in doubt, say N.
+
 endif # NETDEV_1000
 
 #
diff --git a/drivers/net/atlx/atl1.c b/drivers/net/atlx/atl1.c
index 8b4aa94..88ff000 100644
--- a/drivers/net/atlx/atl1.c
+++ b/drivers/net/atlx/atl1.c
@@ -754,13 +754,6 @@ static void atl1_set_mac_addr(struct atl1_hw *hw)
 	iowrite32(value, (hw->hw_addr + REG_MAC_STA_ADDR) + (1 << 2));
 }
 
-static int atl1_alloc_queues(struct atl1_adapter *adapter)
-{
-	/* temporary placeholder function for NAPI */
-
-	return 0;
-}
-
 /*
  * atl1_sw_init - Initialize general software structures (struct atl1_adapter)
  * @adapter: board private structure to initialize
@@ -769,7 +762,6 @@ static int __devinit atl1_sw_init(struct atl1_adapter *adapter)
 {
 	struct atl1_hw *hw = &adapter->hw;
 	struct net_device *netdev = adapter->netdev;
-	int err;
 
 	hw->max_frame_size = netdev->mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
 	hw->min_frame_size = ETH_ZLEN + ETH_FCS_LEN;
@@ -811,13 +803,6 @@ static int __devinit atl1_sw_init(struct atl1_adapter *adapter)
 	hw->cmb_tx_timer = 256;	/* about 512us */
 	hw->smb_timer = 100000;	/* about 200ms */
 
-	err = atl1_alloc_queues(adapter);
-	if (err) {
-		dev_printk(KERN_DEBUG, &adapter->pdev->dev,
-			"unable to allocate memory for queues\n");
-		return -ENOMEM;
-	}
-
 	spin_lock_init(&adapter->lock);
 	spin_lock_init(&adapter->mb_lock);
 	set_bit(__ATL1_DOWN, &adapter->flags);
@@ -1209,9 +1194,8 @@ static u32 atl1_check_link(struct atl1_adapter *adapter)
 	}
 
 	/* autoneg, insert timer to reconfig phy */
-	if (!test_and_set_bit(0, &adapter->cfg_phy)) {
+	if (!test_and_set_bit(0, &adapter->cfg_phy))
 		mod_timer(&adapter->phy_config_timer, jiffies + 5 * HZ);
-	}
 
 	return 0;
 }
@@ -1699,18 +1683,21 @@ next:
 	return num_alloc;
 }
 
-static void atl1_clean_rx_irq(struct atl1_adapter *adapter)
+#ifdef CONFIG_ATL1_NAPI
+static int atl1_clean_rx_irq(struct atl1_adapter *adapter, int work_to_do)
+#else
+static int atl1_clean_rx_irq(struct atl1_adapter *adapter)
+#endif
 {
 	struct net_device *netdev = adapter->netdev;
-	int i, count;
+	int i, count = 0;
 	u16 length, rrd_next_to_clean;
 	struct atl1_rfd_ring *rfd_ring = &adapter->rfd_ring;
 	struct atl1_rrd_ring *rrd_ring = &adapter->rrd_ring;
 	struct atl1_buffer *buffer_info;
 	struct rx_return_desc *rrd;
 	struct sk_buff *skb;
-
-	count = 0;
+	int work_done = 0;
 
 	rrd_next_to_clean = (u16) atomic_read(&rrd_ring->next_to_clean);
 
@@ -1809,6 +1796,18 @@ rrd_ok:
 		atl1_rx_checksum(adapter, rrd, skb);
 		skb->protocol = eth_type_trans(skb, adapter->netdev);
 
+#ifdef CONFIG_ATL1_NAPI
+		if (adapter->vlgrp && (rrd->pkt_flg & PACKET_FLAG_VLAN_INS)) {
+			u16 vlan_tag = (rrd->vlan_tag >> 4) |
+					((rrd->vlan_tag & 7) << 13) |
+					((rrd->vlan_tag & 8) << 9);
+			vlan_hwaccel_receive_skb(skb, adapter->vlgrp, vlan_tag);
+		} else
+			netif_receive_skb(skb);
+
+		if (++work_done >= work_to_do)
+			break;
+#else
 		if (adapter->vlgrp && (rrd->pkt_flg & PACKET_FLAG_VLAN_INS)) {
 			u16 vlan_tag = (rrd->vlan_tag >> 4) |
 					((rrd->vlan_tag & 7) << 13) |
@@ -1816,6 +1815,7 @@ rrd_ok:
 			vlan_hwaccel_rx(skb, adapter->vlgrp, vlan_tag);
 		} else
 			netif_rx(skb);
+#endif
 
 		/* let protocol layer free skb */
 		buffer_info->skb = NULL;
@@ -1830,9 +1830,11 @@ rrd_ok:
 
 	if (count)
 		atl1_update_mailbox(adapter);
+
+	return work_done;
 }
 
-static bool atl1_clean_tx_irq(struct atl1_adapter *adapter)
+static void atl1_clean_tx_irq(struct atl1_adapter *adapter)
 {
 	struct atl1_tpd_ring *tpd_ring = &adapter->tpd_ring;
 	struct atl1_buffer *buffer_info;
@@ -1866,12 +1868,34 @@ static bool atl1_clean_tx_irq(struct atl1_adapter *adapter)
 	if (netif_queue_stopped(adapter->netdev)
 	    && netif_carrier_ok(adapter->netdev))
 		netif_wake_queue(adapter->netdev);
+}
 
-	if (sw_tpd_next_to_clean == (u16) atomic_read(&tpd_ring->next_to_use))
-		return true;
-	else
-		return false;
+#ifdef CONFIG_ATL1_NAPI
+static int atl1_clean(struct napi_struct *napi, int budget)
+{
+	struct atl1_adapter *adapter = container_of(napi, struct atl1_adapter,
+		napi);
+	struct net_device *poll_dev = adapter->netdev;
+	int work_done = 0;
+
+	adapter = poll_dev->priv;
+
+	if (!netif_carrier_ok(poll_dev))
+		goto quit_polling;
+
+	work_done = atl1_clean_rx_irq(adapter, budget);
+
+	if ((work_done < budget) || !netif_running(poll_dev)) {
+quit_polling:
+		netif_rx_complete(poll_dev, napi);
+
+		if (!test_bit(__ATL1_DOWN, &adapter->flags))
+			atlx_irq_enable(adapter);
+	}
+
+	return work_done;
 }
+#endif
 
 static u16 atl1_tpd_avail(struct atl1_tpd_ring *tpd_ring)
 {
@@ -2176,7 +2200,6 @@ static int atl1_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
 
 	if (!spin_trylock_irqsave(&adapter->lock, flags)) {
 		/* Can't get lock - tell upper layer to requeue */
-		dev_printk(KERN_DEBUG, &adapter->pdev->dev, "tx locked\n");
 		return NETDEV_TX_LOCKED;
 	}
 
@@ -2184,7 +2207,6 @@ static int atl1_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
 		/* not enough descriptors */
 		netif_stop_queue(netdev);
 		spin_unlock_irqrestore(&adapter->lock, flags);
-		dev_printk(KERN_DEBUG, &adapter->pdev->dev, "tx busy\n");
 		return NETDEV_TX_BUSY;
 	}
 
@@ -2233,16 +2255,21 @@ static int atl1_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
  */
 static irqreturn_t atl1_intr(int irq, void *data)
 {
+	struct net_device *netdev = data;
 	struct atl1_adapter *adapter = netdev_priv(data);
 	struct atl1_hw *hw = &adapter->hw;
 	u32 status;
-	int max_ints = 10;
+#ifndef CONFIG_ATL1_NAPI
+	int max_ints = 12;
+#endif
 
 	status = adapter->cmb.cmb->int_stats;
 	if (!status)
 		return IRQ_NONE;
 
+#ifndef CONFIG_ATL1_NAPI
 loopint:
+#endif
 	/* clear CMB interrupt status at once */
 	adapter->cmb.cmb->int_stats = 0;
 
@@ -2257,7 +2284,7 @@ loopint:
 	if (status & ISR_PHY_LINKDOWN) {
 		dev_printk(KERN_DEBUG, &adapter->pdev->dev,
 			"pcie phy link down %x\n", status);
-		if (netif_running(adapter->netdev)) {
+		if (netif_running(netdev)) {
 			/* reset MAC */
 			iowrite32(0, hw->hw_addr + REG_ISR);
 			iowrite32(0, hw->hw_addr + REG_IMR);
@@ -2293,13 +2320,29 @@ loopint:
 	if (status & ISR_TX_EVENT)
 		atl1_clean_tx_irq(adapter);
 
-	/* rx event */
+#ifdef CONFIG_ATL1_NAPI
+	/* rx event with NAPI */
+	if (status & ISR_RX_EVENT) {
+		struct atl1_rrd_ring *rrd_ring = &adapter->rrd_ring;
+		struct rx_return_desc *rrd;
+		rrd = ATL1_RRD_DESC(rrd_ring,
+			((u16) atomic_read(&rrd_ring->next_to_clean)));
+		if (rrd->xsz.valid) {
+			/* valid packet; disable interrupts */
+			iowrite32(IMR_NONRX_MASK, hw->hw_addr + REG_IMR);
+			ioread32(hw->hw_addr + REG_IMR);
+			napi_schedule(&adapter->napi);
+		}
+	}
+#else
+	/* rx event no NAPI */
 	if (status & ISR_RX_EVENT)
 		atl1_clean_rx_irq(adapter);
 
 	status = adapter->cmb.cmb->int_stats;
 	if ((--max_ints > 0) && status)
 		goto loopint;
+#endif
 
 	/* re-enable interrupts */
 	iowrite32((ISR_DIS_SMB | ISR_DIS_DMA), adapter->hw.hw_addr + REG_ISR);
@@ -2376,6 +2419,10 @@ static s32 atl1_up(struct atl1_adapter *adapter)
 	retval = ioread32(&adapter->hw + REG_MASTER_CTRL);
 	retval |= MASTER_CTRL_MANUAL_INT;
 	iowrite32(retval, &adapter->hw + REG_MASTER_CTRL);
+
+#ifdef CONFIG_ATL1_NAPI
+	napi_enable(&adapter->napi);
+#endif
 	atlx_irq_enable(adapter);
 
 	return 0;
@@ -2390,6 +2437,10 @@ void atl1_down(struct atl1_adapter *adapter)
 	atl1_reset_hw(&adapter->hw);
 	adapter->cmb.cmb->int_stats = 0;
 	msleep(1);
+
+#ifdef CONFIG_ATL1_NAPI
+	napi_disable(&adapter->napi);
+#endif
 	atlx_irq_disable(adapter);
 	del_timer_sync(&adapter->watchdog_timer);
 	del_timer_sync(&adapter->phy_config_timer);
@@ -2529,6 +2580,10 @@ static int atl1_open(struct net_device *netdev)
 		goto err_open;
 
 	clear_bit(__ATL1_DOWN, &adapter->flags);
+
+#ifdef CONFIG_ATL1_NAPI
+	napi_enable(&adapter->napi);
+#endif
 	mod_timer(&adapter->watchdog_timer, jiffies + (4 * HZ));
 	val = ioread32(hw->hw_addr + REG_MASTER_CTRL);
 	val |= MASTER_CTRL_MANUAL_INT;
@@ -2615,7 +2670,7 @@ static int atl1_suspend(struct pci_dev *pdev, pm_message_t state)
 	if ((ctrl & BMSR_LSTATUS) && wufc) {
 		retval = atl1_get_speed_and_duplex(hw, &speed, &duplex);
 		if (retval) {
-			dev_printk (KERN_DEBUG, &adapter->pdev->dev,
+			dev_printk(KERN_DEBUG, &adapter->pdev->dev,
 				"speed/duplex error during suspend\n");
 			goto wol_dis;
 		}
@@ -2737,6 +2792,9 @@ static void atl1_netpoll(struct net_device *netdev)
 	disable_irq(adapter->pdev->irq);
 	atl1_intr(netdev->irq, netdev);
 	atl1_clean_tx_irq(adapter);
+#ifndef CONFIG_ATL1_NAPI
+	atl1_clean_rx_irq(adapter);
+#endif
 	enable_irq(adapter->pdev->irq);
 }
 #endif
@@ -2837,6 +2895,9 @@ static int __devinit atl1_probe(struct pci_dev *pdev,
 	netdev->do_ioctl = &atlx_ioctl;
 	netdev->tx_timeout = &atlx_tx_timeout;
 	netdev->watchdog_timeo = 5 * HZ;
+#ifdef CONFIG_ATL1_NAPI
+	netif_napi_add(netdev, &adapter->napi, &atl1_clean, ATL1_NAPI_WEIGHT);
+#endif
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	netdev->poll_controller = atl1_netpoll;
 #endif
diff --git a/drivers/net/atlx/atl1.h b/drivers/net/atlx/atl1.h
index 5187f74..c1ae519 100644
--- a/drivers/net/atlx/atl1.h
+++ b/drivers/net/atlx/atl1.h
@@ -63,6 +63,9 @@ static void atl1_set_mac_addr(struct atl1_hw *hw);
 static int atl1_mii_ioctl(struct net_device *netdev, struct ifreq *ifr,
 	int cmd);
 static u32 atl1_check_link(struct atl1_adapter *adapter);
+#ifdef CONFIG_ATL1_NAPI
+static int atl1_clean(struct napi_struct *napi, int budget);
+#endif
 
 extern const struct ethtool_ops atl1_ethtool_ops;
 
@@ -288,6 +291,15 @@ extern const struct ethtool_ops atl1_ethtool_ops;
 	ISR_CMB_TX	|\
 	ISR_CMB_RX)
 
+#define IMR_NONRX_MASK	(\
+	ISR_MANUAL	|\
+	ISR_SMB		|\
+	ISR_GPHY	|\
+	ISR_PHY_LINKDOWN|\
+	ISR_DMAR_TO_RST	|\
+	ISR_DMAW_TO_RST	|\
+	ISR_CMB_TX)
+
 #define ISR_TX_EVENT	(\
 	ISR_TXF_UNRUN	|\
 	ISR_TX_PKT	|\
@@ -606,6 +618,10 @@ enum atl1_dma_req_block {
 #define ATL1_MAX_RFD		2048
 #define ATL1_REG_COUNT		1538
 
+#ifdef CONFIG_ATL1_NAPI
+#define ATL1_NAPI_WEIGHT 64
+#endif
+
 #define ATL1_GET_DESC(R, i, type)	(&(((type *)((R)->desc))[i]))
 #define ATL1_RFD_DESC(R, i)	ATL1_GET_DESC(R, i, struct rx_free_desc)
 #define ATL1_TPD_DESC(R, i)	ATL1_GET_DESC(R, i, struct tx_packet_desc)
@@ -818,6 +834,9 @@ struct atl1_adapter {
 	struct atl1_cmb cmb;
 	unsigned long flags;
 	int num_rx_queues;
+#ifdef CONFIG_ATL1_NAPI
+	struct napi_struct napi;
+#endif
 };
 
 enum atl1_state {
diff --git a/drivers/net/atlx/atlx.h b/drivers/net/atlx/atlx.h
index 352432e..18021d9 100644
--- a/drivers/net/atlx/atlx.h
+++ b/drivers/net/atlx/atlx.h
@@ -29,7 +29,12 @@
 #include <linux/module.h>
 #include <linux/types.h>
 
-#define ATLX_DRIVER_VERSION "2.1.1"
+#ifndef CONFIG_ATL1_NAPI
+#define DRIVERNAPI
+#else
+#define DRIVERNAPI "-NAPI"
+#endif
+#define ATLX_DRIVER_VERSION "2.1.1"DRIVERNAPI
 MODULE_AUTHOR("Xiong Huang <xiong.huang@atheros.com>, \
 	Chris Snook <csnook@redhat.com>, Jay Cliburn <jcliburn@gmail.com>");
 MODULE_LICENSE("GPL");
-- 
1.5.3.3

RE: [PATCH 25/26] [REVISED] atl1: add NAPI support

[Joonwoo Park]

Hi Jay,

+	if ((work_done < budget) || !netif_running(poll_dev)) {
+quit_polling:
+		netif_rx_complete(poll_dev, napi);
+
+		if (!test_bit(__ATL1_DOWN, &adapter->flags))
+			atlx_irq_enable(adapter);
+	}

Not enough :)
If netif_running() is false, it can make problem.
The problem occurs calling netif_rx_complete with work_done == budget.
If do that, net_rx_action would do poll list double deletion.

Since we had reached a consensus on fixing it without each drivers modifications, there is no best solution for that problem for
now. I'm expecting Dave or others work for net-core.
(http://lkml.org/lkml/2007/12/20/600)

IMHO, atl1_clean should wait for work_done != budget even though netif_running is false at this time.
At least, It would not make oops.

Thanks,
Joonwoo

Re: [PATCH 25/26] [REVISED] atl1: add NAPI support

[David Miller]

From: "Joonwoo Park" <joonwpark81@gmail.com>
Date: Wed, 2 Jan 2008 11:56:36 +0900

> Since we had reached a consensus on fixing it without each drivers
> modifications, there is no best solution for that problem for
> now. I'm expecting Dave or others work for net-core.
> (http://lkml.org/lkml/2007/12/20/600)

Indeed, I really hope to try and close this out very soon.
Sorry for taking so long.

Re: [PATCH 09/26] atl1: refactor tx processing

[Jay Cliburn]

On Tue, 22 Jan 2008 04:58:17 -0500
Jeff Garzik <jeff@garzik.org> wrote:

> jacliburn@bellsouth.net wrote:
> > From: Jay Cliburn <jacliburn@bellsouth.net>
> > 
> > Refactor tx processing to use a less convoluted tx packet
> > descriptor and to conform generally with the vendor's current
> > version 1.2.40.2.
> > 
> > Signed-off-by: Jay Cliburn <jacliburn@bellsouth.net>
> > ---
> >  drivers/net/atlx/atl1.c |  265
> > +++++++++++++++++++++++++----------------------
> > drivers/net/atlx/atl1.h |  201 +++++++++++++++++++-----------------
> > 2 files changed, 246 insertions(+), 220 deletions(-)
> 
> for such a huge patch, this description is very tiny.  [describe]
> what is refactored, and why.

Okay, I'll go back and rework the offending descriptions for this and
the other patches in this set.

> what does "less convoluted" mean?

I should have written "simpler," I suppose.

Before:
=======
struct tso_param {
	u32 tsopu;      /* tso_param upper word */
	u32 tsopl;      /* tso_param lower word */
};

struct csum_param {
	u32 csumpu;     /* csum_param upper word */
	u32 csumpl;     /* csum_param lower word */
};

union tpd_descr {
	u64 data;
	struct csum_param csum;
	struct tso_param tso;
};

struct tx_packet_desc {
	__le64 buffer_addr;
	union tpd_descr desc;
};


After:
======
struct tx_packet_desc {
        __le64 buffer_addr;
        __le32 word2;
        __le32 word3;
};

Re: [PATCH 09/26] atl1: refactor tx processing

[Jay Cliburn]

On Tue, 22 Jan 2008 18:31:09 -0600
Jay Cliburn <jacliburn@bellsouth.net> wrote:

> On Tue, 22 Jan 2008 04:58:17 -0500
> Jeff Garzik <jeff@garzik.org> wrote:
> 
[...]
> > for such a huge patch, this description is very tiny.  [describe]
> > what is refactored, and why.

Is this one any better?



>From df475e2eea401f9dc18ca23dab538b99fb9e710c Mon Sep 17 00:00:00 2001
From: Jay Cliburn <jacliburn@bellsouth.net>
Date: Wed, 23 Jan 2008 21:36:36 -0600
Subject: [PATCH] atl1: simplify tx packet descriptor

The transmit packet descriptor consists of four 32-bit words, with word 3
upper bits overloaded depending upon the condition of its bits 3 and 4.
The driver currently duplicates all word 2 and some word 3 register bit
definitions unnecessarily and also uses a set of nested structures in its
definition of the TPD without good cause. This patch adds a lengthy
comment describing the TPD, eliminates duplicate TPD bit definitions,
and simplifies the TPD structure itself. It also expands the TSO check
to correctly handle custom checksum versus TSO processing using the revised
TPD definitions. Finally, shorten some variable names in the transmit
processing path to reduce line lengths, rename some variables to better
describe their purpose (e.g., nseg versus m), and add a comment or two
to better describe what the code is doing.

Signed-off-by: Jay Cliburn <jacliburn@bellsouth.net>
---
 drivers/net/atlx/atl1.c |  265 +++++++++++++++++++++++++----------------------
 drivers/net/atlx/atl1.h |  201 +++++++++++++++++++-----------------
 2 files changed, 246 insertions(+), 220 deletions(-)

diff --git a/drivers/net/atlx/atl1.c b/drivers/net/atlx/atl1.c
index 1f564f0..f4add3c 100644
--- a/drivers/net/atlx/atl1.c
+++ b/drivers/net/atlx/atl1.c
@@ -1259,8 +1259,6 @@ static void atl1_intr_tx(struct atl1_adapter *adapter)
 			dev_kfree_skb_irq(buffer_info->skb);
 			buffer_info->skb = NULL;
 		}
-		tpd->buffer_addr = 0;
-		tpd->desc.data = 0;
 
 		if (++sw_tpd_next_to_clean == tpd_ring->count)
 			sw_tpd_next_to_clean = 0;
@@ -1282,48 +1280,69 @@ static u16 atl1_tpd_avail(struct atl1_tpd_ring *tpd_ring)
 }
 
 static int atl1_tso(struct atl1_adapter *adapter, struct sk_buff *skb,
-			 struct tso_param *tso)
+	struct tx_packet_desc *ptpd)
 {
-	/* We enter this function holding a spinlock. */
-	u8 ipofst;
+	/* spinlock held */
+	u8 hdr_len, ip_off;
+	u32 real_len;
 	int err;
 
 	if (skb_shinfo(skb)->gso_size) {
 		if (skb_header_cloned(skb)) {
 			err = pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
 			if (unlikely(err))
-				return err;
+				return -1;
 		}
 
 		if (skb->protocol == ntohs(ETH_P_IP)) {
 			struct iphdr *iph = ip_hdr(skb);
 
-			iph->tot_len = 0;
+			real_len = (((unsigned char *)iph - skb->data) +
+				ntohs(iph->tot_len));
+			if (real_len < skb->len)
+				pskb_trim(skb, real_len);
+			hdr_len = (skb_transport_offset(skb) + tcp_hdrlen(skb));
+			if (skb->len == hdr_len) {
+				iph->check = 0;
+				tcp_hdr(skb)->check =
+					~csum_tcpudp_magic(iph->saddr,
+					iph->daddr, tcp_hdrlen(skb),
+					IPPROTO_TCP, 0);
+				ptpd->word3 |= (iph->ihl & TPD_IPHL_MASK) <<
+					TPD_IPHL_SHIFT;
+				ptpd->word3 |= ((tcp_hdrlen(skb) >> 2) &
+					TPD_TCPHDRLEN_MASK) <<
+					TPD_TCPHDRLEN_SHIFT;
+				ptpd->word3 |= 1 << TPD_IP_CSUM_SHIFT;
+				ptpd->word3 |= 1 << TPD_TCP_CSUM_SHIFT;
+				return 1;
+			}
+
 			iph->check = 0;
 			tcp_hdr(skb)->check = ~csum_tcpudp_magic(iph->saddr,
-				iph->daddr, 0, IPPROTO_TCP, 0);
-			ipofst = skb_network_offset(skb);
-			if (ipofst != ETH_HLEN) /* 802.3 frame */
-				tso->tsopl |= 1 << TSO_PARAM_ETHTYPE_SHIFT;
-
-			tso->tsopl |= (iph->ihl &
-				TSO_PARAM_IPHL_MASK) << TSO_PARAM_IPHL_SHIFT;
-			tso->tsopl |= ((tcp_hdrlen(skb) >> 2) &
-				TSO_PARAM_TCPHDRLEN_MASK) <<
-				TSO_PARAM_TCPHDRLEN_SHIFT;
-			tso->tsopl |= (skb_shinfo(skb)->gso_size &
-				TSO_PARAM_MSS_MASK) << TSO_PARAM_MSS_SHIFT;
-			tso->tsopl |= 1 << TSO_PARAM_IPCKSUM_SHIFT;
-			tso->tsopl |= 1 << TSO_PARAM_TCPCKSUM_SHIFT;
-			tso->tsopl |= 1 << TSO_PARAM_SEGMENT_SHIFT;
-			return true;
+					iph->daddr, 0, IPPROTO_TCP, 0);
+			ip_off = (unsigned char *)iph -
+				(unsigned char *) skb_network_header(skb);
+			if (ip_off == 8) /* 802.3-SNAP frame */
+				ptpd->word3 |= 1 << TPD_ETHTYPE_SHIFT;
+			else if (ip_off != 0)
+				return -2;
+
+			ptpd->word3 |= (iph->ihl & TPD_IPHL_MASK) <<
+				TPD_IPHL_SHIFT;
+			ptpd->word3 |= ((tcp_hdrlen(skb) >> 2) &
+				TPD_TCPHDRLEN_MASK) << TPD_TCPHDRLEN_SHIFT;
+			ptpd->word3 |= (skb_shinfo(skb)->gso_size &
+				TPD_MSS_MASK) << TPD_MSS_SHIFT;
+			ptpd->word3 |= 1 << TPD_SEGMENT_EN_SHIFT;
+			return 3;
 		}
 	}
 	return false;
 }
 
 static int atl1_tx_csum(struct atl1_adapter *adapter, struct sk_buff *skb,
-	struct csum_param *csum)
+	struct tx_packet_desc *ptpd)
 {
 	u8 css, cso;
 
@@ -1335,115 +1354,116 @@ static int atl1_tx_csum(struct atl1_adapter *adapter, struct sk_buff *skb,
 				"payload offset not an even number\n");
 			return -1;
 		}
-		csum->csumpl |= (cso & CSUM_PARAM_PLOADOFFSET_MASK) <<
-			CSUM_PARAM_PLOADOFFSET_SHIFT;
-		csum->csumpl |= (css & CSUM_PARAM_XSUMOFFSET_MASK) <<
-			CSUM_PARAM_XSUMOFFSET_SHIFT;
-		csum->csumpl |= 1 << CSUM_PARAM_CUSTOMCKSUM_SHIFT;
+		ptpd->word3 |= (cso & TPD_PLOADOFFSET_MASK) <<
+			TPD_PLOADOFFSET_SHIFT;
+		ptpd->word3 |= (css & TPD_CCSUMOFFSET_MASK) <<
+			TPD_CCSUMOFFSET_SHIFT;
+		ptpd->word3 |= 1 << TPD_CUST_CSUM_EN_SHIFT;
 		return true;
 	}
-
-	return true;
+	return 0;
 }
 
 static void atl1_tx_map(struct atl1_adapter *adapter, struct sk_buff *skb,
-	bool tcp_seg)
+	struct tx_packet_desc *ptpd)
 {
-	/* We enter this function holding a spinlock. */
+	/* spinlock held */
 	struct atl1_tpd_ring *tpd_ring = &adapter->tpd_ring;
 	struct atl1_buffer *buffer_info;
+	u16 buf_len = skb->len;
 	struct page *page;
-	int first_buf_len = skb->len;
 	unsigned long offset;
 	unsigned int nr_frags;
 	unsigned int f;
-	u16 tpd_next_to_use;
-	u16 proto_hdr_len;
-	u16 len12;
+	int retval;
+	u16 next_to_use;
+	u16 data_len;
+	u8 hdr_len;
 
-	first_buf_len -= skb->data_len;
+	buf_len -= skb->data_len;
 	nr_frags = skb_shinfo(skb)->nr_frags;
-	tpd_next_to_use = atomic_read(&tpd_ring->next_to_use);
-	buffer_info = &tpd_ring->buffer_info[tpd_next_to_use];
+	next_to_use = atomic_read(&tpd_ring->next_to_use);
+	buffer_info = &tpd_ring->buffer_info[next_to_use];
 	if (unlikely(buffer_info->skb))
 		BUG();
 	/* put skb in last TPD */
 	buffer_info->skb = NULL;
 
-	if (tcp_seg) {
-		/* TSO/GSO */
-		proto_hdr_len = skb_transport_offset(skb) + tcp_hdrlen(skb);
-		buffer_info->length = proto_hdr_len;
+	retval = (ptpd->word3 >> TPD_SEGMENT_EN_SHIFT) & TPD_SEGMENT_EN_MASK;
+	if (retval) {
+		/* TSO */
+		hdr_len = skb_transport_offset(skb) + tcp_hdrlen(skb);
+		buffer_info->length = hdr_len;
 		page = virt_to_page(skb->data);
 		offset = (unsigned long)skb->data & ~PAGE_MASK;
 		buffer_info->dma = pci_map_page(adapter->pdev, page,
-						offset, proto_hdr_len,
+						offset, hdr_len,
 						PCI_DMA_TODEVICE);
 
-		if (++tpd_next_to_use == tpd_ring->count)
-			tpd_next_to_use = 0;
+		if (++next_to_use == tpd_ring->count)
+			next_to_use = 0;
 
-		if (first_buf_len > proto_hdr_len) {
-			int i, m;
+		if (buf_len > hdr_len) {
+			int i, nseg;
 
-			len12 = first_buf_len - proto_hdr_len;
-			m = (len12 + ATL1_MAX_TX_BUF_LEN - 1) /
+			data_len = buf_len - hdr_len;
+			nseg = (data_len + ATL1_MAX_TX_BUF_LEN - 1) /
 				ATL1_MAX_TX_BUF_LEN;
-			for (i = 0; i < m; i++) {
+			for (i = 0; i < nseg; i++) {
 				buffer_info =
-				    &tpd_ring->buffer_info[tpd_next_to_use];
+				    &tpd_ring->buffer_info[next_to_use];
 				buffer_info->skb = NULL;
 				buffer_info->length =
 				    (ATL1_MAX_TX_BUF_LEN >=
-				     len12) ? ATL1_MAX_TX_BUF_LEN : len12;
-				len12 -= buffer_info->length;
+				     data_len) ? ATL1_MAX_TX_BUF_LEN : data_len;
+				data_len -= buffer_info->length;
 				page = virt_to_page(skb->data +
-					(proto_hdr_len +
-					i * ATL1_MAX_TX_BUF_LEN));
+					(hdr_len + i * ATL1_MAX_TX_BUF_LEN));
 				offset = (unsigned long)(skb->data +
-					(proto_hdr_len +
-					i * ATL1_MAX_TX_BUF_LEN)) & ~PAGE_MASK;
+					(hdr_len + i * ATL1_MAX_TX_BUF_LEN)) &
+					~PAGE_MASK;
 				buffer_info->dma = pci_map_page(adapter->pdev,
 					page, offset, buffer_info->length,
 					PCI_DMA_TODEVICE);
-				if (++tpd_next_to_use == tpd_ring->count)
-					tpd_next_to_use = 0;
+				if (++next_to_use == tpd_ring->count)
+					next_to_use = 0;
 			}
 		}
 	} else {
-		/* not TSO/GSO */
-		buffer_info->length = first_buf_len;
+		/* not TSO */
+		buffer_info->length = buf_len;
 		page = virt_to_page(skb->data);
 		offset = (unsigned long)skb->data & ~PAGE_MASK;
 		buffer_info->dma = pci_map_page(adapter->pdev, page,
-			offset, first_buf_len, PCI_DMA_TODEVICE);
-		if (++tpd_next_to_use == tpd_ring->count)
-			tpd_next_to_use = 0;
+			offset, buf_len, PCI_DMA_TODEVICE);
+		if (++next_to_use == tpd_ring->count)
+			next_to_use = 0;
 	}
 
 	for (f = 0; f < nr_frags; f++) {
 		struct skb_frag_struct *frag;
-		u16 lenf, i, m;
+		u16 i, nseg;
 
 		frag = &skb_shinfo(skb)->frags[f];
-		lenf = frag->size;
+		buf_len = frag->size;
 
-		m = (lenf + ATL1_MAX_TX_BUF_LEN - 1) / ATL1_MAX_TX_BUF_LEN;
-		for (i = 0; i < m; i++) {
-			buffer_info = &tpd_ring->buffer_info[tpd_next_to_use];
+		nseg = (buf_len + ATL1_MAX_TX_BUF_LEN - 1) /
+			ATL1_MAX_TX_BUF_LEN;
+		for (i = 0; i < nseg; i++) {
+			buffer_info = &tpd_ring->buffer_info[next_to_use];
 			if (unlikely(buffer_info->skb))
 				BUG();
 			buffer_info->skb = NULL;
-			buffer_info->length = (lenf > ATL1_MAX_TX_BUF_LEN) ?
-				ATL1_MAX_TX_BUF_LEN : lenf;
-			lenf -= buffer_info->length;
+			buffer_info->length = (buf_len > ATL1_MAX_TX_BUF_LEN) ?
+				ATL1_MAX_TX_BUF_LEN : buf_len;
+			buf_len -= buffer_info->length;
 			buffer_info->dma = pci_map_page(adapter->pdev,
 				frag->page,
 				frag->page_offset + (i * ATL1_MAX_TX_BUF_LEN),
 				buffer_info->length, PCI_DMA_TODEVICE);
 
-			if (++tpd_next_to_use == tpd_ring->count)
-				tpd_next_to_use = 0;
+			if (++next_to_use == tpd_ring->count)
+				next_to_use = 0;
 		}
 	}
 
@@ -1451,39 +1471,44 @@ static void atl1_tx_map(struct atl1_adapter *adapter, struct sk_buff *skb,
 	buffer_info->skb = skb;
 }
 
-static void atl1_tx_queue(struct atl1_adapter *adapter, int count,
-       union tpd_descr *descr)
+static void atl1_tx_queue(struct atl1_adapter *adapter, u16 count,
+       struct tx_packet_desc *ptpd)
 {
-	/* We enter this function holding a spinlock. */
+	/* spinlock held */
 	struct atl1_tpd_ring *tpd_ring = &adapter->tpd_ring;
-	int j;
-	u32 val;
 	struct atl1_buffer *buffer_info;
 	struct tx_packet_desc *tpd;
-	u16 tpd_next_to_use = atomic_read(&tpd_ring->next_to_use);
+	u16 j;
+	u32 val;
+	u16 next_to_use = (u16) atomic_read(&tpd_ring->next_to_use);
 
 	for (j = 0; j < count; j++) {
-		buffer_info = &tpd_ring->buffer_info[tpd_next_to_use];
-		tpd = ATL1_TPD_DESC(&adapter->tpd_ring, tpd_next_to_use);
-		tpd->desc.csum.csumpu = descr->csum.csumpu;
-		tpd->desc.csum.csumpl = descr->csum.csumpl;
-		tpd->desc.tso.tsopu = descr->tso.tsopu;
-		tpd->desc.tso.tsopl = descr->tso.tsopl;
+		buffer_info = &tpd_ring->buffer_info[next_to_use];
+		tpd = ATL1_TPD_DESC(&adapter->tpd_ring, next_to_use);
+		if (tpd != ptpd)
+			memcpy(tpd, ptpd, sizeof(struct tx_packet_desc));
 		tpd->buffer_addr = cpu_to_le64(buffer_info->dma);
-		tpd->desc.data = descr->data;
-		tpd->desc.tso.tsopu |= (cpu_to_le16(buffer_info->length) &
-			TSO_PARAM_BUFLEN_MASK) << TSO_PARAM_BUFLEN_SHIFT;
+		tpd->word2 = (cpu_to_le16(buffer_info->length) &
+			TPD_BUFLEN_MASK) << TPD_BUFLEN_SHIFT;
 
-		val = (descr->tso.tsopl >> TSO_PARAM_SEGMENT_SHIFT) &
-			TSO_PARAM_SEGMENT_MASK;
-		if (val && !j)
-			tpd->desc.tso.tsopl |= 1 << TSO_PARAM_HDRFLAG_SHIFT;
+		/*
+		 * if this is the first packet in a TSO chain, set
+		 * TPD_HDRFLAG, otherwise, clear it.
+		 */
+		val = (tpd->word3 >> TPD_SEGMENT_EN_SHIFT) &
+			TPD_SEGMENT_EN_MASK;
+		if (val) {
+			if (!j)
+				tpd->word3 |= 1 << TPD_HDRFLAG_SHIFT;
+			else
+				tpd->word3 &= ~(1 << TPD_HDRFLAG_SHIFT);
+		}
 
 		if (j == (count - 1))
-			tpd->desc.tso.tsopl |= 1 << TSO_PARAM_EOP_SHIFT;
+			tpd->word3 |= 1 << TPD_EOP_SHIFT;
 
-		if (++tpd_next_to_use == tpd_ring->count)
-			tpd_next_to_use = 0;
+		if (++next_to_use == tpd_ring->count)
+			next_to_use = 0;
 	}
 	/*
 	 * Force memory writes to complete before letting h/w
@@ -1493,18 +1518,18 @@ static void atl1_tx_queue(struct atl1_adapter *adapter, int count,
 	 */
 	wmb();
 
-	atomic_set(&tpd_ring->next_to_use, (int)tpd_next_to_use);
+	atomic_set(&tpd_ring->next_to_use, next_to_use);
 }
 
 static int atl1_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
 {
 	struct atl1_adapter *adapter = netdev_priv(netdev);
+	struct atl1_tpd_ring *tpd_ring = &adapter->tpd_ring;
 	int len = skb->len;
 	int tso;
 	int count = 1;
 	int ret_val;
-	u32 val;
-	union tpd_descr param;
+	struct tx_packet_desc *ptpd;
 	u16 frag_size;
 	u16 vlan_tag;
 	unsigned long flags;
@@ -1515,18 +1540,11 @@ static int atl1_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
 
 	len -= skb->data_len;
 
-	if (unlikely(skb->len == 0)) {
+	if (unlikely(skb->len <= 0)) {
 		dev_kfree_skb_any(skb);
 		return NETDEV_TX_OK;
 	}
 
-	param.data = 0;
-	param.tso.tsopu = 0;
-	param.tso.tsopl = 0;
-	param.csum.csumpu = 0;
-	param.csum.csumpl = 0;
-
-	/* nr_frags will be nonzero if we're doing scatter/gather (SG) */
 	nr_frags = skb_shinfo(skb)->nr_frags;
 	for (f = 0; f < nr_frags; f++) {
 		frag_size = skb_shinfo(skb)->frags[f].size;
@@ -1535,10 +1553,9 @@ static int atl1_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
 				ATL1_MAX_TX_BUF_LEN;
 	}
 
-	/* mss will be nonzero if we're doing segment offload (TSO/GSO) */
 	mss = skb_shinfo(skb)->gso_size;
 	if (mss) {
-		if (skb->protocol == htons(ETH_P_IP)) {
+		if (skb->protocol == ntohs(ETH_P_IP)) {
 			proto_hdr_len = (skb_transport_offset(skb) +
 					 tcp_hdrlen(skb));
 			if (unlikely(proto_hdr_len > len)) {
@@ -1567,18 +1584,20 @@ static int atl1_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
 		return NETDEV_TX_BUSY;
 	}
 
-	param.data = 0;
+	ptpd = ATL1_TPD_DESC(tpd_ring,
+		(u16) atomic_read(&tpd_ring->next_to_use));
+	memset(ptpd, 0, sizeof(struct tx_packet_desc));
 
 	if (adapter->vlgrp && vlan_tx_tag_present(skb)) {
 		vlan_tag = vlan_tx_tag_get(skb);
 		vlan_tag = (vlan_tag << 4) | (vlan_tag >> 13) |
 			((vlan_tag >> 9) & 0x8);
-		param.tso.tsopl |= 1 << TSO_PARAM_INSVLAG_SHIFT;
-		param.tso.tsopu |= (vlan_tag & TSO_PARAM_VLANTAG_MASK) <<
-			TSO_PARAM_VLAN_SHIFT;
+		ptpd->word3 |= 1 << TPD_INS_VL_TAG_SHIFT;
+		ptpd->word3 |= (vlan_tag & TPD_VL_TAGGED_MASK) <<
+			TPD_VL_TAGGED_SHIFT;
 	}
 
-	tso = atl1_tso(adapter, skb, &param.tso);
+	tso = atl1_tso(adapter, skb, ptpd);
 	if (tso < 0) {
 		spin_unlock_irqrestore(&adapter->lock, flags);
 		dev_kfree_skb_any(skb);
@@ -1586,7 +1605,7 @@ static int atl1_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
 	}
 
 	if (!tso) {
-		ret_val = atl1_tx_csum(adapter, skb, &param.csum);
+		ret_val = atl1_tx_csum(adapter, skb, ptpd);
 		if (ret_val < 0) {
 			spin_unlock_irqrestore(&adapter->lock, flags);
 			dev_kfree_skb_any(skb);
@@ -1594,13 +1613,11 @@ static int atl1_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
 		}
 	}
 
-	val = (param.tso.tsopl >> TSO_PARAM_SEGMENT_SHIFT) &
-		TSO_PARAM_SEGMENT_MASK;
-	atl1_tx_map(adapter, skb, 1 == val);
-	atl1_tx_queue(adapter, count, &param);
-	netdev->trans_start = jiffies;
-	spin_unlock_irqrestore(&adapter->lock, flags);
+	atl1_tx_map(adapter, skb, ptpd);
+	atl1_tx_queue(adapter, count, ptpd);
 	atl1_update_mailbox(adapter);
+	spin_unlock_irqrestore(&adapter->lock, flags);
+	netdev->trans_start = jiffies;
 	return NETDEV_TX_OK;
 }
 
@@ -2759,7 +2776,7 @@ const struct ethtool_ops atl1_ethtool_ops = {
 	.get_ringparam		= atl1_get_ringparam,
 	.set_ringparam		= atl1_set_ringparam,
 	.get_pauseparam		= atl1_get_pauseparam,
-	.set_pauseparam 	= atl1_set_pauseparam,
+	.set_pauseparam		= atl1_set_pauseparam,
 	.get_rx_csum		= atl1_get_rx_csum,
 	.set_tx_csum		= ethtool_op_set_tx_hw_csum,
 	.get_link		= ethtool_op_get_link,
diff --git a/drivers/net/atlx/atl1.h b/drivers/net/atlx/atl1.h
index 30c5a8d..4d3d65b 100644
--- a/drivers/net/atlx/atl1.h
+++ b/drivers/net/atlx/atl1.h
@@ -452,106 +452,115 @@ struct rx_free_desc {
 	/* __attribute__ ((packed)) is required */
 } __attribute__ ((packed));
 
-/* tsopu defines */
-#define TSO_PARAM_BUFLEN_MASK		0x3FFF
-#define TSO_PARAM_BUFLEN_SHIFT		0
-#define TSO_PARAM_DMAINT_MASK		0x0001
-#define TSO_PARAM_DMAINT_SHIFT		14
-#define TSO_PARAM_PKTNT_MASK		0x0001
-#define TSO_PARAM_PKTINT_SHIFT		15
-#define TSO_PARAM_VLANTAG_MASK		0xFFFF
-#define TSO_PARAM_VLAN_SHIFT		16
-
-/* tsopl defines */
-#define TSO_PARAM_EOP_MASK		0x0001
-#define TSO_PARAM_EOP_SHIFT		0
-#define TSO_PARAM_COALESCE_MASK		0x0001
-#define TSO_PARAM_COALESCE_SHIFT	1
-#define TSO_PARAM_INSVLAG_MASK		0x0001
-#define TSO_PARAM_INSVLAG_SHIFT		2
-#define TSO_PARAM_CUSTOMCKSUM_MASK	0x0001
-#define TSO_PARAM_CUSTOMCKSUM_SHIFT	3
-#define TSO_PARAM_SEGMENT_MASK		0x0001
-#define TSO_PARAM_SEGMENT_SHIFT		4
-#define TSO_PARAM_IPCKSUM_MASK		0x0001
-#define TSO_PARAM_IPCKSUM_SHIFT		5
-#define TSO_PARAM_TCPCKSUM_MASK		0x0001
-#define TSO_PARAM_TCPCKSUM_SHIFT	6
-#define TSO_PARAM_UDPCKSUM_MASK		0x0001
-#define TSO_PARAM_UDPCKSUM_SHIFT	7
-#define TSO_PARAM_VLANTAGGED_MASK	0x0001
-#define TSO_PARAM_VLANTAGGED_SHIFT	8
-#define TSO_PARAM_ETHTYPE_MASK		0x0001
-#define TSO_PARAM_ETHTYPE_SHIFT		9
-#define TSO_PARAM_IPHL_MASK		0x000F
-#define TSO_PARAM_IPHL_SHIFT		10
-#define TSO_PARAM_TCPHDRLEN_MASK	0x000F
-#define TSO_PARAM_TCPHDRLEN_SHIFT	14
-#define TSO_PARAM_HDRFLAG_MASK		0x0001
-#define TSO_PARAM_HDRFLAG_SHIFT		18
-#define TSO_PARAM_MSS_MASK		0x1FFF
-#define TSO_PARAM_MSS_SHIFT		19
-
-/* csumpu defines */
-#define CSUM_PARAM_BUFLEN_MASK		0x3FFF
-#define CSUM_PARAM_BUFLEN_SHIFT		0
-#define CSUM_PARAM_DMAINT_MASK		0x0001
-#define CSUM_PARAM_DMAINT_SHIFT		14
-#define CSUM_PARAM_PKTINT_MASK		0x0001
-#define CSUM_PARAM_PKTINT_SHIFT		15
-#define CSUM_PARAM_VALANTAG_MASK	0xFFFF
-#define CSUM_PARAM_VALAN_SHIFT		16
-
-/* csumpl defines*/
-#define CSUM_PARAM_EOP_MASK		0x0001
-#define CSUM_PARAM_EOP_SHIFT		0
-#define CSUM_PARAM_COALESCE_MASK	0x0001
-#define CSUM_PARAM_COALESCE_SHIFT	1
-#define CSUM_PARAM_INSVLAG_MASK		0x0001
-#define CSUM_PARAM_INSVLAG_SHIFT	2
-#define CSUM_PARAM_CUSTOMCKSUM_MASK	0x0001
-#define CSUM_PARAM_CUSTOMCKSUM_SHIFT	3
-#define CSUM_PARAM_SEGMENT_MASK		0x0001
-#define CSUM_PARAM_SEGMENT_SHIFT	4
-#define CSUM_PARAM_IPCKSUM_MASK		0x0001
-#define CSUM_PARAM_IPCKSUM_SHIFT	5
-#define CSUM_PARAM_TCPCKSUM_MASK	0x0001
-#define CSUM_PARAM_TCPCKSUM_SHIFT	6
-#define CSUM_PARAM_UDPCKSUM_MASK	0x0001
-#define CSUM_PARAM_UDPCKSUM_SHIFT	7
-#define CSUM_PARAM_VLANTAGGED_MASK	0x0001
-#define CSUM_PARAM_VLANTAGGED_SHIFT	8
-#define CSUM_PARAM_ETHTYPE_MASK		0x0001
-#define CSUM_PARAM_ETHTYPE_SHIFT	9
-#define CSUM_PARAM_IPHL_MASK		0x000F
-#define CSUM_PARAM_IPHL_SHIFT		10
-#define CSUM_PARAM_PLOADOFFSET_MASK	0x00FF
-#define CSUM_PARAM_PLOADOFFSET_SHIFT	16
-#define CSUM_PARAM_XSUMOFFSET_MASK	0x00FF
-#define CSUM_PARAM_XSUMOFFSET_SHIFT	24
-
-/* TPD descriptor */
-struct tso_param {
-	/* The order of these declarations is important -- don't change it */
-	u32 tsopu;	/* tso_param upper word */
-	u32 tsopl;	/* tso_param lower word */
-};
-
-struct csum_param {
-	/* The order of these declarations is important -- don't change it */
-	u32 csumpu;	/* csum_param upper word */
-	u32 csumpl;	/* csum_param lower word */
-};
+/*
+ * The L1 transmit packet descriptor is comprised of four 32-bit words.
+ *
+ *	31					0
+ *	+---------------------------------------+
+ *      |	Word 0: Buffer addr lo 		|
+ *      +---------------------------------------+
+ *      |	Word 1: Buffer addr hi		|
+ *      +---------------------------------------+
+ *      |		Word 2			|
+ *      +---------------------------------------+
+ *      |		Word 3			|
+ *      +---------------------------------------+
+ *
+ * Words 0 and 1 combine to form a 64-bit buffer address.
+ *
+ * Word 2 is self explanatory in the #define block below.
+ *
+ * Word 3 has two forms, depending upon the state of bits 3 and 4.
+ * If bits 3 and 4 are both zero, then bits 14:31 are unused by the
+ * hardware.  Otherwise, if either bit 3 or 4 is set, the definition
+ * of bits 14:31 vary according to the following depiction.
+ *
+ *	0	End of packet			0	End of packet
+ *	1	Coalesce			1	Coalesce
+ *	2	Insert VLAN tag			2	Insert VLAN tag
+ *	3	Custom csum enable = 0		3	Custom csum enable = 1
+ *	4	Segment enable = 1		4	Segment enable = 0
+ *	5	Generate IP checksum		5	Generate IP checksum
+ *	6	Generate TCP checksum		6	Generate TCP checksum
+ *	7	Generate UDP checksum		7	Generate UDP checksum
+ *	8	VLAN tagged			8	VLAN tagged
+ *	9	Ethernet frame type		9	Ethernet frame type
+ *	10-+ 					10-+
+ *	11 |	IP hdr length (10:13)		11 |	IP hdr length (10:13)
+ *	12 |	(num 32-bit words)		12 |	(num 32-bit words)
+ *	13-+					13-+
+ *	14-+					14	Unused
+ *	15 |	TCP hdr length (14:17)		15	Unused
+ *	16 |	(num 32-bit words)		16-+
+ *	17-+					17 |
+ *	18	Header TPD flag			18 |
+ *	19-+					19 |	Payload offset
+ *	20 |					20 |	    (16:23)
+ *	21 |					21 |
+ *	22 |					22 |
+ *	23 |					23-+
+ *	24 |					24-+
+ *	25 |	MSS (19:31)			25 |
+ *	26 |					26 |
+ *	27 |					27 |	Custom csum offset
+ *	28 |					28 |	     (24:31)
+ *	29 |					29 |
+ *	30 |					30 |
+ *	31-+					31-+
+ */
 
-union tpd_descr {
-	u64 data;
-	struct csum_param csum;
-	struct tso_param tso;
-};
+/* tpd word 2 */
+#define TPD_BUFLEN_MASK		0x3FFF
+#define TPD_BUFLEN_SHIFT	0
+#define TPD_DMAINT_MASK		0x0001
+#define TPD_DMAINT_SHIFT	14
+#define TPD_PKTNT_MASK		0x0001
+#define TPD_PKTINT_SHIFT	15
+#define TPD_VLANTAG_MASK	0xFFFF
+#define TPD_VLAN_SHIFT		16
+
+/* tpd word 3 bits 0:13 */
+#define TPD_EOP_MASK		0x0001
+#define TPD_EOP_SHIFT		0
+#define TPD_COALESCE_MASK	0x0001
+#define TPD_COALESCE_SHIFT	1
+#define TPD_INS_VL_TAG_MASK	0x0001
+#define TPD_INS_VL_TAG_SHIFT	2
+#define TPD_CUST_CSUM_EN_MASK	0x0001
+#define TPD_CUST_CSUM_EN_SHIFT	3
+#define TPD_SEGMENT_EN_MASK	0x0001
+#define TPD_SEGMENT_EN_SHIFT	4
+#define TPD_IP_CSUM_MASK	0x0001
+#define TPD_IP_CSUM_SHIFT	5
+#define TPD_TCP_CSUM_MASK	0x0001
+#define TPD_TCP_CSUM_SHIFT	6
+#define TPD_UDP_CSUM_MASK	0x0001
+#define TPD_UDP_CSUM_SHIFT	7
+#define TPD_VL_TAGGED_MASK	0x0001
+#define TPD_VL_TAGGED_SHIFT	8
+#define TPD_ETHTYPE_MASK	0x0001
+#define TPD_ETHTYPE_SHIFT	9
+#define TPD_IPHL_MASK		0x000F
+#define TPD_IPHL_SHIFT		10
+
+/* tpd word 3 bits 14:31 if segment enabled */
+#define TPD_TCPHDRLEN_MASK	0x000F
+#define TPD_TCPHDRLEN_SHIFT	14
+#define TPD_HDRFLAG_MASK	0x0001
+#define TPD_HDRFLAG_SHIFT	18
+#define TPD_MSS_MASK		0x1FFF
+#define TPD_MSS_SHIFT		19
+
+/* tpd word 3 bits 16:31 if custom csum enabled */
+#define TPD_PLOADOFFSET_MASK	0x00FF
+#define TPD_PLOADOFFSET_SHIFT	16
+#define TPD_CCSUMOFFSET_MASK	0x00FF
+#define TPD_CCSUMOFFSET_SHIFT	24
 
 struct tx_packet_desc {
 	__le64 buffer_addr;
-	union tpd_descr desc;
+	__le32 word2;
+	__le32 word3;
 };
 
 /* DMA Order Settings */
-- 
1.5.3.7

Re: [PATCH 09/26] atl1: refactor tx processing

[Chris Snook]

Jay Cliburn wrote:
> On Tue, 22 Jan 2008 18:31:09 -0600
> Jay Cliburn <jacliburn@bellsouth.net> wrote:
> 
>> On Tue, 22 Jan 2008 04:58:17 -0500
>> Jeff Garzik <jeff@garzik.org> wrote:
>>
> [...]
>>> for such a huge patch, this description is very tiny.  [describe]
>>> what is refactored, and why.
> 
> Is this one any better?

This satisfies me.

Acked-by: Chris Snook <csnook@redhat.com>

> From df475e2eea401f9dc18ca23dab538b99fb9e710c Mon Sep 17 00:00:00 2001
> From: Jay Cliburn <jacliburn@bellsouth.net>
> Date: Wed, 23 Jan 2008 21:36:36 -0600
> Subject: [PATCH] atl1: simplify tx packet descriptor
> 
> The transmit packet descriptor consists of four 32-bit words, with word 3
> upper bits overloaded depending upon the condition of its bits 3 and 4.
> The driver currently duplicates all word 2 and some word 3 register bit
> definitions unnecessarily and also uses a set of nested structures in its
> definition of the TPD without good cause. This patch adds a lengthy
> comment describing the TPD, eliminates duplicate TPD bit definitions,
> and simplifies the TPD structure itself. It also expands the TSO check
> to correctly handle custom checksum versus TSO processing using the revised
> TPD definitions. Finally, shorten some variable names in the transmit
> processing path to reduce line lengths, rename some variables to better
> describe their purpose (e.g., nseg versus m), and add a comment or two
> to better describe what the code is doing.
> 
> Signed-off-by: Jay Cliburn <jacliburn@bellsouth.net>

Re: [PATCH 09/26] atl1: refactor tx processing

[Jeff Garzik]

Jay Cliburn wrote:
> On Tue, 22 Jan 2008 18:31:09 -0600
> Jay Cliburn <jacliburn@bellsouth.net> wrote:
> 
>> On Tue, 22 Jan 2008 04:58:17 -0500
>> Jeff Garzik <jeff@garzik.org> wrote:
>>
> [...]
>>> for such a huge patch, this description is very tiny.  [describe]
>>> what is refactored, and why.
> 
> Is this one any better?
> 
> 
> 
>>From df475e2eea401f9dc18ca23dab538b99fb9e710c Mon Sep 17 00:00:00 2001
> From: Jay Cliburn <jacliburn@bellsouth.net>
> Date: Wed, 23 Jan 2008 21:36:36 -0600
> Subject: [PATCH] atl1: simplify tx packet descriptor
> 
> The transmit packet descriptor consists of four 32-bit words, with word 3
> upper bits overloaded depending upon the condition of its bits 3 and 4.
> The driver currently duplicates all word 2 and some word 3 register bit
> definitions unnecessarily and also uses a set of nested structures in its
> definition of the TPD without good cause. This patch adds a lengthy
> comment describing the TPD, eliminates duplicate TPD bit definitions,
> and simplifies the TPD structure itself. It also expands the TSO check
> to correctly handle custom checksum versus TSO processing using the revised
> TPD definitions. Finally, shorten some variable names in the transmit
> processing path to reduce line lengths, rename some variables to better
> describe their purpose (e.g., nseg versus m), and add a comment or two
> to better describe what the code is doing.
> 
> Signed-off-by: Jay Cliburn <jacliburn@bellsouth.net>
> ---
>  drivers/net/atlx/atl1.c |  265 +++++++++++++++++++++++++----------------------
>  drivers/net/atlx/atl1.h |  201 +++++++++++++++++++-----------------
>  2 files changed, 246 insertions(+), 220 deletions(-)

Yep, better.

	Jeff

Re: [PATCH 06/26] atl1: update initialization parameters

[Jay Cliburn]

On Tue, 22 Jan 2008 04:56:11 -0500
Jeff Garzik <jeff@garzik.org> wrote:

> jacliburn@bellsouth.net wrote:
> > From: Jay Cliburn <jacliburn@bellsouth.net>
> > 
> > Update initialization parameters to match the current vendor driver
> > version 1.2.40.2.

[...]

> ACK without any better knowledge...  but is any addition insight 
> available at all?

No, sorry Jeff.  I simply took the vendor's current driver and matched
his initialization settings.  I can only assume he discovered these
values through lab testing.

For this and the other "conform to vendor driver" patches in this set, I
thought it important to have the in-tree driver match the vendor driver
as closely as possible.  The primary motivations are (1) my belief that
he's in a better position to test the NIC, and (2) to be able to go to
him for assistance occasionally and not be rejected because of
significant differences between his and our drivers.

Jay

Re: [PATCH 06/26] atl1: update initialization parameters

[Jeff Garzik]

Jay Cliburn wrote:
> On Tue, 22 Jan 2008 04:56:11 -0500
> Jeff Garzik <jeff@garzik.org> wrote:
> 
>> jacliburn@bellsouth.net wrote:
>>> From: Jay Cliburn <jacliburn@bellsouth.net>
>>>
>>> Update initialization parameters to match the current vendor driver
>>> version 1.2.40.2.
> 
> [...]
> 
>> ACK without any better knowledge...  but is any addition insight 
>> available at all?
> 
> No, sorry Jeff.  I simply took the vendor's current driver and matched
> his initialization settings.  I can only assume he discovered these
> values through lab testing.
> 
> For this and the other "conform to vendor driver" patches in this set, I
> thought it important to have the in-tree driver match the vendor driver
> as closely as possible.  The primary motivations are (1) my belief that
> he's in a better position to test the NIC, and (2) to be able to go to
> him for assistance occasionally and not be rejected because of
> significant differences between his and our drivers.

Since these changes are not simply moving code around, we really do need 
full explanations for them, and to understand their need.

Blindly copying code from an exterior driver is pointless, and no way at 
all to run an engineering process.

If the driver is not going to get the review and attention necessary, 
bug fixes and feedback attended-to, then there's not much point in 
having this driver in the kernel at all.

You will only lead yourself to frustration, if you set up a system where 
changes only flow one way.  That's not how Linux development is done at all.

	Jeff

Re: [PATCH 06/26] atl1: update initialization parameters

[Chris Snook]

Jay Cliburn wrote:
> On Tue, 22 Jan 2008 04:56:11 -0500
> Jeff Garzik <jeff@garzik.org> wrote:
> 
>> jacliburn@bellsouth.net wrote:
>>> From: Jay Cliburn <jacliburn@bellsouth.net>
>>>
>>> Update initialization parameters to match the current vendor driver
>>> version 1.2.40.2.
> 
> [...]
> 
>> ACK without any better knowledge...  but is any addition insight 
>> available at all?
> 
> No, sorry Jeff.  I simply took the vendor's current driver and matched
> his initialization settings.  I can only assume he discovered these
> values through lab testing.
> 
> For this and the other "conform to vendor driver" patches in this set, I
> thought it important to have the in-tree driver match the vendor driver
> as closely as possible.  The primary motivations are (1) my belief that
> he's in a better position to test the NIC, and (2) to be able to go to
> him for assistance occasionally and not be rejected because of
> significant differences between his and our drivers.

I don't think we should be doing this without justification.  From all the atl1 
and atl2 code I've looked at, I've gotten the impression that their driver 
development processes are extremely ad-hoc.  There is code in the Atheros 
version of atl2 that cannot *possibly* apply to that hardware and was just 
copied and pasted from atl1, just as much of atl1 was copied and pasted from 
e1000.  The fact that various versions have different magic numbers may simply 
mean they copied and pasted from different irrelevant and incorrect sources.

Our contacts at Atheros seem to be very good electrical engineers, so when they 
tell us that a certain setting should be changed to match particular properties 
of the hardware, I trust them.  They are not, however, experienced and 
disciplined kernel developers, so absent such justification I think we should 
stick with what we have, which has been improved and reviewed by people who 
*are* experienced and disciplined kernel developers.

We have at least as much to teach Atheros about writing kernel code as they have 
to teach us about their hardware.

	-- Chris