[RESEND,v2,52/53] mtd: nand: denali: use non-managed kmalloc() for DMA buffer

Message ID 1490228282-10805-26-git-send-email-yamada.masahiro@socionext.com
State New
Headers show
Series
  • Untitled series #858
Related show

Commit Message

Masahiro Yamada March 23, 2017, 12:18 a.m.
As Russell and Lars stated in the discussion [1], using
devm_k*alloc() with DMA is not a good idea.

Let's use kmalloc (not kzalloc because no need for zero-out).
Also, allocate the buffer as late as possible because it must be
freed for any error that follows.

[1] https://lkml.org/lkml/2017/3/8/693

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

Cc: Russell King <rmk+kernel@armlinux.org.uk>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Robin Murphy <robin.murphy@arm.com>
---

Changes in v2:
  - Newly added

 drivers/mtd/nand/denali.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

-- 
2.7.4


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Comments

Robin Murphy March 23, 2017, 11:33 a.m. | #1
On 23/03/17 00:18, Masahiro Yamada wrote:
> As Russell and Lars stated in the discussion [1], using

> devm_k*alloc() with DMA is not a good idea.

> 

> Let's use kmalloc (not kzalloc because no need for zero-out).

> Also, allocate the buffer as late as possible because it must be

> freed for any error that follows.


I still think the way the driver actually uses this buffer looks very
suspect - keeping it permanently mapped for bidirectional (AKA "I don't
know") streaming DMA across multiple operations doesn't add up. If the
device might access it at any time (or the driver simply doesn't want to
have to care) it should be a coherent allocation. Otherwise, it should
just be mapped/unmapped with the appropriate direction around each
operation.

Either way though, this patch at least makes things somewhat *less*
incorrect, so:

Acked-by: Robin Murphy <robin.murphy@arm.com>


> [1] https://lkml.org/lkml/2017/3/8/693

> 

> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

> Cc: Russell King <rmk+kernel@armlinux.org.uk>

> Cc: Lars-Peter Clausen <lars@metafoo.de>

> Cc: Robin Murphy <robin.murphy@arm.com>

> ---

> 

> Changes in v2:

>   - Newly added

> 

>  drivers/mtd/nand/denali.c | 25 ++++++++++++++++---------

>  1 file changed, 16 insertions(+), 9 deletions(-)

> 

> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c

> index 4900745..28622a9 100644

> --- a/drivers/mtd/nand/denali.c

> +++ b/drivers/mtd/nand/denali.c

> @@ -23,6 +23,7 @@

>  #include <linux/mutex.h>

>  #include <linux/mtd/mtd.h>

>  #include <linux/module.h>

> +#include <linux/slab.h>

>  

>  #include "denali.h"

>  

> @@ -1343,13 +1344,6 @@ int denali_init(struct denali_nand_info *denali)

>  	if (ret)

>  		goto disable_irq;

>  

> -	denali->buf = devm_kzalloc(denali->dev, mtd->writesize + mtd->oobsize,

> -				   GFP_KERNEL);

> -	if (!denali->buf) {

> -		ret = -ENOMEM;

> -		goto disable_irq;

> -	}

> -

>  	if (ioread32(denali->flash_reg + FEATURES) & FEATURES__DMA)

>  		denali->dma_avail = 1;

>  

> @@ -1460,17 +1454,29 @@ int denali_init(struct denali_nand_info *denali)

>  	if (ret)

>  		goto disable_irq;

>  

> +	/*

> +	 * This buffer may be DMA-mapped.  Do not use devm_kmalloc() because

> +	 * the memory allocated by devm_ is not cache line aligned.

> +	 */

> +	denali->buf = kmalloc(mtd->writesize + mtd->oobsize, GFP_KERNEL);

> +	if (!denali->buf) {

> +		ret = -ENOMEM;

> +		goto disable_irq;

> +	}

> +

>  	ret = nand_scan_tail(mtd);

>  	if (ret)

> -		goto disable_irq;

> +		goto free_buf;

>  

>  	ret = mtd_device_register(mtd, NULL, 0);

>  	if (ret) {

>  		dev_err(denali->dev, "Failed to register MTD: %d\n", ret);

> -		goto disable_irq;

> +		goto free_buf;

>  	}

>  	return 0;

>  

> +free_buf:

> +	kfree(denali->buf);

>  disable_irq:

>  	denali_disable_irq(denali);

>  

> @@ -1490,6 +1496,7 @@ void denali_remove(struct denali_nand_info *denali)

>  	int bufsize = mtd->writesize + mtd->oobsize;

>  

>  	nand_release(mtd);

> +	kfree(denali->buf);

>  	denali_disable_irq(denali);

>  	dma_unmap_single(denali->dev, denali->dma_addr, bufsize,

>  			 DMA_BIDIRECTIONAL);

> 



______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
Masahiro Yamada March 24, 2017, 1:41 a.m. | #2
Hi Robin,

> I still think the way the driver actually uses this buffer looks very

> suspect - keeping it permanently mapped for bidirectional (AKA "I don't

> know") streaming DMA across multiple operations doesn't add up. If the

> device might access it at any time (or the driver simply doesn't want to

> have to care) it should be a coherent allocation. Otherwise, it should

> just be mapped/unmapped with the appropriate direction around each

> operation.


This buffer is not permanently mapped for bidirectional any more.

I fixed it in 51/53:
http://patchwork.ozlabs.org/patch/742411/

When possible, the read/write callbacks
directly calls dma_map_single() for the buffer
passed from the core framework.
(either DMA_TO_DEVICE or DMA_FROM_DEVICE)

This will bypass the memcpy() to/from the internal buffer.



This driver internal buffer is still needed for raw read/write.
The Denali IP uses syndrome page layout.
So, the raw accessors must arrange the payload/ECC data layout.

This problem is addressed by 48/53:
http://patchwork.ozlabs.org/patch/742416/


I also examined the possibility for coherent allocation.
Even if I use dma_alloc_coherent, I need another bounce buffer anyway
for the syndrome page shuffling.  So, this way seems less efficient.


Masahiro

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
Robin Murphy March 24, 2017, 5:09 p.m. | #3
On 24/03/17 01:41, yamada.masahiro@socionext.com wrote:
> Hi Robin,

> 

>> I still think the way the driver actually uses this buffer looks very

>> suspect - keeping it permanently mapped for bidirectional (AKA "I don't

>> know") streaming DMA across multiple operations doesn't add up. If the

>> device might access it at any time (or the driver simply doesn't want to

>> have to care) it should be a coherent allocation. Otherwise, it should

>> just be mapped/unmapped with the appropriate direction around each

>> operation.

> 

> This buffer is not permanently mapped for bidirectional any more.

> 

> I fixed it in 51/53:

> http://patchwork.ozlabs.org/patch/742411/

> 

> When possible, the read/write callbacks

> directly calls dma_map_single() for the buffer

> passed from the core framework.

> (either DMA_TO_DEVICE or DMA_FROM_DEVICE)

> 

> This will bypass the memcpy() to/from the internal buffer.

> 

> 

> 

> This driver internal buffer is still needed for raw read/write.

> The Denali IP uses syndrome page layout.

> So, the raw accessors must arrange the payload/ECC data layout.

> 

> This problem is addressed by 48/53:

> http://patchwork.ozlabs.org/patch/742416/

> 

> 

> I also examined the possibility for coherent allocation.

> Even if I use dma_alloc_coherent, I need another bounce buffer anyway

> for the syndrome page shuffling.  So, this way seems less efficient.


Ah, so it's only a visibility issue at my end, then. Great stuff!

(on balance, I'm fine with not being sent the other 52 patches just for
the sake of this one!)

Thanks,
Robin.

> 

> 

> Masahiro

> 



______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Patch hide | download patch | download mbox

diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 4900745..28622a9 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -23,6 +23,7 @@ 
 #include <linux/mutex.h>
 #include <linux/mtd/mtd.h>
 #include <linux/module.h>
+#include <linux/slab.h>
 
 #include "denali.h"
 
@@ -1343,13 +1344,6 @@  int denali_init(struct denali_nand_info *denali)
 	if (ret)
 		goto disable_irq;
 
-	denali->buf = devm_kzalloc(denali->dev, mtd->writesize + mtd->oobsize,
-				   GFP_KERNEL);
-	if (!denali->buf) {
-		ret = -ENOMEM;
-		goto disable_irq;
-	}
-
 	if (ioread32(denali->flash_reg + FEATURES) & FEATURES__DMA)
 		denali->dma_avail = 1;
 
@@ -1460,17 +1454,29 @@  int denali_init(struct denali_nand_info *denali)
 	if (ret)
 		goto disable_irq;
 
+	/*
+	 * This buffer may be DMA-mapped.  Do not use devm_kmalloc() because
+	 * the memory allocated by devm_ is not cache line aligned.
+	 */
+	denali->buf = kmalloc(mtd->writesize + mtd->oobsize, GFP_KERNEL);
+	if (!denali->buf) {
+		ret = -ENOMEM;
+		goto disable_irq;
+	}
+
 	ret = nand_scan_tail(mtd);
 	if (ret)
-		goto disable_irq;
+		goto free_buf;
 
 	ret = mtd_device_register(mtd, NULL, 0);
 	if (ret) {
 		dev_err(denali->dev, "Failed to register MTD: %d\n", ret);
-		goto disable_irq;
+		goto free_buf;
 	}
 	return 0;
 
+free_buf:
+	kfree(denali->buf);
 disable_irq:
 	denali_disable_irq(denali);
 
@@ -1490,6 +1496,7 @@  void denali_remove(struct denali_nand_info *denali)
 	int bufsize = mtd->writesize + mtd->oobsize;
 
 	nand_release(mtd);
+	kfree(denali->buf);
 	denali_disable_irq(denali);
 	dma_unmap_single(denali->dev, denali->dma_addr, bufsize,
 			 DMA_BIDIRECTIONAL);