Skip to contentSkip to conversation

forcedeth: seperate handler for msix and normal int.

PATCH In review Yinghai Lu 3 participants 4 messages Dec 29, 2007 – Feb 12, 2008 Network drivers

lore
Share & tools
lore

Yinghai Lu

author
#0 lore raw

PATCH 1/1 forcedeth: seperate handler for msix and normal int.

[PATCH] forcedeth: seperate handler for msix and normal int.

so we don't need to keep checking np->msi_flags to see if NV_MSI_X_ENABLED is set in handler

Signed-off-by Yinghai Lu <[email protected]>
drivers/net/forcedeth.c +51 -16
Index: linux-2.6/drivers/net/forcedeth.c
===================================================================
@@ -2979,7 +2979,7 @@ static void nv_link_irq(struct net_devic
 	dprintk(KERN_DEBUG "%s: link change notification done.\n", dev->name);
 }
 
-static irqreturn_t nv_nic_irq(int foo, void *data)
+static inline irqreturn_t nv_nic_irq_x(int foo, void *data, int msix)
 {
 	struct net_device *dev = (struct net_device *) data;
 	struct fe_priv *np = netdev_priv(dev);

@@ -2990,7 +2990,7 @@ static irqreturn_t nv_nic_irq(int foo, v
 	dprintk(KERN_DEBUG "%s: nv_nic_irq\n", dev->name);
 
 	for (i=0; ; i++) {
-		if (!(np->msi_flags & NV_MSI_X_ENABLED)) {
+		if (!msix) {
 			events = readl(base + NvRegIrqStatus) & NVREG_IRQSTAT_MASK;
 			writel(NVREG_IRQSTAT_MASK, base + NvRegIrqStatus);
 		} else {

@@ -3013,7 +3013,7 @@ static irqreturn_t nv_nic_irq(int foo, v
 			spin_lock(&np->lock);
 			np->irqmask &= ~NVREG_IRQ_RX_ALL;
 
-			if (np->msi_flags & NV_MSI_X_ENABLED)
+			if (msix)
 				writel(NVREG_IRQ_RX_ALL, base + NvRegIrqMask);
 			else
 				writel(np->irqmask, base + NvRegIrqMask);

@@ -3051,7 +3051,7 @@ static irqreturn_t nv_nic_irq(int foo, v
 		if (unlikely(events & NVREG_IRQ_RECOVER_ERROR)) {
 			spin_lock(&np->lock);
 			/* disable interrupts on the nic */
-			if (!(np->msi_flags & NV_MSI_X_ENABLED))
+			if (!msix)
 				writel(0, base + NvRegIrqMask);
 			else
 				writel(np->irqmask, base + NvRegIrqMask);

@@ -3068,7 +3068,7 @@ static irqreturn_t nv_nic_irq(int foo, v
 		if (unlikely(i > max_interrupt_work)) {
 			spin_lock(&np->lock);
 			/* disable interrupts on the nic */
-			if (!(np->msi_flags & NV_MSI_X_ENABLED))
+			if (!msix)
 				writel(0, base + NvRegIrqMask);
 			else
 				writel(np->irqmask, base + NvRegIrqMask);

@@ -3089,12 +3089,23 @@ static irqreturn_t nv_nic_irq(int foo, v
 	return IRQ_RETVAL(i);
 }
 
+static irqreturn_t nv_nic_irq_msix(int foo, void *data)
+{
+	return nv_nic_irq_x(foo, data, 1);
+}
+
+static irqreturn_t nv_nic_irq_normal(int foo, void *data)
+{
+	return nv_nic_irq_x(foo, data, 0);
+}
+
+
 /**
  * All _optimized functions are used to help increase performance
  * (reduce CPU and increase throughput). They use descripter version 3,
  * compiler directives, and reduce memory accesses.
  */
-static irqreturn_t nv_nic_irq_optimized(int foo, void *data)
+static inline irqreturn_t nv_nic_irq_optimized_x(int foo, void *data, int msix)
 {
 	struct net_device *dev = (struct net_device *) data;
 	struct fe_priv *np = netdev_priv(dev);

@@ -3105,7 +3116,7 @@ static irqreturn_t nv_nic_irq_optimized(
 	dprintk(KERN_DEBUG "%s: nv_nic_irq_optimized\n", dev->name);
 
 	for (i=0; ; i++) {
-		if (!(np->msi_flags & NV_MSI_X_ENABLED)) {
+		if (!msix) {
 			events = readl(base + NvRegIrqStatus) & NVREG_IRQSTAT_MASK;
 			writel(NVREG_IRQSTAT_MASK, base + NvRegIrqStatus);
 		} else {

@@ -3128,7 +3139,7 @@ static irqreturn_t nv_nic_irq_optimized(
 			spin_lock(&np->lock);
 			np->irqmask &= ~NVREG_IRQ_RX_ALL;
 
-			if (np->msi_flags & NV_MSI_X_ENABLED)
+			if (msix)
 				writel(NVREG_IRQ_RX_ALL, base + NvRegIrqMask);
 			else
 				writel(np->irqmask, base + NvRegIrqMask);

@@ -3166,7 +3177,7 @@ static irqreturn_t nv_nic_irq_optimized(
 		if (unlikely(events & NVREG_IRQ_RECOVER_ERROR)) {
 			spin_lock(&np->lock);
 			/* disable interrupts on the nic */
-			if (!(np->msi_flags & NV_MSI_X_ENABLED))
+			if (!msix)
 				writel(0, base + NvRegIrqMask);
 			else
 				writel(np->irqmask, base + NvRegIrqMask);

@@ -3184,7 +3195,7 @@ static irqreturn_t nv_nic_irq_optimized(
 		if (unlikely(i > max_interrupt_work)) {
 			spin_lock(&np->lock);
 			/* disable interrupts on the nic */
-			if (!(np->msi_flags & NV_MSI_X_ENABLED))
+			if (!msix)
 				writel(0, base + NvRegIrqMask);
 			else
 				writel(np->irqmask, base + NvRegIrqMask);

@@ -3205,6 +3216,16 @@ static irqreturn_t nv_nic_irq_optimized(
 	return IRQ_RETVAL(i);
 }
 
+static irqreturn_t nv_nic_irq_optimized_msix(int foo, void *data)
+{
+	return nv_nic_irq_optimized_x(foo, data, 1);
+}
+
+static irqreturn_t nv_nic_irq_optimized_normal(int foo, void *data)
+{
+	return nv_nic_irq_optimized_x(foo, data, 0);
+}
+
 static irqreturn_t nv_nic_irq_tx(int foo, void *data)
 {
 	struct net_device *dev = (struct net_device *) data;

@@ -3503,9 +3524,9 @@ static int nv_request_irq(struct net_dev
 		handler = nv_nic_irq_test;
 	} else {
 		if (np->desc_ver == DESC_VER_3)
-			handler = nv_nic_irq_optimized;
+			handler = nv_nic_irq_optimized_normal;
 		else
-			handler = nv_nic_irq;
+			handler = nv_nic_irq_normal;
 	}
 
 	if (np->msi_flags & NV_MSI_X_CAPABLE) {

@@ -3543,6 +3564,13 @@ static int nv_request_irq(struct net_dev
 				set_msix_vector_map(dev, NV_MSI_X_VECTOR_TX, NVREG_IRQ_TX_ALL);
 				set_msix_vector_map(dev, NV_MSI_X_VECTOR_OTHER, NVREG_IRQ_OTHER);
 			} else {
+				if (!intr_test) {
+					if (np->desc_ver == DESC_VER_3)
+						handler = nv_nic_irq_optimized_msix;
+					else
+						handler = nv_nic_irq_msix;
+				}
+
 				/* Request irq for all interrupts */
 				if (request_irq(np->msi_x_entry[NV_MSI_X_VECTOR_ALL].vector, handler, IRQF_SHARED, dev->name, dev) != 0) {
 					printk(KERN_INFO "forcedeth: request_irq failed %d\n", ret);

@@ -3689,10 +3717,17 @@ static void nv_do_nic_poll(unsigned long
 	pci_push(base);
 
 	if (!using_multi_irqs(dev)) {
-		if (np->desc_ver == DESC_VER_3)
-			nv_nic_irq_optimized(0, dev);
-		else
-			nv_nic_irq(0, dev);
+		if (np->msi_flags & NV_MSI_X_ENABLED) {
+			if (np->desc_ver == DESC_VER_3)
+				nv_nic_irq_optimized_msix(0, dev);
+			else
+				nv_nic_irq_msix(0, dev);
+		} else {
+			if (np->desc_ver == DESC_VER_3)
+				nv_nic_irq_optimized_normal(0, dev);
+			else
+				nv_nic_irq_normal(0, dev);
+		}
 		if (np->msi_flags & NV_MSI_X_ENABLED)
 			enable_irq_lockdep(np->msi_x_entry[NV_MSI_X_VECTOR_ALL].vector);
 		else
#1 lore raw
Quoted 4 lines · Yinghai Lu

[PATCH] forcedeth: seperate handler for msix and normal int.

so we don't need to keep checking np->msi_flags to see if NV_MSI_X_ENABLED is set in handler

hm, why is this patch needed and what's the effect of this patch?

i'm also wondering, what happened to Jeff's very nice flow cleanups and simplifications to forcedeth.c. They are not in 2.6.24 - are they queued up for 2.6.25 perhaps?

Ingo

#2 lore raw
In reply to Yinghai Lu
Quoted 6 lines · Yinghai Lu

[PATCH] forcedeth: seperate handler for msix and normal int.

so we don't need to keep checking np->msi_flags to see if NV_MSI_X_ENABLED is set in handler

Signed-off-by Yinghai Lu <[email protected]>

Two comments:

* can you regenerate this on top of 2.6.25-rc1?

* take a look at the 'fe' branch of git://git.kernel.org/pub/scm/linux/kernel/git/jgarzik/netdev-2.6.git

and see what you think about the work in there. We need to get cleanups like that (and more) upstream. It also intersects with Tom @ Google's "Reduce locking in TX path" patch.

Jeff

author
#3 lore raw

PATCH 1/1 forcedeth: seperate handler for msix and normal int v2

so we don't need to keep check np->msi_flags to see if NV_MSI_X_ENABLED is set in handler

Signed-off-by Yinghai Lu <[email protected]>
drivers/net/forcedeth.c +33 -14
Index: linux-2.6/drivers/net/forcedeth.c
===================================================================
@@ -2901,12 +2901,12 @@ static void nv_link_irq(struct net_device *dev)
 	dprintk(KERN_DEBUG "%s: link change notification done.\n", dev->name);
 }
 
-static irqreturn_t __nv_nic_irq(struct net_device *dev, bool optimized)
+static inline irqreturn_t __nv_nic_irq(struct net_device *dev, bool optimized,
+				bool msix)
 {
 	struct fe_priv *np = netdev_priv(dev);
 	u8 __iomem *base = np->base;
 	u32 events, updmask = 0;
-	bool msix = np->msi_flags & NV_MSI_X_ENABLED;
 
 	dprintk(KERN_DEBUG "%s: __nv_nic_irq\n", dev->name);
 

@@ -2984,16 +2984,26 @@ out:
 	return IRQ_HANDLED;
 }
 
-static irqreturn_t nv_nic_irq(int foo, void *data)
+static irqreturn_t nv_nic_irq_msix(int foo, void *data)
 {
 	struct net_device *dev = data;
-	return __nv_nic_irq(dev, false);
+	return __nv_nic_irq(dev, false, true);
+}
+static irqreturn_t nv_nic_irq_normal(int foo, void *data)
+{
+	struct net_device *dev = data;
+	return __nv_nic_irq(dev, false, false);
 }
 
-static irqreturn_t nv_nic_irq_optimized(int foo, void *data)
+static irqreturn_t nv_nic_irq_optimized_msix(int foo, void *data)
 {
 	struct net_device *dev = data;
-	return __nv_nic_irq(dev, true);
+	return __nv_nic_irq(dev, true, true);
+}
+static irqreturn_t nv_nic_irq_optimized_normal(int foo, void *data)
+{
+	struct net_device *dev = data;
+	return __nv_nic_irq(dev, true, false);
 }
 
 static int nv_napi_poll(struct napi_struct *napi, int budget)

@@ -3222,9 +3232,9 @@ static int nv_request_irq(struct net_device *dev, int intr_test)
 		handler = nv_nic_irq_test;
 	} else {
 		if (nv_optimized(np))
-			handler = nv_nic_irq_optimized;
+			handler = nv_nic_irq_optimized_normal;
 		else
-			handler = nv_nic_irq;
+			handler = nv_nic_irq_normal;
 	}
 
 	if (np->msi_flags & NV_MSI_X_CAPABLE) {

@@ -3262,6 +3272,10 @@ static int nv_request_irq(struct net_device *dev, int intr_test)
 				set_msix_vector_map(dev, NV_MSI_X_VECTOR_TX, NVREG_IRQ_TX_ALL);
 				set_msix_vector_map(dev, NV_MSI_X_VECTOR_OTHER, NVREG_IRQ_OTHER);
 			} else {
+				if (nv_optimized(np))
+					handler = nv_nic_irq_optimized_msix;
+				else
+					handler = nv_nic_irq_msix;
 				/* Request irq for all interrupts */
 				if (request_irq(np->msi_x_entry[NV_MSI_X_VECTOR_ALL].vector, handler, IRQF_SHARED, dev->name, dev) != 0) {
 					printk(KERN_INFO "forcedeth: request_irq failed %d\n", ret);

@@ -3403,14 +3417,19 @@ static void nv_reset_task(struct work_struct *work)
 	pci_push(base);
 
 	if (!using_multi_irqs(dev)) {
-		if (nv_optimized(np))
-			nv_nic_irq_optimized(0, dev);
-		else
-			nv_nic_irq(0, dev);
-		if (np->msi_flags & NV_MSI_X_ENABLED)
+		if (np->msi_flags & NV_MSI_X_ENABLED) {
+			if (nv_optimized(np))
+				nv_nic_irq_optimized_msix(0, dev);
+			else
+				nv_nic_irq_msix(0, dev);
 			enable_irq_lockdep(np->msi_x_entry[NV_MSI_X_VECTOR_ALL].vector);
-		else
+		} else {
+			if (nv_optimized(np))
+				nv_nic_irq_optimized_normal(0, dev);
+			else
+				nv_nic_irq_normal(0, dev);
 			enable_irq_lockdep(np->pci_dev->irq);
+		}
 	} else {
 		if (np->nic_poll_irq & NVREG_IRQ_RX_ALL) {
 			nv_nic_irq_rx(0, dev);