e1000: Fix DMA mapping error handling on TX

Check for error return from pci_map_single/pci_map_page and clean up.

With this and the previous patch the driver was able to handle a significant percentage of errors (I set the fault injection rate to 10% and could still download large files at a reasonable speed).

Signed-off-by Anton Blanchard <[email protected]>

---

I wasn't able to stress the jumbo frame path, so that part could do with some more eyes.

Index: linux.trees.git/drivers/net/e1000/e1000_main.c
===================================================================

drivers/net/e1000/e1000_main.c      2010-01-21 11:15:54.000000000 +1100 renamed from drivers/net/e1000/e1000_main.c 2010-01-21 11:13:10.000000000 +1100 +11 -1

@@ -4014,11 +4014,21 @@ check_page:
                        }
                }
 
-               if (!buffer_info->dma)
+               if (!buffer_info->dma) {
                        buffer_info->dma = pci_map_page(pdev,
                                                        buffer_info->page, 0,
                                                        buffer_info->length,
                                                        PCI_DMA_FROMDEVICE);
+                       if (pci_dma_mapping_error(pdev, buffer_info->dma)) {
+                               put_page(buffer_info->page);
+                               dev_kfree_skb(skb);
+                               buffer_info->page = NULL;
+                               buffer_info->skb = NULL;
+                               buffer_info->dma = 0;
+                               adapter->alloc_rx_buff_failed++;
+                               break; /* while !buffer_info->skb */
+                       }
+               }
 
                rx_desc = E1000_RX_DESC(*rx_ring, i);
                rx_desc->buffer_addr = cpu_to_le64(buffer_info->dma);

@@ -4109,6 +4119,13 @@ map_skb:
                                                  skb->data,
                                                  buffer_info->length,
                                                  PCI_DMA_FROMDEVICE);

There were a few issues in the DMA mapping error handling in e1000_tx_map which I found via fault injection.

If we fail to map the first descriptor count will end up as -1 but the check of (count >= 0) will still be true since count is unsigned. Instead of changing count to be signed, just simplify the logic.

Secondly, when we wrap the tx ring we rely on i to go negative, but it was unsigned.

Signed-off-by Anton Blanchard <[email protected]>

---

Index: linux.trees.git/drivers/net/e1000/e1000_main.c
===================================================================

drivers/net/e1000/e1000_main.c      2010-01-21 11:12:52.000000000 +1100 renamed from drivers/net/e1000/e1000_main.c 2010-01-21 11:10:00.000000000 +1100 +2 -1

@@ -2693,8 +2693,9 @@ static int e1000_tx_map(struct e1000_ada
        struct pci_dev *pdev = adapter->pdev;
        struct e1000_buffer *buffer_info;
        unsigned int len = skb_headlen(skb);
-       unsigned int offset = 0, size, count = 0, i;
+       unsigned int offset = 0, size, count = 0;
        unsigned int f;
+       int i;
 
        i = tx_ring->next_to_use;
 

@@ -2802,10 +2803,8 @@ static int e1000_tx_map(struct e1000_ada

 dma_error:
       dev_err(&pdev->dev, "TX DMA map failed\n"); buffer_info->dma = 0;
-       count--;

-       while (count >= 0) {
-               count--;
+       while (count--) {
               i--;
               if (i < 0)
                       i += tx_ring->count;

This patch does not apply to the current e1000 driver in net-2.6, much of this patch has already been corrected (applied) by Roel Kluin recent patch.

Hi,

Quoted 3 lines

This patch does not apply to the current e1000 driver in net-2.6, much of this patch has already been corrected (applied) by Roel Kluin recent patch.

Sorry I was basing off net-next. I just compared it to my fix and looks like the patch in net-2.6 has an off by one error doesn't it?

Anton

--

Quoted 5 lines

Quoted 4 lines

This patch does not apply to the current e1000 driver in net-2.6, much of this patch has already been corrected (applied) by Roel Kluin recent patch.

Quoted 2 lines

Sorry I was basing off net-next. I just compared it to my fix and looks like the patch in net-2.6 has an off by one error doesn't it?

This was discussed during our code review of Roel's patch, and it was found that there was not an issue. But I will review the code again to ensure that there is not "an off by one error". Thanks for looking at this.

Before your patch I suppose the logic disregarding the signed/unsigned error was :
1) if count==0, no unmapping/freeing inside while loop
2) if count>0, do 'count' loops unmapping/freeing

After your patch the logic is :
1) if count==0, no unmapping/freeing inside while loop
1) if count==1, no unmapping/freeing inside while loop
2) if count>1, do 'count-1' loops unmapping/freeing

Can tx_ring->count be zero? I hope not.

dma_error:

	dev_err(&pdev->dev, "TX DMA map failed\n");
	buffer_info->dma = 0;

drivers/net/e1000/e1000_main.c +0 -2

-	if (count)
-		count--;

	while (count--) {
		if (i==0)

(quoted diff) +1 -1

-			i += tx_ring->count;
+			i = tx_ring->count;

		i--;
		buffer_info = &tx_ring->buffer_info[i];
		e1000_unmap_and_free_tx_resource(adapter, buffer_info);
	}

	return 0;
}