diff mbox

net: smsc911x: fix RX FIFO fastforwarding when dropping packets

Message ID 20120412155409.GA28204@mudshark.cambridge.arm.com
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Will Deacon April 12, 2012, 3:54 p.m. UTC 
On Thu, Apr 12, 2012 at 03:01:39PM +0100, Eric Dumazet wrote:
> On Thu, 2012-04-12 at 14:47 +0100, Will Deacon wrote:
> 
> > 
> > I don't think we want an skb_reserve at all, since the hardware shifts the
> > data in the RX FIFO, meaning that we will read two bytes of 0 anyway before
> > valid data.
> > 
> > > 		skb_put(skb, pktlength - 4);
> > 
> > I can move the put here if you like, but we need to use pktwords << 2 to
> > make sure that we read the leading and trailing zeroes inserted by the
> > hardware.
> 
> before calling linux stack, you'll have to skip those 2 bytes.
> 
> This is skb_reserve() purpose.

Gotcha, so I can lose the pull too. Here's an updated patch with log, thanks
for the help.

Will


Author: Will Deacon <will.deacon@arm.com>
Date:   Thu Apr 12 13:54:17 2012 +0100

    net: smsc911x: fix skb handling in receive path
    
    The SMSC911x driver resets the ->head, ->data and ->tail pointers in the
    skb on the reset path in order to avoid buffer overflow due to packet
    padding performed by the hardware.
    
    This patch fixes the receive path so that the skb pointers are fixed up
    after the data has been read from the device, The error path is also
    fixed to use number of words consistently and prevent erroneous FIFO
    fastforwarding when skipping over bad data.
    
    Signed-off-by: Will Deacon <will.deacon@arm.com>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Eric Dumazet April 12, 2012, 4:08 p.m. UTC | #1 
On Thu, 2012-04-12 at 16:54 +0100, Will Deacon wrote:

> Gotcha, so I can lose the pull too. Here's an updated patch with log, thanks
> for the help.
> 
> Will
> 
> 
> Author: Will Deacon <will.deacon@arm.com>
> Date:   Thu Apr 12 13:54:17 2012 +0100
> 
>     net: smsc911x: fix skb handling in receive path
>     
>     The SMSC911x driver resets the ->head, ->data and ->tail pointers in the
>     skb on the reset path in order to avoid buffer overflow due to packet
>     padding performed by the hardware.
>     
>     This patch fixes the receive path so that the skb pointers are fixed up
>     after the data has been read from the device, The error path is also
>     fixed to use number of words consistently and prevent erroneous FIFO
>     fastforwarding when skipping over bad data.
>     
>     Signed-off-by: Will Deacon <will.deacon@arm.com>
> 
> diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
> index 4a69710..5aa2dbe 100644
> --- a/drivers/net/ethernet/smsc/smsc911x.c
> +++ b/drivers/net/ethernet/smsc/smsc911x.c
> @@ -1166,10 +1166,8 @@ smsc911x_rx_counterrors(struct net_device *dev, unsigned int rxstat)
>  
>  /* Quickly dumps bad packets */
>  static void
> -smsc911x_rx_fastforward(struct smsc911x_data *pdata, unsigned int pktbytes)
> +smsc911x_rx_fastforward(struct smsc911x_data *pdata, unsigned int pktwords)
>  {
> -	unsigned int pktwords = (pktbytes + NET_IP_ALIGN + 3) >> 2;
> -
>  	if (likely(pktwords >= 4)) {
>  		unsigned int timeout = 500;
>  		unsigned int val;
> @@ -1233,7 +1231,7 @@ static int smsc911x_poll(struct napi_struct *napi, int budget)
>  			continue;
>  		}
>  
> -		skb = netdev_alloc_skb(dev, pktlength + NET_IP_ALIGN);
> +		skb = netdev_alloc_skb(dev, pktwords << 2);
>  		if (unlikely(!skb)) {
>  			SMSC_WARN(pdata, rx_err,
>  				  "Unable to allocate skb for rx packet");
> @@ -1243,14 +1241,12 @@ static int smsc911x_poll(struct napi_struct *napi, int budget)
>  			break;
>  		}
>  
> -		skb->data = skb->head;
> -		skb_reset_tail_pointer(skb);
> +		pdata->ops->rx_readfifo(pdata,
> +				 (unsigned int *)skb->data, pktwords);
>  
>  		/* Align IP on 16B boundary */
>  		skb_reserve(skb, NET_IP_ALIGN);
>  		skb_put(skb, pktlength - 4);
> -		pdata->ops->rx_readfifo(pdata,
> -				 (unsigned int *)skb->head, pktwords);
>  		skb->protocol = eth_type_trans(skb, dev);
>  		skb_checksum_none_assert(skb);
>  		netif_receive_skb(skb);
> @@ -1565,7 +1561,7 @@ static int smsc911x_open(struct net_device *dev)
>  	smsc911x_reg_write(pdata, FIFO_INT, temp);
>  
>  	/* set RX Data offset to 2 bytes for alignment */
> -	smsc911x_reg_write(pdata, RX_CFG, (2 << 8));
> +	smsc911x_reg_write(pdata, RX_CFG, (NET_IP_ALIGN << 8));
>  
>  	/* enable NAPI polling before enabling RX interrupts */
>  	napi_enable(&pdata->napi);

Seems fine to me

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Will Deacon April 12, 2012, 5:03 p.m. UTC | #2 
On Thu, Apr 12, 2012 at 05:08:00PM +0100, Eric Dumazet wrote:
> On Thu, 2012-04-12 at 16:54 +0100, Will Deacon wrote:
> >     net: smsc911x: fix skb handling in receive path
> >     
> >     The SMSC911x driver resets the ->head, ->data and ->tail pointers in the
> >     skb on the reset path in order to avoid buffer overflow due to packet
> >     padding performed by the hardware.
> >     
> >     This patch fixes the receive path so that the skb pointers are fixed up
> >     after the data has been read from the device, The error path is also
> >     fixed to use number of words consistently and prevent erroneous FIFO
> >     fastforwarding when skipping over bad data.
> >     
> >     Signed-off-by: Will Deacon <will.deacon@arm.com>

[...]

> Seems fine to me
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Thanks, Eric. Given that Steve's email doesn't appear to be working, can
somebody else pick this up please?

Cheers,

Will
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet April 12, 2012, 5:41 p.m. UTC | #3 
On Thu, 2012-04-12 at 18:03 +0100, Will Deacon wrote:


> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> 
> Thanks, Eric. Given that Steve's email doesn't appear to be working, can
> somebody else pick this up please?

It seems trivial enough, you tested it, and I reviewed it.

I believe David can apply it safely.

Thanks


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller April 13, 2012, 6:08 p.m. UTC | #4 
From: Will Deacon <will.deacon@arm.com>
Date: Thu, 12 Apr 2012 18:03:50 +0100

> On Thu, Apr 12, 2012 at 05:08:00PM +0100, Eric Dumazet wrote:
>> On Thu, 2012-04-12 at 16:54 +0100, Will Deacon wrote:
>> >     net: smsc911x: fix skb handling in receive path
>> >     
>> >     The SMSC911x driver resets the ->head, ->data and ->tail pointers in the
>> >     skb on the reset path in order to avoid buffer overflow due to packet
>> >     padding performed by the hardware.
>> >     
>> >     This patch fixes the receive path so that the skb pointers are fixed up
>> >     after the data has been read from the device, The error path is also
>> >     fixed to use number of words consistently and prevent erroneous FIFO
>> >     fastforwarding when skipping over bad data.
>> >     
>> >     Signed-off-by: Will Deacon <will.deacon@arm.com>
> 
> [...]
> 
>> Seems fine to me
>> 
>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> 
> Thanks, Eric. Given that Steve's email doesn't appear to be working, can
> somebody else pick this up please?

Applied, thanks.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
index 4a69710..5aa2dbe 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -1166,10 +1166,8 @@  smsc911x_rx_counterrors(struct net_device *dev, unsigned int rxstat)
 
 /* Quickly dumps bad packets */
 static void
-smsc911x_rx_fastforward(struct smsc911x_data *pdata, unsigned int pktbytes)
+smsc911x_rx_fastforward(struct smsc911x_data *pdata, unsigned int pktwords)
 {
-	unsigned int pktwords = (pktbytes + NET_IP_ALIGN + 3) >> 2;
-
 	if (likely(pktwords >= 4)) {
 		unsigned int timeout = 500;
 		unsigned int val;
@@ -1233,7 +1231,7 @@  static int smsc911x_poll(struct napi_struct *napi, int budget)
 			continue;
 		}
 
-		skb = netdev_alloc_skb(dev, pktlength + NET_IP_ALIGN);
+		skb = netdev_alloc_skb(dev, pktwords << 2);
 		if (unlikely(!skb)) {
 			SMSC_WARN(pdata, rx_err,
 				  "Unable to allocate skb for rx packet");
@@ -1243,14 +1241,12 @@  static int smsc911x_poll(struct napi_struct *napi, int budget)
 			break;
 		}
 
-		skb->data = skb->head;
-		skb_reset_tail_pointer(skb);
+		pdata->ops->rx_readfifo(pdata,
+				 (unsigned int *)skb->data, pktwords);
 
 		/* Align IP on 16B boundary */
 		skb_reserve(skb, NET_IP_ALIGN);
 		skb_put(skb, pktlength - 4);
-		pdata->ops->rx_readfifo(pdata,
-				 (unsigned int *)skb->head, pktwords);
 		skb->protocol = eth_type_trans(skb, dev);
 		skb_checksum_none_assert(skb);
 		netif_receive_skb(skb);
@@ -1565,7 +1561,7 @@  static int smsc911x_open(struct net_device *dev)
 	smsc911x_reg_write(pdata, FIFO_INT, temp);
 
 	/* set RX Data offset to 2 bytes for alignment */
-	smsc911x_reg_write(pdata, RX_CFG, (2 << 8));
+	smsc911x_reg_write(pdata, RX_CFG, (NET_IP_ALIGN << 8));
 
 	/* enable NAPI polling before enabling RX interrupts */
 	napi_enable(&pdata->napi);