[PATCH 1/6] r8169: check dma mapping failures

[PATCH 1/6] r8169: check dma mapping failures

[Stanislaw Gruszka]

Check possible dma mapping errors and do clean up if it happens,
when sending frames stop the tx queue.

Fix overwrap bug in rtl8169_tx_clear on the way.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
All patches in series was tested on RTL8111/8168B

 drivers/net/r8169.c |   69 ++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 46 insertions(+), 23 deletions(-)

diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index bc669a4..a6c4f90 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -4018,20 +4018,25 @@ static struct sk_buff *rtl8169_alloc_rx_skb(struct pci_dev *pdev,
 
 	skb = __netdev_alloc_skb(dev, rx_buf_sz + pad, gfp);
 	if (!skb)
-		goto err_out;
+		goto err_out_0;
 
 	skb_reserve(skb, align ? ((pad - 1) & (unsigned long)skb->data) : pad);
 
 	mapping = dma_map_single(&pdev->dev, skb->data, rx_buf_sz,
 				 PCI_DMA_FROMDEVICE);
+	if (dma_mapping_error(&pdev->dev, mapping))
+		goto err_free_skb_1;
 
 	rtl8169_map_to_asic(desc, mapping, rx_buf_sz);
-out:
+
 	return skb;
 
-err_out:
+err_free_skb_1:
+	dev_kfree_skb(skb);
+
+err_out_0:
 	rtl8169_make_unusable_by_asic(desc);
-	goto out;
+	return NULL;
 }
 
 static void rtl8169_rx_clear(struct rtl8169_private *tp)
@@ -4115,12 +4120,12 @@ static void rtl8169_unmap_tx_skb(struct pci_dev *pdev, struct ring_info *tx_skb,
 	tx_skb->len = 0;
 }
 
-static void rtl8169_tx_clear(struct rtl8169_private *tp)
+static void rtl8169_tx_clear_range(struct rtl8169_private *tp, u32 start, int n)
 {
-	unsigned int i;
+	int i;
 
-	for (i = tp->dirty_tx; i < tp->dirty_tx + NUM_TX_DESC; i++) {
-		unsigned int entry = i % NUM_TX_DESC;
+	for (i = 0; i < n; i++) {
+		unsigned int entry = (start + i) % NUM_TX_DESC;
 		struct ring_info *tx_skb = tp->tx_skb + entry;
 		unsigned int len = tx_skb->len;
 
@@ -4136,6 +4141,11 @@ static void rtl8169_tx_clear(struct rtl8169_private *tp)
 			tp->dev->stats.tx_dropped++;
 		}
 	}
+}
+
+static void rtl8169_tx_clear(struct rtl8169_private *tp)
+{
+	rtl8169_tx_clear_range(tp, tp->dirty_tx, NUM_TX_DESC);
 	tp->cur_tx = tp->dirty_tx = 0;
 }
 
@@ -4254,6 +4264,8 @@ static int rtl8169_xmit_frags(struct rtl8169_private *tp, struct sk_buff *skb,
 		addr = ((void *) page_address(frag->page)) + frag->page_offset;
 		mapping = dma_map_single(&tp->pci_dev->dev, addr, len,
 					 PCI_DMA_TODEVICE);
+		if (unlikely(dma_mapping_error(&tp->pci_dev->dev, mapping)))
+			goto err_out;
 
 		/* anti gcc 2.95.3 bugware (sic) */
 		status = opts1 | len | (RingEnd * !((entry + 1) % NUM_TX_DESC));
@@ -4270,6 +4282,10 @@ static int rtl8169_xmit_frags(struct rtl8169_private *tp, struct sk_buff *skb,
 	}
 
 	return cur_frag;
+
+err_out:
+	rtl8169_tx_clear_range(tp, tp->cur_tx, cur_frag);
+	return -EIO;
 }
 
 static inline u32 rtl8169_tso_csum(struct sk_buff *skb, struct net_device *dev)
@@ -4296,40 +4312,44 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
 				      struct net_device *dev)
 {
 	struct rtl8169_private *tp = netdev_priv(dev);
-	unsigned int frags, entry = tp->cur_tx % NUM_TX_DESC;
+	unsigned int entry = tp->cur_tx % NUM_TX_DESC;
 	struct TxDesc *txd = tp->TxDescArray + entry;
 	void __iomem *ioaddr = tp->mmio_addr;
 	dma_addr_t mapping;
 	u32 status, len;
 	u32 opts1;
+	int frags;
 
 	if (unlikely(TX_BUFFS_AVAIL(tp) < skb_shinfo(skb)->nr_frags)) {
 		netif_err(tp, drv, dev, "BUG! Tx Ring full when queue awake!\n");
-		goto err_stop;
+		goto err_stop_0;
 	}
 
 	if (unlikely(le32_to_cpu(txd->opts1) & DescOwn))
-		goto err_stop;
+		goto err_stop_0;
+
+	len = skb_headlen(skb);
+	mapping = dma_map_single(&tp->pci_dev->dev, skb->data, len,
+				 PCI_DMA_TODEVICE);
+	if (unlikely(dma_mapping_error(&tp->pci_dev->dev, mapping)))
+		goto err_stop_0;
+
+	tp->tx_skb[entry].len = len;
+	txd->addr = cpu_to_le64(mapping);
+	txd->opts2 = cpu_to_le32(rtl8169_tx_vlan_tag(tp, skb));
 
 	opts1 = DescOwn | rtl8169_tso_csum(skb, dev);
 
 	frags = rtl8169_xmit_frags(tp, skb, opts1);
-	if (frags) {
-		len = skb_headlen(skb);
+	if (frags < 0)
+		goto err_dma_1;
+	else if (frags)
 		opts1 |= FirstFrag;
-	} else {
-		len = skb->len;
+	else {
 		opts1 |= FirstFrag | LastFrag;
 		tp->tx_skb[entry].skb = skb;
 	}
 
-	mapping = dma_map_single(&tp->pci_dev->dev, skb->data, len,
-				 PCI_DMA_TODEVICE);
-
-	tp->tx_skb[entry].len = len;
-	txd->addr = cpu_to_le64(mapping);
-	txd->opts2 = cpu_to_le32(rtl8169_tx_vlan_tag(tp, skb));
-
 	wmb();
 
 	/* anti gcc 2.95.3 bugware (sic) */
@@ -4351,7 +4371,10 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
 
 	return NETDEV_TX_OK;
 
-err_stop:
+err_dma_1:
+	rtl8169_unmap_tx_skb(tp->pci_dev, tp->tx_skb + entry, txd);
+
+err_stop_0:
 	netif_stop_queue(dev);
 	dev->stats.tx_dropped++;
 	return NETDEV_TX_BUSY;
-- 
1.7.1

[PATCH 2/6] r8169: reduce number of functions arguments

[Stanislaw Gruszka]

We don't need to pass arguments on stack since we have them in per
device private structure.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/r8169.c |   30 ++++++++++++------------------
 1 files changed, 12 insertions(+), 18 deletions(-)

diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index a6c4f90..86be06c 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -4005,29 +4005,26 @@ static inline void rtl8169_map_to_asic(struct RxDesc *desc, dma_addr_t mapping,
 	rtl8169_mark_to_asic(desc, rx_buf_sz);
 }
 
-static struct sk_buff *rtl8169_alloc_rx_skb(struct pci_dev *pdev,
-					    struct net_device *dev,
-					    struct RxDesc *desc, int rx_buf_sz,
-					    unsigned int align, gfp_t gfp)
+static struct sk_buff *rtl8169_alloc_rx_skb(struct rtl8169_private *tp,
+					    struct RxDesc *desc, gfp_t gfp)
 {
 	struct sk_buff *skb;
 	dma_addr_t mapping;
-	unsigned int pad;
+	unsigned int align = tp->align;
+	unsigned int pad = align ? align : NET_IP_ALIGN;
 
-	pad = align ? align : NET_IP_ALIGN;
-
-	skb = __netdev_alloc_skb(dev, rx_buf_sz + pad, gfp);
+	skb = __netdev_alloc_skb(tp->dev, tp->rx_buf_sz + pad, gfp);
 	if (!skb)
 		goto err_out_0;
 
 	skb_reserve(skb, align ? ((pad - 1) & (unsigned long)skb->data) : pad);
 
-	mapping = dma_map_single(&pdev->dev, skb->data, rx_buf_sz,
+	mapping = dma_map_single(&tp->pci_dev->dev, skb->data, tp->rx_buf_sz,
 				 PCI_DMA_FROMDEVICE);
-	if (dma_mapping_error(&pdev->dev, mapping))
+	if (dma_mapping_error(&tp->pci_dev->dev, mapping))
 		goto err_free_skb_1;
 
-	rtl8169_map_to_asic(desc, mapping, rx_buf_sz);
+	rtl8169_map_to_asic(desc, mapping, tp->rx_buf_sz);
 
 	return skb;
 
@@ -4051,8 +4048,7 @@ static void rtl8169_rx_clear(struct rtl8169_private *tp)
 	}
 }
 
-static u32 rtl8169_rx_fill(struct rtl8169_private *tp, struct net_device *dev,
-			   u32 start, u32 end, gfp_t gfp)
+static u32 rtl8169_rx_fill(struct rtl8169_private *tp, u32 start, u32 end, gfp_t gfp)
 {
 	u32 cur;
 
@@ -4065,9 +4061,7 @@ static u32 rtl8169_rx_fill(struct rtl8169_private *tp, struct net_device *dev,
 		if (tp->Rx_skbuff[i])
 			continue;
 
-		skb = rtl8169_alloc_rx_skb(tp->pci_dev, dev,
-					   tp->RxDescArray + i,
-					   tp->rx_buf_sz, tp->align, gfp);
+		skb = rtl8169_alloc_rx_skb(tp, tp->RxDescArray + i, gfp);
 		if (!skb)
 			break;
 
@@ -4095,7 +4089,7 @@ static int rtl8169_init_ring(struct net_device *dev)
 	memset(tp->tx_skb, 0x0, NUM_TX_DESC * sizeof(struct ring_info));
 	memset(tp->Rx_skbuff, 0x0, NUM_RX_DESC * sizeof(struct sk_buff *));
 
-	if (rtl8169_rx_fill(tp, dev, 0, NUM_RX_DESC, GFP_KERNEL) != NUM_RX_DESC)
+	if (rtl8169_rx_fill(tp, 0, NUM_RX_DESC, GFP_KERNEL) != NUM_RX_DESC)
 		goto err_out;
 
 	rtl8169_mark_as_last_descriptor(tp->RxDescArray + NUM_RX_DESC - 1);
@@ -4615,7 +4609,7 @@ static int rtl8169_rx_interrupt(struct net_device *dev,
 	count = cur_rx - tp->cur_rx;
 	tp->cur_rx = cur_rx;
 
-	delta = rtl8169_rx_fill(tp, dev, tp->dirty_rx, tp->cur_rx, GFP_ATOMIC);
+	delta = rtl8169_rx_fill(tp, tp->dirty_rx, tp->cur_rx, GFP_ATOMIC);
 	if (!delta && count)
 		netif_info(tp, intr, dev, "no Rx buffer allocated\n");
 	tp->dirty_rx += delta;
-- 
1.7.1

[PATCH 3/6] r8169: replace PCI_DMA_{TO,FROM}DEVICE to DMA_{TO,FROM}_DEVICE

[Stanislaw Gruszka]

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/r8169.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index 86be06c..1eafe9b 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -3984,7 +3984,7 @@ static void rtl8169_free_rx_skb(struct rtl8169_private *tp,
 	struct pci_dev *pdev = tp->pci_dev;
 
 	dma_unmap_single(&pdev->dev, le64_to_cpu(desc->addr), tp->rx_buf_sz,
-			 PCI_DMA_FROMDEVICE);
+			 DMA_FROM_DEVICE);
 	dev_kfree_skb(*sk_buff);
 	*sk_buff = NULL;
 	rtl8169_make_unusable_by_asic(desc);
@@ -4020,7 +4020,7 @@ static struct sk_buff *rtl8169_alloc_rx_skb(struct rtl8169_private *tp,
 	skb_reserve(skb, align ? ((pad - 1) & (unsigned long)skb->data) : pad);
 
 	mapping = dma_map_single(&tp->pci_dev->dev, skb->data, tp->rx_buf_sz,
-				 PCI_DMA_FROMDEVICE);
+				 DMA_FROM_DEVICE);
 	if (dma_mapping_error(&tp->pci_dev->dev, mapping))
 		goto err_free_skb_1;
 
@@ -4107,7 +4107,7 @@ static void rtl8169_unmap_tx_skb(struct pci_dev *pdev, struct ring_info *tx_skb,
 	unsigned int len = tx_skb->len;
 
 	dma_unmap_single(&pdev->dev, le64_to_cpu(desc->addr), len,
-			 PCI_DMA_TODEVICE);
+			 DMA_TO_DEVICE);
 	desc->opts1 = 0x00;
 	desc->opts2 = 0x00;
 	desc->addr = 0x00;
@@ -4257,7 +4257,7 @@ static int rtl8169_xmit_frags(struct rtl8169_private *tp, struct sk_buff *skb,
 		len = frag->size;
 		addr = ((void *) page_address(frag->page)) + frag->page_offset;
 		mapping = dma_map_single(&tp->pci_dev->dev, addr, len,
-					 PCI_DMA_TODEVICE);
+					 DMA_TO_DEVICE);
 		if (unlikely(dma_mapping_error(&tp->pci_dev->dev, mapping)))
 			goto err_out;
 
@@ -4324,7 +4324,7 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
 
 	len = skb_headlen(skb);
 	mapping = dma_map_single(&tp->pci_dev->dev, skb->data, len,
-				 PCI_DMA_TODEVICE);
+				 DMA_TO_DEVICE);
 	if (unlikely(dma_mapping_error(&tp->pci_dev->dev, mapping)))
 		goto err_stop_0;
 
@@ -4505,7 +4505,7 @@ static inline bool rtl8169_try_rx_copy(struct sk_buff **sk_buff,
 		goto out;
 
 	dma_sync_single_for_cpu(&tp->pci_dev->dev, addr, pkt_size,
-				PCI_DMA_FROMDEVICE);
+				DMA_FROM_DEVICE);
 	skb_copy_from_linear_data(*sk_buff, skb->data, pkt_size);
 	*sk_buff = skb;
 	done = true;
@@ -4575,11 +4575,11 @@ static int rtl8169_rx_interrupt(struct net_device *dev,
 
 			if (rtl8169_try_rx_copy(&skb, tp, pkt_size, addr)) {
 				dma_sync_single_for_device(&pdev->dev, addr,
-					pkt_size, PCI_DMA_FROMDEVICE);
+					pkt_size, DMA_FROM_DEVICE);
 				rtl8169_mark_to_asic(desc, tp->rx_buf_sz);
 			} else {
 				dma_unmap_single(&pdev->dev, addr, tp->rx_buf_sz,
-						 PCI_DMA_FROMDEVICE);
+						 DMA_FROM_DEVICE);
 				tp->Rx_skbuff[entry] = NULL;
 			}
 
-- 
1.7.1

[PATCH 4/6] r8169: introduce some more local variables

[Stanislaw Gruszka]

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/r8169.c |   50 ++++++++++++++++++++++++++------------------------
 1 files changed, 26 insertions(+), 24 deletions(-)

diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index 1eafe9b..f79ddb2 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -3981,9 +3981,9 @@ static inline void rtl8169_make_unusable_by_asic(struct RxDesc *desc)
 static void rtl8169_free_rx_skb(struct rtl8169_private *tp,
 				struct sk_buff **sk_buff, struct RxDesc *desc)
 {
-	struct pci_dev *pdev = tp->pci_dev;
+	struct device *d = &tp->pci_dev->dev;
 
-	dma_unmap_single(&pdev->dev, le64_to_cpu(desc->addr), tp->rx_buf_sz,
+	dma_unmap_single(d, le64_to_cpu(desc->addr), tp->rx_buf_sz,
 			 DMA_FROM_DEVICE);
 	dev_kfree_skb(*sk_buff);
 	*sk_buff = NULL;
@@ -4010,21 +4010,22 @@ static struct sk_buff *rtl8169_alloc_rx_skb(struct rtl8169_private *tp,
 {
 	struct sk_buff *skb;
 	dma_addr_t mapping;
+	struct device *d = &tp->pci_dev->dev;
+	unsigned int rx_buf_sz = tp->rx_buf_sz;
 	unsigned int align = tp->align;
 	unsigned int pad = align ? align : NET_IP_ALIGN;
 
-	skb = __netdev_alloc_skb(tp->dev, tp->rx_buf_sz + pad, gfp);
+	skb = __netdev_alloc_skb(tp->dev, rx_buf_sz + pad, gfp);
 	if (!skb)
 		goto err_out_0;
 
 	skb_reserve(skb, align ? ((pad - 1) & (unsigned long)skb->data) : pad);
 
-	mapping = dma_map_single(&tp->pci_dev->dev, skb->data, tp->rx_buf_sz,
-				 DMA_FROM_DEVICE);
-	if (dma_mapping_error(&tp->pci_dev->dev, mapping))
+	mapping = dma_map_single(d, skb->data, rx_buf_sz, DMA_FROM_DEVICE);
+	if (dma_mapping_error(d, mapping))
 		goto err_free_skb_1;
 
-	rtl8169_map_to_asic(desc, mapping, tp->rx_buf_sz);
+	rtl8169_map_to_asic(desc, mapping, rx_buf_sz);
 
 	return skb;
 
@@ -4101,13 +4102,13 @@ err_out:
 	return -ENOMEM;
 }
 
-static void rtl8169_unmap_tx_skb(struct pci_dev *pdev, struct ring_info *tx_skb,
+static void rtl8169_unmap_tx_skb(struct device *d, struct ring_info *tx_skb,
 				 struct TxDesc *desc)
 {
 	unsigned int len = tx_skb->len;
 
-	dma_unmap_single(&pdev->dev, le64_to_cpu(desc->addr), len,
-			 DMA_TO_DEVICE);
+	dma_unmap_single(d, le64_to_cpu(desc->addr), len, DMA_TO_DEVICE);
+
 	desc->opts1 = 0x00;
 	desc->opts2 = 0x00;
 	desc->addr = 0x00;
@@ -4117,6 +4118,7 @@ static void rtl8169_unmap_tx_skb(struct pci_dev *pdev, struct ring_info *tx_skb,
 static void rtl8169_tx_clear_range(struct rtl8169_private *tp, u32 start, int n)
 {
 	int i;
+	struct device *d = &tp->pci_dev->dev;
 
 	for (i = 0; i < n; i++) {
 		unsigned int entry = (start + i) % NUM_TX_DESC;
@@ -4126,8 +4128,7 @@ static void rtl8169_tx_clear_range(struct rtl8169_private *tp, u32 start, int n)
 		if (len) {
 			struct sk_buff *skb = tx_skb->skb;
 
-			rtl8169_unmap_tx_skb(tp->pci_dev, tx_skb,
-					     tp->TxDescArray + entry);
+			rtl8169_unmap_tx_skb(d, tx_skb, tp->TxDescArray + entry);
 			if (skb) {
 				dev_kfree_skb(skb);
 				tx_skb->skb = NULL;
@@ -4243,6 +4244,7 @@ static int rtl8169_xmit_frags(struct rtl8169_private *tp, struct sk_buff *skb,
 	struct skb_shared_info *info = skb_shinfo(skb);
 	unsigned int cur_frag, entry;
 	struct TxDesc * uninitialized_var(txd);
+	struct device *d = &tp->pci_dev->dev;
 
 	entry = tp->cur_tx;
 	for (cur_frag = 0; cur_frag < info->nr_frags; cur_frag++) {
@@ -4256,9 +4258,8 @@ static int rtl8169_xmit_frags(struct rtl8169_private *tp, struct sk_buff *skb,
 		txd = tp->TxDescArray + entry;
 		len = frag->size;
 		addr = ((void *) page_address(frag->page)) + frag->page_offset;
-		mapping = dma_map_single(&tp->pci_dev->dev, addr, len,
-					 DMA_TO_DEVICE);
-		if (unlikely(dma_mapping_error(&tp->pci_dev->dev, mapping)))
+		mapping = dma_map_single(d, addr, len, DMA_TO_DEVICE);
+		if (unlikely(dma_mapping_error(d, mapping)))
 			goto err_out;
 
 		/* anti gcc 2.95.3 bugware (sic) */
@@ -4309,6 +4310,7 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
 	unsigned int entry = tp->cur_tx % NUM_TX_DESC;
 	struct TxDesc *txd = tp->TxDescArray + entry;
 	void __iomem *ioaddr = tp->mmio_addr;
+	struct device *d = &tp->pci_dev->dev;
 	dma_addr_t mapping;
 	u32 status, len;
 	u32 opts1;
@@ -4323,9 +4325,8 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
 		goto err_stop_0;
 
 	len = skb_headlen(skb);
-	mapping = dma_map_single(&tp->pci_dev->dev, skb->data, len,
-				 DMA_TO_DEVICE);
-	if (unlikely(dma_mapping_error(&tp->pci_dev->dev, mapping)))
+	mapping = dma_map_single(d, skb->data, len, DMA_TO_DEVICE);
+	if (unlikely(dma_mapping_error(d, mapping)))
 		goto err_stop_0;
 
 	tp->tx_skb[entry].len = len;
@@ -4366,7 +4367,7 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
 	return NETDEV_TX_OK;
 
 err_dma_1:
-	rtl8169_unmap_tx_skb(tp->pci_dev, tp->tx_skb + entry, txd);
+	rtl8169_unmap_tx_skb(d, tp->tx_skb + entry, txd);
 
 err_stop_0:
 	netif_stop_queue(dev);
@@ -4444,7 +4445,8 @@ static void rtl8169_tx_interrupt(struct net_device *dev,
 		dev->stats.tx_bytes += len;
 		dev->stats.tx_packets++;
 
-		rtl8169_unmap_tx_skb(tp->pci_dev, tx_skb, tp->TxDescArray + entry);
+		rtl8169_unmap_tx_skb(&tp->pci_dev->dev, tx_skb,
+				     tp->TxDescArray + entry);
 
 		if (status & LastFrag) {
 			dev_kfree_skb(tx_skb->skb);
@@ -4559,7 +4561,7 @@ static int rtl8169_rx_interrupt(struct net_device *dev,
 			struct sk_buff *skb = tp->Rx_skbuff[entry];
 			dma_addr_t addr = le64_to_cpu(desc->addr);
 			int pkt_size = (status & 0x00001FFF) - 4;
-			struct pci_dev *pdev = tp->pci_dev;
+			struct device *d = &tp->pci_dev->dev;
 
 			/*
 			 * The driver does not support incoming fragmented
@@ -4574,11 +4576,11 @@ static int rtl8169_rx_interrupt(struct net_device *dev,
 			}
 
 			if (rtl8169_try_rx_copy(&skb, tp, pkt_size, addr)) {
-				dma_sync_single_for_device(&pdev->dev, addr,
-					pkt_size, DMA_FROM_DEVICE);
+				dma_sync_single_for_device(d, addr, pkt_size,
+							   DMA_FROM_DEVICE);
 				rtl8169_mark_to_asic(desc, tp->rx_buf_sz);
 			} else {
-				dma_unmap_single(&pdev->dev, addr, tp->rx_buf_sz,
+				dma_unmap_single(d, addr, tp->rx_buf_sz,
 						 DMA_FROM_DEVICE);
 				tp->Rx_skbuff[entry] = NULL;
 			}
-- 
1.7.1

[PATCH 5/6] r8169: do not account fragments as packets

[Stanislaw Gruszka]

Only increase tx_{packets,dropped} statistics when transmit or drop
full skb, not just fragment.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/r8169.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index f79ddb2..0ef49b4 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -4130,10 +4130,10 @@ static void rtl8169_tx_clear_range(struct rtl8169_private *tp, u32 start, int n)
 
 			rtl8169_unmap_tx_skb(d, tx_skb, tp->TxDescArray + entry);
 			if (skb) {
+				tp->dev->stats.tx_dropped++;
 				dev_kfree_skb(skb);
 				tx_skb->skb = NULL;
 			}
-			tp->dev->stats.tx_dropped++;
 		}
 	}
 }
@@ -4443,12 +4443,12 @@ static void rtl8169_tx_interrupt(struct net_device *dev,
 			break;
 
 		dev->stats.tx_bytes += len;
-		dev->stats.tx_packets++;
 
 		rtl8169_unmap_tx_skb(&tp->pci_dev->dev, tx_skb,
 				     tp->TxDescArray + entry);
 
 		if (status & LastFrag) {
+			dev->stats.tx_packets++;
 			dev_kfree_skb(tx_skb->skb);
 			tx_skb->skb = NULL;
 		}
-- 
1.7.1

[PATCH 6/6] r8169: print errors when dma mapping fail

[Stanislaw Gruszka]

Print errors because dma mapping failures can cause device to stop
working and will need user intervention to recover.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/r8169.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index 0ef49b4..b27b989 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -4022,8 +4022,10 @@ static struct sk_buff *rtl8169_alloc_rx_skb(struct rtl8169_private *tp,
 	skb_reserve(skb, align ? ((pad - 1) & (unsigned long)skb->data) : pad);
 
 	mapping = dma_map_single(d, skb->data, rx_buf_sz, DMA_FROM_DEVICE);
-	if (dma_mapping_error(d, mapping))
+	if (dma_mapping_error(d, mapping)) {
+		netif_err(tp, drv, tp->dev, "Failed to map RX DMA!\n");
 		goto err_free_skb_1;
+	}
 
 	rtl8169_map_to_asic(desc, mapping, rx_buf_sz);
 
@@ -4259,8 +4261,11 @@ static int rtl8169_xmit_frags(struct rtl8169_private *tp, struct sk_buff *skb,
 		len = frag->size;
 		addr = ((void *) page_address(frag->page)) + frag->page_offset;
 		mapping = dma_map_single(d, addr, len, DMA_TO_DEVICE);
-		if (unlikely(dma_mapping_error(d, mapping)))
+		if (unlikely(dma_mapping_error(d, mapping))) {
+			netif_err(tp, drv, tp->dev,
+				  "Failed to map TX fragments DMA!\n");
 			goto err_out;
+		}
 
 		/* anti gcc 2.95.3 bugware (sic) */
 		status = opts1 | len | (RingEnd * !((entry + 1) % NUM_TX_DESC));
@@ -4326,8 +4331,10 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
 
 	len = skb_headlen(skb);
 	mapping = dma_map_single(d, skb->data, len, DMA_TO_DEVICE);
-	if (unlikely(dma_mapping_error(d, mapping)))
+	if (unlikely(dma_mapping_error(d, mapping))) {
+		netif_err(tp, drv, dev, "Failed to map TX DMA!\n");
 		goto err_stop_0;
+	}
 
 	tp->tx_skb[entry].len = len;
 	txd->addr = cpu_to_le64(mapping);
-- 
1.7.1

Re: [PATCH 1/6] r8169: check dma mapping failures

[Francois Romieu]

Stanislaw Gruszka <sgruszka@redhat.com> :
> Check possible dma mapping errors and do clean up if it happens,
> when sending frames stop the tx queue.

Almost ok: NETDEV_TX_BUSY can not be used like that. Afaik the DMA
failure path in the driver really wants a NETDEV_TX_OK (and a device
stats update, though missing in tg3 ?).

Actually the former NETDEV_TX_BUSY condition mostly checks for a bug.

-- 
Ueimor

Re: [PATCH 1/6] r8169: check dma mapping failures

[Stanislaw Gruszka]

On Fri, Oct 15, 2010 at 03:41:58PM +0200, Francois Romieu wrote:
> Stanislaw Gruszka <sgruszka@redhat.com> :
> > Check possible dma mapping errors and do clean up if it happens,
> > when sending frames stop the tx queue.
> 
> Almost ok: NETDEV_TX_BUSY can not be used like that. Afaik the DMA
> failure path in the driver really wants a NETDEV_TX_OK (and a device
> stats update, though missing in tg3 ?).

I'm not sure if any driver handle that in the right way. Returning
"TX OK" when the transmission was not "OK", doesn't look correctly
to me.

Eric, David, what you think?

> Actually the former NETDEV_TX_BUSY condition mostly checks for a bug.

Driver handling code from net/core/*.c does not give me such impression.

Stanislaw

Re: [PATCH 1/6] r8169: check dma mapping failures

[Denis Kirjanov]

Right, we should pass TX_BUSY to upper layers only when the device hw
queue is full

On Fri, Oct 15, 2010 at 5:41 PM, Francois Romieu <romieu@fr.zoreil.com> wrote:
> Stanislaw Gruszka <sgruszka@redhat.com> :
>> Check possible dma mapping errors and do clean up if it happens,
>> when sending frames stop the tx queue.
>
> Almost ok: NETDEV_TX_BUSY can not be used like that. Afaik the DMA
> failure path in the driver really wants a NETDEV_TX_OK (and a device
> stats update, though missing in tg3 ?).
>
> Actually the former NETDEV_TX_BUSY condition mostly checks for a bug.
>
> --
> Ueimor
>



-- 
Regards,
Denis

Re: [PATCH 6/6] r8169: print errors when dma mapping fail

[Francois Romieu]

Stanislaw Gruszka <sgruszka@redhat.com> :
> Print errors because dma mapping failures can cause device to stop
> working and will need user intervention to recover.

I am hesitating (overengineered ? bloaty ? not the right place ?).

The Tx stats are kept up-to-date : Tx failure will go along a Tx drop
stat increase.

Regarding a mapping failure in the Rx path, either it will behave as
an allocation failure at open / resume time - and I have no idea how
the user will recover - or it will happen during a Rx ring refill.

...

Ok, something could have gone unnoticed. Let's try it.

-- 
Ueimor

Re: [PATCH 6/6] r8169: print errors when dma mapping fail

[Stanislaw Gruszka]

On Fri, Oct 15, 2010 at 04:52:01PM +0200, Francois Romieu wrote:
> Stanislaw Gruszka <sgruszka@redhat.com> :
> > Print errors because dma mapping failures can cause device to stop
> > working and will need user intervention to recover.
> 
> I am hesitating (overengineered ? bloaty ? not the right place ?).

As someone who seen lot's of bug reports like "my network device stops
working, nothing in dmesg", or like "my network device stops working,
there is NETDEV WATCHDOG: eth0 (r8169): transmit queue 0 timed out in
dmesg" (what is nothing but useful information), I do no think this is
overengineered or bloaty. I could agree for "not the right place", but
even if the error would be reported by upper layers, exact reason of
the problem will be unknown. Regarding lower layers, I don't think iommu
or other dma code print warning with calltrace in case of failure.

> The Tx stats are kept up-to-date : Tx failure will go along a Tx drop
> stat increase.

In current implementation, I stop tx queue on dma errors, if that
happens the queue can never be started again. I will probably change
that as you suggest not returning NETDEV_TX_BUSY, stopping the queue
is also wrong. But I would like to keep this error messages, perhaps
after adding net_ratelimit() check.
 
> Regarding a mapping failure in the Rx path, either it will behave as
> an allocation failure at open / resume time -

Still it's worth to know exact reason of failure.

> and I have no idea how
> the user will recover - or it will happen during a Rx ring refill.

ifconfig eth0 down/up or reloading module

Stanislaw

Re: [PATCH 1/6] r8169: check dma mapping failures

[Stanislaw Gruszka]

On Fri, Oct 15, 2010 at 06:23:55PM +0400, Denis Kirjanov wrote:
> Right, we should pass TX_BUSY to upper layers only when the device hw
> queue is full
> 
> On Fri, Oct 15, 2010 at 5:41 PM, Francois Romieu <romieu@fr.zoreil.com> wrote:
> > Stanislaw Gruszka <sgruszka@redhat.com> :
> >> Check possible dma mapping errors and do clean up if it happens,
> >> when sending frames stop the tx queue.
> >
> > Almost ok: NETDEV_TX_BUSY can not be used like that. Afaik the DMA
> > failure path in the driver really wants a NETDEV_TX_OK (and a device
> > stats update, though missing in tg3 ?).

Ok, I will change that and repost on top of currently applied Eric's 
patch.

Stanislaw