From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752398AbbKKAkQ (ORCPT ); Tue, 10 Nov 2015 19:40:16 -0500 Received: from unicorn.mansr.com ([81.2.72.234]:43191 "EHLO unicorn.mansr.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752245AbbKKAkO convert rfc822-to-8bit (ORCPT ); Tue, 10 Nov 2015 19:40:14 -0500 From: =?iso-8859-1?Q?M=E5ns_Rullg=E5rd?= To: Francois Romieu Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org, slash.tmp@free.fr Subject: Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller References: <1447172063-27234-1-git-send-email-mans@mansr.com> <20151110233448.GA8646@electric-eye.fr.zoreil.com> Date: Wed, 11 Nov 2015 00:40:09 +0000 In-Reply-To: <20151110233448.GA8646@electric-eye.fr.zoreil.com> (Francois Romieu's message of "Wed, 11 Nov 2015 00:34:48 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Francois Romieu writes: > Mans Rullgard : >> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c >> new file mode 100644 >> index 0000000..11cd389 >> --- /dev/null >> +++ b/drivers/net/ethernet/aurora/nb8800.c > [...] >> +static int nb8800_xmit(struct sk_buff *skb, struct net_device *dev) >> +{ > [...] >> + >> + netdev_sent_queue(dev, skb->len); >> + >> + smp_mb__before_spinlock(); >> + spin_lock_irqsave(&priv->tx_lock, flags); > > At some point you may consider performing both Tx and Rx from napi context > and thus replacing priv->tx_lock with netif_tx_lock. That lock is to synchronise the DMA start between nb8800_xmit() and the interrupt handler. When the DMA complete interrupt arrives, the next chain should be kicked off as quickly as possible, and I don't see why that would benefit from being done in napi context. >> + >> + if (!skb->xmit_more) { >> + priv->tx_chain->ready = true; >> + priv->tx_chain = NULL; >> + nb8800_tx_dma_start(dev); >> + } >> + >> + spin_unlock_irqrestore(&priv->tx_lock, flags); >> + >> + priv->tx_next = next; > > Are there strong reasons why nb8800_tx_done could not kick between > spin_unlock_irqrestore and the non-local update of priv->tx_next ? Good catch. priv->tx_next wasn't accessed elsewhere in an earlier version, and I forgot to fix that. nb8800_tx_done() makes sure the DMA has actually finished, so priv->tx_next should be updated before starting the DMA rather than after. The check against tx_next in nb8800_tx_done() is only to put some limit on the loop and to avoid confusion when nb8800_dma_stop() does it's dance. There should be no need for more synchronisation here than what the already present memory barriers provide. > [...] >> +static irqreturn_t nb8800_irq(int irq, void *dev_id) >> +{ >> + struct net_device *dev = dev_id; >> + struct nb8800_priv *priv = netdev_priv(dev); >> + u32 val; >> + >> + /* tx interrupt */ >> + val = nb8800_readl(priv, NB8800_TXC_SR); >> + if (val) { > [...] >> + } >> + >> + /* rx interrupt */ >> + val = nb8800_readl(priv, NB8800_RXC_SR); >> + if (val) { > [...] >> + } >> + >> + return IRQ_HANDLED; > > Returning IRQ_HANDLED is fine if one of those hold: > 1. you're sure that at least one of the "if" branch is used > 2. you'll be able to quickly figure out what's happening whenever 1. stops > being true. You're right, better to check that the device really did have something to say. -- Måns Rullgård mans@mansr.com