From patchwork Sat May 21 22:58:55 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Lino Sanfilippo X-Patchwork-Id: 9130673 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 59F1660762 for ; Sat, 21 May 2016 22:59:47 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 40A9628159 for ; Sat, 21 May 2016 22:59:47 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 3511E28160; Sat, 21 May 2016 22:59:47 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00,FREEMAIL_FROM, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 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.wl.linuxfoundation.org (Postfix) with ESMTPS id 9EF0728159 for ; Sat, 21 May 2016 22:59:46 +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 1b4Frx-0002gY-TH; Sat, 21 May 2016 22:59:41 +0000 Received: from mout.gmx.net ([212.227.15.15]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1b4Frv-0002fw-Lh for linux-rockchip@lists.infradead.org; Sat, 21 May 2016 22:59:40 +0000 Received: from [192.168.178.39] ([78.43.221.200]) by mail.gmx.com (mrgmx001) with ESMTPSA (Nemesis) id 0MhhwJ-1arpfk0lEp-00Mta5; Sun, 22 May 2016 00:58:59 +0200 Subject: Re: [PATCH v2] ethernet:arc: Fix racing of TX ring buffer To: Shuyu Wei , Francois Romieu References: <20160517152520.GA2750@debian-dorm> <20160517.142456.2247845107325931733.davem@davemloft.net> <20160518000153.GA21757@electric-eye.fr.zoreil.com> <573CD09D.1060307@gmx.de> <20160518225529.GA18671@electric-eye.fr.zoreil.com> <573E2D0C.604@gmx.de> <20160520003145.GA22420@electric-eye.fr.zoreil.com> <20160521160910.GA14945@debian-dorm> From: Lino Sanfilippo Message-ID: <5740E82F.8040903@gmx.de> Date: Sun, 22 May 2016 00:58:55 +0200 User-Agent: Mozilla/5.0 (X11; Linux i686; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-Version: 1.0 In-Reply-To: <20160521160910.GA14945@debian-dorm> X-Provags-ID: V03:K0:jz0zCRIurkft2t3JDwHK69bxZ5GQx5Ic3276v//WAsC9yRNJAdd 1iavnqBSukxjOC5vrMbLMiz6PJ4jgLrTf47gj7/DFRwzlnNRs3y9/Z9c7C41avRK4yJGGki I3g0QHDv98FLE8TCtAkzgNkMT9FdNS3Qnp8jHHuDIZ0A4b5Ef8FJhiKmqVIcDHXldslD6wZ 16a3GoDCG9bK1mqpHX2ZA== X-UI-Out-Filterresults: notjunk:1; V01:K0:c4nC59t3PY8=:qfwrnAyOD2V8gDLP6hAXNb gzL1bjFG+94ohQkkY0z580fym9EDJJnMBmsxT2XNr2ASjxps7VKD9qT16oFv+vcPDUwuBUUAj d7hAE3qEIO7rj8nJgjWD2FIpMHikLFvFhn1adVJsL+vLA2uW/VRk+hseU80nkwaiu3j3GPt7Y F4lFVVP6fiSfX5j2mAfbWW8WiloLSehFJG0LSPWo0z133D2aiy0gF8oLIPZWeCU0KzHXb3fFS 1WYi9/0yAkfryzQc3ZnnsV/5h1OvOUN1yRpHQlBAnP9Zs7NG6mJu296GOiUKwSeQ2+tSrpkuL N5bv6q5BhnkTSFGeqCq975H+e5c/V1nSzP3wRMP5cuNImXGm3IGVx8Wh0psPyi2gawC1UTR6W t+kXfiJiebR+fINf4AQ+5nY4qZpkdOk485aelopuR4O87iIAidobw6mcOYku+a0ApvepRIUD8 e8OdCzx0J51v3/4QKSQNdfloYib2K867CCNNDt8Yt03o6eQez4UKUgLRTrfPcUKgBNBDrB4Pd 7k51kavBPZ5Q789v8o9kIzTOcGVuvbV8JT7MjX5bjXMK6VejWETFZxEp6EcdFqKWT2r4Snfw0 zrF3s4PA8Ji4JdCdWfSlnb+SXV6KqINlMiMGTII5FSTNagNFm5SYgdI2U+WgyYXsk4XgFo6FT k6/yEo+mdugyNtAKguAmrWFxzJDPG4gAz0V0dw+/xwc2T+TCDugJih8+PxuXoPdNDZieT9WDX QsBbU8RkaMRSSjpaW6I72uY6agJUc4PYtiiqfL1CTQ3rxAxFYKrVuWcdrkSwsDFxa98WvOOQA PnFLva5 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20160521_155940_058271_FF78BC57 X-CRM114-Status: GOOD ( 17.33 ) 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: heiko@sntech.de, al.kochet@gmail.com, netdev@vger.kernel.org, linux-rockchip@lists.infradead.org, David Miller , wxt@rock-chips.com Sender: "Linux-rockchip" Errors-To: linux-rockchip-bounces+patchwork-linux-rockchip=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP On 21.05.2016 18:09, Shuyu Wei wrote: > Looks like I got it wrong in the first place. > > priv->tx_buff is not for the device, so there's no need to move it. > The race has been fixed by commit c278c253f3d9, I forgot to check > it out. That's my fault. > > I do find another problem. We need to use a barrier to make sure > skb_tx_timestamp() is called before setting the FOR_EMAC flag. > Shuyu, Could you please test the patch below? I implemented a new approach in which tx_clean uses txbd_curr to determine if there are more descriptors to check or if the loop can be left. Memory barriers on both sides (xmit and clean) should ensure that SKB and info are only accessed if they are valid. I also hope that skb_tx_timestamp is not an issue any more. BTW: concerning get_maintainer Alexander Kochetkov should be CCed for modifications in this area, so thats what I do hereby. --- a/drivers/net/ethernet/arc/emac_main.c +++ b/drivers/net/ethernet/arc/emac_main.c @@ -162,7 +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) + break; + + /* Make sure curr pointer is consistent with info */ + rmb(); + + if (*txbd_dirty == priv->txbd_curr) break; if (unlikely(info & (DROP | DEFR | LTCL | UFLO))) { @@ -195,8 +201,8 @@ static void arc_emac_tx_clean(struct net_device *ndev) *txbd_dirty = (*txbd_dirty + 1) % TX_BD_NUM; } - /* Ensure that txbd_dirty is visible to tx() before checking - * for queue stopped. + /* Ensure that txbd_dirty is visible to tx() and we see the most recent + * value for txbd_curr. */ smp_mb(); @@ -680,35 +686,29 @@ static int arc_emac_tx(struct sk_buff *skb, struct net_device *ndev) 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->tx_buff[*txbd_curr].skb = skb; skb_tx_timestamp(skb); *info = cpu_to_le32(FOR_EMAC | FIRST_OR_LAST_MASK | len); - /* Make sure info word is set */ + /* 1. Make sure that with respect to tx_clean everything is set up + * properly before we advance txbd_curr. + * 2. Make sure writes to DMA descriptors are completed before we inform + * the hardware. + */ 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 - * checking the queue status. This prevents an unneeded wake - * of the queue in tx_clean(). + /* Ensure we see the most recent value of txbd_dirty and tx_clean() sees + * the updated value of txbd_curr. */ smp_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); - } arc_reg_set(priv, R_STATUS, TXPL_MASK);