Patchwork ARM: dmabounce: fix partial sync in dma_sync_single_* API

login
register
mail settings
Submitter FUJITA Tomonori
Date April 13, 2010, 5:27 a.m.
Message ID <20100413142607V.fujita.tomonori@lab.ntt.co.jp>
Download mbox | patch
Permalink /patch/50021/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

FUJITA Tomonori - April 13, 2010, 5:27 a.m.
On Mon, 12 Apr 2010 20:35:36 +0100
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Mon, Apr 05, 2010 at 12:39:32PM +0900, FUJITA Tomonori wrote:
> > I don't have arm hardware that uses dmabounce so I can't confirm the
> > problem but seems that dmabounce doesn't work for some drivers...
> 
> Patch reviews fine, except for one niggle.  I too don't have hardware
> I can test (well, I do except the kernel stopped supporting the UDA1341
> audio codec on the SA1110 Neponset.)

Thanks for reviewing.

> > @@ -171,10 +172,17 @@ find_safe_buffer(struct dmabounce_device_info *device_info, dma_addr_t safe_dma_
> >  	read_lock_irqsave(&device_info->lock, flags);
> >  
> >  	list_for_each_entry(b, &device_info->safe_buffers, node)
> > -		if (b->safe_dma_addr == safe_dma_addr) {
> > -			rb = b;
> > -			break;
> > -		}
> > +		if (for_sync) {
> > +			if (b->safe_dma_addr <= safe_dma_addr &&
> > +			    safe_dma_addr < b->safe_dma_addr + b->size) {
> > +				rb = b;
> > +				break;
> > +			}
> > +		} else
> > +			if (b->safe_dma_addr == safe_dma_addr) {
> > +				rb = b;
> > +				break;
> > +			}
> 
> This is the niggle; I don't like this indentation style.  If you want to
> indent this if () statement, then please format like this:
> 
> 		} else {
> 			if (b->safe...) {
> 				...
> 			}
> 		}
> 
> or format it as:
> 
> 		} else if (b->safe...) {
> 			...
> 		}

ok, here's the fixed patch.

=
From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Subject: [PATCH] ARM: dmabounce: fix partial sync in dma_sync_single_* API

Some network drivers do a partial sync with
dma_sync_single_for_{device|cpu}. The dma_addr argument might not be
the same as one as passed into the mapping API.

This adds some tricks to find_safe_buffer() for
dma_sync_single_for_{device|cpu}.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 arch/arm/common/dmabounce.c |   30 +++++++++++++++++++++---------
 1 files changed, 21 insertions(+), 9 deletions(-)
FUJITA Tomonori - April 27, 2010, 10:50 p.m.
On Tue, 13 Apr 2010 14:27:04 +0900
FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:

> On Mon, 12 Apr 2010 20:35:36 +0100
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> 
> > On Mon, Apr 05, 2010 at 12:39:32PM +0900, FUJITA Tomonori wrote:
> > > I don't have arm hardware that uses dmabounce so I can't confirm the
> > > problem but seems that dmabounce doesn't work for some drivers...
> > 
> > Patch reviews fine, except for one niggle.  I too don't have hardware
> > I can test (well, I do except the kernel stopped supporting the UDA1341
> > audio codec on the SA1110 Neponset.)
> 
> Thanks for reviewing.
> 
> > > @@ -171,10 +172,17 @@ find_safe_buffer(struct dmabounce_device_info *device_info, dma_addr_t safe_dma_
> > >  	read_lock_irqsave(&device_info->lock, flags);
> > >  
> > >  	list_for_each_entry(b, &device_info->safe_buffers, node)
> > > -		if (b->safe_dma_addr == safe_dma_addr) {
> > > -			rb = b;
> > > -			break;
> > > -		}
> > > +		if (for_sync) {
> > > +			if (b->safe_dma_addr <= safe_dma_addr &&
> > > +			    safe_dma_addr < b->safe_dma_addr + b->size) {
> > > +				rb = b;
> > > +				break;
> > > +			}
> > > +		} else
> > > +			if (b->safe_dma_addr == safe_dma_addr) {
> > > +				rb = b;
> > > +				break;
> > > +			}
> > 
> > This is the niggle; I don't like this indentation style.  If you want to
> > indent this if () statement, then please format like this:
> > 
> > 		} else {
> > 			if (b->safe...) {
> > 				...
> > 			}
> > 		}
> > 
> > or format it as:
> > 
> > 		} else if (b->safe...) {
> > 			...
> > 		}
> 
> ok, here's the fixed patch.
> 
> =
> From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Subject: [PATCH] ARM: dmabounce: fix partial sync in dma_sync_single_* API
> 
> Some network drivers do a partial sync with
> dma_sync_single_for_{device|cpu}. The dma_addr argument might not be
> the same as one as passed into the mapping API.
> 
> This adds some tricks to find_safe_buffer() for
> dma_sync_single_for_{device|cpu}.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> ---
>  arch/arm/common/dmabounce.c |   30 +++++++++++++++++++++---------
>  1 files changed, 21 insertions(+), 9 deletions(-)

Ping?

Is this going to be merged via the arm tree?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Russell King - ARM Linux - April 29, 2010, 11:34 a.m.
> Ping?
> 
> Is this going to be merged via the arm tree?

Ping all you want; I've only just got back online after a week of no
internet connectivity thanks to the carrier being unable to fix the
problem (the faulty line is still faulty.)

I'm now going to be catching up over the course of the next week, so
please be patient.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/arm/common/dmabounce.c b/arch/arm/common/dmabounce.c
index cc0a932..2e6deec 100644
--- a/arch/arm/common/dmabounce.c
+++ b/arch/arm/common/dmabounce.c
@@ -163,7 +163,8 @@  alloc_safe_buffer(struct dmabounce_device_info *device_info, void *ptr,
 
 /* determine if a buffer is from our "safe" pool */
 static inline struct safe_buffer *
-find_safe_buffer(struct dmabounce_device_info *device_info, dma_addr_t safe_dma_addr)
+find_safe_buffer(struct dmabounce_device_info *device_info, dma_addr_t safe_dma_addr,
+		 int for_sync)
 {
 	struct safe_buffer *b, *rb = NULL;
 	unsigned long flags;
@@ -171,9 +172,17 @@  find_safe_buffer(struct dmabounce_device_info *device_info, dma_addr_t safe_dma_
 	read_lock_irqsave(&device_info->lock, flags);
 
 	list_for_each_entry(b, &device_info->safe_buffers, node)
-		if (b->safe_dma_addr == safe_dma_addr) {
-			rb = b;
-			break;
+		if (for_sync) {
+			if (b->safe_dma_addr <= safe_dma_addr &&
+			    safe_dma_addr < b->safe_dma_addr + b->size) {
+				rb = b;
+				break;
+			}
+		} else {
+			if (b->safe_dma_addr == safe_dma_addr) {
+				rb = b;
+				break;
+			}
 		}
 
 	read_unlock_irqrestore(&device_info->lock, flags);
@@ -205,7 +214,8 @@  free_safe_buffer(struct dmabounce_device_info *device_info, struct safe_buffer *
 /* ************************************************** */
 
 static struct safe_buffer *find_safe_buffer_dev(struct device *dev,
-		dma_addr_t dma_addr, const char *where)
+						dma_addr_t dma_addr, const char *where,
+						int for_sync)
 {
 	if (!dev || !dev->archdata.dmabounce)
 		return NULL;
@@ -216,7 +226,7 @@  static struct safe_buffer *find_safe_buffer_dev(struct device *dev,
 			pr_err("unknown device: Trying to %s invalid mapping\n", where);
 		return NULL;
 	}
-	return find_safe_buffer(dev->archdata.dmabounce, dma_addr);
+	return find_safe_buffer(dev->archdata.dmabounce, dma_addr, for_sync);
 }
 
 static inline dma_addr_t map_single(struct device *dev, void *ptr, size_t size,
@@ -286,7 +296,7 @@  static inline dma_addr_t map_single(struct device *dev, void *ptr, size_t size,
 static inline void unmap_single(struct device *dev, dma_addr_t dma_addr,
 		size_t size, enum dma_data_direction dir)
 {
-	struct safe_buffer *buf = find_safe_buffer_dev(dev, dma_addr, "unmap");
+	struct safe_buffer *buf = find_safe_buffer_dev(dev, dma_addr, "unmap", 0);
 
 	if (buf) {
 		BUG_ON(buf->size != size);
@@ -398,7 +408,7 @@  int dmabounce_sync_for_cpu(struct device *dev, dma_addr_t addr,
 	dev_dbg(dev, "%s(dma=%#x,off=%#lx,sz=%zx,dir=%x)\n",
 		__func__, addr, off, sz, dir);
 
-	buf = find_safe_buffer_dev(dev, addr, __func__);
+	buf = find_safe_buffer_dev(dev, addr, __func__, 1);
 	if (!buf)
 		return 1;
 
@@ -411,6 +421,8 @@  int dmabounce_sync_for_cpu(struct device *dev, dma_addr_t addr,
 	DO_STATS(dev->archdata.dmabounce->bounce_count++);
 
 	if (dir == DMA_FROM_DEVICE || dir == DMA_BIDIRECTIONAL) {
+		if (addr != buf->safe_dma_addr)
+			off = addr - buf->safe_dma_addr;
 		dev_dbg(dev, "%s: copy back safe %p to unsafe %p size %d\n",
 			__func__, buf->safe + off, buf->ptr + off, sz);
 		memcpy(buf->ptr + off, buf->safe + off, sz);
@@ -427,7 +439,7 @@  int dmabounce_sync_for_device(struct device *dev, dma_addr_t addr,
 	dev_dbg(dev, "%s(dma=%#x,off=%#lx,sz=%zx,dir=%x)\n",
 		__func__, addr, off, sz, dir);
 
-	buf = find_safe_buffer_dev(dev, addr, __func__);
+	buf = find_safe_buffer_dev(dev, addr, __func__, 1);
 	if (!buf)
 		return 1;