* Do I need to skb_put() Ethernet frames to a minimum of 60 bytes? @ 2012-08-14 18:53 Arvid Brodin 2012-08-14 20:35 ` Ben Hutchings 0 siblings, 1 reply; 7+ messages in thread From: Arvid Brodin @ 2012-08-14 18:53 UTC (permalink / raw) To: netdev@vger.kernel.org; +Cc: Nicolas Ferre Hi, If I create an sk_buff with a payload of less than 28 bytes (ethheader + data), and send it using the cadence/macb (Ethernet) driver, I get eth0: TX underrun, resetting buffers Now I know the minimum Ethernet frame size is 64 bytes (including the 4-byte FCS), but whose responsibility is it to pad the frame to this size if necessary? Mine or the driver's - i.e. should I just skb_put() to the minimum size or should I report the underrun as a driver bug? -- Arvid Brodin | Consultant (Linux) XDIN AB | Jan Stenbecks Torg 17 | SE-164 40 Kista | Sweden | xdin.com ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Do I need to skb_put() Ethernet frames to a minimum of 60 bytes? 2012-08-14 18:53 Do I need to skb_put() Ethernet frames to a minimum of 60 bytes? Arvid Brodin @ 2012-08-14 20:35 ` Ben Hutchings 2012-08-21 17:34 ` Arvid Brodin 0 siblings, 1 reply; 7+ messages in thread From: Ben Hutchings @ 2012-08-14 20:35 UTC (permalink / raw) To: Arvid Brodin; +Cc: netdev@vger.kernel.org, Nicolas Ferre On Tue, 2012-08-14 at 18:53 +0000, Arvid Brodin wrote: > Hi, > > If I create an sk_buff with a payload of less than 28 bytes (ethheader + data), > and send it using the cadence/macb (Ethernet) driver, I get > > eth0: TX underrun, resetting buffers > > Now I know the minimum Ethernet frame size is 64 bytes (including the 4-byte > FCS), but whose responsibility is it to pad the frame to this size if necessary? > Mine or the driver's - i.e. should I just skb_put() to the minimum size or > should I report the underrun as a driver bug? If the hardware doesn't pad frames automatically then it's the driver's reponsibility to do so. Ben. -- Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Do I need to skb_put() Ethernet frames to a minimum of 60 bytes? 2012-08-14 20:35 ` Ben Hutchings @ 2012-08-21 17:34 ` Arvid Brodin 2012-08-21 17:42 ` Eric Dumazet ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Arvid Brodin @ 2012-08-21 17:34 UTC (permalink / raw) To: Nicolas Ferre; +Cc: Ben Hutchings, netdev@vger.kernel.org On 2012-08-14 22:35, Ben Hutchings wrote: > On Tue, 2012-08-14 at 18:53 +0000, Arvid Brodin wrote: >> Hi, >> >> If I create an sk_buff with a payload of less than 28 bytes (ethheader + data), >> and send it using the cadence/macb (Ethernet) driver, I get >> >> eth0: TX underrun, resetting buffers >> >> Now I know the minimum Ethernet frame size is 64 bytes (including the 4-byte >> FCS), but whose responsibility is it to pad the frame to this size if necessary? >> Mine or the driver's - i.e. should I just skb_put() to the minimum size or >> should I report the underrun as a driver bug? > > If the hardware doesn't pad frames automatically then it's the driver's > reponsibility to do so. > Nicolas, can you take a look at this? At the moment I'm using the following change in macb.c to avoid TX underruns on short packages: --- a/drivers/net/ethernet/cadence/macb.c 2012-05-04 19:14:41.927719667 +0200 +++ b/drivers/net/ethernet/cadence/macb.c 2012-08-21 19:22:40.063739049 +0200 @@ -618,6 +618,7 @@ static void macb_poll_controller(struct } #endif +#define MIN_ETHFRAME_LEN 60 static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev) { struct macb *bp = netdev_priv(dev); @@ -635,6 +636,12 @@ static int macb_start_xmit(struct sk_buf printk("\n"); #endif + if (skb->len < MIN_ETHFRAME_LEN) { + /* Pad skb to minium Ethernet frame size */ + if (skb_tailroom(skb) >= MIN_ETHFRAME_LEN - skb->len) + memset(skb_put(skb, MIN_ETHFRAME_LEN - skb->len), 0, + MIN_ETHFRAME_LEN - skb->len); + } len = skb->len; spin_lock_irqsave(&bp->lock, flags); ... but as you can see this is limited to linear skbs which has been allocated with enough tailroom. Perhaps there are better ways to fix the problem? (Maybe the hardware is actually doing the padding already and the problem has to do with the way the DMA transfer is set up?) -- Arvid Brodin | Consultant (Linux) XDIN AB | Jan Stenbecks Torg 17 | SE-164 40 Kista | Sweden | xdin.com ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Do I need to skb_put() Ethernet frames to a minimum of 60 bytes? 2012-08-21 17:34 ` Arvid Brodin @ 2012-08-21 17:42 ` Eric Dumazet 2012-08-21 18:07 ` Ben Hutchings 2012-12-17 13:43 ` Nicolas Ferre 2 siblings, 0 replies; 7+ messages in thread From: Eric Dumazet @ 2012-08-21 17:42 UTC (permalink / raw) To: Arvid Brodin; +Cc: Nicolas Ferre, Ben Hutchings, netdev@vger.kernel.org On Tue, 2012-08-21 at 17:34 +0000, Arvid Brodin wrote: > Nicolas, can you take a look at this? At the moment I'm using the following change > in macb.c to avoid TX underruns on short packages: > > --- a/drivers/net/ethernet/cadence/macb.c 2012-05-04 19:14:41.927719667 +0200 > +++ b/drivers/net/ethernet/cadence/macb.c 2012-08-21 19:22:40.063739049 +0200 > @@ -618,6 +618,7 @@ static void macb_poll_controller(struct > } > #endif > > +#define MIN_ETHFRAME_LEN 60 > static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev) > { > struct macb *bp = netdev_priv(dev); > @@ -635,6 +636,12 @@ static int macb_start_xmit(struct sk_buf > printk("\n"); > #endif > > + if (skb->len < MIN_ETHFRAME_LEN) { > + /* Pad skb to minium Ethernet frame size */ > + if (skb_tailroom(skb) >= MIN_ETHFRAME_LEN - skb->len) > + memset(skb_put(skb, MIN_ETHFRAME_LEN - skb->len), 0, > + MIN_ETHFRAME_LEN - skb->len); > + } > len = skb->len; > spin_lock_irqsave(&bp->lock, flags); > > > ... but as you can see this is limited to linear skbs which has been allocated with > enough tailroom. Perhaps there are better ways to fix the problem? (Maybe the hardware > is actually doing the padding already and the problem has to do with the way the DMA > transfer is set up?) > other net drivers use skb_padto() for this ... ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Do I need to skb_put() Ethernet frames to a minimum of 60 bytes? 2012-08-21 17:34 ` Arvid Brodin 2012-08-21 17:42 ` Eric Dumazet @ 2012-08-21 18:07 ` Ben Hutchings 2012-12-17 13:43 ` Nicolas Ferre 2 siblings, 0 replies; 7+ messages in thread From: Ben Hutchings @ 2012-08-21 18:07 UTC (permalink / raw) To: Arvid Brodin; +Cc: Nicolas Ferre, netdev@vger.kernel.org On Tue, 2012-08-21 at 17:34 +0000, Arvid Brodin wrote: > On 2012-08-14 22:35, Ben Hutchings wrote: > > On Tue, 2012-08-14 at 18:53 +0000, Arvid Brodin wrote: > >> Hi, > >> > >> If I create an sk_buff with a payload of less than 28 bytes (ethheader + data), > >> and send it using the cadence/macb (Ethernet) driver, I get > >> > >> eth0: TX underrun, resetting buffers > >> > >> Now I know the minimum Ethernet frame size is 64 bytes (including the 4-byte > >> FCS), but whose responsibility is it to pad the frame to this size if necessary? > >> Mine or the driver's - i.e. should I just skb_put() to the minimum size or > >> should I report the underrun as a driver bug? > > > > If the hardware doesn't pad frames automatically then it's the driver's > > reponsibility to do so. > > > > Nicolas, can you take a look at this? At the moment I'm using the following change > in macb.c to avoid TX underruns on short packages: > > --- a/drivers/net/ethernet/cadence/macb.c 2012-05-04 19:14:41.927719667 +0200 > +++ b/drivers/net/ethernet/cadence/macb.c 2012-08-21 19:22:40.063739049 +0200 > @@ -618,6 +618,7 @@ static void macb_poll_controller(struct > } > #endif > > +#define MIN_ETHFRAME_LEN 60 <linux/etherdevice.h> already names this as ETH_ZLEN, by the way. > static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev) > { > struct macb *bp = netdev_priv(dev); > @@ -635,6 +636,12 @@ static int macb_start_xmit(struct sk_buf > printk("\n"); > #endif > > + if (skb->len < MIN_ETHFRAME_LEN) { > + /* Pad skb to minium Ethernet frame size */ > + if (skb_tailroom(skb) >= MIN_ETHFRAME_LEN - skb->len) > + memset(skb_put(skb, MIN_ETHFRAME_LEN - skb->len), 0, > + MIN_ETHFRAME_LEN - skb->len); > + } > len = skb->len; > spin_lock_irqsave(&bp->lock, flags); > > > ... but as you can see this is limited to linear skbs which has been allocated with > enough tailroom. Perhaps there are better ways to fix the problem? skb_padto() should be all you need. Note that it frees the skb on failure, so you must just return NETDEV_TX_OK then. Ben. > (Maybe the hardware > is actually doing the padding already and the problem has to do with the way the DMA > transfer is set up?) -- Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Do I need to skb_put() Ethernet frames to a minimum of 60 bytes? 2012-08-21 17:34 ` Arvid Brodin 2012-08-21 17:42 ` Eric Dumazet 2012-08-21 18:07 ` Ben Hutchings @ 2012-12-17 13:43 ` Nicolas Ferre 2012-12-17 15:15 ` Arvid Brodin 2 siblings, 1 reply; 7+ messages in thread From: Nicolas Ferre @ 2012-12-17 13:43 UTC (permalink / raw) To: Arvid Brodin Cc: Ben Hutchings, netdev@vger.kernel.org, Eric Dumazet, linux-arm-kernel On 08/21/2012 07:34 PM, Arvid Brodin : > On 2012-08-14 22:35, Ben Hutchings wrote: >> On Tue, 2012-08-14 at 18:53 +0000, Arvid Brodin wrote: >>> Hi, >>> >>> If I create an sk_buff with a payload of less than 28 bytes (ethheader + data), >>> and send it using the cadence/macb (Ethernet) driver, I get >>> >>> eth0: TX underrun, resetting buffers >>> >>> Now I know the minimum Ethernet frame size is 64 bytes (including the 4-byte >>> FCS), but whose responsibility is it to pad the frame to this size if necessary? >>> Mine or the driver's - i.e. should I just skb_put() to the minimum size or >>> should I report the underrun as a driver bug? >> >> If the hardware doesn't pad frames automatically then it's the driver's >> reponsibility to do so. >> > > Nicolas, can you take a look at this? At the moment I'm using the following change > in macb.c to avoid TX underruns on short packages: > > --- a/drivers/net/ethernet/cadence/macb.c 2012-05-04 19:14:41.927719667 +0200 > +++ b/drivers/net/ethernet/cadence/macb.c 2012-08-21 19:22:40.063739049 +0200 > @@ -618,6 +618,7 @@ static void macb_poll_controller(struct > } > #endif > > +#define MIN_ETHFRAME_LEN 60 > static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev) > { > struct macb *bp = netdev_priv(dev); > @@ -635,6 +636,12 @@ static int macb_start_xmit(struct sk_buf > printk("\n"); > #endif > > + if (skb->len < MIN_ETHFRAME_LEN) { > + /* Pad skb to minium Ethernet frame size */ > + if (skb_tailroom(skb) >= MIN_ETHFRAME_LEN - skb->len) > + memset(skb_put(skb, MIN_ETHFRAME_LEN - skb->len), 0, > + MIN_ETHFRAME_LEN - skb->len); > + } > len = skb->len; > spin_lock_irqsave(&bp->lock, flags); > > > ... but as you can see this is limited to linear skbs which has been allocated with > enough tailroom. Perhaps there are better ways to fix the problem? (Maybe the hardware > is actually doing the padding already and the problem has to do with the way the DMA > transfer is set up?) I come back to this issue. It seems to me that the macb Cadence IP is padding automatically a too little packet. It is the usual behavior unless you specify otherwise in the CTRL register embedded in the tx descriptor. I also verified this with wireshark on both ICMP and UDP packets. The error that you are experiencing is on at91sam9260 or at91sam9263 SoCs, am I right? Best regards, -- Nicolas Ferre ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Do I need to skb_put() Ethernet frames to a minimum of 60 bytes? 2012-12-17 13:43 ` Nicolas Ferre @ 2012-12-17 15:15 ` Arvid Brodin 0 siblings, 0 replies; 7+ messages in thread From: Arvid Brodin @ 2012-12-17 15:15 UTC (permalink / raw) To: Nicolas Ferre Cc: Ben Hutchings, netdev@vger.kernel.org, Eric Dumazet, linux-arm-kernel On 2012-12-17 14:43, Nicolas Ferre wrote: > On 08/21/2012 07:34 PM, Arvid Brodin : >> On 2012-08-14 22:35, Ben Hutchings wrote: >>> On Tue, 2012-08-14 at 18:53 +0000, Arvid Brodin wrote: >>>> Hi, >>>> >>>> If I create an sk_buff with a payload of less than 28 bytes (ethheader + data), >>>> and send it using the cadence/macb (Ethernet) driver, I get >>>> >>>> eth0: TX underrun, resetting buffers >>>> >>>> Now I know the minimum Ethernet frame size is 64 bytes (including the 4-byte >>>> FCS), but whose responsibility is it to pad the frame to this size if necessary? >>>> Mine or the driver's - i.e. should I just skb_put() to the minimum size or >>>> should I report the underrun as a driver bug? >>> >>> If the hardware doesn't pad frames automatically then it's the driver's >>> reponsibility to do so. >>> >> >> Nicolas, can you take a look at this? At the moment I'm using the following change >> in macb.c to avoid TX underruns on short packages: >> >> --- a/drivers/net/ethernet/cadence/macb.c 2012-05-04 19:14:41.927719667 +0200 >> +++ b/drivers/net/ethernet/cadence/macb.c 2012-08-21 19:22:40.063739049 +0200 >> @@ -618,6 +618,7 @@ static void macb_poll_controller(struct >> } >> #endif >> >> +#define MIN_ETHFRAME_LEN 60 >> static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev) >> { >> struct macb *bp = netdev_priv(dev); >> @@ -635,6 +636,12 @@ static int macb_start_xmit(struct sk_buf >> printk("\n"); >> #endif >> >> + if (skb->len < MIN_ETHFRAME_LEN) { >> + /* Pad skb to minium Ethernet frame size */ >> + if (skb_tailroom(skb) >= MIN_ETHFRAME_LEN - skb->len) >> + memset(skb_put(skb, MIN_ETHFRAME_LEN - skb->len), 0, >> + MIN_ETHFRAME_LEN - skb->len); >> + } >> len = skb->len; >> spin_lock_irqsave(&bp->lock, flags); >> >> >> ... but as you can see this is limited to linear skbs which has been allocated with >> enough tailroom. Perhaps there are better ways to fix the problem? (Maybe the hardware >> is actually doing the padding already and the problem has to do with the way the DMA >> transfer is set up?) > > I come back to this issue. It seems to me that the macb Cadence IP is > padding automatically a too little packet. It is the usual behavior > unless you specify otherwise in the CTRL register embedded in the tx > descriptor. I also verified this with wireshark on both ICMP and UDP > packets. > > The error that you are experiencing is on at91sam9260 or at91sam9263 > SoCs, am I right? No, this was on an AVR32 AP7000 board. I believe this is what I did to solve the issue (patch for linux-2.6.37): diff -Nurp linux-2.6.37-001-bsa400/drivers/net//macb.c linux-2.6.37-macb-hsr/drivers/net//macb.c --- linux-2.6.37-orig/drivers/net//macb.c 2012-09-16 22:41:02.746754672 +0200 +++ linux-2.6.37-macb/drivers/net//macb.c 2012-09-17 00:34:35.161389720 +0200 @@ -376,8 +379,9 @@ static void macb_tx(struct macb *bp) rmb(); - dma_unmap_single(&bp->pdev->dev, rp->mapping, skb->len, - DMA_TO_DEVICE); + dma_unmap_single(&bp->pdev->dev, rp->mapping, + max(skb->len, (unsigned int) ETH_ZLEN), + DMA_TO_DEVICE); rp->skb = NULL; dev_kfree_skb_irq(skb); } @@ -413,7 +417,8 @@ static void macb_tx(struct macb *bp) dev_dbg(&bp->pdev->dev, "skb %u (data %p) TX complete\n", tail, skb->data); - dma_unmap_single(&bp->pdev->dev, rp->mapping, skb->len, + dma_unmap_single(&bp->pdev->dev, rp->mapping, + max(skb->len, (unsigned int) ETH_ZLEN), DMA_TO_DEVICE); bp->stats.tx_packets++; bp->stats.tx_bytes += skb->len; @@ -675,7 +680,10 @@ static int macb_start_xmit(struct sk_buf printk("\n"); #endif - len = skb->len; + if (skb_padto(skb, ETH_ZLEN) != 0) + return NETDEV_TX_OK; /* There is no NETDEV_TX_FAIL... */ + + len = max(skb->len, (unsigned int) ETH_ZLEN); spin_lock_irqsave(&bp->lock, flags); /* This is a hard error, log it. */ -- Arvid Brodin | Consultant (Linux) XDIN AB | Knarrarnäsgatan 7 | SE-164 40 Kista | Sweden | xdin.com ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-12-17 15:15 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-08-14 18:53 Do I need to skb_put() Ethernet frames to a minimum of 60 bytes? Arvid Brodin 2012-08-14 20:35 ` Ben Hutchings 2012-08-21 17:34 ` Arvid Brodin 2012-08-21 17:42 ` Eric Dumazet 2012-08-21 18:07 ` Ben Hutchings 2012-12-17 13:43 ` Nicolas Ferre 2012-12-17 15:15 ` Arvid Brodin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).