Hi Dave and Thomas,
please take a look at this series which fixes DMA API usage in the ioc3
ethernet driver. At least the first one is a nasty abuse of internal
APIs introduced in 5.4-rc which I'd prefer to be fixed before 5.4 final.
dma_direct_ is a low-level API that must never be used by drivers
directly. Switch to use the proper DMA API instead.
Fixes: ed870f6a7aa2 ("net: sgi: ioc3-eth: use dma-direct for dma allocations")
Signed-off-by: Christoph Hellwig <[email protected]>
---
drivers/net/ethernet/sgi/ioc3-eth.c | 25 +++++++++++--------------
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/drivers/net/ethernet/sgi/ioc3-eth.c b/drivers/net/ethernet/sgi/ioc3-eth.c
index deb636d653f3..477af82bf8a9 100644
--- a/drivers/net/ethernet/sgi/ioc3-eth.c
+++ b/drivers/net/ethernet/sgi/ioc3-eth.c
@@ -48,7 +48,7 @@
#include <linux/etherdevice.h>
#include <linux/ethtool.h>
#include <linux/skbuff.h>
-#include <linux/dma-direct.h>
+#include <linux/dma-mapping.h>
#include <net/ip.h>
@@ -1242,8 +1242,8 @@ static int ioc3_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
ioc3_stop(ip);
/* Allocate rx ring. 4kb = 512 entries, must be 4kb aligned */
- ip->rxr = dma_direct_alloc_pages(ip->dma_dev, RX_RING_SIZE,
- &ip->rxr_dma, GFP_ATOMIC, 0);
+ ip->rxr = dma_alloc_coherent(ip->dma_dev, RX_RING_SIZE, &ip->rxr_dma,
+ GFP_ATOMIC);
if (!ip->rxr) {
pr_err("ioc3-eth: rx ring allocation failed\n");
err = -ENOMEM;
@@ -1251,9 +1251,8 @@ static int ioc3_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
}
/* Allocate tx rings. 16kb = 128 bufs, must be 16kb aligned */
- ip->txr = dma_direct_alloc_pages(ip->dma_dev, TX_RING_SIZE,
- &ip->txr_dma,
- GFP_KERNEL | __GFP_ZERO, 0);
+ ip->txr = dma_alloc_coherent(ip->dma_dev, TX_RING_SIZE, &ip->txr_dma,
+ GFP_KERNEL | __GFP_ZERO);
if (!ip->txr) {
pr_err("ioc3-eth: tx ring allocation failed\n");
err = -ENOMEM;
@@ -1313,11 +1312,11 @@ static int ioc3_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
out_stop:
del_timer_sync(&ip->ioc3_timer);
if (ip->rxr)
- dma_direct_free_pages(ip->dma_dev, RX_RING_SIZE, ip->rxr,
- ip->rxr_dma, 0);
+ dma_free_coherent(ip->dma_dev, RX_RING_SIZE, ip->rxr,
+ ip->rxr_dma);
if (ip->txr)
- dma_direct_free_pages(ip->dma_dev, TX_RING_SIZE, ip->txr,
- ip->txr_dma, 0);
+ dma_free_coherent(ip->dma_dev, TX_RING_SIZE, ip->txr,
+ ip->txr_dma);
out_res:
pci_release_regions(pdev);
out_free:
@@ -1335,10 +1334,8 @@ static void ioc3_remove_one(struct pci_dev *pdev)
struct net_device *dev = pci_get_drvdata(pdev);
struct ioc3_private *ip = netdev_priv(dev);
- dma_direct_free_pages(ip->dma_dev, RX_RING_SIZE, ip->rxr,
- ip->rxr_dma, 0);
- dma_direct_free_pages(ip->dma_dev, TX_RING_SIZE, ip->txr,
- ip->txr_dma, 0);
+ dma_free_coherent(ip->dma_dev, RX_RING_SIZE, ip->rxr, ip->rxr_dma);
+ dma_free_coherent(ip->dma_dev, TX_RING_SIZE, ip->txr, ip->txr_dma);
unregister_netdev(dev);
del_timer_sync(&ip->ioc3_timer);
--
2.20.1
dma_alloc_coherent always zeroes memory, there is no need for
__GFP_ZERO. Also doing a GFP_ATOMIC allocation just before a GFP_KERNEL
one is clearly bogus.
Fixes: ed870f6a7aa2 ("net: sgi: ioc3-eth: use dma-direct for dma allocations")
Signed-off-by: Christoph Hellwig <[email protected]>
---
drivers/net/ethernet/sgi/ioc3-eth.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/sgi/ioc3-eth.c b/drivers/net/ethernet/sgi/ioc3-eth.c
index 477af82bf8a9..8a684d882e63 100644
--- a/drivers/net/ethernet/sgi/ioc3-eth.c
+++ b/drivers/net/ethernet/sgi/ioc3-eth.c
@@ -1243,7 +1243,7 @@ static int ioc3_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
/* Allocate rx ring. 4kb = 512 entries, must be 4kb aligned */
ip->rxr = dma_alloc_coherent(ip->dma_dev, RX_RING_SIZE, &ip->rxr_dma,
- GFP_ATOMIC);
+ GFP_KERNEL);
if (!ip->rxr) {
pr_err("ioc3-eth: rx ring allocation failed\n");
err = -ENOMEM;
@@ -1252,7 +1252,7 @@ static int ioc3_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
/* Allocate tx rings. 16kb = 128 bufs, must be 16kb aligned */
ip->txr = dma_alloc_coherent(ip->dma_dev, TX_RING_SIZE, &ip->txr_dma,
- GFP_KERNEL | __GFP_ZERO);
+ GFP_KERNEL);
if (!ip->txr) {
pr_err("ioc3-eth: tx ring allocation failed\n");
err = -ENOMEM;
--
2.20.1
Set NETIF_F_HIGHDMA together with the NETIF_F_IP_CSUM flag insted of
letting the second assignment overwrite it. Probably doesn't matter
in practice as none of the systems a IOC3 is usually found in has
highmem to start with.
Signed-off-by: Christoph Hellwig <[email protected]>
---
drivers/net/ethernet/sgi/ioc3-eth.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/sgi/ioc3-eth.c b/drivers/net/ethernet/sgi/ioc3-eth.c
index dc2e22652b55..1af68826810a 100644
--- a/drivers/net/ethernet/sgi/ioc3-eth.c
+++ b/drivers/net/ethernet/sgi/ioc3-eth.c
@@ -1192,8 +1192,6 @@ static int ioc3_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
goto out_disable;
}
- dev->features |= NETIF_F_HIGHDMA;
-
err = pci_request_regions(pdev, "ioc3");
if (err)
goto out_free;
@@ -1274,7 +1272,7 @@ static int ioc3_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
dev->netdev_ops = &ioc3_netdev_ops;
dev->ethtool_ops = &ioc3_ethtool_ops;
dev->hw_features = NETIF_F_IP_CSUM | NETIF_F_RXCSUM;
- dev->features = NETIF_F_IP_CSUM;
+ dev->features = NETIF_F_IP_CSUM | NETIF_F_HIGHDMA;
sw_physid1 = ioc3_mdio_read(dev, ip->mii.phy_id, MII_PHYSID1);
sw_physid2 = ioc3_mdio_read(dev, ip->mii.phy_id, MII_PHYSID2);
--
2.20.1
There is no need to fall back to a lower mask these days, the DMA mask
just communictes the hardware supported features.
Signed-off-by: Christoph Hellwig <[email protected]>
---
drivers/net/ethernet/sgi/ioc3-eth.c | 27 +++++++--------------------
1 file changed, 7 insertions(+), 20 deletions(-)
diff --git a/drivers/net/ethernet/sgi/ioc3-eth.c b/drivers/net/ethernet/sgi/ioc3-eth.c
index 8a684d882e63..dc2e22652b55 100644
--- a/drivers/net/ethernet/sgi/ioc3-eth.c
+++ b/drivers/net/ethernet/sgi/ioc3-eth.c
@@ -1173,26 +1173,14 @@ static int ioc3_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
struct ioc3 *ioc3;
unsigned long ioc3_base, ioc3_size;
u32 vendor, model, rev;
- int err, pci_using_dac;
+ int err;
/* Configure DMA attributes. */
- err = pci_set_dma_mask(pdev, DMA_BIT_MASK(64));
- if (!err) {
- pci_using_dac = 1;
- err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(64));
- if (err < 0) {
- pr_err("%s: Unable to obtain 64 bit DMA for consistent allocations\n",
- pci_name(pdev));
- goto out;
- }
- } else {
- err = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
- if (err) {
- pr_err("%s: No usable DMA configuration, aborting.\n",
- pci_name(pdev));
- goto out;
- }
- pci_using_dac = 0;
+ err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
+ if (err) {
+ pr_err("%s: No usable DMA configuration, aborting.\n",
+ pci_name(pdev));
+ goto out;
}
if (pci_enable_device(pdev))
@@ -1204,8 +1192,7 @@ static int ioc3_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
goto out_disable;
}
- if (pci_using_dac)
- dev->features |= NETIF_F_HIGHDMA;
+ dev->features |= NETIF_F_HIGHDMA;
err = pci_request_regions(pdev, "ioc3");
if (err)
--
2.20.1
On Wed, 30 Oct 2019 14:12:30 -0700
Christoph Hellwig <[email protected]> wrote:
> dma_direct_ is a low-level API that must never be used by drivers
> directly. Switch to use the proper DMA API instead.
is the 4kb/16kb alignment still guaranteed ? If not how is the way
to get such an alignment ?
Thomas.
--
SUSE Software Solutions Germany GmbH
HRB 36809 (AG N?rnberg)
Gesch?ftsf?hrer: Felix Imend?rffer
On Wed, Oct 30, 2019 at 11:05:49PM +0100, Thomas Bogendoerfer wrote:
> On Wed, 30 Oct 2019 14:12:30 -0700
> Christoph Hellwig <[email protected]> wrote:
>
> > dma_direct_ is a low-level API that must never be used by drivers
> > directly. Switch to use the proper DMA API instead.
>
> is the 4kb/16kb alignment still guaranteed ? If not how is the way
> to get such an alignment ?
The DMA API gives you page aligned memory. dma_direct doesn't give you
any gurantees as it is an internal API explicitly documented as not
for driver usage that can change at any time.
On Wed, 30 Oct 2019 23:38:18 +0100
Christoph Hellwig <[email protected]> wrote:
> On Wed, Oct 30, 2019 at 11:05:49PM +0100, Thomas Bogendoerfer wrote:
> > On Wed, 30 Oct 2019 14:12:30 -0700
> > Christoph Hellwig <[email protected]> wrote:
> >
> > > dma_direct_ is a low-level API that must never be used by drivers
> > > directly. Switch to use the proper DMA API instead.
> >
> > is the 4kb/16kb alignment still guaranteed ? If not how is the way
> > to get such an alignment ?
>
> The DMA API gives you page aligned memory. dma_direct doesn't give you
> any gurantees as it is an internal API explicitly documented as not
> for driver usage that can change at any time.
I didn't want to argue about that. What I'm interested in is a way how
to allocate dma memory, which is 16kB aligned, via the DMA API ?
Thomas.
--
SUSE Software Solutions Germany GmbH
HRB 36809 (AG N?rnberg)
Gesch?ftsf?hrer: Felix Imend?rffer
On Thu, Oct 31, 2019 at 09:54:30AM +0100, Thomas Bogendoerfer wrote:
> I didn't want to argue about that. What I'm interested in is a way how
> to allocate dma memory, which is 16kB aligned, via the DMA API ?
You can't.
On Thu, 31 Oct 2019 14:15:01 +0100
Christoph Hellwig <[email protected]> wrote:
> On Thu, Oct 31, 2019 at 09:54:30AM +0100, Thomas Bogendoerfer wrote:
> > I didn't want to argue about that. What I'm interested in is a way how
> > to allocate dma memory, which is 16kB aligned, via the DMA API ?
>
> You can't.
So then __get_free_pages() and dma_map_page() is the only way ?
BTW I've successful tested your patches on an 8 CPU Origin 2k.
Thomas.
--
SUSE Software Solutions Germany GmbH
HRB 36809 (AG N?rnberg)
Gesch?ftsf?hrer: Felix Imend?rffer
On Thu, Oct 31, 2019 at 04:18:17PM +0100, Thomas Bogendoerfer wrote:
> On Thu, 31 Oct 2019 14:15:01 +0100
> Christoph Hellwig <[email protected]> wrote:
>
> > On Thu, Oct 31, 2019 at 09:54:30AM +0100, Thomas Bogendoerfer wrote:
> > > I didn't want to argue about that. What I'm interested in is a way how
> > > to allocate dma memory, which is 16kB aligned, via the DMA API ?
> >
> > You can't.
>
> So then __get_free_pages() and dma_map_page() is the only way ?
Or dma_alloc_coherent + check alignment + allocate larger and align
yourself. In practice you'll always get alignmened memory at the
moment, but there is no API gurantee.
From: Thomas Bogendoerfer <[email protected]>
Date: Thu, 31 Oct 2019 16:18:17 +0100
> On Thu, 31 Oct 2019 14:15:01 +0100
> Christoph Hellwig <[email protected]> wrote:
>
>> On Thu, Oct 31, 2019 at 09:54:30AM +0100, Thomas Bogendoerfer wrote:
>> > I didn't want to argue about that. What I'm interested in is a way how
>> > to allocate dma memory, which is 16kB aligned, via the DMA API ?
>>
>> You can't.
>
> So then __get_free_pages() and dma_map_page() is the only way ?
>
> BTW I've successful tested your patches on an 8 CPU Origin 2k.
Nope, it is not the only way.
Allocate a (SIZE*2)-1 sized piece of DMA memory and align the
resulting CPU pointer and DMA address as needed.
From: Christoph Hellwig <[email protected]>
Date: Wed, 30 Oct 2019 14:12:29 -0700
> Hi Dave and Thomas,
>
> please take a look at this series which fixes DMA API usage in the ioc3
> ethernet driver. At least the first one is a nasty abuse of internal
> APIs introduced in 5.4-rc which I'd prefer to be fixed before 5.4 final.
Please add the alignment code for 16K or whatever they need and I'll apply
this series.
Thanks.
On Thu, Oct 31, 2019 at 02:13:34PM -0700, David Miller wrote:
> From: Christoph Hellwig <[email protected]>
> Date: Wed, 30 Oct 2019 14:12:29 -0700
>
> > Hi Dave and Thomas,
> >
> > please take a look at this series which fixes DMA API usage in the ioc3
> > ethernet driver. At least the first one is a nasty abuse of internal
> > APIs introduced in 5.4-rc which I'd prefer to be fixed before 5.4 final.
>
> Please add the alignment code for 16K or whatever they need and I'll apply
> this series.
I'll let Thomas add this as this series doesn't change anything in that
regard.