[PATCH] iommu/dma: Use a better type for dma_limit

[PATCH] iommu/dma: Use a better type for dma_limit

[Robin Murphy]

It makes little sense for dma_limit to be a dma_addr_t when we only use
it to pass u64 arguments, and combine it with another u64 along the way.
Just make it u64, and head off any possible size mismatches.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/dma-iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 0cc702a70a96..4acc4199a740 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -399,7 +399,7 @@ static int dma_info_to_prot(enum dma_data_direction dir, bool coherent,
 }
 
 static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain,
-		size_t size, dma_addr_t dma_limit, struct device *dev)
+		size_t size, u64 dma_limit, struct device *dev)
 {
 	struct iommu_dma_cookie *cookie = domain->iova_cookie;
 	struct iova_domain *iovad = &cookie->iovad;
-- 
2.23.0.dirty

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu/dma: Use a better type for dma_limit

[Stephan Gerhold]

On Mon, Dec 09, 2019 at 01:08:32PM +0000, Robin Murphy wrote:
> It makes little sense for dma_limit to be a dma_addr_t when we only use
> it to pass u64 arguments, and combine it with another u64 along the way.
> Just make it u64, and head off any possible size mismatches.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/iommu/dma-iommu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 0cc702a70a96..4acc4199a740 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -399,7 +399,7 @@ static int dma_info_to_prot(enum dma_data_direction dir, bool coherent,
>  }
>  
>  static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain,
> -		size_t size, dma_addr_t dma_limit, struct device *dev)
> +		size_t size, u64 dma_limit, struct device *dev)
>  {
>  	struct iommu_dma_cookie *cookie = domain->iova_cookie;
>  	struct iova_domain *iovad = &cookie->iovad;

Just wondering, maybe you should do the same for:

static dma_addr_t __iommu_dma_map(struct device *dev, phys_addr_t phys,
		size_t size, int prot, dma_addr_t dma_mask)

since that passes on the dma_mask as dma_addr_t instead of u64?


Also FYI, this fixes the warning on arm32 reported by Nathan [1],
but introduces another similar warning on the min() macro below:

In file included from ../include/linux/list.h:9,
                 from ../include/linux/kobject.h:19,
                 from ../include/linux/of.h:17,
                 from ../include/linux/irqdomain.h:35,
                 from ../include/linux/acpi.h:13,
                 from ../include/linux/acpi_iort.h:10,
                 from ../drivers/iommu/dma-iommu.c:11:
../drivers/iommu/dma-iommu.c: In function 'iommu_dma_alloc_iova':
../include/linux/kernel.h:844:29: warning: comparison of distinct pointer types lacks a cast
  844 |   (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
      |                             ^~
../include/linux/kernel.h:858:4: note: in expansion of macro '__typecheck'
  858 |   (__typecheck(x, y) && __no_side_effects(x, y))
      |    ^~~~~~~~~~~
../include/linux/kernel.h:868:24: note: in expansion of macro '__safe_cmp'
  868 |  __builtin_choose_expr(__safe_cmp(x, y), \
      |                        ^~~~~~~~~~
../include/linux/kernel.h:877:19: note: in expansion of macro '__careful_cmp'
  877 | #define min(x, y) __careful_cmp(x, y, <)
      |                   ^~~~~~~~~~~~~
../drivers/iommu/dma-iommu.c:427:15: note: in expansion of macro 'min'
  427 |   dma_limit = min(dma_limit, domain->geometry.aperture_end);
      |               ^~~

min_t(u64, dma_limit, domain->geometry.aperture_end) seems to fix it,
but not sure if that is correct.

Thanks,
Stephan

[1]: https://lore.kernel.org/linux-iommu/20191123165108.GA15306@ubuntu-x2-xlarge-x86/
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu/dma: Use a better type for dma_limit

[Robin Murphy]

On 10/12/2019 9:28 am, Stephan Gerhold wrote:
> On Mon, Dec 09, 2019 at 01:08:32PM +0000, Robin Murphy wrote:
>> It makes little sense for dma_limit to be a dma_addr_t when we only use
>> it to pass u64 arguments, and combine it with another u64 along the way.
>> Just make it u64, and head off any possible size mismatches.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>   drivers/iommu/dma-iommu.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>> index 0cc702a70a96..4acc4199a740 100644
>> --- a/drivers/iommu/dma-iommu.c
>> +++ b/drivers/iommu/dma-iommu.c
>> @@ -399,7 +399,7 @@ static int dma_info_to_prot(enum dma_data_direction dir, bool coherent,
>>   }
>>   
>>   static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain,
>> -		size_t size, dma_addr_t dma_limit, struct device *dev)
>> +		size_t size, u64 dma_limit, struct device *dev)
>>   {
>>   	struct iommu_dma_cookie *cookie = domain->iova_cookie;
>>   	struct iova_domain *iovad = &cookie->iovad;
> 
> Just wondering, maybe you should do the same for:
> 
> static dma_addr_t __iommu_dma_map(struct device *dev, phys_addr_t phys,
> 		size_t size, int prot, dma_addr_t dma_mask)
> 
> since that passes on the dma_mask as dma_addr_t instead of u64?
> 
> 
> Also FYI, this fixes the warning on arm32 reported by Nathan [1],
> but introduces another similar warning on the min() macro below:

Oh bums, I guess it's time to do a proper type audit and some more 
exhaustive build-testing then - no point in just moving warnings 
around... Thanks for the heads-up!

Robin.

> In file included from ../include/linux/list.h:9,
>                   from ../include/linux/kobject.h:19,
>                   from ../include/linux/of.h:17,
>                   from ../include/linux/irqdomain.h:35,
>                   from ../include/linux/acpi.h:13,
>                   from ../include/linux/acpi_iort.h:10,
>                   from ../drivers/iommu/dma-iommu.c:11:
> ../drivers/iommu/dma-iommu.c: In function 'iommu_dma_alloc_iova':
> ../include/linux/kernel.h:844:29: warning: comparison of distinct pointer types lacks a cast
>    844 |   (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
>        |                             ^~
> ../include/linux/kernel.h:858:4: note: in expansion of macro '__typecheck'
>    858 |   (__typecheck(x, y) && __no_side_effects(x, y))
>        |    ^~~~~~~~~~~
> ../include/linux/kernel.h:868:24: note: in expansion of macro '__safe_cmp'
>    868 |  __builtin_choose_expr(__safe_cmp(x, y), \
>        |                        ^~~~~~~~~~
> ../include/linux/kernel.h:877:19: note: in expansion of macro '__careful_cmp'
>    877 | #define min(x, y) __careful_cmp(x, y, <)
>        |                   ^~~~~~~~~~~~~
> ../drivers/iommu/dma-iommu.c:427:15: note: in expansion of macro 'min'
>    427 |   dma_limit = min(dma_limit, domain->geometry.aperture_end);
>        |               ^~~
> 
> min_t(u64, dma_limit, domain->geometry.aperture_end) seems to fix it,
> but not sure if that is correct.
> 
> Thanks,
> Stephan
> 
> [1]: https://lore.kernel.org/linux-iommu/20191123165108.GA15306@ubuntu-x2-xlarge-x86/
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu