my small cpsw queue

my small cpsw queue

[Sebastian Andrzej Siewior]

Hi Mugunthan,

this are the things I saw while looking at the interrupt mask problem.

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.

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

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

[Mugunthan V N]

On 4/18/2013 3:22 AM, Sebastian Andrzej Siewior wrote:
> __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.
>
> Signed-off-by: Sebastian Andrzej Siewior<bigeasy@linutronix.de>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>

Regards
Mugunthan V N

[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

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

[Sebastian Andrzej Siewior]

netif_running() reports false before even 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 (or non-present)
- since the ISR always reports IRQ_HANDLED the line is never deactivated
  because it looks like the ISR feels respsonsible.

This patch introduces now the ->active field which is set/cleared in
ndo_open / ndo_stop.

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

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 3b22a36..c32780d 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -341,6 +341,7 @@ struct cpsw_priv {
 	int				host_port;
 	struct clk			*clk;
 	u8				mac_addr[ETH_ALEN];
+	u8				active;
 	struct cpsw_slave		*slaves;
 	struct cpdma_ctlr		*dma;
 	struct cpdma_chan		*txch, *rxch;
@@ -511,19 +512,24 @@ static irqreturn_t cpsw_interrupt(int irq, void *dev_id)
 {
 	struct cpsw_priv *priv = dev_id;
 
-	if (likely(netif_running(priv->ndev))) {
+	if (priv->active) {
 		cpsw_intr_disable(priv);
 		cpsw_disable_irq(priv);
 		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 (priv->active) {
+		cpsw_intr_disable(priv);
+		cpsw_disable_irq(priv);
+		napi_schedule(&priv->napi);
+		return IRQ_HANDLED;
 	}
-	return IRQ_HANDLED;
+	return IRQ_NONE;
 }
 
 static int cpsw_poll(struct napi_struct *napi, int budget)
@@ -937,6 +943,7 @@ static int cpsw_ndo_open(struct net_device *ndev)
 		cpsw_set_coalesce(ndev, &coal);
 	}
 
+	priv->active = 1;
 	cpdma_ctlr_start(priv->dma);
 	cpsw_intr_enable(priv);
 	napi_enable(&priv->napi);
@@ -980,6 +987,7 @@ static int cpsw_ndo_stop(struct net_device *ndev)
 	pm_runtime_put_sync(&priv->pdev->dev);
 	if (priv->data.dual_emac)
 		priv->slaves[priv->emac_port].open_stat = false;
+	priv->active = 0;
 	return 0;
 }
 
-- 
1.7.10.4

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

[Mugunthan V N]

On 4/18/2013 3:22 AM, Sebastian Andrzej Siewior wrote:
> netif_running() reports false before even 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 (or non-present)
> - since the ISR always reports IRQ_HANDLED the line is never deactivated
>    because it looks like the ISR feels respsonsible.
>
> This patch introduces now the ->active field which is set/cleared in
> ndo_open / ndo_stop.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>   drivers/net/ethernet/ti/cpsw.c |   26 +++++++++++++++++---------
>   1 file changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 3b22a36..c32780d 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -341,6 +341,7 @@ struct cpsw_priv {
>   	int				host_port;
>   	struct clk			*clk;
>   	u8				mac_addr[ETH_ALEN];
> +	u8				active;
Frame work is providing this feature then why you need to have additional
book keeping in device private?
>   	struct cpsw_slave		*slaves;
>   	struct cpdma_ctlr		*dma;
>   	struct cpdma_chan		*txch, *rxch;
> @@ -511,19 +512,24 @@ static irqreturn_t cpsw_interrupt(int irq, void *dev_id)
>   {
>   	struct cpsw_priv *priv = dev_id;
>   
> -	if (likely(netif_running(priv->ndev))) {
> +	if (priv->active) {
>   		cpsw_intr_disable(priv);
>   		cpsw_disable_irq(priv);
>   		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 (priv->active) {
> +		cpsw_intr_disable(priv);
> +		cpsw_disable_irq(priv);
> +		napi_schedule(&priv->napi);
> +		return IRQ_HANDLED;
>   	}
> -	return IRQ_HANDLED;
> +	return IRQ_NONE;
>   }
>   
>   static int cpsw_poll(struct napi_struct *napi, int budget)
> @@ -937,6 +943,7 @@ static int cpsw_ndo_open(struct net_device *ndev)
>   		cpsw_set_coalesce(ndev, &coal);
>   	}
>   
> +	priv->active = 1;
>   	cpdma_ctlr_start(priv->dma);
>   	cpsw_intr_enable(priv);
>   	napi_enable(&priv->napi);
> @@ -980,6 +987,7 @@ static int cpsw_ndo_stop(struct net_device *ndev)
>   	pm_runtime_put_sync(&priv->pdev->dev);
>   	if (priv->data.dual_emac)
>   		priv->slaves[priv->emac_port].open_stat = false;
> +	priv->active = 0;
>   	return 0;
>   }
>   

Regards
Mugunthan V N

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

[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 3b22a36..c32780d 100644
>> --- a/drivers/net/ethernet/ti/cpsw.c
>> +++ b/drivers/net/ethernet/ti/cpsw.c
>> @@ -341,6 +341,7 @@ struct cpsw_priv {
>>       int                host_port;
>>       struct clk            *clk;
>>       u8                mac_addr[ETH_ALEN];
>> +    u8                active;
> Frame work is providing this feature then why you need to have additional
> book keeping in device private?

Which function? I more than happy to use it.

> 
> Regards
> Mugunthan V N

Sebastian

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

[Mugunthan V N]

On 4/18/2013 5:40 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 3b22a36..c32780d 100644
>>> --- a/drivers/net/ethernet/ti/cpsw.c
>>> +++ b/drivers/net/ethernet/ti/cpsw.c
>>> @@ -341,6 +341,7 @@ struct cpsw_priv {
>>>        int                host_port;
>>>        struct clk            *clk;
>>>        u8                mac_addr[ETH_ALEN];
>>> +    u8                active;
>> Frame work is providing this feature then why you need to have additional
>> book keeping in device private?
> Which function? I more than happy to use it.
>
The same netif_running(). But why you are telling not to rely on this API?
device state is updated immediately after successful ndo_open.

Regards
Mugunthan V N

[PATCH 3/5 v2] 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 (or non-present) 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 looks at both, netif_running() and IFF_UP to check if the
device is up. IFF_UP is set after ndo_open() completes and cleared after
ndo_close() completes and netif_running is set before ndo_open() is
called and cleared before ndo_close() completes. Using both will avoid
false positives during the bring down sequence.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
* Mugunthan V N | 2013-04-19 16:05:55 [+0530]:

>The same netif_running(). But why you are telling not to rely on this API?
>device state is updated immediately after successful ndo_open.

I tried to describe the reason for that in my patch description. Here 
it is again: netif_running() reports false before ndo_close() has been
called. That means an interrupt between the flag change and interrupt
disabling in ndo_close() will currently lockup the box (IRQ_NONE would
at least allow the core to disable the interrupt line).

Initialyly I decided against using IFF_UP as well but using would work
without the private active field. So here we have 

-v1…v2: replacing private active field with IFF_UP + netif_running()

How about this?

>Regards
>Mugunthan V N
 drivers/net/ethernet/ti/cpsw.c |   23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 3b22a36..2705725 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -511,19 +511,24 @@ static irqreturn_t cpsw_interrupt(int irq, void *dev_id)
 {
 	struct cpsw_priv *priv = dev_id;
 
-	if (likely(netif_running(priv->ndev))) {
+	if (likely(netif_running(priv->ndev) || priv->ndev->flags & IFF_UP)) {
 		cpsw_intr_disable(priv);
 		cpsw_disable_irq(priv);
 		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 (likely(netif_running(priv->ndev) || priv->ndev->flags & IFF_UP)) {
+		cpsw_intr_disable(priv);
+		cpsw_disable_irq(priv);
+		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

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

[Mugunthan V N]

On 4/22/2013 2:00 PM, Sebastian Andrzej Siewior wrote:
> I tried to describe the reason for that in my patch description. Here
> it is again: netif_running() reports false before ndo_close() has been
> called. That means an interrupt between the flag change and interrupt
> disabling in ndo_close() will currently lockup the box (IRQ_NONE would
> at least allow the core to disable the interrupt line).
>
> Initialyly I decided against using IFF_UP as well but using would work
> without the private active field. So here we have
>
> -v1…v2: replacing private active field with IFF_UP + netif_running()
>
> How about this?
If this is the case, then other Ethernet drivers will also have the same 
scenario,
if it can be fixed in a generic way then i will be good.

Regards
Mugunthan V N

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

[Sebastian Andrzej Siewior]

On 04/22/2013 11:14 AM, Mugunthan V N wrote:
>> How about this?
> If this is the case, then other Ethernet drivers will also have the same
> scenario,
> if it can be fixed in a generic way then i will be good.

Other ethernet drivers know if their device generated an interrupt,
stop the source and schedule napi. But here it could be either the
device or the slave device, right?
After writing this, a better idea idea just bumped into my head.

btw: Any update on http://www.spinics.net/lists/netdev/msg233296.html?

> Regards
> Mugunthan V N

Sebastian

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

[Mugunthan V N]

On 4/22/2013 3:00 PM, Sebastian Andrzej Siewior wrote:
> On 04/22/2013 11:14 AM, Mugunthan V N wrote:
>>> How about this?
>> If this is the case, then other Ethernet drivers will also have the same
>> scenario,
>> if it can be fixed in a generic way then i will be good.
> Other ethernet drivers know if their device generated an interrupt,
> stop the source and schedule napi. But here it could be either the
> device or the slave device, right?
> After writing this, a better idea idea just bumped into my head.
>
> btw: Any update on http://www.spinics.net/lists/netdev/msg233296.html?
>
Yes, when interrupt is disabled in hardware then interrupt must be 
disabled in
my initial days i have verified it, CPSW doesn't allow any new interrupts to
propagate  to arm if CPSW interrupt is disabled but there is an issue. 
CPSW irq
signal is not directly connected to irq controller, it is connected via 
edge to
level conversion so if one interrupt reaches arm, irq controller has to be
disabled else arm get blocked in CPSW ISR.

Regards
Mugunthan V N

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

[Sebastian Andrzej Siewior]

On 04/22/2013 11:40 AM, Mugunthan V N wrote:
> Yes, when interrupt is disabled in hardware then interrupt must be
> disabled in
here is a word missing and I can't figure it out.

> my initial days i have verified it, CPSW doesn't allow any new
> interrupts to
> propagate  to arm if CPSW interrupt is disabled but there is an issue.
Not completed i.e. outstanding I think.

> CPSW irq
> signal is not directly connected to irq controller, it is connected via
> edge to
> level conversion so if one interrupt reaches arm, irq controller has to be
> disabled else arm get blocked in CPSW ISR.

Okay. Is there an errata document describing this behavior? If the CPSW
interrupt source is disabled, the CPSW should keep quiet. This is also
what the manual says. If cpsw does not allow new interrupts to
propagate to ARM's INTC then there is something wrong. It looks like
the source is not disabled in CPSW properly. This works on ES1.0.

Lets get back to this ofter this series of five is merged.

> Regards
> Mugunthan V N

Sebastian

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

[Mugunthan V N]

On 4/22/2013 3:19 PM, Sebastian Andrzej Siewior wrote:
> On 04/22/2013 11:40 AM, Mugunthan V N wrote:
>> Yes, when interrupt is disabled in hardware then interrupt must be
>> disabled in
> here is a word missing and I can't figure it out.
>
>> my initial days i have verified it, CPSW doesn't allow any new
>> interrupts to
>> propagate  to arm if CPSW interrupt is disabled but there is an issue.
> Not completed i.e. outstanding I think.
>
>> CPSW irq
>> signal is not directly connected to irq controller, it is connected via
>> edge to
>> level conversion so if one interrupt reaches arm, irq controller has to be
>> disabled else arm get blocked in CPSW ISR.
> Okay. Is there an errata document describing this behavior? If the CPSW
> interrupt source is disabled, the CPSW should keep quiet. This is also
> what the manual says. If cpsw does not allow new interrupts to
> propagate to ARM's INTC then there is something wrong. It looks like
> the source is not disabled in CPSW properly. This works on ES1.0.
>
> Lets get back to this ofter this series of five is merged.
>
Sorry there was a formatting issue...

Yes, when interrupt is disabled in hardware then interrupt must be disabled.
In my initial days i have verified it, CPSW doesn't allow any new 
interrupts to
propagate  to arm if CPSW interrupt is disabled which is correct. CPSW irq
signal is not directly connected to irq controller, it is connected via 
edge to
level conversion so it is a kind of latched interrupt and so the 
interrupt has
to be disabled in interrupt controller to disable the interrupt, else 
arm get
blocked in CPSW ISR.

This behavior is not only in AM335x CPSW, it is as is from its previous
versions in TI814x and TI8107.

Regards
Mugunthan V N

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

[Sebastian Andrzej Siewior]

On 04/22/2013 12:12 PM, Mugunthan V N wrote:
> 
> Yes, when interrupt is disabled in hardware then interrupt must be
> disabled.
> In my initial days i have verified it, CPSW doesn't allow any new
> interrupts to
> propagate  to arm if CPSW interrupt is disabled which is correct. CPSW irq
> signal is not directly connected to irq controller, it is connected via
> edge to
> level conversion so it is a kind of latched interrupt and so the
> interrupt has
> to be disabled in interrupt controller to disable the interrupt, else
> arm get
> blocked in CPSW ISR.
> 
> This behavior is not only in AM335x CPSW, it is as is from its previous
> versions in TI814x and TI8107.

And I don't see this on ES1.0 because they forgot to add the
edge-to-level converter?

> Regards
> Mugunthan V N

Sebastian

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

[David Miller]

When sending a new version of a patch, you must resubmit the entire
series, not just the patch which is changing.

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

[Sergei Shtylyov]

Hello.

On 18-04-2013 1:52, Sebastian Andrzej Siewior wrote:

> netif_running() reports false before even 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 (or non-present)
> - since the ISR always reports IRQ_HANDLED the line is never deactivated
>    because it looks like the ISR feels respsonsible.

> This patch introduces now the ->active field which is set/cleared in
> ndo_open / ndo_stop.

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

> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 3b22a36..c32780d 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -341,6 +341,7 @@ struct cpsw_priv {
>   	int				host_port;
>   	struct clk			*clk;
>   	u8				mac_addr[ETH_ALEN];
> +	u8				active;

    *bool* seems to fit better here.

WBR, Sergei

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

[Sebastian Andrzej Siewior]

On 04/19/2013 03:10 PM, Sergei Shtylyov wrote:
Hello Sergei,

>> --- a/drivers/net/ethernet/ti/cpsw.c
>> +++ b/drivers/net/ethernet/ti/cpsw.c
>> @@ -341,6 +341,7 @@ struct cpsw_priv {
>>       int                host_port;
>>       struct clk            *clk;
>>       u8                mac_addr[ETH_ALEN];
>> +    u8                active;
> 
>    *bool* seems to fit better here.

yes but would require the compiler to add an extra pad between u8 and
bool and another between bool and the pointer. That way it does not
cause the struct to grow :) Anyway, v2 is out without the need for it.

> 
> WBR, Sergei
> 

Sebastian

[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.

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 c32780d..559b020 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -503,7 +503,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);
 }
@@ -742,14 +742,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(
@@ -923,7 +923,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

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

[Mugunthan V N]

On 4/18/2013 3:22 AM, Sebastian Andrzej Siewior wrote:
> 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.
>
> 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(-)
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>

Regards
Mugunthan V N

[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.

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 559b020..f684e9b 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -469,43 +469,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 5/5] net/cpsw: redo rx skb allocation in rx path

[Mugunthan V N]

On 4/18/2013 3:22 AM, Sebastian Andrzej Siewior wrote:
> 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.
>
> 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 559b020..f684e9b 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -469,43 +469,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;
Why you want to drop a successfully received packet as you memory alloc 
failed?
Let the stack get it processed and there after you can continue with one 
less
rx 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:

Regards
Mugunthan V N

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

[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 559b020..f684e9b 100644
>> --- a/drivers/net/ethernet/ti/cpsw.c
>> +++ b/drivers/net/ethernet/ti/cpsw.c
>> @@ -469,43 +469,36 @@ void cpsw_tx_handler(void *token, int len, int
<snip>
>> -    if (unlikely(!netif_running(ndev))) {
>> -        if (skb)
>> -            dev_kfree_skb_any(skb);
>> -        return;
>> +    } else {
>> +        priv->stats.rx_dropped++;
>> +        new_skb = skb;
> Why you want to drop a successfully received packet as you memory alloc
> failed?
> Let the stack get it processed and there after you can continue with one
> less
> rx skb

Because, as I wrote, if this continues you end up without rx skbs.
However, if you if you are under memory pressure it is possible that
other callers in the stack have the same problem.

> 
> Regards
> Mugunthan V N

Sebastian

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

[Mugunthan V N]

On 4/18/2013 5:43 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 559b020..f684e9b 100644
>>> --- a/drivers/net/ethernet/ti/cpsw.c
>>> +++ b/drivers/net/ethernet/ti/cpsw.c
>>> @@ -469,43 +469,36 @@ void cpsw_tx_handler(void *token, int len, int
> <snip>
>>> -    if (unlikely(!netif_running(ndev))) {
>>> -        if (skb)
>>> -            dev_kfree_skb_any(skb);
>>> -        return;
>>> +    } else {
>>> +        priv->stats.rx_dropped++;
>>> +        new_skb = skb;
>> Why you want to drop a successfully received packet as you memory alloc
>> failed?
>> Let the stack get it processed and there after you can continue with one
>> less
>> rx skb
> Because, as I wrote, if this continues you end up without rx skbs.
> However, if you if you are under memory pressure it is possible that
> other callers in the stack have the same problem.
>
I had gone through some other drivers and found that this patch is valid 
so add

Acked-by: Mugunthan V N <mugunthanvnm@ti.com>

Regards
Mugunthan V N

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

[Eric Dumazet]

On Thu, 2013-04-18 at 17:20 +0530, Mugunthan V N wrote:
> On 4/18/2013 3:22 AM, Sebastian Andrzej Siewior wrote:
> > 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.
> >
> > 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 559b020..f684e9b 100644
> > --- a/drivers/net/ethernet/ti/cpsw.c
> > +++ b/drivers/net/ethernet/ti/cpsw.c
> > @@ -469,43 +469,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;
> Why you want to drop a successfully received packet as you memory alloc 
> failed?
> Let the stack get it processed and there after you can continue with one 
> less
> rx skb

Have you read the changelog at all ?

This patch sounds quite correct to me.

If you cannot allocate a new skb, then drop the incoming frame, instead
of dealing with strange allocation retries later.

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 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