[PATCH v4 00/19] iommu: Retire detach_dev callback

[PATCH v4 00/19] iommu: Retire detach_dev callback

[Lu Baolu]

Hi folks,

The iommu core calls the driver's detach_dev domain op callback only when
a device is finished assigning to user space and
iommu_group_release_dma_owner() is called to return the device to the
kernel, where iommu core wants to set the default domain to the device but
the driver didn't provide one. The code looks like:

        /*
         * New drivers should support default domains and so the detach_dev() op
         * will never be called. Otherwise the NULL domain represents some
         * platform specific behavior.
         */
        if (!new_domain) {
                if (WARN_ON(!group->domain->ops->detach_dev))
                        return -EINVAL;
                __iommu_group_for_each_dev(group, group->domain,
                                           iommu_group_do_detach_device);
                group->domain = NULL;
                return 0;
        }

In other words, if the iommu driver provides default domains, the
.detach_dev callback will never be called; Otherwise, the .detach_dev
callback is actually called to return control back to the platform DMA
ops, other than detaching the domain from device.

This series cleanups this by:

- If the IOMMU driver provides default domains, remove .detach_dev
  callback.
- Adds a new set_platform_dma iommu op. Any IOMMU driver that doesn't
  provide default domain should implement set_platform_dma callback
  instead.
- Retire .detach_dev callback.

This series originates from various discussion in the community. Thank
Jason, Robin and all others for their ideas. This series looks very
long. It is necessary because I hope that change in each driver has a
confirmation from its stakeholder. I will then be able to merge some of
them to make the series cute.

The whole series is available on github:
https://github.com/LuBaolu/intel-iommu/commits/iommu-retire-detach_dev-v4

Please review and suggest.

Change log:
v4:
 - Drop the patch which renamed .attach_dev to .set_dev. As Robin said,
   "... I also wouldn't say that "attach" loses its meaning in a context
   where an equivalent "detach" operation is only ever implicit in 
   reattaching to something else...". If we have a better name in the
   future, we can do it in other series.
 - Adjust the patch of "iommu: Add set_platform_dma_ops iommu ops"
   according to Jason's following suggestion " ... This is a bit ugly,
   it would be better to make the previous patch call set_platform_dma
   if it is set instead of detach_dev and then these patches should just
   rename the driver's fsl_pamu_detach_device to
   fsl_pamu_set_platform_dma ..."
 - Add a new patch to remove deferred attach check from
   __iommu_detach_domain() path. Make it a separate patch as the
   prerequisite to remove __iommu_detach_device() helper.
 - Rename set_platform_dma to set_platform_dma_ops to make it more
   meaningful.

v3:
 - https://lore.kernel.org/linux-iommu/20221128064648.1934720-1-baolu.lu@linux.intel.com/
 - Setting blocking domain is not conceptually equal to detach_dev.
   Dropped all blocking domain related staffs in the previous version.

v2:
 - https://lore.kernel.org/linux-iommu/20220826123014.52709-1-baolu.lu@linux.intel.com/
 - Replace .detach_dev callback with static block domain ops;
 - Rename .attach_dev to .set_dev.

v1:
 - https://lore.kernel.org/linux-iommu/20220516015759.2952771-1-baolu.lu@linux.intel.com/

Jason Gunthorpe (1):
  iommu: Remove deferred attach check from __iommu_detach_device()

Lu Baolu (18):
  iommu/amd: Remove detach_dev callback
  iommu/apple-dart: Remove detach_dev callback
  iommu/qcom: Remove detach_dev callback
  iommu/exynos: Remove detach_dev callback
  iommu/ipmmu: Remove detach_dev callback
  iommu/mtk: Remove detach_dev callback
  iommu/rockchip: Remove detach_dev callback
  iommu/sprd: Remove detach_dev callback
  iommu/sun50i: Remove detach_dev callback
  iommu: Add set_platform_dma_ops iommu ops
  iommu/fsl_pamu: Add set_platform_dma_ops callback
  iommu/msm: Add set_platform_dma_ops callback
  iommu/mtk_v1: Add set_platform_dma_ops callback
  iommu/omap: Add set_platform_dma_ops callback
  iommu/s390: Add set_platform_dma_ops callback
  iommu/gart: Add set_platform_dma_ops callback
  iommu/tegra: Add set_platform_dma_ops callback
  iommu: Remove detach_dev callback

 include/linux/iommu.h                   |  8 +-
 include/trace/events/iommu.h            |  7 --
 drivers/iommu/amd/iommu.c               | 26 -------
 drivers/iommu/apple-dart.c              | 24 ------
 drivers/iommu/arm/arm-smmu/qcom_iommu.c | 23 ------
 drivers/iommu/exynos-iommu.c            |  1 -
 drivers/iommu/fsl_pamu_domain.c         |  6 +-
 drivers/iommu/iommu-traces.c            |  1 -
 drivers/iommu/iommu.c                   | 98 ++++++++++++-------------
 drivers/iommu/ipmmu-vmsa.c              | 16 ----
 drivers/iommu/msm_iommu.c               |  6 +-
 drivers/iommu/mtk_iommu.c               |  9 ---
 drivers/iommu/mtk_iommu_v1.c            |  4 +-
 drivers/iommu/omap-iommu.c              |  6 +-
 drivers/iommu/rockchip-iommu.c          |  1 -
 drivers/iommu/s390-iommu.c              |  7 +-
 drivers/iommu/sprd-iommu.c              | 16 ----
 drivers/iommu/sun50i-iommu.c            |  1 -
 drivers/iommu/tegra-gart.c              |  6 +-
 drivers/iommu/tegra-smmu.c              |  5 +-
 20 files changed, 73 insertions(+), 198 deletions(-)

-- 
2.34.1

[PATCH v4 01/19] iommu/amd: Remove detach_dev callback

[Lu Baolu]

The AMD IOMMU supports default domain, so the detach_dev op will never
be called. Remove it to avoid dead code.

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/amd/iommu.c | 26 --------------------------
 1 file changed, 26 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index cbeaab55c0db..92319c9b877c 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2126,31 +2126,6 @@ static void amd_iommu_domain_free(struct iommu_domain *dom)
 	protection_domain_free(domain);
 }
 
-static void amd_iommu_detach_device(struct iommu_domain *dom,
-				    struct device *dev)
-{
-	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
-	struct amd_iommu *iommu;
-
-	if (!check_device(dev))
-		return;
-
-	if (dev_data->domain != NULL)
-		detach_device(dev);
-
-	iommu = rlookup_amd_iommu(dev);
-	if (!iommu)
-		return;
-
-#ifdef CONFIG_IRQ_REMAP
-	if (AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) &&
-	    (dom->type == IOMMU_DOMAIN_UNMANAGED))
-		dev_data->use_vapic = 0;
-#endif
-
-	iommu_completion_wait(iommu);
-}
-
 static int amd_iommu_attach_device(struct iommu_domain *dom,
 				   struct device *dev)
 {
@@ -2416,7 +2391,6 @@ const struct iommu_ops amd_iommu_ops = {
 	.def_domain_type = amd_iommu_def_domain_type,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= amd_iommu_attach_device,
-		.detach_dev	= amd_iommu_detach_device,
 		.map_pages	= amd_iommu_map_pages,
 		.unmap_pages	= amd_iommu_unmap_pages,
 		.iotlb_sync_map	= amd_iommu_iotlb_sync_map,
-- 
2.34.1

[PATCH v4 02/19] iommu/apple-dart: Remove detach_dev callback

[Lu Baolu]

The IOMMU driver supports default domain, so the detach_dev op will never
be called. Remove it to avoid dead code.

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Sven Peter <sven@svenpeter.dev>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/apple-dart.c | 24 ------------------------
 1 file changed, 24 deletions(-)

diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
index 4f4a323be0d0..96843d468801 100644
--- a/drivers/iommu/apple-dart.c
+++ b/drivers/iommu/apple-dart.c
@@ -486,13 +486,6 @@ static int apple_dart_domain_add_streams(struct apple_dart_domain *domain,
 				      true);
 }
 
-static int apple_dart_domain_remove_streams(struct apple_dart_domain *domain,
-					    struct apple_dart_master_cfg *cfg)
-{
-	return apple_dart_mod_streams(domain->stream_maps, cfg->stream_maps,
-				      false);
-}
-
 static int apple_dart_attach_dev(struct iommu_domain *domain,
 				 struct device *dev)
 {
@@ -535,22 +528,6 @@ static int apple_dart_attach_dev(struct iommu_domain *domain,
 	return ret;
 }
 
-static void apple_dart_detach_dev(struct iommu_domain *domain,
-				  struct device *dev)
-{
-	int i;
-	struct apple_dart_stream_map *stream_map;
-	struct apple_dart_master_cfg *cfg = dev_iommu_priv_get(dev);
-	struct apple_dart_domain *dart_domain = to_dart_domain(domain);
-
-	for_each_stream_map(i, cfg, stream_map)
-		apple_dart_hw_disable_dma(stream_map);
-
-	if (domain->type == IOMMU_DOMAIN_DMA ||
-	    domain->type == IOMMU_DOMAIN_UNMANAGED)
-		apple_dart_domain_remove_streams(dart_domain, cfg);
-}
-
 static struct iommu_device *apple_dart_probe_device(struct device *dev)
 {
 	struct apple_dart_master_cfg *cfg = dev_iommu_priv_get(dev);
@@ -780,7 +757,6 @@ static const struct iommu_ops apple_dart_iommu_ops = {
 	.owner = THIS_MODULE,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= apple_dart_attach_dev,
-		.detach_dev	= apple_dart_detach_dev,
 		.map_pages	= apple_dart_map_pages,
 		.unmap_pages	= apple_dart_unmap_pages,
 		.flush_iotlb_all = apple_dart_flush_iotlb_all,
-- 
2.34.1

[PATCH v4 03/19] iommu/qcom: Remove detach_dev callback

[Lu Baolu]

The IOMMU driver supports default domain, so the detach_dev op will never
be called. Remove it to avoid dead code.

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/arm/arm-smmu/qcom_iommu.c | 23 -----------------------
 1 file changed, 23 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index 270c3d9128ba..d7be3adee426 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -387,28 +387,6 @@ static int qcom_iommu_attach_dev(struct iommu_domain *domain, struct device *dev
 	return 0;
 }
 
-static void qcom_iommu_detach_dev(struct iommu_domain *domain, struct device *dev)
-{
-	struct qcom_iommu_domain *qcom_domain = to_qcom_iommu_domain(domain);
-	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
-	struct qcom_iommu_dev *qcom_iommu = to_iommu(dev);
-	unsigned i;
-
-	if (WARN_ON(!qcom_domain->iommu))
-		return;
-
-	pm_runtime_get_sync(qcom_iommu->dev);
-	for (i = 0; i < fwspec->num_ids; i++) {
-		struct qcom_iommu_ctx *ctx = to_ctx(qcom_domain, fwspec->ids[i]);
-
-		/* Disable the context bank: */
-		iommu_writel(ctx, ARM_SMMU_CB_SCTLR, 0);
-
-		ctx->domain = NULL;
-	}
-	pm_runtime_put_sync(qcom_iommu->dev);
-}
-
 static int qcom_iommu_map(struct iommu_domain *domain, unsigned long iova,
 			  phys_addr_t paddr, size_t pgsize, size_t pgcount,
 			  int prot, gfp_t gfp, size_t *mapped)
@@ -583,7 +561,6 @@ static const struct iommu_ops qcom_iommu_ops = {
 	.pgsize_bitmap	= SZ_4K | SZ_64K | SZ_1M | SZ_16M,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= qcom_iommu_attach_dev,
-		.detach_dev	= qcom_iommu_detach_dev,
 		.map_pages	= qcom_iommu_map,
 		.unmap_pages	= qcom_iommu_unmap,
 		.flush_iotlb_all = qcom_iommu_flush_iotlb_all,
-- 
2.34.1

[PATCH v4 04/19] iommu/exynos: Remove detach_dev callback

[Lu Baolu]

The IOMMU driver supports default domain, so the detach_dev op will never
be called. Remove it to avoid dead code.

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/exynos-iommu.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index b0cde2211987..29ec713e8a21 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -1404,7 +1404,6 @@ static const struct iommu_ops exynos_iommu_ops = {
 	.of_xlate = exynos_iommu_of_xlate,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= exynos_iommu_attach_device,
-		.detach_dev	= exynos_iommu_detach_device,
 		.map		= exynos_iommu_map,
 		.unmap		= exynos_iommu_unmap,
 		.iova_to_phys	= exynos_iommu_iova_to_phys,
-- 
2.34.1

[PATCH v4 05/19] iommu/ipmmu: Remove detach_dev callback

[Lu Baolu]

The IOMMU driver supports default domain, so the detach_dev op will never
be called. Remove it to avoid dead code.

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/ipmmu-vmsa.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index a003bd5fc65c..3112822ac7be 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -643,21 +643,6 @@ static int ipmmu_attach_device(struct iommu_domain *io_domain,
 	return 0;
 }
 
-static void ipmmu_detach_device(struct iommu_domain *io_domain,
-				struct device *dev)
-{
-	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
-	struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
-	unsigned int i;
-
-	for (i = 0; i < fwspec->num_ids; ++i)
-		ipmmu_utlb_disable(domain, fwspec->ids[i]);
-
-	/*
-	 * TODO: Optimize by disabling the context when no device is attached.
-	 */
-}
-
 static int ipmmu_map(struct iommu_domain *io_domain, unsigned long iova,
 		     phys_addr_t paddr, size_t pgsize, size_t pgcount,
 		     int prot, gfp_t gfp, size_t *mapped)
@@ -876,7 +861,6 @@ static const struct iommu_ops ipmmu_ops = {
 	.of_xlate = ipmmu_of_xlate,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= ipmmu_attach_device,
-		.detach_dev	= ipmmu_detach_device,
 		.map_pages	= ipmmu_map,
 		.unmap_pages	= ipmmu_unmap,
 		.flush_iotlb_all = ipmmu_flush_iotlb_all,
-- 
2.34.1

[PATCH v4 06/19] iommu/mtk: Remove detach_dev callback

[Lu Baolu]

The IOMMU driver supports default domain, so the detach_dev op will never
be called. Remove it to avoid dead code.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/mtk_iommu.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 2badd6acfb23..d5a4955910ff 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -710,14 +710,6 @@ static int mtk_iommu_attach_device(struct iommu_domain *domain,
 	return ret;
 }
 
-static void mtk_iommu_detach_device(struct iommu_domain *domain,
-				    struct device *dev)
-{
-	struct mtk_iommu_data *data = dev_iommu_priv_get(dev);
-
-	mtk_iommu_config(data, dev, false, 0);
-}
-
 static int mtk_iommu_map(struct iommu_domain *domain, unsigned long iova,
 			 phys_addr_t paddr, size_t pgsize, size_t pgcount,
 			 int prot, gfp_t gfp, size_t *mapped)
@@ -946,7 +938,6 @@ static const struct iommu_ops mtk_iommu_ops = {
 	.owner		= THIS_MODULE,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= mtk_iommu_attach_device,
-		.detach_dev	= mtk_iommu_detach_device,
 		.map_pages	= mtk_iommu_map,
 		.unmap_pages	= mtk_iommu_unmap,
 		.flush_iotlb_all = mtk_iommu_flush_iotlb_all,
-- 
2.34.1

[PATCH v4 07/19] iommu/rockchip: Remove detach_dev callback

[Lu Baolu]

The IOMMU driver supports default domain, so the detach_dev op will never
be called. Remove it to avoid dead code.

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/rockchip-iommu.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index a68eadd64f38..f30db22ea5d7 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -1192,7 +1192,6 @@ static const struct iommu_ops rk_iommu_ops = {
 	.of_xlate = rk_iommu_of_xlate,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= rk_iommu_attach_device,
-		.detach_dev	= rk_iommu_detach_device,
 		.map		= rk_iommu_map,
 		.unmap		= rk_iommu_unmap,
 		.iova_to_phys	= rk_iommu_iova_to_phys,
-- 
2.34.1

[PATCH v4 08/19] iommu/sprd: Remove detach_dev callback

[Lu Baolu]

The IOMMU driver supports default domain, so the detach_dev op will never
be called. Remove it to avoid dead code.

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Acked-by: Chunyan Zhang <zhang.lyra@gmail.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/sprd-iommu.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/drivers/iommu/sprd-iommu.c b/drivers/iommu/sprd-iommu.c
index 219bfa11f7f4..ae94d74b73f4 100644
--- a/drivers/iommu/sprd-iommu.c
+++ b/drivers/iommu/sprd-iommu.c
@@ -255,21 +255,6 @@ static int sprd_iommu_attach_device(struct iommu_domain *domain,
 	return 0;
 }
 
-static void sprd_iommu_detach_device(struct iommu_domain *domain,
-					     struct device *dev)
-{
-	struct sprd_iommu_domain *dom = to_sprd_domain(domain);
-	struct sprd_iommu_device *sdev = dom->sdev;
-	size_t pgt_size = sprd_iommu_pgt_size(domain);
-
-	if (!sdev)
-		return;
-
-	dma_free_coherent(sdev->dev, pgt_size, dom->pgt_va, dom->pgt_pa);
-	sprd_iommu_hw_en(sdev, false);
-	dom->sdev = NULL;
-}
-
 static int sprd_iommu_map(struct iommu_domain *domain, unsigned long iova,
 			  phys_addr_t paddr, size_t pgsize, size_t pgcount,
 			  int prot, gfp_t gfp, size_t *mapped)
@@ -414,7 +399,6 @@ static const struct iommu_ops sprd_iommu_ops = {
 	.owner		= THIS_MODULE,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= sprd_iommu_attach_device,
-		.detach_dev	= sprd_iommu_detach_device,
 		.map_pages	= sprd_iommu_map,
 		.unmap_pages	= sprd_iommu_unmap,
 		.iotlb_sync_map	= sprd_iommu_sync_map,
-- 
2.34.1

[PATCH v4 09/19] iommu/sun50i: Remove detach_dev callback

[Lu Baolu]

The IOMMU driver supports default domain, so the detach_dev op will never
be called. Remove it to avoid dead code.

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/sun50i-iommu.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
index 5b585eace3d4..2d993d0cea7d 100644
--- a/drivers/iommu/sun50i-iommu.c
+++ b/drivers/iommu/sun50i-iommu.c
@@ -834,7 +834,6 @@ static const struct iommu_ops sun50i_iommu_ops = {
 	.probe_device	= sun50i_iommu_probe_device,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= sun50i_iommu_attach_device,
-		.detach_dev	= sun50i_iommu_detach_device,
 		.flush_iotlb_all = sun50i_iommu_flush_iotlb_all,
 		.iotlb_sync_map = sun50i_iommu_iotlb_sync_map,
 		.iotlb_sync	= sun50i_iommu_iotlb_sync,
-- 
2.34.1

[PATCH v4 10/19] iommu: Add set_platform_dma_ops iommu ops

[Lu Baolu]

When VFIO finishes assigning a device to user space and calls
iommu_group_release_dma_owner() to return the device to kernel, the IOMMU
core will attach the default domain to the device. Unfortunately, some
IOMMU drivers don't support default domain, hence in the end, the core
calls .detach_dev instead.

This adds set_platform_dma_ops iommu ops to make it clear that what it
does is returning control back to the platform DMA ops.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 include/linux/iommu.h |  4 ++++
 drivers/iommu/iommu.c | 23 +++++++++++++++++++----
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 46e1347bfa22..7b3e3775b069 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -228,6 +228,9 @@ struct iommu_iotlb_gather {
  * @release_device: Remove device from iommu driver handling
  * @probe_finalize: Do final setup work after the device is added to an IOMMU
  *                  group and attached to the groups domain
+ * @set_platform_dma_ops: Returning control back to the platform DMA ops. This op
+ *                        is to support old IOMMU drivers, new drivers should use
+ *                        default domains, and the common IOMMU DMA ops.
  * @device_group: find iommu group for a particular device
  * @get_resv_regions: Request list of reserved regions for a device
  * @of_xlate: add OF master IDs to iommu grouping
@@ -256,6 +259,7 @@ struct iommu_ops {
 	struct iommu_device *(*probe_device)(struct device *dev);
 	void (*release_device)(struct device *dev);
 	void (*probe_finalize)(struct device *dev);
+	void (*set_platform_dma_ops)(struct device *dev);
 	struct iommu_group *(*device_group)(struct device *dev);
 
 	/* Request/Free a list of reserved regions for a device */
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index de91dd88705b..4e35a9f94873 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2163,6 +2163,17 @@ static int iommu_group_do_detach_device(struct device *dev, void *data)
 	return 0;
 }
 
+static int iommu_group_do_set_platform_dma(struct device *dev, void *data)
+{
+	const struct iommu_ops *ops = dev_iommu_ops(dev);
+
+	if (!ops->set_platform_dma_ops)
+		return -EINVAL;
+
+	ops->set_platform_dma_ops(dev);
+	return 0;
+}
+
 static int __iommu_group_set_domain(struct iommu_group *group,
 				    struct iommu_domain *new_domain)
 {
@@ -2177,10 +2188,14 @@ static int __iommu_group_set_domain(struct iommu_group *group,
 	 * platform specific behavior.
 	 */
 	if (!new_domain) {
-		if (WARN_ON(!group->domain->ops->detach_dev))
-			return -EINVAL;
-		__iommu_group_for_each_dev(group, group->domain,
-					   iommu_group_do_detach_device);
+		ret = __iommu_group_for_each_dev(group, NULL,
+				iommu_group_do_set_platform_dma);
+		if (ret) {
+			if (WARN_ON(!group->domain->ops->detach_dev))
+				return -EINVAL;
+			__iommu_group_for_each_dev(group, group->domain,
+				iommu_group_do_detach_device);
+		}
 		group->domain = NULL;
 		return 0;
 	}
-- 
2.34.1

[PATCH v4 11/19] iommu/fsl_pamu: Add set_platform_dma_ops callback

[Lu Baolu]

This IOMMU driver doesn't support default domain. Add the implementation
of set_platform_dma_ops callback so that the IOMMU core could return the
DMA control. With the set_platform_dma_ops implemented, there's no need
for detach_dev. Remove it to avoid dead code.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/fsl_pamu_domain.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/fsl_pamu_domain.c b/drivers/iommu/fsl_pamu_domain.c
index 4408ac3c49b6..e123161c211a 100644
--- a/drivers/iommu/fsl_pamu_domain.c
+++ b/drivers/iommu/fsl_pamu_domain.c
@@ -283,9 +283,9 @@ static int fsl_pamu_attach_device(struct iommu_domain *domain,
 	return ret;
 }
 
-static void fsl_pamu_detach_device(struct iommu_domain *domain,
-				   struct device *dev)
+static void fsl_pamu_set_platform_dma(struct device *dev)
 {
+	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
 	struct fsl_dma_domain *dma_domain = to_fsl_dma_domain(domain);
 	const u32 *prop;
 	int len;
@@ -452,9 +452,9 @@ static const struct iommu_ops fsl_pamu_ops = {
 	.domain_alloc	= fsl_pamu_domain_alloc,
 	.probe_device	= fsl_pamu_probe_device,
 	.device_group   = fsl_pamu_device_group,
+	.set_platform_dma_ops = fsl_pamu_set_platform_dma;
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= fsl_pamu_attach_device,
-		.detach_dev	= fsl_pamu_detach_device,
 		.iova_to_phys	= fsl_pamu_iova_to_phys,
 		.free		= fsl_pamu_domain_free,
 	}
-- 
2.34.1

[PATCH v4 12/19] iommu/msm: Add set_platform_dma_ops callback

[Lu Baolu]

This IOMMU driver doesn't support default domain. Add the implementation
of set_platform_dma_ops callback so that the IOMMU core could return the
DMA control. With the set_platform_dma_ops implemented, there's no need
for detach_dev. Remove it to avoid dead code.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/msm_iommu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
index c60624910872..454f6331c889 100644
--- a/drivers/iommu/msm_iommu.c
+++ b/drivers/iommu/msm_iommu.c
@@ -443,9 +443,9 @@ static int msm_iommu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	return ret;
 }
 
-static void msm_iommu_detach_dev(struct iommu_domain *domain,
-				 struct device *dev)
+static void msm_iommu_set_platform_dma(struct device *dev)
 {
+	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
 	struct msm_priv *priv = to_msm_priv(domain);
 	unsigned long flags;
 	struct msm_iommu_dev *iommu;
@@ -678,11 +678,11 @@ static struct iommu_ops msm_iommu_ops = {
 	.domain_alloc = msm_iommu_domain_alloc,
 	.probe_device = msm_iommu_probe_device,
 	.device_group = generic_device_group,
+	.set_platform_dma_ops = msm_iommu_set_platform_dma,
 	.pgsize_bitmap = MSM_IOMMU_PGSIZES,
 	.of_xlate = qcom_iommu_of_xlate,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= msm_iommu_attach_dev,
-		.detach_dev	= msm_iommu_detach_dev,
 		.map_pages	= msm_iommu_map,
 		.unmap_pages	= msm_iommu_unmap,
 		/*
-- 
2.34.1

[PATCH v4 13/19] iommu/mtk_v1: Add set_platform_dma_ops callback

[Lu Baolu]

This IOMMU driver doesn't support default domain. Add the implementation
of set_platform_dma_ops callback so that the IOMMU core could return the
DMA control. With the set_platform_dma_ops implemented, there's no need
for detach_dev. Remove it to avoid dead code.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/mtk_iommu_v1.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index 69682ee068d2..78d0a84c704f 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -319,7 +319,7 @@ static int mtk_iommu_v1_attach_device(struct iommu_domain *domain, struct device
 	return 0;
 }
 
-static void mtk_iommu_v1_detach_device(struct iommu_domain *domain, struct device *dev)
+static void mtk_iommu_v1_set_platform_dma(struct device *dev)
 {
 	struct mtk_iommu_v1_data *data = dev_iommu_priv_get(dev);
 
@@ -585,10 +585,10 @@ static const struct iommu_ops mtk_iommu_v1_ops = {
 	.def_domain_type = mtk_iommu_v1_def_domain_type,
 	.device_group	= generic_device_group,
 	.pgsize_bitmap	= MT2701_IOMMU_PAGE_SIZE,
+	.set_platform_dma_ops = mtk_iommu_v1_set_platform_dma,
 	.owner          = THIS_MODULE,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= mtk_iommu_v1_attach_device,
-		.detach_dev	= mtk_iommu_v1_detach_device,
 		.map_pages	= mtk_iommu_v1_map,
 		.unmap_pages	= mtk_iommu_v1_unmap,
 		.iova_to_phys	= mtk_iommu_v1_iova_to_phys,
-- 
2.34.1

[PATCH v4 14/19] iommu/omap: Add set_platform_dma_ops callback

[Lu Baolu]

This IOMMU driver doesn't support default domain. Add the implementation
of set_platform_dma_ops callback so that the IOMMU core could return the
DMA control. With the set_platform_dma implemented, there's no need for
detach_dev. Remove it to avoid dead code.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/omap-iommu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 2fd7702c6709..3ab078112a7c 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1556,9 +1556,9 @@ static void _omap_iommu_detach_dev(struct omap_iommu_domain *omap_domain,
 	omap_domain->dev = NULL;
 }
 
-static void omap_iommu_detach_dev(struct iommu_domain *domain,
-				  struct device *dev)
+static void omap_iommu_set_platform_dma(struct device *dev)
 {
+	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
 	struct omap_iommu_domain *omap_domain = to_omap_domain(domain);
 
 	spin_lock(&omap_domain->lock);
@@ -1737,10 +1737,10 @@ static const struct iommu_ops omap_iommu_ops = {
 	.probe_device	= omap_iommu_probe_device,
 	.release_device	= omap_iommu_release_device,
 	.device_group	= omap_iommu_device_group,
+	.set_platform_dma_ops = omap_iommu_set_platform_dma,
 	.pgsize_bitmap	= OMAP_IOMMU_PGSIZES,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= omap_iommu_attach_dev,
-		.detach_dev	= omap_iommu_detach_dev,
 		.map		= omap_iommu_map,
 		.unmap		= omap_iommu_unmap,
 		.iova_to_phys	= omap_iommu_iova_to_phys,
-- 
2.34.1

[PATCH v4 15/19] iommu/s390: Add set_platform_dma_ops callback

[Lu Baolu]

This IOMMU driver doesn't support default domain. Add the implementation
of set_platform_dma_ops callback so that the IOMMU core could return the
DMA control. With the set_platform_dma implemented, there's no need for
detach_dev. Remove it to avoid dead code.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/s390-iommu.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
index ed33c6cce083..5591dab99446 100644
--- a/drivers/iommu/s390-iommu.c
+++ b/drivers/iommu/s390-iommu.c
@@ -144,13 +144,10 @@ static int s390_iommu_attach_device(struct iommu_domain *domain,
 	return 0;
 }
 
-static void s390_iommu_detach_device(struct iommu_domain *domain,
-				     struct device *dev)
+static void s390_iommu_set_platform_dma(struct device *dev)
 {
 	struct zpci_dev *zdev = to_zpci_dev(dev);
 
-	WARN_ON(zdev->s390_domain != to_s390_domain(domain));
-
 	__s390_iommu_detach_device(zdev);
 	zpci_dma_init_device(zdev);
 }
@@ -435,11 +432,11 @@ static const struct iommu_ops s390_iommu_ops = {
 	.probe_device = s390_iommu_probe_device,
 	.release_device = s390_iommu_release_device,
 	.device_group = generic_device_group,
+	.set_platform_dma_ops = s390_iommu_set_platform_dma,
 	.pgsize_bitmap = SZ_4K,
 	.get_resv_regions = s390_iommu_get_resv_regions,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= s390_iommu_attach_device,
-		.detach_dev	= s390_iommu_detach_device,
 		.map_pages	= s390_iommu_map_pages,
 		.unmap_pages	= s390_iommu_unmap_pages,
 		.flush_iotlb_all = s390_iommu_flush_iotlb_all,
-- 
2.34.1

[PATCH v4 16/19] iommu/gart: Add set_platform_dma_ops callback

[Lu Baolu]

This IOMMU driver doesn't support default domain. Add the implementation
of set_platform_dma_ops callback so that the IOMMU core could return the
DMA control. With set_platform_dma_ops implemented, there's no need for
detach_dev. Remove it to avoid dead code.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/tegra-gart.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index ed53279d1106..a482ff838b53 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -124,9 +124,9 @@ static int gart_iommu_attach_dev(struct iommu_domain *domain,
 	return ret;
 }
 
-static void gart_iommu_detach_dev(struct iommu_domain *domain,
-				  struct device *dev)
+static void gart_iommu_set_platform_dma(struct device *dev)
 {
+	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
 	struct gart_device *gart = gart_handle;
 
 	spin_lock(&gart->dom_lock);
@@ -270,11 +270,11 @@ static const struct iommu_ops gart_iommu_ops = {
 	.domain_alloc	= gart_iommu_domain_alloc,
 	.probe_device	= gart_iommu_probe_device,
 	.device_group	= generic_device_group,
+	.set_platform_dma_ops = gart_iommu_set_platform_dma,
 	.pgsize_bitmap	= GART_IOMMU_PGSIZES,
 	.of_xlate	= gart_iommu_of_xlate,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= gart_iommu_attach_dev,
-		.detach_dev	= gart_iommu_detach_dev,
 		.map		= gart_iommu_map,
 		.unmap		= gart_iommu_unmap,
 		.iova_to_phys	= gart_iommu_iova_to_phys,
-- 
2.34.1

[PATCH v4 17/19] iommu/tegra: Add set_platform_dma_ops callback

[Lu Baolu]

This IOMMU driver doesn't support default domain. Add the implementation
of set_platform_dma_ops callback so that the IOMMU core could return the
DMA control. With the set_platform_dma_ops implemented, there's no need
for detach_dev. Remove it to avoid dead code.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/tegra-smmu.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 5b1af40221ec..4c4ac22d5fb1 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -511,8 +511,9 @@ static int tegra_smmu_attach_dev(struct iommu_domain *domain,
 	return err;
 }
 
-static void tegra_smmu_detach_dev(struct iommu_domain *domain, struct device *dev)
+static void tegra_smmu_set_platform_dma(struct device *dev)
 {
+	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
 	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
 	struct tegra_smmu_as *as = to_smmu_as(domain);
 	struct tegra_smmu *smmu = as->smmu;
@@ -965,11 +966,11 @@ static const struct iommu_ops tegra_smmu_ops = {
 	.domain_alloc = tegra_smmu_domain_alloc,
 	.probe_device = tegra_smmu_probe_device,
 	.device_group = tegra_smmu_device_group,
+	.set_platform_dma_ops = tegra_smmu_set_platform_dma,
 	.of_xlate = tegra_smmu_of_xlate,
 	.pgsize_bitmap = SZ_4K,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= tegra_smmu_attach_dev,
-		.detach_dev	= tegra_smmu_detach_dev,
 		.map		= tegra_smmu_map,
 		.unmap		= tegra_smmu_unmap,
 		.iova_to_phys	= tegra_smmu_iova_to_phys,
-- 
2.34.1

[PATCH v4 18/19] iommu: Remove deferred attach check from __iommu_detach_device()

[Lu Baolu]

From: Jason Gunthorpe <jgg@nvidia.com>

At the current moment, __iommu_detach_device() is only called via call
chains that are after the device driver is attached - eg via explicit
attach APIs called by the device driver.

Commit bd421264ed30 ("iommu: Fix deferred domain attachment") has removed
deferred domain attachment check from __iommu_attach_device() path, so it
should just unconditionally work in the __iommu_detach_device() path.

It actually looks like a bug that we were blocking detach on these paths
since the attach was unconditional and the caller is going to free the
(probably) UNAMANGED domain once this returns.

The only place we should be testing for deferred attach is during the
initial point the dma device is linked to the group, and then again
during the dma api calls.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 include/linux/iommu.h |  2 ++
 drivers/iommu/iommu.c | 70 ++++++++++++++++++++++---------------------
 2 files changed, 38 insertions(+), 34 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 7b3e3775b069..0d10566b3cb2 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -405,6 +405,7 @@ struct iommu_fault_param {
  * @iommu_dev:	 IOMMU device this device is linked to
  * @priv:	 IOMMU Driver private data
  * @max_pasids:  number of PASIDs this device can consume
+ * @attach_deferred: the dma domain attachment is deferred
  *
  * TODO: migrate other per device data pointers under iommu_dev_data, e.g.
  *	struct iommu_group	*iommu_group;
@@ -417,6 +418,7 @@ struct dev_iommu {
 	struct iommu_device		*iommu_dev;
 	void				*priv;
 	u32				max_pasids;
+	u32				attach_deferred:1;
 };
 
 int iommu_device_register(struct iommu_device *iommu,
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 4e35a9f94873..c7bd8663f1f5 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -371,6 +371,30 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 	return ret;
 }
 
+static bool iommu_is_attach_deferred(struct device *dev)
+{
+	const struct iommu_ops *ops = dev_iommu_ops(dev);
+
+	if (ops->is_attach_deferred)
+		return ops->is_attach_deferred(dev);
+
+	return false;
+}
+
+static int iommu_group_do_dma_first_attach(struct device *dev, void *data)
+{
+	struct iommu_domain *domain = data;
+
+	lockdep_assert_held(&dev->iommu_group->mutex);
+
+	if (iommu_is_attach_deferred(dev)) {
+		dev->iommu->attach_deferred = 1;
+		return 0;
+	}
+
+	return __iommu_attach_device(domain, dev);
+}
+
 int iommu_probe_device(struct device *dev)
 {
 	const struct iommu_ops *ops;
@@ -401,7 +425,7 @@ int iommu_probe_device(struct device *dev)
 	 * attach the default domain.
 	 */
 	if (group->default_domain && !group->owner) {
-		ret = __iommu_attach_device(group->default_domain, dev);
+		ret = iommu_group_do_dma_first_attach(dev, group->default_domain);
 		if (ret) {
 			mutex_unlock(&group->mutex);
 			iommu_group_put(group);
@@ -947,16 +971,6 @@ static int iommu_create_device_direct_mappings(struct iommu_group *group,
 	return ret;
 }
 
-static bool iommu_is_attach_deferred(struct device *dev)
-{
-	const struct iommu_ops *ops = dev_iommu_ops(dev);
-
-	if (ops->is_attach_deferred)
-		return ops->is_attach_deferred(dev);
-
-	return false;
-}
-
 /**
  * iommu_group_add_device - add a device to an iommu group
  * @group: the group into which to add the device (reference should be held)
@@ -1009,8 +1023,8 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
 
 	mutex_lock(&group->mutex);
 	list_add_tail(&device->list, &group->devices);
-	if (group->domain  && !iommu_is_attach_deferred(dev))
-		ret = __iommu_attach_device(group->domain, dev);
+	if (group->domain)
+		ret = iommu_group_do_dma_first_attach(dev, group->domain);
 	mutex_unlock(&group->mutex);
 	if (ret)
 		goto err_put_group;
@@ -1776,21 +1790,10 @@ static void probe_alloc_default_domain(struct bus_type *bus,
 
 }
 
-static int iommu_group_do_dma_attach(struct device *dev, void *data)
-{
-	struct iommu_domain *domain = data;
-	int ret = 0;
-
-	if (!iommu_is_attach_deferred(dev))
-		ret = __iommu_attach_device(domain, dev);
-
-	return ret;
-}
-
-static int __iommu_group_dma_attach(struct iommu_group *group)
+static int __iommu_group_dma_first_attach(struct iommu_group *group)
 {
 	return __iommu_group_for_each_dev(group, group->default_domain,
-					  iommu_group_do_dma_attach);
+					  iommu_group_do_dma_first_attach);
 }
 
 static int iommu_group_do_probe_finalize(struct device *dev, void *data)
@@ -1855,7 +1858,7 @@ int bus_iommu_probe(struct bus_type *bus)
 
 		iommu_group_create_direct_mappings(group);
 
-		ret = __iommu_group_dma_attach(group);
+		ret = __iommu_group_dma_first_attach(group);
 
 		mutex_unlock(&group->mutex);
 
@@ -1987,9 +1990,11 @@ static int __iommu_attach_device(struct iommu_domain *domain,
 		return -ENODEV;
 
 	ret = domain->ops->attach_dev(domain, dev);
-	if (!ret)
-		trace_attach_device_to_domain(dev);
-	return ret;
+	if (ret)
+		return ret;
+	dev->iommu->attach_deferred = 0;
+	trace_attach_device_to_domain(dev);
+	return 0;
 }
 
 /**
@@ -2034,7 +2039,7 @@ EXPORT_SYMBOL_GPL(iommu_attach_device);
 
 int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain)
 {
-	if (iommu_is_attach_deferred(dev))
+	if (dev->iommu && dev->iommu->attach_deferred)
 		return __iommu_attach_device(domain, dev);
 
 	return 0;
@@ -2043,9 +2048,6 @@ int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain)
 static void __iommu_detach_device(struct iommu_domain *domain,
 				  struct device *dev)
 {
-	if (iommu_is_attach_deferred(dev))
-		return;
-
 	domain->ops->detach_dev(domain, dev);
 	trace_detach_device_from_domain(dev);
 }
-- 
2.34.1

[PATCH v4 19/19] iommu: Remove detach_dev callback

[Lu Baolu]

The detach_dev callback of domain ops is not called in the IOMMU core.
Remove this callback to avoid dead code. The trace event for detaching
domain from device is removed accordingly.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 include/linux/iommu.h        |  2 --
 include/trace/events/iommu.h |  7 -------
 drivers/iommu/iommu-traces.c |  1 -
 drivers/iommu/iommu.c        | 29 +++++------------------------
 4 files changed, 5 insertions(+), 34 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 0d10566b3cb2..a8063f26ff69 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -299,7 +299,6 @@ struct iommu_ops {
  * * EBUSY	- device is attached to a domain and cannot be changed
  * * ENODEV	- device specific errors, not able to be attached
  * * <others>	- treated as ENODEV by the caller. Use is discouraged
- * @detach_dev: detach an iommu domain from a device
  * @set_dev_pasid: set an iommu domain to a pasid of device
  * @map: map a physically contiguous memory region to an iommu domain
  * @map_pages: map a physically contiguous set of pages of the same size to
@@ -320,7 +319,6 @@ struct iommu_ops {
  */
 struct iommu_domain_ops {
 	int (*attach_dev)(struct iommu_domain *domain, struct device *dev);
-	void (*detach_dev)(struct iommu_domain *domain, struct device *dev);
 	int (*set_dev_pasid)(struct iommu_domain *domain, struct device *dev,
 			     ioasid_t pasid);
 
diff --git a/include/trace/events/iommu.h b/include/trace/events/iommu.h
index 29096fe12623..70743db1fb75 100644
--- a/include/trace/events/iommu.h
+++ b/include/trace/events/iommu.h
@@ -76,13 +76,6 @@ DEFINE_EVENT(iommu_device_event, attach_device_to_domain,
 	TP_ARGS(dev)
 );
 
-DEFINE_EVENT(iommu_device_event, detach_device_from_domain,
-
-	TP_PROTO(struct device *dev),
-
-	TP_ARGS(dev)
-);
-
 TRACE_EVENT(map,
 
 	TP_PROTO(unsigned long iova, phys_addr_t paddr, size_t size),
diff --git a/drivers/iommu/iommu-traces.c b/drivers/iommu/iommu-traces.c
index 1e9ca7789de1..23416bf76df9 100644
--- a/drivers/iommu/iommu-traces.c
+++ b/drivers/iommu/iommu-traces.c
@@ -18,7 +18,6 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(remove_device_from_group);
 
 /* iommu_device_event */
 EXPORT_TRACEPOINT_SYMBOL_GPL(attach_device_to_domain);
-EXPORT_TRACEPOINT_SYMBOL_GPL(detach_device_from_domain);
 
 /* iommu_map_unmap */
 EXPORT_TRACEPOINT_SYMBOL_GPL(map);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index c7bd8663f1f5..d2b3210170a8 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2045,13 +2045,6 @@ int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain)
 	return 0;
 }
 
-static void __iommu_detach_device(struct iommu_domain *domain,
-				  struct device *dev)
-{
-	domain->ops->detach_dev(domain, dev);
-	trace_detach_device_from_domain(dev);
-}
-
 void iommu_detach_device(struct iommu_domain *domain, struct device *dev)
 {
 	struct iommu_group *group;
@@ -2156,15 +2149,6 @@ int iommu_attach_group(struct iommu_domain *domain, struct iommu_group *group)
 }
 EXPORT_SYMBOL_GPL(iommu_attach_group);
 
-static int iommu_group_do_detach_device(struct device *dev, void *data)
-{
-	struct iommu_domain *domain = data;
-
-	__iommu_detach_device(domain, dev);
-
-	return 0;
-}
-
 static int iommu_group_do_set_platform_dma(struct device *dev, void *data)
 {
 	const struct iommu_ops *ops = dev_iommu_ops(dev);
@@ -2185,19 +2169,16 @@ static int __iommu_group_set_domain(struct iommu_group *group,
 		return 0;
 
 	/*
-	 * New drivers should support default domains and so the detach_dev() op
-	 * will never be called. Otherwise the NULL domain represents some
+	 * New drivers should support default domains and so set_platform_dma()
+	 * op will never be called. Otherwise the NULL domain represents some
 	 * platform specific behavior.
 	 */
 	if (!new_domain) {
 		ret = __iommu_group_for_each_dev(group, NULL,
 				iommu_group_do_set_platform_dma);
-		if (ret) {
-			if (WARN_ON(!group->domain->ops->detach_dev))
-				return -EINVAL;
-			__iommu_group_for_each_dev(group, group->domain,
-				iommu_group_do_detach_device);
-		}
+		if (ret)
+			return ret;
+
 		group->domain = NULL;
 		return 0;
 	}
-- 
2.34.1

Re: [PATCH v4 06/19] iommu/mtk: Remove detach_dev callback

[Jason Gunthorpe]

On Wed, Jan 04, 2023 at 08:57:12PM +0800, Lu Baolu wrote:
> The IOMMU driver supports default domain, so the detach_dev op will never
> be called. Remove it to avoid dead code.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/mtk_iommu.c | 9 ---------
>  1 file changed, 9 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH v4 10/19] iommu: Add set_platform_dma_ops iommu ops

[Jason Gunthorpe]

On Wed, Jan 04, 2023 at 08:57:16PM +0800, Lu Baolu wrote:

> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index de91dd88705b..4e35a9f94873 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2163,6 +2163,17 @@ static int iommu_group_do_detach_device(struct device *dev, void *data)
>  	return 0;
>  }
>  
> +static int iommu_group_do_set_platform_dma(struct device *dev, void *data)
> +{
> +	const struct iommu_ops *ops = dev_iommu_ops(dev);
> +
> +	if (!ops->set_platform_dma_ops)
> +		return -EINVAL;
> +
> +	ops->set_platform_dma_ops(dev);
> +	return 0;
> +}
> +
>  static int __iommu_group_set_domain(struct iommu_group *group,
>  				    struct iommu_domain *new_domain)
>  {
> @@ -2177,10 +2188,14 @@ static int __iommu_group_set_domain(struct iommu_group *group,
>  	 * platform specific behavior.
>  	 */
>  	if (!new_domain) {
> -		if (WARN_ON(!group->domain->ops->detach_dev))
> -			return -EINVAL;

This should still have the WARN_ON..

if (WARN_ON(!group->domain->ops->detach_dev && !dev_iommu_ops(dev)->set_platform_dma_ops)

Jason

Re: [PATCH v4 11/19] iommu/fsl_pamu: Add set_platform_dma_ops callback

[Jason Gunthorpe]

On Wed, Jan 04, 2023 at 08:57:17PM +0800, Lu Baolu wrote:
> This IOMMU driver doesn't support default domain. Add the implementation
> of set_platform_dma_ops callback so that the IOMMU core could return the
> DMA control. With the set_platform_dma_ops implemented, there's no need
> for detach_dev. Remove it to avoid dead code.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/fsl_pamu_domain.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH v4 12/19] iommu/msm: Add set_platform_dma_ops callback

[Jason Gunthorpe]

On Wed, Jan 04, 2023 at 08:57:18PM +0800, Lu Baolu wrote:
> This IOMMU driver doesn't support default domain. Add the implementation
> of set_platform_dma_ops callback so that the IOMMU core could return the
> DMA control. With the set_platform_dma_ops implemented, there's no need
> for detach_dev. Remove it to avoid dead code.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/msm_iommu.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH v4 13/19] iommu/mtk_v1: Add set_platform_dma_ops callback

[Jason Gunthorpe]

On Wed, Jan 04, 2023 at 08:57:19PM +0800, Lu Baolu wrote:
> This IOMMU driver doesn't support default domain. Add the implementation
> of set_platform_dma_ops callback so that the IOMMU core could return the
> DMA control. With the set_platform_dma_ops implemented, there's no need
> for detach_dev. Remove it to avoid dead code.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/mtk_iommu_v1.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH v4 14/19] iommu/omap: Add set_platform_dma_ops callback

[Jason Gunthorpe]

On Wed, Jan 04, 2023 at 08:57:20PM +0800, Lu Baolu wrote:
> This IOMMU driver doesn't support default domain. Add the implementation
> of set_platform_dma_ops callback so that the IOMMU core could return the
> DMA control. With the set_platform_dma implemented, there's no need for
> detach_dev. Remove it to avoid dead code.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/omap-iommu.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH v4 15/19] iommu/s390: Add set_platform_dma_ops callback

[Jason Gunthorpe]

On Wed, Jan 04, 2023 at 08:57:21PM +0800, Lu Baolu wrote:
> This IOMMU driver doesn't support default domain. Add the implementation
> of set_platform_dma_ops callback so that the IOMMU core could return the
> DMA control. With the set_platform_dma implemented, there's no need for
> detach_dev. Remove it to avoid dead code.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/s390-iommu.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH v4 16/19] iommu/gart: Add set_platform_dma_ops callback

[Jason Gunthorpe]

On Wed, Jan 04, 2023 at 08:57:22PM +0800, Lu Baolu wrote:
> This IOMMU driver doesn't support default domain. Add the implementation
> of set_platform_dma_ops callback so that the IOMMU core could return the
> DMA control. With set_platform_dma_ops implemented, there's no need for
> detach_dev. Remove it to avoid dead code.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/tegra-gart.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH v4 17/19] iommu/tegra: Add set_platform_dma_ops callback

[Jason Gunthorpe]

On Wed, Jan 04, 2023 at 08:57:23PM +0800, Lu Baolu wrote:
> This IOMMU driver doesn't support default domain. Add the implementation
> of set_platform_dma_ops callback so that the IOMMU core could return the
> DMA control. With the set_platform_dma_ops implemented, there's no need
> for detach_dev. Remove it to avoid dead code.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/tegra-smmu.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH v4 19/19] iommu: Remove detach_dev callback

[Jason Gunthorpe]

On Wed, Jan 04, 2023 at 08:57:25PM +0800, Lu Baolu wrote:
> The detach_dev callback of domain ops is not called in the IOMMU core.
> Remove this callback to avoid dead code. The trace event for detaching
> domain from device is removed accordingly.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  include/linux/iommu.h        |  2 --
>  include/trace/events/iommu.h |  7 -------
>  drivers/iommu/iommu-traces.c |  1 -
>  drivers/iommu/iommu.c        | 29 +++++------------------------
>  4 files changed, 5 insertions(+), 34 deletions(-)
> 
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 0d10566b3cb2..a8063f26ff69 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -299,7 +299,6 @@ struct iommu_ops {
>   * * EBUSY	- device is attached to a domain and cannot be changed
>   * * ENODEV	- device specific errors, not able to be attached
>   * * <others>	- treated as ENODEV by the caller. Use is discouraged
> - * @detach_dev: detach an iommu domain from a device
>   * @set_dev_pasid: set an iommu domain to a pasid of device
>   * @map: map a physically contiguous memory region to an iommu domain
>   * @map_pages: map a physically contiguous set of pages of the same size to
> @@ -320,7 +319,6 @@ struct iommu_ops {
>   */
>  struct iommu_domain_ops {
>  	int (*attach_dev)(struct iommu_domain *domain, struct device *dev);
> -	void (*detach_dev)(struct iommu_domain *domain, struct device *dev);
>  	int (*set_dev_pasid)(struct iommu_domain *domain, struct device *dev,
>  			     ioasid_t pasid);
>  
> diff --git a/include/trace/events/iommu.h b/include/trace/events/iommu.h
> index 29096fe12623..70743db1fb75 100644
> --- a/include/trace/events/iommu.h
> +++ b/include/trace/events/iommu.h
> @@ -76,13 +76,6 @@ DEFINE_EVENT(iommu_device_event, attach_device_to_domain,
>  	TP_ARGS(dev)
>  );
>  
> -DEFINE_EVENT(iommu_device_event, detach_device_from_domain,
> -
> -	TP_PROTO(struct device *dev),
> -
> -	TP_ARGS(dev)
> -);
> -

did you want to add a trace for the set_platform_dma_ops in the
earlier patch?

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH v4 10/19] iommu: Add set_platform_dma_ops iommu ops

[Baolu Lu]

Hi Jason,

On 2023/1/4 21:17, Jason Gunthorpe wrote:
> On Wed, Jan 04, 2023 at 08:57:16PM +0800, Lu Baolu wrote:
> 
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index de91dd88705b..4e35a9f94873 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -2163,6 +2163,17 @@ static int iommu_group_do_detach_device(struct device *dev, void *data)
>>   	return 0;
>>   }
>>   
>> +static int iommu_group_do_set_platform_dma(struct device *dev, void *data)
>> +{
>> +	const struct iommu_ops *ops = dev_iommu_ops(dev);
>> +
>> +	if (!ops->set_platform_dma_ops)
>> +		return -EINVAL;
>> +
>> +	ops->set_platform_dma_ops(dev);
>> +	return 0;
>> +}
>> +
>>   static int __iommu_group_set_domain(struct iommu_group *group,
>>   				    struct iommu_domain *new_domain)
>>   {
>> @@ -2177,10 +2188,14 @@ static int __iommu_group_set_domain(struct iommu_group *group,
>>   	 * platform specific behavior.
>>   	 */
>>   	if (!new_domain) {
>> -		if (WARN_ON(!group->domain->ops->detach_dev))
>> -			return -EINVAL;
> This should still have the WARN_ON..
> 
> if (WARN_ON(!group->domain->ops->detach_dev && !dev_iommu_ops(dev)->set_platform_dma_ops)

This has been implicitly included in the code.

iommu_group_do_set_platform_dma() returns -EINVAL if the iommu driver
doesn't support set_platform_dma_ops (otherwise always return success).
Then, the domain->ops->detach_dev is required and a WARN_ON was there.

         if (!new_domain) {
                 ret = __iommu_group_for_each_dev(group, NULL,
                                 iommu_group_do_set_platform_dma);
                 if (ret) {
                         if (WARN_ON(!group->domain->ops->detach_dev))
                                 return -EINVAL;
                         __iommu_group_for_each_dev(group, group->domain,
                                 iommu_group_do_detach_device);
                 }
                 group->domain = NULL;
                 return 0;
         }

Perhaps I should add a comment to explain this?

--
Best regards,
baolu

Re: [PATCH v4 19/19] iommu: Remove detach_dev callback

[Baolu Lu]

On 2023/1/4 21:22, Jason Gunthorpe wrote:
> On Wed, Jan 04, 2023 at 08:57:25PM +0800, Lu Baolu wrote:
>> The detach_dev callback of domain ops is not called in the IOMMU core.
>> Remove this callback to avoid dead code. The trace event for detaching
>> domain from device is removed accordingly.
>>
>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>> ---
>>   include/linux/iommu.h        |  2 --
>>   include/trace/events/iommu.h |  7 -------
>>   drivers/iommu/iommu-traces.c |  1 -
>>   drivers/iommu/iommu.c        | 29 +++++------------------------
>>   4 files changed, 5 insertions(+), 34 deletions(-)
>>
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index 0d10566b3cb2..a8063f26ff69 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -299,7 +299,6 @@ struct iommu_ops {
>>    * * EBUSY	- device is attached to a domain and cannot be changed
>>    * * ENODEV	- device specific errors, not able to be attached
>>    * * <others>	- treated as ENODEV by the caller. Use is discouraged
>> - * @detach_dev: detach an iommu domain from a device
>>    * @set_dev_pasid: set an iommu domain to a pasid of device
>>    * @map: map a physically contiguous memory region to an iommu domain
>>    * @map_pages: map a physically contiguous set of pages of the same size to
>> @@ -320,7 +319,6 @@ struct iommu_ops {
>>    */
>>   struct iommu_domain_ops {
>>   	int (*attach_dev)(struct iommu_domain *domain, struct device *dev);
>> -	void (*detach_dev)(struct iommu_domain *domain, struct device *dev);
>>   	int (*set_dev_pasid)(struct iommu_domain *domain, struct device *dev,
>>   			     ioasid_t pasid);
>>   
>> diff --git a/include/trace/events/iommu.h b/include/trace/events/iommu.h
>> index 29096fe12623..70743db1fb75 100644
>> --- a/include/trace/events/iommu.h
>> +++ b/include/trace/events/iommu.h
>> @@ -76,13 +76,6 @@ DEFINE_EVENT(iommu_device_event, attach_device_to_domain,
>>   	TP_ARGS(dev)
>>   );
>>   
>> -DEFINE_EVENT(iommu_device_event, detach_device_from_domain,
>> -
>> -	TP_PROTO(struct device *dev),
>> -
>> -	TP_ARGS(dev)
>> -);
>> -
> did you want to add a trace for the set_platform_dma_ops in the
> earlier patch?

If necessary, we can add such a trace event. But I do hope to further
evolve the iommu drivers that need this callback. Finally, we should
remove it totally.

--
Best regards,
baolu

Re: [PATCH v4 10/19] iommu: Add set_platform_dma_ops iommu ops

[Jason Gunthorpe]

On Thu, Jan 05, 2023 at 01:58:42PM +0800, Baolu Lu wrote:
> Hi Jason,
> 
> On 2023/1/4 21:17, Jason Gunthorpe wrote:
> > On Wed, Jan 04, 2023 at 08:57:16PM +0800, Lu Baolu wrote:
> > 
> > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > index de91dd88705b..4e35a9f94873 100644
> > > --- a/drivers/iommu/iommu.c
> > > +++ b/drivers/iommu/iommu.c
> > > @@ -2163,6 +2163,17 @@ static int iommu_group_do_detach_device(struct device *dev, void *data)
> > >   	return 0;
> > >   }
> > > +static int iommu_group_do_set_platform_dma(struct device *dev, void *data)
> > > +{
> > > +	const struct iommu_ops *ops = dev_iommu_ops(dev);
> > > +
> > > +	if (!ops->set_platform_dma_ops)
> > > +		return -EINVAL;
> > > +
> > > +	ops->set_platform_dma_ops(dev);
> > > +	return 0;
> > > +}
> > > +
> > >   static int __iommu_group_set_domain(struct iommu_group *group,
> > >   				    struct iommu_domain *new_domain)
> > >   {
> > > @@ -2177,10 +2188,14 @@ static int __iommu_group_set_domain(struct iommu_group *group,
> > >   	 * platform specific behavior.
> > >   	 */
> > >   	if (!new_domain) {
> > > -		if (WARN_ON(!group->domain->ops->detach_dev))
> > > -			return -EINVAL;
> > This should still have the WARN_ON..
> > 
> > if (WARN_ON(!group->domain->ops->detach_dev && !dev_iommu_ops(dev)->set_platform_dma_ops)
> 
> This has been implicitly included in the code.
> 
> iommu_group_do_set_platform_dma() returns -EINVAL if the iommu driver
> doesn't support set_platform_dma_ops (otherwise always return success).
> Then, the domain->ops->detach_dev is required and a WARN_ON was there.
> 
>         if (!new_domain) {
>                 ret = __iommu_group_for_each_dev(group, NULL,
>                                 iommu_group_do_set_platform_dma);
>                 if (ret) {
>                         if (WARN_ON(!group->domain->ops->detach_dev))
>                                 return -EINVAL;
>                         __iommu_group_for_each_dev(group, group->domain,
>                                 iommu_group_do_detach_device);
>                 }
>                 group->domain = NULL;
>                 return 0;
>         }
> 
> Perhaps I should add a comment to explain this?

But you delete this later when you remove this.

I think testing the op directly is much clearer, get rid of the whole
ret and EINVAL thinig:

if (dev_iommu_ops(dev)->set_platform_dma_ops)
   __iommu_group_for_each_dev(group, NULL,
                                 iommu_group_do_set_platform_dma); //	 Can't fail!
else if (group->domain->ops->detach_dev)
     __iommu_group_for_each_dev(group, group->domain,
                                 iommu_group_do_detach_device);
else
   WARN(true)

Jason

Re: [PATCH v4 10/19] iommu: Add set_platform_dma_ops iommu ops

[Baolu Lu]

On 1/5/23 9:15 PM, Jason Gunthorpe wrote:
> On Thu, Jan 05, 2023 at 01:58:42PM +0800, Baolu Lu wrote:
>> Hi Jason,
>>
>> On 2023/1/4 21:17, Jason Gunthorpe wrote:
>>> On Wed, Jan 04, 2023 at 08:57:16PM +0800, Lu Baolu wrote:
>>>
>>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>>> index de91dd88705b..4e35a9f94873 100644
>>>> --- a/drivers/iommu/iommu.c
>>>> +++ b/drivers/iommu/iommu.c
>>>> @@ -2163,6 +2163,17 @@ static int iommu_group_do_detach_device(struct device *dev, void *data)
>>>>    	return 0;
>>>>    }
>>>> +static int iommu_group_do_set_platform_dma(struct device *dev, void *data)
>>>> +{
>>>> +	const struct iommu_ops *ops = dev_iommu_ops(dev);
>>>> +
>>>> +	if (!ops->set_platform_dma_ops)
>>>> +		return -EINVAL;
>>>> +
>>>> +	ops->set_platform_dma_ops(dev);
>>>> +	return 0;
>>>> +}
>>>> +
>>>>    static int __iommu_group_set_domain(struct iommu_group *group,
>>>>    				    struct iommu_domain *new_domain)
>>>>    {
>>>> @@ -2177,10 +2188,14 @@ static int __iommu_group_set_domain(struct iommu_group *group,
>>>>    	 * platform specific behavior.
>>>>    	 */
>>>>    	if (!new_domain) {
>>>> -		if (WARN_ON(!group->domain->ops->detach_dev))
>>>> -			return -EINVAL;
>>> This should still have the WARN_ON..
>>>
>>> if (WARN_ON(!group->domain->ops->detach_dev && !dev_iommu_ops(dev)->set_platform_dma_ops)
>> This has been implicitly included in the code.
>>
>> iommu_group_do_set_platform_dma() returns -EINVAL if the iommu driver
>> doesn't support set_platform_dma_ops (otherwise always return success).
>> Then, the domain->ops->detach_dev is required and a WARN_ON was there.
>>
>>          if (!new_domain) {
>>                  ret = __iommu_group_for_each_dev(group, NULL,
>>                                  iommu_group_do_set_platform_dma);
>>                  if (ret) {
>>                          if (WARN_ON(!group->domain->ops->detach_dev))
>>                                  return -EINVAL;
>>                          __iommu_group_for_each_dev(group, group->domain,
>>                                  iommu_group_do_detach_device);
>>                  }
>>                  group->domain = NULL;
>>                  return 0;
>>          }
>>
>> Perhaps I should add a comment to explain this?
> But you delete this later when you remove this.
> 
> I think testing the op directly is much clearer, get rid of the whole
> ret and EINVAL thinig:
> 
> if (dev_iommu_ops(dev)->set_platform_dma_ops)
>     __iommu_group_for_each_dev(group, NULL,
>                                   iommu_group_do_set_platform_dma); //	 Can't fail!
> else if (group->domain->ops->detach_dev)
>       __iommu_group_for_each_dev(group, group->domain,
>                                   iommu_group_do_detach_device);
> else
>     WARN(true)

Above looks good to me. Thanks! I have updated this part of code like
below:

@@ -2177,10 +2188,20 @@ static int __iommu_group_set_domain(struct 
iommu_group *group,
          * platform specific behavior.
          */
         if (!new_domain) {
-               if (WARN_ON(!group->domain->ops->detach_dev))
-                       return -EINVAL;
-               __iommu_group_for_each_dev(group, group->domain,
-                                          iommu_group_do_detach_device);
+               struct group_device *grp_dev;
+
+               grp_dev = list_first_entry(&group->devices,
+                                          struct group_device, list);
+
+               if (dev_iommu_ops(grp_dev->dev)->set_platform_dma_ops)
+                       __iommu_group_for_each_dev(group, NULL,
+                                       iommu_group_do_set_platform_dma);
+               else if (group->domain->ops->detach_dev)
+                       __iommu_group_for_each_dev(group, group->domain,
+                                       iommu_group_do_detach_device);
+               else
+                       WARN_ON_ONCE(1);
+
                 group->domain = NULL;
                 return 0;
         }

--
Best regards,
baolu

Re: [PATCH v4 10/19] iommu: Add set_platform_dma_ops iommu ops

[Jason Gunthorpe]

On Fri, Jan 06, 2023 at 02:07:32PM +0800, Baolu Lu wrote:

> Above looks good to me. Thanks! I have updated this part of code like
> below:
> 
> @@ -2177,10 +2188,20 @@ static int __iommu_group_set_domain(struct
> iommu_group *group,
>          * platform specific behavior.
>          */
>         if (!new_domain) {
> -               if (WARN_ON(!group->domain->ops->detach_dev))
> -                       return -EINVAL;
> -               __iommu_group_for_each_dev(group, group->domain,
> -                                          iommu_group_do_detach_device);
> +               struct group_device *grp_dev;
> +
> +               grp_dev = list_first_entry(&group->devices,
> +                                          struct group_device, list);

It seems OK - I hope we naturally can't ever get in a situation where
a group has disjoint iommu drivers.

Jason

Re: [PATCH v4 10/19] iommu: Add set_platform_dma_ops iommu ops

[Baolu Lu]

On 1/6/2023 10:26 PM, Jason Gunthorpe wrote:
> On Fri, Jan 06, 2023 at 02:07:32PM +0800, Baolu Lu wrote:
> 
>> Above looks good to me. Thanks! I have updated this part of code like
>> below:
>>
>> @@ -2177,10 +2188,20 @@ static int __iommu_group_set_domain(struct
>> iommu_group *group,
>>           * platform specific behavior.
>>           */
>>          if (!new_domain) {
>> -               if (WARN_ON(!group->domain->ops->detach_dev))
>> -                       return -EINVAL;
>> -               __iommu_group_for_each_dev(group, group->domain,
>> -                                          iommu_group_do_detach_device);
>> +               struct group_device *grp_dev;
>> +
>> +               grp_dev = list_first_entry(&group->devices,
>> +                                          struct group_device, list);
> It seems OK - I hope we naturally can't ever get in a situation where
> a group has disjoint iommu drivers.

The final code after cleanup looks like below. We will WARN_ON the lack
of callback in the iommu_group_do_set_platform_dma() helper.

2152 static int iommu_group_do_set_platform_dma(struct device *dev, void 
*data)
2153 {
2154         const struct iommu_ops *ops = dev_iommu_ops(dev);
2155
2156         if (!WARN_ON(!ops->set_platform_dma_ops))
2157                 ops->set_platform_dma_ops(dev);
2158
2159         return 0;
2160 }
2161
2162 static int __iommu_group_set_domain(struct iommu_group *group,
2163                                     struct iommu_domain *new_domain)
2164 {
2165         int ret;
2166
2167         if (group->domain == new_domain)
2168                 return 0;
2169
2170         /*
2171          * New drivers should support default domains, so 
set_platform_dma()
2172          * op will never be called. Otherwise the NULL domain 
represents some
2173          * platform specific behavior.
2174          */
2175         if (!new_domain) {
2176                 __iommu_group_for_each_dev(group, NULL,
2177 
iommu_group_do_set_platform_dma);
2178                 group->domain = NULL;
2179                 return 0;
2180         }

How do you like this?

--
Best regards,
baolu

Re: [PATCH v4 01/19] iommu/amd: Remove detach_dev callback

[Vasant Hegde]

On 1/4/2023 6:27 PM, Lu Baolu wrote:
> The AMD IOMMU supports default domain, so the detach_dev op will never
> be called. Remove it to avoid dead code.
> 
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>

Looks good to me.

Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>

-Vasant

Re: [PATCH v4 10/19] iommu: Add set_platform_dma_ops iommu ops

[Jason Gunthorpe]

On Sat, Jan 07, 2023 at 10:48:31AM +0800, Baolu Lu wrote:
> On 1/6/2023 10:26 PM, Jason Gunthorpe wrote:
> > On Fri, Jan 06, 2023 at 02:07:32PM +0800, Baolu Lu wrote:
> > 
> > > Above looks good to me. Thanks! I have updated this part of code like
> > > below:
> > > 
> > > @@ -2177,10 +2188,20 @@ static int __iommu_group_set_domain(struct
> > > iommu_group *group,
> > >           * platform specific behavior.
> > >           */
> > >          if (!new_domain) {
> > > -               if (WARN_ON(!group->domain->ops->detach_dev))
> > > -                       return -EINVAL;
> > > -               __iommu_group_for_each_dev(group, group->domain,
> > > -                                          iommu_group_do_detach_device);
> > > +               struct group_device *grp_dev;
> > > +
> > > +               grp_dev = list_first_entry(&group->devices,
> > > +                                          struct group_device, list);
> > It seems OK - I hope we naturally can't ever get in a situation where
> > a group has disjoint iommu drivers.
> 
> The final code after cleanup looks like below. We will WARN_ON the lack
> of callback in the iommu_group_do_set_platform_dma() helper.

Yeah OK

Jason