Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757477AbcDGTqx (ORCPT ); Thu, 7 Apr 2016 15:46:53 -0400 Received: from mail-lb0-f171.google.com ([209.85.217.171]:36216 "EHLO mail-lb0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757256AbcDGTqv (ORCPT ); Thu, 7 Apr 2016 15:46:51 -0400 Subject: Re: [PATCH V2 6/8] net: mediatek: fix TX locking To: John Crispin , "David S. Miller" References: <1460057210-55786-1-git-send-email-blogic@openwrt.org> <1460057210-55786-7-git-send-email-blogic@openwrt.org> Cc: Felix Fietkau , Matthias Brugger , =?UTF-8?B?U2VhbiBXYW5nICjnjovlv5fkupgp?= , netdev@vger.kernel.org, linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org From: Sergei Shtylyov Organization: Cogent Embedded Message-ID: <5706B927.8090309@cogentembedded.com> Date: Thu, 7 Apr 2016 22:46:47 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <1460057210-55786-7-git-send-email-blogic@openwrt.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1785 Lines: 50 Hello. On 04/07/2016 10:26 PM, John Crispin wrote: > Inside the TX path there is a lock inside the tx_map function. This is > however too late. The patch moves the lock to the start of the xmit > function right before the free count check of the DMA ring happens. > If we do not do this, the code becomes racy leading to TX stalls and > dropped packets. This happens as there are 2 netdevs running on the > same physical DMA ring. > > Signed-off-by: John Crispin > --- > drivers/net/ethernet/mediatek/mtk_eth_soc.c | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c > index 60b66ab..8434355 100644 > --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c > +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c [...] > @@ -712,14 +702,22 @@ static int mtk_start_xmit(struct sk_buff *skb, struct net_device *dev) > struct mtk_eth *eth = mac->hw; > struct mtk_tx_ring *ring = ð->tx_ring; > struct net_device_stats *stats = &dev->stats; > + unsigned long flags; > bool gso = false; > int tx_num; > > + /* normally we can rely on the stack not calling this more than once, > + * however we have 2 queues running ont he same ring so we need to lock s/ont he/ on the/, perhaps a good chance to fix the comment? > + * the ring access > + */ > + spin_lock_irqsave(ð->page_lock, flags); > + > tx_num = mtk_cal_txd_req(skb); > if (unlikely(atomic_read(&ring->free_count) <= tx_num)) { > mtk_stop_queue(eth); > netif_err(eth, tx_queued, dev, > "Tx Ring full when queue awake!\n"); > + spin_unlock_irqrestore(ð->page_lock, flags); > return NETDEV_TX_BUSY; > } > [...] MBR, Sergei