From patchwork Wed May 18 20:29:17 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Lino Sanfilippo X-Patchwork-Id: 9122061 Return-Path: X-Original-To: patchwork-linux-rockchip@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id C78A3BF29F for ; Wed, 18 May 2016 20:30:16 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id E51512035D for ; Wed, 18 May 2016 20:30:15 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 23D332034E for ; Wed, 18 May 2016 20:30:13 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1b386c-0004t3-UE; Wed, 18 May 2016 20:30:10 +0000 Received: from mout.gmx.net ([212.227.15.19]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1b386a-0003dy-DN for linux-rockchip@lists.infradead.org; Wed, 18 May 2016 20:30:09 +0000 Received: from [192.168.178.39] ([78.43.221.200]) by mail.gmx.com (mrgmx003) with ESMTPSA (Nemesis) id 0MAgzj-1at08p03wJ-00BtHL; Wed, 18 May 2016 22:29:20 +0200 Subject: Re: [PATCH v2] ethernet:arc: Fix racing of TX ring buffer To: Francois Romieu , David Miller References: <20160517152520.GA2750@debian-dorm> <20160517.142456.2247845107325931733.davem@davemloft.net> <20160518000153.GA21757@electric-eye.fr.zoreil.com> From: Lino Sanfilippo Message-ID: <573CD09D.1060307@gmx.de> Date: Wed, 18 May 2016 22:29:17 +0200 User-Agent: Mozilla/5.0 (X11; Linux i686; rv:38.0) Gecko/20100101 Thunderbird/38.7.2 MIME-Version: 1.0 In-Reply-To: <20160518000153.GA21757@electric-eye.fr.zoreil.com> X-Provags-ID: V03:K0:PB/kWczU6et2QcuzcX03rcIWahlv3dUivPAc87Lo7dcIs/1uHbZ ThhORZJfJ205H0JjuqD80nwF+1zQjg3SnW6t70TPTTeUHgE6YbvpSU9qhyWMmPECIHlmWnu Znjcj1+qSfDT++t/hszWDw7BXgOrM+4YnkrbpmoX9H8vEzmdpadYF04tR7kjiVjp1oI4edN MNBrVvbB8gQD4YGt/8uKw== X-UI-Out-Filterresults: notjunk:1; V01:K0:BXNzHEcGwmQ=:oJetTEnqR/4+LVc6iJWDnW pBpeY+qHZSetLiyKR/Jh3njziq9FKb3eBZhyJyJtZPmj4UV7vwwQnnWpg+o9d+hfunX3oTBa+ Xfu+UHQ+KFJm/h7irJ3wBTlX1l/0xK0akVJvsyVFBWr+GXWnY4DgJE47mifOafYQIkhZ8NZxQ vosCvxyZqNkuMwkDc/qezLu4mfZuLJvEBOdtwYvu9oVq4rduj5HSLXLh/iXmqweDNCe9tE72Q JOQ9fY6t2PKbke0Se6uASnkDp/UpwG/F+aPqEzLxCUSTLPx8gg2qWtZ8QEIekq45g1pSVTp58 /DfmxWyQJN9nvhffEwDQip9+7sd2fV0qwgaZ8wYz4QZEL0lix9iAZyJzXXv+ZJsecHOtvCU3R /hx6avLia4Qjk8dUb8sz4Bc+t6nch5X15uQW1lB3l36HfzD/OaL74bJtrKp2nbSa82Cn60Cui 4GUjen0H9GC36eLvaRtsTZeAfwrALffLbIk/3REVx3sbzTOsDG6C4oY0iQlGDNtpz3cYgo4jp w8NHwPXsOUd5XcUUsLJPfbKDXqpphSjhDBr4QyiMwAdV7zXV/u1lcijHn58OJOLp+Fy/aTq8K MhGYZJP8zjm1gqYKs7p1XYCoK7c74yLQMfqplP00OE5nWN//QpmjlV6rcV+ySL+63R1cbK+Kp huTLW4bx9qDFa19LFobOXwBtlXKfYmRHeyRo1kvW35WIwBNAYY61KhDTCUfjKvV1bvp33vCpu eYc/XpdqliDgeeofsNwzgbW5DsR5PoMkBTtuFuZiRIGkTKF0geCLp9dPoLm/xo1QlSTIWE3kH ofenLlG X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20160518_133008_816686_FE535074 X-CRM114-Status: GOOD ( 18.43 ) X-Spam-Score: -2.6 (--) X-BeenThere: linux-rockchip@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: Upstream kernel work for Rockchip platforms List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-rockchip@lists.infradead.org, netdev@vger.kernel.org, wsy2220@gmail.com, heiko@sntech.de, wxt@rock-chips.com Sender: "Linux-rockchip" Errors-To: linux-rockchip-bounces+patchwork-linux-rockchip=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-5.6 required=5.0 tests=BAYES_00,FREEMAIL_FROM, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 18.05.2016 02:01, Francois Romieu wrote: > The smp_wmb() and wmb() could be made side-by-side once *info is > updated but I don't see the adequate idiom to improve the smp_wmb + wmb > combo. :o/ > >> And the wmb() looks like it should be a dma_wmb(). > > I see two points against it: > - it could be too late for skb_tx_timestamp(). > - arc_emac_tx_clean must not see an index update before the device > got a chance to acquire the descriptor. arc_emac_tx_clean can't > tell the difference between an about-to-be-released descriptor > and a returned-from-device one. > Hi, what about the (only compile tested) code below? The smp_wmb() in tx function combined with the smp_rmb() in tx_clean ensures that the CPU running tx_clean sees consistent values for info, data and skb (thus no need to check for validity of all three values any more). The mb() fulfills several tasks: 1. makes sure that DMA writes to descriptor are completed before the HW is informed. 2. On multi processor systems: ensures that txbd_curr is updated (this is paired with the smp_mb() at the end of tx_clean). 3. Ensure we see the most recent value for tx_dirty. With this we do not have to recheck after we stopped the tx queue. --- a/drivers/net/ethernet/arc/emac_main.c +++ b/drivers/net/ethernet/arc/emac_main.c @@ -162,8 +162,13 @@ static void arc_emac_tx_clean(struct net_device *ndev) struct sk_buff *skb = tx_buff->skb; unsigned int info = le32_to_cpu(txbd->info); - if ((info & FOR_EMAC) || !txbd->data || !skb) + if (info & FOR_EMAC) { + /* Make sure we see consistent values for info, skb + * and data. + */ + smp_rmb(); break; + } if (unlikely(info & (DROP | DEFR | LTCL | UFLO))) { stats->tx_errors++; @@ -679,36 +684,33 @@ static int arc_emac_tx(struct sk_buff *skb, struct net_device *ndev) dma_unmap_addr_set(&priv->tx_buff[*txbd_curr], addr, addr); dma_unmap_len_set(&priv->tx_buff[*txbd_curr], len, len); - priv->txbd[*txbd_curr].data = cpu_to_le32(addr); - /* Make sure pointer to data buffer is set */ - wmb(); + priv->txbd[*txbd_curr].data = cpu_to_le32(addr); + priv->tx_buff[*txbd_curr].skb = skb; - skb_tx_timestamp(skb); + /* Make sure info is set after data and skb with respect to + * other tx_clean(). + */ + smp_wmb(); *info = cpu_to_le32(FOR_EMAC | FIRST_OR_LAST_MASK | len); - /* Make sure info word is set */ - wmb(); - - priv->tx_buff[*txbd_curr].skb = skb; - /* Increment index to point to the next BD */ *txbd_curr = (*txbd_curr + 1) % TX_BD_NUM; - /* Ensure that tx_clean() sees the new txbd_curr before + /* 1.Ensure that tx_clean() sees the new txbd_curr before * checking the queue status. This prevents an unneeded wake * of the queue in tx_clean(). + * 2.Ensure that all values are written to RAM and to DMA + * before hardware is informed. + * 3.Ensure we see the most recent value for tx_dirty. */ - smp_mb(); + mb(); - if (!arc_emac_tx_avail(priv)) { + if (!arc_emac_tx_avail(priv)) netif_stop_queue(ndev); - /* Refresh tx_dirty */ - smp_mb(); - if (arc_emac_tx_avail(priv)) - netif_start_queue(ndev); - } + + skb_tx_timestamp(skb); arc_reg_set(priv, R_STATUS, TXPL_MASK);