[PATCH 2/5] net/cpsw: don't continue if we miss to allocate rx skbs

[PATCH 2/5] net/cpsw: don't continue if we miss to allocate rx skbs

[Sebastian Andrzej Siewior]

if during "ifconfig up" we run out of mem we continue regardless how
many skbs we got. In worst case we have zero RX skbs and can't ever
receive further packets since the RX skbs are never reallocated. If
cpdma_chan_submit() fails we even leak the skb.
This patch changes the behavior here:
If we fail to allocate an skb during bring up we don't continue and
report that error. Same goes for errors from cpdma_chan_submit().
While here I changed to __netdev_alloc_skb_ip_align() so GFP_KERNEL can
be used.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/net/ethernet/ti/cpsw.c |   16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index e2ba702..3b22a36 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -912,14 +912,16 @@ static int cpsw_ndo_open(struct net_device *ndev)
 			struct sk_buff *skb;
 
 			ret = -ENOMEM;
-			skb = netdev_alloc_skb_ip_align(priv->ndev,
-							priv->rx_packet_max);
+			skb = __netdev_alloc_skb_ip_align(priv->ndev,
+					priv->rx_packet_max, GFP_KERNEL);
 			if (!skb)
-				break;
+				goto err_cleanup;
 			ret = cpdma_chan_submit(priv->rxch, skb, skb->data,
 					skb_tailroom(skb), 0, GFP_KERNEL);
-			if (WARN_ON(ret < 0))
-				break;
+			if (ret < 0) {
+				kfree_skb(skb);
+				goto err_cleanup;
+			}
 		}
 		/* continue even if we didn't manage to submit all
 		 * receive descs
@@ -944,6 +946,10 @@ static int cpsw_ndo_open(struct net_device *ndev)
 	if (priv->data.dual_emac)
 		priv->slaves[priv->emac_port].open_stat = true;
 	return 0;
+
+err_cleanup:
+	cpdma_ctlr_stop(priv->dma);
+	return ret;
 }
 
 static void cpsw_slave_stop(struct cpsw_slave *slave, struct cpsw_priv *priv)
-- 
1.7.10.4

Re: [PATCH 2/5] net/cpsw: don't continue if we miss to allocate rx skbs

[Mugunthan V N]

On 4/18/2013 3:22 AM, Sebastian Andrzej Siewior wrote:
> if during "ifconfig up" we run out of mem we continue regardless how
> many skbs we got. In worst case we have zero RX skbs and can't ever
> receive further packets since the RX skbs are never reallocated. If
> cpdma_chan_submit() fails we even leak the skb.
> This patch changes the behavior here:
> If we fail to allocate an skb during bring up we don't continue and
> report that error. Same goes for errors from cpdma_chan_submit().
> While here I changed to __netdev_alloc_skb_ip_align() so GFP_KERNEL can
> be used.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>   drivers/net/ethernet/ti/cpsw.c |   16 +++++++++++-----
>   1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index e2ba702..3b22a36 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -912,14 +912,16 @@ static int cpsw_ndo_open(struct net_device *ndev)
>   			struct sk_buff *skb;
>   
>   			ret = -ENOMEM;
> -			skb = netdev_alloc_skb_ip_align(priv->ndev,
> -							priv->rx_packet_max);
> +			skb = __netdev_alloc_skb_ip_align(priv->ndev,
> +					priv->rx_packet_max, GFP_KERNEL);
>   			if (!skb)
> -				break;
> +				goto err_cleanup;
>   			ret = cpdma_chan_submit(priv->rxch, skb, skb->data,
>   					skb_tailroom(skb), 0, GFP_KERNEL);
> -			if (WARN_ON(ret < 0))
> -				break;
> +			if (ret < 0) {
> +				kfree_skb(skb);
> +				goto err_cleanup;
Why you need to close the device even you have some skb allocated and
submitted successfully. Can allow the device to continue with lower
performance
> +			}
>   		}
>   		/* continue even if we didn't manage to submit all
>   		 * receive descs
> @@ -944,6 +946,10 @@ static int cpsw_ndo_open(struct net_device *ndev)
>   	if (priv->data.dual_emac)
>   		priv->slaves[priv->emac_port].open_stat = true;
>   	return 0;
> +
> +err_cleanup:
> +	cpdma_ctlr_stop(priv->dma);
> +	return ret;
>   }
only cpdma is halted and allocated skb are released, need to have other
calls like pm_runtime_put_sync, close host and slave ports

Regards
Mugunthan V N

Re: [PATCH 2/5] net/cpsw: don't continue if we miss to allocate rx skbs

[Sebastian Andrzej Siewior]

On 04/18/2013 01:50 PM, Mugunthan V N wrote:

>> diff --git a/drivers/net/ethernet/ti/cpsw.c
>> b/drivers/net/ethernet/ti/cpsw.c
>> index e2ba702..3b22a36 100644
>> --- a/drivers/net/ethernet/ti/cpsw.c
>> +++ b/drivers/net/ethernet/ti/cpsw.c
>> @@ -912,14 +912,16 @@ static int cpsw_ndo_open(struct net_device *ndev)
>>               struct sk_buff *skb;
>>                 ret = -ENOMEM;
>> -            skb = netdev_alloc_skb_ip_align(priv->ndev,
>> -                            priv->rx_packet_max);
>> +            skb = __netdev_alloc_skb_ip_align(priv->ndev,
>> +                    priv->rx_packet_max, GFP_KERNEL);
>>               if (!skb)
>> -                break;
>> +                goto err_cleanup;
>>               ret = cpdma_chan_submit(priv->rxch, skb, skb->data,
>>                       skb_tailroom(skb), 0, GFP_KERNEL);
>> -            if (WARN_ON(ret < 0))
>> -                break;
>> +            if (ret < 0) {
>> +                kfree_skb(skb);
>> +                goto err_cleanup;
> Why you need to close the device even you have some skb allocated and
> submitted successfully. Can allow the device to continue with lower
> performance

Because this should not happen. If you run out-of-memory because an
application is going crazy than you won't have much anyway. If you
configured too much skbs then this should be fixed as well.

>> +            }
>>           }
>>           /* continue even if we didn't manage to submit all
>>            * receive descs
>> @@ -944,6 +946,10 @@ static int cpsw_ndo_open(struct net_device *ndev)
>>       if (priv->data.dual_emac)
>>           priv->slaves[priv->emac_port].open_stat = true;
>>       return 0;
>> +
>> +err_cleanup:
>> +    cpdma_ctlr_stop(priv->dma);
>> +    return ret;
>>   }
> only cpdma is halted and allocated skb are released, need to have other
> calls like pm_runtime_put_sync, close host and slave ports

Okay, will fix.

> 
> Regards
> Mugunthan V N


Sebastian

Re: [PATCH 2/5] net/cpsw: don't continue if we miss to allocate rx skbs

[Mugunthan V N]

On 4/18/2013 5:39 PM, Sebastian Andrzej Siewior wrote:
> On 04/18/2013 01:50 PM, Mugunthan V N wrote:
>
>>> diff --git a/drivers/net/ethernet/ti/cpsw.c
>>> b/drivers/net/ethernet/ti/cpsw.c
>>> index e2ba702..3b22a36 100644
>>> --- a/drivers/net/ethernet/ti/cpsw.c
>>> +++ b/drivers/net/ethernet/ti/cpsw.c
>>> @@ -912,14 +912,16 @@ static int cpsw_ndo_open(struct net_device *ndev)
>>>                struct sk_buff *skb;
>>>                  ret = -ENOMEM;
>>> -            skb = netdev_alloc_skb_ip_align(priv->ndev,
>>> -                            priv->rx_packet_max);
>>> +            skb = __netdev_alloc_skb_ip_align(priv->ndev,
>>> +                    priv->rx_packet_max, GFP_KERNEL);
>>>                if (!skb)
>>> -                break;
>>> +                goto err_cleanup;
>>>                ret = cpdma_chan_submit(priv->rxch, skb, skb->data,
>>>                        skb_tailroom(skb), 0, GFP_KERNEL);
>>> -            if (WARN_ON(ret < 0))
>>> -                break;
>>> +            if (ret < 0) {
>>> +                kfree_skb(skb);
>>> +                goto err_cleanup;
>> Why you need to close the device even you have some skb allocated and
>> submitted successfully. Can allow the device to continue with lower
>> performance
> Because this should not happen. If you run out-of-memory because an
> application is going crazy than you won't have much anyway. If you
> configured too much skbs then this should be fixed as well.
>
But i am seeing most of the drivers allowing to open the device with lesser
rx skb count? But i don't know where this has been changed recently, may
be some other network experts can comment on this.

Regards
Mugunthan V N

Re: [PATCH 2/5] net/cpsw: don't continue if we miss to allocate rx skbs

[Sebastian Andrzej Siewior]

On 04/19/2013 12:40 PM, Mugunthan V N wrote:
> But i am seeing most of the drivers allowing to open the device with lesser
> rx skb count? But i don't know where this has been changed recently, may
> be some other network experts can comment on this.

If you configure a special value in your device tree you should obey
it or else you won't the same result.

> 
> Regards
> Mugunthan V N

Sebastian

cpsw queue, v2

[Sebastian Andrzej Siewior]

Hi,

patches one, four and five gained acked-by tags nothing else changed.
Patch two gained pm_runtime_put_sync() and clean up for the slave.
Patch three is a complete redo of the initial patch.

Sebastian

[PATCH 1/5] net/davinci_cpdma: don't check for jiffies with interrupts

[Sebastian Andrzej Siewior]

__cpdma_chan_process() holds the lock with interrupts off (and its
caller as well), same goes for cpdma_ctlr_start(). With interrupts off,
jiffies will not make any progress and if the wait condition never gets
true we wait for ever.
Tgis patch adds a a simple udelay and counting down attempt.

Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/net/ethernet/ti/davinci_cpdma.c |   20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
index ee13dc7..3e34187 100644
--- a/drivers/net/ethernet/ti/davinci_cpdma.c
+++ b/drivers/net/ethernet/ti/davinci_cpdma.c
@@ -20,6 +20,7 @@
 #include <linux/err.h>
 #include <linux/dma-mapping.h>
 #include <linux/io.h>
+#include <linux/delay.h>
 
 #include "davinci_cpdma.h"
 
@@ -312,14 +313,16 @@ int cpdma_ctlr_start(struct cpdma_ctlr *ctlr)
 	}
 
 	if (ctlr->params.has_soft_reset) {
-		unsigned long timeout = jiffies + HZ/10;
+		unsigned timeout = 10 * 100;
 
 		dma_reg_write(ctlr, CPDMA_SOFTRESET, 1);
-		while (time_before(jiffies, timeout)) {
+		while (timeout) {
 			if (dma_reg_read(ctlr, CPDMA_SOFTRESET) == 0)
 				break;
+			udelay(10);
+			timeout--;
 		}
-		WARN_ON(!time_before(jiffies, timeout));
+		WARN_ON(!timeout);
 	}
 
 	for (i = 0; i < ctlr->num_chan; i++) {
@@ -868,7 +871,7 @@ int cpdma_chan_stop(struct cpdma_chan *chan)
 	struct cpdma_desc_pool	*pool = ctlr->pool;
 	unsigned long		flags;
 	int			ret;
-	unsigned long		timeout;
+	unsigned		timeout;
 
 	spin_lock_irqsave(&chan->lock, flags);
 	if (chan->state != CPDMA_STATE_ACTIVE) {
@@ -883,14 +886,15 @@ int cpdma_chan_stop(struct cpdma_chan *chan)
 	dma_reg_write(ctlr, chan->td, chan_linear(chan));
 
 	/* wait for teardown complete */
-	timeout = jiffies + HZ/10;	/* 100 msec */
-	while (time_before(jiffies, timeout)) {
+	timeout = 100 * 100; /* 100 ms */
+	while (timeout) {
 		u32 cp = chan_read(chan, cp);
 		if ((cp & CPDMA_TEARDOWN_VALUE) == CPDMA_TEARDOWN_VALUE)
 			break;
-		cpu_relax();
+		udelay(10);
+		timeout--;
 	}
-	WARN_ON(!time_before(jiffies, timeout));
+	WARN_ON(!timeout);
 	chan_write(chan, cp, CPDMA_TEARDOWN_VALUE);
 
 	/* handle completed packets */
-- 
1.7.10.4

[PATCH 2/5] net/cpsw: don't continue if we miss to allocate rx skbs

[Sebastian Andrzej Siewior]

if during "ifconfig up" we run out of mem we continue regardless how
many skbs we got. In worst case we have zero RX skbs and can't ever
receive further packets since the RX skbs are never reallocated. If
cpdma_chan_submit() fails we even leak the skb.
This patch changes the behavior here:
If we fail to allocate an skb during bring up we don't continue and
report that error. Same goes for errors from cpdma_chan_submit().
While here I changed to __netdev_alloc_skb_ip_align() so GFP_KERNEL can
be used.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/net/ethernet/ti/cpsw.c |   35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index e2ba702..29700fb 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -867,6 +867,15 @@ static void cpsw_init_host_port(struct cpsw_priv *priv)
 	}
 }
 
+static void cpsw_slave_stop(struct cpsw_slave *slave, struct cpsw_priv *priv)
+{
+	if (!slave->phy)
+		return;
+	phy_stop(slave->phy);
+	phy_disconnect(slave->phy);
+	slave->phy = NULL;
+}
+
 static int cpsw_ndo_open(struct net_device *ndev)
 {
 	struct cpsw_priv *priv = netdev_priv(ndev);
@@ -912,14 +921,16 @@ static int cpsw_ndo_open(struct net_device *ndev)
 			struct sk_buff *skb;
 
 			ret = -ENOMEM;
-			skb = netdev_alloc_skb_ip_align(priv->ndev,
-							priv->rx_packet_max);
+			skb = __netdev_alloc_skb_ip_align(priv->ndev,
+					priv->rx_packet_max, GFP_KERNEL);
 			if (!skb)
-				break;
+				goto err_cleanup;
 			ret = cpdma_chan_submit(priv->rxch, skb, skb->data,
 					skb_tailroom(skb), 0, GFP_KERNEL);
-			if (WARN_ON(ret < 0))
-				break;
+			if (ret < 0) {
+				kfree_skb(skb);
+				goto err_cleanup;
+			}
 		}
 		/* continue even if we didn't manage to submit all
 		 * receive descs
@@ -944,15 +955,13 @@ static int cpsw_ndo_open(struct net_device *ndev)
 	if (priv->data.dual_emac)
 		priv->slaves[priv->emac_port].open_stat = true;
 	return 0;
-}
 
-static void cpsw_slave_stop(struct cpsw_slave *slave, struct cpsw_priv *priv)
-{
-	if (!slave->phy)
-		return;
-	phy_stop(slave->phy);
-	phy_disconnect(slave->phy);
-	slave->phy = NULL;
+err_cleanup:
+	cpdma_ctlr_stop(priv->dma);
+	for_each_slave(priv, cpsw_slave_stop, priv);
+	pm_runtime_put_sync(&priv->pdev->dev);
+	netif_carrier_off(priv->ndev);
+	return ret;
 }
 
 static int cpsw_ndo_stop(struct net_device *ndev)
-- 
1.7.10.4

[PATCH 3/5] net/cpsw: don't rely only on netif_running() to check which device is active

[Sebastian Andrzej Siewior]

netif_running() reports false before the ->ndo_stop() callback is
called. That means if one executes "ifconfig down" and the system
receives an interrupt before the interrupt source has been disabled we
hang for always for two reasons:
- we never disable the interrupt source because devices claim to be
  already inactive and don't feel responsible.
- since the ISR always reports IRQ_HANDLED the line is never deactivated
  because it looks like the ISR feels responsible.

This patch changes the logic in the ISR a little:
- If none of the status registers reports an active source (RX or TX,
  misc is ignored because it is not actived) we leave with IRQ_NONE.
- the interrupt is deactivated
- The first active network device is taken and napi is scheduled. If
  none are active (a small race window between ndo_down() and the
  interrupt the) then we leave and should not come back because the
  source is off.
  There is no need to schedule the second NAPI because both share the
  same dma queue.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/net/ethernet/ti/cpsw.c |   33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 29700fb..13d4ed8 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -510,20 +510,31 @@ void cpsw_rx_handler(void *token, int len, int status)
 static irqreturn_t cpsw_interrupt(int irq, void *dev_id)
 {
 	struct cpsw_priv *priv = dev_id;
+	u32 rx, tx, rx_thresh;
 
-	if (likely(netif_running(priv->ndev))) {
-		cpsw_intr_disable(priv);
-		cpsw_disable_irq(priv);
+	rx_thresh = __raw_readl(&priv->wr_regs->rx_thresh_stat);
+	rx = __raw_readl(&priv->wr_regs->rx_stat);
+	tx = __raw_readl(&priv->wr_regs->tx_stat);
+	if (!rx_thresh && !rx && !tx)
+		return IRQ_NONE;
+
+	cpsw_intr_disable(priv);
+	cpsw_disable_irq(priv);
+
+	if (netif_running(priv->ndev)) {
 		napi_schedule(&priv->napi);
-	} else {
-		priv = cpsw_get_slave_priv(priv, 1);
-		if (likely(priv) && likely(netif_running(priv->ndev))) {
-			cpsw_intr_disable(priv);
-			cpsw_disable_irq(priv);
-			napi_schedule(&priv->napi);
-		}
+		return IRQ_HANDLED;
+	}
+
+	priv = cpsw_get_slave_priv(priv, 1);
+	if (!priv)
+		return IRQ_NONE;
+
+	if (netif_running(priv->ndev)) {
+		napi_schedule(&priv->napi);
+		return IRQ_HANDLED;
 	}
-	return IRQ_HANDLED;
+	return IRQ_NONE;
 }
 
 static int cpsw_poll(struct napi_struct *napi, int budget)
-- 
1.7.10.4

[PATCH 4/5] net/davinci_cpdma: remove unused argument in cpdma_chan_submit()

[Sebastian Andrzej Siewior]

The gfp_mask argument is not used in cpdma_chan_submit() and always set
to GFP_KERNEL even in atomic sections. This patch drops it since it is
unused.

Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/net/ethernet/ti/cpsw.c          |   10 +++++-----
 drivers/net/ethernet/ti/davinci_cpdma.c |    2 +-
 drivers/net/ethernet/ti/davinci_cpdma.h |    2 +-
 drivers/net/ethernet/ti/davinci_emac.c  |    6 +++---
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 13d4ed8..82c4574 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -502,7 +502,7 @@ void cpsw_rx_handler(void *token, int len, int status)
 			return;
 
 		ret = cpdma_chan_submit(priv->rxch, skb, skb->data,
-					skb_tailroom(skb), 0, GFP_KERNEL);
+					skb_tailroom(skb), 0);
 	}
 	WARN_ON(ret < 0);
 }
@@ -747,14 +747,14 @@ static inline int cpsw_tx_packet_submit(struct net_device *ndev,
 {
 	if (!priv->data.dual_emac)
 		return cpdma_chan_submit(priv->txch, skb, skb->data,
-				  skb->len, 0, GFP_KERNEL);
+				  skb->len, 0);
 
 	if (ndev == cpsw_get_slave_ndev(priv, 0))
 		return cpdma_chan_submit(priv->txch, skb, skb->data,
-				  skb->len, 1, GFP_KERNEL);
+				  skb->len, 1);
 	else
 		return cpdma_chan_submit(priv->txch, skb, skb->data,
-				  skb->len, 2, GFP_KERNEL);
+				  skb->len, 2);
 }
 
 static inline void cpsw_add_dual_emac_def_ale_entries(
@@ -937,7 +937,7 @@ static int cpsw_ndo_open(struct net_device *ndev)
 			if (!skb)
 				goto err_cleanup;
 			ret = cpdma_chan_submit(priv->rxch, skb, skb->data,
-					skb_tailroom(skb), 0, GFP_KERNEL);
+					skb_tailroom(skb), 0);
 			if (ret < 0) {
 				kfree_skb(skb);
 				goto err_cleanup;
diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
index 3e34187..3cc20e7 100644
--- a/drivers/net/ethernet/ti/davinci_cpdma.c
+++ b/drivers/net/ethernet/ti/davinci_cpdma.c
@@ -676,7 +676,7 @@ static void __cpdma_chan_submit(struct cpdma_chan *chan,
 }
 
 int cpdma_chan_submit(struct cpdma_chan *chan, void *token, void *data,
-		      int len, int directed, gfp_t gfp_mask)
+		      int len, int directed)
 {
 	struct cpdma_ctlr		*ctlr = chan->ctlr;
 	struct cpdma_desc __iomem	*desc;
diff --git a/drivers/net/ethernet/ti/davinci_cpdma.h b/drivers/net/ethernet/ti/davinci_cpdma.h
index d9bcc60..86dee48 100644
--- a/drivers/net/ethernet/ti/davinci_cpdma.h
+++ b/drivers/net/ethernet/ti/davinci_cpdma.h
@@ -89,7 +89,7 @@ int cpdma_chan_dump(struct cpdma_chan *chan);
 int cpdma_chan_get_stats(struct cpdma_chan *chan,
 			 struct cpdma_chan_stats *stats);
 int cpdma_chan_submit(struct cpdma_chan *chan, void *token, void *data,
-		      int len, int directed, gfp_t gfp_mask);
+		      int len, int directed);
 int cpdma_chan_process(struct cpdma_chan *chan, int quota);
 
 int cpdma_ctlr_int_ctrl(struct cpdma_ctlr *ctlr, bool enable);
diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
index 1fd7260..f1daa4d 100644
--- a/drivers/net/ethernet/ti/davinci_emac.c
+++ b/drivers/net/ethernet/ti/davinci_emac.c
@@ -1037,7 +1037,7 @@ static void emac_rx_handler(void *token, int len, int status)
 
 recycle:
 	ret = cpdma_chan_submit(priv->rxchan, skb, skb->data,
-			skb_tailroom(skb), 0, GFP_KERNEL);
+			skb_tailroom(skb), 0);
 
 	WARN_ON(ret == -ENOMEM);
 	if (unlikely(ret < 0))
@@ -1092,7 +1092,7 @@ static int emac_dev_xmit(struct sk_buff *skb, struct net_device *ndev)
 	skb_tx_timestamp(skb);
 
 	ret_code = cpdma_chan_submit(priv->txchan, skb, skb->data, skb->len,
-				     0, GFP_KERNEL);
+				     0);
 	if (unlikely(ret_code != 0)) {
 		if (netif_msg_tx_err(priv) && net_ratelimit())
 			dev_err(emac_dev, "DaVinci EMAC: desc submit failed");
@@ -1558,7 +1558,7 @@ static int emac_dev_open(struct net_device *ndev)
 			break;
 
 		ret = cpdma_chan_submit(priv->rxchan, skb, skb->data,
-					skb_tailroom(skb), 0, GFP_KERNEL);
+					skb_tailroom(skb), 0);
 		if (WARN_ON(ret < 0))
 			break;
 	}
-- 
1.7.10.4

[PATCH 5/5] net/cpsw: redo rx skb allocation in rx path

[Sebastian Andrzej Siewior]

In case that we run into OOM during the allocation of the new rx-skb we
don't get one and we have one skb less than we used to have. If this
continues to happen then we end up with no rx-skbs at all.
This patch changes the following:
- if we fail to allocate the new skb, then we treat the currently
  completed skb as the new one and so drop the currently received data.
- instead of testing multiple times if the device is gone we rely one
  the status field which is set to -ENOSYS in case the channel is going
  down and incomplete requests are purged.
  cpdma_chan_stop() removes most of the packages with -ENOSYS. The
  currently active packet which is removed has the "tear down" bit set.
  So if that bit is set, we send ENOSYS as well otherwise we pass the
  status bits which are required to figure out which of the two possible
  just finished.

Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/net/ethernet/ti/cpsw.c          |   33 ++++++++++++-------------------
 drivers/net/ethernet/ti/davinci_cpdma.c |    7 ++++++-
 2 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 82c4574..1fc89a5 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -468,43 +468,36 @@ void cpsw_tx_handler(void *token, int len, int status)
 void cpsw_rx_handler(void *token, int len, int status)
 {
 	struct sk_buff		*skb = token;
+	struct sk_buff		*new_skb;
 	struct net_device	*ndev = skb->dev;
 	struct cpsw_priv	*priv = netdev_priv(ndev);
 	int			ret = 0;
 
 	cpsw_dual_emac_src_port_detect(status, priv, ndev, skb);
 
-	/* free and bail if we are shutting down */
-	if (unlikely(!netif_running(ndev)) ||
-			unlikely(!netif_carrier_ok(ndev))) {
+	if (unlikely(status < 0)) {
+		/* the interface is going down, skbs are purged */
 		dev_kfree_skb_any(skb);
 		return;
 	}
-	if (likely(status >= 0)) {
+
+	new_skb = netdev_alloc_skb_ip_align(ndev, priv->rx_packet_max);
+	if (new_skb) {
 		skb_put(skb, len);
 		cpts_rx_timestamp(priv->cpts, skb);
 		skb->protocol = eth_type_trans(skb, ndev);
 		netif_receive_skb(skb);
 		priv->stats.rx_bytes += len;
 		priv->stats.rx_packets++;
-		skb = NULL;
-	}
-
-	if (unlikely(!netif_running(ndev))) {
-		if (skb)
-			dev_kfree_skb_any(skb);
-		return;
+	} else {
+		priv->stats.rx_dropped++;
+		new_skb = skb;
 	}
 
-	if (likely(!skb)) {
-		skb = netdev_alloc_skb_ip_align(ndev, priv->rx_packet_max);
-		if (WARN_ON(!skb))
-			return;
-
-		ret = cpdma_chan_submit(priv->rxch, skb, skb->data,
-					skb_tailroom(skb), 0);
-	}
-	WARN_ON(ret < 0);
+	ret = cpdma_chan_submit(priv->rxch, new_skb, new_skb->data,
+			skb_tailroom(new_skb), 0);
+	if (WARN_ON(ret < 0))
+		dev_kfree_skb_any(new_skb);
 }
 
 static irqreturn_t cpsw_interrupt(int irq, void *dev_id)
diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
index 3cc20e7..6b0a89f 100644
--- a/drivers/net/ethernet/ti/davinci_cpdma.c
+++ b/drivers/net/ethernet/ti/davinci_cpdma.c
@@ -776,6 +776,7 @@ static int __cpdma_chan_process(struct cpdma_chan *chan)
 	struct cpdma_ctlr		*ctlr = chan->ctlr;
 	struct cpdma_desc __iomem	*desc;
 	int				status, outlen;
+	int				cb_status = 0;
 	struct cpdma_desc_pool		*pool = ctlr->pool;
 	dma_addr_t			desc_dma;
 	unsigned long			flags;
@@ -811,8 +812,12 @@ static int __cpdma_chan_process(struct cpdma_chan *chan)
 	}
 
 	spin_unlock_irqrestore(&chan->lock, flags);
+	if (unlikely(status & CPDMA_DESC_TD_COMPLETE))
+		cb_status = -ENOSYS;
+	else
+		cb_status = status;
 
-	__cpdma_chan_free(chan, desc, outlen, status);
+	__cpdma_chan_free(chan, desc, outlen, cb_status);
 	return status;
 
 unlock_ret:
-- 
1.7.10.4

Re: [PATCH 2/5] net/cpsw: don't continue if we miss to allocate rx skbs

[Mugunthan V N]

On 4/23/2013 11:01 PM, Sebastian Andrzej Siewior wrote:
> if during "ifconfig up" we run out of mem we continue regardless how
> many skbs we got. In worst case we have zero RX skbs and can't ever
> receive further packets since the RX skbs are never reallocated. If
> cpdma_chan_submit() fails we even leak the skb.
> This patch changes the behavior here:
> If we fail to allocate an skb during bring up we don't continue and
> report that error. Same goes for errors from cpdma_chan_submit().
> While here I changed to __netdev_alloc_skb_ip_align() so GFP_KERNEL can
> be used.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>   drivers/net/ethernet/ti/cpsw.c |   35 ++++++++++++++++++++++-------------
>   1 file changed, 22 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index e2ba702..29700fb 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -867,6 +867,15 @@ static void cpsw_init_host_port(struct cpsw_priv *priv)
>   	}
>   }
>   
> +static void cpsw_slave_stop(struct cpsw_slave *slave, struct cpsw_priv *priv)
> +{
> +	if (!slave->phy)
> +		return;
> +	phy_stop(slave->phy);
> +	phy_disconnect(slave->phy);
> +	slave->phy = NULL;
> +}
> +
>   static int cpsw_ndo_open(struct net_device *ndev)
>   {
>   	struct cpsw_priv *priv = netdev_priv(ndev);
> @@ -912,14 +921,16 @@ static int cpsw_ndo_open(struct net_device *ndev)
>   			struct sk_buff *skb;
>   
>   			ret = -ENOMEM;
> -			skb = netdev_alloc_skb_ip_align(priv->ndev,
> -							priv->rx_packet_max);
> +			skb = __netdev_alloc_skb_ip_align(priv->ndev,
> +					priv->rx_packet_max, GFP_KERNEL);
>   			if (!skb)
> -				break;
> +				goto err_cleanup;
>   			ret = cpdma_chan_submit(priv->rxch, skb, skb->data,
>   					skb_tailroom(skb), 0, GFP_KERNEL);
> -			if (WARN_ON(ret < 0))
> -				break;
> +			if (ret < 0) {
> +				kfree_skb(skb);
> +				goto err_cleanup;
> +			}
>   		}
>   		/* continue even if we didn't manage to submit all
>   		 * receive descs
> @@ -944,15 +955,13 @@ static int cpsw_ndo_open(struct net_device *ndev)
>   	if (priv->data.dual_emac)
>   		priv->slaves[priv->emac_port].open_stat = true;
>   	return 0;
> -}
>   
> -static void cpsw_slave_stop(struct cpsw_slave *slave, struct cpsw_priv *priv)
> -{
> -	if (!slave->phy)
> -		return;
> -	phy_stop(slave->phy);
> -	phy_disconnect(slave->phy);
> -	slave->phy = NULL;
> +err_cleanup:
> +	cpdma_ctlr_stop(priv->dma);
> +	for_each_slave(priv, cpsw_slave_stop, priv);
> +	pm_runtime_put_sync(&priv->pdev->dev);
> +	netif_carrier_off(priv->ndev);
> +	return ret;
>   }
>   
>   static int cpsw_ndo_stop(struct net_device *ndev)
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>

Regards
Mugunthan V N

Re: [PATCH 3/5] net/cpsw: don't rely only on netif_running() to check which device is active

[Mugunthan V N]

On 4/23/2013 11:01 PM, Sebastian Andrzej Siewior wrote:
> netif_running() reports false before the ->ndo_stop() callback is
> called. That means if one executes "ifconfig down" and the system
> receives an interrupt before the interrupt source has been disabled we
> hang for always for two reasons:
> - we never disable the interrupt source because devices claim to be
>    already inactive and don't feel responsible.
> - since the ISR always reports IRQ_HANDLED the line is never deactivated
>    because it looks like the ISR feels responsible.
>
> This patch changes the logic in the ISR a little:
> - If none of the status registers reports an active source (RX or TX,
>    misc is ignored because it is not actived) we leave with IRQ_NONE.
> - the interrupt is deactivated
> - The first active network device is taken and napi is scheduled. If
>    none are active (a small race window between ndo_down() and the
>    interrupt the) then we leave and should not come back because the
>    source is off.
>    There is no need to schedule the second NAPI because both share the
>    same dma queue.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>   drivers/net/ethernet/ti/cpsw.c |   33 ++++++++++++++++++++++-----------
>   1 file changed, 22 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 29700fb..13d4ed8 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -510,20 +510,31 @@ void cpsw_rx_handler(void *token, int len, int status)
>   static irqreturn_t cpsw_interrupt(int irq, void *dev_id)
>   {
>   	struct cpsw_priv *priv = dev_id;
> +	u32 rx, tx, rx_thresh;
>   
> -	if (likely(netif_running(priv->ndev))) {
> -		cpsw_intr_disable(priv);
> -		cpsw_disable_irq(priv);
> +	rx_thresh = __raw_readl(&priv->wr_regs->rx_thresh_stat);
> +	rx = __raw_readl(&priv->wr_regs->rx_stat);
> +	tx = __raw_readl(&priv->wr_regs->tx_stat);
> +	if (!rx_thresh && !rx && !tx)
> +		return IRQ_NONE;
> +
> +	cpsw_intr_disable(priv);
> +	cpsw_disable_irq(priv);
> +
> +	if (netif_running(priv->ndev)) {
>   		napi_schedule(&priv->napi);
> -	} else {
> -		priv = cpsw_get_slave_priv(priv, 1);
> -		if (likely(priv) && likely(netif_running(priv->ndev))) {
> -			cpsw_intr_disable(priv);
> -			cpsw_disable_irq(priv);
> -			napi_schedule(&priv->napi);
> -		}
> +		return IRQ_HANDLED;
> +	}
> +
> +	priv = cpsw_get_slave_priv(priv, 1);
> +	if (!priv)
> +		return IRQ_NONE;
> +
> +	if (netif_running(priv->ndev)) {
> +		napi_schedule(&priv->napi);
> +		return IRQ_HANDLED;
>   	}
> -	return IRQ_HANDLED;
> +	return IRQ_NONE;
>   }
>   
>   static int cpsw_poll(struct napi_struct *napi, int budget)
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>

Regards
Mugunthan V N

Re: cpsw queue, v2

[David Miller]

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Date: Tue, 23 Apr 2013 19:31:34 +0200

> patches one, four and five gained acked-by tags nothing else changed.
> Patch two gained pm_runtime_put_sync() and clean up for the slave.
> Patch three is a complete redo of the initial patch.

Series applied to net-next.