[PATCH 1/2] 8139cp: Use dev_kfree_skb_any() instead of dev_kfree_skb() in cp_clean_rings()

[PATCH 1/2] 8139cp: Use dev_kfree_skb_any() instead of dev_kfree_skb() in cp_clean_rings()

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1256 bytes --]

This can be called from cp_tx_timeout() with interrupts disabled.
Spotted by Francois Romieu <romieu@fr.zoreil.com>

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
 drivers/net/ethernet/realtek/8139cp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index d79e33b..52a5334 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -1151,7 +1151,7 @@ static void cp_clean_rings (struct cp_private *cp)
 			desc = cp->rx_ring + i;
 			dma_unmap_single(&cp->pdev->dev,le64_to_cpu(desc->addr),
 					 cp->rx_buf_sz, PCI_DMA_FROMDEVICE);
-			dev_kfree_skb(cp->rx_skb[i]);
+			dev_kfree_skb_any(cp->rx_skb[i]);
 		}
 	}
 
@@ -1164,7 +1164,7 @@ static void cp_clean_rings (struct cp_private *cp)
 					 le32_to_cpu(desc->opts1) & 0xffff,
 					 PCI_DMA_TODEVICE);
 			if (le32_to_cpu(desc->opts1) & LastFrag)
-				dev_kfree_skb(skb);
+				dev_kfree_skb_any(skb);
 			cp->dev->stats.tx_dropped++;
 		}
 	}
-- 
2.4.3

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

[PATCH 2/2] 8139cp: Call __cp_set_rx_mode() from cp_tx_timeout()

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1366 bytes --]

Unless we reset the RX config, on real hardware I don't seem to receive
any packets after a TX timeout.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
Now it does actually recover from the TX timeout, lots of the time.
Sometimes it still hits that IRQ storm, which probably explains the
apparent lockup right after the 'popf'... although I thought we handled
it more gracefully than that these days.

That's probably a race with the RX handling code, or something. I'll
try harder to reproduce the TX timeout with the debugging enabled.
Which might shed some light on this, and also on the reason why it
happens in the first place. If we're lucky.

 drivers/net/ethernet/realtek/8139cp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index 52a5334..ba3dab7 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -1261,6 +1261,7 @@ static void cp_tx_timeout(struct net_device *dev)
 	cp_clean_rings(cp);
 	rc = cp_init_rings(cp);
 	cp_start_hw(cp);
+	__cp_set_rx_mode(dev);
 	cp_enable_irq(cp);
 
 	netif_wake_queue(dev);
-- 
2.4.3


-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

[PATCH 3/2] 8139cp: Improve accuracy of cp_interrupt() return, to survive IRQ storms

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 3525 bytes --]

The TX timeout handling has been observed to trigger RX IRQ storms. And
since cp_interrupt() just keeps saying that it handled the interrupt,
the machine then dies. Fix the return value from cp_interrupt(), and
the offending IRQ gets disabled and the machine survives.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
... and I have to make fewer trips to the cupboard under the stairs.
There follows a fix to prevent the IRQ storm from happening at all.

 drivers/net/ethernet/realtek/8139cp.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index ba3dab7..f1054ad 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -371,7 +371,7 @@ struct cp_private {
 
 
 static void __cp_set_rx_mode (struct net_device *dev);
-static void cp_tx (struct cp_private *cp);
+static int cp_tx (struct cp_private *cp);
 static void cp_clean_rings (struct cp_private *cp);
 #ifdef CONFIG_NET_POLL_CONTROLLER
 static void cp_poll_controller(struct net_device *dev);
@@ -587,8 +587,6 @@ static irqreturn_t cp_interrupt (int irq, void *dev_instance)
 	if (!status || (status == 0xFFFF))
 		goto out_unlock;
 
-	handled = 1;
-
 	netif_dbg(cp, intr, dev, "intr, status %04x cmd %02x cpcmd %04x\n",
 		  status, cpr8(Cmd), cpr16(CpCmd));
 
@@ -596,25 +594,30 @@ static irqreturn_t cp_interrupt (int irq, void *dev_instance)
 
 	/* close possible race's with dev_close */
 	if (unlikely(!netif_running(dev))) {
+		handled = 1;
 		cpw16(IntrMask, 0);
 		goto out_unlock;
 	}
 
-	if (status & (RxOK | RxErr | RxEmpty | RxFIFOOvr))
+	if (status & (RxOK | RxErr | RxEmpty | RxFIFOOvr)) {
 		if (napi_schedule_prep(&cp->napi)) {
+			handled = 1;
 			cpw16_f(IntrMask, cp_norx_intr_mask);
 			__napi_schedule(&cp->napi);
 		}
-
+	}
 	if (status & (TxOK | TxErr | TxEmpty | SWInt))
-		cp_tx(cp);
-	if (status & LinkChg)
-		mii_check_media(&cp->mii_if, netif_msg_link(cp), false);
+		handled |= cp_tx(cp);
 
+	if (status & LinkChg) {
+		handled = 1;
+		mii_check_media(&cp->mii_if, netif_msg_link(cp), false);
+	}
 
 	if (status & PciErr) {
 		u16 pci_status;
 
+		handled = 1;
 		pci_read_config_word(cp->pdev, PCI_STATUS, &pci_status);
 		pci_write_config_word(cp->pdev, PCI_STATUS, pci_status);
 		netdev_err(dev, "PCI bus error, status=%04x, PCI status=%04x\n",
@@ -645,11 +648,12 @@ static void cp_poll_controller(struct net_device *dev)
 }
 #endif
 
-static void cp_tx (struct cp_private *cp)
+static int cp_tx (struct cp_private *cp)
 {
 	unsigned tx_head = cp->tx_head;
 	unsigned tx_tail = cp->tx_tail;
 	unsigned bytes_compl = 0, pkts_compl = 0;
+	int handled = 0;
 
 	while (tx_tail != tx_head) {
 		struct cp_desc *txd = cp->tx_ring + tx_tail;
@@ -661,6 +665,7 @@ static void cp_tx (struct cp_private *cp)
 		if (status & DescOwn)
 			break;
 
+		handled = 1;
 		skb = cp->tx_skb[tx_tail];
 		BUG_ON(!skb);
 
@@ -704,6 +709,8 @@ static void cp_tx (struct cp_private *cp)
 	netdev_completed_queue(cp->dev, pkts_compl, bytes_compl);
 	if (TX_BUFFS_AVAIL(cp) > (MAX_SKB_FRAGS + 1))
 		netif_wake_queue(cp->dev);
+
+	return handled;
 }
 
 static inline u32 cp_tx_vlan_tag(struct sk_buff *skb)
-- 
2.4.3

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

[PATCH 4/2] 8139cp: Do not re-enable RX interrupts in cp_tx_timeout()

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 2238 bytes --]

If an RX interrupt was already received but NAPI has not yet run when
the RX timeout happens, we end up in cp_tx_timeout() with RX interrupts
already disabled. Blindly re-enabling them will cause an IRQ storm.

This is somewhat less painful than it was a few minutes ago before I
fixed the return value from cp_interrupt(), but still suboptimal.

Unconditionally leave RX interrupts disabled after the reset, and
schedule NAPI to check the receive ring and re-enable them.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
It might seem a little odd to deliberately schedule NAPI when we know
there's going to be nothing there since we just cleared the rings.

But given that we only get here if the hardware is playing silly
buggers, I don't much like the idea of preserving the existing IntrMask
that we read from the hardware. And there's no pretty way of
determining whether NAPI is already scheduled. So just ensuring that
it *is* scheduled seems simplest. It's not like we're going to
care about the performance implications of one fruitless poll when
we've just suffered a TX timeout and reset the hardware.

In future I think I might want to fix cp_tx_timeout() to *only* reset
the TX ring anyway. Especially as I'm entirely unsure about its locking
w.r.t. cp_rx_poll()... what prevents cp_rx_poll() from running while
we're in the middle of screwing around with the RX rings?

 drivers/net/ethernet/realtek/8139cp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/realtek/8139cp.c
b/drivers/net/ethernet/realtek/8139cp.c
index f1054ad..d12fc50 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -1269,9 +1269,10 @@ static void cp_tx_timeout(struct net_device
*dev)
 	rc = cp_init_rings(cp);
 	cp_start_hw(cp);
 	__cp_set_rx_mode(dev);
-	cp_enable_irq(cp);
+	cpw16_f(IntrMask, cp_norx_intr_mask);
 
 	netif_wake_queue(dev);
+	napi_schedule_irqoff(&cp->napi);
 
 	spin_unlock_irqrestore(&cp->lock, flags);
 }
-- 
2.4.3

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

Re: [PATCH 1/2] 8139cp: Use dev_kfree_skb_any() instead of dev_kfree_skb() in cp_clean_rings()

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Fri, 18 Sep 2015 00:19:08 +0100

> This can be called from cp_tx_timeout() with interrupts disabled.
> Spotted by Francois Romieu <romieu@fr.zoreil.com>
> 
> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>

Applied.

Re: [PATCH 2/2] 8139cp: Call __cp_set_rx_mode() from cp_tx_timeout()

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Fri, 18 Sep 2015 00:21:54 +0100

> Unless we reset the RX config, on real hardware I don't seem to receive
> any packets after a TX timeout.
> 
> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>

Applied.

Re: [PATCH 2/2] 8139cp: Call __cp_set_rx_mode() from cp_tx_timeout()

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 798 bytes --]

On Sun, 2015-09-20 at 22:24 -0700, David Miller wrote:
> From: David Woodhouse <dwmw2@infradead.org>
> Date: Fri, 18 Sep 2015 00:21:54 +0100
> 
> > Unless we reset the RX config, on real hardware I don't seem to
> receive
> > any packets after a TX timeout.
> > 
> > Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> 
> Applied.

Thanks. I'll send another batch, including the original patches 3/2 and
4/3 from this series, in reply to this message.

After which, I think we might be able to turn on TX checksumming by
default and I also have a way to implement early detection of the TX
stall I've been seeing.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

[PATCH 1/7] 8139cp: Improve accuracy of cp_interrupt() return, to survive IRQ storms

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 3437 bytes --]

From: David Woodhouse <David.Woodhouse@intel.com>

The TX timeout handling has been observed to trigger RX IRQ storms. And
since cp_interrupt() just keeps saying that it handled the interrupt,
the machine then dies. Fix the return value from cp_interrupt(), and
the offending IRQ gets disabled and the machine survives.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
 drivers/net/ethernet/realtek/8139cp.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index ba3dab7..f1054ad 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -371,7 +371,7 @@ struct cp_private {
 
 
 static void __cp_set_rx_mode (struct net_device *dev);
-static void cp_tx (struct cp_private *cp);
+static int cp_tx (struct cp_private *cp);
 static void cp_clean_rings (struct cp_private *cp);
 #ifdef CONFIG_NET_POLL_CONTROLLER
 static void cp_poll_controller(struct net_device *dev);
@@ -587,8 +587,6 @@ static irqreturn_t cp_interrupt (int irq, void *dev_instance)
 	if (!status || (status == 0xFFFF))
 		goto out_unlock;
 
-	handled = 1;
-
 	netif_dbg(cp, intr, dev, "intr, status %04x cmd %02x cpcmd %04x\n",
 		  status, cpr8(Cmd), cpr16(CpCmd));
 
@@ -596,25 +594,30 @@ static irqreturn_t cp_interrupt (int irq, void *dev_instance)
 
 	/* close possible race's with dev_close */
 	if (unlikely(!netif_running(dev))) {
+		handled = 1;
 		cpw16(IntrMask, 0);
 		goto out_unlock;
 	}
 
-	if (status & (RxOK | RxErr | RxEmpty | RxFIFOOvr))
+	if (status & (RxOK | RxErr | RxEmpty | RxFIFOOvr)) {
 		if (napi_schedule_prep(&cp->napi)) {
+			handled = 1;
 			cpw16_f(IntrMask, cp_norx_intr_mask);
 			__napi_schedule(&cp->napi);
 		}
-
+	}
 	if (status & (TxOK | TxErr | TxEmpty | SWInt))
-		cp_tx(cp);
-	if (status & LinkChg)
-		mii_check_media(&cp->mii_if, netif_msg_link(cp), false);
+		handled |= cp_tx(cp);
 
+	if (status & LinkChg) {
+		handled = 1;
+		mii_check_media(&cp->mii_if, netif_msg_link(cp), false);
+	}
 
 	if (status & PciErr) {
 		u16 pci_status;
 
+		handled = 1;
 		pci_read_config_word(cp->pdev, PCI_STATUS, &pci_status);
 		pci_write_config_word(cp->pdev, PCI_STATUS, pci_status);
 		netdev_err(dev, "PCI bus error, status=%04x, PCI status=%04x\n",
@@ -645,11 +648,12 @@ static void cp_poll_controller(struct net_device *dev)
 }
 #endif
 
-static void cp_tx (struct cp_private *cp)
+static int cp_tx (struct cp_private *cp)
 {
 	unsigned tx_head = cp->tx_head;
 	unsigned tx_tail = cp->tx_tail;
 	unsigned bytes_compl = 0, pkts_compl = 0;
+	int handled = 0;
 
 	while (tx_tail != tx_head) {
 		struct cp_desc *txd = cp->tx_ring + tx_tail;
@@ -661,6 +665,7 @@ static void cp_tx (struct cp_private *cp)
 		if (status & DescOwn)
 			break;
 
+		handled = 1;
 		skb = cp->tx_skb[tx_tail];
 		BUG_ON(!skb);
 
@@ -704,6 +709,8 @@ static void cp_tx (struct cp_private *cp)
 	netdev_completed_queue(cp->dev, pkts_compl, bytes_compl);
 	if (TX_BUFFS_AVAIL(cp) > (MAX_SKB_FRAGS + 1))
 		netif_wake_queue(cp->dev);
+
+	return handled;
 }
 
 static inline u32 cp_tx_vlan_tag(struct sk_buff *skb)
-- 
2.4.3

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

[PATCH 2/7] 8139cp: Do not re-enable RX interrupts in cp_tx_timeout()

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1408 bytes --]

From: David Woodhouse <David.Woodhouse@intel.com>

If an RX interrupt was already received but NAPI has not yet run when
the RX timeout happens, we end up in cp_tx_timeout() with RX interrupts
already disabled. Blindly re-enabling them will cause an IRQ storm.

This is somewhat less painful than it was a few minutes ago before I
fixed the return value from cp_interrupt(), but still suboptimal.

Unconditionally leave RX interrupts disabled after the reset, and
schedule NAPI to check the receive ring and re-enable them.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
 drivers/net/ethernet/realtek/8139cp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index f1054ad..d12fc50 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -1269,9 +1269,10 @@ static void cp_tx_timeout(struct net_device *dev)
 	rc = cp_init_rings(cp);
 	cp_start_hw(cp);
 	__cp_set_rx_mode(dev);
-	cp_enable_irq(cp);
+	cpw16_f(IntrMask, cp_norx_intr_mask);
 
 	netif_wake_queue(dev);
+	napi_schedule_irqoff(&cp->napi);
 
 	spin_unlock_irqrestore(&cp->lock, flags);
 }
-- 
2.4.3

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

[PATCH 3/7] 8139cp: Fix tx_queued debug message to print correct slot numbers

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 2399 bytes --]

From: David Woodhouse <David.Woodhouse@intel.com>

After a certain amount of staring at the debug output of this driver, I
realised it was lying to me.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
 drivers/net/ethernet/realtek/8139cp.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index d12fc50..058f835 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -793,7 +793,8 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 		wmb();
 
 		cp->tx_skb[entry] = skb;
-		entry = NEXT_TX(entry);
+		netif_dbg(cp, tx_queued, cp->dev, "tx queued, slot %d, skblen %d\n",
+			  entry, skb->len);
 	} else {
 		struct cp_desc *txd;
 		u32 first_len, first_eor;
@@ -812,7 +813,6 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 			goto out_dma_error;
 
 		cp->tx_skb[entry] = skb;
-		entry = NEXT_TX(entry);
 
 		for (frag = 0; frag < skb_shinfo(skb)->nr_frags; frag++) {
 			const skb_frag_t *this_frag = &skb_shinfo(skb)->frags[frag];
@@ -820,6 +820,8 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 			u32 ctrl;
 			dma_addr_t mapping;
 
+			entry = NEXT_TX(entry);
+
 			len = skb_frag_size(this_frag);
 			mapping = dma_map_single(&cp->pdev->dev,
 						 skb_frag_address(this_frag),
@@ -855,9 +857,7 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 
 			txd->opts1 = cpu_to_le32(ctrl);
 			wmb();
-
 			cp->tx_skb[entry] = skb;
-			entry = NEXT_TX(entry);
 		}
 
 		txd = &cp->tx_ring[first_entry];
@@ -880,12 +880,13 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 			txd->opts1 = cpu_to_le32(first_eor | first_len |
 						 FirstFrag | DescOwn);
 		wmb();
+
+		netif_dbg(cp, tx_queued, cp->dev, "tx queued, slots %d-%d, skblen %d\n",
+			  first_entry, entry, skb->len);
 	}
-	cp->tx_head = entry;
+	cp->tx_head = NEXT_TX(entry);
 
 	netdev_sent_queue(dev, skb->len);
-	netif_dbg(cp, tx_queued, cp->dev, "tx queued, slot %d, skblen %d\n",
-		  entry, skb->len);
 	if (TX_BUFFS_AVAIL(cp) <= (MAX_SKB_FRAGS + 1))
 		netif_stop_queue(dev);
 
-- 
2.4.3

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

[PATCH 4/7] 8139cp: Fix TSO/scatter-gather descriptor setup

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 2316 bytes --]

From: David Woodhouse <David.Woodhouse@intel.com>

When sending a TSO frame in multiple buffers, we were neglecting to set
the first descriptor up in TSO mode.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
 drivers/net/ethernet/realtek/8139cp.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index 058f835..07621b5 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -797,7 +797,7 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 			  entry, skb->len);
 	} else {
 		struct cp_desc *txd;
-		u32 first_len, first_eor;
+		u32 first_len, first_eor, ctrl;
 		dma_addr_t first_mapping;
 		int frag, first_entry = entry;
 		const struct iphdr *ip = ip_hdr(skb);
@@ -817,7 +817,6 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 		for (frag = 0; frag < skb_shinfo(skb)->nr_frags; frag++) {
 			const skb_frag_t *this_frag = &skb_shinfo(skb)->frags[frag];
 			u32 len;
-			u32 ctrl;
 			dma_addr_t mapping;
 
 			entry = NEXT_TX(entry);
@@ -865,20 +864,20 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 		txd->addr = cpu_to_le64(first_mapping);
 		wmb();
 
-		if (skb->ip_summed == CHECKSUM_PARTIAL) {
+		ctrl = first_eor | first_len | FirstFrag | DescOwn;
+		if (mss)
+			ctrl |= LargeSend |
+				((mss & MSSMask) << MSSShift);
+		else if (skb->ip_summed == CHECKSUM_PARTIAL) {
 			if (ip->protocol == IPPROTO_TCP)
-				txd->opts1 = cpu_to_le32(first_eor | first_len |
-							 FirstFrag | DescOwn |
-							 IPCS | TCPCS);
+				ctrl |= IPCS | TCPCS;
 			else if (ip->protocol == IPPROTO_UDP)
-				txd->opts1 = cpu_to_le32(first_eor | first_len |
-							 FirstFrag | DescOwn |
-							 IPCS | UDPCS);
+				ctrl |= IPCS | UDPCS;
 			else
 				BUG();
-		} else
-			txd->opts1 = cpu_to_le32(first_eor | first_len |
-						 FirstFrag | DescOwn);
+		}
+
+		txd->opts1 = cpu_to_le32(ctrl);
 		wmb();
 
 		netif_dbg(cp, tx_queued, cp->dev, "tx queued, slots %d-%d, skblen %d\n",
-- 
2.4.3

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

[PATCH 5/7] 8139cp: Fix DMA unmapping of transmitted buffers

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 3094 bytes --]

From: David Woodhouse <David.Woodhouse@intel.com>

The low 16 bits of the 'opts1' field in the TX descriptor are supposed
to still contain the buffer length when the descriptor is handed back to
us. In practice, at least on my hardware, they don't. So stash the
original value of the opts1 field and get the length to unmap from
there.

There are other ways we could have worked out the length, but I actually
want a stash of the opts1 field anyway so that I can dump it alongside
the contents of the descriptor ring when we suffer a TX timeout.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
 drivers/net/ethernet/realtek/8139cp.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index 07621b5..036ad85 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -341,6 +341,7 @@ struct cp_private {
 	unsigned		tx_tail;
 	struct cp_desc		*tx_ring;
 	struct sk_buff		*tx_skb[CP_TX_RING_SIZE];
+	u32			tx_opts[CP_TX_RING_SIZE];
 
 	unsigned		rx_buf_sz;
 	unsigned		wol_enabled : 1; /* Is Wake-on-LAN enabled? */
@@ -670,7 +671,7 @@ static int cp_tx (struct cp_private *cp)
 		BUG_ON(!skb);
 
 		dma_unmap_single(&cp->pdev->dev, le64_to_cpu(txd->addr),
-				 le32_to_cpu(txd->opts1) & 0xffff,
+				 cp->tx_opts[tx_tail] & 0xffff,
 				 PCI_DMA_TODEVICE);
 
 		if (status & LastFrag) {
@@ -793,6 +794,7 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 		wmb();
 
 		cp->tx_skb[entry] = skb;
+		cp->tx_opts[entry] = flags;
 		netif_dbg(cp, tx_queued, cp->dev, "tx queued, slot %d, skblen %d\n",
 			  entry, skb->len);
 	} else {
@@ -856,6 +858,8 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 
 			txd->opts1 = cpu_to_le32(ctrl);
 			wmb();
+
+			cp->tx_opts[entry] = ctrl;
 			cp->tx_skb[entry] = skb;
 		}
 
@@ -880,6 +884,7 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 		txd->opts1 = cpu_to_le32(ctrl);
 		wmb();
 
+		cp->tx_opts[first_entry] = ctrl;
 		netif_dbg(cp, tx_queued, cp->dev, "tx queued, slots %d-%d, skblen %d\n",
 			  first_entry, entry, skb->len);
 	}
@@ -1122,6 +1127,7 @@ static int cp_init_rings (struct cp_private *cp)
 {
 	memset(cp->tx_ring, 0, sizeof(struct cp_desc) * CP_TX_RING_SIZE);
 	cp->tx_ring[CP_TX_RING_SIZE - 1].opts1 = cpu_to_le32(RingEnd);
+	memset(cp->tx_opts, 0, sizeof(cp->tx_opts));
 
 	cp_init_rings_index(cp);
 
@@ -1179,6 +1185,7 @@ static void cp_clean_rings (struct cp_private *cp)
 
 	memset(cp->rx_ring, 0, sizeof(struct cp_desc) * CP_RX_RING_SIZE);
 	memset(cp->tx_ring, 0, sizeof(struct cp_desc) * CP_TX_RING_SIZE);
+	memset(cp->tx_opts, 0, sizeof(cp->tx_opts));
 
 	memset(cp->rx_skb, 0, sizeof(struct sk_buff *) * CP_RX_RING_SIZE);
 	memset(cp->tx_skb, 0, sizeof(struct sk_buff *) * CP_TX_RING_SIZE);
-- 
2.4.3

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

[PATCH 6/7] 8139cp: Dump contents of descriptor ring on TX timeout

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 2020 bytes --]

From: David Woodhouse <David.Woodhouse@intel.com>

We are seeing unexplained TX timeouts under heavy load. Let's try to get
a better idea of what's going on.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
 drivers/net/ethernet/realtek/8139cp.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index 036ad85..6feff9f 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -157,6 +157,7 @@ enum {
 	NWayAdvert	= 0x66, /* MII ADVERTISE */
 	NWayLPAR	= 0x68, /* MII LPA */
 	NWayExpansion	= 0x6A, /* MII Expansion */
+	TxDmaOkLowDesc  = 0x82, /* Low 16 bit address of a Tx descriptor. */
 	Config5		= 0xD8,	/* Config5 */
 	TxPoll		= 0xD9,	/* Tell chip to check Tx descriptors for work */
 	RxMaxSize	= 0xDA, /* Max size of an Rx packet (8169 only) */
@@ -1263,7 +1264,7 @@ static void cp_tx_timeout(struct net_device *dev)
 {
 	struct cp_private *cp = netdev_priv(dev);
 	unsigned long flags;
-	int rc;
+	int rc, i;
 
 	netdev_warn(dev, "Transmit timeout, status %2x %4x %4x %4x\n",
 		    cpr8(Cmd), cpr16(CpCmd),
@@ -1271,6 +1272,17 @@ static void cp_tx_timeout(struct net_device *dev)
 
 	spin_lock_irqsave(&cp->lock, flags);
 
+	netif_dbg(cp, tx_err, cp->dev, "TX ring head %d tail %d desc %x\n",
+		  cp->tx_head, cp->tx_tail, cpr16(TxDmaOkLowDesc));
+	for (i = 0; i < CP_TX_RING_SIZE; i++) {
+		netif_dbg(cp, tx_err, cp->dev,
+			  "TX slot %d @%p: %08x (%08x) %08x %llx %p\n",
+			  i, &cp->tx_ring[i], le32_to_cpu(cp->tx_ring[i].opts1),
+			  cp->tx_opts[i], le32_to_cpu(cp->tx_ring[i].opts2),
+			  le64_to_cpu(cp->tx_ring[i].addr),
+			  cp->tx_skb[i]);
+	}
+
 	cp_stop_hw(cp);
 	cp_clean_rings(cp);
 	rc = cp_init_rings(cp);
-- 
2.4.3

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

[PATCH 7/7] 8139cp: Avoid gratuitous writes to TxPoll register when already running

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 4215 bytes --]

From: David Woodhouse <David.Woodhouse@intel.com>

This is a minor optimisation, but as a side-effect it means we can know
precisely which descriptors were already in the ring when we last
prodded it to run.

This will give us a better chance to catch the case where we get a
TxEmpty interrupt when it hasn't actually finished the descriptors we
*know* it should have seen, before it ends up being a full-blown TX
timeout and reset.

Since QEMU's emulation doesn't give TxEmpty interrupts, *always* bash on
TxPoll until we see the first TxEmpty interrupt and cp->txempty_seen
gets set.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
I'm actually having second thoughts about this one since realising that
QEMU doesn't implement it correctly. The workaround isn't *that* horrid
but it's not clear it's enough of a performance win — or whether it's
entirely necessary for catching my TX stall.

 drivers/net/ethernet/realtek/8139cp.c | 37 +++++++++++++++++++++++++++++------
 1 file changed, 31 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index 6feff9f..67a4fcf 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -340,6 +340,9 @@ struct cp_private {
 
 	unsigned		tx_head		____cacheline_aligned;
 	unsigned		tx_tail;
+	unsigned		tx_running;
+	unsigned		txempty_seen;
+
 	struct cp_desc		*tx_ring;
 	struct sk_buff		*tx_skb[CP_TX_RING_SIZE];
 	u32			tx_opts[CP_TX_RING_SIZE];
@@ -611,6 +614,22 @@ static irqreturn_t cp_interrupt (int irq, void *dev_instance)
 	if (status & (TxOK | TxErr | TxEmpty | SWInt))
 		handled |= cp_tx(cp);
 
+	if ((status & TxEmpty) && cp->tx_running) {
+		handled = 1;
+		/* Qemu's emulation doesn't give TxEmpty interrupts */
+		cp->txempty_seen = 1;
+		if (cp->tx_head == cp->tx_tail) {
+			/* Out of descriptors and we have nothing more for it.
+			   Let it stop. */
+			cp->tx_running = 0;
+		} else {
+			/* The hardware raced with us adding a new descriptor,
+			   and we didn't get the TxEmpty IRQ in time so we
+			   didn't prod it. Prod it now to restart. */
+			cpw8(TxPoll, NormalTxPoll);
+		}
+	}
+
 	if (status & LinkChg) {
 		handled = 1;
 		mii_check_media(&cp->mii_if, netif_msg_link(cp), false);
@@ -796,8 +815,9 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 
 		cp->tx_skb[entry] = skb;
 		cp->tx_opts[entry] = flags;
-		netif_dbg(cp, tx_queued, cp->dev, "tx queued, slot %d, skblen %d\n",
-			  entry, skb->len);
+		netif_dbg(cp, tx_queued, cp->dev,
+			  "tx queued, slot %d, skblen %d r %d\n",
+			  entry, skb->len, cp->tx_running);
 	} else {
 		struct cp_desc *txd;
 		u32 first_len, first_eor, ctrl;
@@ -886,8 +906,9 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 		wmb();
 
 		cp->tx_opts[first_entry] = ctrl;
-		netif_dbg(cp, tx_queued, cp->dev, "tx queued, slots %d-%d, skblen %d\n",
-			  first_entry, entry, skb->len);
+		netif_dbg(cp, tx_queued, cp->dev,
+			  "tx queued, slots %d-%d, skblen %d r %d\n",
+			  first_entry, entry, skb->len, cp->tx_running);
 	}
 	cp->tx_head = NEXT_TX(entry);
 
@@ -895,11 +916,13 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 	if (TX_BUFFS_AVAIL(cp) <= (MAX_SKB_FRAGS + 1))
 		netif_stop_queue(dev);
 
+	if (!cp->tx_running || !cp->txempty_seen) {
+		cpw8(TxPoll, NormalTxPoll);
+		cp->tx_running = 1;
+	}
 out_unlock:
 	spin_unlock_irqrestore(&cp->lock, intr_flags);
 
-	cpw8(TxPoll, NormalTxPoll);
-
 	return NETDEV_TX_OK;
 out_dma_error:
 	dev_kfree_skb_any(skb);
@@ -989,6 +1012,7 @@ static void cp_stop_hw (struct cp_private *cp)
 
 	cp->rx_tail = 0;
 	cp->tx_head = cp->tx_tail = 0;
+	cp->tx_running = 0;
 
 	netdev_reset_queue(cp->dev);
 }
@@ -1041,6 +1065,7 @@ static inline void cp_start_hw (struct cp_private *cp)
 	 * This variant appears to work fine.
 	 */
 	cpw8(Cmd, RxOn | TxOn);
+	cp->tx_running = 0;
 
 	netdev_reset_queue(cp->dev);
 }
-- 
2.4.3

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

Re: [PATCH 2/2] 8139cp: Call __cp_set_rx_mode() from cp_tx_timeout()

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 4095 bytes --]

On Mon, 2015-09-21 at 14:59 +0100, David Woodhouse wrote:

> After which, I think we might be able to turn on TX checksumming by
> default and I also have a way to implement early detection of the TX
> stall I've been seeing.

This is a patch I've been playing with to catch the TX stall.

When it happens we get a TxEmpty interrupt *without* TxDone.

After loading the driver, we never get TxEmpty without TxDone until the
problem has happened. I've got a minimal cp_tx_timeout() which just
clears the TxOn bit in the Cmd register and turns it back on again,
after moving the descriptors all down to the start of the TX ring.

Strangely, *after* this has happened we do see a lot of instances of
TxEmpty without TxDone. But then the TxDone usually comes immediately
afterwards. Until the driver is reloaded, at which point the next
instance of TxEmpty without TxDone *is* the hardware stalling again.

I'm very confused about what's going on there. I think I possibly need
to set a timer when the TxEmpty/!TxDone interrupt happens, and do a
preemptive reset of the TX queue (as shown below) if the timer manages
to expire before the TxDone actually happens.

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index 67a4fcf..64b44ec 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -592,8 +592,8 @@ static irqreturn_t cp_interrupt (int irq, void *dev_instance)
 	if (!status || (status == 0xFFFF))
 		goto out_unlock;
 
-	netif_dbg(cp, intr, dev, "intr, status %04x cmd %02x cpcmd %04x\n",
-		  status, cpr8(Cmd), cpr16(CpCmd));
+	netif_dbg(cp, intr, dev, "intr, status %04x cmd %02x cpcmd %04x desc %04x\n",
+		  status, cpr8(Cmd), cpr16(CpCmd), cpr16(TxDmaOkLowDesc));
 
 	cpw16(IntrStatus, status & ~cp_rx_intr_mask);
 
@@ -623,6 +623,10 @@ static irqreturn_t cp_interrupt (int irq, void *dev_instance)
 			   Let it stop. */
 			cp->tx_running = 0;
 		} else {
+			if (!(status & (TxOK | TxErr)))
+				netdev_warn(dev, "TxEmpty without TxDone. h %d t %d d %04x\n",
+					    cp->tx_head, cp->tx_tail, cpr16(TxDmaOkLowDesc));
+
 			/* The hardware raced with us adding a new descriptor,
 			   and we didn't get the TxEmpty IRQ in time so we
 			   didn't prod it. Prod it now to restart. */
@@ -1307,12 +1311,49 @@ static void cp_tx_timeout(struct net_device *dev)
 			  le64_to_cpu(cp->tx_ring[i].addr),
 			  cp->tx_skb[i]);
 	}
-
+#if 1
+	/* Stop the TX (which is already stopped), move the
+	   descriptors down, and start it again */
+	cpw8_f(Cmd, RxOn);
+	if (cp->tx_tail == 0) {
+		/* Do nothing */
+	} else if (cp->tx_head > cp->tx_tail) {
+		for (i = 0; i < cp->tx_head - cp->tx_tail; i++) {
+			int from = i + cp->tx_tail;
+			cp->tx_ring[i].addr = cp->tx_ring[from].addr;
+			cp->tx_ring[i].opts2 = cp->tx_ring[from].opts2;
+			cp->tx_ring[i].opts1 = cp->tx_ring[from].opts1;
+			cp->tx_ring[from].opts1 &= ~cpu_to_le32(DescOwn);
+			cp->tx_opts[i] = cp->tx_opts[from];
+			cp->tx_skb[i] = cp->tx_skb[from];
+			cp->tx_skb[from] = NULL;
+			printk("Ring move %d->%d\n", from, i);
+		}
+	} else {
+		for (i = cp->tx_tail - cp->tx_head - 1; i >= 0; i--) {
+			int from = (i + cp->tx_tail) & (CP_TX_RING_SIZE - 1);
+			cp->tx_ring[i].addr = cp->tx_ring[from].addr;
+			cp->tx_ring[i].opts2 = cp->tx_ring[from].opts2;
+			cp->tx_ring[i].opts1 = cp->tx_ring[from].opts1;
+			cp->tx_ring[from].opts1 &= ~cpu_to_le32(DescOwn);
+			cp->tx_opts[i] = cp->tx_opts[from];
+			cp->tx_skb[i] = cp->tx_skb[from];
+			cp->tx_skb[from] = NULL;
+			printk("Ring move %d->%d\n", from, i);
+		}
+	}
+	cpw8_f(Cmd, RxOn|TxOn);
+	cp->tx_head = (cp->tx_head - cp->tx_tail) & (CP_TX_RING_SIZE - 1);
+	cp->tx_tail = 0;
+	printk("head %d tail %d\n", cp->tx_head, cp->tx_tail);
+	cpw8(TxPoll, NormalTxPoll);
+#else
 	cp_stop_hw(cp);
 	cp_clean_rings(cp);
 	rc = cp_init_rings(cp);
 	cp_start_hw(cp);
 	__cp_set_rx_mode(dev);
+#endif
 	cpw16_f(IntrMask, cp_norx_intr_mask);
 
 	netif_wake_queue(dev);

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

Re: [PATCH 1/7] 8139cp: Improve accuracy of cp_interrupt() return, to survive IRQ storms

[Francois Romieu]

David Woodhouse <dwmw2@infradead.org> :
> From: David Woodhouse <David.Woodhouse@intel.com>
> 
> The TX timeout handling has been observed to trigger RX IRQ storms. And
> since cp_interrupt() just keeps saying that it handled the interrupt,
> the machine then dies. Fix the return value from cp_interrupt(), and
> the offending IRQ gets disabled and the machine survives.

I am not fond of the way it dissociates the hardware status word and the
software "handled" variable.

What you are describing - RX IRQ storms - looks like a problem between
the irq and poll handlers. That's where I expect the problem to be solved.
Sprinkling "handled" operations does not make me terribly confortable,
especially as I'd happily trade the old-style part irq, part napi
processing for a plain napi processing (I can get over it though :o) ).

Once the code is past the "if (!status || (status == 0xFFFF))" test -
or whatever test against some mask - I don't see why the driver could
refuse to take ownership of the irq.

-- 
Ueimor

Re: [PATCH 1/7] 8139cp: Improve accuracy of cp_interrupt() return, to survive IRQ storms

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1961 bytes --]

On Mon, 2015-09-21 at 22:25 +0200, Francois Romieu wrote:
> David Woodhouse <dwmw2@infradead.org> :
> > From: David Woodhouse <David.Woodhouse@intel.com>
> > 
> > The TX timeout handling has been observed to trigger RX IRQ storms. And
> > since cp_interrupt() just keeps saying that it handled the interrupt,
> > the machine then dies. Fix the return value from cp_interrupt(), and
> > the offending IRQ gets disabled and the machine survives.
> 
> I am not fond of the way it dissociates the hardware status word and the
> software "handled" variable.

Oh, I like that part very much :)

The practice of returning a 'handled' status only when you've actually
*done* something you expect to mitigate the interrupt, is a useful way
of also protecting against both hardware misbehaviour and software
bugs.

> What you are describing - RX IRQ storms - looks like a problem between
> the irq and poll handlers. That's where I expect the problem to be solved.

I already fixed that, in the next patch in the series. But the failure
mode *should* have been 'IRQ disabled' and the device continuing to
work via polling. Not a complete death of the machine. That's the
difference this patch makes.

> Sprinkling "handled" operations does not make me terribly confortable,
> especially as I'd happily trade the old-style part irq, part napi
> processing for a plain napi processing (I can get over it though :o) ).

The existing cp_rx_poll() function mostly runs without taking cp->lock.
But cp_tx() *does* need cp->lock for the tx_head/tx_tail manipulations.
I'm guessing that's why it's still being called directly from the hard
IRQ handler? I suppose we could probably find a way to move that out.

But I was mostly just trying to fix stuff that was actually broken...
:)

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

Re: [PATCH 7/7] 8139cp: Avoid gratuitous writes to TxPoll register when already running

[Francois Romieu]

David Woodhouse <dwmw2@infradead.org> :
[...]
> I'm actually having second thoughts about this one since realising that
> QEMU doesn't implement it correctly. The workaround isn't *that* horrid
> but it's not clear it's enough of a performance win — or whether it's
> entirely necessary for catching my TX stall.

It don't indulge in this kind of fetish but I'm fine with people who want
to play with QEMU 8139cp emulation where they could use virtio. They should
imvho realize that it's their job to fix QEMU 8139cp emulation, not the
other way around.

Please keep things sane (*cough*) and avoid the qemu workaround.

-- 
Ueimor

Re: [PATCH 4/7] 8139cp: Fix TSO/scatter-gather descriptor setup

[Francois Romieu]

David Woodhouse <dwmw2@infradead.org> :
[...]
> diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
> index 058f835..07621b5 100644
> --- a/drivers/net/ethernet/realtek/8139cp.c
> +++ b/drivers/net/ethernet/realtek/8139cp.c
[...]
> @@ -865,20 +864,20 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
>  		txd->addr = cpu_to_le64(first_mapping);
>  		wmb();
>  
> -		if (skb->ip_summed == CHECKSUM_PARTIAL) {
> +		ctrl = first_eor | first_len | FirstFrag | DescOwn;
> +		if (mss)
> +			ctrl |= LargeSend |
> +				((mss & MSSMask) << MSSShift);

			ctrl |= LargeSend | ((mss & MSSMask) << MSSShift);

> +		else if (skb->ip_summed == CHECKSUM_PARTIAL) {
>  			if (ip->protocol == IPPROTO_TCP)
> -				txd->opts1 = cpu_to_le32(first_eor | first_len |
> -							 FirstFrag | DescOwn |
> -							 IPCS | TCPCS);
> +				ctrl |= IPCS | TCPCS;
>  			else if (ip->protocol == IPPROTO_UDP)
> -				txd->opts1 = cpu_to_le32(first_eor | first_len |
> -							 FirstFrag | DescOwn |
> -							 IPCS | UDPCS);
> +				ctrl |= IPCS | UDPCS;
>  			else
>  				BUG();

Can you pile a patch to replace BUG with WARN_ON_ONCE(1) ?

-- 
Ueimor

Re: [PATCH 4/7] 8139cp: Fix TSO/scatter-gather descriptor setup

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 308 bytes --]

On Mon, 2015-09-21 at 23:01 +0200, Francois Romieu wrote:
> Can you pile a patch to replace BUG with WARN_ON_ONCE(1) ?

OK. I can probably contrive a userspace program using AF_PACKET and
PACKET_VNET_HDR to trigger it, too¹ :)

-- 
dwmw2


¹ http://comments.gmane.org/gmane.linux.network/254981

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

Re: [PATCH 7/7] 8139cp: Avoid gratuitous writes to TxPoll register when already running

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1896 bytes --]

On Mon, 2015-09-21 at 22:54 +0200, Francois Romieu wrote:
> David Woodhouse <dwmw2@infradead.org> :
> [...]
> > I'm actually having second thoughts about this one since realising that
> > QEMU doesn't implement it correctly. The workaround isn't *that* horrid
> > but it's not clear it's enough of a performance win — or whether it's
> > entirely necessary for catching my TX stall.
> 
> It don't indulge in this kind of fetish but I'm fine with people who want
> to play with QEMU 8139cp emulation where they could use virtio. They should
> imvho realize that it's their job to fix QEMU 8139cp emulation, not the
> other way around.

Oh, I'll fix the QEMU emulation (but not this week; updating my router
has taken long enough already and I have Real Work™ to do).

But those fixes will take time to propagate to actual users. I'm not
sure it's so reasonable to knowingly break the 8139cp driver for all
currently-released versions of QEMU.

It wouldn't surprise me to find that QEMU probably accounts for the
*majority* of 8139cp use on modern Linux kernels. I've had to fix
regressions which *only* fail on real hardware, and were *only* tested
on QEMU :)

> Please keep things sane (*cough*) and avoid the qemu workaround.

I don't even know that I need it. As you saw in my last WIP patch for
catching the TX stall, I was just checking for TxEmpty without TxDone.
An earlier iteration had actually remembered the last slot that was
already present when we prodded the TxPoll, and would warn on getting a
TxEmpty interrupt while that slot was still owned by the hardware. But
that has the *same* false positives, only *after* the initial stall,
that my current version has.

So I'm just not sure I care about eliminating the gratuitous TxPoll
writes. It's not as if we even wait for them. It's an MMIO write which
can complete later.

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

Re: [PATCH 4/7] 8139cp: Fix TSO/scatter-gather descriptor setup

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 4443 bytes --]

On Mon, 2015-09-21 at 23:01 +0200, Francois Romieu wrote:
> 
> Can you pile a patch to replace BUG with WARN_ON_ONCE(1) ?

Let's avoid having three copies of the same damn code, while we're at
it... http://git.infradead.org/users/dwmw2/linux-8139cp.git has this
and the appropriate minor fixes to subsequent patches in the series.

What do you think of finally enabling hw csum and TSO by default, btw?

Subject: [PATCH 4½/7] 8139cp: Reduce duplicate csum/tso code in cp_start_xmit()

We calculate the value of the opts1 descriptor field in three different
places. With two different behaviours when given an invalid packet to
be checksummed — none of them correct. Sort that out.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
 drivers/net/ethernet/realtek/8139cp.c | 61 ++++++++++++-----------------------
 1 file changed, 20 insertions(+), 41 deletions(-)

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index 3b219aa..a2c471d 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -740,7 +740,7 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 {
 	struct cp_private *cp = netdev_priv(dev);
 	unsigned entry;
-	u32 eor, flags;
+	u32 eor, opts1;
 	unsigned long intr_flags;
 	__le32 opts2;
 	int mss = 0;
@@ -760,6 +760,21 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 	mss = skb_shinfo(skb)->gso_size;
 
 	opts2 = cpu_to_le32(cp_tx_vlan_tag(skb));
+	opts1 = DescOwn;
+	if (mss)
+		opts1 |= LargeSend | ((mss & MSSMask) << MSSShift);
+	else if (skb->ip_summed == CHECKSUM_PARTIAL) {
+		const struct iphdr *ip = ip_hdr(skb);
+		if (ip->protocol == IPPROTO_TCP)
+			opts1 |= IPCS | TCPCS;
+		else if (ip->protocol == IPPROTO_UDP)
+			opts1 |= IPCS | UDPCS;
+		else {
+			WARN_ONCE(1,
+				  "Net bug: asked to checksum invalid Legacy IP packet\n");
+			goto out_dma_error;
+		}
+	}
 
 	if (skb_shinfo(skb)->nr_frags == 0) {
 		struct cp_desc *txd = &cp->tx_ring[entry];
@@ -775,21 +790,9 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 		txd->addr = cpu_to_le64(mapping);
 		wmb();
 
-		flags = eor | len | DescOwn | FirstFrag | LastFrag;
-
-		if (mss)
-			flags |= LargeSend | ((mss & MSSMask) << MSSShift);
-		else if (skb->ip_summed == CHECKSUM_PARTIAL) {
-			const struct iphdr *ip = ip_hdr(skb);
-			if (ip->protocol == IPPROTO_TCP)
-				flags |= IPCS | TCPCS;
-			else if (ip->protocol == IPPROTO_UDP)
-				flags |= IPCS | UDPCS;
-			else
-				WARN_ON(1);	/* we need a WARN() */
-		}
+		opts1 |= eor | len | FirstFrag | LastFrag;
 
-		txd->opts1 = cpu_to_le32(flags);
+		txd->opts1 = cpu_to_le32(opts1);
 		wmb();
 
 		cp->tx_skb[entry] = skb;
@@ -800,7 +803,6 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 		u32 first_len, first_eor, ctrl;
 		dma_addr_t first_mapping;
 		int frag, first_entry = entry;
-		const struct iphdr *ip = ip_hdr(skb);
 
 		/* We must give this initial chunk to the device last.
 		 * Otherwise we could race with the device.
@@ -832,19 +834,7 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 
 			eor = (entry == (CP_TX_RING_SIZE - 1)) ? RingEnd : 0;
 
-			ctrl = eor | len | DescOwn;
-
-			if (mss)
-				ctrl |= LargeSend |
-					((mss & MSSMask) << MSSShift);
-			else if (skb->ip_summed == CHECKSUM_PARTIAL) {
-				if (ip->protocol == IPPROTO_TCP)
-					ctrl |= IPCS | TCPCS;
-				else if (ip->protocol == IPPROTO_UDP)
-					ctrl |= IPCS | UDPCS;
-				else
-					BUG();
-			}
+			ctrl = opts1 | eor | len;
 
 			if (frag == skb_shinfo(skb)->nr_frags - 1)
 				ctrl |= LastFrag;
@@ -864,18 +854,7 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 		txd->addr = cpu_to_le64(first_mapping);
 		wmb();
 
-		ctrl = first_eor | first_len | FirstFrag | DescOwn;
-		if (mss)
-			ctrl |= LargeSend | (mss & MSSMask) << MSSShift);
-		else if (skb->ip_summed == CHECKSUM_PARTIAL) {
-			if (ip->protocol == IPPROTO_TCP)
-				ctrl |= IPCS | TCPCS;
-			else if (ip->protocol == IPPROTO_UDP)
-				ctrl |= IPCS | UDPCS;
-			else
-				BUG();
-		}
-
+		ctrl = opts1 | first_eor | first_len | FirstFrag;
 		txd->opts1 = cpu_to_le32(ctrl);
 		wmb();
 
-- 
2.4.3



-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

Re: [PATCH 4/7] 8139cp: Fix TSO/scatter-gather descriptor setup

[Francois Romieu]

David Woodhouse <dwmw2@infradead.org> :
[...]
> What do you think of finally enabling hw csum and TSO by default, btw?

Your router can actively use it and you'll have to keep the pieces
together if it breaks, right ? So I'd go for it.

-- 
Ueimor

Re: [PATCH 1/7] 8139cp: Improve accuracy of cp_interrupt() return, to survive IRQ storms

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Mon, 21 Sep 2015 15:01:49 +0100

> From: David Woodhouse <David.Woodhouse@intel.com>
> 
> The TX timeout handling has been observed to trigger RX IRQ storms. And
> since cp_interrupt() just keeps saying that it handled the interrupt,
> the machine then dies. Fix the return value from cp_interrupt(), and
> the offending IRQ gets disabled and the machine survives.
> 
> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>

Like Francois, I don't like this.

First of all, there are only 3 bits not handled explicitly by
cp_interrupt().  And for those if they are set and no other condition
was rasied, you should report the event and the status bits set, and
then forcibly clear the interrupt.

And if we are getting Rx* interrupts with napi_schedule_prep()
returning false, that's a serious problem.  It can mean that the TX
timeout handler's resetting of the chip is either miscoded or is
racing with either NAPI polling or this interrupt handler.

And if that's the case your patch is making the chip's IRQ line get
disabled when this race triggers.

This change is even worse, in my opinion, if patch #2 indeed makes
the problem go away.

Re: [PATCH 2/7] 8139cp: Do not re-enable RX interrupts in cp_tx_timeout()

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Mon, 21 Sep 2015 15:02:07 +0100

> From: David Woodhouse <David.Woodhouse@intel.com>
> 
> If an RX interrupt was already received but NAPI has not yet run when
> the RX timeout happens, we end up in cp_tx_timeout() with RX interrupts
> already disabled. Blindly re-enabling them will cause an IRQ storm.
> 
> This is somewhat less painful than it was a few minutes ago before I
> fixed the return value from cp_interrupt(), but still suboptimal.
> 
> Unconditionally leave RX interrupts disabled after the reset, and
> schedule NAPI to check the receive ring and re-enable them.
> 
> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>

As I said, if this makes your IRQ storms go away then therefore it
obviates the need for patch #1.

Re: [PATCH 1/7] 8139cp: Improve accuracy of cp_interrupt() return, to survive IRQ storms

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 3094 bytes --]

On Tue, 2015-09-22 at 16:45 -0700, David Miller wrote:
> And if we are getting Rx* interrupts with napi_schedule_prep()
> returning false, that's a serious problem.  It can mean that the TX
> timeout handler's resetting of the chip is either miscoded or is
> racing with either NAPI polling or this interrupt handler.

Such bugs happen. That's why we have IRQ-storm detection, to help
protect us.

> And if that's the case your patch is making the chip's IRQ line get
> disabled when this race triggers.
> 
> This change is even worse, in my opinion, if patch #2 indeed makes
> the problem go away.

Even worse than what?

When a bug like the one I've fixed in #2 happens, let's look at the
options:

  - With this patch, the IRQ storm is detected. We disable the IRQ 
    line and the driver continues to work, albeit with a higher
    latency.

  - Without this patch, the box just dies completely. First the 
    hardware watchdog was triggering an apparently spontaneous reset. 
    Then when I disabled the hardware watchdog, the machine locked up 
    and doesn't even respond to serial sysrq. Eventually I got a
    little bit of sense out of it using the NMI watchdog.

Trust me, the disabled IRQ you get with my patch really *isn't* worse
than what happened without it :)

If cp_interrupt() had already worked the way I suggest, I would
literally have two days of my life back right now. I spent a *lot* of
time working out that it was dying in an interrupt storm. And trying to
find the underlying problem while having to physically visit it under
the stairs and reset it all the time.

In fact, I've been seeing undebuggable spontaneous resets of this
router for a while now. If I'd had an 'IRQ disabled' backtrace and a
smoking gun, I might have been able to fix it a long time ago.

I even discounted the IRQ-storm hypothesis, early on, on the basis that
"Linux handles those quite gracefully now, and disables the interrupt".
Except of course *that* relies on the IRQ handler returning an
appropriate value. So it doesn't work with 8139cp as things stand.

That's why I'm really quite keen on fixing cp_interrupt() to be more
defensive in when it claims to have handled the interrupt.

Surely that's *why* we have the IRQ-storm detection in the first place
— to help us survive precisely this situation. IRQ storms are either
caused by software bugs like the one fixed in patch #2, or hardware
misbehaviour. And if IRQ handlers blindly return 'handled' just because
the hardware actually *did* assert an interrupt, regardless of whether
they've made any *progress*, then we don't get that protection in a
*lot* of the situations where we'd want it.

But sure, for now I'll drop this from the series and I can try to
convince you separately. In fact I think it only affects the last patch
in the series which reduces the banging on TxPoll. And I'm going to
drop that one too for now anyway. I'll repost shortly. And I think I'll
add a new one at the end, enabling TX checksums, SG and TSO by default.

-- 
dwmw2

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

[PATCH 1/7] 8139cp: Do not re-enable RX interrupts in cp_tx_timeout()

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1753 bytes --]

From: David Woodhouse <David.Woodhouse@intel.com>

If an RX interrupt was already received but NAPI has not yet run when
the RX timeout happens, we end up in cp_tx_timeout() with RX interrupts
already disabled. Blindly re-enabling them will cause an IRQ storm.

(This is made particularly horrid by the fact that cp_interrupt() always
returns that it's handled the interrupt, even when it hasn't actually
done anything. If it didn't do that, the core IRQ code would have
detected the storm and handled it, I'd have had a clear smoking gun
backtrace instead of just a spontaneously resetting router, and I'd have
at *least* two days of my life back. Changing the return value of
cp_interrupt() will be argued about under separate cover.)

Unconditionally leave RX interrupts disabled after the reset, and
schedule NAPI to check the receive ring and re-enable them.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
 drivers/net/ethernet/realtek/8139cp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index ba3dab7..947932d 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -1262,9 +1262,10 @@ static void cp_tx_timeout(struct net_device *dev)
 	rc = cp_init_rings(cp);
 	cp_start_hw(cp);
 	__cp_set_rx_mode(dev);
-	cp_enable_irq(cp);
+	cpw16_f(IntrMask, cp_norx_intr_mask);
 
 	netif_wake_queue(dev);
+	napi_schedule_irqoff(&cp->napi);
 
 	spin_unlock_irqrestore(&cp->lock, flags);
 }
-- 
2.4.3

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

[PATCH 2/7] 8139cp: Fix tx_queued debug message to print correct slot numbers

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 2399 bytes --]

From: David Woodhouse <David.Woodhouse@intel.com>

After a certain amount of staring at the debug output of this driver, I
realised it was lying to me.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
 drivers/net/ethernet/realtek/8139cp.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index 947932d..cbca0de 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -786,7 +786,8 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 		wmb();
 
 		cp->tx_skb[entry] = skb;
-		entry = NEXT_TX(entry);
+		netif_dbg(cp, tx_queued, cp->dev, "tx queued, slot %d, skblen %d\n",
+			  entry, skb->len);
 	} else {
 		struct cp_desc *txd;
 		u32 first_len, first_eor;
@@ -805,7 +806,6 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 			goto out_dma_error;
 
 		cp->tx_skb[entry] = skb;
-		entry = NEXT_TX(entry);
 
 		for (frag = 0; frag < skb_shinfo(skb)->nr_frags; frag++) {
 			const skb_frag_t *this_frag = &skb_shinfo(skb)->frags[frag];
@@ -813,6 +813,8 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 			u32 ctrl;
 			dma_addr_t mapping;
 
+			entry = NEXT_TX(entry);
+
 			len = skb_frag_size(this_frag);
 			mapping = dma_map_single(&cp->pdev->dev,
 						 skb_frag_address(this_frag),
@@ -848,9 +850,7 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 
 			txd->opts1 = cpu_to_le32(ctrl);
 			wmb();
-
 			cp->tx_skb[entry] = skb;
-			entry = NEXT_TX(entry);
 		}
 
 		txd = &cp->tx_ring[first_entry];
@@ -873,12 +873,13 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 			txd->opts1 = cpu_to_le32(first_eor | first_len |
 						 FirstFrag | DescOwn);
 		wmb();
+
+		netif_dbg(cp, tx_queued, cp->dev, "tx queued, slots %d-%d, skblen %d\n",
+			  first_entry, entry, skb->len);
 	}
-	cp->tx_head = entry;
+	cp->tx_head = NEXT_TX(entry);
 
 	netdev_sent_queue(dev, skb->len);
-	netif_dbg(cp, tx_queued, cp->dev, "tx queued, slot %d, skblen %d\n",
-		  entry, skb->len);
 	if (TX_BUFFS_AVAIL(cp) <= (MAX_SKB_FRAGS + 1))
 		netif_stop_queue(dev);
 
-- 
2.4.3

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

[PATCH 3/7] 8139cp: Fix TSO/scatter-gather descriptor setup

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 2309 bytes --]

From: David Woodhouse <David.Woodhouse@intel.com>

When sending a TSO frame in multiple buffers, we were neglecting to set
the first descriptor up in TSO mode.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
 drivers/net/ethernet/realtek/8139cp.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index cbca0de..75a8cee 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -790,7 +790,7 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 			  entry, skb->len);
 	} else {
 		struct cp_desc *txd;
-		u32 first_len, first_eor;
+		u32 first_len, first_eor, ctrl;
 		dma_addr_t first_mapping;
 		int frag, first_entry = entry;
 		const struct iphdr *ip = ip_hdr(skb);
@@ -810,7 +810,6 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 		for (frag = 0; frag < skb_shinfo(skb)->nr_frags; frag++) {
 			const skb_frag_t *this_frag = &skb_shinfo(skb)->frags[frag];
 			u32 len;
-			u32 ctrl;
 			dma_addr_t mapping;
 
 			entry = NEXT_TX(entry);
@@ -858,20 +857,19 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 		txd->addr = cpu_to_le64(first_mapping);
 		wmb();
 
-		if (skb->ip_summed == CHECKSUM_PARTIAL) {
+		ctrl = first_eor | first_len | FirstFrag | DescOwn;
+		if (mss)
+			ctrl |= LargeSend | ((mss & MSSMask) << MSSShift);
+		else if (skb->ip_summed == CHECKSUM_PARTIAL) {
 			if (ip->protocol == IPPROTO_TCP)
-				txd->opts1 = cpu_to_le32(first_eor | first_len |
-							 FirstFrag | DescOwn |
-							 IPCS | TCPCS);
+				ctrl |= IPCS | TCPCS;
 			else if (ip->protocol == IPPROTO_UDP)
-				txd->opts1 = cpu_to_le32(first_eor | first_len |
-							 FirstFrag | DescOwn |
-							 IPCS | UDPCS);
+				ctrl |= IPCS | UDPCS;
 			else
 				BUG();
-		} else
-			txd->opts1 = cpu_to_le32(first_eor | first_len |
-						 FirstFrag | DescOwn);
+		}
+
+		txd->opts1 = cpu_to_le32(ctrl);
 		wmb();
 
 		netif_dbg(cp, tx_queued, cp->dev, "tx queued, slots %d-%d, skblen %d\n",
-- 
2.4.3

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

[PATCH 4/7] 8139cp: Reduce duplicate csum/tso code in cp_start_xmit()

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 3993 bytes --]

From: David Woodhouse <David.Woodhouse@intel.com>

We calculate the value of the opts1 descriptor field in three different
places. With two different behaviours when given an invalid packet to
be checksummed — none of them correct. Sort that out.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
 drivers/net/ethernet/realtek/8139cp.c | 61 ++++++++++++-----------------------
 1 file changed, 20 insertions(+), 41 deletions(-)

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index 75a8cee..b3bd8b1 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -733,7 +733,7 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 {
 	struct cp_private *cp = netdev_priv(dev);
 	unsigned entry;
-	u32 eor, flags;
+	u32 eor, opts1;
 	unsigned long intr_flags;
 	__le32 opts2;
 	int mss = 0;
@@ -753,6 +753,21 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 	mss = skb_shinfo(skb)->gso_size;
 
 	opts2 = cpu_to_le32(cp_tx_vlan_tag(skb));
+	opts1 = DescOwn;
+	if (mss)
+		opts1 |= LargeSend | ((mss & MSSMask) << MSSShift);
+	else if (skb->ip_summed == CHECKSUM_PARTIAL) {
+		const struct iphdr *ip = ip_hdr(skb);
+		if (ip->protocol == IPPROTO_TCP)
+			opts1 |= IPCS | TCPCS;
+		else if (ip->protocol == IPPROTO_UDP)
+			opts1 |= IPCS | UDPCS;
+		else {
+			WARN_ONCE(1,
+				  "Net bug: asked to checksum invalid Legacy IP packet\n");
+			goto out_dma_error;
+		}
+	}
 
 	if (skb_shinfo(skb)->nr_frags == 0) {
 		struct cp_desc *txd = &cp->tx_ring[entry];
@@ -768,21 +783,9 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 		txd->addr = cpu_to_le64(mapping);
 		wmb();
 
-		flags = eor | len | DescOwn | FirstFrag | LastFrag;
-
-		if (mss)
-			flags |= LargeSend | ((mss & MSSMask) << MSSShift);
-		else if (skb->ip_summed == CHECKSUM_PARTIAL) {
-			const struct iphdr *ip = ip_hdr(skb);
-			if (ip->protocol == IPPROTO_TCP)
-				flags |= IPCS | TCPCS;
-			else if (ip->protocol == IPPROTO_UDP)
-				flags |= IPCS | UDPCS;
-			else
-				WARN_ON(1);	/* we need a WARN() */
-		}
+		opts1 |= eor | len | FirstFrag | LastFrag;
 
-		txd->opts1 = cpu_to_le32(flags);
+		txd->opts1 = cpu_to_le32(opts1);
 		wmb();
 
 		cp->tx_skb[entry] = skb;
@@ -793,7 +796,6 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 		u32 first_len, first_eor, ctrl;
 		dma_addr_t first_mapping;
 		int frag, first_entry = entry;
-		const struct iphdr *ip = ip_hdr(skb);
 
 		/* We must give this initial chunk to the device last.
 		 * Otherwise we could race with the device.
@@ -825,19 +827,7 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 
 			eor = (entry == (CP_TX_RING_SIZE - 1)) ? RingEnd : 0;
 
-			ctrl = eor | len | DescOwn;
-
-			if (mss)
-				ctrl |= LargeSend |
-					((mss & MSSMask) << MSSShift);
-			else if (skb->ip_summed == CHECKSUM_PARTIAL) {
-				if (ip->protocol == IPPROTO_TCP)
-					ctrl |= IPCS | TCPCS;
-				else if (ip->protocol == IPPROTO_UDP)
-					ctrl |= IPCS | UDPCS;
-				else
-					BUG();
-			}
+			ctrl = opts1 | eor | len;
 
 			if (frag == skb_shinfo(skb)->nr_frags - 1)
 				ctrl |= LastFrag;
@@ -857,18 +847,7 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 		txd->addr = cpu_to_le64(first_mapping);
 		wmb();
 
-		ctrl = first_eor | first_len | FirstFrag | DescOwn;
-		if (mss)
-			ctrl |= LargeSend | ((mss & MSSMask) << MSSShift);
-		else if (skb->ip_summed == CHECKSUM_PARTIAL) {
-			if (ip->protocol == IPPROTO_TCP)
-				ctrl |= IPCS | TCPCS;
-			else if (ip->protocol == IPPROTO_UDP)
-				ctrl |= IPCS | UDPCS;
-			else
-				BUG();
-		}
-
+		ctrl = opts1 | first_eor | first_len | FirstFrag;
 		txd->opts1 = cpu_to_le32(ctrl);
 		wmb();
 
-- 
2.4.3

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

[PATCH 5/7] 8139cp: Fix DMA unmapping of transmitted buffers

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 3095 bytes --]

From: David Woodhouse <David.Woodhouse@intel.com>

The low 16 bits of the 'opts1' field in the TX descriptor are supposed
to still contain the buffer length when the descriptor is handed back to
us. In practice, at least on my hardware, they don't. So stash the
original value of the opts1 field and get the length to unmap from
there.

There are other ways we could have worked out the length, but I actually
want a stash of the opts1 field anyway so that I can dump it alongside
the contents of the descriptor ring when we suffer a TX timeout.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
 drivers/net/ethernet/realtek/8139cp.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index b3bd8b1..ec6bd86 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -341,6 +341,7 @@ struct cp_private {
 	unsigned		tx_tail;
 	struct cp_desc		*tx_ring;
 	struct sk_buff		*tx_skb[CP_TX_RING_SIZE];
+	u32			tx_opts[CP_TX_RING_SIZE];
 
 	unsigned		rx_buf_sz;
 	unsigned		wol_enabled : 1; /* Is Wake-on-LAN enabled? */
@@ -665,7 +666,7 @@ static void cp_tx (struct cp_private *cp)
 		BUG_ON(!skb);
 
 		dma_unmap_single(&cp->pdev->dev, le64_to_cpu(txd->addr),
-				 le32_to_cpu(txd->opts1) & 0xffff,
+				 cp->tx_opts[tx_tail] & 0xffff,
 				 PCI_DMA_TODEVICE);
 
 		if (status & LastFrag) {
@@ -789,6 +790,7 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 		wmb();
 
 		cp->tx_skb[entry] = skb;
+		cp->tx_opts[entry] = opts1;
 		netif_dbg(cp, tx_queued, cp->dev, "tx queued, slot %d, skblen %d\n",
 			  entry, skb->len);
 	} else {
@@ -839,6 +841,8 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 
 			txd->opts1 = cpu_to_le32(ctrl);
 			wmb();
+
+			cp->tx_opts[entry] = ctrl;
 			cp->tx_skb[entry] = skb;
 		}
 
@@ -851,6 +855,7 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 		txd->opts1 = cpu_to_le32(ctrl);
 		wmb();
 
+		cp->tx_opts[first_entry] = ctrl;
 		netif_dbg(cp, tx_queued, cp->dev, "tx queued, slots %d-%d, skblen %d\n",
 			  first_entry, entry, skb->len);
 	}
@@ -1093,6 +1098,7 @@ static int cp_init_rings (struct cp_private *cp)
 {
 	memset(cp->tx_ring, 0, sizeof(struct cp_desc) * CP_TX_RING_SIZE);
 	cp->tx_ring[CP_TX_RING_SIZE - 1].opts1 = cpu_to_le32(RingEnd);
+	memset(cp->tx_opts, 0, sizeof(cp->tx_opts));
 
 	cp_init_rings_index(cp);
 
@@ -1150,6 +1156,7 @@ static void cp_clean_rings (struct cp_private *cp)
 
 	memset(cp->rx_ring, 0, sizeof(struct cp_desc) * CP_RX_RING_SIZE);
 	memset(cp->tx_ring, 0, sizeof(struct cp_desc) * CP_TX_RING_SIZE);
+	memset(cp->tx_opts, 0, sizeof(cp->tx_opts));
 
 	memset(cp->rx_skb, 0, sizeof(struct sk_buff *) * CP_RX_RING_SIZE);
 	memset(cp->tx_skb, 0, sizeof(struct sk_buff *) * CP_TX_RING_SIZE);
-- 
2.4.3

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

[PATCH 6/7] 8139cp: Dump contents of descriptor ring on TX timeout

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 2020 bytes --]

From: David Woodhouse <David.Woodhouse@intel.com>

We are seeing unexplained TX timeouts under heavy load. Let's try to get
a better idea of what's going on.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
 drivers/net/ethernet/realtek/8139cp.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index ec6bd86..686334f 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -157,6 +157,7 @@ enum {
 	NWayAdvert	= 0x66, /* MII ADVERTISE */
 	NWayLPAR	= 0x68, /* MII LPA */
 	NWayExpansion	= 0x6A, /* MII Expansion */
+	TxDmaOkLowDesc  = 0x82, /* Low 16 bit address of a Tx descriptor. */
 	Config5		= 0xD8,	/* Config5 */
 	TxPoll		= 0xD9,	/* Tell chip to check Tx descriptors for work */
 	RxMaxSize	= 0xDA, /* Max size of an Rx packet (8169 only) */
@@ -1234,7 +1235,7 @@ static void cp_tx_timeout(struct net_device *dev)
 {
 	struct cp_private *cp = netdev_priv(dev);
 	unsigned long flags;
-	int rc;
+	int rc, i;
 
 	netdev_warn(dev, "Transmit timeout, status %2x %4x %4x %4x\n",
 		    cpr8(Cmd), cpr16(CpCmd),
@@ -1242,6 +1243,17 @@ static void cp_tx_timeout(struct net_device *dev)
 
 	spin_lock_irqsave(&cp->lock, flags);
 
+	netif_dbg(cp, tx_err, cp->dev, "TX ring head %d tail %d desc %x\n",
+		  cp->tx_head, cp->tx_tail, cpr16(TxDmaOkLowDesc));
+	for (i = 0; i < CP_TX_RING_SIZE; i++) {
+		netif_dbg(cp, tx_err, cp->dev,
+			  "TX slot %d @%p: %08x (%08x) %08x %llx %p\n",
+			  i, &cp->tx_ring[i], le32_to_cpu(cp->tx_ring[i].opts1),
+			  cp->tx_opts[i], le32_to_cpu(cp->tx_ring[i].opts2),
+			  le64_to_cpu(cp->tx_ring[i].addr),
+			  cp->tx_skb[i]);
+	}
+
 	cp_stop_hw(cp);
 	cp_clean_rings(cp);
 	rc = cp_init_rings(cp);
-- 
2.4.3

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

[PATCH 7/7] 8139cp: Enable offload features by default

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1473 bytes --]

From: David Woodhouse <David.Woodhouse@intel.com>

I fixed TSO. Hardware checksum and scatter/gather also appear to be
working correctly both on real hardware and in QEMU's emulation.

Let's enable them by default and see if anyone screams...

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
 drivers/net/ethernet/realtek/8139cp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index 686334f..e5173f3 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -1983,12 +1983,12 @@ static int cp_init_one (struct pci_dev *pdev, const struct pci_device_id *ent)
 	dev->ethtool_ops = &cp_ethtool_ops;
 	dev->watchdog_timeo = TX_TIMEOUT;
 
-	dev->features |= NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX;
+	dev->features |= NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO |
+		NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX;
 
 	if (pci_using_dac)
 		dev->features |= NETIF_F_HIGHDMA;
 
-	/* disabled by default until verified */
 	dev->hw_features |= NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO |
 		NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX;
 	dev->vlan_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO |
-- 
2.4.3

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

Re: [PATCH 1/7] 8139cp: Improve accuracy of cp_interrupt() return, to survive IRQ storms

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Wed, 23 Sep 2015 09:14:09 +0100

> But sure, for now I'll drop this from the series and I can try to
> convince you separately.

Yes, let's discuss this independantly to the nice bug fixing
that's happening in the rest of this series.

Thanks.

Re: [PATCH 1/7] 8139cp: Improve accuracy of cp_interrupt() return, to survive IRQ storms

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 2174 bytes --]

On Wed, 2015-09-23 at 10:58 -0700, David Miller wrote:
> From: David Woodhouse <dwmw2@infradead.org>
> Date: Wed, 23 Sep 2015 09:14:09 +0100
> 
> > But sure, for now I'll drop this from the series and I can try to
> > convince you separately.
> 
> Yes, let's discuss this independantly to the nice bug fixing
> that's happening in the rest of this series.

Since you explicitly call them 'bug fixes'... I suppose the last patch
in the latest series (enabling the various offloads by default) isn't a
bug fix and should be held back for net-next.

I think I can live with pushing the rest for the net tree. Just to
recap, the series is:

      8139cp: Do not re-enable RX interrupts in cp_tx_timeout()
      8139cp: Fix tx_queued debug message to print correct slot numbers
      8139cp: Fix TSO/scatter-gather descriptor setup
      8139cp: Reduce duplicate csum/tso code in cp_start_xmit()
      8139cp: Fix DMA unmapping of transmitted buffers
      8139cp: Dump contents of descriptor ring on TX timeout
      8139cp: Enable offload features by default

The penultimate patch is also not strictly a bug fix, but it's only
adding additional diagnostics to a code path which was already broken 
until I fixed it anyway, so I can live with that in net although I'll
also be happy if you want to defer it.

The fourth patch which removes the three duplicate versions of the
csum/tso checks might also be deferrable but really, it's *also*
fixing the failure mode when we see an inappropriate CHECKSUM_PARTIAL
packet, and it's touching a code path which I've *also* fixed in this
patch set. So it might as well stay.

I can shuffle things around if you disagree, but assuming Francois
concurs I'd suggest merging patches 1-6 (or just pulling from
git://git.infradead.org/users/dwmw2/linux-8139cp.git fixes-only)
and then I'll resubmit the last patch some time later, after you
next pull net into net-next.

Otherwise, please let me know if/how you'd like me to reorganise it.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

Re: [PATCH 1/7] 8139cp: Improve accuracy of cp_interrupt() return, to survive IRQ storms

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Wed, 23 Sep 2015 20:45:30 +0100

> Since you explicitly call them 'bug fixes'... I suppose the last patch
> in the latest series (enabling the various offloads by default) isn't a
> bug fix and should be held back for net-next.
> 
> I think I can live with pushing the rest for the net tree. Just to
> recap, the series is:

I've applied patches #1-#6 and left #7 sitting around in patchwork
for when I next merge net into net-next, thanks.

Re: [PATCH 1/7] 8139cp: Improve accuracy of cp_interrupt() return, to survive IRQ storms

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 618 bytes --]

On Wed, 2015-09-23 at 14:48 -0700, David Miller wrote:
> I've applied patches #1-#6 and left #7 sitting around in patchwork
> for when I next merge net into net-next, thanks.

Great, thanks. I've got one more for the net tree; just masking the
gso_size to the maximum (0xfff) that the hardware can handle, and never
telling the net stack that that's our maximum, doesn't seem like such a
good idea...

I think I really am going to stop looking now :)

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

[PATCH] 8139cp: Set GSO max size and enforce it

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1823 bytes --]

The maximum MSS value we can set is 0xFFF. When fixing the TSO code I
noticed we just mask the gso_size value to the hardware's maximum and
don't care about the consequences. That seems... unsagacious.

Instead of masking, WARN and drop the packet if it's too large. And use
 netif_set_gso_max_size() so it shouldn't happen in the first place.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
 drivers/net/ethernet/realtek/8139cp.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index e5173f3..fa8f850 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -754,10 +754,16 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 	eor = (entry == (CP_TX_RING_SIZE - 1)) ? RingEnd : 0;
 	mss = skb_shinfo(skb)->gso_size;
 
+	if (mss > MSSMask) {
+		WARN_ONCE(1, "Net bug: GSO size %d too large for 8139CP\n",
+			  mss);
+		goto out_dma_error;
+	}
+
 	opts2 = cpu_to_le32(cp_tx_vlan_tag(skb));
 	opts1 = DescOwn;
 	if (mss)
-		opts1 |= LargeSend | ((mss & MSSMask) << MSSShift);
+		opts1 |= LargeSend | (mss << MSSShift);
 	else if (skb->ip_summed == CHECKSUM_PARTIAL) {
 		const struct iphdr *ip = ip_hdr(skb);
 		if (ip->protocol == IPPROTO_TCP)
@@ -1994,6 +2000,8 @@ static int cp_init_one (struct pci_dev *pdev, const struct pci_device_id *ent)
 	dev->vlan_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO |
 		NETIF_F_HIGHDMA;
 
+	netif_set_gso_max_size(dev, MSSMask);
+
 	rc = register_netdev(dev);
 	if (rc)
 		goto err_out_iomap;
-- 
2.4.3

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

Re: [PATCH 1/7] 8139cp: Improve accuracy of cp_interrupt() return, to survive IRQ storms

[Francois Romieu]

David Woodhouse <dwmw2@infradead.org> :
[...]
> But sure, for now I'll drop this from the series and I can try to
> convince you separately.

You may as well change the IRQ storm avoidance logic so that it does not
require conformant driver code if you do not want to waste more time :o)

-- 
Ueimor

Re: [PATCH 1/7] 8139cp: Improve accuracy of cp_interrupt() return, to survive IRQ storms

[Francois Romieu]

David Woodhouse <dwmw2@infradead.org> :
[...]
> I can shuffle things around if you disagree, but assuming Francois
> concurs I'd suggest merging patches 1-6 (or just pulling from
> git://git.infradead.org/users/dwmw2/linux-8139cp.git fixes-only)
> and then I'll resubmit the last patch some time later, after you
> next pull net into net-next.

No objection but I do not really trust whatever review I do this late.

Did you try #5 with the DMA allocation debug code enabled ?

-- 
Ueimor

Re: [PATCH 1/7] 8139cp: Improve accuracy of cp_interrupt() return, to survive IRQ storms

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 803 bytes --]

On Thu, 2015-09-24 at 00:44 +0200, Francois Romieu wrote:
> David Woodhouse <dwmw2@infradead.org> :
> [...]
> > I can shuffle things around if you disagree, but assuming Francois
> > concurs I'd suggest merging patches 1-6 (or just pulling from
> > git://git.infradead.org/users/dwmw2/linux-8139cp.git fixes-only)
> > and then I'll resubmit the last patch some time later, after you
> > next pull net into net-next.
> 
> No objection but I do not really trust whatever review I do this late.
> 
> Did you try #5 with the DMA allocation debug code enabled ?

Only in QEMU (where there wasn't a problem in the first place). But
yes.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

Re: [PATCH 1/7] 8139cp: Improve accuracy of cp_interrupt() return, to survive IRQ storms

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1163 bytes --]

On Thu, 2015-09-24 at 00:44 +0200, Francois Romieu wrote:
> David Woodhouse <dwmw2@infradead.org> :
> [...]
> > But sure, for now I'll drop this from the series and I can try to
> > convince you separately.
> 
> You may as well change the IRQ storm avoidance logic so that it does not
> require conformant driver code if you do not want to waste more time :o)

That's not entirely trivial — at a certain level we *do* require that
drivers don't lie to the core IRQ logic.

I suppose even if a rogue driver is returning IRQ_HANDLED in an IRQ
storm, when it's *not* actually doing anything or making any progress,
perhaps there's a way we could detect that we're always going straight
back into the next interrupt handler as *soon* as we exit the previous
one. Perhaps there's merit in exploring that option.

I'm not sure that gives you a valid excuse for not wanting to fix the
driver though :)

I might start by "clarifying" the documentation on the IRQ handler's
return value...

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

Re: [PATCH 1/7] 8139cp: Improve accuracy of cp_interrupt() return, to survive IRQ storms

[Francois Romieu]

David Woodhouse <dwmw2@infradead.org> :
[...]
> Great, thanks. I've got one more for the net tree; just masking the
> gso_size to the maximum (0xfff) that the hardware can handle, and never
> telling the net stack that that's our maximum, doesn't seem like such a
> good idea...

0xfff, really ?

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index d79e33b..8c6811d 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -174,7 +180,7 @@ enum {
 	LastFrag	= (1 << 28), /* Final segment of a packet */
 	LargeSend	= (1 << 27), /* TCP Large Send Offload (TSO) */
 	MSSShift	= 16,	     /* MSS value position */
-	MSSMask		= 0xfff,     /* MSS value: 11 bits */
+	MSSMask		= 0x7ff,     /* MSS value: 11 bits */
 	TxError		= (1 << 23), /* Tx error summary */
 	RxError		= (1 << 20), /* Rx error summary */
 	IPCS		= (1 << 18), /* Calculate IP checksum */
-- 
Ueimor

[PATCH WTF] 8139cp: Fix GSO MSS handling

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 2786 bytes --]

When fixing the TSO support I noticed we just mask skb->gso_size with
the MSSMask value and don't care about the consequences.

That seems... unsagacious.

Instead of masking, WARN and drop the packet if the maximum is exceeded.
And call netif_set_gso_max_size() so it shouldn't happen in the first
place.

Finally, Francois Romieu noticed that we didn't even have the right
value for MSSMask anyway; it should be 0x7ff (11 bits) not 0xfff.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---

Nice and simple and obvious, right?

So why does it stop TSO from happening at all?

When I call netif_set_gso_max_size() with a value of 0xaad (2733) or
higher, I see TSO being used (with an MSS of 1214; this is just between
a VM and its host; Legacy IP with an MTU of 1500).

If I set a maximum of 0xaac or lower, TSO never gets used. Am I doing
something wrong? Or is there a problem somewhere else?

 drivers/net/ethernet/realtek/8139cp.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index 686334f..ae24d42 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -175,7 +175,7 @@ enum {
 	LastFrag	= (1 << 28), /* Final segment of a packet */
 	LargeSend	= (1 << 27), /* TCP Large Send Offload (TSO) */
 	MSSShift	= 16,	     /* MSS value position */
-	MSSMask		= 0xfff,     /* MSS value: 11 bits */
+	MSSMask		= 0x7ff,     /* MSS value: 11 bits */
 	TxError		= (1 << 23), /* Tx error summary */
 	RxError		= (1 << 20), /* Rx error summary */
 	IPCS		= (1 << 18), /* Calculate IP checksum */
@@ -754,10 +754,16 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 	eor = (entry == (CP_TX_RING_SIZE - 1)) ? RingEnd : 0;
 	mss = skb_shinfo(skb)->gso_size;
 
+	if (mss > MSSMask) {
+		WARN_ONCE(1, "Net bug: GSO size %d too large for 8139CP\n",
+			  mss);
+		goto out_dma_error;
+	}
+
 	opts2 = cpu_to_le32(cp_tx_vlan_tag(skb));
 	opts1 = DescOwn;
 	if (mss)
-		opts1 |= LargeSend | ((mss & MSSMask) << MSSShift);
+		opts1 |= LargeSend | (mss << MSSShift);
 	else if (skb->ip_summed == CHECKSUM_PARTIAL) {
 		const struct iphdr *ip = ip_hdr(skb);
 		if (ip->protocol == IPPROTO_TCP)
@@ -1994,6 +2000,8 @@ static int cp_init_one (struct pci_dev *pdev, const struct pci_device_id *ent)
 	dev->vlan_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO |
 		NETIF_F_HIGHDMA;
 
+	netif_set_gso_max_size(dev, MSSMask);
+
 	rc = register_netdev(dev);
 	if (rc)
 		goto err_out_iomap;
-- 
2.4.3

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

[PATCH v2 RFC] 8139cp: Fix GSO MSS handling

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 3719 bytes --]

When fixing the TSO support I noticed we just mask ->gso_size with the
MSSMask value and don't care about the consequences.

Provide a .ndo_features_check() method which drops the NETIF_F_TSO
feature for any skb which would exceed the maximum, and thus forces it
to be segmented by software.

Then we can stop the masking in cp_start_xmit(), and just WARN if the
maximum is exceeded, which should now never happen.

Finally, Francois Romieu noticed that we didn't even have the right
value for MSSMask anyway; it should be 0x7ff (11 bits) not 0xfff.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
OK, netif_set_gso_max_size() was *not*, as I had naïvely assumed, a way
to set the maximum value of ->gso_size. That's an unfortunate set of
naming decisions.

I don't see a simple way to tell the net stack about the maximum MSS
value we can cope with in the general case. But we can do it this way
by clearing NETIF_F_TSO on a per-packet basis instead.

Incidentally, other drivers might benefit from doing the same if this
is considered reasonable. For example it looks like the r8169 driver
manually calls skb_gso_segment() for its fallback case, and I don't see
what then prevents it from hitting the "BUG!" at the start of
rtl8169_start_xmit() if it runs out of space in the descriptor ring.

 drivers/net/ethernet/realtek/8139cp.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index 686334f..fe1040d 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -175,7 +175,7 @@ enum {
 	LastFrag	= (1 << 28), /* Final segment of a packet */
 	LargeSend	= (1 << 27), /* TCP Large Send Offload (TSO) */
 	MSSShift	= 16,	     /* MSS value position */
-	MSSMask		= 0xfff,     /* MSS value: 11 bits */
+	MSSMask		= 0x7ff,     /* MSS value: 11 bits */
 	TxError		= (1 << 23), /* Tx error summary */
 	RxError		= (1 << 20), /* Rx error summary */
 	IPCS		= (1 << 18), /* Calculate IP checksum */
@@ -754,10 +754,16 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 	eor = (entry == (CP_TX_RING_SIZE - 1)) ? RingEnd : 0;
 	mss = skb_shinfo(skb)->gso_size;
 
+	if (mss > MSSMask) {
+		WARN_ONCE(1, "Net bug: GSO size %d too large for 8139CP\n",
+			  mss);
+		goto out_dma_error;
+	}
+
 	opts2 = cpu_to_le32(cp_tx_vlan_tag(skb));
 	opts1 = DescOwn;
 	if (mss)
-		opts1 |= LargeSend | ((mss & MSSMask) << MSSShift);
+		opts1 |= LargeSend | (mss << MSSShift);
 	else if (skb->ip_summed == CHECKSUM_PARTIAL) {
 		const struct iphdr *ip = ip_hdr(skb);
 		if (ip->protocol == IPPROTO_TCP)
@@ -1852,6 +1858,15 @@ static void cp_set_d3_state (struct cp_private *cp)
 	pci_set_power_state (cp->pdev, PCI_D3hot);
 }
 
+static netdev_features_t cp_features_check(struct sk_buff *skb,
+					   struct net_device *dev,
+					   netdev_features_t features)
+{
+	if (skb_shinfo(skb)->gso_size > MSSMask)
+		features &= ~NETIF_F_TSO;
+
+	return vlan_features_check(skb, features);
+}
 static const struct net_device_ops cp_netdev_ops = {
 	.ndo_open		= cp_open,
 	.ndo_stop		= cp_close,
@@ -1864,6 +1879,7 @@ static const struct net_device_ops cp_netdev_ops = {
 	.ndo_tx_timeout		= cp_tx_timeout,
 	.ndo_set_features	= cp_set_features,
 	.ndo_change_mtu		= cp_change_mtu,
+	.ndo_features_check	= cp_features_check,
 
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller	= cp_poll_controller,
-- 
2.4.3

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

Re: [PATCH v2 RFC] 8139cp: Fix GSO MSS handling

[Eric Dumazet]

On Thu, 2015-09-24 at 11:38 +0100, David Woodhouse wrote:
> When fixing the TSO support I noticed we just mask ->gso_size with the
> MSSMask value and don't care about the consequences.
> 
> Provide a .ndo_features_check() method which drops the NETIF_F_TSO
> feature for any skb which would exceed the maximum, and thus forces it
> to be segmented by software.
> 
> Then we can stop the masking in cp_start_xmit(), and just WARN if the
> maximum is exceeded, which should now never happen.
> 
> Finally, Francois Romieu noticed that we didn't even have the right
> value for MSSMask anyway; it should be 0x7ff (11 bits) not 0xfff.
> 
> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> ---
> OK, netif_set_gso_max_size() was *not*, as I had naïvely assumed, a way
> to set the maximum value of ->gso_size. That's an unfortunate set of
> naming decisions.

Right, netif_skb_features() only has the following checks :

if (gso_segs > dev->gso_max_segs || gso_segs < dev->gso_min_segs)
    features &= ~NETIF_F_GSO_MASK;

But now we have .ndo_features_check() we could remove this generic check
from fast path.

Re: [PATCH v2 RFC] 8139cp: Fix GSO MSS handling

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 979 bytes --]

On Thu, 2015-09-24 at 05:05 -0700, Eric Dumazet wrote:
> 
> Right, netif_skb_features() only has the following checks :
> 
> if (gso_segs > dev->gso_max_segs || gso_segs < dev->gso_min_segs)
>     features &= ~NETIF_F_GSO_MASK;
> 
> But now we have .ndo_features_check() we could remove this generic 
> check from fast path.

Perhaps so, yes.

Any thoughts on the other reason I was staring at this same code path
this week? I am able to reliably feed inappropriate packets to a
NETIF_F_IP_CSUM-capable device with the test program at
http://bombadil.infradead.org/~dwmw2/raw.c (and equivalent code paths
via virtio_net, tun and others).

They're *supposed* to get checksummed by software if the device can't
cope, but netif_skb_features() returns the wrong answer, so we fail to
do that and they're fed with CHECKSUM_PARTIAL to a device which can't
handle them. Causing a WARN() or a BUG() or a silent corruption,
depending on the driver.

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

Re: [PATCH v2 RFC] 8139cp: Fix GSO MSS handling

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Thu, 24 Sep 2015 11:38:22 +0100

> When fixing the TSO support I noticed we just mask ->gso_size with the
> MSSMask value and don't care about the consequences.
> 
> Provide a .ndo_features_check() method which drops the NETIF_F_TSO
> feature for any skb which would exceed the maximum, and thus forces it
> to be segmented by software.
> 
> Then we can stop the masking in cp_start_xmit(), and just WARN if the
> maximum is exceeded, which should now never happen.
> 
> Finally, Francois Romieu noticed that we didn't even have the right
> value for MSSMask anyway; it should be 0x7ff (11 bits) not 0xfff.
> 
> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>

I'm going to apply this to net-next, thanks David.

Re: [PATCH v2 RFC] 8139cp: Fix GSO MSS handling

[Tom Herbert]

On Thu, Sep 24, 2015 at 5:31 AM, David Woodhouse <dwmw2@infradead.org> wrote:
> On Thu, 2015-09-24 at 05:05 -0700, Eric Dumazet wrote:
>>
>> Right, netif_skb_features() only has the following checks :
>>
>> if (gso_segs > dev->gso_max_segs || gso_segs < dev->gso_min_segs)
>>     features &= ~NETIF_F_GSO_MASK;
>>
>> But now we have .ndo_features_check() we could remove this generic
>> check from fast path.
>
> Perhaps so, yes.
>
> Any thoughts on the other reason I was staring at this same code path
> this week? I am able to reliably feed inappropriate packets to a
> NETIF_F_IP_CSUM-capable device with the test program at
> http://bombadil.infradead.org/~dwmw2/raw.c (and equivalent code paths
> via virtio_net, tun and others).
>
> They're *supposed* to get checksummed by software if the device can't
> cope, but netif_skb_features() returns the wrong answer, so we fail to
> do that and they're fed with CHECKSUM_PARTIAL to a device which can't
> handle them. Causing a WARN() or a BUG() or a silent corruption,
> depending on the driver.
>
Which drivers are doing this? It is up to the driver to determine
whether a particular packet being sent can have checksum offloaded to
the device. If it cannot offload the checksum it must call
skb_checksum_help.

Tom

> --
> dwmw2
>

Re: [PATCH v2 RFC] 8139cp: Fix GSO MSS handling

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1624 bytes --]

On Sun, 2015-09-27 at 22:37 -0700, Tom Herbert wrote:
> 
> Which drivers are doing this? It is up to the driver to determine
> whether a particular packet being sent can have checksum offloaded to
> the device. If it cannot offload the checksum it must call
> skb_checksum_help.

Not so.

A driver sets the NETIF_F_IP_CSUM feature to indicate that it can do
the checksum on Legacy IP TCP or UDP frames and *nothing* else.

It most certainly does not expect to be handed any other kind of packet
for checksumming, and bad things will often happen if it is. If drivers
*do* spot that they've been given something they don't handle, I see
BUG() calls and warnings, but I don't see any of them calling
skb_checksum_help() to silently cope. Many of them just feed it to the
hardware and don't even notice at all because it's the *hardware* which
decides whether to do a TCP or a UDP checksum. So who knows what'll
happen.

The check is supposed to be done in can_checksum_protocol(), called
from harmonize_features(). But as noted, that check has false positives
and lets some inappropriate packets through — for NETIF_F_IP_CSUM it
lets through *all* skbuffs with ->protocol == ETH_P_IP instead of only
TCP and UDP.

I originally couldn't see how to deal with this except by looking at
the contents of the packet, which sucked. But I think I've found a
somewhat more acceptable approach now:
http://lists.openwall.net/netdev/2015/09/25/85

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

Re: [PATCH 1/7] 8139cp: Improve accuracy of cp_interrupt() return, to survive IRQ storms

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 880 bytes --]

On Wed, 2015-09-23 at 10:58 -0700, David Miller wrote:
> From: David Woodhouse <dwmw2@infradead.org>
> Date: Wed, 23 Sep 2015 09:14:09 +0100
> 
> > But sure, for now I'll drop this from the series and I can try to
> > convince you separately.
> 
> Yes, let's discuss this independantly to the nice bug fixing
> that's happening in the rest of this series.
> 
> Thanks.

This is the patch we discussed earlier, and I came away from the
conversation believing that I had achieved my goal of trying to
convince you separately :)

The patch in patchwork at https://patchwork.ozlabs.org/patch/520318/
still applies cleanly; is that sufficient or would you like it
resubmitted (perhaps after -rc1)?

Thanks.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]