lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 11 Nov 2015 00:40:09 +0000 From: Måns Rullgård <mans@...sr.com> To: Francois Romieu <romieu@...zoreil.com> Cc: linux-kernel@...r.kernel.org, netdev@...r.kernel.org, slash.tmp@...e.fr Subject: Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller Francois Romieu <romieu@...zoreil.com> writes: > Mans Rullgard <mans@...sr.com> : >> 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@...sr.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists