Linux cryptographic layer development
 help / color / mirror / Atom feed
* [PATCH 1/3] crypto: marvell/cesa - replace dma_to_phys with dma_map_single
@ 2016-03-17 22:02 Sinan Kaya
  2016-03-17 22:54 ` Russell King - ARM Linux
  2016-03-18 20:18 ` kbuild test robot
  0 siblings, 2 replies; 12+ messages in thread
From: Sinan Kaya @ 2016-03-17 22:02 UTC (permalink / raw)
  To: linux-arm-kernel, timur, cov, nwatters
  Cc: Sinan Kaya, Boris Brezillon, Arnaud Ebalard, Herbert Xu,
	David S. Miller, linux-crypto, linux-kernel

Getting ready to remove dma_to_phys API. Drivers should not be
using this API for DMA operations. Instead, they should go
through the dma_map or dma_alloc APIs.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/crypto/marvell/cesa.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/marvell/cesa.c b/drivers/crypto/marvell/cesa.c
index c0656e7..52ddfa4 100644
--- a/drivers/crypto/marvell/cesa.c
+++ b/drivers/crypto/marvell/cesa.c
@@ -350,8 +350,8 @@ static int mv_cesa_get_sram(struct platform_device *pdev, int idx)
 	if (IS_ERR(engine->sram))
 		return PTR_ERR(engine->sram);
 
-	engine->sram_dma = phys_to_dma(cesa->dev,
-				       (phys_addr_t)res->start);
+	engine->sram_dma = dma_map_single(cesa->dev, engine->sram,
+					  DMA_TO_DEVICE);
 
 	return 0;
 }
-- 
1.8.2.1

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

* Re: [PATCH 1/3] crypto: marvell/cesa - replace dma_to_phys with dma_map_single
  2016-03-17 22:02 [PATCH 1/3] crypto: marvell/cesa - replace dma_to_phys with dma_map_single Sinan Kaya
@ 2016-03-17 22:54 ` Russell King - ARM Linux
  2016-03-17 23:17   ` okaya
  2016-03-18 20:18 ` kbuild test robot
  1 sibling, 1 reply; 12+ messages in thread
From: Russell King - ARM Linux @ 2016-03-17 22:54 UTC (permalink / raw)
  To: Sinan Kaya
  Cc: linux-arm-kernel, timur, cov, nwatters, Boris Brezillon,
	Herbert Xu, Arnaud Ebalard, linux-kernel, linux-crypto,
	David S. Miller

On Thu, Mar 17, 2016 at 06:02:15PM -0400, Sinan Kaya wrote:
> Getting ready to remove dma_to_phys API. Drivers should not be
> using this API for DMA operations. Instead, they should go
> through the dma_map or dma_alloc APIs.
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  drivers/crypto/marvell/cesa.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/crypto/marvell/cesa.c b/drivers/crypto/marvell/cesa.c
> index c0656e7..52ddfa4 100644
> --- a/drivers/crypto/marvell/cesa.c
> +++ b/drivers/crypto/marvell/cesa.c
> @@ -350,8 +350,8 @@ static int mv_cesa_get_sram(struct platform_device *pdev, int idx)
>  	if (IS_ERR(engine->sram))
>  		return PTR_ERR(engine->sram);
>  
> -	engine->sram_dma = phys_to_dma(cesa->dev,
> -				       (phys_addr_t)res->start);
> +	engine->sram_dma = dma_map_single(cesa->dev, engine->sram,
> +					  DMA_TO_DEVICE);

Documentation/DMA-API.txt

dma_addr_t
dma_map_single(struct device *dev, void *cpu_addr, size_t size,
                      enum dma_data_direction direction)

Notes:  Not all memory regions in a machine can be mapped by this API.
Further, contiguous kernel virtual space may not be contiguous as
physical memory.  Since this API does not provide any scatter/gather
capability, it will fail if the user tries to map a non-physically
contiguous piece of memory.  For this reason, memory to be mapped by
this API should be obtained from sources which guarantee it to be
physically contiguous (like kmalloc).

Specifically, ioremapped memory will *not* work as you expect with
dma_map_single().

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

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

* Re: [PATCH 1/3] crypto: marvell/cesa - replace dma_to_phys with dma_map_single
  2016-03-17 22:54 ` Russell King - ARM Linux
@ 2016-03-17 23:17   ` okaya
  2016-03-17 23:50     ` Russell King - ARM Linux
  0 siblings, 1 reply; 12+ messages in thread
From: okaya @ 2016-03-17 23:17 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: linux-arm-kernel, timur, cov, nwatters, Boris Brezillon,
	Herbert Xu, Arnaud Ebalard, linux-kernel, linux-crypto,
	David S. Miller

On 2016-03-17 18:54, Russell King - ARM Linux wrote:
> On Thu, Mar 17, 2016 at 06:02:15PM -0400, Sinan Kaya wrote:
>> Getting ready to remove dma_to_phys API. Drivers should not be
>> using this API for DMA operations. Instead, they should go
>> through the dma_map or dma_alloc APIs.
>> 
>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
>> ---
>>  drivers/crypto/marvell/cesa.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/crypto/marvell/cesa.c 
>> b/drivers/crypto/marvell/cesa.c
>> index c0656e7..52ddfa4 100644
>> --- a/drivers/crypto/marvell/cesa.c
>> +++ b/drivers/crypto/marvell/cesa.c
>> @@ -350,8 +350,8 @@ static int mv_cesa_get_sram(struct platform_device 
>> *pdev, int idx)
>>  	if (IS_ERR(engine->sram))
>>  		return PTR_ERR(engine->sram);
>> 
>> -	engine->sram_dma = phys_to_dma(cesa->dev,
>> -				       (phys_addr_t)res->start);
>> +	engine->sram_dma = dma_map_single(cesa->dev, engine->sram,
>> +					  DMA_TO_DEVICE);
> 
> Documentation/DMA-API.txt
> 
> dma_addr_t
> dma_map_single(struct device *dev, void *cpu_addr, size_t size,
>                       enum dma_data_direction direction)
> 
> Notes:  Not all memory regions in a machine can be mapped by this API.
> Further, contiguous kernel virtual space may not be contiguous as
> physical memory.  Since this API does not provide any scatter/gather
> capability, it will fail if the user tries to map a non-physically
> contiguous piece of memory.  For this reason, memory to be mapped by
> this API should be obtained from sources which guarantee it to be
> physically contiguous (like kmalloc).
> 
> Specifically, ioremapped memory will *not* work as you expect with
> dma_map_single().


What is the correct way? I don't want to write engine->sram_dma = sram

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

* Re: [PATCH 1/3] crypto: marvell/cesa - replace dma_to_phys with dma_map_single
  2016-03-17 23:17   ` okaya
@ 2016-03-17 23:50     ` Russell King - ARM Linux
  2016-03-18  9:30       ` Boris Brezillon
  0 siblings, 1 reply; 12+ messages in thread
From: Russell King - ARM Linux @ 2016-03-17 23:50 UTC (permalink / raw)
  To: okaya
  Cc: linux-arm-kernel, timur, cov, nwatters, Boris Brezillon,
	Herbert Xu, Arnaud Ebalard, linux-kernel, linux-crypto,
	David S. Miller

On Thu, Mar 17, 2016 at 07:17:24PM -0400, okaya@codeaurora.org wrote:
> What is the correct way? I don't want to write engine->sram_dma = sram

Well, what the driver _is_ wanting to do is to go from a CPU physical
address to a device DMA address.  phys_to_dma() looks like the correct
thing there to me, but I guess that's just an offset and doesn't take
account of any IOMMU that may be in the way.

If you have an IOMMU, then the whole phys_to_dma() thing is a total
failure as it only does a linear translation, and there are no
interfaces in the kernel to take account of an IOMMU in the way.  So,
it needs something designed for the job, implemented and discussed by
the normal methods of proposing a new cross-arch interface for drivers
to use.

What I'm certain of, though, is that the change proposed in this patch
will break current users of this driver: virt_to_page() on an address
returned by ioremap() is completely undefined, and will result in
either a kernel oops, or if not poking at memory which isn't a struct
page, ultimately resulting in something that isn't SRAM being pointed
to by "engine->sram_dma".

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

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

* Re: [PATCH 1/3] crypto: marvell/cesa - replace dma_to_phys with dma_map_single
  2016-03-17 23:50     ` Russell King - ARM Linux
@ 2016-03-18  9:30       ` Boris Brezillon
  2016-03-18 11:25         ` Robin Murphy
  0 siblings, 1 reply; 12+ messages in thread
From: Boris Brezillon @ 2016-03-18  9:30 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: okaya, linux-arm-kernel, timur, cov, nwatters, Herbert Xu,
	Arnaud Ebalard, linux-kernel, linux-crypto, David S. Miller

On Thu, 17 Mar 2016 23:50:20 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Thu, Mar 17, 2016 at 07:17:24PM -0400, okaya@codeaurora.org wrote:
> > What is the correct way? I don't want to write engine->sram_dma = sram
> 
> Well, what the driver _is_ wanting to do is to go from a CPU physical
> address to a device DMA address.  phys_to_dma() looks like the correct
> thing there to me, but I guess that's just an offset and doesn't take
> account of any IOMMU that may be in the way.
> 
> If you have an IOMMU, then the whole phys_to_dma() thing is a total
> failure as it only does a linear translation, and there are no
> interfaces in the kernel to take account of an IOMMU in the way.  So,
> it needs something designed for the job, implemented and discussed by
> the normal methods of proposing a new cross-arch interface for drivers
> to use.
> 
> What I'm certain of, though, is that the change proposed in this patch
> will break current users of this driver: virt_to_page() on an address
> returned by ioremap() is completely undefined, and will result in
> either a kernel oops, or if not poking at memory which isn't a struct
> page, ultimately resulting in something that isn't SRAM being pointed
> to by "engine->sram_dma".
> 

Or we could just do

	engine->sram_dma = res->start;

which is pretty much what the SRAM/genalloc code is doing already.


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

* Re: [PATCH 1/3] crypto: marvell/cesa - replace dma_to_phys with dma_map_single
  2016-03-18  9:30       ` Boris Brezillon
@ 2016-03-18 11:25         ` Robin Murphy
  2016-03-18 11:32           ` Boris Brezillon
  2016-03-18 13:51           ` Sinan Kaya
  0 siblings, 2 replies; 12+ messages in thread
From: Robin Murphy @ 2016-03-18 11:25 UTC (permalink / raw)
  To: Boris Brezillon, Russell King - ARM Linux
  Cc: linux-arm-kernel, Herbert Xu, timur, Arnaud Ebalard, linux-kernel,
	okaya, cov, David S. Miller, nwatters, linux-crypto

On 18/03/16 09:30, Boris Brezillon wrote:
> On Thu, 17 Mar 2016 23:50:20 +0000
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
>
>> On Thu, Mar 17, 2016 at 07:17:24PM -0400, okaya@codeaurora.org wrote:
>>> What is the correct way? I don't want to write engine->sram_dma = sram
>>
>> Well, what the driver _is_ wanting to do is to go from a CPU physical
>> address to a device DMA address.  phys_to_dma() looks like the correct
>> thing there to me, but I guess that's just an offset and doesn't take
>> account of any IOMMU that may be in the way.
>>
>> If you have an IOMMU, then the whole phys_to_dma() thing is a total
>> failure as it only does a linear translation, and there are no
>> interfaces in the kernel to take account of an IOMMU in the way.  So,
>> it needs something designed for the job, implemented and discussed by
>> the normal methods of proposing a new cross-arch interface for drivers
>> to use.
>>
>> What I'm certain of, though, is that the change proposed in this patch
>> will break current users of this driver: virt_to_page() on an address
>> returned by ioremap() is completely undefined, and will result in
>> either a kernel oops, or if not poking at memory which isn't a struct
>> page, ultimately resulting in something that isn't SRAM being pointed
>> to by "engine->sram_dma".
>>
>
> Or we could just do
>
> 	engine->sram_dma = res->start;
>
> which is pretty much what the SRAM/genalloc code is doing already.

As Russell points out this is yet another type of "set up a DMA master 
to access something other than kernel RAM" - there's already discussion 
in progress over how to handle this for dmaengine slaves[1], so 
gathering more use-cases might help distil exactly what the design of 
not-strictly-DMA-but-so-closely-coupled-it-can't-really-live-anywhere-else 
needs to be.

Robin.

[1]:http://lists.infradead.org/pipermail/linux-arm-kernel/2016-March/414422.html

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

* Re: [PATCH 1/3] crypto: marvell/cesa - replace dma_to_phys with dma_map_single
  2016-03-18 11:25         ` Robin Murphy
@ 2016-03-18 11:32           ` Boris Brezillon
  2016-03-18 13:51           ` Sinan Kaya
  1 sibling, 0 replies; 12+ messages in thread
From: Boris Brezillon @ 2016-03-18 11:32 UTC (permalink / raw)
  To: Robin Murphy
  Cc: Russell King - ARM Linux, linux-arm-kernel, Herbert Xu, timur,
	Arnaud Ebalard, linux-kernel, okaya, cov, David S. Miller,
	nwatters, linux-crypto

On Fri, 18 Mar 2016 11:25:48 +0000
Robin Murphy <robin.murphy@arm.com> wrote:

> On 18/03/16 09:30, Boris Brezillon wrote:
> > On Thu, 17 Mar 2016 23:50:20 +0000
> > Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> >
> >> On Thu, Mar 17, 2016 at 07:17:24PM -0400, okaya@codeaurora.org wrote:
> >>> What is the correct way? I don't want to write engine->sram_dma = sram
> >>
> >> Well, what the driver _is_ wanting to do is to go from a CPU physical
> >> address to a device DMA address.  phys_to_dma() looks like the correct
> >> thing there to me, but I guess that's just an offset and doesn't take
> >> account of any IOMMU that may be in the way.
> >>
> >> If you have an IOMMU, then the whole phys_to_dma() thing is a total
> >> failure as it only does a linear translation, and there are no
> >> interfaces in the kernel to take account of an IOMMU in the way.  So,
> >> it needs something designed for the job, implemented and discussed by
> >> the normal methods of proposing a new cross-arch interface for drivers
> >> to use.
> >>
> >> What I'm certain of, though, is that the change proposed in this patch
> >> will break current users of this driver: virt_to_page() on an address
> >> returned by ioremap() is completely undefined, and will result in
> >> either a kernel oops, or if not poking at memory which isn't a struct
> >> page, ultimately resulting in something that isn't SRAM being pointed
> >> to by "engine->sram_dma".
> >>
> >
> > Or we could just do
> >
> > 	engine->sram_dma = res->start;
> >
> > which is pretty much what the SRAM/genalloc code is doing already.
> 
> As Russell points out this is yet another type of "set up a DMA master 
> to access something other than kernel RAM" - there's already discussion 
> in progress over how to handle this for dmaengine slaves[1], so 
> gathering more use-cases might help distil exactly what the design of 
> not-strictly-DMA-but-so-closely-coupled-it-can't-really-live-anywhere-else 
> needs to be.
> 
> Robin.
> 
> [1]:http://lists.infradead.org/pipermail/linux-arm-kernel/2016-March/414422.html
> 

Hm, interesting, thanks for the pointer.

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

* Re: [PATCH 1/3] crypto: marvell/cesa - replace dma_to_phys with dma_map_single
  2016-03-18 11:25         ` Robin Murphy
  2016-03-18 11:32           ` Boris Brezillon
@ 2016-03-18 13:51           ` Sinan Kaya
  2016-03-18 14:00             ` Sinan Kaya
  2016-03-18 14:20             ` Boris Brezillon
  1 sibling, 2 replies; 12+ messages in thread
From: Sinan Kaya @ 2016-03-18 13:51 UTC (permalink / raw)
  To: Robin Murphy, Boris Brezillon, Russell King - ARM Linux
  Cc: linux-arm-kernel, Herbert Xu, timur, Arnaud Ebalard, linux-kernel,
	cov, David S. Miller, nwatters, linux-crypto

On 3/18/2016 7:25 AM, Robin Murphy wrote:
> On 18/03/16 09:30, Boris Brezillon wrote:
>> On Thu, 17 Mar 2016 23:50:20 +0000
>> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
>>
>>> On Thu, Mar 17, 2016 at 07:17:24PM -0400, okaya@codeaurora.org wrote:
>>>> What is the correct way? I don't want to write engine->sram_dma = sram
>>>
>>> Well, what the driver _is_ wanting to do is to go from a CPU physical
>>> address to a device DMA address.  phys_to_dma() looks like the correct
>>> thing there to me, but I guess that's just an offset and doesn't take
>>> account of any IOMMU that may be in the way.
>>>
>>> If you have an IOMMU, then the whole phys_to_dma() thing is a total
>>> failure as it only does a linear translation, and there are no
>>> interfaces in the kernel to take account of an IOMMU in the way.  So,
>>> it needs something designed for the job, implemented and discussed by
>>> the normal methods of proposing a new cross-arch interface for drivers
>>> to use.
>>>
>>> What I'm certain of, though, is that the change proposed in this patch
>>> will break current users of this driver: virt_to_page() on an address
>>> returned by ioremap() is completely undefined, and will result in
>>> either a kernel oops, or if not poking at memory which isn't a struct
>>> page, ultimately resulting in something that isn't SRAM being pointed
>>> to by "engine->sram_dma".
>>>
>>
>> Or we could just do
>>
>>     engine->sram_dma = res->start;
>>
>> which is pretty much what the SRAM/genalloc code is doing already.
> 
> As Russell points out this is yet another type of "set up a DMA master to access something other than kernel RAM" - there's already discussion in progress over how to handle this for dmaengine slaves[1], so gathering more use-cases might help distil exactly what the design of not-strictly-DMA-but-so-closely-coupled-it-can't-really-live-anywhere-else needs to be.
> 
> Robin.
> 
> [1]:http://lists.infradead.org/pipermail/linux-arm-kernel/2016-March/414422.html
> 

Thanks for the link. 

dma_map_resource looks like to be the correct way of doing things. Just from
the purist point of view, a driver is not supposed to know the physical address
of a DMA address. That kills the intent of using DMA API. When programming descriptors,
the DMA addresses should be programmed not physical addresses so that the same 
driver can be used in a system with IOMMU. The IOMMU DMA ops will remap the DMA 
address to a bus address that is not physical address. All of this operation needs
to be isolated from the device driver.


I don't know the architecture or the driver enough to write this. This is not ideally
right but I can do this if Boris you are OK with this. 

     engine->sram_dma = res->start;

Another option is I can write

     engine->sram_dma = swiotlb_dma_to_phys(res->start)

I agree that dma_map_single is not the right thing. 

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

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

* Re: [PATCH 1/3] crypto: marvell/cesa - replace dma_to_phys with dma_map_single
  2016-03-18 13:51           ` Sinan Kaya
@ 2016-03-18 14:00             ` Sinan Kaya
  2016-03-18 14:20             ` Boris Brezillon
  1 sibling, 0 replies; 12+ messages in thread
From: Sinan Kaya @ 2016-03-18 14:00 UTC (permalink / raw)
  To: Robin Murphy, Boris Brezillon, Russell King - ARM Linux
  Cc: linux-arm-kernel, Herbert Xu, timur, Arnaud Ebalard, linux-kernel,
	cov, David S. Miller, nwatters, linux-crypto

On 3/18/2016 9:51 AM, Sinan Kaya wrote:
> Another option is I can write
> 
>      engine->sram_dma = swiotlb_dma_to_phys(res->start)

I realized that I made a mistake in the commit message and the code above.

The code is trying to find DMA address from physical address. Not the other
way around. I'll fix it on the next version. 

The correct suggestion above would be 

      engine->sram_dma = swiotlb_phys_to_dmares->start)

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

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

* Re: [PATCH 1/3] crypto: marvell/cesa - replace dma_to_phys with dma_map_single
  2016-03-18 13:51           ` Sinan Kaya
  2016-03-18 14:00             ` Sinan Kaya
@ 2016-03-18 14:20             ` Boris Brezillon
  2016-03-18 14:21               ` Sinan Kaya
  1 sibling, 1 reply; 12+ messages in thread
From: Boris Brezillon @ 2016-03-18 14:20 UTC (permalink / raw)
  To: Sinan Kaya
  Cc: Robin Murphy, Russell King - ARM Linux, linux-arm-kernel,
	Herbert Xu, timur, Arnaud Ebalard, linux-kernel, cov,
	David S. Miller, nwatters, linux-crypto

On Fri, 18 Mar 2016 09:51:37 -0400
Sinan Kaya <okaya@codeaurora.org> wrote:

> On 3/18/2016 7:25 AM, Robin Murphy wrote:
> > On 18/03/16 09:30, Boris Brezillon wrote:
> >> On Thu, 17 Mar 2016 23:50:20 +0000
> >> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> >>
> >>> On Thu, Mar 17, 2016 at 07:17:24PM -0400, okaya@codeaurora.org wrote:
> >>>> What is the correct way? I don't want to write engine->sram_dma = sram
> >>>
> >>> Well, what the driver _is_ wanting to do is to go from a CPU physical
> >>> address to a device DMA address.  phys_to_dma() looks like the correct
> >>> thing there to me, but I guess that's just an offset and doesn't take
> >>> account of any IOMMU that may be in the way.
> >>>
> >>> If you have an IOMMU, then the whole phys_to_dma() thing is a total
> >>> failure as it only does a linear translation, and there are no
> >>> interfaces in the kernel to take account of an IOMMU in the way.  So,
> >>> it needs something designed for the job, implemented and discussed by
> >>> the normal methods of proposing a new cross-arch interface for drivers
> >>> to use.
> >>>
> >>> What I'm certain of, though, is that the change proposed in this patch
> >>> will break current users of this driver: virt_to_page() on an address
> >>> returned by ioremap() is completely undefined, and will result in
> >>> either a kernel oops, or if not poking at memory which isn't a struct
> >>> page, ultimately resulting in something that isn't SRAM being pointed
> >>> to by "engine->sram_dma".
> >>>
> >>
> >> Or we could just do
> >>
> >>     engine->sram_dma = res->start;
> >>
> >> which is pretty much what the SRAM/genalloc code is doing already.
> > 
> > As Russell points out this is yet another type of "set up a DMA master to access something other than kernel RAM" - there's already discussion in progress over how to handle this for dmaengine slaves[1], so gathering more use-cases might help distil exactly what the design of not-strictly-DMA-but-so-closely-coupled-it-can't-really-live-anywhere-else needs to be.
> > 
> > Robin.
> > 
> > [1]:http://lists.infradead.org/pipermail/linux-arm-kernel/2016-March/414422.html
> > 
> 
> Thanks for the link. 
> 
> dma_map_resource looks like to be the correct way of doing things. Just from
> the purist point of view, a driver is not supposed to know the physical address
> of a DMA address. That kills the intent of using DMA API. When programming descriptors,
> the DMA addresses should be programmed not physical addresses so that the same 
> driver can be used in a system with IOMMU. The IOMMU DMA ops will remap the DMA 
> address to a bus address that is not physical address. All of this operation needs
> to be isolated from the device driver.
> 
> 
> I don't know the architecture or the driver enough to write this. This is not ideally
> right but I can do this if Boris you are OK with this. 
> 
>      engine->sram_dma = res->start;

I don't know.

How about waiting for the 'dma_{map,unmap}_resource' discussion to
settle down before removing phy_to_dma()/dma_to_phys() APIs (as
suggested by Robin and Russell)?


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

* Re: [PATCH 1/3] crypto: marvell/cesa - replace dma_to_phys with dma_map_single
  2016-03-18 14:20             ` Boris Brezillon
@ 2016-03-18 14:21               ` Sinan Kaya
  0 siblings, 0 replies; 12+ messages in thread
From: Sinan Kaya @ 2016-03-18 14:21 UTC (permalink / raw)
  To: Boris Brezillon
  Cc: Robin Murphy, Russell King - ARM Linux, linux-arm-kernel,
	Herbert Xu, timur, Arnaud Ebalard, linux-kernel, cov,
	David S. Miller, nwatters, linux-crypto

On 3/18/2016 10:20 AM, Boris Brezillon wrote:
> On Fri, 18 Mar 2016 09:51:37 -0400
> Sinan Kaya <okaya@codeaurora.org> wrote:
> 
>> On 3/18/2016 7:25 AM, Robin Murphy wrote:
>>> On 18/03/16 09:30, Boris Brezillon wrote:
>>>> On Thu, 17 Mar 2016 23:50:20 +0000
>>>> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
>>>>
>>>>> On Thu, Mar 17, 2016 at 07:17:24PM -0400, okaya@codeaurora.org wrote:
>>>>>> What is the correct way? I don't want to write engine->sram_dma = sram
>>>>>
>>>>> Well, what the driver _is_ wanting to do is to go from a CPU physical
>>>>> address to a device DMA address.  phys_to_dma() looks like the correct
>>>>> thing there to me, but I guess that's just an offset and doesn't take
>>>>> account of any IOMMU that may be in the way.
>>>>>
>>>>> If you have an IOMMU, then the whole phys_to_dma() thing is a total
>>>>> failure as it only does a linear translation, and there are no
>>>>> interfaces in the kernel to take account of an IOMMU in the way.  So,
>>>>> it needs something designed for the job, implemented and discussed by
>>>>> the normal methods of proposing a new cross-arch interface for drivers
>>>>> to use.
>>>>>
>>>>> What I'm certain of, though, is that the change proposed in this patch
>>>>> will break current users of this driver: virt_to_page() on an address
>>>>> returned by ioremap() is completely undefined, and will result in
>>>>> either a kernel oops, or if not poking at memory which isn't a struct
>>>>> page, ultimately resulting in something that isn't SRAM being pointed
>>>>> to by "engine->sram_dma".
>>>>>
>>>>
>>>> Or we could just do
>>>>
>>>>     engine->sram_dma = res->start;
>>>>
>>>> which is pretty much what the SRAM/genalloc code is doing already.
>>>
>>> As Russell points out this is yet another type of "set up a DMA master to access something other than kernel RAM" - there's already discussion in progress over how to handle this for dmaengine slaves[1], so gathering more use-cases might help distil exactly what the design of not-strictly-DMA-but-so-closely-coupled-it-can't-really-live-anywhere-else needs to be.
>>>
>>> Robin.
>>>
>>> [1]:http://lists.infradead.org/pipermail/linux-arm-kernel/2016-March/414422.html
>>>
>>
>> Thanks for the link. 
>>
>> dma_map_resource looks like to be the correct way of doing things. Just from
>> the purist point of view, a driver is not supposed to know the physical address
>> of a DMA address. That kills the intent of using DMA API. When programming descriptors,
>> the DMA addresses should be programmed not physical addresses so that the same 
>> driver can be used in a system with IOMMU. The IOMMU DMA ops will remap the DMA 
>> address to a bus address that is not physical address. All of this operation needs
>> to be isolated from the device driver.
>>
>>
>> I don't know the architecture or the driver enough to write this. This is not ideally
>> right but I can do this if Boris you are OK with this. 
>>
>>      engine->sram_dma = res->start;
> 
> I don't know.
> 
> How about waiting for the 'dma_{map,unmap}_resource' discussion to
> settle down before removing phy_to_dma()/dma_to_phys() APIs (as
> suggested by Robin and Russell)?
> 
> 

Sure, that's fine for me.

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

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

* Re: [PATCH 1/3] crypto: marvell/cesa - replace dma_to_phys with dma_map_single
  2016-03-17 22:02 [PATCH 1/3] crypto: marvell/cesa - replace dma_to_phys with dma_map_single Sinan Kaya
  2016-03-17 22:54 ` Russell King - ARM Linux
@ 2016-03-18 20:18 ` kbuild test robot
  1 sibling, 0 replies; 12+ messages in thread
From: kbuild test robot @ 2016-03-18 20:18 UTC (permalink / raw)
  To: Sinan Kaya
  Cc: kbuild-all, linux-arm-kernel, timur, cov, nwatters, Sinan Kaya,
	Boris Brezillon, Arnaud Ebalard, Herbert Xu, David S. Miller,
	linux-crypto, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1838 bytes --]

Hi Sinan,

[auto build test ERROR on arm64/for-next/core]
[also build test ERROR on v4.5 next-20160318]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Sinan-Kaya/crypto-marvell-cesa-replace-dma_to_phys-with-dma_map_single/20160318-060640
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux for-next/core
config: arm-allmodconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   drivers/crypto/marvell/cesa.c: In function 'mv_cesa_get_sram':
>> drivers/crypto/marvell/cesa.c:354:21: error: macro "dma_map_single" requires 4 arguments, but only 3 given
           DMA_TO_DEVICE);
                        ^
>> drivers/crypto/marvell/cesa.c:353:21: error: 'dma_map_single' undeclared (first use in this function)
     engine->sram_dma = dma_map_single(cesa->dev, engine->sram,
                        ^
   drivers/crypto/marvell/cesa.c:353:21: note: each undeclared identifier is reported only once for each function it appears in

vim +/dma_map_single +354 drivers/crypto/marvell/cesa.c

   347			return -EINVAL;
   348	
   349		engine->sram = devm_ioremap_resource(cesa->dev, res);
   350		if (IS_ERR(engine->sram))
   351			return PTR_ERR(engine->sram);
   352	
 > 353		engine->sram_dma = dma_map_single(cesa->dev, engine->sram,
 > 354						  DMA_TO_DEVICE);
   355	
   356		return 0;
   357	}

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 56216 bytes --]

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

end of thread, other threads:[~2016-03-18 20:20 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-03-17 22:02 [PATCH 1/3] crypto: marvell/cesa - replace dma_to_phys with dma_map_single Sinan Kaya
2016-03-17 22:54 ` Russell King - ARM Linux
2016-03-17 23:17   ` okaya
2016-03-17 23:50     ` Russell King - ARM Linux
2016-03-18  9:30       ` Boris Brezillon
2016-03-18 11:25         ` Robin Murphy
2016-03-18 11:32           ` Boris Brezillon
2016-03-18 13:51           ` Sinan Kaya
2016-03-18 14:00             ` Sinan Kaya
2016-03-18 14:20             ` Boris Brezillon
2016-03-18 14:21               ` Sinan Kaya
2016-03-18 20:18 ` kbuild test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox