[PATCH] ibm_newemac: Fixes entry of short packets

[PATCH] ibm_newemac: Fixes entry of short packets

[Stefan Roese]

From: Sathya Narayanan <sathyan@teamf1.com>

Short packets has to be discarded by the driver. So this patch addresses the
issue of discarding the short packets of size lesser then ethernet header
size.

Signed-off-by: Sathya Narayanan <sathyan@teamf1.com>
Signed-off-by: Stefan Roese <sr@denx.de>
---
 drivers/net/ibm_newemac/core.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ibm_newemac/core.c b/drivers/net/ibm_newemac/core.c
index 6dfc2c9..aa407b2 100644
--- a/drivers/net/ibm_newemac/core.c
+++ b/drivers/net/ibm_newemac/core.c
@@ -1652,6 +1652,13 @@ static int emac_poll_rx(void *param, int budget)
 
 		skb_put(skb, len);
 	push_packet:
+		if (skb->len < ETH_HLEN) {
+			dev_kfree_skb(skb);
+			printk(KERN_WARNING "%s: short packets dropped\n",
+			       dev->ndev->name);
+			++dev->estats.rx_dropped_stack;
+			goto next;
+		}
 		skb->dev = dev->ndev;
 		skb->protocol = eth_type_trans(skb, dev->ndev);
 		emac_rx_csum(dev, skb, ctrl);
-- 
1.5.6

Re: [PATCH] ibm_newemac: Fixes entry of short packets

[Benjamin Herrenschmidt]

On Mon, 2008-06-23 at 14:55 +0200, Stefan Roese wrote:
> From: Sathya Narayanan <sathyan@teamf1.com>
> 
> Short packets has to be discarded by the driver. So this patch addresses the
> issue of discarding the short packets of size lesser then ethernet header
> size.

You are freeing the skb, why ? Shouldn't we just keep the skb in the
ring for further rx ?

> Signed-off-by: Sathya Narayanan <sathyan@teamf1.com>
> Signed-off-by: Stefan Roese <sr@denx.de>
> ---
>  drivers/net/ibm_newemac/core.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/ibm_newemac/core.c b/drivers/net/ibm_newemac/core.c
> index 6dfc2c9..aa407b2 100644
> --- a/drivers/net/ibm_newemac/core.c
> +++ b/drivers/net/ibm_newemac/core.c
> @@ -1652,6 +1652,13 @@ static int emac_poll_rx(void *param, int budget)
>  
>  		skb_put(skb, len);
>  	push_packet:
> +		if (skb->len < ETH_HLEN) {
> +			dev_kfree_skb(skb);
> +			printk(KERN_WARNING "%s: short packets dropped\n",
> +			       dev->ndev->name);
> +			++dev->estats.rx_dropped_stack;
> +			goto next;
> +		}
>  		skb->dev = dev->ndev;
>  		skb->protocol = eth_type_trans(skb, dev->ndev);
>  		emac_rx_csum(dev, skb, ctrl);

Re: [PATCH] ibm_newemac: Fixes entry of short packets

[SathyaNarayanan]

[-- Attachment #1: Type: text/plain, Size: 1888 bytes --]

Hi benh,

            Please find my comments inline.

On Tue, Jun 24, 2008 at 4:50 AM, Benjamin Herrenschmidt <
benh@kernel.crashing.org> wrote:

> On Mon, 2008-06-23 at 14:55 +0200, Stefan Roese wrote:
> > From: Sathya Narayanan <sathyan@teamf1.com>
> >
> > Short packets has to be discarded by the driver. So this patch addresses
> the
> > issue of discarding the short packets of size lesser then ethernet header
> > size.
>
> You are freeing the skb, why ? Shouldn't we just keep the skb in the
> ring for further rx ?


Actually , short packets are not allowed to flow through the higher layers,
If any of the layer tried to use the extra room available may hit wit crash
.
Since it is a invalid packet it has to be dropped and freed in driver.
Actually if you see in code, the other invalid packets are also handelled
similar.


>
> > Signed-off-by: Sathya Narayanan <sathyan@teamf1.com>
> > Signed-off-by: Stefan Roese <sr@denx.de>
> > ---
> >  drivers/net/ibm_newemac/core.c |    7 +++++++
> >  1 files changed, 7 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/net/ibm_newemac/core.c
> b/drivers/net/ibm_newemac/core.c
> > index 6dfc2c9..aa407b2 100644
> > --- a/drivers/net/ibm_newemac/core.c
> > +++ b/drivers/net/ibm_newemac/core.c
> > @@ -1652,6 +1652,13 @@ static int emac_poll_rx(void *param, int budget)
> >
> >               skb_put(skb, len);
> >       push_packet:
> > +             if (skb->len < ETH_HLEN) {
> > +                     dev_kfree_skb(skb);
> > +                     printk(KERN_WARNING "%s: short packets dropped\n",
> > +                            dev->ndev->name);
> > +                     ++dev->estats.rx_dropped_stack;
> > +                     goto next;
> > +             }
> >               skb->dev = dev->ndev;
> >               skb->protocol = eth_type_trans(skb, dev->ndev);
> >               emac_rx_csum(dev, skb, ctrl);
>
>

[-- Attachment #2: Type: text/html, Size: 3259 bytes --]

Re: [PATCH] ibm_newemac: Fixes entry of short packets

[Benjamin Herrenschmidt]

>         
> 
> Actually , short packets are not allowed to flow through the higher
> layers, If any of the layer tried to use the extra room available may
> hit wit crash .
> Since it is a invalid packet it has to be dropped and freed in driver.
> Actually if you see in code, the other invalid packets are also
> handelled similar.

My point is they should not. The rx skb should be kept in the ring for
further rx (ie, the data ignored and re-use the skb). A bit like we do
when we decide the packet is small enough to be copied to a new skb.

Ie. Move you test above the threshold test and recycle the skb.

We need to fix the usage of the dma operations in this driver anyway
and this will make it easier as we'll be able to avoid re-mapping an
skb we just recycle. (the current driver never unmaps which means it
can't be used with an iommu, which is a problem on some cell based
platforms).

Ben.