[PATCH] firmware: qcom_scm: fix bogous abuse of dma-direct internals

[PATCH] firmware: qcom_scm: fix bogous abuse of dma-direct internals

[Christoph Hellwig]

As far as the device is concerned the dma address is the physical
address.  There is no need to convert it to a physical address,
especially not using dma-direct internals that are not available
to drivers and which will interact badly with IOMMUs.  Last but not
least the commit introducing it claimed to just fix a type issue,
but actually changed behavior.

Fixes: 6e37ccf78a532 ("firmware: qcom_scm: Use proper types for dma mappings")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/firmware/qcom_scm.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 059bb0fbae9e..4701487573f7 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -6,7 +6,6 @@
 #include <linux/init.h>
 #include <linux/cpumask.h>
 #include <linux/export.h>
-#include <linux/dma-direct.h>
 #include <linux/dma-mapping.h>
 #include <linux/module.h>
 #include <linux/types.h>
@@ -806,8 +805,7 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
 	struct qcom_scm_mem_map_info *mem_to_map;
 	phys_addr_t mem_to_map_phys;
 	phys_addr_t dest_phys;
-	phys_addr_t ptr_phys;
-	dma_addr_t ptr_dma;
+	dma_addr_t ptr_phys;
 	size_t mem_to_map_sz;
 	size_t dest_sz;
 	size_t src_sz;
@@ -824,10 +822,9 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
 	ptr_sz = ALIGN(src_sz, SZ_64) + ALIGN(mem_to_map_sz, SZ_64) +
 			ALIGN(dest_sz, SZ_64);
 
-	ptr = dma_alloc_coherent(__scm->dev, ptr_sz, &ptr_dma, GFP_KERNEL);
+	ptr = dma_alloc_coherent(__scm->dev, ptr_sz, &ptr_phys, GFP_KERNEL);
 	if (!ptr)
 		return -ENOMEM;
-	ptr_phys = dma_to_phys(__scm->dev, ptr_dma);
 
 	/* Fill source vmid detail */
 	src = ptr;
@@ -855,7 +852,7 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
 
 	ret = __qcom_scm_assign_mem(__scm->dev, mem_to_map_phys, mem_to_map_sz,
 				    ptr_phys, src_sz, dest_phys, dest_sz);
-	dma_free_coherent(__scm->dev, ptr_sz, ptr, ptr_dma);
+	dma_free_coherent(__scm->dev, ptr_sz, ptr, ptr_phys);
 	if (ret) {
 		dev_err(__scm->dev,
 			"Assign memory protection call failed %d\n", ret);
-- 
2.25.1

Re: [PATCH] firmware: qcom_scm: fix bogous abuse of dma-direct internals

[Bjorn Andersson]

On Tue 14 Apr 05:31 PDT 2020, Christoph Hellwig wrote:

> As far as the device is concerned the dma address is the physical
> address.  There is no need to convert it to a physical address,
> especially not using dma-direct internals that are not available
> to drivers and which will interact badly with IOMMUs.  Last but not
> least the commit introducing it claimed to just fix a type issue,
> but actually changed behavior.
> 
> Fixes: 6e37ccf78a532 ("firmware: qcom_scm: Use proper types for dma mappings")
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

and applied.

Thanks,
Bjorn

> ---
>  drivers/firmware/qcom_scm.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 059bb0fbae9e..4701487573f7 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -6,7 +6,6 @@
>  #include <linux/init.h>
>  #include <linux/cpumask.h>
>  #include <linux/export.h>
> -#include <linux/dma-direct.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/module.h>
>  #include <linux/types.h>
> @@ -806,8 +805,7 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
>  	struct qcom_scm_mem_map_info *mem_to_map;
>  	phys_addr_t mem_to_map_phys;
>  	phys_addr_t dest_phys;
> -	phys_addr_t ptr_phys;
> -	dma_addr_t ptr_dma;
> +	dma_addr_t ptr_phys;
>  	size_t mem_to_map_sz;
>  	size_t dest_sz;
>  	size_t src_sz;
> @@ -824,10 +822,9 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
>  	ptr_sz = ALIGN(src_sz, SZ_64) + ALIGN(mem_to_map_sz, SZ_64) +
>  			ALIGN(dest_sz, SZ_64);
>  
> -	ptr = dma_alloc_coherent(__scm->dev, ptr_sz, &ptr_dma, GFP_KERNEL);
> +	ptr = dma_alloc_coherent(__scm->dev, ptr_sz, &ptr_phys, GFP_KERNEL);
>  	if (!ptr)
>  		return -ENOMEM;
> -	ptr_phys = dma_to_phys(__scm->dev, ptr_dma);
>  
>  	/* Fill source vmid detail */
>  	src = ptr;
> @@ -855,7 +852,7 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
>  
>  	ret = __qcom_scm_assign_mem(__scm->dev, mem_to_map_phys, mem_to_map_sz,
>  				    ptr_phys, src_sz, dest_phys, dest_sz);
> -	dma_free_coherent(__scm->dev, ptr_sz, ptr, ptr_dma);
> +	dma_free_coherent(__scm->dev, ptr_sz, ptr, ptr_phys);
>  	if (ret) {
>  		dev_err(__scm->dev,
>  			"Assign memory protection call failed %d\n", ret);
> -- 
> 2.25.1
>