Re: [PATCH v6 20/25] iommu/sun50i: Add an IOMMU_IDENTITIY_DOMAIN

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Aug 14, 2023 at 02:44:50PM +0800, Baolu Lu wrote:
> On 2023/8/3 8:08, Jason Gunthorpe wrote:
> > Prior to commit 1b932ceddd19 ("iommu: Remove detach_dev callbacks") the
> > sun50i_iommu_detach_device() function was being called by
> > ops->detach_dev().
> > 
> > This is an IDENTITY domain so convert sun50i_iommu_detach_device() into
> > sun50i_iommu_identity_attach() and a full IDENTITY domain and thus hook it
> > back up the same was as the old ops->detach_dev().
> > 
> > Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>
> > ---
> >   drivers/iommu/sun50i-iommu.c | 26 +++++++++++++++++++-------
> >   1 file changed, 19 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
> > index 74c5cb93e90027..0bf08b120cf105 100644
> > --- a/drivers/iommu/sun50i-iommu.c
> > +++ b/drivers/iommu/sun50i-iommu.c
> > @@ -757,21 +757,32 @@ static void sun50i_iommu_detach_domain(struct sun50i_iommu *iommu,
> >   	iommu->domain = NULL;
> >   }
> > -static void sun50i_iommu_detach_device(struct iommu_domain *domain,
> > -				       struct device *dev)
> > +static int sun50i_iommu_identity_attach(struct iommu_domain *identity_domain,
> > +					struct device *dev)
> >   {
> > -	struct sun50i_iommu_domain *sun50i_domain = to_sun50i_domain(domain);
> >   	struct sun50i_iommu *iommu = dev_iommu_priv_get(dev);
> > +	struct sun50i_iommu_domain *sun50i_domain;
> >   	dev_dbg(dev, "Detaching from IOMMU domain\n");
> > -	if (iommu->domain != domain)
> > -		return;
> > +	if (iommu->domain == identity_domain)
> > +		return 0;
> > +	sun50i_domain = to_sun50i_domain(iommu->domain);
> >   	if (refcount_dec_and_test(&sun50i_domain->refcnt))
> >   		sun50i_iommu_detach_domain(iommu, sun50i_domain);
> > +	return 0;
> >   }
> 
> Does iommu->domain need to be set to identity_domain before returning?

sun50i_iommu_detach_domain() does it.

This driver is confused because it uses generic_single_device_group
but also has this weird refcounting stuff. It should just make the
first attach alter the HW and have the remaining ones (eg new domain
== current domani) be NOPs. It should not refcount.

Jason