| Message ID | 49B55E45.3040902@cosmosbay.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
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? Ron -- 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
Ron Yorgason a écrit : > 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... -- 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
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. -- 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
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) -- 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 -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);