[PATCH 0/4] ibmveth fixes

[PATCH 0/4] ibmveth fixes

[Anton Blanchard]

Here are a number of fixes found during ibmveth testing.

Anton

[PATCH 1/4] ibmveth: Fix DMA unmap error

[Anton Blanchard]

[-- Attachment #1: ibmveth_1.patch --]
[-- Type: text/plain, Size: 1208 bytes --]

From: Brian King <brking@linux.vnet.ibm.com>

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 <brking@linux.vnet.ibm.com>
Acked-by: Anton Blanchard <anton@samba.org>
Cc: <stable@kernel.org> # v2.6.37+
---

Index: linux-build/drivers/net/ibmveth.c
===================================================================
--- linux-build.orig/drivers/net/ibmveth.c	2011-09-08 08:00:16.842856634 +1000
+++ linux-build/drivers/net/ibmveth.c	2011-09-08 09:45:43.163851274 +1000
@@ -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);

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

[Anton Blanchard]

[-- Attachment #1: ibmveth_dma_mapping_error.patch --]
[-- Type: text/plain, Size: 1604 bytes --]

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 <anton@samba.org>
Cc: <stable@kernel.org> # v2.6.37+
---

Index: linux-build/drivers/net/ibmveth.c
===================================================================
--- linux-build.orig/drivers/net/ibmveth.c	2011-09-01 15:01:18.066844039 +1000
+++ linux-build/drivers/net/ibmveth.c	2011-09-01 15:03:34.299079796 +1000
@@ -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,

[PATCH 3/4] ibmveth: Checksum offload is always disabled

[Anton Blanchard]

[-- Attachment #1: ibmveth_fix_csum.patch --]
[-- Type: text/plain, Size: 766 bytes --]

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 <anton@samba.org>
Cc: <stable@kernel.org> # 3.0+
---

Index: linux-build/drivers/net/ibmveth.c
===================================================================
--- linux-build.orig/drivers/net/ibmveth.c	2011-09-01 16:02:12.198726425 +1000
+++ linux-build/drivers/net/ibmveth.c	2011-09-01 16:05:37.282482851 +1000
@@ -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;

[PATCH 4/4] ibmveth: Fix checksum offload failure handling

[Anton Blanchard]

[-- Attachment #1: ibmveth_fix_csum2.patch --]
[-- Type: text/plain, Size: 2988 bytes --]

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 <anton@samba.org>
---

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.

Index: linux-build/drivers/net/ibmveth.c
===================================================================
--- linux-build.orig/drivers/net/ibmveth.c	2011-09-01 16:06:19.000000000 +1000
+++ linux-build/drivers/net/ibmveth.c	2011-09-02 09:05:57.585353722 +1000
@@ -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;

Re: [PATCH 0/4] ibmveth fixes

[David Miller]

From: Anton Blanchard <anton@samba.org>
Date: Thu, 08 Sep 2011 10:41:02 +1000

> 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.

Re: [PATCH 0/4] ibmveth fixes

[Anton Blanchard]

Hi Dave,

> All applied.

Thanks!

> 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