3.16.83-rc1 review patch. If anyone has any objections, please let me know.
------------------
From: Finn Thain fthain@telegraphics.com.au
commit 865ad2f2201dc18685ba2686f13217f8b3a9c52c upstream.
The netif_stop_queue() call in sonic_send_packet() races with the netif_wake_queue() call in sonic_interrupt(). This causes issues like "NETDEV WATCHDOG: eth0 (macsonic): transmit queue 0 timed out". Fix this by disabling interrupts when accessing tx_skb[] and next_tx. Update a comment to clarify the synchronization properties.
Fixes: efcce839360f ("[PATCH] macsonic/jazzsonic network drivers update") Tested-by: Stan Johnson userm57@yahoo.com Signed-off-by: Finn Thain fthain@telegraphics.com.au Signed-off-by: David S. Miller davem@davemloft.net Signed-off-by: Ben Hutchings ben@decadent.org.uk --- drivers/net/ethernet/natsemi/sonic.c | 49 ++++++++++++++++++++-------- drivers/net/ethernet/natsemi/sonic.h | 1 + 2 files changed, 36 insertions(+), 14 deletions(-)
--- a/drivers/net/ethernet/natsemi/sonic.c +++ b/drivers/net/ethernet/natsemi/sonic.c @@ -50,6 +50,8 @@ static int sonic_open(struct net_device if (sonic_debug > 2) printk("sonic_open: initializing sonic driver.\n");
+ spin_lock_init(&lp->lock); + for (i = 0; i < SONIC_NUM_RRS; i++) { struct sk_buff *skb = netdev_alloc_skb(dev, SONIC_RBSIZE + 2); if (skb == NULL) { @@ -194,8 +196,6 @@ static void sonic_tx_timeout(struct net_ * wake the tx queue * Concurrently with all of this, the SONIC is potentially writing to * the status flags of the TDs. - * Until some mutual exclusion is added, this code will not work with SMP. However, - * MIPS Jazz machines and m68k Macs were all uni-processor machines. */
static int sonic_send_packet(struct sk_buff *skb, struct net_device *dev) @@ -203,7 +203,8 @@ static int sonic_send_packet(struct sk_b struct sonic_local *lp = netdev_priv(dev); dma_addr_t laddr; int length; - int entry = lp->next_tx; + int entry; + unsigned long flags;
if (sonic_debug > 2) printk("sonic_send_packet: skb=%p, dev=%p\n", skb, dev); @@ -226,6 +227,10 @@ static int sonic_send_packet(struct sk_b return NETDEV_TX_OK; }
+ spin_lock_irqsave(&lp->lock, flags); + + entry = lp->next_tx; + sonic_tda_put(dev, entry, SONIC_TD_STATUS, 0); /* clear status */ sonic_tda_put(dev, entry, SONIC_TD_FRAG_COUNT, 1); /* single fragment */ sonic_tda_put(dev, entry, SONIC_TD_PKTSIZE, length); /* length of packet */ @@ -235,10 +240,6 @@ static int sonic_send_packet(struct sk_b sonic_tda_put(dev, entry, SONIC_TD_LINK, sonic_tda_get(dev, entry, SONIC_TD_LINK) | SONIC_EOL);
- /* - * Must set tx_skb[entry] only after clearing status, and - * before clearing EOL and before stopping queue - */ wmb(); lp->tx_len[entry] = length; lp->tx_laddr[entry] = laddr; @@ -263,6 +264,8 @@ static int sonic_send_packet(struct sk_b
SONIC_WRITE(SONIC_CMD, SONIC_CR_TXP);
+ spin_unlock_irqrestore(&lp->lock, flags); + return NETDEV_TX_OK; }
@@ -275,9 +278,21 @@ static irqreturn_t sonic_interrupt(int i struct net_device *dev = dev_id; struct sonic_local *lp = netdev_priv(dev); int status; + unsigned long flags; + + /* The lock has two purposes. Firstly, it synchronizes sonic_interrupt() + * with sonic_send_packet() so that the two functions can share state. + * Secondly, it makes sonic_interrupt() re-entrant, as that is required + * by macsonic which must use two IRQs with different priority levels. + */ + spin_lock_irqsave(&lp->lock, flags); + + status = SONIC_READ(SONIC_ISR) & SONIC_IMR_DEFAULT; + if (!status) { + spin_unlock_irqrestore(&lp->lock, flags);
- if (!(status = SONIC_READ(SONIC_ISR) & SONIC_IMR_DEFAULT)) return IRQ_NONE; + }
do { if (status & SONIC_INT_PKTRX) { @@ -292,11 +307,12 @@ static irqreturn_t sonic_interrupt(int i int td_status; int freed_some = 0;
- /* At this point, cur_tx is the index of a TD that is one of: - * unallocated/freed (status set & tx_skb[entry] clear) - * allocated and sent (status set & tx_skb[entry] set ) - * allocated and not yet sent (status clear & tx_skb[entry] set ) - * still being allocated by sonic_send_packet (status clear & tx_skb[entry] clear) + /* The state of a Transmit Descriptor may be inferred + * from { tx_skb[entry], td_status } as follows. + * { clear, clear } => the TD has never been used + * { set, clear } => the TD was handed to SONIC + * { set, set } => the TD was handed back + * { clear, set } => the TD is available for re-use */
if (sonic_debug > 2) @@ -398,7 +414,12 @@ static irqreturn_t sonic_interrupt(int i /* load CAM done */ if (status & SONIC_INT_LCD) SONIC_WRITE(SONIC_ISR, SONIC_INT_LCD); /* clear the interrupt */ - } while((status = SONIC_READ(SONIC_ISR) & SONIC_IMR_DEFAULT)); + + status = SONIC_READ(SONIC_ISR) & SONIC_IMR_DEFAULT; + } while (status); + + spin_unlock_irqrestore(&lp->lock, flags); + return IRQ_HANDLED; }
--- a/drivers/net/ethernet/natsemi/sonic.h +++ b/drivers/net/ethernet/natsemi/sonic.h @@ -320,6 +320,7 @@ struct sonic_local { unsigned int next_tx; /* next free TD */ struct device *device; /* generic device */ struct net_device_stats stats; + spinlock_t lock; };
#define TX_TIMEOUT (3 * HZ)