Skip to contentSkip to conversation

netxen bug fixes

PATCH Applied [email protected] 3 participants 19 messages Dec 26, 2007 – Jan 14, 2008

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

cover netxen bug fixes

Resending recent patch set with an additional patch to fix byte ordering in tx desc besides the original bug fixes and some enhancements in tx and rx.

Recreated these patches from a fresh cloned tree.

Thanks,
Dhananjay

Signature · 9 lines
 MAINTAINERS                              |    4 +-
 drivers/net/netxen/netxen_nic.h          |   76 +++++++-------
 drivers/net/netxen/netxen_nic_hdr.h      |   12 ++
 drivers/net/netxen/netxen_nic_hw.c       |    2 +
 drivers/net/netxen/netxen_nic_init.c     |   77 +++-----------
 drivers/net/netxen/netxen_nic_main.c     |  174 ++++++++++--------------------
 drivers/net/netxen/netxen_nic_niu.c      |    8 +-
 drivers/net/netxen/netxen_nic_phan_reg.h |    3 +
 8 files changed, 133 insertions(+), 223 deletions(-)
#1 lore raw

PATCH 1/7 netxen: update MAINTAINERS

Changing MAINTAINERS for netxen nic driver.

MAINTAINERS +2 -2
Index: upstream/MAINTAINERS
===================================================================
@@ -2738,8 +2738,8 @@ T:	git kernel.org:/pub/scm/linux/kernel/
 S:	Maintained
 
 NETXEN (1/10) GbE SUPPORT
-P:	Amit S. Kale
-M:	[email protected]
+P:	Dhananjay Phadke
+M:	[email protected]
 L:	[email protected]
 W:	http://www.netxen.com
 S:	Supported
#2 lore raw

Re: [patch 1/7] netxen: update MAINTAINERS

applied 1-2 to #upstream-fixes

#3 lore raw

PATCH 2/7 netxen: update driver version

In reply to [email protected]

Bumping up driver version to 3.4.18, several fixes have gone in since version 3.4.2.

drivers/net/netxen/netxen_nic.h +2 -2
Index: upstream/drivers/net/netxen/netxen_nic.h
===================================================================
@@ -65,8 +65,8 @@
 
 #define _NETXEN_NIC_LINUX_MAJOR 3
 #define _NETXEN_NIC_LINUX_MINOR 4
-#define _NETXEN_NIC_LINUX_SUBVERSION 2
-#define NETXEN_NIC_LINUX_VERSIONID  "3.4.2"
+#define _NETXEN_NIC_LINUX_SUBVERSION 18
+#define NETXEN_NIC_LINUX_VERSIONID  "3.4.18"
 
 #define NETXEN_NUM_FLASH_SECTORS (64)
 #define NETXEN_FLASH_SECTOR_SIZE (64 * 1024)
#4 lore raw

PATCH 3/7 netxen: improve MSI interrupt handling

In reply to [email protected]

Recent netxen firmware has new scheme of generating MSI interrupts, it raises interrupt and blocks itself, waiting for driver to unmask. This reduces chance of spurious interrupts.

The driver will be able to deal with older firmware as well.

drivers/net/netxen/netxen_nic_hw.c +2 -0
Index: upstream/drivers/net/netxen/netxen_nic_hw.c
===================================================================
@@ -398,6 +398,8 @@ int netxen_nic_hw_resources(struct netxe
 		NETXEN_CRB_NORMALIZE(adapter, CRB_NIC_CAPABILITIES_FW));
 	printk(KERN_NOTICE "%s: FW capabilities:0x%x\n", netxen_nic_driver_name,
 			adapter->intr_scheme);
+	adapter->msi_mode = readl(
+		NETXEN_CRB_NORMALIZE(adapter, CRB_NIC_MSI_MODE_FW));
 	DPRINTK(INFO, "Receive Peg ready too. starting stuff\n");
 
 	addr = netxen_alloc(adapter->ahw.pdev,
drivers/net/netxen/netxen_nic_main.c +24 -30

95 lines not on this page · raw message

drivers/net/netxen/netxen_nic.h +1 -0
Index: upstream/drivers/net/netxen/netxen_nic.h
===================================================================
@@ -939,6 +939,7 @@ struct netxen_adapter {
 	struct pci_dev *ctx_desc_pdev;
 	dma_addr_t ctx_desc_phys_addr;
 	int intr_scheme;
+	int msi_mode;
 	int (*enable_phy_interrupts) (struct netxen_adapter *);
 	int (*disable_phy_interrupts) (struct netxen_adapter *);
 	void (*handle_phy_intr) (struct netxen_adapter *);
drivers/net/netxen/netxen_nic_phan_reg.h +3 -0
Index: upstream/drivers/net/netxen/netxen_nic_phan_reg.h
===================================================================
@@ -126,8 +126,11 @@
  */
 #define CRB_NIC_CAPABILITIES_HOST	NETXEN_NIC_REG(0x1a8)
 #define CRB_NIC_CAPABILITIES_FW	  	NETXEN_NIC_REG(0x1dc)
+#define CRB_NIC_MSI_MODE_HOST		NETXEN_NIC_REG(0x270)
+#define CRB_NIC_MSI_MODE_FW	  		NETXEN_NIC_REG(0x274)
 
 #define INTR_SCHEME_PERPORT	      	0x1
+#define MSI_MODE_MULTIFUNC	      	0x1
 
 /* used for ethtool tests */
 #define CRB_SCRATCHPAD_TEST	    NETXEN_NIC_REG(0x280)
drivers/net/netxen/netxen_nic_hdr.h +12 -0
Index: upstream/drivers/net/netxen/netxen_nic_hdr.h
===================================================================
@@ -456,6 +456,12 @@ enum {
 #define ISR_INT_MASK_SLOW	(NETXEN_PCIX_PS_REG(PCIX_INT_MASK))
 #define ISR_INT_TARGET_STATUS	(NETXEN_PCIX_PS_REG(PCIX_TARGET_STATUS))
 #define ISR_INT_TARGET_MASK	(NETXEN_PCIX_PS_REG(PCIX_TARGET_MASK))
+#define ISR_INT_TARGET_STATUS_F1   (NETXEN_PCIX_PS_REG(PCIX_TARGET_STATUS_F1))
+#define ISR_INT_TARGET_MASK_F1     (NETXEN_PCIX_PS_REG(PCIX_TARGET_MASK_F1))
+#define ISR_INT_TARGET_STATUS_F2   (NETXEN_PCIX_PS_REG(PCIX_TARGET_STATUS_F2))
+#define ISR_INT_TARGET_MASK_F2     (NETXEN_PCIX_PS_REG(PCIX_TARGET_MASK_F2))
+#define ISR_INT_TARGET_STATUS_F3   (NETXEN_PCIX_PS_REG(PCIX_TARGET_STATUS_F3))
+#define ISR_INT_TARGET_MASK_F3     (NETXEN_PCIX_PS_REG(PCIX_TARGET_MASK_F3))
 
 #define NETXEN_PCI_MAPSIZE	128
 #define NETXEN_PCI_DDR_NET	(0x00000000UL)
@@ -662,6 +668,12 @@ enum {
 
 #define PCIX_TARGET_STATUS	(0x10118)
 #define PCIX_TARGET_MASK	(0x10128)
+#define PCIX_TARGET_STATUS_F1 (0x10160)
+#define PCIX_TARGET_MASK_F1   (0x10170)
+#define PCIX_TARGET_STATUS_F2 (0x10164)
+#define PCIX_TARGET_MASK_F2   (0x10174)
+#define PCIX_TARGET_STATUS_F3 (0x10168)
+#define PCIX_TARGET_MASK_F3   (0x10178)
 
 #define PCIX_MSI_F0		(0x13000)
 #define PCIX_MSI_F1		(0x13004)
drivers/net/netxen/netxen_nic_init.c +2 -0
Index: upstream/drivers/net/netxen/netxen_nic_init.c
===================================================================
@@ -145,6 +145,8 @@ int netxen_init_firmware(struct netxen_a
 	/* Window 1 call */
 	writel(INTR_SCHEME_PERPORT,
 	       NETXEN_CRB_NORMALIZE(adapter, CRB_NIC_CAPABILITIES_HOST));
+	writel(MSI_MODE_MULTIFUNC,
+	       NETXEN_CRB_NORMALIZE(adapter, CRB_NIC_MSI_MODE_HOST));
 	writel(MPORT_MULTI_FUNCTION_MODE,
 	       NETXEN_CRB_NORMALIZE(adapter, CRB_MPORT_MODE));
 	writel(PHAN_INITIALIZE_ACK,
#5 lore raw

Re: [patch 3/7] netxen: improve MSI interrupt handling

patch failed to apply (possibly because of recent NAPI updates)

#6 lore raw

PATCH 4/7 netxen: stop second phy correctly

In reply to [email protected]

This patch fixes bug that doesn't quiesce second port when interface is brought down, which could lead to unwarranted interrupt during rmmod / ifdown.

drivers/net/netxen/netxen_nic_niu.c +4 -4
Index: upstream/drivers/net/netxen/netxen_nic_niu.c
===================================================================
@@ -742,12 +742,12 @@ int netxen_niu_disable_xg_port(struct ne
 	__u32 mac_cfg;
 	u32 port = physical_port[adapter->portnum];
 
-	if (port != 0)
+	if (port > NETXEN_NIU_MAX_XG_PORTS)
 		return -EINVAL;
+
 	mac_cfg = 0;
-	netxen_xg_soft_reset(mac_cfg);
-	if (netxen_nic_hw_write_wx(adapter, NETXEN_NIU_XGE_CONFIG_0,
-				   &mac_cfg, 4))
+	if (netxen_nic_hw_write_wx(adapter,
+		NETXEN_NIU_XGE_CONFIG_0 + (0x10000 * port), &mac_cfg, 4))
 		return -EIO;
 	return 0;
 }
drivers/net/netxen/netxen_nic_main.c +3 -5
Index: upstream/drivers/net/netxen/netxen_nic_main.c
===================================================================
@@ -725,11 +725,6 @@ static void __devexit netxen_nic_remove(
 
 	unregister_netdev(netdev);
 
-	if (adapter->stop_port)
-		adapter->stop_port(adapter);
-
-	netxen_nic_disable_int(adapter);
-
 	if (adapter->is_up == NETXEN_ADAPTER_UP_MAGIC) {
 		init_firmware_done++;
 		netxen_free_hw_resources(adapter);
@@ -912,6 +907,9 @@ static int netxen_nic_close(struct net_d
 	netif_stop_queue(netdev);
 	napi_disable(&adapter->napi);
 
+	if (adapter->stop_port)
+		adapter->stop_port(adapter);
+
 	netxen_nic_disable_int(adapter);
 
 	cmd_buff = adapter->cmd_buf_arr;
Applied
#7 lore raw

Re: [patch 4/7] netxen: stop second phy correctly

applied

#8 lore raw

PATCH 5/7 netxen: fix race in interrupt / napi

In reply to [email protected]

This patch simplifies netxen ISR and poll() routine. Interrupts are not unmasked in interrupt routine based on a racy has_work() checks, but left to the napi poll function to enable them.

This also fixes crash in netif_rx_action(), when work_done == budget.

drivers/net/netxen/netxen_nic_main.c +10 -50
Index: upstream/drivers/net/netxen/netxen_nic_main.c
===================================================================
@@ -63,7 +63,6 @@ static int netxen_nic_xmit_frame(struct 
 static void netxen_tx_timeout(struct net_device *netdev);
 static void netxen_tx_timeout_task(struct work_struct *work);
 static void netxen_watchdog(unsigned long);
-static int netxen_handle_int(struct netxen_adapter *, struct net_device *);
 static int netxen_nic_poll(struct napi_struct *napi, int budget);
 #ifdef CONFIG_NET_POLL_CONTROLLER
 static void netxen_nic_poll_controller(struct net_device *netdev);
@@ -1218,40 +1217,6 @@ static void netxen_tx_timeout_task(struc
 	netif_wake_queue(adapter->netdev);
 }
 
-static int
-netxen_handle_int(struct netxen_adapter *adapter, struct net_device *netdev)
-{
-	u32 ret = 0;
-
-	DPRINTK(INFO, "Entered handle ISR\n");
-	adapter->stats.ints++;
-
-	netxen_nic_disable_int(adapter);
-
-	if (netxen_nic_rx_has_work(adapter) || netxen_nic_tx_has_work(adapter)) {
-		if (netif_rx_schedule_prep(netdev, &adapter->napi)) {
-			/*
-			 * Interrupts are already disabled.
-			 */
-			__netif_rx_schedule(netdev, &adapter->napi);
-		} else {
-			static unsigned int intcount = 0;
-			if ((++intcount & 0xfff) == 0xfff)
-				DPRINTK(KERN_ERR
-				       "%s: %s interrupt %d while in poll\n",
-				       netxen_nic_driver_name, netdev->name,
-				       intcount);
-		}
-		ret = 1;
-	}
-
-	if (ret == 0) {
-		netxen_nic_enable_int(adapter);
-	}
-
-	return ret;
-}
-
 /*
  * netxen_intr - Interrupt Handler
  * @irq: interrupt number
@@ -1278,8 +1243,12 @@ irqreturn_t netxen_intr(int irq, void *d
 		}
 	}
 
-	if (netif_running(netdev))
-		netxen_handle_int(adapter, netdev);
+	adapter->stats.ints++;
+
+	if (netif_rx_schedule_prep(netdev, &adapter->napi)) {
+		netxen_nic_disable_int(adapter);
+		__netif_rx_schedule(netdev, &adapter->napi);
+	}
 
 	return IRQ_HANDLED;
 }
@@ -1287,12 +1256,11 @@ irqreturn_t netxen_intr(int irq, void *d
 static int netxen_nic_poll(struct napi_struct *napi, int budget)
 {
 	struct netxen_adapter *adapter = container_of(napi, struct netxen_adapter, napi);
-	struct net_device *netdev = adapter->netdev;
-	int done = 1;
+	int tx_complete;
 	int ctx;
 	int work_done;
 
-	DPRINTK(INFO, "polling for %d descriptors\n", *budget);
+	tx_complete = netxen_process_cmd_ring(adapter);
 
 	work_done = 0;
 	for (ctx = 0; ctx < MAX_RCV_CTX; ++ctx) {
@@ -1312,16 +1280,8 @@ static int netxen_nic_poll(struct napi_s
 						     budget / MAX_RCV_CTX);
 	}
 
-	if (work_done >= budget && netxen_nic_rx_has_work(adapter) != 0)
-		done = 0;
-
-	if (netxen_process_cmd_ring((unsigned long)adapter) == 0)
-		done = 0;
-
-	DPRINTK(INFO, "new work_done: %d work_to_do: %d\n",
-		work_done, work_to_do);
-	if (done) {
-		netif_rx_complete(netdev, napi);
+	if ((work_done < budget) && tx_complete) {
+		netif_rx_complete(adapter->netdev, &adapter->napi);
 		netxen_nic_enable_int(adapter);
 	}
 
drivers/net/netxen/netxen_nic.h +1 -4
Index: upstream/drivers/net/netxen/netxen_nic.h
===================================================================
@@ -839,7 +839,6 @@ struct netxen_rcv_desc_ctx {
 	u32 flags;
 	u32 producer;
 	u32 rcv_pending;	/* Num of bufs posted in phantom */
-	u32 rcv_free;		/* Num of bufs in free list */
 	dma_addr_t phys_addr;
 	struct pci_dev *phys_pdev;
 	struct rcv_desc *desc_head;	/* address of rx ring in Phantom */
@@ -1073,12 +1072,10 @@ void netxen_tso_check(struct netxen_adap
 		      struct cmd_desc_type0 *desc, struct sk_buff *skb);
 int netxen_nic_hw_resources(struct netxen_adapter *adapter);
 void netxen_nic_clear_stats(struct netxen_adapter *adapter);
-int netxen_nic_rx_has_work(struct netxen_adapter *adapter);
-int netxen_nic_tx_has_work(struct netxen_adapter *adapter);
 void netxen_watchdog_task(struct work_struct *work);
 void netxen_post_rx_buffers(struct netxen_adapter *adapter, u32 ctx,
 			    u32 ringid);
-int netxen_process_cmd_ring(unsigned long data);
+int netxen_process_cmd_ring(struct netxen_adapter *adapter);
 u32 netxen_process_rcv_ring(struct netxen_adapter *adapter, int ctx, int max);
 void netxen_nic_set_multi(struct net_device *netdev);
 int netxen_nic_change_mtu(struct net_device *netdev, int new_mtu);
drivers/net/netxen/netxen_nic_init.c +3 -50

124 lines not on this page · raw message

#9 lore raw

Re: [patch 5/7] netxen: fix race in interrupt / napi

patch conflicted with

commit 1706287f6eb58726a9a0e5cbbde87f49757615e3

Author: David S. Miller <[email protected]>
Date:   Mon Jan 7 20:51:29 2008 -0800

[NETXEN]: Fix ->poll() done logic.

If work_done >= budget we should always elide the NAPI completion.

#10 lore raw

Re: [patch 5/7] netxen: fix race in interrupt / napi

Ok, I will respin the failed patches.

Thanks,
-Dhananjay

Quoted 14 lines · Jeff Garzik

patch conflicted with

commit 1706287f6eb58726a9a0e5cbbde87f49757615e3

Author: David S. Miller <[email protected]>
Date:   Mon Jan 7 20:51:29 2008 -0800

[NETXEN]: Fix ->poll() done logic.

If work_done >= budget we should always elide the NAPI completion.

#11 lore raw

PATCH 6/7 netxen: optimize tx handling

In reply to [email protected]

netxen driver allows limited number of threads simultaneously posting skb's in tx ring. If transmit slot is unavailable, driver calls schedule() or loops in xmit_frame().

This patch returns TX_BUSY and lets the stack reschedule the packet if transmit slot is unavailable. Also removes unnecessary check for tx timeout in the driver itself, the network stack does that anyway.

drivers/net/netxen/netxen_nic_main.c +21 -31
Index: upstream/drivers/net/netxen/netxen_nic_main.c
===================================================================
@@ -986,28 +986,6 @@ static int netxen_nic_xmit_frame(struct 
 		return NETDEV_TX_OK;
 	}
 
-	/*
-	 * Everything is set up. Now, we just need to transmit it out.
-	 * Note that we have to copy the contents of buffer over to
-	 * right place. Later on, this can be optimized out by de-coupling the
-	 * producer index from the buffer index.
-	 */
-      retry_getting_window:
-	spin_lock_bh(&adapter->tx_lock);
-	if (adapter->total_threads >= MAX_XMIT_PRODUCERS) {
-		spin_unlock_bh(&adapter->tx_lock);
-		/*
-		 * Yield CPU
-		 */
-		if (!in_atomic())
-			schedule();
-		else {
-			for (i = 0; i < 20; i++)
-				cpu_relax();	/*This a nop instr on i386 */
-		}
-		goto retry_getting_window;
-	}
-	local_producer = adapter->cmd_producer;
 	/* There 4 fragments per descriptor */
 	no_of_desc = (frag_count + 3) >> 2;
 	if (netdev->features & NETIF_F_TSO) {
@@ -1021,16 +999,19 @@ static int netxen_nic_xmit_frame(struct 
 			}
 		}
 	}
+
+	spin_lock_bh(&adapter->tx_lock);
+	if (adapter->total_threads >= MAX_XMIT_PRODUCERS) {
+		goto out_requeue;
+	}
+	local_producer = adapter->cmd_producer;
 	k = adapter->cmd_producer;
 	max_tx_desc_count = adapter->max_tx_desc_count;
 	last_cmd_consumer = adapter->last_cmd_consumer;
 	if ((k + no_of_desc) >=
 	    ((last_cmd_consumer <= k) ? last_cmd_consumer + max_tx_desc_count :
 	     last_cmd_consumer)) {
-		netif_stop_queue(netdev);
-		adapter->flags |= NETXEN_NETDEV_STATUS;
-		spin_unlock_bh(&adapter->tx_lock);
-		return NETDEV_TX_BUSY;
+		goto out_requeue;
 	}
 	k = get_index_range(k, max_tx_desc_count, no_of_desc);
 	adapter->cmd_producer = k;
@@ -1083,6 +1064,8 @@ static int netxen_nic_xmit_frame(struct 
 						  adapter->max_tx_desc_count);
 			hwdesc = &hw->cmd_desc_head[producer];
 			memset(hwdesc, 0, sizeof(struct cmd_desc_type0));
+			pbuf = &adapter->cmd_buf_arr[producer];
+			pbuf->skb = NULL;
 		}
 		frag = &skb_shinfo(skb)->frags[i - 1];
 		len = frag->size;
@@ -1138,6 +1121,8 @@ static int netxen_nic_xmit_frame(struct 
 		}
 		/* copy the MAC/IP/TCP headers to the cmd descriptor list */
 		hwdesc = &hw->cmd_desc_head[producer];
+		pbuf = &adapter->cmd_buf_arr[producer];
+		pbuf->skb = NULL;
 
 		/* copy the first 64 bytes */
 		memcpy(((void *)hwdesc) + 2,
@@ -1146,6 +1131,8 @@ static int netxen_nic_xmit_frame(struct 
 
 		if (more_hdr) {
 			hwdesc = &hw->cmd_desc_head[producer];
+			pbuf = &adapter->cmd_buf_arr[producer];
+			pbuf->skb = NULL;
 			/* copy the next 64 bytes - should be enough except
 			 * for pathological case
 			 */
@@ -1179,14 +1166,17 @@ static int netxen_nic_xmit_frame(struct 
 	}
 
 	adapter->stats.xmitfinished++;
-	spin_unlock_bh(&adapter->tx_lock);
-
 	netdev->trans_start = jiffies;
 
-	DPRINTK(INFO, "wrote CMD producer %x to phantom\n", producer);
-
-	DPRINTK(INFO, "Done. Send\n");
+	spin_unlock_bh(&adapter->tx_lock);
 	return NETDEV_TX_OK;
+
+out_requeue:
+	netif_stop_queue(netdev);
+	adapter->flags |= NETXEN_NETDEV_STATUS;
+
+	spin_unlock_bh(&adapter->tx_lock);
+	return NETDEV_TX_BUSY;
 }
 
 static void netxen_watchdog(unsigned long v)
drivers/net/netxen/netxen_nic_init.c +4 -6
Index: upstream/drivers/net/netxen/netxen_nic_init.c
===================================================================
@@ -1229,7 +1229,6 @@ int netxen_process_cmd_ring(struct netxe
 	struct pci_dev *pdev;
 	struct netxen_skb_frag *frag;
 	u32 i;
-	struct sk_buff *skb = NULL;
 	int done;
 
 	spin_lock(&adapter->tx_lock);
@@ -1259,9 +1258,8 @@ int netxen_process_cmd_ring(struct netxe
 	while ((last_consumer != consumer) && (count1 < MAX_STATUS_HANDLE)) {
 		buffer = &adapter->cmd_buf_arr[last_consumer];
 		pdev = adapter->pdev;
-		frag = &buffer->frag_array[0];
-		skb = buffer->skb;
-		if (skb && (cmpxchg(&buffer->skb, skb, 0) == skb)) {
+		if (buffer->skb) {
+			frag = &buffer->frag_array[0];
 			pci_unmap_single(pdev, frag->dma, frag->length,
 					 PCI_DMA_TODEVICE);
 			frag->dma = 0ULL;
@@ -1274,8 +1272,8 @@ int netxen_process_cmd_ring(struct netxe
 			}
 
 			adapter->stats.skbfreed++;
-			dev_kfree_skb_any(skb);
-			skb = NULL;
+			dev_kfree_skb_any(buffer->skb);
+			buffer->skb = NULL;
 		} else if (adapter->proc_cmd_buf_counter == 1) {
 			adapter->stats.txnullskb++;
 		}
Applied
#12 lore raw

Re: [patch 6/7] netxen: optimize tx handling

applied

#13 lore raw

PATCH 7/7 netxen: fix byte-swapping in tx and rx

In reply to [email protected]

This cleans up some unnecessary byte-swapping while setting up tx and interpreting rx desc. The 64 bit rx status data should be converted to host endian format only once and the macros just need to extract bitfields.

This saves a spate of interrupts on pseries blades caused by buggy (non) processing rx status ring.

drivers/net/netxen/netxen_nic.h +33 -33
Index: netdev-2.6/drivers/net/netxen/netxen_nic.h
===================================================================
@@ -309,23 +309,23 @@ struct netxen_ring_ctx {
 	((cmd_desc)->port_ctxid |= ((var) & 0xF0))
 
 #define netxen_set_cmd_desc_flags(cmd_desc, val)	\
-	((cmd_desc)->flags_opcode &= ~cpu_to_le16(0x7f), \
-	(cmd_desc)->flags_opcode |= cpu_to_le16((val) & 0x7f))
+	((cmd_desc)->flags_opcode &= ~(0x7f), \
+	(cmd_desc)->flags_opcode |= (val) & 0x7f)
 #define netxen_set_cmd_desc_opcode(cmd_desc, val)	\
-	((cmd_desc)->flags_opcode &= ~cpu_to_le16(0x3f<<7), \
-	(cmd_desc)->flags_opcode |= cpu_to_le16(((val & 0x3f)<<7)))
+	((cmd_desc)->flags_opcode &= ~(0x3f<<7), \
+	(cmd_desc)->flags_opcode |= ((val) & 0x3f) << 7)
 
 #define netxen_set_cmd_desc_num_of_buff(cmd_desc, val)	\
-	((cmd_desc)->num_of_buffers_total_length &= ~cpu_to_le32(0xff), \
-	(cmd_desc)->num_of_buffers_total_length |= cpu_to_le32((val) & 0xff))
+	((cmd_desc)->num_of_buffers_total_length &= ~(0xff), \
+	(cmd_desc)->num_of_buffers_total_length |= (val) & 0xff)
 #define netxen_set_cmd_desc_totallength(cmd_desc, val)	\
-	((cmd_desc)->num_of_buffers_total_length &= ~cpu_to_le32(0xffffff00), \
-	(cmd_desc)->num_of_buffers_total_length |= cpu_to_le32(val << 8))
+	((cmd_desc)->num_of_buffers_total_length &= ~(0xffffff << 8), \
+	(cmd_desc)->num_of_buffers_total_length |= (val) << 8)
 
 #define netxen_get_cmd_desc_opcode(cmd_desc)	\
-	((le16_to_cpu((cmd_desc)->flags_opcode) >> 7) & 0x003F)
+	(((cmd_desc)->flags_opcode >> 7) & 0x003f)
 #define netxen_get_cmd_desc_totallength(cmd_desc)	\
-	(le32_to_cpu((cmd_desc)->num_of_buffers_total_length) >> 8)
+	(((cmd_desc)->num_of_buffers_total_length >> 8) & 0xffffff)
 
 struct cmd_desc_type0 {
 	u8 tcp_hdr_offset;	/* For LSO only */
@@ -412,29 +412,29 @@ struct rcv_desc {
 #define netxen_get_sts_desc_lro_last_frag(status_desc)	\
 	(((status_desc)->lro & 0x80) >> 7)
 
-#define netxen_get_sts_port(status_desc)	\
-	(le64_to_cpu((status_desc)->status_desc_data) & 0x0F)
-#define netxen_get_sts_status(status_desc)	\
-	((le64_to_cpu((status_desc)->status_desc_data) >> 4) & 0x0F)
-#define netxen_get_sts_type(status_desc)	\
-	((le64_to_cpu((status_desc)->status_desc_data) >> 8) & 0x0F)
-#define netxen_get_sts_totallength(status_desc)	\
-	((le64_to_cpu((status_desc)->status_desc_data) >> 12) & 0xFFFF)
-#define netxen_get_sts_refhandle(status_desc)	\
-	((le64_to_cpu((status_desc)->status_desc_data) >> 28) & 0xFFFF)
-#define netxen_get_sts_prot(status_desc)	\
-	((le64_to_cpu((status_desc)->status_desc_data) >> 44) & 0x0F)
-#define netxen_get_sts_owner(status_desc)	\
-	((le64_to_cpu((status_desc)->status_desc_data) >> 56) & 0x03)
-#define netxen_get_sts_opcode(status_desc)	\
-	((le64_to_cpu((status_desc)->status_desc_data) >> 58) & 0x03F)
-
-#define netxen_clear_sts_owner(status_desc)	\
-	((status_desc)->status_desc_data &=	\
-	~cpu_to_le64(((unsigned long long)3) << 56 ))
-#define netxen_set_sts_owner(status_desc, val)	\
-	((status_desc)->status_desc_data |=	\
-	cpu_to_le64(((unsigned long long)((val) & 0x3)) << 56 ))
+#define netxen_get_sts_port(sts_data)	\
+	((sts_data) & 0x0F)
+#define netxen_get_sts_status(sts_data)	\
+	(((sts_data) >> 4) & 0x0F)
+#define netxen_get_sts_type(sts_data)	\
+	(((sts_data) >> 8) & 0x0F)
+#define netxen_get_sts_totallength(sts_data)	\
+	(((sts_data) >> 12) & 0xFFFF)
+#define netxen_get_sts_refhandle(sts_data)	\
+	(((sts_data) >> 28) & 0xFFFF)
+#define netxen_get_sts_prot(sts_data)	\
+	(((sts_data) >> 44) & 0x0F)
+#define netxen_get_sts_owner(sts_data)	\
+	(((sts_data) >> 56) & 0x03)
+#define netxen_get_sts_opcode(sts_data)	\
+	(((sts_data) >> 58) & 0x03F)
+
+#define netxen_set_sts_owner(status_desc, val)	{ \
+	u64 value = le64_to_cpu((status_desc)->status_desc_data); \
+	value &= ~(0x3ULL << 56); \
+	value |= (u64)(((u64)(val) << 56) & (0x3ULL << 56)); \
+	(status_desc)->status_desc_data = cpu_to_le64(value); \
+}
 
 struct status_desc {
 	/* Bit pattern: 0-3 port, 4-7 status, 8-11 type, 12-27 total_length
drivers/net/netxen/netxen_nic_init.c +6 -6
Index: netdev-2.6/drivers/net/netxen/netxen_nic_init.c
===================================================================
@@ -1053,16 +1053,17 @@ static void netxen_process_rcv(struct ne
 {
 	struct pci_dev *pdev = adapter->pdev;
 	struct net_device *netdev = adapter->netdev;
-	int index = netxen_get_sts_refhandle(desc);
+	u64 sts_data = le64_to_cpu(desc->status_desc_data);
+	int index = netxen_get_sts_refhandle(sts_data);
 	struct netxen_recv_context *recv_ctx = &(adapter->recv_ctx[ctxid]);
 	struct netxen_rx_buffer *buffer;
 	struct sk_buff *skb;
-	u32 length = netxen_get_sts_totallength(desc);
+	u32 length = netxen_get_sts_totallength(sts_data);
 	u32 desc_ctx;
 	struct netxen_rcv_desc_ctx *rcv_desc;
 	int ret;
 
-	desc_ctx = netxen_get_sts_type(desc);
+	desc_ctx = netxen_get_sts_type(sts_data);
 	if (unlikely(desc_ctx >= NUM_RCV_DESC_RINGS)) {
 		printk("%s: %s Bad Rcv descriptor ring\n",
 		       netxen_nic_driver_name, netdev->name);
@@ -1102,7 +1103,7 @@ static void netxen_process_rcv(struct ne
 	skb = (struct sk_buff *)buffer->skb;
 
 	if (likely(adapter->rx_csum &&
-				netxen_get_sts_status(desc) == STATUS_CKSUM_OK)) {
+				netxen_get_sts_status(sts_data) == STATUS_CKSUM_OK)) {
 		adapter->stats.csummed++;
 		skb->ip_summed = CHECKSUM_UNNECESSARY;
 	} else
@@ -1185,13 +1186,12 @@ u32 netxen_process_rcv_ring(struct netxe
 	 */
 	while (count < max) {
 		desc = &desc_head[consumer];
-		if (!(netxen_get_sts_owner(desc) & STATUS_OWNER_HOST)) {
+		if (!(netxen_get_sts_owner(le64_to_cpu(desc->status_desc_data)) & STATUS_OWNER_HOST)) {
 			DPRINTK(ERR, "desc %p ownedby %x\n", desc,
 				netxen_get_sts_owner(desc));
 			break;
 		}
 		netxen_process_rcv(adapter, ctxid, desc);
-		netxen_clear_sts_owner(desc);
 		netxen_set_sts_owner(desc, STATUS_OWNER_PHANTOM);
 		consumer = (consumer + 1) & (adapter->max_rx_desc_count - 1);
 		count++;
#14 lore raw

Re: [patch 7/7] netxen: fix byte-swapping in tx and rx

drivers/net/netxen/netxen_nic.h struct netxen_ring_ctx { · line 316

Quoted 13 lines · On Wed, Dec 26, 2007 at 10:23:59AM -0800, [email protected] wrote:

This cleans up some unnecessary byte-swapping while setting up tx and interpreting rx desc. The 64 bit rx status data should be converted to host endian format only once and the macros just need to extract bitfields.

drivers/net/netxen/netxen_nic.h +4 -3
-	(cmd_desc)->flags_opcode |= cpu_to_le16((val) & 0x7f))
+	((cmd_desc)->flags_opcode &= ~(0x7f), \
+	(cmd_desc)->flags_opcode |= (val) & 0x7f)
 #define netxen_set_cmd_desc_opcode(cmd_desc, val)	\
-	((cmd_desc)->flags_opcode &= ~cpu_to_le16(0x3f<<7), \
-	(cmd_desc)->flags_opcode |= cpu_to_le16(((val & 0x3f)<<7)))
+	((cmd_desc)->flags_opcode &= ~(0x3f<<7), \
+	(cmd_desc)->flags_opcode |= ((val) & 0x3f) << 7)

_oh_?

That's actually a pessimisation and I'd rather see that conversion in place gone - the thing will be less brittle that way.

BTW, note that cpu_to_le16() et.al. get evaluated by compiler when the argument is constant.

#15 lore raw

Re: [patch 7/7] netxen: fix byte-swapping in tx and rx

I agree for tx desc, the compiler would optimize.

But for rx (status) desc, I didn't like multiple le64_to_cpu() for extracting various fields out of same status dword.

-Dhananjay

Quoted 23 lines · Al Viro

drivers/net/netxen/netxen_nic.h struct netxen_ring_ctx { · line 316

Quoted 13 lines · On Wed, Dec 26, 2007 at 10:23:59AM -0800, [email protected] wrote:

This cleans up some unnecessary byte-swapping while setting up tx and interpreting rx desc. The 64 bit rx status data should be converted to host endian format only once and the macros just need to extract bitfields.

drivers/net/netxen/netxen_nic.h +4 -3
-	(cmd_desc)->flags_opcode |= cpu_to_le16((val) & 0x7f))
+	((cmd_desc)->flags_opcode &= ~(0x7f), \
+	(cmd_desc)->flags_opcode |= (val) & 0x7f)
 #define netxen_set_cmd_desc_opcode(cmd_desc, val)	\
-	((cmd_desc)->flags_opcode &= ~cpu_to_le16(0x3f<<7), \
-	(cmd_desc)->flags_opcode |= cpu_to_le16(((val & 0x3f)<<7)))
+	((cmd_desc)->flags_opcode &= ~(0x3f<<7), \
+	(cmd_desc)->flags_opcode |= ((val) & 0x3f) << 7)

_oh_?

That's actually a pessimisation and I'd rather see that conversion in place gone - the thing will be less brittle that way.

BTW, note that cpu_to_le16() et.al. get evaluated by compiler when the argument is constant.

#16 lore raw

Re: [patch 7/7] netxen: fix byte-swapping in tx and rx

Quoted 4 lines · Dhananjay Phadke

I agree for tx desc, the compiler would optimize.

But for rx (status) desc, I didn't like multiple le64_to_cpu() for extracting various fields out of same status dword.

Fair enough; AFAICS, on rx side you don't mess with conversion in place and single le64_to_cpu() into a local variable is sane.

BTW,

+       u64 value = le64_to_cpu((status_desc)->status_desc_data); \
+       value &= ~(0x3ULL << 56); \
+       value |= (u64)(((u64)(val) << 56) & (0x3ULL << 56)); \  
+       (status_desc)->status_desc_data = cpu_to_le64(value); \
would be better off as
	status_desc->status_desc_data =
		status_desc->status_desc_data &
		cpu_to_le64(3ULL << 56) |
		cpu_to_le64(((u64)val & 3) << 56);

since the second term will be constant and you get one conversion instead of two (and if val is constant, you'll get no conversions at all, just one and + one or)

#17 lore raw

Re: [patch 7/7] netxen: fix byte-swapping in tx and rx

Here's the reworked patch.

This cleans up some unnecessary byte-swapping while setting up tx and interpreting rx desc. The 64 bit rx status data should be converted to host endian format only once and the macros just need to extract bitfields.

This saves a spate of interrupts on pseries blades caused by buggy (non) processing rx status ring.

drivers/net/netxen/netxen_nic.h +34 -31
Index: netdev-2.6/drivers/net/netxen/netxen_nic.h
===================================================================
@@ -309,23 +309,26 @@ struct netxen_ring_ctx {
 	((cmd_desc)->port_ctxid |= ((var) & 0xF0))
 
 #define netxen_set_cmd_desc_flags(cmd_desc, val)	\
-	((cmd_desc)->flags_opcode &= ~cpu_to_le16(0x7f), \
-	(cmd_desc)->flags_opcode |= cpu_to_le16((val) & 0x7f))
+	(cmd_desc)->flags_opcode = ((cmd_desc)->flags_opcode & \
+		~cpu_to_le16(0x7f)) | cpu_to_le16((val) & 0x7f)
 #define netxen_set_cmd_desc_opcode(cmd_desc, val)	\
-	((cmd_desc)->flags_opcode &= ~cpu_to_le16(0x3f<<7), \
-	(cmd_desc)->flags_opcode |= cpu_to_le16(((val & 0x3f)<<7)))
+	(cmd_desc)->flags_opcode = ((cmd_desc)->flags_opcode & \
+		~cpu_to_le16((u16)0x3f << 7)) | cpu_to_le16(((val) & 0x3f) << 7)
 
 #define netxen_set_cmd_desc_num_of_buff(cmd_desc, val)	\
-	((cmd_desc)->num_of_buffers_total_length &= ~cpu_to_le32(0xff), \
-	(cmd_desc)->num_of_buffers_total_length |= cpu_to_le32((val) & 0xff))
+	(cmd_desc)->num_of_buffers_total_length = \
+		((cmd_desc)->num_of_buffers_total_length & \
+		~cpu_to_le32(0xff)) | cpu_to_le32((val) & 0xff)
 #define netxen_set_cmd_desc_totallength(cmd_desc, val)	\
-	((cmd_desc)->num_of_buffers_total_length &= ~cpu_to_le32(0xffffff00), \
-	(cmd_desc)->num_of_buffers_total_length |= cpu_to_le32(val << 8))
+	(cmd_desc)->num_of_buffers_total_length = \
+		((cmd_desc)->num_of_buffers_total_length & \
+		~cpu_to_le32((u32)0xffffff << 8)) | \
+		cpu_to_le32(((val) & 0xffffff) << 8)
 
 #define netxen_get_cmd_desc_opcode(cmd_desc)	\
-	((le16_to_cpu((cmd_desc)->flags_opcode) >> 7) & 0x003F)
+	((le16_to_cpu((cmd_desc)->flags_opcode) >> 7) & 0x003f)
 #define netxen_get_cmd_desc_totallength(cmd_desc)	\
-	(le32_to_cpu((cmd_desc)->num_of_buffers_total_length) >> 8)
+	((le32_to_cpu((cmd_desc)->num_of_buffers_total_length) >> 8) & 0xffffff)
 
 struct cmd_desc_type0 {
 	u8 tcp_hdr_offset;	/* For LSO only */
@@ -412,29 +415,29 @@ struct rcv_desc {
 #define netxen_get_sts_desc_lro_last_frag(status_desc)	\
 	(((status_desc)->lro & 0x80) >> 7)
 
-#define netxen_get_sts_port(status_desc)	\
-	(le64_to_cpu((status_desc)->status_desc_data) & 0x0F)
-#define netxen_get_sts_status(status_desc)	\
-	((le64_to_cpu((status_desc)->status_desc_data) >> 4) & 0x0F)
-#define netxen_get_sts_type(status_desc)	\
-	((le64_to_cpu((status_desc)->status_desc_data) >> 8) & 0x0F)
-#define netxen_get_sts_totallength(status_desc)	\
-	((le64_to_cpu((status_desc)->status_desc_data) >> 12) & 0xFFFF)
-#define netxen_get_sts_refhandle(status_desc)	\
-	((le64_to_cpu((status_desc)->status_desc_data) >> 28) & 0xFFFF)
-#define netxen_get_sts_prot(status_desc)	\
-	((le64_to_cpu((status_desc)->status_desc_data) >> 44) & 0x0F)
+#define netxen_get_sts_port(sts_data)	\
+	((sts_data) & 0x0F)
+#define netxen_get_sts_status(sts_data)	\
+	(((sts_data) >> 4) & 0x0F)
+#define netxen_get_sts_type(sts_data)	\
+	(((sts_data) >> 8) & 0x0F)
+#define netxen_get_sts_totallength(sts_data)	\
+	(((sts_data) >> 12) & 0xFFFF)
+#define netxen_get_sts_refhandle(sts_data)	\
+	(((sts_data) >> 28) & 0xFFFF)
+#define netxen_get_sts_prot(sts_data)	\
+	(((sts_data) >> 44) & 0x0F)
+#define netxen_get_sts_opcode(sts_data)	\
+	(((sts_data) >> 58) & 0x03F)
+
 #define netxen_get_sts_owner(status_desc)	\
 	((le64_to_cpu((status_desc)->status_desc_data) >> 56) & 0x03)
-#define netxen_get_sts_opcode(status_desc)	\
-	((le64_to_cpu((status_desc)->status_desc_data) >> 58) & 0x03F)
-
-#define netxen_clear_sts_owner(status_desc)	\
-	((status_desc)->status_desc_data &=	\
-	~cpu_to_le64(((unsigned long long)3) << 56 ))
-#define netxen_set_sts_owner(status_desc, val)	\
-	((status_desc)->status_desc_data |=	\
-	cpu_to_le64(((unsigned long long)((val) & 0x3)) << 56 ))
+#define netxen_set_sts_owner(status_desc, val)	{ \
+	(status_desc)->status_desc_data = \
+		((status_desc)->status_desc_data & \
+		~cpu_to_le64(0x3ULL << 56)) | \
+		cpu_to_le64((u64)((val) & 0x3) << 56); \
+}
 
 struct status_desc {
 	/* Bit pattern: 0-3 port, 4-7 status, 8-11 type, 12-27 total_length
drivers/net/netxen/netxen_nic_init.c +5 -5
Index: netdev-2.6/drivers/net/netxen/netxen_nic_init.c
===================================================================
@@ -1053,16 +1053,17 @@ static void netxen_process_rcv(struct ne
 {
 	struct pci_dev *pdev = adapter->pdev;
 	struct net_device *netdev = adapter->netdev;
-	int index = netxen_get_sts_refhandle(desc);
+	u64 sts_data = le64_to_cpu(desc->status_desc_data);
+	int index = netxen_get_sts_refhandle(sts_data);
 	struct netxen_recv_context *recv_ctx = &(adapter->recv_ctx[ctxid]);
 	struct netxen_rx_buffer *buffer;
 	struct sk_buff *skb;
-	u32 length = netxen_get_sts_totallength(desc);
+	u32 length = netxen_get_sts_totallength(sts_data);
 	u32 desc_ctx;
 	struct netxen_rcv_desc_ctx *rcv_desc;
 	int ret;
 
-	desc_ctx = netxen_get_sts_type(desc);
+	desc_ctx = netxen_get_sts_type(sts_data);
 	if (unlikely(desc_ctx >= NUM_RCV_DESC_RINGS)) {
 		printk("%s: %s Bad Rcv descriptor ring\n",
 		       netxen_nic_driver_name, netdev->name);
@@ -1102,7 +1103,7 @@ static void netxen_process_rcv(struct ne
 	skb = (struct sk_buff *)buffer->skb;
 
 	if (likely(adapter->rx_csum &&
-				netxen_get_sts_status(desc) == STATUS_CKSUM_OK)) {
+				netxen_get_sts_status(sts_data) == STATUS_CKSUM_OK)) {
 		adapter->stats.csummed++;
 		skb->ip_summed = CHECKSUM_UNNECESSARY;
 	} else
@@ -1191,7 +1192,6 @@ u32 netxen_process_rcv_ring(struct netxe
 			break;
 		}
 		netxen_process_rcv(adapter, ctxid, desc);
-		netxen_clear_sts_owner(desc);
 		netxen_set_sts_owner(desc, STATUS_OWNER_PHANTOM);
 		consumer = (consumer + 1) & (adapter->max_rx_desc_count - 1);
 		count++;
drivers/net/netxen/netxen_nic_main.c +1 -9
Index: netdev-2.6/drivers/net/netxen/netxen_nic_main.c
===================================================================
@@ -1144,16 +1144,8 @@ static int netxen_nic_xmit_frame(struct 
 		}
 	}
 
-	i = netxen_get_cmd_desc_totallength(&hw->cmd_desc_head[saved_producer]);
-
-	hw->cmd_desc_head[saved_producer].flags_opcode =
-		cpu_to_le16(hw->cmd_desc_head[saved_producer].flags_opcode);
-	hw->cmd_desc_head[saved_producer].num_of_buffers_total_length =
-	  cpu_to_le32(hw->cmd_desc_head[saved_producer].
-			  num_of_buffers_total_length);
-
 	spin_lock_bh(&adapter->tx_lock);
-	adapter->stats.txbytes += i;
+	adapter->stats.txbytes += skb->len;
 
 	/* Code to update the adapter considering how many producer threads
 	   are currently working */
Applied
#18 lore raw

Re: [patch 7/7] netxen: fix byte-swapping in tx and rx

applied