[net PATCH 1/1] drivers: net: cpsw: fix disabling of tx interrupt in rx isr

[net PATCH 1/1] drivers: net: cpsw: fix disabling of tx interrupt in rx isr

[Mugunthan V N]

With the below commit, common isr is split into tx and rx, but in
rx isr tx interrupt is also disabled. So tx packets are not handled
during rx interrupts and rx napi completion. Fixing by disabling on
rx interrupt in rx isr.

Fixes: c03abd84634d ("net: ethernet: cpsw: don't requests IRQs we don't use")
Cc: Felipe Balbi <balbi@ti.com>
Cc: <stable@vger.kernel.org> # v4.0+
Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
---
 drivers/net/ethernet/ti/cpsw.c | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index e778703..f335bf1 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -138,19 +138,6 @@ do {								\
 #define CPSW_CMINTMAX_INTVL	(1000 / CPSW_CMINTMIN_CNT)
 #define CPSW_CMINTMIN_INTVL	((1000 / CPSW_CMINTMAX_CNT) + 1)
 
-#define cpsw_enable_irq(priv)	\
-	do {			\
-		u32 i;		\
-		for (i = 0; i < priv->num_irqs; i++) \
-			enable_irq(priv->irqs_table[i]); \
-	} while (0)
-#define cpsw_disable_irq(priv)	\
-	do {			\
-		u32 i;		\
-		for (i = 0; i < priv->num_irqs; i++) \
-			disable_irq_nosync(priv->irqs_table[i]); \
-	} while (0)
-
 #define cpsw_slave_index(priv)				\
 		((priv->data.dual_emac) ? priv->emac_port :	\
 		priv->data.active_slave)
@@ -783,7 +770,7 @@ static irqreturn_t cpsw_rx_interrupt(int irq, void *dev_id)
 
 	cpsw_intr_disable(priv);
 	if (priv->irq_enabled == true) {
-		cpsw_disable_irq(priv);
+		disable_irq_nosync(priv->irqs_table[0]);
 		priv->irq_enabled = false;
 	}
 
@@ -819,7 +806,7 @@ static int cpsw_poll(struct napi_struct *napi, int budget)
 		prim_cpsw = cpsw_get_slave_priv(priv, 0);
 		if (prim_cpsw->irq_enabled == false) {
 			prim_cpsw->irq_enabled = true;
-			cpsw_enable_irq(priv);
+			enable_irq(priv->irqs_table[0]);
 		}
 	}
 
@@ -1335,7 +1322,7 @@ static int cpsw_ndo_open(struct net_device *ndev)
 	if (prim_cpsw->irq_enabled == false) {
 		if ((priv == prim_cpsw) || !netif_running(prim_cpsw->ndev)) {
 			prim_cpsw->irq_enabled = true;
-			cpsw_enable_irq(prim_cpsw);
+			enable_irq(prim_cpsw->irqs_table[0]);
 		}
 	}
 
-- 
2.5.0.rc0

Re: [net PATCH 1/1] drivers: net: cpsw: fix disabling of tx interrupt in rx isr

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 782 bytes --]

Hi,

On Wed, Jul 08, 2015 at 12:17:46PM +0530, Mugunthan V N wrote:
> With the below commit, common isr is split into tx and rx, but in
> rx isr tx interrupt is also disabled. So tx packets are not handled
> during rx interrupts and rx napi completion. Fixing by disabling on
> rx interrupt in rx isr.
> 
> Fixes: c03abd84634d ("net: ethernet: cpsw: don't requests IRQs we don't use")

why does this fix that commit ? What was the regression ? So RX IRQ fire
and both RX and TX are disabled, sure that's bad. But it should cause
lower throughput. At a minimum, I think your commit log needs some work.

Also, disable_irq_nosync() shouldn't really be needed, you could just
mask the IRQ in the IRQ Enable register, but I guess that'd be v4.3
material.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [net PATCH 1/1] drivers: net: cpsw: fix disabling of tx interrupt in rx isr

[Mugunthan V N]

On Wednesday 08 July 2015 01:08 PM, Felipe Balbi wrote:
> Hi,
> 
> On Wed, Jul 08, 2015 at 12:17:46PM +0530, Mugunthan V N wrote:
>> With the below commit, common isr is split into tx and rx, but in
>> rx isr tx interrupt is also disabled. So tx packets are not handled
>> during rx interrupts and rx napi completion. Fixing by disabling on
>> rx interrupt in rx isr.
>>
>> Fixes: c03abd84634d ("net: ethernet: cpsw: don't requests IRQs we don't use")
> 
> why does this fix that commit ? What was the regression ? So RX IRQ fire
> and both RX and TX are disabled, sure that's bad. But it should cause
> lower throughput. At a minimum, I think your commit log needs some work.

Agreed, will change the commit and resubmit the patch as it affects
performance only and not a regression.

> 
> Also, disable_irq_nosync() shouldn't really be needed, you could just
> mask the IRQ in the IRQ Enable register, but I guess that'd be v4.3
> material.

This was needed because there is a latch inbetween CPSW interrupt line
and Interrupt Controller (to convert from pulse interrupt to level
interrupt), because of this even though interrupt is disabled from the
IP, CPU is held up in isr as it is not cleared or masked to Interrupt
controller. Because of disable_irq_nosync() was added initially. I will
revisit this but as you said it will be for 4.3

> 

With this patch I am able to see +3 Mbps with omap2plus_defconfig and
+40Mbps with having AM43xx SoC only and removing some kernel debugging
options.

*with omap2plus_defconfig*
[  5] local 192.168.10.116 port 5001 connected with 192.168.10.118 port
46470
[  5]  0.0-60.0 sec  1.03 GBytes   147 Mbits/sec

[  4] local 192.168.10.116 port 5001 connected with 192.168.10.118 port
45968
[ ID] Interval       Transfer     Bandwidth
[  4]  0.0-60.0 sec  1.05 GBytes   150 Mbits/sec


*with omap2plus_defconfig + removing other SoCs and Some debug options*
[  5] local 192.168.10.116 port 5001 connected with 192.168.10.118 port
53272
[  5]  0.0-60.0 sec  2.11 GBytes   302 Mbits/sec


[  4] local 192.168.10.116 port 5001 connected with 192.168.10.118 port
51896
[  4]  0.0-60.0 sec  2.40 GBytes   343 Mbits/sec

Regards
Mugunthan V N

Re: [net PATCH 1/1] drivers: net: cpsw: fix disabling of tx interrupt in rx isr

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1840 bytes --]

Hi,

On Wed, Jul 08, 2015 at 02:21:27PM +0530, Mugunthan V N wrote:
> On Wednesday 08 July 2015 01:08 PM, Felipe Balbi wrote:
> > Hi,
> > 
> > On Wed, Jul 08, 2015 at 12:17:46PM +0530, Mugunthan V N wrote:
> >> With the below commit, common isr is split into tx and rx, but in
> >> rx isr tx interrupt is also disabled. So tx packets are not handled
> >> during rx interrupts and rx napi completion. Fixing by disabling on
> >> rx interrupt in rx isr.
> >>
> >> Fixes: c03abd84634d ("net: ethernet: cpsw: don't requests IRQs we don't use")
> > 
> > why does this fix that commit ? What was the regression ? So RX IRQ fire
> > and both RX and TX are disabled, sure that's bad. But it should cause
> > lower throughput. At a minimum, I think your commit log needs some work.
> 
> Agreed, will change the commit and resubmit the patch as it affects
> performance only and not a regression.

thanks

> > Also, disable_irq_nosync() shouldn't really be needed, you could just
> > mask the IRQ in the IRQ Enable register, but I guess that'd be v4.3
> > material.
> 
> This was needed because there is a latch inbetween CPSW interrupt line
> and Interrupt Controller (to convert from pulse interrupt to level
> interrupt), because of this even though interrupt is disabled from the
> IP, CPU is held up in isr as it is not cleared or masked to Interrupt
> controller. Because of disable_irq_nosync() was added initially. I will
> revisit this but as you said it will be for 4.3

still, that shouldn't be needed. Just mask and clear the IRQ straight
away, then handle it.

> With this patch I am able to see +3 Mbps with omap2plus_defconfig and
> +40Mbps with having AM43xx SoC only and removing some kernel debugging
> options.

looks like this should be in commit log in some shape or form.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]