[net PATCH] 8139cp: Add dma_mapping_error checking

[net PATCH] 8139cp: Add dma_mapping_error checking

[Neil Horman]

Self explanitory dma_mapping_error addition to the 8139 driver, based on this:
https://bugzilla.redhat.com/show_bug.cgi?id=947250

It showed several backtraces arising for dma_map_* usage without checking the
return code on the mapping.  Add the check and abort the rx/tx operation if its
failed.  Untested as I have no hardware and the reporter has wandered off, but
seems pretty straightforward.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: "David S. Miller" <davem@davemloft.net>
---
 drivers/net/ethernet/realtek/8139cp.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index 0352345..b7ec733 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -533,6 +533,11 @@ rx_status_loop:
 
 		mapping = dma_map_single(&cp->pdev->dev, new_skb->data, buflen,
 					 PCI_DMA_FROMDEVICE);
+		if (dma_mapping_error(&cp->pdev->dev, mapping)) {
+			kfree_skb(skb);
+			break;
+		}
+
 		cp->rx_skb[rx_tail] = new_skb;
 
 		cp_rx_skb(cp, skb, desc);
@@ -749,6 +754,11 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 
 		len = skb->len;
 		mapping = dma_map_single(&cp->pdev->dev, skb->data, len, PCI_DMA_TODEVICE);
+		if (dma_mapping_error(&cp->pdev->dev, mapping)) {
+			kfree_skb(skb);
+			goto out_unlock;
+		}
+
 		txd->opts2 = opts2;
 		txd->addr = cpu_to_le64(mapping);
 		wmb();
@@ -786,6 +796,11 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 		first_len = skb_headlen(skb);
 		first_mapping = dma_map_single(&cp->pdev->dev, skb->data,
 					       first_len, PCI_DMA_TODEVICE);
+		if (dma_mapping_error(&cp->pdev->dev, first_mapping)) {
+			kfree_skb(skb);
+			goto out_unlock;
+		}
+
 		cp->tx_skb[entry] = skb;
 		entry = NEXT_TX(entry);
 
@@ -799,6 +814,20 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 			mapping = dma_map_single(&cp->pdev->dev,
 						 skb_frag_address(this_frag),
 						 len, PCI_DMA_TODEVICE);
+			if (dma_mapping_error(&cp->pdev->dev, mapping)) {
+				/* Unwind the mappnigs we have */
+				for (frag = 0; frag+first_entry < entry; frag++) {
+					cp->tx_skb[frag+first_entry] = NULL;
+					txd = &cp->tx_ring[frag+first_entry];
+					this_frag = &skb_shinfo(skb)->frags[frag];
+					dma_unmap_single(&cp->pdev->dev, le64_to_cpu(txd->addr),
+							 skb_frag_size(this_frag), PCI_DMA_TODEVICE); 
+				}
+				
+				kfree_skb(skb);
+				goto out_unlock;
+			}
+
 			eor = (entry == (CP_TX_RING_SIZE - 1)) ? RingEnd : 0;
 
 			ctrl = eor | len | DescOwn;
@@ -859,6 +888,7 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 	if (TX_BUFFS_AVAIL(cp) <= (MAX_SKB_FRAGS + 1))
 		netif_stop_queue(dev);
 
+out_unlock:
 	spin_unlock_irqrestore(&cp->lock, intr_flags);
 
 	cpw8(TxPoll, NormalTxPoll);
@@ -1054,6 +1084,10 @@ static int cp_refill_rx(struct cp_private *cp)
 
 		mapping = dma_map_single(&cp->pdev->dev, skb->data,
 					 cp->rx_buf_sz, PCI_DMA_FROMDEVICE);
+		if (dma_mapping_error(&cp->pdev->dev, mapping)) {
+			kfree_skb(skb);
+			goto err_out;
+		}
 		cp->rx_skb[i] = skb;
 
 		cp->rx_ring[i].opts2 = 0;
-- 
1.8.1.4

Re: [net PATCH] 8139cp: Add dma_mapping_error checking

[Francois Romieu]

Neil Horman <nhorman@tuxdriver.com> :
[...]
> failed.  Untested as I have no hardware and the reporter has wandered off, but
> seems pretty straightforward.

It does not update dev->stats.tx_dropped in the xmit path.

-- 
Ueimor

Re: [net PATCH] 8139cp: Add dma_mapping_error checking

[Neil Horman]

On Mon, Jul 22, 2013 at 11:48:39PM +0200, Francois Romieu wrote:
> Neil Horman <nhorman@tuxdriver.com> :
> [...]
> > failed.  Untested as I have no hardware and the reporter has wandered off, but
> > seems pretty straightforward.
> 
> It does not update dev->stats.tx_dropped in the xmit path.
> 
Thanks, missed that, I'll respin
Neil

> -- 
> Ueimor
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[net PATCH v2] 8139cp: Add dma_mapping_error checking

[Neil Horman]

Self explanitory dma_mapping_error addition to the 8139 driver, based on this:
https://bugzilla.redhat.com/show_bug.cgi?id=947250

It showed several backtraces arising for dma_map_* usage without checking the
return code on the mapping.  Add the check and abort the rx/tx operation if its
failed.  Untested as I have no hardware and the reporter has wandered off, but
seems pretty straightforward.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: "David S. Miller" <davem@davemloft.net>
CC: Francois Romieu <romieu@fr.zoreil.com>

---
Change notes:

v2) Added stats update for tx_dropped as per Francois Romieu
---
 drivers/net/ethernet/realtek/8139cp.c | 37 +++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index 0352345..ba0570a 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -533,6 +533,11 @@ rx_status_loop:
 
 		mapping = dma_map_single(&cp->pdev->dev, new_skb->data, buflen,
 					 PCI_DMA_FROMDEVICE);
+		if (dma_mapping_error(&cp->pdev->dev, mapping)) {
+			kfree_skb(skb);
+			break;
+		}
+
 		cp->rx_skb[rx_tail] = new_skb;
 
 		cp_rx_skb(cp, skb, desc);
@@ -749,6 +754,12 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 
 		len = skb->len;
 		mapping = dma_map_single(&cp->pdev->dev, skb->data, len, PCI_DMA_TODEVICE);
+		if (dma_mapping_error(&cp->pdev->dev, mapping)) {
+			kfree_skb(skb);
+			cp->dev->stats.tx_dropped++;
+			goto out_unlock;
+		}
+
 		txd->opts2 = opts2;
 		txd->addr = cpu_to_le64(mapping);
 		wmb();
@@ -786,6 +797,12 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 		first_len = skb_headlen(skb);
 		first_mapping = dma_map_single(&cp->pdev->dev, skb->data,
 					       first_len, PCI_DMA_TODEVICE);
+		if (dma_mapping_error(&cp->pdev->dev, first_mapping)) {
+			kfree_skb(skb);
+			cp->dev->stats.tx_dropped++;
+			goto out_unlock;
+		}
+
 		cp->tx_skb[entry] = skb;
 		entry = NEXT_TX(entry);
 
@@ -799,6 +816,21 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 			mapping = dma_map_single(&cp->pdev->dev,
 						 skb_frag_address(this_frag),
 						 len, PCI_DMA_TODEVICE);
+			if (dma_mapping_error(&cp->pdev->dev, mapping)) {
+				/* Unwind the mappnigs we have */
+				for (frag = 0; frag+first_entry < entry; frag++) {
+					cp->tx_skb[frag+first_entry] = NULL;
+					txd = &cp->tx_ring[frag+first_entry];
+					this_frag = &skb_shinfo(skb)->frags[frag];
+					dma_unmap_single(&cp->pdev->dev, le64_to_cpu(txd->addr),
+							 skb_frag_size(this_frag), PCI_DMA_TODEVICE);
+				}
+
+				cp->dev->stats.tx_dropped++;
+				kfree_skb(skb);
+				goto out_unlock;
+			}
+
 			eor = (entry == (CP_TX_RING_SIZE - 1)) ? RingEnd : 0;
 
 			ctrl = eor | len | DescOwn;
@@ -859,6 +891,7 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 	if (TX_BUFFS_AVAIL(cp) <= (MAX_SKB_FRAGS + 1))
 		netif_stop_queue(dev);
 
+out_unlock:
 	spin_unlock_irqrestore(&cp->lock, intr_flags);
 
 	cpw8(TxPoll, NormalTxPoll);
@@ -1054,6 +1087,10 @@ static int cp_refill_rx(struct cp_private *cp)
 
 		mapping = dma_map_single(&cp->pdev->dev, skb->data,
 					 cp->rx_buf_sz, PCI_DMA_FROMDEVICE);
+		if (dma_mapping_error(&cp->pdev->dev, mapping)) {
+			kfree_skb(skb);
+			goto err_out;
+		}
 		cp->rx_skb[i] = skb;
 
 		cp->rx_ring[i].opts2 = 0;
-- 
1.8.1.4

Re: [net PATCH v2] 8139cp: Add dma_mapping_error checking

[Francois Romieu]

Neil Horman <nhorman@tuxdriver.com> :
[...]
> diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
> index 0352345..ba0570a 100644
> --- a/drivers/net/ethernet/realtek/8139cp.c
> +++ b/drivers/net/ethernet/realtek/8139cp.c
> @@ -533,6 +533,11 @@ rx_status_loop:
>  
>  		mapping = dma_map_single(&cp->pdev->dev, new_skb->data, buflen,
>  					 PCI_DMA_FROMDEVICE);
> +		if (dma_mapping_error(&cp->pdev->dev, mapping)) {
> +			kfree_skb(skb);

			dev->stats.rx_dropped++;

Sorry, I did not notice that skb contained newly received data :o/

> +			break;
> +		}
> +
[...]
> @@ -749,6 +754,12 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
>  
>  		len = skb->len;
>  		mapping = dma_map_single(&cp->pdev->dev, skb->data, len, PCI_DMA_TODEVICE);
> +		if (dma_mapping_error(&cp->pdev->dev, mapping)) {
> +			kfree_skb(skb);
> +			cp->dev->stats.tx_dropped++;
> +			goto out_unlock;

These lines appear several times. You may factor them out with a
goto out_drop or such.

> @@ -799,6 +816,21 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
>  			mapping = dma_map_single(&cp->pdev->dev,
>  						 skb_frag_address(this_frag),
>  						 len, PCI_DMA_TODEVICE);
> +			if (dma_mapping_error(&cp->pdev->dev, mapping)) {
> +				/* Unwind the mappnigs we have */
                                                  ^^

> +				for (frag = 0; frag+first_entry < entry; frag++) {
> +					cp->tx_skb[frag+first_entry] = NULL;
> +					txd = &cp->tx_ring[frag+first_entry];
> +					this_frag = &skb_shinfo(skb)->frags[frag];
> +					dma_unmap_single(&cp->pdev->dev, le64_to_cpu(txd->addr),
> +							 skb_frag_size(this_frag), PCI_DMA_TODEVICE);
> +				}

Imho, even with local &cp->pdev->dev and removal of whitespaces, it
calls for a separate xmit_frag helper or an out of line unwinder.

-- 
Ueimor

Re: [net PATCH v2] 8139cp: Add dma_mapping_error checking

[Neil Horman]

On Fri, Jul 26, 2013 at 10:06:30PM +0200, Francois Romieu wrote:
> Neil Horman <nhorman@tuxdriver.com> :
> [...]
> > diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
> > index 0352345..ba0570a 100644
> > --- a/drivers/net/ethernet/realtek/8139cp.c
> > +++ b/drivers/net/ethernet/realtek/8139cp.c
> > @@ -533,6 +533,11 @@ rx_status_loop:
> >  
> >  		mapping = dma_map_single(&cp->pdev->dev, new_skb->data, buflen,
> >  					 PCI_DMA_FROMDEVICE);
> > +		if (dma_mapping_error(&cp->pdev->dev, mapping)) {
> > +			kfree_skb(skb);
> 
> 			dev->stats.rx_dropped++;
> 
> Sorry, I did not notice that skb contained newly received data :o/
> 
Yeah, this isn't needed.  The skb being mapped in is newly allocated, and
contains no data.  We haven't dropped anything here, so theres no need for the
stats bump.

> > +			break;
> > +		}
> > +
> [...]
> > @@ -749,6 +754,12 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
> >  
> >  		len = skb->len;
> >  		mapping = dma_map_single(&cp->pdev->dev, skb->data, len, PCI_DMA_TODEVICE);
> > +		if (dma_mapping_error(&cp->pdev->dev, mapping)) {
> > +			kfree_skb(skb);
> > +			cp->dev->stats.tx_dropped++;
> > +			goto out_unlock;
> 
> These lines appear several times. You may factor them out with a
> goto out_drop or such.
> 
I may, I'm not sure its worth it.  If the item below results in a respin, then
I'l work this in as well.

> > @@ -799,6 +816,21 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
> >  			mapping = dma_map_single(&cp->pdev->dev,
> >  						 skb_frag_address(this_frag),
> >  						 len, PCI_DMA_TODEVICE);
> > +			if (dma_mapping_error(&cp->pdev->dev, mapping)) {
> > +				/* Unwind the mappnigs we have */
>                                                   ^^
> 
> > +				for (frag = 0; frag+first_entry < entry; frag++) {
> > +					cp->tx_skb[frag+first_entry] = NULL;
> > +					txd = &cp->tx_ring[frag+first_entry];
> > +					this_frag = &skb_shinfo(skb)->frags[frag];
> > +					dma_unmap_single(&cp->pdev->dev, le64_to_cpu(txd->addr),
> > +							 skb_frag_size(this_frag), PCI_DMA_TODEVICE);
> > +				}
> 
> Imho, even with local &cp->pdev->dev and removal of whitespaces, it
> calls for a separate xmit_frag helper or an out of line unwinder.
> 
I'm not 100% sure its actually going to be more readable with an extra function
there.  We still have to pass in all the start points and descriptor counts, do
the machinations of iterating over two separately biased lists, and undo what
was done in this function.  I honestly kind of like it better in line, as you
can see where the values originated from, and the mapping/unmapping looks
balanced.

Thoughts?
Neil

> -- 
> Ueimor
> 

Re: [net PATCH v2] 8139cp: Add dma_mapping_error checking

[Francois Romieu]

Neil Horman <nhorman@tuxdriver.com> :
> On Fri, Jul 26, 2013 at 10:06:30PM +0200, Francois Romieu wrote:
> > Neil Horman <nhorman@tuxdriver.com> :
[...]
> > > @@ -533,6 +533,11 @@ rx_status_loop:
> > >  
> > >  		mapping = dma_map_single(&cp->pdev->dev, new_skb->data, buflen,
> > >  					 PCI_DMA_FROMDEVICE);
> > > +		if (dma_mapping_error(&cp->pdev->dev, mapping)) {
> > > +			kfree_skb(skb);
> > 
> > 			dev->stats.rx_dropped++;
> > 
> > Sorry, I did not notice that skb contained newly received data :o/
> > 
> Yeah, this isn't needed.  The skb being mapped in is newly allocated, and
> contains no data.  We haven't dropped anything here, so theres no need for the
> stats bump.

Huh ?

[...error path...]
		kfree(skb);
[...normal path...]
	cp_rx_skb(cp, skb, desc);

Afaiks it's the same skb. We drop data and a stats bump is needed.

[...]
> Thoughts?

Nevermind. cp_start_xmit is a piece of it and it isn't your work to
turn it into something sensible.

-- 
Ueimor

Re: [net PATCH v2] 8139cp: Add dma_mapping_error checking

[Neil Horman]

On Fri, Jul 26, 2013 at 11:24:03PM +0200, Francois Romieu wrote:
> Neil Horman <nhorman@tuxdriver.com> :
> > On Fri, Jul 26, 2013 at 10:06:30PM +0200, Francois Romieu wrote:
> > > Neil Horman <nhorman@tuxdriver.com> :
> [...]
> > > > @@ -533,6 +533,11 @@ rx_status_loop:
> > > >  
> > > >  		mapping = dma_map_single(&cp->pdev->dev, new_skb->data, buflen,
> > > >  					 PCI_DMA_FROMDEVICE);
> > > > +		if (dma_mapping_error(&cp->pdev->dev, mapping)) {
> > > > +			kfree_skb(skb);
> > > 
> > > 			dev->stats.rx_dropped++;
> > > 
> > > Sorry, I did not notice that skb contained newly received data :o/
> > > 
> > Yeah, this isn't needed.  The skb being mapped in is newly allocated, and
> > contains no data.  We haven't dropped anything here, so theres no need for the
> > stats bump.
> 
> Huh ?
> 
> [...error path...]
> 		kfree(skb);
> [...normal path...]
> 	cp_rx_skb(cp, skb, desc);
> 
> Afaiks it's the same skb. We drop data and a stats bump is needed.
> 
> [...]
> > Thoughts?
> 
> Nevermind. cp_start_xmit is a piece of it and it isn't your work to
> turn it into something sensible.
> 
So you agree with the patch as it stands?
Neil

> -- 
> Ueimor
> 

Re: [net PATCH v2] 8139cp: Add dma_mapping_error checking

[Francois Romieu]

Neil Horman <nhorman@tuxdriver.com> :
[...]
> So you agree with the patch as it stands?

This part, yes.

However I still see a need for a rx_dropped increase.

-- 
Ueimor

Re: [net PATCH v2] 8139cp: Add dma_mapping_error checking

[Neil Horman]

On Sun, Jul 28, 2013 at 11:34:19PM +0200, Francois Romieu wrote:
> Neil Horman <nhorman@tuxdriver.com> :
> [...]
> > So you agree with the patch as it stands?
> 
> This part, yes.
> 
> However I still see a need for a rx_dropped increase.
> 
Very well, I'll take another look tomorrow morning
Neil

> -- 
> Ueimor
> 

[net PATCH v3] 8139cp: Add dma_mapping_error checking

[Neil Horman]

Self explanitory dma_mapping_error addition to the 8139 driver, based on this:
https://bugzilla.redhat.com/show_bug.cgi?id=947250

It showed several backtraces arising for dma_map_* usage without checking the
return code on the mapping.  Add the check and abort the rx/tx operation if its
failed.  Untested as I have no hardware and the reporter has wandered off, but
seems pretty straightforward.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: "David S. Miller" <davem@davemloft.net>
CC: Francois Romieu <romieu@fr.zoreil.com>

---
Change notes:

v2) Added stats update for tx_dropped as per Francois Romieu

v3)
* Updated rx patch to handle dma errors properly.  Francois had pointed out that
I needed to increment the rx_dropped stat there, but the problem was a bit
deeper.  I actually had to move the dma_mapping and error check higher in the
path, so that, in the event of a failure, we could drop the received skb, and
reuse its existing mapping in the rx ring.  this is in line with what happens if
the skb allocation fails

* Updated the tx path to have a common goto for mapping failures, so we only
have one location to drop the skb

* Moved the unwind code into a separate helper function, as per Francois
suggestion.  I though it would be alot of parameter passing and not very
adventageous, but on second look I think its more readable.
---
 drivers/net/ethernet/realtek/8139cp.c | 48 ++++++++++++++++++++++++++++++++---
 1 file changed, 45 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index 0352345..3042030 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -478,7 +478,7 @@ rx_status_loop:
 
 	while (1) {
 		u32 status, len;
-		dma_addr_t mapping;
+		dma_addr_t mapping, new_mapping;
 		struct sk_buff *skb, *new_skb;
 		struct cp_desc *desc;
 		const unsigned buflen = cp->rx_buf_sz;
@@ -520,6 +520,13 @@ rx_status_loop:
 			goto rx_next;
 		}
 
+		new_mapping = dma_map_single(&cp->pdev->dev, new_skb->data, buflen,
+					 PCI_DMA_FROMDEVICE);
+		if (dma_mapping_error(&cp->pdev->dev, new_mapping)) {
+			dev->stats.rx_dropped++;
+			goto rx_next;
+		}
+
 		dma_unmap_single(&cp->pdev->dev, mapping,
 				 buflen, PCI_DMA_FROMDEVICE);
 
@@ -531,12 +538,11 @@ rx_status_loop:
 
 		skb_put(skb, len);
 
-		mapping = dma_map_single(&cp->pdev->dev, new_skb->data, buflen,
-					 PCI_DMA_FROMDEVICE);
 		cp->rx_skb[rx_tail] = new_skb;
 
 		cp_rx_skb(cp, skb, desc);
 		rx++;
+		mapping = new_mapping;
 
 rx_next:
 		cp->rx_ring[rx_tail].opts2 = 0;
@@ -716,6 +722,22 @@ static inline u32 cp_tx_vlan_tag(struct sk_buff *skb)
 		TxVlanTag | swab16(vlan_tx_tag_get(skb)) : 0x00;
 }
 
+static void unwind_rx_frag_mapping(struct cp_private *cp, struct sk_buff *skb,
+				   int first, int entry_last)
+{
+	int frag, index;
+	struct cp_desc *txd;
+	skb_frag_t *this_frag;
+	for (frag = 0; frag+first < entry_last; frag++) {
+		index = first+frag;
+		cp->tx_skb[index] = NULL;
+		txd = &cp->tx_ring[index];
+		this_frag = &skb_shinfo(skb)->frags[frag];
+		dma_unmap_single(&cp->pdev->dev, le64_to_cpu(txd->addr),
+				 skb_frag_size(this_frag), PCI_DMA_TODEVICE);
+	}
+}
+
 static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 					struct net_device *dev)
 {
@@ -749,6 +771,9 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 
 		len = skb->len;
 		mapping = dma_map_single(&cp->pdev->dev, skb->data, len, PCI_DMA_TODEVICE);
+		if (dma_mapping_error(&cp->pdev->dev, mapping))
+			goto out_dma_error;
+
 		txd->opts2 = opts2;
 		txd->addr = cpu_to_le64(mapping);
 		wmb();
@@ -786,6 +811,9 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 		first_len = skb_headlen(skb);
 		first_mapping = dma_map_single(&cp->pdev->dev, skb->data,
 					       first_len, PCI_DMA_TODEVICE);
+		if (dma_mapping_error(&cp->pdev->dev, first_mapping))
+			goto out_dma_error;
+
 		cp->tx_skb[entry] = skb;
 		entry = NEXT_TX(entry);
 
@@ -799,6 +827,11 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 			mapping = dma_map_single(&cp->pdev->dev,
 						 skb_frag_address(this_frag),
 						 len, PCI_DMA_TODEVICE);
+			if (dma_mapping_error(&cp->pdev->dev, mapping)) {
+				unwind_rx_frag_mapping(cp, skb, first_entry, entry);
+				goto out_dma_error;
+			}
+
 			eor = (entry == (CP_TX_RING_SIZE - 1)) ? RingEnd : 0;
 
 			ctrl = eor | len | DescOwn;
@@ -859,11 +892,16 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 	if (TX_BUFFS_AVAIL(cp) <= (MAX_SKB_FRAGS + 1))
 		netif_stop_queue(dev);
 
+out_unlock:
 	spin_unlock_irqrestore(&cp->lock, intr_flags);
 
 	cpw8(TxPoll, NormalTxPoll);
 
 	return NETDEV_TX_OK;
+out_dma_error:
+	kfree_skb(skb);
+	cp->dev->stats.tx_dropped++;
+	goto out_unlock;
 }
 
 /* Set or clear the multicast filter for this adaptor.
@@ -1054,6 +1092,10 @@ static int cp_refill_rx(struct cp_private *cp)
 
 		mapping = dma_map_single(&cp->pdev->dev, skb->data,
 					 cp->rx_buf_sz, PCI_DMA_FROMDEVICE);
+		if (dma_mapping_error(&cp->pdev->dev, mapping)) {
+			kfree_skb(skb);
+			goto err_out;
+		}
 		cp->rx_skb[i] = skb;
 
 		cp->rx_ring[i].opts2 = 0;
-- 
1.8.1.4

Re: [net PATCH v3] 8139cp: Add dma_mapping_error checking

[David Miller]

From: Neil Horman <nhorman@tuxdriver.com>
Date: Mon, 29 Jul 2013 13:05:35 -0400

> @@ -716,6 +722,22 @@ static inline u32 cp_tx_vlan_tag(struct sk_buff *skb)
>  		TxVlanTag | swab16(vlan_tx_tag_get(skb)) : 0x00;
>  }
>  
> +static void unwind_rx_frag_mapping(struct cp_private *cp, struct sk_buff *skb,
> +				   int first, int entry_last)
> +{
> +	int frag, index;
> +	struct cp_desc *txd;
> +	skb_frag_t *this_frag;
> +	for (frag = 0; frag+first < entry_last; frag++) {
> +		index = first+frag;
> +		cp->tx_skb[index] = NULL;
> +		txd = &cp->tx_ring[index];
> +		this_frag = &skb_shinfo(skb)->frags[frag];
> +		dma_unmap_single(&cp->pdev->dev, le64_to_cpu(txd->addr),
> +				 skb_frag_size(this_frag), PCI_DMA_TODEVICE);
> +	}
> +}

This is using PCI_DMA_TODEVICE, so seems to be dealing with TX mappings.
Yet the function is named "unwind_rx_frame_mapping()".

> @@ -799,6 +827,11 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
>  			mapping = dma_map_single(&cp->pdev->dev,
>  						 skb_frag_address(this_frag),
>  						 len, PCI_DMA_TODEVICE);
> +			if (dma_mapping_error(&cp->pdev->dev, mapping)) {
> +				unwind_rx_frag_mapping(cp, skb, first_entry, entry);
> +				goto out_dma_error;
> +			}

And you're invoking it from the cp_start_xmit() function, which further
confirms the incorrect function name :-)

Please fix this and resubmit, thanks.

[net PATCH v4] 8139cp: Add dma_mapping_error checking

[Neil Horman]

Self explanitory dma_mapping_error addition to the 8139 driver, based on this:
https://bugzilla.redhat.com/show_bug.cgi?id=947250

It showed several backtraces arising for dma_map_* usage without checking the
return code on the mapping.  Add the check and abort the rx/tx operation if its
failed.  Untested as I have no hardware and the reporter has wandered off, but
seems pretty straightforward.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: "David S. Miller" <davem@davemloft.net>
CC: Francois Romieu <romieu@fr.zoreil.com>

---
Change notes:

v2) Added stats update for tx_dropped as per Francois Romieu

v3)
* Updated rx patch to handle dma errors properly.  Francois had pointed out that
I needed to increment the rx_dropped stat there, but the problem was a bit
deeper.  I actually had to move the dma_mapping and error check higher in the
path, so that, in the event of a failure, we could drop the received skb, and
reuse its existing mapping in the rx ring.  this is in line with what happens if
the skb allocation fails

* Updated the tx path to have a common goto for mapping failures, so we only
have one location to drop the skb

* Moved the unwind code into a separate helper function, as per Francois
suggestion.  I though it would be alot of parameter passing and not very
adventageous, but on second look I think its more readable.

v4)
* Fix rx/tx naming error.
---
 drivers/net/ethernet/realtek/8139cp.c | 48 ++++++++++++++++++++++++++++++++---
 1 file changed, 45 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index 0352345..887aebe 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -478,7 +478,7 @@ rx_status_loop:
 
 	while (1) {
 		u32 status, len;
-		dma_addr_t mapping;
+		dma_addr_t mapping, new_mapping;
 		struct sk_buff *skb, *new_skb;
 		struct cp_desc *desc;
 		const unsigned buflen = cp->rx_buf_sz;
@@ -520,6 +520,13 @@ rx_status_loop:
 			goto rx_next;
 		}
 
+		new_mapping = dma_map_single(&cp->pdev->dev, new_skb->data, buflen,
+					 PCI_DMA_FROMDEVICE);
+		if (dma_mapping_error(&cp->pdev->dev, new_mapping)) {
+			dev->stats.rx_dropped++;
+			goto rx_next;
+		}
+
 		dma_unmap_single(&cp->pdev->dev, mapping,
 				 buflen, PCI_DMA_FROMDEVICE);
 
@@ -531,12 +538,11 @@ rx_status_loop:
 
 		skb_put(skb, len);
 
-		mapping = dma_map_single(&cp->pdev->dev, new_skb->data, buflen,
-					 PCI_DMA_FROMDEVICE);
 		cp->rx_skb[rx_tail] = new_skb;
 
 		cp_rx_skb(cp, skb, desc);
 		rx++;
+		mapping = new_mapping;
 
 rx_next:
 		cp->rx_ring[rx_tail].opts2 = 0;
@@ -716,6 +722,22 @@ static inline u32 cp_tx_vlan_tag(struct sk_buff *skb)
 		TxVlanTag | swab16(vlan_tx_tag_get(skb)) : 0x00;
 }
 
+static void unwind_tx_frag_mapping(struct cp_private *cp, struct sk_buff *skb,
+				   int first, int entry_last)
+{
+	int frag, index;
+	struct cp_desc *txd;
+	skb_frag_t *this_frag;
+	for (frag = 0; frag+first < entry_last; frag++) {
+		index = first+frag;
+		cp->tx_skb[index] = NULL;
+		txd = &cp->tx_ring[index];
+		this_frag = &skb_shinfo(skb)->frags[frag];
+		dma_unmap_single(&cp->pdev->dev, le64_to_cpu(txd->addr),
+				 skb_frag_size(this_frag), PCI_DMA_TODEVICE);
+	}
+}
+
 static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 					struct net_device *dev)
 {
@@ -749,6 +771,9 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 
 		len = skb->len;
 		mapping = dma_map_single(&cp->pdev->dev, skb->data, len, PCI_DMA_TODEVICE);
+		if (dma_mapping_error(&cp->pdev->dev, mapping))
+			goto out_dma_error;
+
 		txd->opts2 = opts2;
 		txd->addr = cpu_to_le64(mapping);
 		wmb();
@@ -786,6 +811,9 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 		first_len = skb_headlen(skb);
 		first_mapping = dma_map_single(&cp->pdev->dev, skb->data,
 					       first_len, PCI_DMA_TODEVICE);
+		if (dma_mapping_error(&cp->pdev->dev, first_mapping))
+			goto out_dma_error;
+
 		cp->tx_skb[entry] = skb;
 		entry = NEXT_TX(entry);
 
@@ -799,6 +827,11 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 			mapping = dma_map_single(&cp->pdev->dev,
 						 skb_frag_address(this_frag),
 						 len, PCI_DMA_TODEVICE);
+			if (dma_mapping_error(&cp->pdev->dev, mapping)) {
+				unwind_tx_frag_mapping(cp, skb, first_entry, entry);
+				goto out_dma_error;
+			}
+
 			eor = (entry == (CP_TX_RING_SIZE - 1)) ? RingEnd : 0;
 
 			ctrl = eor | len | DescOwn;
@@ -859,11 +892,16 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 	if (TX_BUFFS_AVAIL(cp) <= (MAX_SKB_FRAGS + 1))
 		netif_stop_queue(dev);
 
+out_unlock:
 	spin_unlock_irqrestore(&cp->lock, intr_flags);
 
 	cpw8(TxPoll, NormalTxPoll);
 
 	return NETDEV_TX_OK;
+out_dma_error:
+	kfree_skb(skb);
+	cp->dev->stats.tx_dropped++;
+	goto out_unlock;
 }
 
 /* Set or clear the multicast filter for this adaptor.
@@ -1054,6 +1092,10 @@ static int cp_refill_rx(struct cp_private *cp)
 
 		mapping = dma_map_single(&cp->pdev->dev, skb->data,
 					 cp->rx_buf_sz, PCI_DMA_FROMDEVICE);
+		if (dma_mapping_error(&cp->pdev->dev, mapping)) {
+			kfree_skb(skb);
+			goto err_out;
+		}
 		cp->rx_skb[i] = skb;
 
 		cp->rx_ring[i].opts2 = 0;
-- 
1.8.1.4

Re: [net PATCH v4] 8139cp: Add dma_mapping_error checking

[David Miller]

From: Neil Horman <nhorman@tuxdriver.com>
Date: Wed, 31 Jul 2013 09:03:56 -0400

> Self explanitory dma_mapping_error addition to the 8139 driver, based on this:
> https://bugzilla.redhat.com/show_bug.cgi?id=947250
> 
> It showed several backtraces arising for dma_map_* usage without checking the
> return code on the mapping.  Add the check and abort the rx/tx operation if its
> failed.  Untested as I have no hardware and the reporter has wandered off, but
> seems pretty straightforward.
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>

Applied and queued up for -stable, thanks Neil.