Re: [Nouveau] [PATCH 06/28] lib82596: move DMA allocation into the callers of i82596_probe

[Thomas Bogendoerfer]

On Wed, Aug 19, 2020 at 08:55:33AM +0200, Christoph Hellwig wrote:
> This allows us to get rid of the LIB82596_DMA_ATTR defined and prepare
> for untangling the coherent vs non-coherent DMA allocation API.
> 
> Signed-off-by: Christoph Hellwig 
> ---
>  drivers/net/ethernet/i825xx/lasi_82596.c | 24 ++--
>  drivers/net/ethernet/i825xx/lib82596.c   | 36 
>  drivers/net/ethernet/i825xx/sni_82596.c  | 19 +
>  3 files changed, 40 insertions(+), 39 deletions(-)
> 
> [...]
> diff --git a/drivers/net/ethernet/i825xx/sni_82596.c 
> b/drivers/net/ethernet/i825xx/sni_82596.c
> index 22f5887578b2bd..e80e790ffbd4d4 100644
> --- a/drivers/net/ethernet/i825xx/sni_82596.c
> +++ b/drivers/net/ethernet/i825xx/sni_82596.c
> @@ -24,8 +24,6 @@
>  
>  static const char sni_82596_string[] = "snirm_82596";
>  
> -#define LIB82596_DMA_ATTR0
> -
>  #define DMA_WBACK(priv, addr, len) do { } while (0)
>  #define DMA_INV(priv, addr, len)   do { } while (0)
>  #define DMA_WBACK_INV(priv, addr, len) do { } while (0)
> @@ -134,10 +132,19 @@ static int sni_82596_probe(struct platform_device *dev)
>   lp->ca = ca_addr;
>   lp->mpu_port = mpu_addr;
>  
> + lp->dma = dma_alloc_coherent(dev->dev.parent, sizeof(struct i596_dma),
> +  >dma_addr, GFP_KERNEL);

this needs to use >dev as device argument otherwise I get a

WARNING: CPU: 0 PID: 1 at linux/kernel/dma/mapping.c:416 
dma_alloc_attrs+0x64/0x98

(coherent_dma_mask is set correctly).

dev->dev.parent was correct when going from netdevice to underlying device,
but now allocation is done via platform_device probe. I wonder why this works
for parisc.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.[ RFC1925, 2.3 ]
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[Nouveau] [PATCH 06/28] lib82596: move DMA allocation into the callers of i82596_probe

[Christoph Hellwig]

This allows us to get rid of the LIB82596_DMA_ATTR defined and prepare
for untangling the coherent vs non-coherent DMA allocation API.

Signed-off-by: Christoph Hellwig 
---
 drivers/net/ethernet/i825xx/lasi_82596.c | 24 ++--
 drivers/net/ethernet/i825xx/lib82596.c   | 36 
 drivers/net/ethernet/i825xx/sni_82596.c  | 19 +
 3 files changed, 40 insertions(+), 39 deletions(-)

diff --git a/drivers/net/ethernet/i825xx/lasi_82596.c 
b/drivers/net/ethernet/i825xx/lasi_82596.c
index aec7e98bcc853a..8c5ab9b7553e75 100644
--- a/drivers/net/ethernet/i825xx/lasi_82596.c
+++ b/drivers/net/ethernet/i825xx/lasi_82596.c
@@ -96,8 +96,6 @@
 
 #define OPT_SWAP_PORT  0x0001  /* Need to wordswp on the MPU port */
 
-#define LIB82596_DMA_ATTR  DMA_ATTR_NON_CONSISTENT
-
 #define DMA_WBACK(ndev, addr, len) \
do { dma_cache_sync((ndev)->dev.parent, (void *)addr, len, 
DMA_TO_DEVICE); } while (0)
 
@@ -155,7 +153,7 @@ lan_init_chip(struct parisc_device *dev)
 {
struct  net_device *netdevice;
struct i596_private *lp;
-   int retval;
+   int retval = -ENOMEM;
int i;
 
if (!dev->irq) {
@@ -186,12 +184,22 @@ lan_init_chip(struct parisc_device *dev)
 
lp = netdev_priv(netdevice);
lp->options = dev->id.sversion == 0x72 ? OPT_SWAP_PORT : 0;
+   lp->dma = dma_alloc_attrs(dev->dev.parent, sizeof(struct i596_dma),
+ >dma_addr, GFP_KERNEL,
+ DMA_ATTR_NON_CONSISTENT);
+   if (!lp->dma)
+   goto out_free_netdev;
 
retval = i82596_probe(netdevice);
-   if (retval) {
-   free_netdev(netdevice);
-   return -ENODEV;
-   }
+   if (retval)
+   goto out_free_dma;
+   return 0;
+
+out_free_dma:
+   dma_free_attrs(dev->dev.parent, sizeof(struct i596_dma),
+  lp->dma, lp->dma_addr, DMA_ATTR_NON_CONSISTENT);
+out_free_netdev:
+   free_netdev(netdevice);
return retval;
 }
 
@@ -202,7 +210,7 @@ static int __exit lan_remove_chip(struct parisc_device 
*pdev)
 
unregister_netdev (dev);
dma_free_attrs(>dev, sizeof(struct i596_private), lp->dma,
-  lp->dma_addr, LIB82596_DMA_ATTR);
+  lp->dma_addr, DMA_ATTR_NON_CONSISTENT);
free_netdev (dev);
return 0;
 }
diff --git a/drivers/net/ethernet/i825xx/lib82596.c 
b/drivers/net/ethernet/i825xx/lib82596.c
index b03757e169e475..b4e4b3eb5758b5 100644
--- a/drivers/net/ethernet/i825xx/lib82596.c
+++ b/drivers/net/ethernet/i825xx/lib82596.c
@@ -1047,9 +1047,8 @@ static const struct net_device_ops i596_netdev_ops = {
 
 static int i82596_probe(struct net_device *dev)
 {
-   int i;
struct i596_private *lp = netdev_priv(dev);
-   struct i596_dma *dma;
+   int ret;
 
/* This lot is ensure things have been cache line aligned. */
BUILD_BUG_ON(sizeof(struct i596_rfd) != 32);
@@ -1063,41 +1062,28 @@ static int i82596_probe(struct net_device *dev)
if (!dev->base_addr || !dev->irq)
return -ENODEV;
 
-   dma = dma_alloc_attrs(dev->dev.parent, sizeof(struct i596_dma),
- >dma_addr, GFP_KERNEL,
- LIB82596_DMA_ATTR);
-   if (!dma) {
-   printk(KERN_ERR "%s: Couldn't get shared memory\n", __FILE__);
-   return -ENOMEM;
-   }
-
dev->netdev_ops = _netdev_ops;
dev->watchdog_timeo = TX_TIMEOUT;
 
-   memset(dma, 0, sizeof(struct i596_dma));
-   lp->dma = dma;
-
-   dma->scb.command = 0;
-   dma->scb.cmd = I596_NULL;
-   dma->scb.rfd = I596_NULL;
+   memset(lp->dma, 0, sizeof(struct i596_dma));
+   lp->dma->scb.command = 0;
+   lp->dma->scb.cmd = I596_NULL;
+   lp->dma->scb.rfd = I596_NULL;
spin_lock_init(>lock);
 
-   DMA_WBACK_INV(dev, dma, sizeof(struct i596_dma));
+   DMA_WBACK_INV(dev, lp->dma, sizeof(struct i596_dma));
 
-   i = register_netdev(dev);
-   if (i) {
-   dma_free_attrs(dev->dev.parent, sizeof(struct i596_dma),
-  dma, lp->dma_addr, LIB82596_DMA_ATTR);
-   return i;
-   }
+   ret = register_netdev(dev);
+   if (ret)
+   return ret;
 
DEB(DEB_PROBE, printk(KERN_INFO "%s: 82596 at %#3lx, %pM IRQ %d.\n",
  dev->name, dev->base_addr, dev->dev_addr,
  dev->irq));
DEB(DEB_INIT, printk(KERN_INFO
 "%s: dma at 0x%p (%d bytes), lp->scb at 0x%p\n",
-dev->name, dma, (int)sizeof(struct i596_dma),
->scb));
+dev->name, lp->dma, (int)sizeof(struct i596_dma),
+>dma->scb));
 
return 0;
 }
diff --git a/drivers/net/ethernet/i825xx/sni_82596.c