diff mbox series

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

Message ID 20-v6-e8114faedade+425-iommu_all_defdom_jgg@nvidia.com
State Superseded
Headers show
Series iommu: Make default_domain's mandatory | expand

Commit Message

Jason Gunthorpe Aug. 3, 2023, 12:08 a.m. UTC
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(-)

Comments

Baolu Lu Aug. 14, 2023, 6:44 a.m. UTC | #1
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?

Best regards,
baolu
Jason Gunthorpe Aug. 14, 2023, 3:39 p.m. UTC | #2
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
diff mbox series

Patch

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;
 }
 
+static struct iommu_domain_ops sun50i_iommu_identity_ops = {
+	.attach_dev = sun50i_iommu_identity_attach,
+};
+
+static struct iommu_domain sun50i_iommu_identity_domain = {
+	.type = IOMMU_DOMAIN_IDENTITY,
+	.ops = &sun50i_iommu_identity_ops,
+};
+
 static int sun50i_iommu_attach_device(struct iommu_domain *domain,
 				      struct device *dev)
 {
@@ -789,8 +800,7 @@  static int sun50i_iommu_attach_device(struct iommu_domain *domain,
 	if (iommu->domain == domain)
 		return 0;
 
-	if (iommu->domain)
-		sun50i_iommu_detach_device(iommu->domain, dev);
+	sun50i_iommu_identity_attach(&sun50i_iommu_identity_domain, dev);
 
 	sun50i_iommu_attach_domain(iommu, sun50i_domain);
 
@@ -827,6 +837,7 @@  static int sun50i_iommu_of_xlate(struct device *dev,
 }
 
 static const struct iommu_ops sun50i_iommu_ops = {
+	.identity_domain = &sun50i_iommu_identity_domain,
 	.pgsize_bitmap	= SZ_4K,
 	.device_group	= sun50i_iommu_device_group,
 	.domain_alloc	= sun50i_iommu_domain_alloc,
@@ -985,6 +996,7 @@  static int sun50i_iommu_probe(struct platform_device *pdev)
 	if (!iommu)
 		return -ENOMEM;
 	spin_lock_init(&iommu->iommu_lock);
+	iommu->domain = &sun50i_iommu_identity_domain;
 	platform_set_drvdata(pdev, iommu);
 	iommu->dev = &pdev->dev;