Fixing dma mask setting in various network drivers
cover Fixing dma mask setting in various network drivers
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
[email protected]
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
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.
--- drivers/net/ethernet/brocade/bna/bnad.c | 7 +++---- 1 files changed, 3 insertions(+), 4 deletions(-)
drivers/net/ethernet/brocade/bna/bnad.c +3 -4
@@ -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);
Signature · 1 line
1.7.4.4
PATCH 2/7 NET: intel/e1000e/netdev.c: fix 32-bit DMA mask handling
In reply to Russell King - ARM LinuxThe 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.
--- drivers/net/ethernet/intel/e1000e/netdev.c | 11 +++++------ 1 files changed, 5 insertions(+), 6 deletions(-)
drivers/net/ethernet/intel/e1000e/netdev.c +5 -6
@@ -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; } }
Signature · 1 line
1.7.4.4
Re: [PATCH 2/7] NET: intel/e1000e/netdev.c: fix 32-bit DMA mask handling
Quoted 25 lines · 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.
Quoted 4 lines
Signed-off-by Russell King <[email protected]>--- drivers/net/ethernet/intel/e1000e/netdev.c | 11 +++++------ 1 files changed, 5 insertions(+), 6 deletions(-)
Thanks Russell, I will add this to my queue.
PATCH 3/7 NET: intel/igb/igb_main.c: fix 32-bit DMA mask handling
In reply to Russell King - ARM LinuxThe 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.
--- drivers/net/ethernet/intel/igb/igb_main.c | 11 +++++------ 1 files changed, 5 insertions(+), 6 deletions(-)
drivers/net/ethernet/intel/igb/igb_main.c +5 -6
@@ -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; } }
Signature · 1 line
1.7.4.4
Re: [PATCH 3/7] NET: intel/igb/igb_main.c: fix 32-bit DMA mask handling
Quoted 25 lines · 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.
Quoted 4 lines
Signed-off-by Russell King <[email protected]>--- 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.
PATCH 4/7 NET: intel/igbvf/netdev.c: fix 32-bit DMA mask handling
In reply to Russell King - ARM LinuxThe 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.
--- drivers/net/ethernet/intel/igbvf/netdev.c | 11 +++++------ 1 files changed, 5 insertions(+), 6 deletions(-)
drivers/net/ethernet/intel/igbvf/netdev.c +5 -6
@@ -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; } }
Signature · 1 line
1.7.4.4
Re: [PATCH 4/7] NET: intel/igbvf/netdev.c: fix 32-bit DMA mask handling
Quoted 24 lines · 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.
Quoted 4 lines
Signed-off-by Russell King <[email protected]>--- drivers/net/ethernet/intel/igbvf/netdev.c | 11 +++++------ 1 files changed, 5 insertions(+), 6 deletions(-)
Thanks Russell, I will add this to my queue.
PATCH 5/7 NET: intel/ixgb/ixgb_main.c: fix 32-bit DMA mask handling
In reply to Russell King - ARM LinuxThe 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.
--- drivers/net/ethernet/intel/ixgb/ixgb_main.c | 9 ++++----- 1 files changed, 4 insertions(+), 5 deletions(-)
drivers/net/ethernet/intel/ixgb/ixgb_main.c +4 -5
@@ -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; } }
Signature · 1 line
1.7.4.4
Re: [PATCH 5/7] NET: intel/ixgb/ixgb_main.c: fix 32-bit DMA mask handling
Quoted 24 lines · 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.
Quoted 4 lines
Signed-off-by Russell King <[email protected]>--- 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.
PATCH 6/7 NET: intel/ixgbe/ixgbe_main.c: fix 32-bit DMA mask handling
In reply to Russell King - ARM LinuxThe 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.
--- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 11 +++++------ 1 files changed, 5 insertions(+), 6 deletions(-)
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +5 -6
@@ -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 ; }
Signature · 1 line
1.7.4.4
Re: [PATCH 6/7] NET: intel/ixgbe/ixgbe_main.c: fix 32-bit DMA mask handling
Quoted 23 lines · 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.
Quoted 4 lines
Signed-off-by Russell King <[email protected]>--- 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.
PATCH 7/7 NET: intel/ixgbevf/ixgbevf_main.c: fix 32-bit DMA mask handling
In reply to Russell King - ARM LinuxThe 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.
--- drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 11 +++++------ 1 files changed, 5 insertions(+), 6 deletions(-)
drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c +5 -6
@@ -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 ; }
Signature · 1 line
1.7.4.4
Re: [PATCH 7/7] NET: intel/ixgbevf/ixgbevf_main.c: fix 32-bit DMA mask handling
Quoted 23 lines · 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.
Quoted 4 lines
Signed-off-by Russell King <[email protected]>--- 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.
Quoted 9 lines · On Tue, 11 Jun 2013 00:08:49 +0100 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.
Quoted 8 lines
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;
}
Quoted 35 lines · Jesse Brandeburg
Quoted 9 lines · On Tue, 11 Jun 2013 00:08:49 +0100 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.
Quoted 8 lines
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?
(quoted diff) +5
-0+ 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 + */
Review
Quoted 10 lines
(quoted diff) +9
-0+ 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
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
Quoted 4 lines · On Tue, 11 Jun 2013 13:35:05 -0700 Russell King - ARM Linux…
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.
Quoted 8 lines
{ 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 todma_set_mask()though; and
how about dma_set_mask_and_coherent(...)
Quoted 5 lines
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
Quoted 8 lines · Jesse Brandeburg
Quoted 4 lines · On Tue, 11 Jun 2013 13:35:05 -0700 Russell King - ARM Linux…
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.
Quoted 10 lines
Quoted 8 lines
{ 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 todma_set_mask()though; andhow 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
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
Quoted 8 lines · Jesse Brandeburg
This is my proposed change to the intel drivers. Comments?
(quoted diff) +5
-0+ 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 + */
Review
Quoted 10 lines
(quoted diff) +9
-0+ 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
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
Quoted 25 lines · Russell King - ARM Linux
Quoted 8 lines · Jesse Brandeburg
This is my proposed change to the intel drivers. Comments?
(quoted diff) +5
-0+ 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 + */Review
Quoted 10 lines
(quoted diff) +9
-0+ 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.
Quoted 1 line
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.
Quoted 26 lines · Jeff Kirsher
Quoted 25 lines · Russell King - ARM Linux
Quoted 8 lines · Jesse Brandeburg
This is my proposed change to the intel drivers. Comments?
(quoted diff) +5
-0+ 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 + */Review
Quoted 10 lines
(quoted diff) +9
-0+ 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.
Quoted 7 lines
Quoted 1 line
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.