| Message ID | 1490228282-10805-26-git-send-email-yamada.masahiro@socionext.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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/
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/
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/
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);
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/