| Message ID | 1374848918-17183-1-git-send-email-nhorman@tuxdriver.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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.
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 > -- 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
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.
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 > -- 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
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.
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 > -- 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
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;
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(+)