Commit 6e8ab30ec677 (ibmveth: Add scatter-gather support) introduced a DMA mapping API inconsistency resulting in dma_unmap_page getting called on memory mapped via dma_map_single. This was seen when CONFIG_DMA_API_DEBUG was enabled. Fix up this API usage inconsistency.

Signed-off-by Brian King <[email protected]>

Acked-by Anton Blanchard <[email protected]>

Cc <[email protected]> # v2.6.37+

---

drivers/net/ibmveth.c +6 -1

Index: linux-build/drivers/net/ibmveth.c
===================================================================
@@ -1026,7 +1026,12 @@ retry_bounce:
 		netdev->stats.tx_bytes += skb->len;
 	}
 
-	for (i = 0; i < skb_shinfo(skb)->nr_frags + 1; i++)
+	dma_unmap_single(&adapter->vdev->dev,
+			 descs[0].fields.address,
+			 descs[0].fields.flags_len & IBMVETH_BUF_LEN_MASK,
+			 DMA_TO_DEVICE);
+
+	for (i = 1; i < skb_shinfo(skb)->nr_frags + 1; i++)
 		dma_unmap_page(&adapter->vdev->dev, descs[i].fields.address,
 			       descs[i].fields.flags_len & IBMVETH_BUF_LEN_MASK,
 			       DMA_TO_DEVICE);

Anton Blanchard

author

2011-09-08 00:45 UTC #2 lore raw

PATCH 2/4 ibmveth: Fix issue with DMA mapping failure

In reply to Anton Blanchard

descs[].fields.address is 32bit which truncates any dma mapping errors so dma_mapping_error() fails to catch it.

Use a dma_addr_t to do the comparison. With this patch I was able to transfer many gigabytes of data with IOMMU fault injection set at 10% probability.

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

Cc <[email protected]> # v2.6.37+

---

drivers/net/ibmveth.c +5 -5

Index: linux-build/drivers/net/ibmveth.c
===================================================================
@@ -930,6 +930,7 @@ static netdev_tx_t ibmveth_start_xmit(st
 	union ibmveth_buf_desc descs[6];
 	int last, i;
 	int force_bounce = 0;
+	dma_addr_t dma_addr;
 
 	/*
 	 * veth handles a maximum of 6 segments including the header, so

@@ -994,17 +995,16 @@ retry_bounce:
 	}
 
 	/* Map the header */
-	descs[0].fields.address = dma_map_single(&adapter->vdev->dev, skb->data,
-						 skb_headlen(skb),
-						 DMA_TO_DEVICE);
-	if (dma_mapping_error(&adapter->vdev->dev, descs[0].fields.address))
+	dma_addr = dma_map_single(&adapter->vdev->dev, skb->data,
+				  skb_headlen(skb), DMA_TO_DEVICE);
+	if (dma_mapping_error(&adapter->vdev->dev, dma_addr))
 		goto map_failed;
 
 	descs[0].fields.flags_len = desc_flags | skb_headlen(skb);
+	descs[0].fields.address = dma_addr;
 
 	/* Map the frags */
 	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
-		unsigned long dma_addr;
 		skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
 
 		dma_addr = dma_map_page(&adapter->vdev->dev, frag->page,

Anton Blanchard

author

2011-09-08 00:45 UTC #3 lore raw

PATCH 3/4 ibmveth: Checksum offload is always disabled

In reply to Anton Blanchard

Commit b9367bf3ee6d (net: ibmveth: convert to hw_features) reversed a check in ibmveth_set_csum_offload that results in checksum offload never being enabled.

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

Cc <[email protected]> # 3.0+

---

drivers/net/ibmveth.c +1 -1

Index: linux-build/drivers/net/ibmveth.c
===================================================================
@@ -812,7 +812,7 @@ static int ibmveth_set_csum_offload(stru
 		} else
 			adapter->fw_ipv6_csum_support = data;
 
-		if (ret != H_SUCCESS || ret6 != H_SUCCESS)
+		if (ret == H_SUCCESS || ret6 == H_SUCCESS)
 			adapter->rx_csum = data;
 		else
 			rc1 = -EIO;

Anton Blanchard

author

2011-09-08 00:45 UTC #4 lore raw

PATCH 4/4 ibmveth: Fix checksum offload failure handling

In reply to Anton Blanchard

Fix a number of issues in ibmveth_set_csum_offload:

- set_attr6 and clr_attr6 may be used uninitialised

- We store the result of the IPV4 checksum change in ret but overwrite it in a couple of places before checking it again later. Add ret4 to make it obvious what we are doing.

- We weren't clearing the NETIF_F_IP_CSUM and NETIF_F_IPV6_CSUM flags if the enable of that hypervisor feature failed.

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

---

This patch could do with some review and I haven't marked it -stable yet.

One thing that stands out is that we need to return an error back through set_features if any feature bit gets changed.

drivers/net/ibmveth.c +20 ~~-11~~

Index: linux-build/drivers/net/ibmveth.c
===================================================================
@@ -757,7 +757,7 @@ static int ibmveth_set_csum_offload(stru
 	struct ibmveth_adapter *adapter = netdev_priv(dev);
 	unsigned long set_attr, clr_attr, ret_attr;
 	unsigned long set_attr6, clr_attr6;
-	long ret, ret6;
+	long ret, ret4, ret6;
 	int rc1 = 0, rc2 = 0;
 	int restart = 0;

@@ -770,6 +770,8 @@ static int ibmveth_set_csum_offload(stru
 
 	set_attr = 0;
 	clr_attr = 0;
+	set_attr6 = 0;
+	clr_attr6 = 0;
 
 	if (data) {
 		set_attr = IBMVETH_ILLAN_IPV4_TCP_CSUM;

@@ -784,16 +786,20 @@ static int ibmveth_set_csum_offload(stru
 	if (ret == H_SUCCESS && !(ret_attr & IBMVETH_ILLAN_ACTIVE_TRUNK) &&
 	    !(ret_attr & IBMVETH_ILLAN_TRUNK_PRI_MASK) &&
 	    (ret_attr & IBMVETH_ILLAN_PADDED_PKT_CSUM)) {
-		ret = h_illan_attributes(adapter->vdev->unit_address, clr_attr,
+		ret4 = h_illan_attributes(adapter->vdev->unit_address, clr_attr,
 					 set_attr, &ret_attr);
 
-		if (ret != H_SUCCESS) {
+		if (ret4 != H_SUCCESS) {
 			netdev_err(dev, "unable to change IPv4 checksum "
 					"offload settings. %d rc=%ld\n",
-					data, ret);
+					data, ret4);
+
+			h_illan_attributes(adapter->vdev->unit_address,
+					   set_attr, clr_attr, &ret_attr);
+
+			if (data == 1)
+				dev->features &= ~NETIF_F_IP_CSUM;
 
-			ret = h_illan_attributes(adapter->vdev->unit_address,
-						 set_attr, clr_attr, &ret_attr);
 		} else {
 			adapter->fw_ipv4_csum_support = data;
 		}

@@ -804,15 +810,18 @@ static int ibmveth_set_csum_offload(stru
 		if (ret6 != H_SUCCESS) {
 			netdev_err(dev, "unable to change IPv6 checksum "
 					"offload settings. %d rc=%ld\n",
-					data, ret);
+					data, ret6);
+
+			h_illan_attributes(adapter->vdev->unit_address,
+					   set_attr6, clr_attr6, &ret_attr);
+
+			if (data == 1)
+				dev->features &= ~NETIF_F_IPV6_CSUM;
 
-			ret = h_illan_attributes(adapter->vdev->unit_address,
-						 set_attr6, clr_attr6,
-						 &ret_attr);
 		} else
 			adapter->fw_ipv6_csum_support = data;
 
-		if (ret == H_SUCCESS || ret6 == H_SUCCESS)
+		if (ret4 == H_SUCCESS || ret6 == H_SUCCESS)
 			adapter->rx_csum = data;
 		else
 			rc1 = -EIO;

David Miller

maintainer

2011-09-16 19:28 UTC #5 lore raw

In reply to Anton Blanchard

From: Anton Blanchard <[email protected]>
Date: Thu, 08 Sep 2011 10:41:02 +1000

Quoted 1 line

Here are a number of fixes found during ibmveth testing.

All applied.

I love those "ret1", "ret2", "ret3", "ret4" variables.

Each successive number in the names gives such incredible insight into the meaning and usage of the individual variables, making it painless for reviewers to understand this code.

Anton Blanchard

author

2011-09-16 20:51 UTC #6 lore raw

Hi Dave,

Quoted 1 line

All applied.

Thanks!

Quoted 6 lines

I love those "ret1", "ret2", "ret3", "ret4" variables.
Each successive number in the names gives such incredible insight into the meaning and usage of the individual variables, making it painless for reviewers to understand this code.

:) That function needs to be simplified and rewritten, it took me a while to get my head around it.

Anton