LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v2] mtd: onenand: omap2: Disable DMA for HIGHMEM buffers
@ 2018-04-16  6:52 Ladislav Michl
  2018-04-16 11:34 ` Peter Ujfalusi
  2018-04-20 20:01 ` Boris Brezillon
  0 siblings, 2 replies; 6+ messages in thread
From: Ladislav Michl @ 2018-04-16  6:52 UTC (permalink / raw)
  To: linux-kernel, linux-mtd
  Cc: Boris Brezillon, Peter Ujfalusi, Roger Quadros, Aaro Koskinen,
	Tony Lindgren, H. Nikolaus Schaller, Andreas Kemnade

dma_map_single doesn't get the proper DMA address for vmalloced area,
so disable DMA in this case.

Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
Reported-by: "H. Nikolaus Schaller" <hns@goldelico.com>
Tested-by: "H. Nikolaus Schaller" <hns@goldelico.com>
---
 Changes:
 -v2: Added Tested-by tag, based on v4.17-rc1 (no change in patch itself)

 drivers/mtd/nand/onenand/omap2.c | 105 +++++++++++--------------------
 1 file changed, 38 insertions(+), 67 deletions(-)

diff --git a/drivers/mtd/nand/onenand/omap2.c b/drivers/mtd/nand/onenand/omap2.c
index 9c159f0dd9a6..321137158ff3 100644
--- a/drivers/mtd/nand/onenand/omap2.c
+++ b/drivers/mtd/nand/onenand/omap2.c
@@ -375,56 +375,42 @@ static int omap2_onenand_read_bufferram(struct mtd_info *mtd, int area,
 {
 	struct omap2_onenand *c = container_of(mtd, struct omap2_onenand, mtd);
 	struct onenand_chip *this = mtd->priv;
-	dma_addr_t dma_src, dma_dst;
-	int bram_offset;
+	struct device *dev = &c->pdev->dev;
 	void *buf = (void *)buffer;
+	dma_addr_t dma_src, dma_dst;
+	int bram_offset, err;
 	size_t xtra;
-	int ret;
 
 	bram_offset = omap2_onenand_bufferram_offset(mtd, area) + area + offset;
-	if (bram_offset & 3 || (size_t)buf & 3 || count < 384)
-		goto out_copy;
-
-	/* panic_write() may be in an interrupt context */
-	if (in_interrupt() || oops_in_progress)
+	/*
+	 * If the buffer address is not DMA-able, len is not long enough to make
+	 * DMA transfers profitable or panic_write() may be in an interrupt
+	 * context fallback to PIO mode.
+	 */
+	if (!virt_addr_valid(buf) || bram_offset & 3 || (size_t)buf & 3 ||
+	    count < 384 || in_interrupt() || oops_in_progress )
 		goto out_copy;
 
-	if (buf >= high_memory) {
-		struct page *p1;
-
-		if (((size_t)buf & PAGE_MASK) !=
-		    ((size_t)(buf + count - 1) & PAGE_MASK))
-			goto out_copy;
-		p1 = vmalloc_to_page(buf);
-		if (!p1)
-			goto out_copy;
-		buf = page_address(p1) + ((size_t)buf & ~PAGE_MASK);
-	}
-
 	xtra = count & 3;
 	if (xtra) {
 		count -= xtra;
 		memcpy(buf + count, this->base + bram_offset + count, xtra);
 	}
 
+	dma_dst = dma_map_single(dev, buf, count, DMA_FROM_DEVICE);
 	dma_src = c->phys_base + bram_offset;
-	dma_dst = dma_map_single(&c->pdev->dev, buf, count, DMA_FROM_DEVICE);
-	if (dma_mapping_error(&c->pdev->dev, dma_dst)) {
-		dev_err(&c->pdev->dev,
-			"Couldn't DMA map a %d byte buffer\n",
-			count);
-		goto out_copy;
-	}
 
-	ret = omap2_onenand_dma_transfer(c, dma_src, dma_dst, count);
-	dma_unmap_single(&c->pdev->dev, dma_dst, count, DMA_FROM_DEVICE);
-
-	if (ret) {
-		dev_err(&c->pdev->dev, "timeout waiting for DMA\n");
+	if (dma_mapping_error(dev, dma_dst)) {
+		dev_err(dev, "Couldn't DMA map a %d byte buffer\n", count);
 		goto out_copy;
 	}
 
-	return 0;
+	err = omap2_onenand_dma_transfer(c, dma_src, dma_dst, count);
+	dma_unmap_single(dev, dma_dst, count, DMA_FROM_DEVICE);
+	if (!err)
+		return 0;
+
+	dev_err(dev, "timeout waiting for DMA\n");
 
 out_copy:
 	memcpy(buf, this->base + bram_offset, count);
@@ -437,49 +423,34 @@ static int omap2_onenand_write_bufferram(struct mtd_info *mtd, int area,
 {
 	struct omap2_onenand *c = container_of(mtd, struct omap2_onenand, mtd);
 	struct onenand_chip *this = mtd->priv;
-	dma_addr_t dma_src, dma_dst;
-	int bram_offset;
+	struct device *dev = &c->pdev->dev;
 	void *buf = (void *)buffer;
-	int ret;
+	dma_addr_t dma_src, dma_dst;
+	int bram_offset, err;
 
 	bram_offset = omap2_onenand_bufferram_offset(mtd, area) + area + offset;
-	if (bram_offset & 3 || (size_t)buf & 3 || count < 384)
-		goto out_copy;
-
-	/* panic_write() may be in an interrupt context */
-	if (in_interrupt() || oops_in_progress)
+	/*
+	 * If the buffer address is not DMA-able, len is not long enough to make
+	 * DMA transfers profitable or panic_write() may be in an interrupt
+	 * context fallback to PIO mode.
+	 */
+	if (!virt_addr_valid(buf) || bram_offset & 3 || (size_t)buf & 3 ||
+	    count < 384 || in_interrupt() || oops_in_progress )
 		goto out_copy;
 
-	if (buf >= high_memory) {
-		struct page *p1;
-
-		if (((size_t)buf & PAGE_MASK) !=
-		    ((size_t)(buf + count - 1) & PAGE_MASK))
-			goto out_copy;
-		p1 = vmalloc_to_page(buf);
-		if (!p1)
-			goto out_copy;
-		buf = page_address(p1) + ((size_t)buf & ~PAGE_MASK);
-	}
-
-	dma_src = dma_map_single(&c->pdev->dev, buf, count, DMA_TO_DEVICE);
+	dma_src = dma_map_single(dev, buf, count, DMA_TO_DEVICE);
 	dma_dst = c->phys_base + bram_offset;
-	if (dma_mapping_error(&c->pdev->dev, dma_src)) {
-		dev_err(&c->pdev->dev,
-			"Couldn't DMA map a %d byte buffer\n",
-			count);
-		return -1;
-	}
-
-	ret = omap2_onenand_dma_transfer(c, dma_src, dma_dst, count);
-	dma_unmap_single(&c->pdev->dev, dma_src, count, DMA_TO_DEVICE);
-
-	if (ret) {
-		dev_err(&c->pdev->dev, "timeout waiting for DMA\n");
+	if (dma_mapping_error(dev, dma_src)) {
+		dev_err(dev, "Couldn't DMA map a %d byte buffer\n", count);
 		goto out_copy;
 	}
 
-	return 0;
+	err = omap2_onenand_dma_transfer(c, dma_src, dma_dst, count);
+	dma_unmap_page(dev, dma_src, count, DMA_TO_DEVICE);
+	if (!err)
+		return 0;
+
+	dev_err(dev, "timeout waiting for DMA\n");
 
 out_copy:
 	memcpy(this->base + bram_offset, buf, count);
-- 
2.17.0

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] mtd: onenand: omap2: Disable DMA for HIGHMEM buffers
  2018-04-16  6:52 [PATCH v2] mtd: onenand: omap2: Disable DMA for HIGHMEM buffers Ladislav Michl
@ 2018-04-16 11:34 ` Peter Ujfalusi
  2018-04-16 13:32   ` Ladislav Michl
  2018-04-20 20:01 ` Boris Brezillon
  1 sibling, 1 reply; 6+ messages in thread
From: Peter Ujfalusi @ 2018-04-16 11:34 UTC (permalink / raw)
  To: Ladislav Michl, linux-kernel, linux-mtd
  Cc: Boris Brezillon, Roger Quadros, Aaro Koskinen, Tony Lindgren,
	H. Nikolaus Schaller, Andreas Kemnade



On 2018-04-16 09:52, Ladislav Michl wrote:
> dma_map_single doesn't get the proper DMA address for vmalloced area,

Which is not a big surprise as vmalloc will allocate contiguous virtual
memory (which might corresponds to non-contiguous physical memory). Even
if you somehow get the physical address of the start of the vmalloced
buffer, you don't really know how long that chunk is and where the
buffer continues in physical memory.

Creating sg_list of the vmalloced buffer should be possible also by
walking the virt memory and get the pages with vmalloc_to_page().
I don't think there is a generic vmalloc_to_sg(), one can be implemented.

> so disable DMA in this case.
> 
> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> Reported-by: "H. Nikolaus Schaller" <hns@goldelico.com>
> Tested-by: "H. Nikolaus Schaller" <hns@goldelico.com>
> ---
>  Changes:
>  -v2: Added Tested-by tag, based on v4.17-rc1 (no change in patch itself)
> 
>  drivers/mtd/nand/onenand/omap2.c | 105 +++++++++++--------------------
>  1 file changed, 38 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/mtd/nand/onenand/omap2.c b/drivers/mtd/nand/onenand/omap2.c
> index 9c159f0dd9a6..321137158ff3 100644
> --- a/drivers/mtd/nand/onenand/omap2.c
> +++ b/drivers/mtd/nand/onenand/omap2.c
> @@ -375,56 +375,42 @@ static int omap2_onenand_read_bufferram(struct mtd_info *mtd, int area,
>  {
>  	struct omap2_onenand *c = container_of(mtd, struct omap2_onenand, mtd);
>  	struct onenand_chip *this = mtd->priv;
> -	dma_addr_t dma_src, dma_dst;
> -	int bram_offset;
> +	struct device *dev = &c->pdev->dev;
>  	void *buf = (void *)buffer;
> +	dma_addr_t dma_src, dma_dst;
> +	int bram_offset, err;
>  	size_t xtra;
> -	int ret;
>  
>  	bram_offset = omap2_onenand_bufferram_offset(mtd, area) + area + offset;
> -	if (bram_offset & 3 || (size_t)buf & 3 || count < 384)
> -		goto out_copy;
> -
> -	/* panic_write() may be in an interrupt context */
> -	if (in_interrupt() || oops_in_progress)
> +	/*
> +	 * If the buffer address is not DMA-able, len is not long enough to make
> +	 * DMA transfers profitable or panic_write() may be in an interrupt
> +	 * context fallback to PIO mode.
> +	 */
> +	if (!virt_addr_valid(buf) || bram_offset & 3 || (size_t)buf & 3 ||
> +	    count < 384 || in_interrupt() || oops_in_progress )
>  		goto out_copy;
>  
> -	if (buf >= high_memory) {
> -		struct page *p1;
> -
> -		if (((size_t)buf & PAGE_MASK) !=
> -		    ((size_t)(buf + count - 1) & PAGE_MASK))
> -			goto out_copy;
> -		p1 = vmalloc_to_page(buf);
> -		if (!p1)
> -			goto out_copy;
> -		buf = page_address(p1) + ((size_t)buf & ~PAGE_MASK);
> -	}
> -
>  	xtra = count & 3;
>  	if (xtra) {
>  		count -= xtra;
>  		memcpy(buf + count, this->base + bram_offset + count, xtra);
>  	}
>  
> +	dma_dst = dma_map_single(dev, buf, count, DMA_FROM_DEVICE);
>  	dma_src = c->phys_base + bram_offset;
> -	dma_dst = dma_map_single(&c->pdev->dev, buf, count, DMA_FROM_DEVICE);
> -	if (dma_mapping_error(&c->pdev->dev, dma_dst)) {
> -		dev_err(&c->pdev->dev,
> -			"Couldn't DMA map a %d byte buffer\n",
> -			count);
> -		goto out_copy;
> -	}
>  
> -	ret = omap2_onenand_dma_transfer(c, dma_src, dma_dst, count);
> -	dma_unmap_single(&c->pdev->dev, dma_dst, count, DMA_FROM_DEVICE);
> -
> -	if (ret) {
> -		dev_err(&c->pdev->dev, "timeout waiting for DMA\n");
> +	if (dma_mapping_error(dev, dma_dst)) {
> +		dev_err(dev, "Couldn't DMA map a %d byte buffer\n", count);
>  		goto out_copy;
>  	}
>  
> -	return 0;
> +	err = omap2_onenand_dma_transfer(c, dma_src, dma_dst, count);
> +	dma_unmap_single(dev, dma_dst, count, DMA_FROM_DEVICE);
> +	if (!err)
> +		return 0;
> +
> +	dev_err(dev, "timeout waiting for DMA\n");
>  
>  out_copy:
>  	memcpy(buf, this->base + bram_offset, count);
> @@ -437,49 +423,34 @@ static int omap2_onenand_write_bufferram(struct mtd_info *mtd, int area,
>  {
>  	struct omap2_onenand *c = container_of(mtd, struct omap2_onenand, mtd);
>  	struct onenand_chip *this = mtd->priv;
> -	dma_addr_t dma_src, dma_dst;
> -	int bram_offset;
> +	struct device *dev = &c->pdev->dev;
>  	void *buf = (void *)buffer;
> -	int ret;
> +	dma_addr_t dma_src, dma_dst;
> +	int bram_offset, err;
>  
>  	bram_offset = omap2_onenand_bufferram_offset(mtd, area) + area + offset;
> -	if (bram_offset & 3 || (size_t)buf & 3 || count < 384)
> -		goto out_copy;
> -
> -	/* panic_write() may be in an interrupt context */
> -	if (in_interrupt() || oops_in_progress)
> +	/*
> +	 * If the buffer address is not DMA-able, len is not long enough to make
> +	 * DMA transfers profitable or panic_write() may be in an interrupt
> +	 * context fallback to PIO mode.
> +	 */
> +	if (!virt_addr_valid(buf) || bram_offset & 3 || (size_t)buf & 3 ||
> +	    count < 384 || in_interrupt() || oops_in_progress )
>  		goto out_copy;
>  
> -	if (buf >= high_memory) {
> -		struct page *p1;
> -
> -		if (((size_t)buf & PAGE_MASK) !=
> -		    ((size_t)(buf + count - 1) & PAGE_MASK))
> -			goto out_copy;
> -		p1 = vmalloc_to_page(buf);
> -		if (!p1)
> -			goto out_copy;
> -		buf = page_address(p1) + ((size_t)buf & ~PAGE_MASK);
> -	}
> -
> -	dma_src = dma_map_single(&c->pdev->dev, buf, count, DMA_TO_DEVICE);
> +	dma_src = dma_map_single(dev, buf, count, DMA_TO_DEVICE);
>  	dma_dst = c->phys_base + bram_offset;
> -	if (dma_mapping_error(&c->pdev->dev, dma_src)) {
> -		dev_err(&c->pdev->dev,
> -			"Couldn't DMA map a %d byte buffer\n",
> -			count);
> -		return -1;
> -	}
> -
> -	ret = omap2_onenand_dma_transfer(c, dma_src, dma_dst, count);
> -	dma_unmap_single(&c->pdev->dev, dma_src, count, DMA_TO_DEVICE);
> -
> -	if (ret) {
> -		dev_err(&c->pdev->dev, "timeout waiting for DMA\n");
> +	if (dma_mapping_error(dev, dma_src)) {
> +		dev_err(dev, "Couldn't DMA map a %d byte buffer\n", count);
>  		goto out_copy;
>  	}
>  
> -	return 0;
> +	err = omap2_onenand_dma_transfer(c, dma_src, dma_dst, count);
> +	dma_unmap_page(dev, dma_src, count, DMA_TO_DEVICE);
> +	if (!err)
> +		return 0;
> +
> +	dev_err(dev, "timeout waiting for DMA\n");
>  
>  out_copy:
>  	memcpy(this->base + bram_offset, buf, count);
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] mtd: onenand: omap2: Disable DMA for HIGHMEM buffers
  2018-04-16 11:34 ` Peter Ujfalusi
@ 2018-04-16 13:32   ` Ladislav Michl
  0 siblings, 0 replies; 6+ messages in thread
From: Ladislav Michl @ 2018-04-16 13:32 UTC (permalink / raw)
  To: Peter Ujfalusi
  Cc: linux-kernel, linux-mtd, Boris Brezillon, Roger Quadros,
	Aaro Koskinen, Tony Lindgren, H. Nikolaus Schaller,
	Andreas Kemnade

Hi Péter,

On Mon, Apr 16, 2018 at 02:34:54PM +0300, Peter Ujfalusi wrote:
> On 2018-04-16 09:52, Ladislav Michl wrote:
> > dma_map_single doesn't get the proper DMA address for vmalloced area,
> 
> Which is not a big surprise as vmalloc will allocate contiguous virtual
> memory (which might corresponds to non-contiguous physical memory). Even
> if you somehow get the physical address of the start of the vmalloced
> buffer, you don't really know how long that chunk is and where the
> buffer continues in physical memory.
> 
> Creating sg_list of the vmalloced buffer should be possible also by
> walking the virt memory and get the pages with vmalloc_to_page().
> I don't think there is a generic vmalloc_to_sg(), one can be implemented.

Please see previous bugreport here:
https://marc.info/?l=linux-omap&m=152337752611812&w=2

Unfortunately issue was noticed after v4.16 come out and v4.17-rc1 was
about to released. Thus safe change was introduced.

Best regards,
	ladis

> > so disable DMA in this case.
> > 
> > Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> > Reported-by: "H. Nikolaus Schaller" <hns@goldelico.com>
> > Tested-by: "H. Nikolaus Schaller" <hns@goldelico.com>
> > ---
> >  Changes:
> >  -v2: Added Tested-by tag, based on v4.17-rc1 (no change in patch itself)
> > 
> >  drivers/mtd/nand/onenand/omap2.c | 105 +++++++++++--------------------
> >  1 file changed, 38 insertions(+), 67 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/onenand/omap2.c b/drivers/mtd/nand/onenand/omap2.c
> > index 9c159f0dd9a6..321137158ff3 100644
> > --- a/drivers/mtd/nand/onenand/omap2.c
> > +++ b/drivers/mtd/nand/onenand/omap2.c
> > @@ -375,56 +375,42 @@ static int omap2_onenand_read_bufferram(struct mtd_info *mtd, int area,
> >  {
> >  	struct omap2_onenand *c = container_of(mtd, struct omap2_onenand, mtd);
> >  	struct onenand_chip *this = mtd->priv;
> > -	dma_addr_t dma_src, dma_dst;
> > -	int bram_offset;
> > +	struct device *dev = &c->pdev->dev;
> >  	void *buf = (void *)buffer;
> > +	dma_addr_t dma_src, dma_dst;
> > +	int bram_offset, err;
> >  	size_t xtra;
> > -	int ret;
> >  
> >  	bram_offset = omap2_onenand_bufferram_offset(mtd, area) + area + offset;
> > -	if (bram_offset & 3 || (size_t)buf & 3 || count < 384)
> > -		goto out_copy;
> > -
> > -	/* panic_write() may be in an interrupt context */
> > -	if (in_interrupt() || oops_in_progress)
> > +	/*
> > +	 * If the buffer address is not DMA-able, len is not long enough to make
> > +	 * DMA transfers profitable or panic_write() may be in an interrupt
> > +	 * context fallback to PIO mode.
> > +	 */
> > +	if (!virt_addr_valid(buf) || bram_offset & 3 || (size_t)buf & 3 ||
> > +	    count < 384 || in_interrupt() || oops_in_progress )
> >  		goto out_copy;
> >  
> > -	if (buf >= high_memory) {
> > -		struct page *p1;
> > -
> > -		if (((size_t)buf & PAGE_MASK) !=
> > -		    ((size_t)(buf + count - 1) & PAGE_MASK))
> > -			goto out_copy;
> > -		p1 = vmalloc_to_page(buf);
> > -		if (!p1)
> > -			goto out_copy;
> > -		buf = page_address(p1) + ((size_t)buf & ~PAGE_MASK);
> > -	}
> > -
> >  	xtra = count & 3;
> >  	if (xtra) {
> >  		count -= xtra;
> >  		memcpy(buf + count, this->base + bram_offset + count, xtra);
> >  	}
> >  
> > +	dma_dst = dma_map_single(dev, buf, count, DMA_FROM_DEVICE);
> >  	dma_src = c->phys_base + bram_offset;
> > -	dma_dst = dma_map_single(&c->pdev->dev, buf, count, DMA_FROM_DEVICE);
> > -	if (dma_mapping_error(&c->pdev->dev, dma_dst)) {
> > -		dev_err(&c->pdev->dev,
> > -			"Couldn't DMA map a %d byte buffer\n",
> > -			count);
> > -		goto out_copy;
> > -	}
> >  
> > -	ret = omap2_onenand_dma_transfer(c, dma_src, dma_dst, count);
> > -	dma_unmap_single(&c->pdev->dev, dma_dst, count, DMA_FROM_DEVICE);
> > -
> > -	if (ret) {
> > -		dev_err(&c->pdev->dev, "timeout waiting for DMA\n");
> > +	if (dma_mapping_error(dev, dma_dst)) {
> > +		dev_err(dev, "Couldn't DMA map a %d byte buffer\n", count);
> >  		goto out_copy;
> >  	}
> >  
> > -	return 0;
> > +	err = omap2_onenand_dma_transfer(c, dma_src, dma_dst, count);
> > +	dma_unmap_single(dev, dma_dst, count, DMA_FROM_DEVICE);
> > +	if (!err)
> > +		return 0;
> > +
> > +	dev_err(dev, "timeout waiting for DMA\n");
> >  
> >  out_copy:
> >  	memcpy(buf, this->base + bram_offset, count);
> > @@ -437,49 +423,34 @@ static int omap2_onenand_write_bufferram(struct mtd_info *mtd, int area,
> >  {
> >  	struct omap2_onenand *c = container_of(mtd, struct omap2_onenand, mtd);
> >  	struct onenand_chip *this = mtd->priv;
> > -	dma_addr_t dma_src, dma_dst;
> > -	int bram_offset;
> > +	struct device *dev = &c->pdev->dev;
> >  	void *buf = (void *)buffer;
> > -	int ret;
> > +	dma_addr_t dma_src, dma_dst;
> > +	int bram_offset, err;
> >  
> >  	bram_offset = omap2_onenand_bufferram_offset(mtd, area) + area + offset;
> > -	if (bram_offset & 3 || (size_t)buf & 3 || count < 384)
> > -		goto out_copy;
> > -
> > -	/* panic_write() may be in an interrupt context */
> > -	if (in_interrupt() || oops_in_progress)
> > +	/*
> > +	 * If the buffer address is not DMA-able, len is not long enough to make
> > +	 * DMA transfers profitable or panic_write() may be in an interrupt
> > +	 * context fallback to PIO mode.
> > +	 */
> > +	if (!virt_addr_valid(buf) || bram_offset & 3 || (size_t)buf & 3 ||
> > +	    count < 384 || in_interrupt() || oops_in_progress )
> >  		goto out_copy;
> >  
> > -	if (buf >= high_memory) {
> > -		struct page *p1;
> > -
> > -		if (((size_t)buf & PAGE_MASK) !=
> > -		    ((size_t)(buf + count - 1) & PAGE_MASK))
> > -			goto out_copy;
> > -		p1 = vmalloc_to_page(buf);
> > -		if (!p1)
> > -			goto out_copy;
> > -		buf = page_address(p1) + ((size_t)buf & ~PAGE_MASK);
> > -	}
> > -
> > -	dma_src = dma_map_single(&c->pdev->dev, buf, count, DMA_TO_DEVICE);
> > +	dma_src = dma_map_single(dev, buf, count, DMA_TO_DEVICE);
> >  	dma_dst = c->phys_base + bram_offset;
> > -	if (dma_mapping_error(&c->pdev->dev, dma_src)) {
> > -		dev_err(&c->pdev->dev,
> > -			"Couldn't DMA map a %d byte buffer\n",
> > -			count);
> > -		return -1;
> > -	}
> > -
> > -	ret = omap2_onenand_dma_transfer(c, dma_src, dma_dst, count);
> > -	dma_unmap_single(&c->pdev->dev, dma_src, count, DMA_TO_DEVICE);
> > -
> > -	if (ret) {
> > -		dev_err(&c->pdev->dev, "timeout waiting for DMA\n");
> > +	if (dma_mapping_error(dev, dma_src)) {
> > +		dev_err(dev, "Couldn't DMA map a %d byte buffer\n", count);
> >  		goto out_copy;
> >  	}
> >  
> > -	return 0;
> > +	err = omap2_onenand_dma_transfer(c, dma_src, dma_dst, count);
> > +	dma_unmap_page(dev, dma_src, count, DMA_TO_DEVICE);
> > +	if (!err)
> > +		return 0;
> > +
> > +	dev_err(dev, "timeout waiting for DMA\n");
> >  
> >  out_copy:
> >  	memcpy(this->base + bram_offset, buf, count);
> > 
> 
> - Péter
> 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] mtd: onenand: omap2: Disable DMA for HIGHMEM buffers
  2018-04-16  6:52 [PATCH v2] mtd: onenand: omap2: Disable DMA for HIGHMEM buffers Ladislav Michl
  2018-04-16 11:34 ` Peter Ujfalusi
@ 2018-04-20 20:01 ` Boris Brezillon
  2018-05-02  8:06   ` Ladislav Michl
  1 sibling, 1 reply; 6+ messages in thread
From: Boris Brezillon @ 2018-04-20 20:01 UTC (permalink / raw)
  To: Ladislav Michl
  Cc: linux-kernel, linux-mtd, Boris Brezillon, Peter Ujfalusi,
	Roger Quadros, Aaro Koskinen, Tony Lindgren,
	H. Nikolaus Schaller, Andreas Kemnade

Hi Ladislav,

On Mon, 16 Apr 2018 08:52:59 +0200
Ladislav Michl <ladis@linux-mips.org> wrote:

> dma_map_single doesn't get the proper DMA address for vmalloced area,

That's not true, it returns the right DMA (physical) address, it's just
that:

1/ the memory location is not necessarily physically contiguous
2/ in case your arch is VIVT ot VIPT, there may be several entries in
   the cache pointing to the same physical location, and the cache
   maintenance operations done by dma_map_single() will only operate on
   one of these entries.

> so disable DMA in this case.
> 

The fix looks good though. Can you rephrase your commit message to make
it clearer.

Thanks,

Boris

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] mtd: onenand: omap2: Disable DMA for HIGHMEM buffers
  2018-04-20 20:01 ` Boris Brezillon
@ 2018-05-02  8:06   ` Ladislav Michl
  2018-05-02  9:16     ` Boris Brezillon
  0 siblings, 1 reply; 6+ messages in thread
From: Ladislav Michl @ 2018-05-02  8:06 UTC (permalink / raw)
  To: Boris Brezillon
  Cc: linux-kernel, linux-mtd, Boris Brezillon, Peter Ujfalusi,
	Roger Quadros, Aaro Koskinen, Tony Lindgren,
	H. Nikolaus Schaller, Andreas Kemnade

Hi Boris,

(and apologies for delay)

On Fri, Apr 20, 2018 at 10:01:34PM +0200, Boris Brezillon wrote:
> Hi Ladislav,
> 
> On Mon, 16 Apr 2018 08:52:59 +0200
> Ladislav Michl <ladis@linux-mips.org> wrote:
> 
> > dma_map_single doesn't get the proper DMA address for vmalloced area,
> 
> That's not true, it returns the right DMA (physical) address, it's just
> that:

To be honest I used log message from commit dcf08227e964 which is dealing
with the same issue.

> 1/ the memory location is not necessarily physically contiguous
> 2/ in case your arch is VIVT ot VIPT, there may be several entries in
>    the cache pointing to the same physical location, and the cache
>    maintenance operations done by dma_map_single() will only operate on
>    one of these entries.

Well, there are few things suspicious here: DMA is used to transfer data
to system memory from bufferram which is 3kB in size. Nikolaus reported
Internal error: Oops: 805 [#1] PREEMPT SMP ARM
PC is at v7_dma_inv_range+0x30/0x48
LR is at dma_cache_maint_page+0xd0/0xe0
but I do not see how could it happen if it would be caused by 1/ or 2/ above.
Cache issue would cause data inconsistencies which should be caught by upper
MTD layers.

And given that replacing dma_map_single with dma_map_page done in first
version of patch also fixed oops, it is not caused by 1/
CPU is OMAP3630 based on Cortex-A8, D-cache is often described as PIPT but
seems to be VIPT with built-in alias detection mechanism - anyone with
better knowledge will eventually correct me, thank you.

> > so disable DMA in this case.
> > 
> 
> The fix looks good though. Can you rephrase your commit message to make
> it clearer.

Sure, I'd like to, but I do not know what's root cause yet :)

> Thanks,
> 
> Boris
	
Best regards,
	ladis

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] mtd: onenand: omap2: Disable DMA for HIGHMEM buffers
  2018-05-02  8:06   ` Ladislav Michl
@ 2018-05-02  9:16     ` Boris Brezillon
  0 siblings, 0 replies; 6+ messages in thread
From: Boris Brezillon @ 2018-05-02  9:16 UTC (permalink / raw)
  To: Ladislav Michl
  Cc: linux-kernel, linux-mtd, Boris Brezillon, Peter Ujfalusi,
	Roger Quadros, Aaro Koskinen, Tony Lindgren,
	H. Nikolaus Schaller, Andreas Kemnade

On Wed, 2 May 2018 10:06:36 +0200
Ladislav Michl <ladis@linux-mips.org> wrote:

> Hi Boris,
> 
> (and apologies for delay)
> 
> On Fri, Apr 20, 2018 at 10:01:34PM +0200, Boris Brezillon wrote:
> > Hi Ladislav,
> > 
> > On Mon, 16 Apr 2018 08:52:59 +0200
> > Ladislav Michl <ladis@linux-mips.org> wrote:
> >   
> > > dma_map_single doesn't get the proper DMA address for vmalloced area,  
> > 
> > That's not true, it returns the right DMA (physical) address, it's just
> > that:  
> 
> To be honest I used log message from commit dcf08227e964 which is dealing
> with the same issue.

Okay, looks like I was wrong. The problem is caused by the
virt_to_page() call done in dma_map_single_attrs() which expects a
valid virtual address (one that is present in the identity mapping). If
you pass a vmalloc address to it, the conversion is broken and that's
probably why you end up with a NULL pointer exception.

Maybe you should just say that dma_map_single() does not work for
vmalloc-ed buffers instead of saying that it does not get the right DMA
address.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-05-02  9:16 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-16  6:52 [PATCH v2] mtd: onenand: omap2: Disable DMA for HIGHMEM buffers Ladislav Michl
2018-04-16 11:34 ` Peter Ujfalusi
2018-04-16 13:32   ` Ladislav Michl
2018-04-20 20:01 ` Boris Brezillon
2018-05-02  8:06   ` Ladislav Michl
2018-05-02  9:16     ` Boris Brezillon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).