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

[Manish Jaggi <mjaggi@xxxxxxxxxxxxxxxxxx>]

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().
-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@xxxxxxxxxxxxxxxxxxxx
> > > > https://lists.xenproject.org/mailman/listinfo/xen-devel
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@xxxxxxxxxxxxxxxxxxxx
> > https://lists.xenproject.org/mailman/listinfo/xen-devel

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