[PATCH] dmaengine: care sd_dma_address/len in dmaengine_prep_slave_single()

[PATCH] dmaengine: care sd_dma_address/len in dmaengine_prep_slave_single()

[Kuninori Morimoto]

dmaengine_prep_slave_single() is helper macro of dmaengine.
But it doesn't have sg_dma_address/len() settings which are required.
And it used void *buf in parameter, but it should be dma_addr_t.
This patch fixes up it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 include/linux/dmaengine.h |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 75f53f8..f47c578 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -521,11 +521,16 @@ static inline int dmaengine_slave_config(struct dma_chan *chan,
 }
 
 static inline struct dma_async_tx_descriptor *dmaengine_prep_slave_single(
-	struct dma_chan *chan, void *buf, size_t len,
+	struct dma_chan *chan, dma_addr_t buf, size_t len,
 	enum dma_data_direction dir, unsigned long flags)
 {
 	struct scatterlist sg;
-	sg_init_one(&sg, buf, len);
+
+	sg_init_table(&sg, 1);
+	sg_set_page(&sg, pfn_to_page(PFN_DOWN(buf)),
+		    len, offset_in_page(buf));
+	sg_dma_address(&sg) = buf;
+	sg_dma_len(&sg) = len;
 
 	return chan->device->device_prep_slave_sg(chan, &sg, 1, dir, flags);
 }
-- 
1.7.5.4

Re: [PATCH] dmaengine: care sd_dma_address/len in dmaengine_prep_slave_single()

[Vinod Koul]

On Mon, 2012-01-30 at 01:25 -0800, Kuninori Morimoto wrote:
> dmaengine_prep_slave_single() is helper macro of dmaengine.
> But it doesn't have sg_dma_address/len() settings which are required.
> And it used void *buf in parameter, but it should be dma_addr_t.
> This patch fixes up it.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  include/linux/dmaengine.h |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index 75f53f8..f47c578 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -521,11 +521,16 @@ static inline int dmaengine_slave_config(struct dma_chan *chan,
>  }
>  
>  static inline struct dma_async_tx_descriptor *dmaengine_prep_slave_single(
> -	struct dma_chan *chan, void *buf, size_t len,
> +	struct dma_chan *chan, dma_addr_t buf, size_t len,
>  	enum dma_data_direction dir, unsigned long flags)
dma_transfer_direction... this is not based on 3.3-rc1 or
slave-dma/next?

>  {
>  	struct scatterlist sg;
> -	sg_init_one(&sg, buf, len);
> +
> +	sg_init_table(&sg, 1);
> +	sg_set_page(&sg, pfn_to_page(PFN_DOWN(buf)),
> +		    len, offset_in_page(buf));
> +	sg_dma_address(&sg) = buf;
> +	sg_dma_len(&sg) = len;
>  
>  	return chan->device->device_prep_slave_sg(chan, &sg, 1, dir, flags);
>  }


-- 
~Vinod

[PATCH v2] dmaengine: care sd_dma_address/len in dmaengine_prep_slave_single()

[Kuninori Morimoto]

dmaengine_prep_slave_single() is helper macro of dmaengine.
But it doesn't have sg_dma_address/len() settings which are required.
And it used void *buf in parameter, but it should be dma_addr_t.
This patch fixes up it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
V1 -> v2

- based on latest linus/master branch

 include/linux/dmaengine.h |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 679b349..97eaf57 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -606,11 +606,16 @@ static inline int dmaengine_slave_config(struct dma_chan *chan,
 }
 
 static inline struct dma_async_tx_descriptor *dmaengine_prep_slave_single(
-	struct dma_chan *chan, void *buf, size_t len,
+	struct dma_chan *chan, dma_addr_t buf, size_t len,
 	enum dma_transfer_direction dir, unsigned long flags)
 {
 	struct scatterlist sg;
-	sg_init_one(&sg, buf, len);
+
+	sg_init_table(&sg, 1);
+	sg_set_page(&sg, pfn_to_page(PFN_DOWN(buf)),
+		    len, offset_in_page(buf));
+	sg_dma_address(&sg) = buf;
+	sg_dma_len(&sg) = len;
 
 	return chan->device->device_prep_slave_sg(chan, &sg, 1, dir, flags);
 }
-- 
1.7.5.4

Re: [PATCH v2] dmaengine: care sd_dma_address/len in dmaengine_prep_slave_single()

[Russell King - ARM Linux]

On Mon, Jan 30, 2012 at 05:13:30PM -0800, Kuninori Morimoto wrote:
> +	sg_init_table(&sg, 1);
> +	sg_set_page(&sg, pfn_to_page(PFN_DOWN(buf)),
> +		    len, offset_in_page(buf));

There's not much point setting this - the page/virtual address should
not be used by DMA engines as that may not reflect what's being requested
(especially if there's an IOMMU in the way.)

Re: [PATCH v2] dmaengine: care sd_dma_address/len in dmaengine_prep_slave_single()

[Lars-Peter Clausen]

On 01/31/2012 02:13 AM, Kuninori Morimoto wrote:
> dmaengine_prep_slave_single() is helper macro of dmaengine.
> But it doesn't have sg_dma_address/len() settings which are required.
> And it used void *buf in parameter, but it should be dma_addr_t.
> This patch fixes up it.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Hi,

Any news regarding this patch? The dmaengine_prep_slave_single in upstream
is not really usable as it is right now.

Thanks,
- Lars

> ---
> V1 -> v2
> 
> - based on latest linus/master branch
> 
>  include/linux/dmaengine.h |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index 679b349..97eaf57 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -606,11 +606,16 @@ static inline int dmaengine_slave_config(struct dma_chan *chan,
>  }
>  
>  static inline struct dma_async_tx_descriptor *dmaengine_prep_slave_single(
> -	struct dma_chan *chan, void *buf, size_t len,
> +	struct dma_chan *chan, dma_addr_t buf, size_t len,
>  	enum dma_transfer_direction dir, unsigned long flags)
>  {
>  	struct scatterlist sg;
> -	sg_init_one(&sg, buf, len);
> +
> +	sg_init_table(&sg, 1);
> +	sg_set_page(&sg, pfn_to_page(PFN_DOWN(buf)),
> +		    len, offset_in_page(buf));
> +	sg_dma_address(&sg) = buf;
> +	sg_dma_len(&sg) = len;
>  
>  	return chan->device->device_prep_slave_sg(chan, &sg, 1, dir, flags);
>  }

Re: [PATCH v2] dmaengine: care sd_dma_address/len in dmaengine_prep_slave_single()

[Kuninori Morimoto]

Hi Lars

> On 01/31/2012 02:13 AM, Kuninori Morimoto wrote:
> > dmaengine_prep_slave_single() is helper macro of dmaengine.
> > But it doesn't have sg_dma_address/len() settings which are required.
> > And it used void *buf in parameter, but it should be dma_addr_t.
> > This patch fixes up it.
> > 
> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> Hi,
> 
> Any news regarding this patch? The dmaengine_prep_slave_single in upstream
> is not really usable as it is right now.

Sorry. I have no news/update for this.
I'm not good at IOMMU which was pointed by Russell.

Best regards
---
Kuninori Morimoto

Re: [PATCH v2] dmaengine: care sd_dma_address/len in dmaengine_prep_slave_single()

[Lars-Peter Clausen]

On 03/09/2012 01:18 AM, Kuninori Morimoto wrote:
> 
> Hi Lars
> 
>> On 01/31/2012 02:13 AM, Kuninori Morimoto wrote:
>>> dmaengine_prep_slave_single() is helper macro of dmaengine.
>>> But it doesn't have sg_dma_address/len() settings which are required.
>>> And it used void *buf in parameter, but it should be dma_addr_t.
>>> This patch fixes up it.
>>>
>>> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>>
>> Hi,
>>
>> Any news regarding this patch? The dmaengine_prep_slave_single in upstream
>> is not really usable as it is right now.
> 
> Sorry. I have no news/update for this.
> I'm not good at IOMMU which was pointed by Russell.
> 
> Best regards
> ---
> Kuninori Morimoto


Well, he pointed out a minor and easy to fix flaw with the patch. But if you
don't want to resend the patch is it OK for you, if I resend the patch with the
flaw fixed? And is it OK, if I keep your Signed-off-by on that patch?

Thanks,
- Lars

Re: [PATCH v2] dmaengine: care sd_dma_address/len in dmaengine_prep_slave_single()

[Kuninori Morimoto]

Hi Lars

> > Sorry. I have no news/update for this.
> > I'm not good at IOMMU which was pointed by Russell.
(snip)
> Well, he pointed out a minor and easy to fix flaw with the patch. But if you
> don't want to resend the patch is it OK for you, if I resend the patch with the
> flaw fixed? And is it OK, if I keep your Signed-off-by on that patch?

Thank you for your help.
Yes I agree.
I want Reported-by for it

Best regards
---
Kuninori Morimoto

Re: [PATCH v2] dmaengine: care sd_dma_address/len in dmaengine_prep_slave_single()

[Lars-Peter Clausen]

On 02/01/2012 07:22 PM, Russell King - ARM Linux wrote:
> On Mon, Jan 30, 2012 at 05:13:30PM -0800, Kuninori Morimoto wrote:
>> +	sg_init_table(&sg, 1);
>> +	sg_set_page(&sg, pfn_to_page(PFN_DOWN(buf)),
>> +		    len, offset_in_page(buf));
> 
> There's not much point setting this - the page/virtual address should
> not be used by DMA engines as that may not reflect what's being requested
> (especially if there's an IOMMU in the way.)

I was just looking at this again and there are actually quite a few dma
engine drivers which use sg->length and sg_phys(sg). I guess these would
break if the sg_set_page was to be removed. Should these drivers be changed
to use sg_dma_len and sg_dma_address?

Thanks
- Lars

Re: [PATCH v2] dmaengine: care sd_dma_address/len in dmaengine_prep_slave_single()

[Russell King - ARM Linux]

On Wed, Apr 25, 2012 at 04:18:23PM +0200, Lars-Peter Clausen wrote:
> On 02/01/2012 07:22 PM, Russell King - ARM Linux wrote:
> > On Mon, Jan 30, 2012 at 05:13:30PM -0800, Kuninori Morimoto wrote:
> >> +	sg_init_table(&sg, 1);
> >> +	sg_set_page(&sg, pfn_to_page(PFN_DOWN(buf)),
> >> +		    len, offset_in_page(buf));
> > 
> > There's not much point setting this - the page/virtual address should
> > not be used by DMA engines as that may not reflect what's being requested
> > (especially if there's an IOMMU in the way.)
> 
> I was just looking at this again and there are actually quite a few dma
> engine drivers which use sg->length and sg_phys(sg). I guess these would
> break if the sg_set_page was to be removed. Should these drivers be changed
> to use sg_dma_len and sg_dma_address?

sg->length is meaningless to something performing DMA - and that's probably
a bug you've found.

In cases where sg_dma_len(sg) and sg->length are the same storage, then
there's no problem.  But scatterlists _can_ (and one some architectures)
do split them - especially when you have an IOMMU which can allow you to
combine a scatterlist into fewer entries.

So, anything using sg->length for the size of a scatterlist's DMA transfer
_after_ a call to dma_map_sg() is almost certainly buggy.

sg_phys(sg) of course has nothing to do with DMA addresses.  It's the
physical address _to the CPU_ of the memory associated with the scatterlist
entry.  That may, or may not have the same value for the DMA engine,
particularly if IOMMUs are involved.  So again, that's a bug.

And if these drivers are used on ARM, they must be fixed, sooner rather
than later.  There's patches in the works which will mean we will end up
with IOMMU support in the DMA mapping later, which means everything I've
said above will become reality.