[net PATCH 1/1] drivers: net: cpsw: dual_emac: fix reducing of rx descriptor during ifdown

[net PATCH 1/1] drivers: net: cpsw: dual_emac: fix reducing of rx descriptor during ifdown

[Mugunthan V N]

In Dual EMAC, when both interface are up and while doing ifdown with heavy
traffic then skbs already processed by DMA from that slave emac has to be
requeued as still the other interface is up and running.

Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
---
 drivers/net/ethernet/ti/cpsw.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 999fb72..04369cf 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -699,8 +699,27 @@ static void cpsw_rx_handler(void *token, int len, int status)
 	cpsw_dual_emac_src_port_detect(status, priv, ndev, skb);
 
 	if (unlikely(status < 0) || unlikely(!netif_running(ndev))) {
-		/* the interface is going down, skbs are purged */
-		dev_kfree_skb_any(skb);
+		bool ndev_status = false;
+		struct cpsw_slave *slave = priv->slaves;
+		int n;
+
+		if (priv->data.dual_emac) {
+			/* In dual emac mode check for all interfaces */
+			for (n = priv->data.slaves; n; n--, slave++)
+				if (netif_running(slave->ndev))
+					ndev_status = true;
+		}
+
+		if (ndev_status) {
+			/* Though this interface is down, other interface is up
+			 * and running so requeue skb back to cpdma.
+			 */
+			new_skb = skb;
+			goto requeue;
+		} else {
+			/* the interface is going down, skbs are purged */
+			dev_kfree_skb_any(skb);
+		}
 		return;
 	}
 
@@ -717,6 +736,7 @@ static void cpsw_rx_handler(void *token, int len, int status)
 		new_skb = skb;
 	}
 
+requeue:
 	ret = cpdma_chan_submit(priv->rxch, new_skb, new_skb->data,
 			skb_tailroom(new_skb), 0);
 	if (WARN_ON(ret < 0))
-- 
2.1.0

Re: [net PATCH 1/1] drivers: net: cpsw: dual_emac: fix reducing of rx descriptor during ifdown

[David Miller]

From: Mugunthan V N <mugunthanvnm@ti.com>
Date: Fri, 29 Aug 2014 14:52:25 +0530

> In Dual EMAC, when both interface are up and while doing ifdown with heavy
> traffic then skbs already processed by DMA from that slave emac has to be
> requeued as still the other interface is up and running.
> 
> Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>

I don't see why this is important.

If the packet arrived via the down interface, let it be dropped and
the sender will simply resend if necessary.

Also you are putting this new multi-slave logic under the "status < 0"
condition as well as the interface being down, that's not right either.

Re: [net PATCH 1/1] drivers: net: cpsw: dual_emac: fix reducing of rx descriptor during ifdown

[Mugunthan V N]

David

On Tuesday 02 September 2014 07:01 AM, David Miller wrote:
> From: Mugunthan V N <mugunthanvnm@ti.com>
> Date: Fri, 29 Aug 2014 14:52:25 +0530
> 
>> In Dual EMAC, when both interface are up and while doing ifdown with heavy
>> traffic then skbs already processed by DMA from that slave emac has to be
>> requeued as still the other interface is up and running.
>>
>> Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
> 
> I don't see why this is important.
> 
> If the packet arrived via the down interface, let it be dropped and
> the sender will simply resend if necessary.
> 
> Also you are putting this new multi-slave logic under the "status < 0"
> condition as well as the interface being down, that's not right either.
> 

Multi slave logic is already there though out the driver, this is one
corner case where it fails. Let me explain more

DMA is common for both interfaces and serves both the slave ports. In
heavy traffic when one interface is put down, there are chances that
some packets are already processed in are in completed state.

Since DMA is a common entity for both interface, DMA is disabled only
when there all the slave interfaces are down. so when both interface is
up and putting down one interface, DMA de-init doesn't  happen as the
other interface is up.

In this scenario, when cpsw_rx_handler is called for processing the
packet which might have packets for interfaces which is down already.
Previously the driver simply drops the skb and doesn't do re-queue of
the descriptor which results in one descriptor less for rx DMA.

When ifup and ifdown is run continuously, for each spilled packet (for
interface which is down) from DMA, the total number of rx descriptor
goes down and at one instance all the descriptor is lost and both the
interface stops working.

To recover from this we need to put down both the interface and open the
interface which will re-init the DMA which intern queues fresh set of
skbs for rx.

So to overcome this issue, I did this fix by re-queuing the rx
descriptor back to DMA when any one interface is so that the no of rx
descriptor is kept constant always.

Regards
Mugunthan V N

Re: [net PATCH 1/1] drivers: net: cpsw: dual_emac: fix reducing of rx descriptor during ifdown

[David Miller]

From: Mugunthan V N <mugunthanvnm@ti.com>
Date: Tue, 2 Sep 2014 14:51:05 +0530

> When ifup and ifdown is run continuously, for each spilled packet (for
> interface which is down) from DMA, the total number of rx descriptor
> goes down and at one instance all the descriptor is lost and both the
> interface stops working.
> 
> To recover from this we need to put down both the interface and open the
> interface which will re-init the DMA which intern queues fresh set of
> skbs for rx.

But you still should not receive packets for a netdev which is down.
As far as I can tell, you're feeding it into the stack still.

Also this doesn't explain why the "status < 0" case applies to this
new logic, you have not explained that at all.

Re: [net PATCH 1/1] drivers: net: cpsw: dual_emac: fix reducing of rx descriptor during ifdown

[Mugunthan V N]

David

On Wednesday 03 September 2014 12:24 AM, David Miller wrote:
> From: Mugunthan V N <mugunthanvnm@ti.com>
> Date: Tue, 2 Sep 2014 14:51:05 +0530
> 
>> When ifup and ifdown is run continuously, for each spilled packet (for
>> interface which is down) from DMA, the total number of rx descriptor
>> goes down and at one instance all the descriptor is lost and both the
>> interface stops working.
>>
>> To recover from this we need to put down both the interface and open the
>> interface which will re-init the DMA which intern queues fresh set of
>> skbs for rx.
> 
> But you still should not receive packets for a netdev which is down.
> As far as I can tell, you're feeding it into the stack still.

If there is separate DMA for each netdev, then it is true that there
won't be a packet for a closed netdev. But in CPSW dual EMAC case, one
DMA engine is shared between two slave ports, so when one slave netdev
is put down, the DMA is not teared down as the other slave netdev is
still active. In heavy traffic network when putting down netdev, there
are chances that packets are already processed by DMA, and waiting for
NAPI to be submitted to network stack might belong to the netdev which
is already down.

So instead of just freeing the skb received on netdev which is already
down (which will reduce the total rx descriptors in rx dma channel),
requeue it back to DMA and make sure that rx dma descriptor count is
never reduced.

> 
> Also this doesn't explain why the "status < 0" case applies to this
> new logic, you have not explained that at all.
> 

This scenario is not generated when "status < 0", "status < 0" happens
when DMA is in tear-down mode, the above scenario happens when
netif_running(ndev) is false which denotes that netdev is down already.

Regards
Mugunthan V N

Re: [net PATCH 1/1] drivers: net: cpsw: dual_emac: fix reducing of rx descriptor during ifdown

[Mugunthan V N]

On Wednesday 03 September 2014 12:46 PM, Mugunthan V N wrote:
> David
> 
> On Wednesday 03 September 2014 12:24 AM, David Miller wrote:
>> From: Mugunthan V N <mugunthanvnm@ti.com>
>> Date: Tue, 2 Sep 2014 14:51:05 +0530
>>
>>> When ifup and ifdown is run continuously, for each spilled packet (for
>>> interface which is down) from DMA, the total number of rx descriptor
>>> goes down and at one instance all the descriptor is lost and both the
>>> interface stops working.
>>>
>>> To recover from this we need to put down both the interface and open the
>>> interface which will re-init the DMA which intern queues fresh set of
>>> skbs for rx.
>>
>> But you still should not receive packets for a netdev which is down.
>> As far as I can tell, you're feeding it into the stack still.
> 
> If there is separate DMA for each netdev, then it is true that there
> won't be a packet for a closed netdev. But in CPSW dual EMAC case, one
> DMA engine is shared between two slave ports, so when one slave netdev
> is put down, the DMA is not teared down as the other slave netdev is
> still active. In heavy traffic network when putting down netdev, there
> are chances that packets are already processed by DMA, and waiting for
> NAPI to be submitted to network stack might belong to the netdev which
> is already down.
> 
> So instead of just freeing the skb received on netdev which is already
> down (which will reduce the total rx descriptors in rx dma channel),
> requeue it back to DMA and make sure that rx dma descriptor count is
> never reduced.
> 
>>
>> Also this doesn't explain why the "status < 0" case applies to this
>> new logic, you have not explained that at all.
>>
> 
> This scenario is not generated when "status < 0", "status < 0" happens
> when DMA is in tear-down mode, the above scenario happens when
> netif_running(ndev) is false which denotes that netdev is down already.
> 

I found an issue with the patch while testing suspend/resume, will fix
that and submit v2 shortly.

Regards
Mugunthan V N

Re: [net PATCH 1/1] drivers: net: cpsw: dual_emac: fix reducing of rx descriptor during ifdown

[David Miller]

From: Mugunthan V N <mugunthanvnm@ti.com>
Date: Wed, 3 Sep 2014 12:46:55 +0530

> On Wednesday 03 September 2014 12:24 AM, David Miller wrote:
>> Also this doesn't explain why the "status < 0" case applies to this
>> new logic, you have not explained that at all.
>> 
> 
> This scenario is not generated when "status < 0", "status < 0" happens
> when DMA is in tear-down mode, the above scenario happens when
> netif_running(ndev) is false which denotes that netdev is down already.

Your condition was:

	if (status < 0 || !netif_running(ndev))

so this "scenerio" is being handled when status < 0, which is what
I am objecting to.