iommu/exynos: Fix set_platform_dma_ops() callback

There are some subtle differences between release_device() and set_platform_dma_ops() callbacks, so separate those two callbacks. Device links should be removed only in release_device(), because they were created in probe_device() on purpose and they are needed for proper Exynos IOMMU driver operation. While fixing this, remove the conditional code as it is not really needed.

Reported-by Jason Gunthorpe <[email protected]>

Fixes 189d496b48b1 ("iommu/exynos: Add missing set_platform_dma_ops callback")

Signed-off-by Marek Szyprowski <[email protected]>

---

v2:
- keep set_platform_dma_ops only on ARM 32bit

Some more background why set_platform_dma_ops is needed on ARM 32bit is available here:
https://lore.kernel.org/all/[email protected]/

---
 drivers/iommu/exynos-iommu.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

drivers/iommu/exynos-iommu.c +10 -8

index 483aaaeb6dae..1abd187c6075 100644
@@ -1415,23 +1415,26 @@ static struct iommu_device *exynos_iommu_probe_device(struct device *dev)
        return &data->iommu;
 }
 
-static void exynos_iommu_release_device(struct device *dev)
+static void exynos_iommu_set_platform_dma(struct device *dev)
 {
        struct exynos_iommu_owner *owner = dev_iommu_priv_get(dev);
-       struct sysmmu_drvdata *data;
 
        if (owner->domain) {
                struct iommu_group *group = iommu_group_get(dev);
 
                if (group) {
-#ifndef CONFIG_ARM
-                       WARN_ON(owner->domain !=
-                               iommu_group_default_domain(group));
-#endif
                        exynos_iommu_detach_device(owner->domain, dev);
                        iommu_group_put(group);
                }
        }
+}
+
+static void exynos_iommu_release_device(struct device *dev)
+{
+       struct exynos_iommu_owner *owner = dev_iommu_priv_get(dev);
+       struct sysmmu_drvdata *data;
+
+       exynos_iommu_set_platform_dma(dev);
 
        list_for_each_entry(data, &owner->controllers, owner_node)
                device_link_del(data->link);

@@ -1479,7 +1482,7 @@ static const struct iommu_ops exynos_iommu_ops = {
        .domain_alloc = exynos_iommu_domain_alloc,
        .device_group = generic_device_group,
 #ifdef CONFIG_ARM
-       .set_platform_dma_ops = exynos_iommu_release_device,
+       .set_platform_dma_ops = exynos_iommu_set_platform_dma,
 #endif
        .probe_device = exynos_iommu_probe_device,
        .release_device = exynos_iommu_release_device,
--

2.34.1

There are some subtle differences between release_device() and set_platform_dma_ops() callbacks, so separate those two callbacks. Device links should be removed only in release_device(), because they were created in probe_device() on purpose and they are needed for proper Exynos IOMMU driver operation. While fixing this, remove the conditional code as it is not really needed.

Reported-by Jason Gunthorpe <[email protected]>

Fixes 189d496b48b1 ("iommu/exynos: Add missing set_platform_dma_ops callback")

Signed-off-by Marek Szyprowski <[email protected]>

---
v2:
- keep set_platform_dma_ops only on ARM 32bit

Some more background why set_platform_dma_ops is needed on ARM 32bit is available here:
https://lore.kernel.org/all/[email protected]/

---
 drivers/iommu/exynos-iommu.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

Quoted 14 lines · Marek Szyprowski

There are some subtle differences between release_device() and set_platform_dma_ops() callbacks, so separate those two callbacks. Device links should be removed only in release_device(), because they were created in probe_device() on purpose and they are needed for proper Exynos IOMMU driver operation. While fixing this, remove the conditional code as it is not really needed.

Reported-by Jason Gunthorpe <[email protected]>

Fixes 189d496b48b1 ("iommu/exynos: Add missing set_platform_dma_ops callback")

Signed-off-by Marek Szyprowski <[email protected]>

---
v2:
- keep set_platform_dma_ops only on ARM 32bit

Quoted 6 lines

Some more background why set_platform_dma_ops is needed on ARM 32bit is available here:
https://lore.kernel.org/all/[email protected]/

---
  drivers/iommu/exynos-iommu.c | 17 ++++++++++-------
  1 file changed, 10 insertions(+), 7 deletions(-)

It seems OK, but do you know what state the device is left in after exynos_iommu_detach_device ? Ie is it blocking or identity?

Quoted 21 lines · Jason Gunthorpe

Quoted 20 lines · Marek Szyprowski

There are some subtle differences between release_device() and set_platform_dma_ops() callbacks, so separate those two callbacks. Device links should be removed only in release_device(), because they were created in probe_device() on purpose and they are needed for proper Exynos IOMMU driver operation. While fixing this, remove the conditional code as it is not really needed.

Reported-by Jason Gunthorpe <[email protected]>

Fixes 189d496b48b1 ("iommu/exynos: Add missing set_platform_dma_ops callback")

Signed-off-by Marek Szyprowski <[email protected]>

---
v2:
- keep set_platform_dma_ops only on ARM 32bit

Some more background why set_platform_dma_ops is needed on ARM 32bit is available here:
https://lore.kernel.org/all/[email protected]/

---
  drivers/iommu/exynos-iommu.c | 17 ++++++++++-------
  1 file changed, 10 insertions(+), 7 deletions(-)

Quoted 2 lines

It seems OK, but do you know what state the device is left in after exynos_iommu_detach_device ? Ie is it blocking or identity?

identity

Quoted 22 lines · Marek Szyprowski

Quoted 21 lines · Jason Gunthorpe

Quoted 20 lines · Marek Szyprowski

There are some subtle differences between release_device() and set_platform_dma_ops() callbacks, so separate those two callbacks. Device links should be removed only in release_device(), because they were created in probe_device() on purpose and they are needed for proper Exynos IOMMU driver operation. While fixing this, remove the conditional code as it is not really needed.

Reported-by Jason Gunthorpe <[email protected]>

Fixes 189d496b48b1 ("iommu/exynos: Add missing set_platform_dma_ops callback")

Signed-off-by Marek Szyprowski <[email protected]>

---
v2:
- keep set_platform_dma_ops only on ARM 32bit

Some more background why set_platform_dma_ops is needed on ARM 32bit is available here:
https://lore.kernel.org/all/[email protected]/

---
   drivers/iommu/exynos-iommu.c | 17 ++++++++++-------
   1 file changed, 10 insertions(+), 7 deletions(-)

Quoted 3 lines

Quoted 2 lines

It seems OK, but do you know what state the device is left in after exynos_iommu_detach_device ? Ie is it blocking or identity?

identity

Can you do this cleanup like this instead?

Jason

drivers/iommu/exynos-iommu.c +39 -24

index 483aaaeb6daeac..2c2b5cba191459 100644
@@ -24,6 +24,7 @@
 
  typedef u32 sysmmu_iova_t;
  typedef u32 sysmmu_pte_t;
+static struct iommu_domain exynos_identity_domain;
 
  /* We do not consider super section mapping (16MB) */
  #define SECT_ORDER 20

@@ -900,6 +901,9 @@ static struct iommu_domain *exynos_iommu_domain_alloc(unsigned type)
  	dma_addr_t handle;
  	int i;
 
+	if (type == IOMMU_DOMAIN_IDENTITY)
+		return &exynos_identity_domain;
+
  	/* Check if correct PTE offsets are initialized */
  	BUG_ON(PG_ENT_SHIFT < 0 || !dma_dev);
 

@@ -988,17 +992,22 @@ static void exynos_iommu_domain_free(struct iommu_domain *iommu_domain)
  	kfree(domain);
  }
 
-static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain,
-				    struct device *dev)
+static int exynos_iommu_identity_attach(struct iommu_domain *identity_domain,
+					struct device *dev)
  {
-	struct exynos_iommu_domain *domain = to_exynos_domain(iommu_domain);
  	struct exynos_iommu_owner *owner = dev_iommu_priv_get(dev);
-	phys_addr_t pagetable = virt_to_phys(domain->pgtable);
+	struct exynos_iommu_domain *domain;
+	phys_addr_t pagetable;
  	struct sysmmu_drvdata *data, *next;
  	unsigned long flags;
 
-	if (!has_sysmmu(dev) || owner->domain != iommu_domain)
-		return;
+	if (!owner)
+		return -ENODEV;
+	if (owner->domain == identity_domain)
+		return 0;
+
+	domain = to_exynos_domain(owner->domain);
+	pagetable = virt_to_phys(domain->pgtable);
 
  	mutex_lock(&owner->rpm_lock);
 

@@ -1017,15 +1026,32 @@ static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain,
  		list_del_init(&data->domain_node);
  		spin_unlock(&data->lock);
  	}
-	owner->domain = NULL;
+	owner->domain = identity_domain;
  	spin_unlock_irqrestore(&domain->lock, flags);
 
  	mutex_unlock(&owner->rpm_lock);
 
  	dev_dbg(dev, "%s: Detached IOMMU with pgtable %pa\n", __func__,
  		&pagetable);
+	return 0;
  }
 
+static struct iommu_domain_ops exynos_identity_ops = {
+	.attach_dev = exynos_iommu_identity_attach,
+};
+
+static struct iommu_domain exynos_identity_domain = {
+	.type = IOMMU_DOMAIN_IDENTITY,
+	.ops = &exynos_identity_ops,
+};
+
+#ifdef CONFIG_ARM
+static void exynos_iommu_set_platform_dma(struct device *dev)
+{
+	WARN_ON(exynos_iommu_identity_attach(&exynos_identity_domain, dev));
+}
+#endif
+
  static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
  				   struct device *dev)
  {

@@ -1034,12 +1060,11 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
  	struct sysmmu_drvdata *data;
  	phys_addr_t pagetable = virt_to_phys(domain->pgtable);
  	unsigned long flags;
+	int err;
 
-	if (!has_sysmmu(dev))
-		return -ENODEV;
-
-	if (owner->domain)
-		exynos_iommu_detach_device(owner->domain, dev);
+	err = exynos_iommu_identity_attach(&exynos_identity_domain, dev);
+	if (err)
+		return err;
 
  	mutex_lock(&owner->rpm_lock);
 

@@ -1420,18 +1445,7 @@ static void exynos_iommu_release_device(struct device *dev)
  	struct exynos_iommu_owner *owner = dev_iommu_priv_get(dev);
  	struct sysmmu_drvdata *data;
 
-	if (owner->domain) {
-		struct iommu_group *group = iommu_group_get(dev);
-
-		if (group) {
-#ifndef CONFIG_ARM
-			WARN_ON(owner->domain !=
-				iommu_group_default_domain(group));
-#endif
-			exynos_iommu_detach_device(owner->domain, dev);
-			iommu_group_put(group);
-		}
-	}
+	WARN_ON(exynos_iommu_identity_attach(&exynos_identity_domain, dev));
 
  	list_for_each_entry(data, &owner->controllers, owner_node)
  		device_link_del(data->link);

@@ -1462,6 +1476,7 @@ static int exynos_iommu_of_xlate(struct device *dev,
 
  		INIT_LIST_HEAD(&owner->controllers);
  		mutex_init(&owner->rpm_lock);
+		owner->domain = &exynos_identity_domain;
  		dev_iommu_priv_set(dev, owner);
  	}
 

Hi Jason,

Quoted 25 lines · Jason Gunthorpe

Quoted 24 lines · Marek Szyprowski

Quoted 23 lines · Jason Gunthorpe

Quoted 14 lines · Marek Szyprowski

There are some subtle differences between release_device() and set_platform_dma_ops() callbacks, so separate those two callbacks. Device links should be removed only in release_device(), because they were created in probe_device() on purpose and they are needed for proper Exynos IOMMU driver operation. While fixing this, remove the conditional code as it is not really needed.

Reported-by Jason Gunthorpe <[email protected]>

Fixes 189d496b48b1 ("iommu/exynos: Add missing set_platform_dma_ops callback")

Signed-off-by Marek Szyprowski <[email protected]>

---
v2:
- keep set_platform_dma_ops only on ARM 32bit

Quoted 6 lines

Some more background why set_platform_dma_ops is needed on ARM 32bit is available here:
https://lore.kernel.org/all/[email protected]/

---
   drivers/iommu/exynos-iommu.c | 17 ++++++++++-------
   1 file changed, 10 insertions(+), 7 deletions(-)

It seems OK, but do you know what state the device is left in after exynos_iommu_detach_device ? Ie is it blocking or identity?

Quoted 2 lines

Quoted 1 line

identity

Can you do this cleanup like this instead?

Frankly speaking I would like to have a minimal fix (like my $subject patch) applied to v6.3-rcX ASAP and then apply this identity domain support on top of than for -next (6.4). I've checked and your proposed patch works fine in my case, so You can send it as formal patch.

Frankly speaking I would like to have a minimal fix (like my $subject patch) applied to v6.3-rcX ASAP and then apply this identity domain support on top of than for -next (6.4). I've checked and your proposed patch works fine in my case, so You can send it as formal patch.

Joerg: is that okay for you?

Quoted 6 lines · Marek Szyprowski

Frankly speaking I would like to have a minimal fix (like my $subject patch) applied to v6.3-rcX ASAP and then apply this identity domain support on top of than for -next (6.4). I've checked and your proposed patch works fine in my case, so You can send it as formal patch.

Joerg: is that okay for you?

Agreed, a minimal fix for 6.3 would be great.

 drivers/iommu/exynos-iommu.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)