[PATCH 2.6.19] AT91RM9200 Ethernet update 3

[PATCH 2.6.19] AT91RM9200 Ethernet update 3

[Andrew Victor]

A minor fix to the Atmel AT91RM9200 Ethernet driver.

1. Use dev_alloc_skb() instead of alloc_skb().
2. It is not necessary to adjust skb->len manually.


Signed-off-by: Andrew Victor <andrew@sanpeople.com>


diff -urN linux-2.6.19-final.orig/drivers/net/arm/at91_ether.c linux-2.6.19-final/drivers/net/arm/at91_ether.c
--- linux-2.6.19-final.orig/drivers/net/arm/at91_ether.c	Mon Dec  4 14:42:05 2006
+++ linux-2.6.19-final/drivers/net/arm/at91_ether.c	Mon Dec  4 14:43:57 2006
@@ -855,14 +855,13 @@
 	while (dlist->descriptors[lp->rxBuffIndex].addr & EMAC_DESC_DONE) {
 		p_recv = dlist->recv_buf[lp->rxBuffIndex];
 		pktlen = dlist->descriptors[lp->rxBuffIndex].size & 0x7ff;	/* Length of frame including FCS */
-		skb = alloc_skb(pktlen + 2, GFP_ATOMIC);
+		skb = dev_alloc_skb(pktlen + 2);
 		if (skb != NULL) {
 			skb_reserve(skb, 2);
 			memcpy(skb_put(skb, pktlen), p_recv, pktlen);
 
 			skb->dev = dev;
 			skb->protocol = eth_type_trans(skb, dev);
-			skb->len = pktlen;
 			dev->last_rx = jiffies;
 			lp->stats.rx_bytes += pktlen;
 			netif_rx(skb);

Re: [PATCH 2.6.19] AT91RM9200 Ethernet update 3

[Stephen Hemminger]

On 04 Dec 2006 14:50:24 +0200
Andrew Victor <andrew@sanpeople.com> wrote:

> A minor fix to the Atmel AT91RM9200 Ethernet driver.
> 
> 1. Use dev_alloc_skb() instead of alloc_skb().
> 2. It is not necessary to adjust skb->len manually.
> 
> 
> Signed-off-by: Andrew Victor <andrew@sanpeople.com>
> 
> 
> diff -urN linux-2.6.19-final.orig/drivers/net/arm/at91_ether.c linux-2.6.19-final/drivers/net/arm/at91_ether.c
> --- linux-2.6.19-final.orig/drivers/net/arm/at91_ether.c	Mon Dec  4 14:42:05 2006
> +++ linux-2.6.19-final/drivers/net/arm/at91_ether.c	Mon Dec  4 14:43:57 2006
> @@ -855,14 +855,13 @@
>  	while (dlist->descriptors[lp->rxBuffIndex].addr & EMAC_DESC_DONE) {
>  		p_recv = dlist->recv_buf[lp->rxBuffIndex];
>  		pktlen = dlist->descriptors[lp->rxBuffIndex].size & 0x7ff;	/* Length of frame including FCS */
> -		skb = alloc_skb(pktlen + 2, GFP_ATOMIC);
> +		skb = dev_alloc_skb(pktlen + 2);
>  		if (skb != NULL) {
>  			skb_reserve(skb, 2);
>  			memcpy(skb_put(skb, pktlen), p_recv, pktlen);
>  
>  			skb->dev = dev;
>  			skb->protocol = eth_type_trans(skb, dev);
> -			skb->len = pktlen;
>  			dev->last_rx = jiffies;
>  			lp->stats.rx_bytes += pktlen;
>  			netif_rx(skb);
>

Use netdev_alloc_skb instead. It sets skb->dev so you don't have to.
Setting skb->len is redundant since that is what skb_put() does.

It would be best if you didn't have to copy data at all and could
receive directly into the skb.

If you have to copy received data, it is better to implement NAPI since that
allows you to copy data in soft irq. The existing code will cause poor realtime
performance since the driver is copying received data with IRQ's disabled.

-- 
Stephen Hemminger <shemminger@osdl.org>

Re: [PATCH 2.6.19] AT91RM9200 Ethernet update 3

[Andrew Victor]

hi Stephen,

> Use netdev_alloc_skb instead. It sets skb->dev so you don't have to.

netdev_alloc_skb() seems to cause 2 function calls [__netdev_alloc_skb()
and __alloc_skb()], instead of just 1 for the dev_alloc_skb() case.
It might be more efficient if the driver manually sets skb->dev.


> Setting skb->len is redundant since that is what skb_put() does.

That's why that line is being removed.


> It would be best if you didn't have to copy data at all and could
> receive directly into the skb.

Probably, but then we'd need to worry about DMA and cache-aligned
skb's.  We'd also need to always allocate maximum-sized skb's;  and that
might be an issue since the AT91RM9200 is mainly used in embedded
devices where the amount of RAM can be limited.


> The existing code will cause poor realtime
> performance since the driver is copying received data with IRQ's disabled.

Only lower-priority interrupts should be disabled.


Regards,
  Andrew Victor

Re: [PATCH 2.6.19] AT91RM9200 Ethernet update 3

[Stephen Hemminger]

On 05 Dec 2006 09:29:52 +0200
Andrew Victor <andrew@sanpeople.com> wrote:

> hi Stephen,
> 
> > Use netdev_alloc_skb instead. It sets skb->dev so you don't have to.
> 
> netdev_alloc_skb() seems to cause 2 function calls [__netdev_alloc_skb()
> and __alloc_skb()], instead of just 1 for the dev_alloc_skb() case.
> It might be more efficient if the driver manually sets skb->dev.

That is trivial, and netdev_alloc_skb() is setup to handle other
issues in future.


-- 
Stephen Hemminger <shemminger@osdl.org>

Re: [PATCH 2.6.19] AT91RM9200 Ethernet update 3

[Jeff Garzik]

Andrew Victor wrote:
> A minor fix to the Atmel AT91RM9200 Ethernet driver.
> 
> 1. Use dev_alloc_skb() instead of alloc_skb().
> 2. It is not necessary to adjust skb->len manually.
> 
> 
> Signed-off-by: Andrew Victor <andrew@sanpeople.com>

ACK patch content