[PATCH] net/gianfar: drop recycled skbs on MTU change

[PATCH] net/gianfar: drop recycled skbs on MTU change

[Sebastian Andrzej Siewior]

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

The size for skb which is added to the recycled list is using the
current descriptor size which is current MTU. gfar_new_skb() is also
using this size. So after changing or alteast increasing the MTU all
recycled skbs should be dropped.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
I'm not 100% sure but it looks like it is wrong.

 drivers/net/gianfar.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index 5267c27..9093106 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -2287,8 +2287,10 @@ static int gfar_change_mtu(struct net_device *dev, int new_mtu)
 
 	/* Only stop and start the controller if it isn't already
 	 * stopped, and we changed something */
-	if ((oldsize != tempsize) && (dev->flags & IFF_UP))
+	if ((oldsize != tempsize) && (dev->flags & IFF_UP)) {
 		stop_gfar(dev);
+		skb_queue_purge(&priv->rx_recycle);
+	}
 
 	priv->rx_buffer_size = tempsize;
 
-- 
1.6.6.1

Re: [PATCH] net/gianfar: drop recycled skbs on MTU change

[David Miller]

From: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
Date: Mon, 3 May 2010 17:17:45 +0200

> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 
> The size for skb which is added to the recycled list is using the
> current descriptor size which is current MTU. gfar_new_skb() is also
> using this size. So after changing or alteast increasing the MTU all
> recycled skbs should be dropped.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> I'm not 100% sure but it looks like it is wrong.

It looks right to me, can I get an ACK from gianfar developers?

Re: [PATCH] net/gianfar: drop recycled skbs on MTU change

[Andy Fleming]

On Mon, May 3, 2010 at 8:17 AM, Sebastian Andrzej Siewior
<sebastian@breakpoint.cc> wrote:
> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>
> The size for skb which is added to the recycled list is using the
> current descriptor size which is current MTU. gfar_new_skb() is also
> using this size. So after changing or alteast increasing the MTU all
> recycled skbs should be dropped.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> I'm not 100% sure but it looks like it is wrong.
>
>  drivers/net/gianfar.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
> index 5267c27..9093106 100644
> --- a/drivers/net/gianfar.c
> +++ b/drivers/net/gianfar.c
> @@ -2287,8 +2287,10 @@ static int gfar_change_mtu(struct net_device *dev, int new_mtu)
>
>        /* Only stop and start the controller if it isn't already
>         * stopped, and we changed something */
> -       if ((oldsize != tempsize) && (dev->flags & IFF_UP))
> +       if ((oldsize != tempsize) && (dev->flags & IFF_UP)) {
>                stop_gfar(dev);
> +               skb_queue_purge(&priv->rx_recycle);
> +       }


I think we should probably do this in free_skb_resources.  And remove
the call from gfar_close().

Andy

Re: [PATCH] net/gianfar: drop recycled skbs on MTU change

[David Miller]

From: Andy Fleming <afleming@gmail.com>
Date: Tue, 4 May 2010 08:29:06 -0700

> On Mon, May 3, 2010 at 8:17 AM, Sebastian Andrzej Siewior
> <sebastian@breakpoint.cc> wrote:
>> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>>
>> The size for skb which is added to the recycled list is using the
>> current descriptor size which is current MTU. gfar_new_skb() is also
>> using this size. So after changing or alteast increasing the MTU all
>> recycled skbs should be dropped.
>>
>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>> ---
>> I'm not 100% sure but it looks like it is wrong.
>>
>>  drivers/net/gianfar.c |    4 +++-
>>  1 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
>> index 5267c27..9093106 100644
>> --- a/drivers/net/gianfar.c
>> +++ b/drivers/net/gianfar.c
>> @@ -2287,8 +2287,10 @@ static int gfar_change_mtu(struct net_device *dev, int new_mtu)
>>
>>        /* Only stop and start the controller if it isn't already
>>         * stopped, and we changed something */
>> -       if ((oldsize != tempsize) && (dev->flags & IFF_UP))
>> +       if ((oldsize != tempsize) && (dev->flags & IFF_UP)) {
>>                stop_gfar(dev);
>> +               skb_queue_purge(&priv->rx_recycle);
>> +       }
> 
> 
> I think we should probably do this in free_skb_resources.  And remove
> the call from gfar_close().

Ok, Sebastian please rework your patch as requested by Andy.

Thanks.

[PATCH v2] net/gianfar: drop recycled skbs on MTU change

[Sebastian Andrzej Siewior]

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

The size for skbs which is added to the recycled list is using the
current descriptor size which is current MTU. gfar_new_skb() is also
using this size. So after changing or alteast increasing the MTU all
recycled skbs should be dropped.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
>> I think we should probably do this in free_skb_resources.  And remove
>> the call from gfar_close().
>
>Ok, Sebastian please rework your patch as requested by Andy.

This has the side effect of dropping them on reset which is not
required.

 drivers/net/gianfar.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index 4e97ca1..5d3763f 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -1649,6 +1649,7 @@ static void free_skb_resources(struct gfar_private *priv)
 			sizeof(struct rxbd8) * priv->total_rx_ring_size,
 			priv->tx_queue[0]->tx_bd_base,
 			priv->tx_queue[0]->tx_bd_dma_base);
+	skb_queue_purge(&priv->rx_recycle);
 }
 
 void gfar_start(struct net_device *dev)
@@ -2088,7 +2089,6 @@ static int gfar_close(struct net_device *dev)
 
 	disable_napi(priv);
 
-	skb_queue_purge(&priv->rx_recycle);
 	cancel_work_sync(&priv->reset_task);
 	stop_gfar(dev);
 
-- 
1.6.6.1

Re: [PATCH v2] net/gianfar: drop recycled skbs on MTU change

[Andy Fleming]

On May 5, 2010, at 3:30 AM, Sebastian Andrzej Siewior wrote:

> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 
> The size for skbs which is added to the recycled list is using the
> current descriptor size which is current MTU. gfar_new_skb() is also
> using this size. So after changing or alteast increasing the MTU all
> recycled skbs should be dropped.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>>> I think we should probably do this in free_skb_resources.  And remove
>>> the call from gfar_close().
>> 
>> Ok, Sebastian please rework your patch as requested by Andy.
> 
> This has the side effect of dropping them on reset which is not
> required.

Sure, but that's true of the buffers in the BD ring, too.  My theory, here, is just that it's best to treat it the same as the other "skb resources".  If we want to avoid reallocation during a reset, that's a separate patch.  :)

Acked-by: Andy Fleming <afleming@freescale.com>

Re: [PATCH v2] net/gianfar: drop recycled skbs on MTU change

[David Miller]

From: Andy Fleming <afleming@freescale.com>
Date: Wed, 5 May 2010 10:18:40 -0500

> 
> On May 5, 2010, at 3:30 AM, Sebastian Andrzej Siewior wrote:
> 
>> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>> 
>> The size for skbs which is added to the recycled list is using the
>> current descriptor size which is current MTU. gfar_new_skb() is also
>> using this size. So after changing or alteast increasing the MTU all
>> recycled skbs should be dropped.
>> 
>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
 ...
> Acked-by: Andy Fleming <afleming@freescale.com>

Applied, thanks everyone.