| Message ID | 1436338066-7685-1-git-send-email-mugunthanvnm@ti.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
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.
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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]); } }
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(-)