[RFC PATCH 1/2] r8169: check dma mapping failures

[RFC PATCH 1/2] r8169: check dma mapping failures

[Stanislaw Gruszka]

This is on top on my two r8169 patches just send.

Check possible dma mapping errors and do clean up if it happens.
Patch was not tested.

BTW: I see many drivers do not check these, so is really possible to
have this errors, and if yes, when ?
---
 drivers/net/r8169.c |   36 ++++++++++++++++++++++++++++--------
 1 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index bc669a4..b3b28b1 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -4018,20 +4018,24 @@ 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 err0;
 
 	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 err1;
 
 	rtl8169_map_to_asic(desc, mapping, rx_buf_sz);
-out:
+
 	return skb;
 
-err_out:
+err1:
+	dev_kfree_skb(skb);
+err0:
 	rtl8169_make_unusable_by_asic(desc);
-	goto out;
+	return NULL;
 }
 
 static void rtl8169_rx_clear(struct rtl8169_private *tp)
@@ -4115,11 +4119,11 @@ 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, u32 end)
 {
-	unsigned int i;
+	u32 i;
 
-	for (i = tp->dirty_tx; i < tp->dirty_tx + NUM_TX_DESC; i++) {
+	for (i = start; i < end; i++) {
 		unsigned int entry = i % NUM_TX_DESC;
 		struct ring_info *tx_skb = tp->tx_skb + entry;
 		unsigned int len = tx_skb->len;
@@ -4136,6 +4140,11 @@ static void rtl8169_tx_clear(struct rtl8169_private *tp)
 			tp->dev->stats.tx_dropped++;
 		}
 	}
+}
+
+static inline void rtl8169_tx_clear(struct rtl8169_private *tp)
+{
+	rtl8169_tx_clear_range(tp, tp->dirty_tx, tp->dirty_tx + NUM_TX_DESC);
 	tp->cur_tx = tp->dirty_tx = 0;
 }
 
@@ -4254,6 +4263,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 (dma_mapping_error(&tp->pci_dev->dev, mapping))
+			return -cur_frag;
 
 		/* anti gcc 2.95.3 bugware (sic) */
 		status = opts1 | len | (RingEnd * !((entry + 1) % NUM_TX_DESC));
@@ -4314,7 +4325,10 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
 	opts1 = DescOwn | rtl8169_tso_csum(skb, dev);
 
 	frags = rtl8169_xmit_frags(tp, skb, opts1);
-	if (frags) {
+	if (frags < 0) {
+		frags = -frags;
+		goto err_dma;
+	} else if (frags) {
 		len = skb_headlen(skb);
 		opts1 |= FirstFrag;
 	} else {
@@ -4325,6 +4339,8 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
 
 	mapping = dma_map_single(&tp->pci_dev->dev, skb->data, len,
 				 PCI_DMA_TODEVICE);
+	if (dma_mapping_error(&tp->pci_dev->dev, mapping))
+		goto err_dma;
 
 	tp->tx_skb[entry].len = len;
 	txd->addr = cpu_to_le64(mapping);
@@ -4355,6 +4371,10 @@ err_stop:
 	netif_stop_queue(dev);
 	dev->stats.tx_dropped++;
 	return NETDEV_TX_BUSY;
+
+err_dma:
+	rtl8169_tx_clear_range(tp, entry, entry + frags + 1);
+	return NETDEV_TX_OK;
 }
 
 static void rtl8169_pcierr_interrupt(struct net_device *dev)
-- 
1.7.1

[RFC PATCH 2/2] 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. Patch was not tested.
---
 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 b3b28b1..65d4219 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 err0;
 
 	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 err1;
 
-	rtl8169_map_to_asic(desc, mapping, rx_buf_sz);
+	rtl8169_map_to_asic(desc, mapping, tp->rx_buf_sz);
 
 	return skb;
 
@@ -4050,8 +4047,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;
 
@@ -4064,9 +4060,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;
 
@@ -4094,7 +4088,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);
@@ -4612,7 +4606,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

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

[Francois Romieu]

Stanislaw Gruszka <sgruszka@redhat.com> :
[...]
> diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
> index bc669a4..b3b28b1 100644
> --- a/drivers/net/r8169.c
> +++ b/drivers/net/r8169.c
> @@ -4018,20 +4018,24 @@ static struct sk_buff *rtl8169_alloc_rx_skb(struct pci_dev *pdev,

You can replace the pci_dev parameter with pdev->dev

>  
>  	skb = __netdev_alloc_skb(dev, rx_buf_sz + pad, gfp);
>  	if (!skb)
> -		goto err_out;
> +		goto err0;

err_out_0 (with a big please)

>  	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 err1;

err_free_skb_1

>  	rtl8169_map_to_asic(desc, mapping, rx_buf_sz);
> -out:
> +
>  	return skb;
>  
> -err_out:
> +err1:
> +	dev_kfree_skb(skb);
> +err0:
>  	rtl8169_make_unusable_by_asic(desc);
> -	goto out;
> +	return NULL;

I'd keep the 'goto out' and NULL the skb after the dev_kfree_skb but
it's up to you.

>  }
>  
>  static void rtl8169_rx_clear(struct rtl8169_private *tp)
> @@ -4115,11 +4119,11 @@ 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, u32 end)
>  {
> -	unsigned int i;
> +	u32 i;
>  
> -	for (i = tp->dirty_tx; i < tp->dirty_tx + NUM_TX_DESC; i++) {
> +	for (i = start; i < end; i++) {

Feel free to fix the (existing) wraparound bug.

>  		unsigned int entry = i % NUM_TX_DESC;
>  		struct ring_info *tx_skb = tp->tx_skb + entry;
>  		unsigned int len = tx_skb->len;
> @@ -4136,6 +4140,11 @@ static void rtl8169_tx_clear(struct rtl8169_private *tp)
>  			tp->dev->stats.tx_dropped++;
>  		}
>  	}
> +}
> +
> +static inline void rtl8169_tx_clear(struct rtl8169_private *tp)

Though used several times, it's hardly time critical : drop the inline.

> +{
> +	rtl8169_tx_clear_range(tp, tp->dirty_tx, tp->dirty_tx + NUM_TX_DESC);
>  	tp->cur_tx = tp->dirty_tx = 0;
>  }
>  
> @@ -4254,6 +4263,8 @@ static int rtl8169_xmit_frags(struct rtl8169_private *tp, struct sk_buff *skb,

Please add a local &tp->pci_dev->dev variable.

>  		addr = ((void *) page_address(frag->page)) + frag->page_offset;
>  		mapping = dma_map_single(&tp->pci_dev->dev, addr, len,
>  					 PCI_DMA_TODEVICE);
> +		if (dma_mapping_error(&tp->pci_dev->dev, mapping))
> +			return -cur_frag;

I ponder adding an 'unlikely', goto some cleaning statement and return a
signification deprieved "I failed" value. The caller does it anyway but
I am not sure if it really is its business.

>  
>  		/* anti gcc 2.95.3 bugware (sic) */
>  		status = opts1 | len | (RingEnd * !((entry + 1) % NUM_TX_DESC));
> @@ -4314,7 +4325,10 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
>  	opts1 = DescOwn | rtl8169_tso_csum(skb, dev);
>  
>  	frags = rtl8169_xmit_frags(tp, skb, opts1);
> -	if (frags) {
> +	if (frags < 0) {

'unsigned int frags' problem.

[...]
> @@ -4355,6 +4371,10 @@ err_stop:
>  	netif_stop_queue(dev);
>  	dev->stats.tx_dropped++;
>  	return NETDEV_TX_BUSY;
> +
> +err_dma:
> +	rtl8169_tx_clear_range(tp, entry, entry + frags + 1);
> +	return NETDEV_TX_OK;

Third return statement, second NETDEV_TX_OK, no tx_dropped increase. May be
a bit heavy handed.

Otherwise it's cool.

-- 
Ueimor