From patchwork Mon Dec 17 15:15:19 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Arvid Brodin X-Patchwork-Id: 206896 Return-Path: X-Original-To: incoming-imx@patchwork.ozlabs.org Delivered-To: patchwork-incoming-imx@bilbo.ozlabs.org Received: from merlin.infradead.org (merlin.infradead.org [IPv6:2001:4978:20e::2]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id A8B912C007B for ; Tue, 18 Dec 2012 02:19:19 +1100 (EST) Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1TkcPk-00004r-Sb; Mon, 17 Dec 2012 15:15:33 +0000 Received: from spam1.webland.se ([91.207.112.90]) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1TkcPe-0008Ua-B0 for linux-arm-kernel@lists.infradead.org; Mon, 17 Dec 2012 15:15:27 +0000 Message-ID: <50CF3707.9060502@xdin.com> Date: Mon, 17 Dec 2012 16:15:19 +0100 From: Arvid Brodin User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:8.0) Gecko/20120410 Thunderbird/8.0 MIME-Version: 1.0 To: Nicolas Ferre Subject: Re: Do I need to skb_put() Ethernet frames to a minimum of 60 bytes? References: <502A9EC4.4040208@xdin.com> <1344976557.2690.43.camel@bwh-desktop.uk.solarflarecom.com> <5033C6B0.4060508@xdin.com> <50CF216F.2010107@atmel.com> In-Reply-To: <50CF216F.2010107@atmel.com> Received-SPF: none X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20121217_101526_796250_21BD9A7E X-CRM114-Status: GOOD ( 23.27 ) X-Spam-Score: -1.9 (-) X-Spam-Report: SpamAssassin version 3.3.2 on merlin.infradead.org summary: Content analysis details: (-1.9 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.0 SPF_PASS SPF: sender matches SPF record -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] Cc: Ben Hutchings , "netdev@vger.kernel.org" , linux-arm-kernel , Eric Dumazet X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.14 Precedence: list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-arm-kernel-bounces@lists.infradead.org Errors-To: linux-arm-kernel-bounces+incoming-imx=patchwork.ozlabs.org@lists.infradead.org List-Id: linux-imx-kernel.lists.patchwork.ozlabs.org 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. */