[RFC 0/7] Fixing dma mask setting in various network drivers

[RFC 0/7] Fixing dma mask setting in various network drivers

[Russell King - ARM Linux]

While looking at the way coherent DMA masks are handled (and the
fact many drivers write directly to the mask) I stumbled across
this set of oddities in various network drivers, which looks like
it's been cut'n'pasted.

I haven't yet tested these patches in any way, which is one reason
I'm sending them out as an RFC.  The other reason is to find out
if other people agree that these are indeed fixes.

 drivers/net/ethernet/brocade/bna/bnad.c           |    7 +++----
 drivers/net/ethernet/intel/e1000e/netdev.c        |   11 +++++------
 drivers/net/ethernet/intel/igb/igb_main.c         |   11 +++++------
 drivers/net/ethernet/intel/igbvf/netdev.c         |   11 +++++------
 drivers/net/ethernet/intel/ixgb/ixgb_main.c       |    9 ++++-----
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     |   11 +++++------
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   11 +++++------
 7 files changed, 32 insertions(+), 39 deletions(-)

The full patchset will be sent to netdev only, the remainder will
be Cc'd depending on the individual driver being modified.  Each
patch is independent of each other.

------------------------------------------------------------------------------
This SF.net email is sponsored by Windows:

Build for Windows Store.

http://p.sf.net/sfu/windows-dev2dev
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired

[PATCH 1/7] NET: brocade/bna/bnad.c: fix 32-bit DMA mask handling

[Russell King]

The fallback to 32-bit DMA mask is rather odd:
	if (!dma_set_mask(&pdev->dev, DMA_BIT_MASK(64)) &&
	    !dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64))) {
		*using_dac = true;
	} else {
		err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
		if (err) {
			err = dma_set_coherent_mask(&pdev->dev,
						    DMA_BIT_MASK(32));
			if (err)
				goto release_regions;
		}

This means we only try and set the coherent DMA mask if we failed to
set a 32-bit DMA mask, and only if both fail do we fail the driver.
Adjust this so that if either setting fails, we fail the driver - and
thereby end up properly setting both the DMA mask and the coherent
DMA mask in the fallback case.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/net/ethernet/brocade/bna/bnad.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/brocade/bna/bnad.c b/drivers/net/ethernet/brocade/bna/bnad.c
index 07f7ef0..15feb8b 100644
--- a/drivers/net/ethernet/brocade/bna/bnad.c
+++ b/drivers/net/ethernet/brocade/bna/bnad.c
@@ -3302,12 +3302,11 @@ bnad_pci_init(struct bnad *bnad,
 		*using_dac = true;
 	} else {
 		err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
-		if (err) {
+		if (!err)
 			err = dma_set_coherent_mask(&pdev->dev,
 						    DMA_BIT_MASK(32));
-			if (err)
-				goto release_regions;
-		}
+		if (err)
+			goto release_regions;
 		*using_dac = false;
 	}
 	pci_set_master(pdev);
-- 
1.7.4.4

[PATCH 2/7] NET: intel/e1000e/netdev.c: fix 32-bit DMA mask handling

[Russell King]

The fallback to 32-bit DMA mask is rather odd:
	err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(64));
	if (!err) {
		err = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64));
		if (!err)
			pci_using_dac = 1;
	} else {
		err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
		if (err) {
			err = dma_set_coherent_mask(&pdev->dev,
						    DMA_BIT_MASK(32));
			if (err) {
				dev_err(&pdev->dev,
					"No usable DMA configuration, aborting\n");
				goto err_dma;
			}
		}
	}
This means we only set the coherent DMA mask in the fallback path if
the DMA mask set failed, which is silly.  This fixes it to set the
coherent DMA mask only if dma_set_mask() succeeded, and to error out
if either fails.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/net/ethernet/intel/e1000e/netdev.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index a27e3bc..cbf8599 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -6518,14 +6518,13 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 			pci_using_dac = 1;
 	} else {
 		err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
-		if (err) {
+		if (!err)
 			err = dma_set_coherent_mask(&pdev->dev,
 						    DMA_BIT_MASK(32));
-			if (err) {
-				dev_err(&pdev->dev,
-					"No usable DMA configuration, aborting\n");
-				goto err_dma;
-			}
+		if (err) {
+			dev_err(&pdev->dev,
+				"No usable DMA configuration, aborting\n");
+			goto err_dma;
 		}
 	}
 
-- 
1.7.4.4

Re: [PATCH 2/7] NET: intel/e1000e/netdev.c: fix 32-bit DMA mask handling

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 1375 bytes --]

On Tue, 2013-06-11 at 00:10 +0100, Russell King wrote:
> The fallback to 32-bit DMA mask is rather odd:
>         err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(64));
>         if (!err) {
>                 err = dma_set_coherent_mask(&pdev->dev,
> DMA_BIT_MASK(64));
>                 if (!err)
>                         pci_using_dac = 1;
>         } else {
>                 err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
>                 if (err) {
>                         err = dma_set_coherent_mask(&pdev->dev,
>                                                     DMA_BIT_MASK(32));
>                         if (err) {
>                                 dev_err(&pdev->dev,
>                                         "No usable DMA configuration,
> aborting\n");
>                                 goto err_dma;
>                         }
>                 }
>         }
> This means we only set the coherent DMA mask in the fallback path if
> the DMA mask set failed, which is silly.  This fixes it to set the
> coherent DMA mask only if dma_set_mask() succeeded, and to error out
> if either fails.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  drivers/net/ethernet/intel/e1000e/netdev.c |   11 +++++------
>  1 files changed, 5 insertions(+), 6 deletions(-) 

Thanks Russell, I will add this to my queue.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 3/7] NET: intel/igb/igb_main.c: fix 32-bit DMA mask handling

[Russell King]

The fallback to 32-bit DMA mask is rather odd:
	err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(64));
	if (!err) {
		err = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64));
		if (!err)
			pci_using_dac = 1;
	} else {
		err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
		if (err) {
			err = dma_set_coherent_mask(&pdev->dev,
						    DMA_BIT_MASK(32));
			if (err) {
				dev_err(&pdev->dev,
					"No usable DMA configuration, aborting\n");
				goto err_dma;
			}
		}
	}
This means we only set the coherent DMA mask in the fallback path if
the DMA mask set failed, which is silly.  This fixes it to set the
coherent DMA mask only if dma_set_mask() succeeded, and to error out
if either fails.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/net/ethernet/intel/igb/igb_main.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 64cbe0d..b64b9e3 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -2024,14 +2024,13 @@ static int igb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 			pci_using_dac = 1;
 	} else {
 		err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
-		if (err) {
+		if (!err)
 			err = dma_set_coherent_mask(&pdev->dev,
 						    DMA_BIT_MASK(32));
-			if (err) {
-				dev_err(&pdev->dev,
-					"No usable DMA configuration, aborting\n");
-				goto err_dma;
-			}
+		if (err) {
+			dev_err(&pdev->dev,
+				"No usable DMA configuration, aborting\n");
+			goto err_dma;
 		}
 	}
 
-- 
1.7.4.4

Re: [PATCH 3/7] NET: intel/igb/igb_main.c: fix 32-bit DMA mask handling

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 1374 bytes --]

On Tue, 2013-06-11 at 00:11 +0100, Russell King wrote:
> The fallback to 32-bit DMA mask is rather odd:
>         err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(64));
>         if (!err) {
>                 err = dma_set_coherent_mask(&pdev->dev,
> DMA_BIT_MASK(64));
>                 if (!err)
>                         pci_using_dac = 1;
>         } else {
>                 err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
>                 if (err) {
>                         err = dma_set_coherent_mask(&pdev->dev,
>                                                     DMA_BIT_MASK(32));
>                         if (err) {
>                                 dev_err(&pdev->dev,
>                                         "No usable DMA configuration,
> aborting\n");
>                                 goto err_dma;
>                         }
>                 }
>         }
> This means we only set the coherent DMA mask in the fallback path if
> the DMA mask set failed, which is silly.  This fixes it to set the
> coherent DMA mask only if dma_set_mask() succeeded, and to error out
> if either fails.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  drivers/net/ethernet/intel/igb/igb_main.c |   11 +++++------
>  1 files changed, 5 insertions(+), 6 deletions(-) 

Thanks Russell, I will add this to my queue.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 4/7] NET: intel/igbvf/netdev.c: fix 32-bit DMA mask handling

[Russell King]

The fallback to 32-bit DMA mask is rather odd:
	err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(64));
	if (!err) {
		err = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64));
		if (!err)
			pci_using_dac = 1;
	} else {
		err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
		if (err) {
			err = dma_set_coherent_mask(&pdev->dev,
						    DMA_BIT_MASK(32));
			if (err) {
				dev_err(&pdev->dev, "No usable DMA "
					"configuration, aborting\n");
				goto err_dma;
			}
		}
	}
This means we only set the coherent DMA mask in the fallback path if
the DMA mask set failed, which is silly.  This fixes it to set the
coherent DMA mask only if dma_set_mask() succeeded, and to error out
if either fails.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/net/ethernet/intel/igbvf/netdev.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/igbvf/netdev.c b/drivers/net/ethernet/intel/igbvf/netdev.c
index 93eb7ee..b4baf3f 100644
--- a/drivers/net/ethernet/intel/igbvf/netdev.c
+++ b/drivers/net/ethernet/intel/igbvf/netdev.c
@@ -2645,14 +2645,13 @@ static int igbvf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 			pci_using_dac = 1;
 	} else {
 		err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
-		if (err) {
+		if (!err)
 			err = dma_set_coherent_mask(&pdev->dev,
 						    DMA_BIT_MASK(32));
-			if (err) {
-				dev_err(&pdev->dev, "No usable DMA "
-				        "configuration, aborting\n");
-				goto err_dma;
-			}
+		if (err) {
+			dev_err(&pdev->dev, "No usable DMA "
+			        "configuration, aborting\n");
+			goto err_dma;
 		}
 	}
 
-- 
1.7.4.4

Re: [PATCH 4/7] NET: intel/igbvf/netdev.c: fix 32-bit DMA mask handling

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 1374 bytes --]

On Tue, 2013-06-11 at 00:12 +0100, Russell King wrote:
> The fallback to 32-bit DMA mask is rather odd:
>         err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(64));
>         if (!err) {
>                 err = dma_set_coherent_mask(&pdev->dev,
> DMA_BIT_MASK(64));
>                 if (!err)
>                         pci_using_dac = 1;
>         } else {
>                 err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
>                 if (err) {
>                         err = dma_set_coherent_mask(&pdev->dev,
>                                                     DMA_BIT_MASK(32));
>                         if (err) {
>                                 dev_err(&pdev->dev, "No usable DMA "
>                                         "configuration, aborting\n");
>                                 goto err_dma;
>                         }
>                 }
>         }
> This means we only set the coherent DMA mask in the fallback path if
> the DMA mask set failed, which is silly.  This fixes it to set the
> coherent DMA mask only if dma_set_mask() succeeded, and to error out
> if either fails.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  drivers/net/ethernet/intel/igbvf/netdev.c |   11 +++++------
>  1 files changed, 5 insertions(+), 6 deletions(-) 

Thanks Russell, I will add this to my queue.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 5/7] NET: intel/ixgb/ixgb_main.c: fix 32-bit DMA mask handling

[Russell King]

The fallback to 32-bit DMA mask is rather odd:
	err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(64));
	if (!err) {
		err = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64));
		if (!err)
			pci_using_dac = 1;
	} else {
		err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
		if (err) {
			err = dma_set_coherent_mask(&pdev->dev,
						    DMA_BIT_MASK(32));
			if (err) {
				pr_err("No usable DMA configuration, aborting\n");
				goto err_dma_mask;
			}
		}
	}
This means we only set the coherent DMA mask in the fallback path if
the DMA mask set failed, which is silly.  This fixes it to set the
coherent DMA mask only if dma_set_mask() succeeded, and to error out
if either fails.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/net/ethernet/intel/ixgb/ixgb_main.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgb/ixgb_main.c b/drivers/net/ethernet/intel/ixgb/ixgb_main.c
index fce3e92..136829e 100644
--- a/drivers/net/ethernet/intel/ixgb/ixgb_main.c
+++ b/drivers/net/ethernet/intel/ixgb/ixgb_main.c
@@ -415,13 +415,12 @@ ixgb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 			pci_using_dac = 1;
 	} else {
 		err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
-		if (err) {
+		if (!err)
 			err = dma_set_coherent_mask(&pdev->dev,
 						    DMA_BIT_MASK(32));
-			if (err) {
-				pr_err("No usable DMA configuration, aborting\n");
-				goto err_dma_mask;
-			}
+		if (err) {
+			pr_err("No usable DMA configuration, aborting\n");
+			goto err_dma_mask;
 		}
 	}
 
-- 
1.7.4.4

Re: [PATCH 5/7] NET: intel/ixgb/ixgb_main.c: fix 32-bit DMA mask handling

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 1323 bytes --]

On Tue, 2013-06-11 at 00:13 +0100, Russell King wrote:
> The fallback to 32-bit DMA mask is rather odd:
>         err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(64));
>         if (!err) {
>                 err = dma_set_coherent_mask(&pdev->dev,
> DMA_BIT_MASK(64));
>                 if (!err)
>                         pci_using_dac = 1;
>         } else {
>                 err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
>                 if (err) {
>                         err = dma_set_coherent_mask(&pdev->dev,
>                                                     DMA_BIT_MASK(32));
>                         if (err) {
>                                 pr_err("No usable DMA configuration,
> aborting\n");
>                                 goto err_dma_mask;
>                         }
>                 }
>         }
> This means we only set the coherent DMA mask in the fallback path if
> the DMA mask set failed, which is silly.  This fixes it to set the
> coherent DMA mask only if dma_set_mask() succeeded, and to error out
> if either fails.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  drivers/net/ethernet/intel/ixgb/ixgb_main.c |    9 ++++-----
>  1 files changed, 4 insertions(+), 5 deletions(-) 

Thanks Russell, I will add this to my queue.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 6/7] NET: intel/ixgbe/ixgbe_main.c: fix 32-bit DMA mask handling

[Russell King]

The fallback to 32-bit DMA mask is rather odd:
	if (!dma_set_mask(&pdev->dev, DMA_BIT_MASK(64)) &&
	    !dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64))) {
		pci_using_dac = 1;
	} else {
		err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
		if (err) {
			err = dma_set_coherent_mask(&pdev->dev,
						    DMA_BIT_MASK(32));
			if (err) {
				dev_err(&pdev->dev,
					"No usable DMA configuration, aborting\n");
				goto err_dma;
			}
		}
		pci_using_dac = 0;
	}
This means we only set the coherent DMA mask in the fallback path if
the DMA mask set failed, which is silly.  This fixes it to set the
coherent DMA mask only if dma_set_mask() succeeded, and to error out
if either fails.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index d30fbdd..e1945c9 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -7301,14 +7301,13 @@ static int ixgbe_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		pci_using_dac = 1;
 	} else {
 		err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
-		if (err) {
+		if (!err)
 			err = dma_set_coherent_mask(&pdev->dev,
 						    DMA_BIT_MASK(32));
-			if (err) {
-				dev_err(&pdev->dev,
-					"No usable DMA configuration, aborting\n");
-				goto err_dma;
-			}
+		if (err) {
+			dev_err(&pdev->dev,
+				"No usable DMA configuration, aborting\n");
+			goto err_dma;
 		}
 		pci_using_dac = 0;
 	}
-- 
1.7.4.4

Re: [PATCH 6/7] NET: intel/ixgbe/ixgbe_main.c: fix 32-bit DMA mask handling

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 1347 bytes --]

On Tue, 2013-06-11 at 00:14 +0100, Russell King wrote:
> The fallback to 32-bit DMA mask is rather odd:
>         if (!dma_set_mask(&pdev->dev, DMA_BIT_MASK(64)) &&
>             !dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64))) {
>                 pci_using_dac = 1;
>         } else {
>                 err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
>                 if (err) {
>                         err = dma_set_coherent_mask(&pdev->dev,
>                                                     DMA_BIT_MASK(32));
>                         if (err) {
>                                 dev_err(&pdev->dev,
>                                         "No usable DMA configuration,
> aborting\n");
>                                 goto err_dma;
>                         }
>                 }
>                 pci_using_dac = 0;
>         }
> This means we only set the coherent DMA mask in the fallback path if
> the DMA mask set failed, which is silly.  This fixes it to set the
> coherent DMA mask only if dma_set_mask() succeeded, and to error out
> if either fails.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   11 +++++------
>  1 files changed, 5 insertions(+), 6 deletions(-) 

Thanks Russell, I will add this to my queue.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 7/7] NET: intel/ixgbevf/ixgbevf_main.c: fix 32-bit DMA mask handling

[Russell King]

The fallback to 32-bit DMA mask is rather odd:
	if (!dma_set_mask(&pdev->dev, DMA_BIT_MASK(64)) &&
	    !dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64))) {
		pci_using_dac = 1;
	} else {
		err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
		if (err) {
			err = dma_set_coherent_mask(&pdev->dev,
						    DMA_BIT_MASK(32));
			if (err) {
				dev_err(&pdev->dev, "No usable DMA "
					"configuration, aborting\n");
				goto err_dma;
			}
		}
		pci_using_dac = 0;
	}
This means we only set the coherent DMA mask in the fallback path if
the DMA mask set failed, which is silly.  This fixes it to set the
coherent DMA mask only if dma_set_mask() succeeded, and to error out
if either fails.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
index 1f5166a..1ae22e0 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
@@ -3331,14 +3331,13 @@ static int ixgbevf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		pci_using_dac = 1;
 	} else {
 		err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
-		if (err) {
+		if (!err)
 			err = dma_set_coherent_mask(&pdev->dev,
 						    DMA_BIT_MASK(32));
-			if (err) {
-				dev_err(&pdev->dev, "No usable DMA "
-					"configuration, aborting\n");
-				goto err_dma;
-			}
+		if (err) {
+			dev_err(&pdev->dev, "No usable DMA "
+				"configuration, aborting\n");
+			goto err_dma;
 		}
 		pci_using_dac = 0;
 	}
-- 
1.7.4.4

Re: [PATCH 7/7] NET: intel/ixgbevf/ixgbevf_main.c: fix 32-bit DMA mask handling

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 1355 bytes --]

On Tue, 2013-06-11 at 00:15 +0100, Russell King wrote:
> 
> The fallback to 32-bit DMA mask is rather odd:
>         if (!dma_set_mask(&pdev->dev, DMA_BIT_MASK(64)) &&
>             !dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64))) {
>                 pci_using_dac = 1;
>         } else {
>                 err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
>                 if (err) {
>                         err = dma_set_coherent_mask(&pdev->dev,
>                                                     DMA_BIT_MASK(32));
>                         if (err) {
>                                 dev_err(&pdev->dev, "No usable DMA "
>                                         "configuration, aborting\n");
>                                 goto err_dma;
>                         }
>                 }
>                 pci_using_dac = 0;
>         }
> This means we only set the coherent DMA mask in the fallback path if
> the DMA mask set failed, which is silly.  This fixes it to set the
> coherent DMA mask only if dma_set_mask() succeeded, and to error out
> if either fails.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   11 +++++------
>  1 files changed, 5 insertions(+), 6 deletions(-) 

Thanks Russell, I will add this to my queue.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC 0/7] Fixing dma mask setting in various network drivers

[Jesse Brandeburg]

On Tue, 11 Jun 2013 00:08:49 +0100
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> While looking at the way coherent DMA masks are handled (and the
> fact many drivers write directly to the mask) I stumbled across
> this set of oddities in various network drivers, which looks like
> it's been cut'n'pasted.
> 
> I haven't yet tested these patches in any way, which is one reason
> I'm sending them out as an RFC.  The other reason is to find out
> if other people agree that these are indeed fixes.
> 
>  drivers/net/ethernet/brocade/bna/bnad.c           |    7 +++----
>  drivers/net/ethernet/intel/e1000e/netdev.c        |   11 +++++------
>  drivers/net/ethernet/intel/igb/igb_main.c         |   11 +++++------
>  drivers/net/ethernet/intel/igbvf/netdev.c         |   11 +++++------
>  drivers/net/ethernet/intel/ixgb/ixgb_main.c       |    9 ++++-----
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     |   11 +++++------
>  drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   11 +++++------
>  7 files changed, 32 insertions(+), 39 deletions(-)

Thanks Russell,

The intel driver changes seem valid (we are testing them now).
According to DMA-API-HOWTO, the coherent mask will always succeed if
the regular mask succeeded, so the code can be further simplified as
well to basically match the example in DMA-API-HOWTO.

This is my proposed change to the intel drivers.  Comments?

+	if (!dma_set_mask(&pdev->dev, DMA_BIT_MASK(64))) {
+		pci_using_dac = true;
+		/* coherent mask for the same size will always succeed if
+		 * dma_set_mask does
+		 */
+		dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64));
+	} else if (!dma_set_mask(&pdev->dev, DMA_BIT_MASK(32))) {
+		pci_using_dac = false;
+		dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
+	} else {
+		dev_err(&pdev->dev, "%s: DMA configuration failed: %d\n",
+			 __func__, err);
+		err = -EIO;
+		goto err_dma;
 	}

Re: [RFC 0/7] Fixing dma mask setting in various network drivers

[Russell King - ARM Linux]

On Tue, Jun 11, 2013 at 11:12:30AM -0700, Jesse Brandeburg wrote:
> On Tue, 11 Jun 2013 00:08:49 +0100
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> 
> > While looking at the way coherent DMA masks are handled (and the
> > fact many drivers write directly to the mask) I stumbled across
> > this set of oddities in various network drivers, which looks like
> > it's been cut'n'pasted.
> > 
> > I haven't yet tested these patches in any way, which is one reason
> > I'm sending them out as an RFC.  The other reason is to find out
> > if other people agree that these are indeed fixes.
> > 
> >  drivers/net/ethernet/brocade/bna/bnad.c           |    7 +++----
> >  drivers/net/ethernet/intel/e1000e/netdev.c        |   11 +++++------
> >  drivers/net/ethernet/intel/igb/igb_main.c         |   11 +++++------
> >  drivers/net/ethernet/intel/igbvf/netdev.c         |   11 +++++------
> >  drivers/net/ethernet/intel/ixgb/ixgb_main.c       |    9 ++++-----
> >  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     |   11 +++++------
> >  drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   11 +++++------
> >  7 files changed, 32 insertions(+), 39 deletions(-)
> 
> Thanks Russell,
> 
> The intel driver changes seem valid (we are testing them now).
> According to DMA-API-HOWTO, the coherent mask will always succeed if
> the regular mask succeeded, so the code can be further simplified as
> well to basically match the example in DMA-API-HOWTO.
> 
> This is my proposed change to the intel drivers.  Comments?
> 
> +	if (!dma_set_mask(&pdev->dev, DMA_BIT_MASK(64))) {
> +		pci_using_dac = true;
> +		/* coherent mask for the same size will always succeed if
> +		 * dma_set_mask does
> +		 */
> +		dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64));
> +	} else if (!dma_set_mask(&pdev->dev, DMA_BIT_MASK(32))) {
> +		pci_using_dac = false;
> +		dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
> +	} else {
> +		dev_err(&pdev->dev, "%s: DMA configuration failed: %d\n",
> +			 __func__, err);
> +		err = -EIO;
> +		goto err_dma;
>  	}

Yep, other drivers do that too, and as you say, the documentation says
it's a valid "optimization", so I don't see a problem with it - if
there is, the documentation would need fixing!

As part of my review of all this stuff, I'm wondering whether a helper
to set both masks makes sense.  Something like:

static inline int dma_set_masks(struct device *dev, u64 mask)
{
	int ret = dma_set_mask(dev, mask);
	if (ret == 0)
		dma_set_coherent_mask(dev, mask);
	return ret;
}

"dma_set_masks()" is a little too close to dma_set_mask() though; and
such a function looks like it would be usable for 20 odd drivers
currently.  The plus point is that it may help to prevent this kind
of issue in the future...

Thoughts?

------------------------------------------------------------------------------
This SF.net email is sponsored by Windows:

Build for Windows Store.

http://p.sf.net/sfu/windows-dev2dev
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

Re: [RFC 0/7] Fixing dma mask setting in various network drivers

[Jesse Brandeburg]

On Tue, 11 Jun 2013 13:35:05 -0700
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> As part of my review of all this stuff, I'm wondering whether a helper
> to set both masks makes sense.  Something like:
> 
> static inline int dma_set_masks(struct device *dev, u64 mask)

it doesn't need to be inline, it is never called in hotpath.

> {
> 	int ret = dma_set_mask(dev, mask);
> 	if (ret == 0)
> 		dma_set_coherent_mask(dev, mask);
> 	return ret;
> }
> 
> "dma_set_masks()" is a little too close to dma_set_mask() though; and

how about dma_set_mask_and_coherent(...)

> such a function looks like it would be usable for 20 odd drivers
> currently.  The plus point is that it may help to prevent this kind
> of issue in the future...
> 
> Thoughts?

I really like the idea of consolidating this in the kernel with a
global helper.


------------------------------------------------------------------------------
This SF.net email is sponsored by Windows:

Build for Windows Store.

http://p.sf.net/sfu/windows-dev2dev
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

Re: [RFC 0/7] Fixing dma mask setting in various network drivers

[Russell King - ARM Linux]

On Tue, Jun 11, 2013 at 04:50:44PM -0700, Jesse Brandeburg wrote:
> On Tue, 11 Jun 2013 13:35:05 -0700
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> > As part of my review of all this stuff, I'm wondering whether a helper
> > to set both masks makes sense.  Something like:
> > 
> > static inline int dma_set_masks(struct device *dev, u64 mask)
> 
> it doesn't need to be inline, it is never called in hotpath.

If it goes into linux/dma-mapping.h as a simple helper it does, otherwise
you'll get "declared but not used" warnings.

> > {
> > 	int ret = dma_set_mask(dev, mask);
> > 	if (ret == 0)
> > 		dma_set_coherent_mask(dev, mask);
> > 	return ret;
> > }
> > 
> > "dma_set_masks()" is a little too close to dma_set_mask() though; and
> 
> how about dma_set_mask_and_coherent(...)

Yea, I'm happy with that.

------------------------------------------------------------------------------
This SF.net email is sponsored by Windows:

Build for Windows Store.

http://p.sf.net/sfu/windows-dev2dev
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

Re: [RFC 0/7] Fixing dma mask setting in various network drivers

[Russell King - ARM Linux]

On Tue, Jun 11, 2013 at 11:12:30AM -0700, Jesse Brandeburg wrote:
> This is my proposed change to the intel drivers.  Comments?
> 
> +	if (!dma_set_mask(&pdev->dev, DMA_BIT_MASK(64))) {
> +		pci_using_dac = true;
> +		/* coherent mask for the same size will always succeed if
> +		 * dma_set_mask does
> +		 */
> +		dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64));
> +	} else if (!dma_set_mask(&pdev->dev, DMA_BIT_MASK(32))) {
> +		pci_using_dac = false;
> +		dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
> +	} else {
> +		dev_err(&pdev->dev, "%s: DMA configuration failed: %d\n",
> +			 __func__, err);
> +		err = -EIO;
> +		goto err_dma;
>  	}
> 

So, will you be going with this change rather than mine, which apparantly
Jeff queued?  Please let me know what you decide so I can keep my private
git tree in sync with what you've decided to avoid conflicting with
further dma-mask changes which I'm working on.

Thanks.

------------------------------------------------------------------------------
This SF.net email is sponsored by Windows:

Build for Windows Store.

http://p.sf.net/sfu/windows-dev2dev
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired

Re: [RFC 0/7] Fixing dma mask setting in various network drivers

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 1307 bytes --]

On Mon, 2013-06-17 at 15:01 +0100, Russell King - ARM Linux wrote:
> On Tue, Jun 11, 2013 at 11:12:30AM -0700, Jesse Brandeburg wrote:
> > This is my proposed change to the intel drivers.  Comments?
> > 
> > +	if (!dma_set_mask(&pdev->dev, DMA_BIT_MASK(64))) {
> > +		pci_using_dac = true;
> > +		/* coherent mask for the same size will always succeed if
> > +		 * dma_set_mask does
> > +		 */
> > +		dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64));
> > +	} else if (!dma_set_mask(&pdev->dev, DMA_BIT_MASK(32))) {
> > +		pci_using_dac = false;
> > +		dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
> > +	} else {
> > +		dev_err(&pdev->dev, "%s: DMA configuration failed: %d\n",
> > +			 __func__, err);
> > +		err = -EIO;
> > +		goto err_dma;
> >  	}
> > 
> 
> So, will you be going with this change rather than mine, which apparantly
> Jeff queued?  Please let me know what you decide so I can keep my private
> git tree in sync with what you've decided to avoid conflicting with
> further dma-mask changes which I'm working on.
> 
> Thanks.

I thought the kernel helper function dma_set_mask_and_coherent(...) was
the best solution.

I dropped your original series of patches from my queue since it
appeared that there was a better alternative to this solution.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC 0/7] Fixing dma mask setting in various network drivers

[Russell King - ARM Linux]

On Mon, Jun 17, 2013 at 01:35:41PM -0700, Jeff Kirsher wrote:
> On Mon, 2013-06-17 at 15:01 +0100, Russell King - ARM Linux wrote:
> > On Tue, Jun 11, 2013 at 11:12:30AM -0700, Jesse Brandeburg wrote:
> > > This is my proposed change to the intel drivers.  Comments?
> > > 
> > > +	if (!dma_set_mask(&pdev->dev, DMA_BIT_MASK(64))) {
> > > +		pci_using_dac = true;
> > > +		/* coherent mask for the same size will always succeed if
> > > +		 * dma_set_mask does
> > > +		 */
> > > +		dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64));
> > > +	} else if (!dma_set_mask(&pdev->dev, DMA_BIT_MASK(32))) {
> > > +		pci_using_dac = false;
> > > +		dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
> > > +	} else {
> > > +		dev_err(&pdev->dev, "%s: DMA configuration failed: %d\n",
> > > +			 __func__, err);
> > > +		err = -EIO;
> > > +		goto err_dma;
> > >  	}
> > > 
> > 
> > So, will you be going with this change rather than mine, which apparantly
> > Jeff queued?  Please let me know what you decide so I can keep my private
> > git tree in sync with what you've decided to avoid conflicting with
> > further dma-mask changes which I'm working on.
> > 
> > Thanks.
> 
> I thought the kernel helper function dma_set_mask_and_coherent(...) was
> the best solution.
> 
> I dropped your original series of patches from my queue since it
> appeared that there was a better alternative to this solution.

Such a helper doesn't exist yet, and I'm not sure these changes should
wait for that.  Hmm, okay, it's probably too late in the -rc cycle
anyway for these changes now.

I'll respin them according to Jesse's patch above, but as I have already
mentioned, this will be part of a much larger series, and I was hoping
to get those trivial fixes in for 3.10 to avoid that kind of dependency.
Instead, they'll be stuck at the bottom of a much larger series, which
will depend on these changes.  Bah.