From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 49EEDC433EF for ; Fri, 18 Feb 2022 10:24:47 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:CC:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=ZaiQxI5RCmX8uEfRUSq80LDomB2+4YpDrNtK7Hpm64Y=; b=2iKIhro1MjjvPY FM0pZnPo6pkxN4Xxk0ioSecEpqsVgbKc13CCZeh8eWRDHQlNceql9uTWwbVnnW5x4/9EPdQ+8Gj85 EUWYDSzY9umO7hhFL3mwkiGdmzZPCICFsgFVxio5rtN9tANbRz1/nkvTYv2JUxmiO2GyFsRB4vaWV PRyntCQQnBMCSH8jl3ppii300/hpE93SSQcvYxfE8xmiv+26YILzw0JM3a5om5oe3bYIAYxFcXr0H rRDw/2A7EkBQ8roEB0mnJ2sl74k866w17Uoc44ibo2ccIqdarBbuYOGBSbetlnY6dNm4pGiv/L0x5 IgwATTmATvix5uLdonpg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nL0Pz-00Dq4j-En; Fri, 18 Feb 2022 10:23:15 +0000 Received: from smtp1.axis.com ([195.60.68.17]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nL0Pq-00Dq2f-U5 for linux-arm-kernel@lists.infradead.org; Fri, 18 Feb 2022 10:23:09 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=axis.com; q=dns/txt; s=axis-central1; t=1645179787; x=1676715787; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=0t0ig3q6Sp2mvoezO0JaagGhH+DGXuT456KDBH79FZ0=; b=pHI6jFzvxoi0j3ne7wO34lrazDpzu6REhaVP5x9nJ/IpQKNqL3FS4GKH rf/wpDyUIngYpdO87FBPTLInPxtDaQO/tCBvjvr7L0/JxybMwot6yZqJa KmhKeD3ttWqwp5B0BSf/ip0MKs8h1l1yU6/Z/SZlngGwHVjwmQ+SrYJd6 uKfIZ9wdMECVwEmXyL3lBipUF6zMQC1KZdm1o7h/RBynLF5iH3HFP+dlS UZiUFrZ+MPLmYGI+x3DEqLhBf3DyQVoPZ3b3Bwe6U17rxSi624TbfVLW+ uE5Ca0jqMRc1VuoWPj4MMZLfpLkFvuKJGsMlJiSEJG0Fti4ytTI6fBfC3 g==; Date: Fri, 18 Feb 2022 11:23:03 +0100 From: Vincent Whitchurch To: Jakub Kicinski CC: Giuseppe Cavallaro , Alexandre Torgue , Jose Abreu , "David S. Miller" , Maxime Coquelin , kernel , Lars Persson , Srinivas Kandagatla , "netdev@vger.kernel.org" , "linux-stm32@st-md-mailman.stormreply.com" , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH] net: stmmac: Enable NAPI before interrupts go live Message-ID: <20220218102303.GA21458@axis.com> References: <20220217145527.2696444-1-vincent.whitchurch@axis.com> <20220217203604.39e318d0@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20220217203604.39e318d0@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> User-Agent: Mutt/1.10.1 (2018-07-13) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220218_022307_662485_3A1E42C5 X-CRM114-Status: GOOD ( 27.60 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Fri, Feb 18, 2022 at 05:36:04AM +0100, Jakub Kicinski wrote: > On Thu, 17 Feb 2022 15:55:26 +0100 Vincent Whitchurch wrote: > > The stmmac_open function has a race window between enabling the RX > > path and its interrupt to the point where napi_enabled is called. > > > > A chatty network with plenty of broadcast/multicast traffic has the > > potential to completely fill the RX ring before the interrupt handler > > is installed. In this scenario the single interrupt taken will find > > napi disabled and the RX ring will not be processed. No further RX > > interrupt will be delivered because the ring is full. > > > > The RX stall could eventually clear because the TX path will trigger a > > DMA interrupt once the tx_coal_frames threshold is reached and then > > NAPI becomes scheduled. > > LGTM, although now the ndo_open and ndo_stop paths are not symmetrical. > Is there no way to mask the IRQs so that they don't fire immediately? The initial enabling of the DMA irqs is done as part of the ->init_chan() callback inside the various variants. We could use the ->disable_dma_irq() callback to to disable the DMA irqs and only enable them at the end of the init sequence with a stmmac_enable_all_dma_irq(). This avoids having to change all the variants and there should be no problem in calling ->disable_dma_irq() after the interrupts have been momentarily enabled in ->stmmac_init_chan() since the DMA is reset before these calls and not started until later. Note that I haven't added a symmetrical stmmac_disable_all_dma_irq() at the top of stmmac_release() before the NAPI disable since I can't see that it would do any good there since NAPI can re-enable DMA irqs. > More common flow (IMO) would be: > - request irq > - mask irq > - populate rings > - start dma > - enable napi > - unmask irq I don't think this driver has ever followed this exact sequence, but the request_irq() was done before the "start dma" and the "enable napi" before the commit mentioned in the Fixes: tag. But that's quite a while ago and the driver has changed a lot since then and gotten support for a lot of variants and features which I can't test, so I didn't dare to rewrite the entire init sequence. > Other than the difference in flow between open/stop there may also be > some unpleasantness around restarting tx queues twice with the patch > as is. New patch below, it works for me (though I don't have hardware with working suspend/resume support). I will send it out as a v2 if there are no objections. Thanks. 8<----- diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 6708ca2aa4f7..43978558d6c0 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -2260,6 +2260,23 @@ static void stmmac_stop_tx_dma(struct stmmac_priv *priv, u32 chan) stmmac_stop_tx(priv, priv->ioaddr, chan); } +static void stmmac_enable_all_dma_irq(struct stmmac_priv *priv) +{ + u32 rx_channels_count = priv->plat->rx_queues_to_use; + u32 tx_channels_count = priv->plat->tx_queues_to_use; + u32 dma_csr_ch = max(rx_channels_count, tx_channels_count); + u32 chan; + + for (chan = 0; chan < dma_csr_ch; chan++) { + struct stmmac_channel *ch = &priv->channel[chan]; + unsigned long flags; + + spin_lock_irqsave(&ch->lock, flags); + stmmac_enable_dma_irq(priv, priv->ioaddr, chan, 1, 1); + spin_unlock_irqrestore(&ch->lock, flags); + } +} + /** * stmmac_start_all_dma - start all RX and TX DMA channels * @priv: driver private structure @@ -2902,8 +2919,10 @@ static int stmmac_init_dma_engine(struct stmmac_priv *priv) stmmac_axi(priv, priv->ioaddr, priv->plat->axi); /* DMA CSR Channel configuration */ - for (chan = 0; chan < dma_csr_ch; chan++) + for (chan = 0; chan < dma_csr_ch; chan++) { stmmac_init_chan(priv, priv->ioaddr, priv->plat->dma_cfg, chan); + stmmac_disable_dma_irq(priv, priv->ioaddr, chan, 1, 1); + } /* DMA RX Channel Configuration */ for (chan = 0; chan < rx_channels_count; chan++) { @@ -3759,6 +3778,7 @@ static int stmmac_open(struct net_device *dev) stmmac_enable_all_queues(priv); netif_tx_start_all_queues(priv->dev); + stmmac_enable_all_dma_irq(priv); return 0; @@ -6508,8 +6528,10 @@ int stmmac_xdp_open(struct net_device *dev) } /* DMA CSR Channel configuration */ - for (chan = 0; chan < dma_csr_ch; chan++) + for (chan = 0; chan < dma_csr_ch; chan++) { stmmac_init_chan(priv, priv->ioaddr, priv->plat->dma_cfg, chan); + stmmac_disable_dma_irq(priv, priv->ioaddr, chan, 1, 1); + } /* Adjust Split header */ sph_en = (priv->hw->rx_csum > 0) && priv->sph; @@ -6570,6 +6592,7 @@ int stmmac_xdp_open(struct net_device *dev) stmmac_enable_all_queues(priv); netif_carrier_on(dev); netif_tx_start_all_queues(dev); + stmmac_enable_all_dma_irq(priv); return 0; @@ -7447,6 +7470,7 @@ int stmmac_resume(struct device *dev) stmmac_restore_hw_vlan_rx_fltr(priv, ndev, priv->hw); stmmac_enable_all_queues(priv); + stmmac_enable_all_dma_irq(priv); mutex_unlock(&priv->lock); rtnl_unlock(); _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel