Re: Kernel Oops in UDP w/ ARM architecture

[Eric Dumazet <dada1@cosmosbay.com>]

Ron Yorgason a écrit :
> On Mon, Mar 9, 2009 at 3:24 PM, Eric Dumazet <dada1@cosmosbay.com> wrote:
>> Ron Yorgason a écrit :
>> - Show quoted text -
>>> On Mon, Mar 9, 2009 at 1:21 PM, Eric Dumazet <dada1@cosmosbay.com> wrote:
>>>> Please dont top post on this mailing list
>>>>
>>>> Ron Yorgason a écrit :
>>>>> We're using the fec driver, found in drivers/net/fec.c.  I modified
>>>>> this driver slightly to get the MAC address from the redboot
>>>>> configuration stored in flash memory, but it's otherwise untouched.  I
>>>>> can send my version of the file if that would help.
>>>>>
>>>>> --Ron
>>>>>
>>>>>
>>>> Given that ARM seems to be picky about non aligned accesses, you might
>>>> try this patch. This should force IP header to be aligned.
>>>>
>>>> diff -u linux-2.6.19/drivers/net/fec.c.old linux-2.6.19/drivers/net/fec.c
>>>> --- linux-2.6.19/drivers/net/fec.c.old
>>>> +++ linux-2.6.19/drivers/net/fec.c
>>>> @@ -641,13 +641,14 @@
>>>>         * include that when passing upstream as it messes up
>>>>         * bridging applications.
>>>>         */
>>>> -       skb = dev_alloc_skb(pkt_len-4);
>>>> +       skb = dev_alloc_skb((pkt_len - 4) + 2);
>>>>
>>>>        if (skb == NULL) {
>>>>                printk("%s: Memory squeeze, dropping packet.\n", dev->name);
>>>>                fep->stats.rx_dropped++;
>>>>        } else {
>>>>                skb->dev = dev;
>>>> +               skb_reserve(skb, 2); /* Align IP on 16 byte boundaries */
>>>>                skb_put(skb,pkt_len-4); /* Make room */
>>>>                eth_copy_and_sum(skb, data, pkt_len-4, 0);
>>>>                skb->protocol=eth_type_trans(skb,dev);
>>>>
>>>>
>>> Thanks for the patch.  It looks like Freescale modified this section
>>> of the code in our BSP, so I'm looking at how to best merge things in.
>>>  Also, since we're allocating an extra 2 bytes in dev_alloc_skb(), do
>>> we need to account for those bytes in skb_put() and
>>> eth_copy_and_sum(), or does the skb_reserve() call handle that?
>> We allocate 2 extra bytes, because skb_reserve() will advance skb->data pointer
>> by two bytes. Those two bytes plus 14 bytes ethernet header : total 16 bytes,
>> so that IP header is aligned on 16 byte boundaries.
>>
>> No other changes are necessary.
>>
>> Then, maybe this patch is not necessary at all on your platform, since crash happens
>> a *long* time after boot. It may be a shoot in the dark...
>>
>>
>>
> 
> The code as supplied by Freescale looks like this:
> 
> 	if ((pkt_len - 4) < fec_copy_threshold) {
> 		skb = dev_alloc_skb(pkt_len);
> 	} else {
> 		skb = dev_alloc_skb(FEC_ENET_RX_FRSIZE);
> 	}
> 
> 	if (skb == NULL) {
> 		printk("%s: Memory squeeze, dropping packet.\n", dev->name);
> 		fep->stats.rx_dropped++;
> 	} else {
> 		if ((pkt_len - 4) < fec_copy_threshold) {
> 			skb_reserve(skb, 2);    /*skip 2bytes, so ipheader is align 4bytes*/
> 			skb_put(skb,pkt_len-4); /* Make room */
> 			eth_copy_and_sum(skb, (unsigned char *)data,
> 					pkt_len-4, 0);
> 		} else {
> 			struct sk_buff *pskb = fep->rx_skbuff[rx_index];
> 			
> 			fep->rx_skbuff[rx_index] = skb;
> 			skb->data = FEC_ADDR_ALIGNMENT(skb->data);
...
> 			bdp->cbd_bufaddr = __pa(skb->data);
...
>                         skb_put(pskb,pkt_len-4);        /* Make room */
>                         skb = pskb;
>                 }
> 		skb->dev = dev;
> 		skb->protocol=eth_type_trans(skb,dev);
> 		netif_rx(skb);
> 	}
> 
> 
> fec_copy_threshold was defined with this comment:
> /*
>  *  fec_copy_threshold controls the copy when recieving ethernet frame.
>  *     If ethernet header aligns 4bytes, the ip header and upper
> header will not aligns 4bytes.
>  *     The resean is ethernet header is 14bytes.
>  *     And the max size of tcp & ip header is 128bytes. Normally it is 40bytes.
>  *     So I set the default value between 128 to 256.
>  */
> static int fec_copy_threshold = 192;
> 
> 
> And the FEC_ADDR_ALIGNMENT uses these definitions:
> 
> ...
> #elif defined(CONFIG_ARCH_MXC)
> #include <asm/arch/hardware.h>
> #include <asm/arch/iim.h>
> #include "fec.h"
> #define FEC_ALIGNMENT  (0x0F)          /*FEC needs 128bits(32bytes) alignment*/	
> #else
> ...
> 
> #define FEC_ADDR_ALIGNMENT(x) ((unsigned char *)(((unsigned long )(x)
> + (FEC_ALIGNMENT)) & (~FEC_ALIGNMENT)))
> 
> 
> It looks like for a packet smaller than 196 byes will get allocated
> with its actual size, but skb_reserve() skips 2 bytes and now it's a
> little short.  For a larger packet, they just allocate 2k, they're not
> skipping 2 bytes and they're aligning the data but not the ip headers.
>  However, everything works 99.999% of the time, so I assume there's
> just some border condition that's not being covered correctly.
> 
> 

This driver seems bugy, and not part of standard kernel tree.

It should not change skb->data without changing skb->tail.
Fortunatly, dev_alloc_skb() already provides an address with a 16bytes alignement
(unless patched by Freescale...)

I suggest you redefine fec_copy_threshold to 1600 to get back a working driver.

My patch is not necessary on this driver (skb_reserve() already called)