Hi David,
Various SONIC driver problems have become apparent over the years,
including tx watchdog timeouts, lost packets and duplicated packets.
The problems are mostly caused by bugs in buffer handling, locking and
(re-)initialization code.
This patch series resolves these problems. Several cleanup patches are
included at the beginning.
This series has been tested on National Semiconductor hardware (macsonic),
qemu-system-m68k (macsonic) and qemu-system-mips64el (jazzsonic).
The emulated dp8393x device used in QEMU also has bugs.
I have fixed the bugs that I know of in a series of patches at,
https://github.com/fthain/qemu/commits/sonic
Finn Thain (19):
net/sonic: Remove obsolete comment
net/sonic: Remove redundant next_tx variable
net/sonic: Refactor duplicated code
net/sonic: Add mutual exclusion for accessing shared state
net/sonic: Remove redundant netif_start_queue() call
net/macsonic: Remove interrupt handler wrapper
net/sonic: Clear interrupt flags immediately
net/sonic: Use MMIO accessors
net/sonic: Remove explicit memory barriers
net/sonic: Start packet transmission immediately
net/sonic: Fix interface error stats collection
net/sonic: Fix receive buffer handling
net/sonic: Avoid needless receive descriptor EOL flag updates
net/sonic: Improve receive descriptor status flag check
net/sonic: Fix receive buffer replenishment
net/sonic: Quiesce SONIC before re-initializing descriptor memory
net/sonic: Fix command register usage
net/sonic: Fix CAM initialization
net/sonic: Prevent tx watchdog timeout
drivers/net/ethernet/natsemi/jazzsonic.c | 31 +-
drivers/net/ethernet/natsemi/macsonic.c | 48 +--
drivers/net/ethernet/natsemi/sonic.c | 433 ++++++++++++++---------
drivers/net/ethernet/natsemi/sonic.h | 45 ++-
drivers/net/ethernet/natsemi/xtsonic.c | 40 +--
5 files changed, 313 insertions(+), 284 deletions(-)
--
2.24.1
The SONIC can sometimes advance its rx buffer pointer (RRP register)
without advancing its rx descriptor pointer (CRDA register). As a result
the index of the current rx descriptor may not equal that of the current
rx buffer. The driver mistakenly assumes that they are always equal.
This assumption leads to incorrect packet lengths and possible packet
duplication. Avoid this by calling a new function to locate the buffer
corresponding to a given descriptor.
Fixes: efcce839360f ("[PATCH] macsonic/jazzsonic network drivers update")
Tested-by: Stan Johnson <[email protected]>
Signed-off-by: Finn Thain <[email protected]>
---
drivers/net/ethernet/natsemi/sonic.c | 36 ++++++++++++++++++++++++----
drivers/net/ethernet/natsemi/sonic.h | 5 ++--
2 files changed, 34 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/natsemi/sonic.c b/drivers/net/ethernet/natsemi/sonic.c
index ea74c3c2c501..23740fde6c02 100644
--- a/drivers/net/ethernet/natsemi/sonic.c
+++ b/drivers/net/ethernet/natsemi/sonic.c
@@ -443,6 +443,22 @@ static irqreturn_t sonic_interrupt(int irq, void *dev_id)
return IRQ_HANDLED;
}
+/* Return the array index corresponding to a given Receive Buffer pointer. */
+
+static inline int index_from_addr(struct sonic_local *lp, dma_addr_t addr,
+ unsigned int last)
+{
+ unsigned int i = last;
+
+ do {
+ i = (i + 1) & SONIC_RRS_MASK;
+ if (addr == lp->rx_laddr[i])
+ return i;
+ } while (i != last);
+
+ return -ENOENT;
+}
+
/*
* We have a good packet(s), pass it/them up the network stack.
*/
@@ -462,6 +478,16 @@ static void sonic_rx(struct net_device *dev)
status = sonic_rda_get(dev, entry, SONIC_RD_STATUS);
if (status & SONIC_RCR_PRX) {
+ u32 addr = (sonic_rda_get(dev, entry,
+ SONIC_RD_PKTPTR_H) << 16) |
+ sonic_rda_get(dev, entry, SONIC_RD_PKTPTR_L);
+ int i = index_from_addr(lp, addr, entry);
+
+ if (i < 0) {
+ WARN_ONCE(1, "failed to find buffer!\n");
+ break;
+ }
+
/* Malloc up new buffer. */
new_skb = netdev_alloc_skb(dev, SONIC_RBSIZE + 2);
if (new_skb == NULL) {
@@ -483,7 +509,7 @@ static void sonic_rx(struct net_device *dev)
/* now we have a new skb to replace it, pass the used one up the stack */
dma_unmap_single(lp->device, lp->rx_laddr[entry], SONIC_RBSIZE, DMA_FROM_DEVICE);
- used_skb = lp->rx_skb[entry];
+ used_skb = lp->rx_skb[i];
pkt_len = sonic_rda_get(dev, entry, SONIC_RD_PKTLEN);
skb_trim(used_skb, pkt_len);
used_skb->protocol = eth_type_trans(used_skb, dev);
@@ -492,13 +518,13 @@ static void sonic_rx(struct net_device *dev)
lp->stats.rx_bytes += pkt_len;
/* and insert the new skb */
- lp->rx_laddr[entry] = new_laddr;
- lp->rx_skb[entry] = new_skb;
+ lp->rx_laddr[i] = new_laddr;
+ lp->rx_skb[i] = new_skb;
bufadr_l = (unsigned long)new_laddr & 0xffff;
bufadr_h = (unsigned long)new_laddr >> 16;
- sonic_rra_put(dev, entry, SONIC_RR_BUFADR_L, bufadr_l);
- sonic_rra_put(dev, entry, SONIC_RR_BUFADR_H, bufadr_h);
+ sonic_rra_put(dev, i, SONIC_RR_BUFADR_L, bufadr_l);
+ sonic_rra_put(dev, i, SONIC_RR_BUFADR_H, bufadr_h);
} else {
/* This should only happen, if we enable accepting broken packets. */
}
diff --git a/drivers/net/ethernet/natsemi/sonic.h b/drivers/net/ethernet/natsemi/sonic.h
index 227975eb4cb8..46052a0cfe22 100644
--- a/drivers/net/ethernet/natsemi/sonic.h
+++ b/drivers/net/ethernet/natsemi/sonic.h
@@ -275,8 +275,9 @@
#define SONIC_NUM_RDS SONIC_NUM_RRS /* number of receive descriptors */
#define SONIC_NUM_TDS 16 /* number of transmit descriptors */
-#define SONIC_RDS_MASK (SONIC_NUM_RDS-1)
-#define SONIC_TDS_MASK (SONIC_NUM_TDS-1)
+#define SONIC_RRS_MASK (SONIC_NUM_RRS - 1)
+#define SONIC_RDS_MASK (SONIC_NUM_RDS - 1)
+#define SONIC_TDS_MASK (SONIC_NUM_TDS - 1)
#define SONIC_RBSIZE 1520 /* size of one resource buffer */
--
2.24.1
After sonic_tx_timeout() calls sonic_init(), it can happen that
sonic_rx() will subsequently encounter a receive descriptor with no
flags set. Remove the comment that says that this can't happen.
When giving a receive descriptor to the SONIC, clear the descriptor
status field. That way, any rx descriptor with flags set can only be
a newly received packet.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Tested-by: Stan Johnson <[email protected]>
Signed-off-by: Finn Thain <[email protected]>
---
drivers/net/ethernet/natsemi/sonic.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/natsemi/sonic.c b/drivers/net/ethernet/natsemi/sonic.c
index 56098557a579..5db107b8c6ee 100644
--- a/drivers/net/ethernet/natsemi/sonic.c
+++ b/drivers/net/ethernet/natsemi/sonic.c
@@ -465,7 +465,6 @@ static inline int index_from_addr(struct sonic_local *lp, dma_addr_t addr,
static void sonic_rx(struct net_device *dev)
{
struct sonic_local *lp = netdev_priv(dev);
- int status;
int entry = lp->cur_rx;
int prev_entry = lp->eol_rx;
@@ -476,9 +475,11 @@ static void sonic_rx(struct net_device *dev)
u16 bufadr_l;
u16 bufadr_h;
int pkt_len;
+ u16 status = sonic_rda_get(dev, entry, SONIC_RD_STATUS);
+
+ /* If the RD has LPKT set, the chip has finished with the RB */
- status = sonic_rda_get(dev, entry, SONIC_RD_STATUS);
- if (status & SONIC_RCR_PRX) {
+ if ((status & SONIC_RCR_PRX) && (status & SONIC_RCR_LPKT)) {
u32 addr = (sonic_rda_get(dev, entry,
SONIC_RD_PKTPTR_H) << 16) |
sonic_rda_get(dev, entry, SONIC_RD_PKTPTR_L);
@@ -526,10 +527,6 @@ static void sonic_rx(struct net_device *dev)
bufadr_h = (unsigned long)new_laddr >> 16;
sonic_rra_put(dev, i, SONIC_RR_BUFADR_L, bufadr_l);
sonic_rra_put(dev, i, SONIC_RR_BUFADR_H, bufadr_h);
- } else {
- /* This should only happen, if we enable accepting broken packets. */
- }
- if (status & SONIC_RCR_LPKT) {
/*
* this was the last packet out of the current receive buffer
* give the buffer back to the SONIC
@@ -542,12 +539,11 @@ static void sonic_rx(struct net_device *dev)
__func__);
SONIC_WRITE(SONIC_ISR, SONIC_INT_RBE); /* clear the flag */
}
- } else
- printk(KERN_ERR "%s: rx desc without RCR_LPKT. Shouldn't happen !?\n",
- dev->name);
+ }
/*
* give back the descriptor
*/
+ sonic_rda_put(dev, entry, SONIC_RD_STATUS, 0);
sonic_rda_put(dev, entry, SONIC_RD_IN_USE, 1);
prev_entry = entry;
--
2.24.1
The chip can change a packet's descriptor status flags at any time.
However, an active interrupt flag gets cleared rather late. This
allows a race condition that could theoretically lose an interrupt.
Fix this by clearing asserted interrupt flags immediately.
Fixes: efcce839360f ("[PATCH] macsonic/jazzsonic network drivers update")
Tested-by: Stan Johnson <[email protected]>
Signed-off-by: Finn Thain <[email protected]>
---
drivers/net/ethernet/natsemi/sonic.c | 28 ++++++----------------------
1 file changed, 6 insertions(+), 22 deletions(-)
diff --git a/drivers/net/ethernet/natsemi/sonic.c b/drivers/net/ethernet/natsemi/sonic.c
index 84a30928d4e2..2cde692a0602 100644
--- a/drivers/net/ethernet/natsemi/sonic.c
+++ b/drivers/net/ethernet/natsemi/sonic.c
@@ -335,10 +335,11 @@ static irqreturn_t sonic_interrupt(int irq, void *dev_id)
}
do {
+ SONIC_WRITE(SONIC_ISR, status); /* clear the interrupt(s) */
+
if (status & SONIC_INT_PKTRX) {
netif_dbg(lp, intr, dev, "%s: packet rx\n", __func__);
sonic_rx(dev); /* got packet(s) */
- SONIC_WRITE(SONIC_ISR, SONIC_INT_PKTRX); /* clear the interrupt */
}
if (status & SONIC_INT_TXDN) {
@@ -393,7 +394,6 @@ static irqreturn_t sonic_interrupt(int irq, void *dev_id)
if (freed_some || lp->tx_skb[entry] == NULL)
netif_wake_queue(dev); /* The ring is no longer full */
lp->cur_tx = entry;
- SONIC_WRITE(SONIC_ISR, SONIC_INT_TXDN); /* clear the interrupt */
}
/*
@@ -403,42 +403,31 @@ static irqreturn_t sonic_interrupt(int irq, void *dev_id)
netif_dbg(lp, rx_err, dev, "%s: rx fifo overrun\n",
__func__);
lp->stats.rx_fifo_errors++;
- SONIC_WRITE(SONIC_ISR, SONIC_INT_RFO); /* clear the interrupt */
}
if (status & SONIC_INT_RDE) {
netif_dbg(lp, rx_err, dev, "%s: rx descriptors exhausted\n",
__func__);
lp->stats.rx_dropped++;
- SONIC_WRITE(SONIC_ISR, SONIC_INT_RDE); /* clear the interrupt */
}
if (status & SONIC_INT_RBAE) {
netif_dbg(lp, rx_err, dev, "%s: rx buffer area exceeded\n",
__func__);
lp->stats.rx_dropped++;
- SONIC_WRITE(SONIC_ISR, SONIC_INT_RBAE); /* clear the interrupt */
}
/* counter overruns; all counters are 16bit wide */
- if (status & SONIC_INT_FAE) {
+ if (status & SONIC_INT_FAE)
lp->stats.rx_frame_errors += 65536;
- SONIC_WRITE(SONIC_ISR, SONIC_INT_FAE); /* clear the interrupt */
- }
- if (status & SONIC_INT_CRC) {
+ if (status & SONIC_INT_CRC)
lp->stats.rx_crc_errors += 65536;
- SONIC_WRITE(SONIC_ISR, SONIC_INT_CRC); /* clear the interrupt */
- }
- if (status & SONIC_INT_MP) {
+ if (status & SONIC_INT_MP)
lp->stats.rx_missed_errors += 65536;
- SONIC_WRITE(SONIC_ISR, SONIC_INT_MP); /* clear the interrupt */
- }
/* transmit error */
- if (status & SONIC_INT_TXER) {
+ if (status & SONIC_INT_TXER)
if (SONIC_READ(SONIC_TCR) & SONIC_TCR_FU)
netif_dbg(lp, tx_err, dev, "%s: tx fifo underrun\n",
__func__);
- SONIC_WRITE(SONIC_ISR, SONIC_INT_TXER); /* clear the interrupt */
- }
/* bus retry */
if (status & SONIC_INT_BR) {
@@ -447,13 +436,8 @@ static irqreturn_t sonic_interrupt(int irq, void *dev_id)
/* ... to help debug DMA problems causing endless interrupts. */
/* Bounce the eth interface to turn on the interrupt again. */
SONIC_WRITE(SONIC_IMR, 0);
- SONIC_WRITE(SONIC_ISR, SONIC_INT_BR); /* clear the interrupt */
}
- /* load CAM done */
- if (status & SONIC_INT_LCD)
- SONIC_WRITE(SONIC_ISR, SONIC_INT_LCD); /* clear the interrupt */
-
status = SONIC_READ(SONIC_ISR) & SONIC_IMR_DEFAULT;
} while (status);
--
2.24.1
Make sure the SONIC's DMA engine is idle before altering the transmit
and receive descriptors. Add a helper for this as it will be needed
again.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Tested-by: Stan Johnson <[email protected]>
Signed-off-by: Finn Thain <[email protected]>
---
drivers/net/ethernet/natsemi/sonic.c | 26 ++++++++++++++++++++++++++
drivers/net/ethernet/natsemi/sonic.h | 3 +++
2 files changed, 29 insertions(+)
diff --git a/drivers/net/ethernet/natsemi/sonic.c b/drivers/net/ethernet/natsemi/sonic.c
index 343a8a7a6edf..b70470ce9c9c 100644
--- a/drivers/net/ethernet/natsemi/sonic.c
+++ b/drivers/net/ethernet/natsemi/sonic.c
@@ -150,6 +150,25 @@ static int sonic_open(struct net_device *dev)
return 0;
}
+/* Wait for the SONIC to become idle. */
+
+static void sonic_quiesce(struct net_device *dev, u16 mask)
+{
+ struct sonic_local * __maybe_unused lp = netdev_priv(dev);
+ int i;
+ u16 bits;
+
+ for (i = 0; i < 1000; ++i) {
+ bits = SONIC_READ(SONIC_CMD) & mask;
+ if (!bits)
+ return;
+ if (irqs_disabled() || in_interrupt())
+ udelay(20);
+ else
+ usleep_range(100, 200);
+ }
+ WARN_ONCE(1, "command deadline expired! 0x%04x\n", bits);
+}
/*
* Close the SONIC device
@@ -166,6 +185,9 @@ static int sonic_close(struct net_device *dev)
/*
* stop the SONIC, disable interrupts
*/
+ SONIC_WRITE(SONIC_CMD, SONIC_CR_RXDIS);
+ sonic_quiesce(dev, SONIC_CR_ALL);
+
SONIC_WRITE(SONIC_IMR, 0);
SONIC_WRITE(SONIC_ISR, 0x7fff);
SONIC_WRITE(SONIC_CMD, SONIC_CR_RST);
@@ -205,6 +227,9 @@ static void sonic_tx_timeout(struct net_device *dev)
* put the Sonic into software-reset mode and
* disable all interrupts before releasing DMA buffers
*/
+ SONIC_WRITE(SONIC_CMD, SONIC_CR_RXDIS);
+ sonic_quiesce(dev, SONIC_CR_ALL);
+
SONIC_WRITE(SONIC_IMR, 0);
SONIC_WRITE(SONIC_ISR, 0x7fff);
SONIC_WRITE(SONIC_CMD, SONIC_CR_RST);
@@ -684,6 +709,7 @@ static int sonic_init(struct net_device *dev)
*/
SONIC_WRITE(SONIC_CMD, 0);
SONIC_WRITE(SONIC_CMD, SONIC_CR_RXDIS);
+ sonic_quiesce(dev, SONIC_CR_ALL);
/*
* initialize the receive resource area
diff --git a/drivers/net/ethernet/natsemi/sonic.h b/drivers/net/ethernet/natsemi/sonic.h
index 61f253be92a0..e0d74ca9a77a 100644
--- a/drivers/net/ethernet/natsemi/sonic.h
+++ b/drivers/net/ethernet/natsemi/sonic.h
@@ -110,6 +110,9 @@
#define SONIC_CR_TXP 0x0002
#define SONIC_CR_HTX 0x0001
+#define SONIC_CR_ALL (SONIC_CR_LCAM | SONIC_CR_RRRA | \
+ SONIC_CR_RXEN | SONIC_CR_TXP)
+
/*
* SONIC data configuration bits
*/
--
2.24.1
This comment is meaningless since mark_bh() was removed a long time ago.
Tested-by: Stan Johnson <[email protected]>
Signed-off-by: Finn Thain <[email protected]>
---
drivers/net/ethernet/natsemi/sonic.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/net/ethernet/natsemi/sonic.c b/drivers/net/ethernet/natsemi/sonic.c
index b339125b2f09..657b5327baf9 100644
--- a/drivers/net/ethernet/natsemi/sonic.c
+++ b/drivers/net/ethernet/natsemi/sonic.c
@@ -501,11 +501,6 @@ static void sonic_rx(struct net_device *dev)
lp->eol_rx = entry;
lp->cur_rx = entry = (entry + 1) & SONIC_RDS_MASK;
}
- /*
- * If any worth-while packets have been received, netif_rx()
- * has done a mark_bh(NET_BH) for us and will work on them
- * when we get to the bottom-half routine.
- */
}
--
2.24.1
There are several issues relating to command register usage during
chip initialization.
Firstly, the SONIC sometimes comes out of software reset with the
Start Timer bit set. This gets logged as,
macsonic macsonic eth0: sonic_init: status=24, i=101
Avoid this by giving the Stop Timer command earlier than later.
Secondly, the loop that waits for the Read RRA command to complete has
the break condition inverted. That's why the for loop iterates until
its termination condition. Call the helper for this instead.
Finally, give the Receiver Enable command after clearing interrupts,
not before, to avoid the possibility of losing an interrupt.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Tested-by: Stan Johnson <[email protected]>
Signed-off-by: Finn Thain <[email protected]>
---
drivers/net/ethernet/natsemi/sonic.c | 18 +++---------------
1 file changed, 3 insertions(+), 15 deletions(-)
diff --git a/drivers/net/ethernet/natsemi/sonic.c b/drivers/net/ethernet/natsemi/sonic.c
index b70470ce9c9c..01055a53517d 100644
--- a/drivers/net/ethernet/natsemi/sonic.c
+++ b/drivers/net/ethernet/natsemi/sonic.c
@@ -691,7 +691,6 @@ static void sonic_multicast_list(struct net_device *dev)
*/
static int sonic_init(struct net_device *dev)
{
- unsigned int cmd;
struct sonic_local *lp = netdev_priv(dev);
int i;
@@ -708,7 +707,7 @@ static int sonic_init(struct net_device *dev)
* enable interrupts, then completely initialize the SONIC
*/
SONIC_WRITE(SONIC_CMD, 0);
- SONIC_WRITE(SONIC_CMD, SONIC_CR_RXDIS);
+ SONIC_WRITE(SONIC_CMD, SONIC_CR_RXDIS | SONIC_CR_STP);
sonic_quiesce(dev, SONIC_CR_ALL);
/*
@@ -738,14 +737,7 @@ static int sonic_init(struct net_device *dev)
netif_dbg(lp, ifup, dev, "%s: issuing RRRA command\n", __func__);
SONIC_WRITE(SONIC_CMD, SONIC_CR_RRRA);
- i = 0;
- while (i++ < 100) {
- if (SONIC_READ(SONIC_CMD) & SONIC_CR_RRRA)
- break;
- }
-
- netif_dbg(lp, ifup, dev, "%s: status=%x, i=%d\n", __func__,
- SONIC_READ(SONIC_CMD), i);
+ sonic_quiesce(dev, SONIC_CR_RRRA);
/*
* Initialize the receive descriptors so that they
@@ -833,15 +825,11 @@ static int sonic_init(struct net_device *dev)
* enable receiver, disable loopback
* and enable all interrupts
*/
- SONIC_WRITE(SONIC_CMD, SONIC_CR_RXEN | SONIC_CR_STP);
SONIC_WRITE(SONIC_RCR, SONIC_RCR_DEFAULT);
SONIC_WRITE(SONIC_TCR, SONIC_TCR_DEFAULT);
SONIC_WRITE(SONIC_ISR, 0x7fff);
SONIC_WRITE(SONIC_IMR, SONIC_IMR_DEFAULT);
-
- cmd = SONIC_READ(SONIC_CMD);
- if ((cmd & SONIC_CR_RXEN) == 0 || (cmd & SONIC_CR_STP) == 0)
- printk(KERN_ERR "sonic_init: failed, status=%x\n", cmd);
+ SONIC_WRITE(SONIC_CMD, SONIC_CR_RXEN);
netif_dbg(lp, ifup, dev, "%s: new status=%x\n", __func__,
SONIC_READ(SONIC_CMD));
--
2.24.1
Section 5.5.3.2 of the datasheet says,
If FIFO Underrun, Byte Count Mismatch, Excessive Collision, or
Excessive Deferral (if enabled) errors occur, transmission ceases.
In this situation, the chip asserts a TXER interrupt rather than TXDN.
But the handler for the TXDN is the only way that the transmit queue
gets restarted. Hence, an aborted transmission can result in a watchdog
timeout.
This problem can be reproduced on congested link, as that can result in
excessive transmitter collisions. Another way to reproduce this is with
a FIFO Underrun, which may be caused by DMA latency.
In event of a TXER interrupt, prevent a watchdog timeout by restarting
transmission.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Tested-by: Stan Johnson <[email protected]>
Signed-off-by: Finn Thain <[email protected]>
---
drivers/net/ethernet/natsemi/sonic.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/natsemi/sonic.c b/drivers/net/ethernet/natsemi/sonic.c
index 1a569df63fde..bebdbb00a595 100644
--- a/drivers/net/ethernet/natsemi/sonic.c
+++ b/drivers/net/ethernet/natsemi/sonic.c
@@ -446,10 +446,19 @@ static irqreturn_t sonic_interrupt(int irq, void *dev_id)
lp->stats.rx_missed_errors += 65536;
/* transmit error */
- if (status & SONIC_INT_TXER)
- if (SONIC_READ(SONIC_TCR) & SONIC_TCR_FU)
- netif_dbg(lp, tx_err, dev, "%s: tx fifo underrun\n",
- __func__);
+ if (status & SONIC_INT_TXER) {
+ u16 tcr = SONIC_READ(SONIC_TCR);
+
+ netif_dbg(lp, tx_err, dev, "%s: TXER intr, TCR %04x\n",
+ __func__, tcr);
+
+ if (tcr & (SONIC_TCR_EXD | SONIC_TCR_EXC |
+ SONIC_TCR_FU | SONIC_TCR_BCM)) {
+ /* Aborted transmission. Try again. */
+ netif_stop_queue(dev);
+ SONIC_WRITE(SONIC_CMD, SONIC_CR_TXP);
+ }
+ }
/* bus retry */
if (status & SONIC_INT_BR) {
--
2.24.1
The tx queue must be running already, otherwise sonic_send_packet()
would not have been called.
Tested-by: Stan Johnson <[email protected]>
Signed-off-by: Finn Thain <[email protected]>
---
drivers/net/ethernet/natsemi/sonic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/natsemi/sonic.c b/drivers/net/ethernet/natsemi/sonic.c
index dbbbc8bc72ff..84a30928d4e2 100644
--- a/drivers/net/ethernet/natsemi/sonic.c
+++ b/drivers/net/ethernet/natsemi/sonic.c
@@ -303,7 +303,7 @@ static int sonic_send_packet(struct sk_buff *skb, struct net_device *dev)
netif_dbg(lp, tx_queued, dev, "%s: stopping queue\n", __func__);
netif_stop_queue(dev);
/* after this packet, wait for ISR to free up some TDAs */
- } else netif_start_queue(dev);
+ }
netif_dbg(lp, tx_queued, dev, "%s: issuing Tx command\n", __func__);
--
2.24.1
Give the transmit command as soon as the transmit descriptor is ready.
Tested-by: Stan Johnson <[email protected]>
Signed-off-by: Finn Thain <[email protected]>
---
drivers/net/ethernet/natsemi/sonic.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/natsemi/sonic.c b/drivers/net/ethernet/natsemi/sonic.c
index 6322b4543e0b..6660bd75b699 100644
--- a/drivers/net/ethernet/natsemi/sonic.c
+++ b/drivers/net/ethernet/natsemi/sonic.c
@@ -287,12 +287,15 @@ static int sonic_send_packet(struct sk_buff *skb, struct net_device *dev)
sonic_tda_put(dev, entry, SONIC_TD_LINK,
sonic_tda_get(dev, entry, SONIC_TD_LINK) | SONIC_EOL);
+ sonic_tda_put(dev, lp->eol_tx, SONIC_TD_LINK, ~SONIC_EOL &
+ sonic_tda_get(dev, lp->eol_tx, SONIC_TD_LINK));
+
+ SONIC_WRITE(SONIC_CMD, SONIC_CR_TXP);
+
lp->tx_len[entry] = length;
lp->tx_laddr[entry] = laddr;
lp->tx_skb[entry] = skb;
- sonic_tda_put(dev, lp->eol_tx, SONIC_TD_LINK,
- sonic_tda_get(dev, lp->eol_tx, SONIC_TD_LINK) & ~SONIC_EOL);
lp->eol_tx = entry;
entry = (entry + 1) & SONIC_TDS_MASK;
@@ -305,8 +308,6 @@ static int sonic_send_packet(struct sk_buff *skb, struct net_device *dev)
netif_dbg(lp, tx_queued, dev, "%s: issuing Tx command\n", __func__);
- SONIC_WRITE(SONIC_CMD, SONIC_CR_TXP);
-
local_irq_restore(flags);
return NETDEV_TX_OK;
--
2.24.1
The explicit memory barriers are redundant now that local irq
save/restore pairs and MMIO accessors have been employed.
Tested-by: Stan Johnson <[email protected]>
Signed-off-by: Finn Thain <[email protected]>
---
drivers/net/ethernet/natsemi/sonic.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/net/ethernet/natsemi/sonic.c b/drivers/net/ethernet/natsemi/sonic.c
index 2cde692a0602..6322b4543e0b 100644
--- a/drivers/net/ethernet/natsemi/sonic.c
+++ b/drivers/net/ethernet/natsemi/sonic.c
@@ -287,12 +287,10 @@ static int sonic_send_packet(struct sk_buff *skb, struct net_device *dev)
sonic_tda_put(dev, entry, SONIC_TD_LINK,
sonic_tda_get(dev, entry, SONIC_TD_LINK) | SONIC_EOL);
- wmb();
lp->tx_len[entry] = length;
lp->tx_laddr[entry] = laddr;
lp->tx_skb[entry] = skb;
- wmb();
sonic_tda_put(dev, lp->eol_tx, SONIC_TD_LINK,
sonic_tda_get(dev, lp->eol_tx, SONIC_TD_LINK) & ~SONIC_EOL);
lp->eol_tx = entry;
--
2.24.1
Section 4.3.1 of the datasheet says,
This bit [TXP] must not be set if a Load CAM operation is in
progress (LCAM is set). The SONIC will lock up if both bits are
set simultaneously.
Testing has shown that the driver sometimes attempts to set LCAM
while TXP is set. Avoid this by waiting for command completion
before and after giving the LCAM command.
After issuing the Load CAM command, poll for !SONIC_CR_LCAM rather than
SONIC_INT_LCD, because the SONIC_CR_TXP bit can't be used until
!SONIC_CR_LCAM.
When in reset mode, take the opportunity to reset the CAM Enable
register.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Tested-by: Stan Johnson <[email protected]>
Signed-off-by: Finn Thain <[email protected]>
---
drivers/net/ethernet/natsemi/sonic.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/natsemi/sonic.c b/drivers/net/ethernet/natsemi/sonic.c
index 01055a53517d..1a569df63fde 100644
--- a/drivers/net/ethernet/natsemi/sonic.c
+++ b/drivers/net/ethernet/natsemi/sonic.c
@@ -661,6 +661,8 @@ static void sonic_multicast_list(struct net_device *dev)
(netdev_mc_count(dev) > 15)) {
rcr |= SONIC_RCR_AMC;
} else {
+ unsigned long flags;
+
netif_dbg(lp, ifup, dev, "%s: mc_count %d\n", __func__,
netdev_mc_count(dev));
sonic_set_cam_enable(dev, 1); /* always enable our own address */
@@ -674,9 +676,14 @@ static void sonic_multicast_list(struct net_device *dev)
i++;
}
SONIC_WRITE(SONIC_CDC, 16);
- /* issue Load CAM command */
SONIC_WRITE(SONIC_CDP, lp->cda_laddr & 0xffff);
+
+ /* LCAM and TXP commands can't be used simultaneously */
+ local_irq_save(flags);
+ sonic_quiesce(dev, SONIC_CR_TXP);
SONIC_WRITE(SONIC_CMD, SONIC_CR_LCAM);
+ sonic_quiesce(dev, SONIC_CR_LCAM);
+ local_irq_restore(flags);
}
}
@@ -702,6 +709,9 @@ static int sonic_init(struct net_device *dev)
SONIC_WRITE(SONIC_ISR, 0x7fff);
SONIC_WRITE(SONIC_CMD, SONIC_CR_RST);
+ /* While in reset mode, clear CAM Enable register */
+ SONIC_WRITE(SONIC_CE, 0);
+
/*
* clear software reset flag, disable receiver, clear and
* enable interrupts, then completely initialize the SONIC
@@ -812,14 +822,7 @@ static int sonic_init(struct net_device *dev)
* load the CAM
*/
SONIC_WRITE(SONIC_CMD, SONIC_CR_LCAM);
-
- i = 0;
- while (i++ < 100) {
- if (SONIC_READ(SONIC_ISR) & SONIC_INT_LCD)
- break;
- }
- netif_dbg(lp, ifup, dev, "%s: CMD=%x, ISR=%x, i=%d\n", __func__,
- SONIC_READ(SONIC_CMD), SONIC_READ(SONIC_ISR), i);
+ sonic_quiesce(dev, SONIC_CR_LCAM);
/*
* enable receiver, disable loopback
--
2.24.1
The tx_aborted_errors statistic should count packets flagged with EXD,
EXC, FU, or BCM bits because those bits denote an aborted transmission.
That corresponds to the bitmask 0x0446, not 0x0642. Use macros for these
constants to avoid mistakes. Better to leave out FIFO Underruns (FU) as
there's a separate counter for that purpose.
Don't lump all these errors in with the general tx_errors counter as
that's used for tx timeout events.
On the rx side, don't count RDE and RBAE interrupts as dropped packets.
These interrupts don't indicate a lost packet, just a lack of resources.
When a lack of resources results in a lost packet, this gets reported
in the rx_missed_errors counter (along with RFO events).
Don't double-count rx_frame_errors and rx_crc_errors.
Don't use the general rx_errors counter for events that already have
special counters.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Tested-by: Stan Johnson <[email protected]>
Signed-off-by: Finn Thain <[email protected]>
---
drivers/net/ethernet/natsemi/sonic.c | 21 +++++++--------------
drivers/net/ethernet/natsemi/sonic.h | 1 +
2 files changed, 8 insertions(+), 14 deletions(-)
diff --git a/drivers/net/ethernet/natsemi/sonic.c b/drivers/net/ethernet/natsemi/sonic.c
index 6660bd75b699..ea74c3c2c501 100644
--- a/drivers/net/ethernet/natsemi/sonic.c
+++ b/drivers/net/ethernet/natsemi/sonic.c
@@ -360,18 +360,19 @@ static irqreturn_t sonic_interrupt(int irq, void *dev_id)
if ((td_status = sonic_tda_get(dev, entry, SONIC_TD_STATUS)) == 0)
break;
- if (td_status & 0x0001) {
+ if (td_status & SONIC_TCR_PTX) {
lp->stats.tx_packets++;
lp->stats.tx_bytes += sonic_tda_get(dev, entry, SONIC_TD_PKTSIZE);
} else {
- lp->stats.tx_errors++;
- if (td_status & 0x0642)
+ if (td_status & (SONIC_TCR_EXD |
+ SONIC_TCR_EXC | SONIC_TCR_BCM))
lp->stats.tx_aborted_errors++;
- if (td_status & 0x0180)
+ if (td_status &
+ (SONIC_TCR_NCRS | SONIC_TCR_CRLS))
lp->stats.tx_carrier_errors++;
- if (td_status & 0x0020)
+ if (td_status & SONIC_TCR_OWC)
lp->stats.tx_window_errors++;
- if (td_status & 0x0004)
+ if (td_status & SONIC_TCR_FU)
lp->stats.tx_fifo_errors++;
}
@@ -401,17 +402,14 @@ static irqreturn_t sonic_interrupt(int irq, void *dev_id)
if (status & SONIC_INT_RFO) {
netif_dbg(lp, rx_err, dev, "%s: rx fifo overrun\n",
__func__);
- lp->stats.rx_fifo_errors++;
}
if (status & SONIC_INT_RDE) {
netif_dbg(lp, rx_err, dev, "%s: rx descriptors exhausted\n",
__func__);
- lp->stats.rx_dropped++;
}
if (status & SONIC_INT_RBAE) {
netif_dbg(lp, rx_err, dev, "%s: rx buffer area exceeded\n",
__func__);
- lp->stats.rx_dropped++;
}
/* counter overruns; all counters are 16bit wide */
@@ -503,11 +501,6 @@ static void sonic_rx(struct net_device *dev)
sonic_rra_put(dev, entry, SONIC_RR_BUFADR_H, bufadr_h);
} else {
/* This should only happen, if we enable accepting broken packets. */
- lp->stats.rx_errors++;
- if (status & SONIC_RCR_FAER)
- lp->stats.rx_frame_errors++;
- if (status & SONIC_RCR_CRCR)
- lp->stats.rx_crc_errors++;
}
if (status & SONIC_RCR_LPKT) {
/*
diff --git a/drivers/net/ethernet/natsemi/sonic.h b/drivers/net/ethernet/natsemi/sonic.h
index f4aa1e4159da..227975eb4cb8 100644
--- a/drivers/net/ethernet/natsemi/sonic.h
+++ b/drivers/net/ethernet/natsemi/sonic.h
@@ -175,6 +175,7 @@
#define SONIC_TCR_NCRS 0x0100
#define SONIC_TCR_CRLS 0x0080
#define SONIC_TCR_EXC 0x0040
+#define SONIC_TCR_OWC 0x0020
#define SONIC_TCR_PMB 0x0008
#define SONIC_TCR_FU 0x0004
#define SONIC_TCR_BCM 0x0002
--
2.24.1
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 <[email protected]>
Signed-off-by: Finn Thain <[email protected]>
---
drivers/net/ethernet/natsemi/sonic.c | 38 +++++++++++++++++++---------
1 file changed, 26 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/natsemi/sonic.c b/drivers/net/ethernet/natsemi/sonic.c
index 3cf84de8ad8e..dbbbc8bc72ff 100644
--- a/drivers/net/ethernet/natsemi/sonic.c
+++ b/drivers/net/ethernet/natsemi/sonic.c
@@ -242,7 +242,7 @@ static void sonic_tx_timeout(struct net_device *dev)
* 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,
+ * A spin lock is needed to make this work on SMP platforms. However,
* MIPS Jazz machines and m68k Macs were all uni-processor machines.
*/
@@ -252,6 +252,7 @@ static int sonic_send_packet(struct sk_buff *skb, struct net_device *dev)
dma_addr_t laddr;
int length;
int entry;
+ unsigned long flags;
netif_dbg(lp, tx_queued, dev, "%s: skb=%p\n", __func__, skb);
@@ -273,6 +274,8 @@ static int sonic_send_packet(struct sk_buff *skb, struct net_device *dev)
return NETDEV_TX_OK;
}
+ local_irq_save(flags);
+
entry = (lp->eol_tx + 1) & SONIC_TDS_MASK;
sonic_tda_put(dev, entry, SONIC_TD_STATUS, 0); /* clear status */
@@ -284,10 +287,6 @@ static int sonic_send_packet(struct sk_buff *skb, struct net_device *dev)
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;
@@ -310,6 +309,8 @@ static int sonic_send_packet(struct sk_buff *skb, struct net_device *dev)
SONIC_WRITE(SONIC_CMD, SONIC_CR_TXP);
+ local_irq_restore(flags);
+
return NETDEV_TX_OK;
}
@@ -322,9 +323,16 @@ static irqreturn_t sonic_interrupt(int irq, void *dev_id)
struct net_device *dev = dev_id;
struct sonic_local *lp = netdev_priv(dev);
int status;
+ unsigned long flags;
+
+ local_irq_save(flags);
+
+ status = SONIC_READ(SONIC_ISR) & SONIC_IMR_DEFAULT;
+ if (!status) {
+ local_irq_restore(flags);
- if (!(status = SONIC_READ(SONIC_ISR) & SONIC_IMR_DEFAULT))
return IRQ_NONE;
+ }
do {
if (status & SONIC_INT_PKTRX) {
@@ -338,11 +346,12 @@ static irqreturn_t sonic_interrupt(int irq, void *dev_id)
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
*/
netif_dbg(lp, intr, dev, "%s: tx done\n", __func__);
@@ -444,7 +453,12 @@ static irqreturn_t sonic_interrupt(int irq, void *dev_id)
/* 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);
+
+ local_irq_restore(flags);
+
return IRQ_HANDLED;
}
--
2.24.1
The driver accesses descriptor memory which is simultaneously accessed by
the chip, so the compiler must not be allowed to re-order CPU accesses.
sonic_buf_get() used 'volatile' to prevent that. sonic_buf_put() should
have done so too but was overlooked.
Fixes: efcce839360f ("[PATCH] macsonic/jazzsonic network drivers update")
Tested-by: Stan Johnson <[email protected]>
Signed-off-by: Finn Thain <[email protected]>
---
drivers/net/ethernet/natsemi/sonic.h | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/natsemi/sonic.h b/drivers/net/ethernet/natsemi/sonic.h
index f919835a9353..f4aa1e4159da 100644
--- a/drivers/net/ethernet/natsemi/sonic.h
+++ b/drivers/net/ethernet/natsemi/sonic.h
@@ -344,30 +344,30 @@ static int sonic_alloc_descriptors(struct net_device *dev);
as far as we can tell. */
/* OpenBSD calls this "SWO". I'd like to think that sonic_buf_put()
is a much better name. */
-static inline void sonic_buf_put(void* base, int bitmode,
+static inline void sonic_buf_put(u16 *base, int bitmode,
int offset, __u16 val)
{
if (bitmode)
#ifdef __BIG_ENDIAN
- ((__u16 *) base + (offset*2))[1] = val;
+ __raw_writew(val, base + (offset * 2) + 1);
#else
- ((__u16 *) base + (offset*2))[0] = val;
+ __raw_writew(val, base + (offset * 2) + 0);
#endif
else
- ((__u16 *) base)[offset] = val;
+ __raw_writew(val, base + (offset * 1) + 0);
}
-static inline __u16 sonic_buf_get(void* base, int bitmode,
+static inline __u16 sonic_buf_get(u16 *base, int bitmode,
int offset)
{
if (bitmode)
#ifdef __BIG_ENDIAN
- return ((volatile __u16 *) base + (offset*2))[1];
+ return __raw_readw(base + (offset * 2) + 1);
#else
- return ((volatile __u16 *) base + (offset*2))[0];
+ return __raw_readw(base + (offset * 2) + 0);
#endif
else
- return ((volatile __u16 *) base)[offset];
+ return __raw_readw(base + (offset * 1) + 0);
}
/* Inlines that you should actually use for reading/writing DMA buffers */
--
2.24.1
The eol_tx variable is the one that matters to the tx algorithm because
packets are always placed at the end of the list. The next_tx variable
just confuses things so remove it.
Tested-by: Stan Johnson <[email protected]>
Signed-off-by: Finn Thain <[email protected]>
---
drivers/net/ethernet/natsemi/sonic.c | 10 ++++++----
drivers/net/ethernet/natsemi/sonic.h | 1 -
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/natsemi/sonic.c b/drivers/net/ethernet/natsemi/sonic.c
index 657b5327baf9..f31cb19ded50 100644
--- a/drivers/net/ethernet/natsemi/sonic.c
+++ b/drivers/net/ethernet/natsemi/sonic.c
@@ -215,7 +215,7 @@ static int sonic_send_packet(struct sk_buff *skb, struct net_device *dev)
struct sonic_local *lp = netdev_priv(dev);
dma_addr_t laddr;
int length;
- int entry = lp->next_tx;
+ int entry;
netif_dbg(lp, tx_queued, dev, "%s: skb=%p\n", __func__, skb);
@@ -237,6 +237,8 @@ static int sonic_send_packet(struct sk_buff *skb, struct net_device *dev)
return NETDEV_TX_OK;
}
+ entry = (lp->eol_tx + 1) & SONIC_TDS_MASK;
+
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 */
@@ -260,8 +262,8 @@ static int sonic_send_packet(struct sk_buff *skb, struct net_device *dev)
sonic_tda_get(dev, lp->eol_tx, SONIC_TD_LINK) & ~SONIC_EOL);
lp->eol_tx = entry;
- lp->next_tx = (entry + 1) & SONIC_TDS_MASK;
- if (lp->tx_skb[lp->next_tx] != NULL) {
+ entry = (entry + 1) & SONIC_TDS_MASK;
+ if (lp->tx_skb[entry]) {
/* The ring is full, the ISR has yet to process the next TD. */
netif_dbg(lp, tx_queued, dev, "%s: stopping queue\n", __func__);
netif_stop_queue(dev);
@@ -684,7 +686,7 @@ static int sonic_init(struct net_device *dev)
SONIC_WRITE(SONIC_UTDA, lp->tda_laddr >> 16);
SONIC_WRITE(SONIC_CTDA, lp->tda_laddr & 0xffff);
- lp->cur_tx = lp->next_tx = 0;
+ lp->cur_tx = 0;
lp->eol_tx = SONIC_NUM_TDS - 1;
/*
diff --git a/drivers/net/ethernet/natsemi/sonic.h b/drivers/net/ethernet/natsemi/sonic.h
index 2b27f7049acb..e402dc29d2aa 100644
--- a/drivers/net/ethernet/natsemi/sonic.h
+++ b/drivers/net/ethernet/natsemi/sonic.h
@@ -318,7 +318,6 @@ struct sonic_local {
unsigned int cur_tx; /* first unacked transmit packet */
unsigned int eol_rx;
unsigned int eol_tx; /* last unacked transmit packet */
- unsigned int next_tx; /* next free TD */
int msg_enable;
struct device *device; /* generic device */
struct net_device_stats stats;
--
2.24.1
On m68k, local irqs remain enabled while interrupt handlers execute.
Therefore the macsonic driver has had to disable interrupts to avoid
re-entering sonic_interrupt(). With the preceding patch,
sonic_interrupt() was made re-entrant, which means its wrapper is now
redundant.
Tested-by: Stan Johnson <[email protected]>
Signed-off-by: Finn Thain <[email protected]>
---
drivers/net/ethernet/natsemi/macsonic.c | 19 ++++---------------
1 file changed, 4 insertions(+), 15 deletions(-)
diff --git a/drivers/net/ethernet/natsemi/macsonic.c b/drivers/net/ethernet/natsemi/macsonic.c
index 0f4d0c25d626..1b5559aacb38 100644
--- a/drivers/net/ethernet/natsemi/macsonic.c
+++ b/drivers/net/ethernet/natsemi/macsonic.c
@@ -114,17 +114,6 @@ static inline void bit_reverse_addr(unsigned char addr[6])
addr[i] = bitrev8(addr[i]);
}
-static irqreturn_t macsonic_interrupt(int irq, void *dev_id)
-{
- irqreturn_t result;
- unsigned long flags;
-
- local_irq_save(flags);
- result = sonic_interrupt(irq, dev_id);
- local_irq_restore(flags);
- return result;
-}
-
static int macsonic_open(struct net_device* dev)
{
int retval;
@@ -135,12 +124,12 @@ static int macsonic_open(struct net_device* dev)
dev->name, dev->irq);
goto err;
}
- /* Under the A/UX interrupt scheme, the onboard SONIC interrupt comes
- * in at priority level 3. However, we sometimes get the level 2 inter-
- * rupt as well, which must prevent re-entrance of the sonic handler.
+ /* Under the A/UX interrupt scheme, the onboard SONIC interrupt gets
+ * moved from level 2 to level 3. Unfortunately we still get some
+ * level 2 interrupts so register the handler for both.
*/
if (dev->irq == IRQ_AUTO_3) {
- retval = request_irq(IRQ_NUBUS_9, macsonic_interrupt, 0,
+ retval = request_irq(IRQ_NUBUS_9, sonic_interrupt, 0,
"sonic", dev);
if (retval) {
printk(KERN_ERR "%s: unable to get IRQ %d.\n",
--
2.24.1
No functional change.
Tested-by: Stan Johnson <[email protected]>
Signed-off-by: Finn Thain <[email protected]>
---
drivers/net/ethernet/natsemi/jazzsonic.c | 31 ++----------------
drivers/net/ethernet/natsemi/macsonic.c | 29 ++---------------
drivers/net/ethernet/natsemi/sonic.c | 36 +++++++++++++++++++++
drivers/net/ethernet/natsemi/sonic.h | 1 +
drivers/net/ethernet/natsemi/xtsonic.c | 40 ++----------------------
5 files changed, 44 insertions(+), 93 deletions(-)
diff --git a/drivers/net/ethernet/natsemi/jazzsonic.c b/drivers/net/ethernet/natsemi/jazzsonic.c
index 51fa82b429a3..bfa0c0d39600 100644
--- a/drivers/net/ethernet/natsemi/jazzsonic.c
+++ b/drivers/net/ethernet/natsemi/jazzsonic.c
@@ -147,39 +147,12 @@ static int sonic_probe1(struct net_device *dev)
dev->dev_addr[i*2+1] = val >> 8;
}
- err = -ENOMEM;
-
- /* Initialize the device structure. */
-
lp->dma_bitmode = SONIC_BITMODE32;
- /* Allocate the entire chunk of memory for the descriptors.
- Note that this cannot cross a 64K boundary. */
- lp->descriptors = dma_alloc_coherent(lp->device,
- SIZEOF_SONIC_DESC *
- SONIC_BUS_SCALE(lp->dma_bitmode),
- &lp->descriptors_laddr,
- GFP_KERNEL);
- if (lp->descriptors == NULL)
+ err = sonic_alloc_descriptors(dev);
+ if (err)
goto out;
- /* Now set up the pointers to point to the appropriate places */
- lp->cda = lp->descriptors;
- lp->tda = lp->cda + (SIZEOF_SONIC_CDA
- * SONIC_BUS_SCALE(lp->dma_bitmode));
- lp->rda = lp->tda + (SIZEOF_SONIC_TD * SONIC_NUM_TDS
- * SONIC_BUS_SCALE(lp->dma_bitmode));
- lp->rra = lp->rda + (SIZEOF_SONIC_RD * SONIC_NUM_RDS
- * SONIC_BUS_SCALE(lp->dma_bitmode));
-
- lp->cda_laddr = lp->descriptors_laddr;
- lp->tda_laddr = lp->cda_laddr + (SIZEOF_SONIC_CDA
- * SONIC_BUS_SCALE(lp->dma_bitmode));
- lp->rda_laddr = lp->tda_laddr + (SIZEOF_SONIC_TD * SONIC_NUM_TDS
- * SONIC_BUS_SCALE(lp->dma_bitmode));
- lp->rra_laddr = lp->rda_laddr + (SIZEOF_SONIC_RD * SONIC_NUM_RDS
- * SONIC_BUS_SCALE(lp->dma_bitmode));
-
dev->netdev_ops = &sonic_netdev_ops;
dev->watchdog_timeo = TX_TIMEOUT;
diff --git a/drivers/net/ethernet/natsemi/macsonic.c b/drivers/net/ethernet/natsemi/macsonic.c
index 0937fc2a928e..0f4d0c25d626 100644
--- a/drivers/net/ethernet/natsemi/macsonic.c
+++ b/drivers/net/ethernet/natsemi/macsonic.c
@@ -186,33 +186,10 @@ static const struct net_device_ops macsonic_netdev_ops = {
static int macsonic_init(struct net_device *dev)
{
struct sonic_local* lp = netdev_priv(dev);
+ int err = sonic_alloc_descriptors(dev);
- /* Allocate the entire chunk of memory for the descriptors.
- Note that this cannot cross a 64K boundary. */
- lp->descriptors = dma_alloc_coherent(lp->device,
- SIZEOF_SONIC_DESC *
- SONIC_BUS_SCALE(lp->dma_bitmode),
- &lp->descriptors_laddr,
- GFP_KERNEL);
- if (lp->descriptors == NULL)
- return -ENOMEM;
-
- /* Now set up the pointers to point to the appropriate places */
- lp->cda = lp->descriptors;
- lp->tda = lp->cda + (SIZEOF_SONIC_CDA
- * SONIC_BUS_SCALE(lp->dma_bitmode));
- lp->rda = lp->tda + (SIZEOF_SONIC_TD * SONIC_NUM_TDS
- * SONIC_BUS_SCALE(lp->dma_bitmode));
- lp->rra = lp->rda + (SIZEOF_SONIC_RD * SONIC_NUM_RDS
- * SONIC_BUS_SCALE(lp->dma_bitmode));
-
- lp->cda_laddr = lp->descriptors_laddr;
- lp->tda_laddr = lp->cda_laddr + (SIZEOF_SONIC_CDA
- * SONIC_BUS_SCALE(lp->dma_bitmode));
- lp->rda_laddr = lp->tda_laddr + (SIZEOF_SONIC_TD * SONIC_NUM_TDS
- * SONIC_BUS_SCALE(lp->dma_bitmode));
- lp->rra_laddr = lp->rda_laddr + (SIZEOF_SONIC_RD * SONIC_NUM_RDS
- * SONIC_BUS_SCALE(lp->dma_bitmode));
+ if (err)
+ return err;
dev->netdev_ops = &macsonic_netdev_ops;
dev->watchdog_timeo = TX_TIMEOUT;
diff --git a/drivers/net/ethernet/natsemi/sonic.c b/drivers/net/ethernet/natsemi/sonic.c
index f31cb19ded50..3cf84de8ad8e 100644
--- a/drivers/net/ethernet/natsemi/sonic.c
+++ b/drivers/net/ethernet/natsemi/sonic.c
@@ -50,6 +50,42 @@ static void sonic_msg_init(struct net_device *dev)
netif_dbg(lp, drv, dev, "%s", version);
}
+static int sonic_alloc_descriptors(struct net_device *dev)
+{
+ struct sonic_local *lp = netdev_priv(dev);
+
+ /* Allocate a chunk of memory for the descriptors. Note that this
+ * must not cross a 64K boundary. It is smaller than one page which
+ * means that page alignment is a sufficient condition.
+ */
+ lp->descriptors =
+ dma_alloc_coherent(lp->device,
+ SIZEOF_SONIC_DESC *
+ SONIC_BUS_SCALE(lp->dma_bitmode),
+ &lp->descriptors_laddr, GFP_KERNEL);
+
+ if (!lp->descriptors)
+ return -ENOMEM;
+
+ lp->cda = lp->descriptors;
+ lp->tda = lp->cda + SIZEOF_SONIC_CDA *
+ SONIC_BUS_SCALE(lp->dma_bitmode);
+ lp->rda = lp->tda + SIZEOF_SONIC_TD * SONIC_NUM_TDS *
+ SONIC_BUS_SCALE(lp->dma_bitmode);
+ lp->rra = lp->rda + SIZEOF_SONIC_RD * SONIC_NUM_RDS *
+ SONIC_BUS_SCALE(lp->dma_bitmode);
+
+ lp->cda_laddr = lp->descriptors_laddr;
+ lp->tda_laddr = lp->cda_laddr + SIZEOF_SONIC_CDA *
+ SONIC_BUS_SCALE(lp->dma_bitmode);
+ lp->rda_laddr = lp->tda_laddr + SIZEOF_SONIC_TD * SONIC_NUM_TDS *
+ SONIC_BUS_SCALE(lp->dma_bitmode);
+ lp->rra_laddr = lp->rda_laddr + SIZEOF_SONIC_RD * SONIC_NUM_RDS *
+ SONIC_BUS_SCALE(lp->dma_bitmode);
+
+ return 0;
+}
+
/*
* Open/initialize the SONIC controller.
*
diff --git a/drivers/net/ethernet/natsemi/sonic.h b/drivers/net/ethernet/natsemi/sonic.h
index e402dc29d2aa..f919835a9353 100644
--- a/drivers/net/ethernet/natsemi/sonic.h
+++ b/drivers/net/ethernet/natsemi/sonic.h
@@ -337,6 +337,7 @@ static void sonic_multicast_list(struct net_device *dev);
static int sonic_init(struct net_device *dev);
static void sonic_tx_timeout(struct net_device *dev);
static void sonic_msg_init(struct net_device *dev);
+static int sonic_alloc_descriptors(struct net_device *dev);
/* Internal inlines for reading/writing DMA buffers. Note that bus
size and endianness matter here, whereas they don't for registers,
diff --git a/drivers/net/ethernet/natsemi/xtsonic.c b/drivers/net/ethernet/natsemi/xtsonic.c
index e1b886e87a76..dda9ec7d9cee 100644
--- a/drivers/net/ethernet/natsemi/xtsonic.c
+++ b/drivers/net/ethernet/natsemi/xtsonic.c
@@ -167,47 +167,11 @@ static int __init sonic_probe1(struct net_device *dev)
dev->dev_addr[i*2+1] = val >> 8;
}
- /* Initialize the device structure. */
-
lp->dma_bitmode = SONIC_BITMODE32;
- /*
- * Allocate local private descriptor areas in uncached space.
- * The entire structure must be located within the same 64kb segment.
- * A simple way to ensure this is to allocate twice the
- * size of the structure -- given that the structure is
- * much less than 64 kB, at least one of the halves of
- * the allocated area will be contained entirely in 64 kB.
- * We also allocate extra space for a pointer to allow freeing
- * this structure later on (in xtsonic_cleanup_module()).
- */
- lp->descriptors = dma_alloc_coherent(lp->device,
- SIZEOF_SONIC_DESC *
- SONIC_BUS_SCALE(lp->dma_bitmode),
- &lp->descriptors_laddr,
- GFP_KERNEL);
- if (lp->descriptors == NULL) {
- err = -ENOMEM;
+ err = sonic_alloc_descriptors(dev);
+ if (err)
goto out;
- }
-
- lp->cda = lp->descriptors;
- lp->tda = lp->cda + (SIZEOF_SONIC_CDA
- * SONIC_BUS_SCALE(lp->dma_bitmode));
- lp->rda = lp->tda + (SIZEOF_SONIC_TD * SONIC_NUM_TDS
- * SONIC_BUS_SCALE(lp->dma_bitmode));
- lp->rra = lp->rda + (SIZEOF_SONIC_RD * SONIC_NUM_RDS
- * SONIC_BUS_SCALE(lp->dma_bitmode));
-
- /* get the virtual dma address */
-
- lp->cda_laddr = lp->descriptors_laddr;
- lp->tda_laddr = lp->cda_laddr + (SIZEOF_SONIC_CDA
- * SONIC_BUS_SCALE(lp->dma_bitmode));
- lp->rda_laddr = lp->tda_laddr + (SIZEOF_SONIC_TD * SONIC_NUM_TDS
- * SONIC_BUS_SCALE(lp->dma_bitmode));
- lp->rra_laddr = lp->rda_laddr + (SIZEOF_SONIC_RD * SONIC_NUM_RDS
- * SONIC_BUS_SCALE(lp->dma_bitmode));
dev->netdev_ops = &xtsonic_netdev_ops;
dev->watchdog_timeo = TX_TIMEOUT;
--
2.24.1
Hi Finn,
On Mon, Jan 20, 2020 at 12:19 AM Finn Thain <[email protected]> wrote:
> 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 <[email protected]>
> Signed-off-by: Finn Thain <[email protected]>
Thanks for your patch!
> --- a/drivers/net/ethernet/natsemi/sonic.c
> +++ b/drivers/net/ethernet/natsemi/sonic.c
> @@ -242,7 +242,7 @@ static void sonic_tx_timeout(struct net_device *dev)
> * 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,
> + * A spin lock is needed to make this work on SMP platforms. However,
> * MIPS Jazz machines and m68k Macs were all uni-processor machines.
> */
>
> @@ -252,6 +252,7 @@ static int sonic_send_packet(struct sk_buff *skb, struct net_device *dev)
> dma_addr_t laddr;
> int length;
> int entry;
> + unsigned long flags;
>
> netif_dbg(lp, tx_queued, dev, "%s: skb=%p\n", __func__, skb);
>
> @@ -273,6 +274,8 @@ static int sonic_send_packet(struct sk_buff *skb, struct net_device *dev)
> return NETDEV_TX_OK;
> }
>
> + local_irq_save(flags);
> +
Wouldn't it be better to use a spinlock instead?
It looks like all currently supported platforms (Mac, Jazz, and XT2000)
do no support SMP, but I'm not 100% sure about the latter.
And this generic sonic.c core may end up being used on other platforms
that do support SMP.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
This is a mix of cleanups and other things and definitely not bug fixes.
Please separate out the true actual bug fixes from the cleanups.
The bug fixes get submitted to 'net'
And the rest go to 'net-next'
Thank you.
On Mon, 20 Jan 2020, Geert Uytterhoeven wrote:
> Hi Finn,
>
> On Mon, Jan 20, 2020 at 12:19 AM Finn Thain <[email protected]> wrote:
> > 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 <[email protected]>
> > Signed-off-by: Finn Thain <[email protected]>
>
> Thanks for your patch!
>
> > --- a/drivers/net/ethernet/natsemi/sonic.c
> > +++ b/drivers/net/ethernet/natsemi/sonic.c
> > @@ -242,7 +242,7 @@ static void sonic_tx_timeout(struct net_device *dev)
> > * 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,
> > + * A spin lock is needed to make this work on SMP platforms. However,
> > * MIPS Jazz machines and m68k Macs were all uni-processor machines.
> > */
> >
> > @@ -252,6 +252,7 @@ static int sonic_send_packet(struct sk_buff *skb, struct net_device *dev)
> > dma_addr_t laddr;
> > int length;
> > int entry;
> > + unsigned long flags;
> >
> > netif_dbg(lp, tx_queued, dev, "%s: skb=%p\n", __func__, skb);
> >
> > @@ -273,6 +274,8 @@ static int sonic_send_packet(struct sk_buff *skb, struct net_device *dev)
> > return NETDEV_TX_OK;
> > }
> >
> > + local_irq_save(flags);
> > +
>
> Wouldn't it be better to use a spinlock instead?
Yes, better in the sense of "more portable". And worse in the sense of
"needless complexity".
> It looks like all currently supported platforms (Mac, Jazz, and XT2000)
> do no support SMP, but I'm not 100% sure about the latter. And this
> generic sonic.c core may end up being used on other platforms that do
> support SMP.
>
I'm not sure about XT2000 either. It would be surprising if they
overlooked this. But I'll add the spinlock, just in case.
Thanks for your review.
> Gr{oetje,eeting}s,
>
> Geert
>
>
On Mon, 20 Jan 2020, David Miller wrote:
>
> This is a mix of cleanups and other things and definitely not bug fixes.
>
That's true. Sorry if the subject line was somehow misleading. That was
not intentional.
> Please separate out the true actual bug fixes from the cleanups.
>
> The bug fixes get submitted to 'net'
>
> And the rest go to 'net-next'
>
> Thank you.
>
OK.