Thanks for the feedback, Robin, and, Dmitry.
I mostly agree with all your comments, pls see my responses inline.
I'll make these fixes in V2.

On 1/17/19 8:50 AM, Robin Murphy wrote:
> 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.

I'll fix it.

>>
>>> Signed-off-by: Navneet Kumar <navne...@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.

Agreed.

> 
>>
>>
>>>         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;
>>     }
>>

Agreed.

>>
>>> +    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.
> 

I'll still keep that check and not rely solely on the put_dma_cookie API to 
handle this case.
This will keep the code robust even if the put_dma_cookie API changes in future 
(for whatever reason).
It also looks like the canonical usage in other drivers.

>>> +        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?
> 

Agreed.

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

Reply via email to