Re: [v2 4/6] xen/iommu: smmu-v3: Add Xen specific code to enable the ported driver

[Sameer Goel <sameer.goel@linaro.org>]

On 5/31/2018 11:16 PM, Manish Jaggi wrote:
>
>
> On 05/31/2018 09:27 PM, Sameer Goel wrote:
>>
>> On 5/30/2018 10:13 PM, Manish Jaggi wrote:
>>>
>>> On 05/31/2018 04:31 AM, Sameer Goel wrote:
>>>>>>>> +
>>>>>>>> +static int arm_smmu_iommu_domain_init(struct domain *d)
>>>>>>> Where is iommu_domain initialized?
>>>>>>> The function does not use a iommu_domain * variable
>>>> Please check iommu.c 2 levels up.
>>> In this function do you see iommu_domain getting allocated or initialized?
>>> As per the name of function arm_smmu iommu_domain_init.
>>> Where is init of iommu_domain in this function?
>> Well without the xen_domain the iommu_domain is not initialized. It is just the default value. This generic iommu code makes an .init call to our code for whatever initialization is needed. So the name here seemed absolutely fine to me.
>>
>> Initialization does not always refer to allocation. In this case this is driver specific initialization. Since, the iommu code is making an init call to the smmu code hence the name arm_smmu_iommu_domain_init. So, again I agree with your comments on the domain variable names and I'm making these changes as they would make the code cleaner. This function name change probably will not do much but the move along the discussion, let me know what you were thinking.
> Sameer, few points
> a. all the functions are prefixed with arm_smmu_ , so what the function is doing can be understood by the rest part of the name
> In this case it is iommu_domain_init.
>
> b. By the name it seems to suggest that you are  doing some kind of init for iommu_domain
>
> c. But in this complete function, iommu_domain pointer is never used.
>
> If I take your point, the appropriate name of the function should be arm_smmu_xen_domain_init().
Its not the iommu domain defined within the current driver.  I'll change the name to arm_smmu_xen_iommu_init().

arm_smmu for just keeping the prefixes as needed. Sounds good?

Thanks,
Sameer
>
> -Manish
>>> +static int arm_smmu_iommu_domain_init(struct domain *d)
>>> +{
>>> +    struct arm_smmu_xen_domain *xen_domain;
>>> +
>>> +    xen_domain = xzalloc(struct arm_smmu_xen_domain);
>>> +    if (!xen_domain)
>>> +        return -ENOMEM;
>>> +
>>> +    spin_lock_init(&xen_domain->lock);
>>> +    INIT_LIST_HEAD(&xen_domain->contexts);
>>> +
>>> +    dom_iommu(d)->arch.priv = xen_domain;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>
>>>
>>>
>>>> Thanks,
>>>> Sameer
>>>>> _______________________________________________
>>>>> Xen-devel mailing list
>>>>> Xen-devel@lists.xenproject.org
>>>>> https://lists.xenproject.org/mailman/listinfo/xen-devel
>>>
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@lists.xenproject.org
>>> https://lists.xenproject.org/mailman/listinfo/xen-devel
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel