Re: [PATCH 1/5] iommu/tegra-smmu: Fix domain_alloc

[Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>]

On 17/01/2019 15:13, Dmitry Osipenko wrote:
> 16.01.2019 23:50, Navneet Kumar пишет:
>> * Allocate dma iova cookie for a domain while adding dma iommu
>> devices.
>> * Perform a stricter check for domain type parameter.
>>
> 
> Commit message should tell what exactly is getting "fixed". Apparently you're trying to support T132 ARM64 here.
> 
>> Signed-off-by: Navneet Kumar <navneetk@nvidia.com>
>> ---
>>   drivers/iommu/tegra-smmu.c | 43 +++++++++++++++++++++++++++----------------
>>   1 file changed, 27 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
>> index 543f7c9..ee4d8a8 100644
>> --- a/drivers/iommu/tegra-smmu.c
>> +++ b/drivers/iommu/tegra-smmu.c
>> @@ -16,6 +16,7 @@
>>   #include <linux/platform_device.h>
>>   #include <linux/slab.h>
>>   #include <linux/dma-mapping.h>
>> +#include <linux/dma-iommu.h>
>>   
>>   #include <soc/tegra/ahb.h>
>>   #include <soc/tegra/mc.h>
>> @@ -271,8 +272,10 @@ static bool tegra_smmu_capable(enum iommu_cap cap)
>>   static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
>>   {
>>   	struct tegra_smmu_as *as;
>> +	int ret;
>>   
>> -	if (type != IOMMU_DOMAIN_UNMANAGED)
>> +	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA &&
>> +			type != IOMMU_DOMAIN_IDENTITY)
>>   		return NULL;
> 
> Should be better to write these lines like this for the sake of readability:
> 
> 	if (type != IOMMU_DOMAIN_UNMANAGED &&
> 	    type != IOMMU_DOMAIN_DMA &&
> 	    type != IOMMU_DOMAIN_IDENTITY)
> 		return NULL;

Furthermore, AFAICS there's no special handling being added for identity 
domains - don't pretend to support them by giving back a regular 
translation domain, that's just misleading and broken.

> 
> 
>>   
>>   	as = kzalloc(sizeof(*as), GFP_KERNEL);
>> @@ -281,26 +284,22 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
>>   
>>   	as->attr = SMMU_PD_READABLE | SMMU_PD_WRITABLE | SMMU_PD_NONSECURE;
>>   
>> +	ret = (type == IOMMU_DOMAIN_DMA) ? iommu_get_dma_cookie(&as->domain) :
>> +		-ENODEV;
> 
> This makes to fail allocation of domain that has type other than IOMMU_DOMAIN_DMA.
> 
> Should be:
> 
> 	if (type == IOMMU_DOMAIN_DMA) {
> 		err = iommu_get_dma_cookie(&as->domain);
> 		if (err)
> 			goto free_as;
> 	}
> 
> 
>> +	if (ret)
>> +		goto free_as;
>> +
>>   	as->pd = alloc_page(GFP_KERNEL | __GFP_DMA | __GFP_ZERO);
>> -	if (!as->pd) {
>> -		kfree(as);
>> -		return NULL;
>> -	}
>> +	if (!as->pd)
>> +		goto put_dma_cookie;
>>   
>>   	as->count = kcalloc(SMMU_NUM_PDE, sizeof(u32), GFP_KERNEL);
>> -	if (!as->count) {
>> -		__free_page(as->pd);
>> -		kfree(as);
>> -		return NULL;
>> -	}
>> +	if (!as->count)
>> +		goto free_pd_range;
>>   
>>   	as->pts = kcalloc(SMMU_NUM_PDE, sizeof(*as->pts), GFP_KERNEL);
>> -	if (!as->pts) {
>> -		kfree(as->count);
>> -		__free_page(as->pd);
>> -		kfree(as);
>> -		return NULL;
>> -	}
>> +	if (!as->pts)
>> +		goto free_pts;
>>   
>>   	/* setup aperture */
>>   	as->domain.geometry.aperture_start = 0;
>> @@ -308,6 +307,18 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
>>   	as->domain.geometry.force_aperture = true;
>>   
>>   	return &as->domain;
>> +
>> +free_pts:
>> +	kfree(as->pts);
>> +free_pd_range:
>> +	__free_page(as->pd);
>> +put_dma_cookie:
>> +	if (type == IOMMU_DOMAIN_DMA)

FWIW you don't strictly need that check - since domain->iova_cookie 
won't be set for other domain types anyway, iommu_put_dma_cookie() will 
simply ignore them.

>> +		iommu_put_dma_cookie(&as->domain);
>> +free_as:
>> +	kfree(as);
>> +
>> +	return NULL;
>>   }
>>   
>>   static void tegra_smmu_domain_free(struct iommu_domain *domain)

How about not leaking the cookie when you free a DMA ops domain?

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