Message ID | 5033C6B0.4060508@xdin.com |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
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 ... -- 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 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?)
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,
--- 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);