[PATCH v3 00/20] iommu: Retire detach_dev callback

[PATCH v3 00/20] 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 and rename .attach_dev to .set_dev.

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-v3

Please review and suggest.

Best regards,
baolu

Change log:
v3:
 - 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/

Lu Baolu (20):
  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 iommu ops
  iommu/fsl_pamu: Add set_platform_dma callback
  iommu/msm: Add set_platform_dma callback
  iommu/mtk_v1: Add set_platform_dma callback
  iommu/omap: Add set_platform_dma callback
  iommu/s390: Add set_platform_dma callback
  iommu/gart: Add set_platform_dma callback
  iommu/tegra: Add set_platform_dma callback
  iommu: Call set_platform_dma if default domain is unavailable
  iommu: Retire detach_dev callback
  iommu: Rename attach_dev to set_dev

 include/linux/iommu.h                       | 10 ++++---
 drivers/iommu/amd/iommu.c                   | 28 +-----------------
 drivers/iommu/apple-dart.c                  | 19 +-----------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c |  2 +-
 drivers/iommu/arm/arm-smmu/arm-smmu.c       |  2 +-
 drivers/iommu/arm/arm-smmu/qcom_iommu.c     | 25 +---------------
 drivers/iommu/exynos-iommu.c                |  3 +-
 drivers/iommu/fsl_pamu_domain.c             | 12 ++++++--
 drivers/iommu/intel/iommu.c                 |  4 +--
 drivers/iommu/iommu.c                       | 32 +++++++--------------
 drivers/iommu/ipmmu-vmsa.c                  | 18 +-----------
 drivers/iommu/msm_iommu.c                   | 12 ++++++--
 drivers/iommu/mtk_iommu.c                   | 11 +------
 drivers/iommu/mtk_iommu_v1.c                | 12 ++++++--
 drivers/iommu/omap-iommu.c                  | 12 ++++++--
 drivers/iommu/rockchip-iommu.c              |  3 +-
 drivers/iommu/s390-iommu.c                  | 12 ++++++--
 drivers/iommu/sprd-iommu.c                  | 18 +-----------
 drivers/iommu/sun50i-iommu.c                |  3 +-
 drivers/iommu/tegra-gart.c                  | 12 ++++++--
 drivers/iommu/tegra-smmu.c                  | 12 ++++++--
 drivers/iommu/virtio-iommu.c                |  2 +-
 22 files changed, 100 insertions(+), 164 deletions(-)

-- 
2.34.1

[PATCH v3 01/20] 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.

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 5c381cc3658e..bd1970b4f48b 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)
 {
@@ -2414,7 +2389,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

Re: [PATCH v3 01/20] iommu/amd: Remove detach_dev callback

[Jason Gunthorpe]

On Mon, Nov 28, 2022 at 02:46:29PM +0800, Lu Baolu wrote:
> The AMD IOMMU 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/amd/iommu.c | 26 --------------------------
>  1 file changed, 26 deletions(-)

Driver oweners should look at what their detach_dev does and comment
in this series - eg if this is actually blocking DMA then it would be
great to convert it into a blocking domain.

But there is no issue with deleting it, and the blocking domain is an
optimization.

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

Jason

[PATCH v3 02/20] 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.

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

diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
index 4f4a323be0d0..6fbe6b275c79 100644
--- a/drivers/iommu/apple-dart.c
+++ b/drivers/iommu/apple-dart.c
@@ -535,22 +535,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 +764,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

Re: [PATCH v3 02/20] iommu/apple-dart: Remove detach_dev callback

[Jason Gunthorpe]

On Mon, Nov 28, 2022 at 02:46:30PM +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/apple-dart.c | 17 -----------------
>  1 file changed, 17 deletions(-)

I suspect this is doing the blocking domain behavior..

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

Jason

Re: [PATCH v3 02/20] iommu/apple-dart: Remove detach_dev callback

[Sven Peter]

Hi,

On Mon, Nov 28, 2022, at 07:46, 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>
> ---

Reviewed-by: Sven Peter <sven@svenpeter.dev>

>  drivers/iommu/apple-dart.c | 17 -----------------
>  1 file changed, 17 deletions(-)
>
> diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
> index 4f4a323be0d0..6fbe6b275c79 100644
> --- a/drivers/iommu/apple-dart.c
> +++ b/drivers/iommu/apple-dart.c
> @@ -535,22 +535,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)
> -{

Thanks for cleaning this entire API up!

It actually turns out that this is slightly broken because I assumed that
detach_dev would always be called for any attach_dev. I think the only
consequence for a device that used to be assigned to domain A and is now moved to
domain B is that any TLB flushes required for A will still flush the TLB for that
device.

That's not a big deal but I'll eventually send a fix.

> -	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);

Something might complain about unused apple_dart_domain_remove_streams now.
Might make sense to drop that for now as well.



Best,

Sven

Re: [PATCH v3 02/20] iommu/apple-dart: Remove detach_dev callback

[Baolu Lu]

On 2022/12/1 1:00, Sven Peter wrote:
> On Mon, Nov 28, 2022, at 07:46, 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>
>> ---
> Reviewed-by: Sven Peter<sven@svenpeter.dev>
> 
>>   drivers/iommu/apple-dart.c | 17 -----------------
>>   1 file changed, 17 deletions(-)
>>
>> diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
>> index 4f4a323be0d0..6fbe6b275c79 100644
>> --- a/drivers/iommu/apple-dart.c
>> +++ b/drivers/iommu/apple-dart.c
>> @@ -535,22 +535,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)
>> -{
> Thanks for cleaning this entire API up!
> 
> It actually turns out that this is slightly broken because I assumed that
> detach_dev would always be called for any attach_dev. I think the only
> consequence for a device that used to be assigned to domain A and is now moved to
> domain B is that any TLB flushes required for A will still flush the TLB for that
> device.
> 
> That's not a big deal but I'll eventually send a fix.
> 
>> -	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);
> Something might complain about unused apple_dart_domain_remove_streams now.
> Might make sense to drop that for now as well.

Done. Thanks!

Best regards,
baolu

[PATCH v3 03/20] 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.

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

Re: [PATCH v3 03/20] iommu/qcom: Remove detach_dev callback

[Jason Gunthorpe]

On Mon, Nov 28, 2022 at 02:46:31PM +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/arm/arm-smmu/qcom_iommu.c | 23 -----------------------
>  1 file changed, 23 deletions(-)

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

Jason

[PATCH v3 04/20] 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.

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

Re: [PATCH v3 04/20] iommu/exynos: Remove detach_dev callback

[Jason Gunthorpe]

On Mon, Nov 28, 2022 at 02:46:32PM +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/exynos-iommu.c | 1 -
>  1 file changed, 1 deletion(-)

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

Jason

[PATCH v3 05/20] 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.

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

Re: [PATCH v3 05/20] iommu/ipmmu: Remove detach_dev callback

[Jason Gunthorpe]

On Mon, Nov 28, 2022 at 02:46:33PM +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/ipmmu-vmsa.c | 16 ----------------
>  1 file changed, 16 deletions(-)

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

Jason

[PATCH v3 06/20] 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 5dc1009a19ed..2022f47529c1 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -713,14 +713,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)
@@ -949,7 +941,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

Re: [PATCH v3 06/20] iommu/mtk: Remove detach_dev callback

[Jason Gunthorpe]

On Mon, Nov 28, 2022 at 02:46:34PM +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(-)

I listed this driver as not supporting default domains:

https://lore.kernel.org/linux-iommu/20220516135741.GV1343366@nvidia.com/

?

Has something changed? Did I get it wrong?

Jason

Re: [PATCH v3 06/20] iommu/mtk: Remove detach_dev callback

[Robin Murphy]

On 2022-11-28 13:49, Jason Gunthorpe wrote:
> On Mon, Nov 28, 2022 at 02:46:34PM +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(-)
> 
> I listed this driver as not supporting default domains:
> 
> https://lore.kernel.org/linux-iommu/20220516135741.GV1343366@nvidia.com/
> 
> ?
> 
> Has something changed? Did I get it wrong?

static struct iommu_domain *mtk_iommu_domain_alloc(unsigned type)
{
	struct mtk_iommu_domain *dom;

	if (type != IOMMU_DOMAIN_DMA && type != IOMMU_DOMAIN_UNMANAGED)
		return NULL;
...


This one runs on arm64, so has always supported default domains for 
iommu-dma to work.

Cheers,
Robin.

Re: [PATCH v3 06/20] iommu/mtk: Remove detach_dev callback

[Baolu Lu]

On 11/28/22 9:59 PM, Robin Murphy wrote:
> On 2022-11-28 13:49, Jason Gunthorpe wrote:
>> On Mon, Nov 28, 2022 at 02:46:34PM +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(-)
>>
>> I listed this driver as not supporting default domains:
>>
>> https://lore.kernel.org/linux-iommu/20220516135741.GV1343366@nvidia.com/
>>
>> ?
>>
>> Has something changed? Did I get it wrong?
> 
> static struct iommu_domain *mtk_iommu_domain_alloc(unsigned type)
> {
>      struct mtk_iommu_domain *dom;
> 
>      if (type != IOMMU_DOMAIN_DMA && type != IOMMU_DOMAIN_UNMANAGED)
>          return NULL;
> ...
> 
> 
> This one runs on arm64, so has always supported default domains for 
> iommu-dma to work.

This, together with several other ones, only support IOMMU_DOMAIN_DMA
type of default domain. The iommu core handle this by falling back to
IOMMU_DOMAIN_DMA if other types fail.

         dom = __iommu_domain_alloc(bus, type);
         if (!dom && type != IOMMU_DOMAIN_DMA) {
                 dom = __iommu_domain_alloc(bus, IOMMU_DOMAIN_DMA);
                 if (dom)
                         pr_warn("Failed to allocate default IOMMU 
domain of type %u for group %s - Falling back to IOMMU_DOMAIN_DMA",
                                 type, group->name);
         }

I have another cleanup series:

https://github.com/LuBaolu/intel-iommu/commits/iommu-use-def_default_type-wip

which adds IOMMU_DOMAIN_DMA default domain type requirement in the
def_domain_type callback. I planed to bring that to discussion after
this one.

Best regards,
baolu

Re: [PATCH v3 06/20] iommu/mtk: Remove detach_dev callback

[Robin Murphy]

On 2022-11-29 02:07, Baolu Lu wrote:
> On 11/28/22 9:59 PM, Robin Murphy wrote:
>> On 2022-11-28 13:49, Jason Gunthorpe wrote:
>>> On Mon, Nov 28, 2022 at 02:46:34PM +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(-)
>>>
>>> I listed this driver as not supporting default domains:
>>>
>>> https://lore.kernel.org/linux-iommu/20220516135741.GV1343366@nvidia.com/
>>>
>>> ?
>>>
>>> Has something changed? Did I get it wrong?
>>
>> static struct iommu_domain *mtk_iommu_domain_alloc(unsigned type)
>> {
>>      struct mtk_iommu_domain *dom;
>>
>>      if (type != IOMMU_DOMAIN_DMA && type != IOMMU_DOMAIN_UNMANAGED)
>>          return NULL;
>> ...
>>
>>
>> This one runs on arm64, so has always supported default domains for 
>> iommu-dma to work.
> 
> This, together with several other ones, only support IOMMU_DOMAIN_DMA
> type of default domain. The iommu core handle this by falling back to
> IOMMU_DOMAIN_DMA if other types fail.
> 
>          dom = __iommu_domain_alloc(bus, type);
>          if (!dom && type != IOMMU_DOMAIN_DMA) {
>                  dom = __iommu_domain_alloc(bus, IOMMU_DOMAIN_DMA);
>                  if (dom)
>                          pr_warn("Failed to allocate default IOMMU 
> domain of type %u for group %s - Falling back to IOMMU_DOMAIN_DMA",
>                                  type, group->name);
>          }
> 
> I have another cleanup series:
> 
> https://github.com/LuBaolu/intel-iommu/commits/iommu-use-def_default_type-wip
> 
> which adds IOMMU_DOMAIN_DMA default domain type requirement in the
> def_domain_type callback. I planed to bring that to discussion after
> this one.

Per the discussion over on the s390 thread, I think that would be a step 
in the wrong direction. I'd prefer to keep .def_domain_type for 
device-specific requirements and express general driver domain support a 
different way. If the IOMMU_DOMAIN_DMA fallback is worth removing then 
the one for IOMMU_DOMAIN_BLOCKED is as well - no point doing half the job ;)

Thanks,
Robin.

Re: [PATCH v3 06/20] iommu/mtk: Remove detach_dev callback

[Baolu Lu]

On 2022/11/29 19:45, Robin Murphy wrote:
> On 2022-11-29 02:07, Baolu Lu wrote:
>> On 11/28/22 9:59 PM, Robin Murphy wrote:
>>> On 2022-11-28 13:49, Jason Gunthorpe wrote:
>>>> On Mon, Nov 28, 2022 at 02:46:34PM +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(-)
>>>>
>>>> I listed this driver as not supporting default domains:
>>>>
>>>> https://lore.kernel.org/linux-iommu/20220516135741.GV1343366@nvidia.com/
>>>>
>>>> ?
>>>>
>>>> Has something changed? Did I get it wrong?
>>>
>>> static struct iommu_domain *mtk_iommu_domain_alloc(unsigned type)
>>> {
>>>      struct mtk_iommu_domain *dom;
>>>
>>>      if (type != IOMMU_DOMAIN_DMA && type != IOMMU_DOMAIN_UNMANAGED)
>>>          return NULL;
>>> ...
>>>
>>>
>>> This one runs on arm64, so has always supported default domains for 
>>> iommu-dma to work.
>>
>> This, together with several other ones, only support IOMMU_DOMAIN_DMA
>> type of default domain. The iommu core handle this by falling back to
>> IOMMU_DOMAIN_DMA if other types fail.
>>
>>          dom = __iommu_domain_alloc(bus, type);
>>          if (!dom && type != IOMMU_DOMAIN_DMA) {
>>                  dom = __iommu_domain_alloc(bus, IOMMU_DOMAIN_DMA);
>>                  if (dom)
>>                          pr_warn("Failed to allocate default IOMMU 
>> domain of type %u for group %s - Falling back to IOMMU_DOMAIN_DMA",
>>                                  type, group->name);
>>          }
>>
>> I have another cleanup series:
>>
>> https://github.com/LuBaolu/intel-iommu/commits/iommu-use-def_default_type-wip
>>
>> which adds IOMMU_DOMAIN_DMA default domain type requirement in the
>> def_domain_type callback. I planed to bring that to discussion after
>> this one.
> 
> Per the discussion over on the s390 thread, I think that would be a step 
> in the wrong direction. I'd prefer to keep .def_domain_type for 
> device-specific requirements and express general driver domain support a 
> different way. If the IOMMU_DOMAIN_DMA fallback is worth removing then 
> the one for IOMMU_DOMAIN_BLOCKED is as well - no point doing half the 
> job ;)

Get you. Thanks for pointing this out. Sure, let's keep the code.

Best regards,
baolu

[PATCH v3 07/20] 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.

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

Re: [PATCH v3 07/20] iommu/rockchip: Remove detach_dev callback

[Jason Gunthorpe]

On Mon, Nov 28, 2022 at 02:46:35PM +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/rockchip-iommu.c | 1 -
>  1 file changed, 1 deletion(-)

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

Jason

[PATCH v3 08/20] 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.

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

Re: [PATCH v3 08/20] iommu/sprd: Remove detach_dev callback

[Jason Gunthorpe]

On Mon, Nov 28, 2022 at 02:46:36PM +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/sprd-iommu.c | 16 ----------------
>  1 file changed, 16 deletions(-)

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

Jason

RE: [PATCH v3 08/20] iommu/sprd: Remove detach_dev callback

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Monday, November 28, 2022 2:47 PM
> 
> 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/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;
> -}
> -

Looks this reveals a bug in this driver (not caused by this removal).

sprd_iommu_attach_device() doesn't check whether the device has
been already attached to a domain and do auto detach.

It's written in a way that .detach_dev() must be called to free the
dma buffer but ignores the fact that it's not called when default
domain support is claimed.

Then the dma buffer allocated for the previous domain was left
unhandled, causing memory leak.

Re: [PATCH v3 08/20] iommu/sprd: Remove detach_dev callback

[Chunyan Zhang]

On Tue, 29 Nov 2022 at 11:35, Tian, Kevin <kevin.tian@intel.com> wrote:
>
> > From: Lu Baolu <baolu.lu@linux.intel.com>
> > Sent: Monday, November 28, 2022 2:47 PM
> >
> > 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/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;
> > -}
> > -
>
> Looks this reveals a bug in this driver (not caused by this removal).
>
> sprd_iommu_attach_device() doesn't check whether the device has
> been already attached to a domain and do auto detach.
>
> It's written in a way that .detach_dev() must be called to free the
> dma buffer but ignores the fact that it's not called when default
> domain support is claimed.
>
> Then the dma buffer allocated for the previous domain was left
> unhandled, causing memory leak.

I'll look into the issue, thanks for pointing this out.

Chunyan

Re: [PATCH v3 08/20] iommu/sprd: Remove detach_dev callback

[Chunyan Zhang]

On Mon, 28 Nov 2022 at 14:55, Lu Baolu <baolu.lu@linux.intel.com> 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>

Acked-by: Chunyan Zhang <zhang.lyra@gmail.com>

Thanks,
Chunyan

> ---
>  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 v3 09/20] 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.

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 5cb2d44dfb92..37b074be87a5 100644
--- a/drivers/iommu/sun50i-iommu.c
+++ b/drivers/iommu/sun50i-iommu.c
@@ -835,7 +835,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

Re: [PATCH v3 09/20] iommu/sun50i: Remove detach_dev callback

[Jason Gunthorpe]

On Mon, Nov 28, 2022 at 02:46:37PM +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/sun50i-iommu.c | 1 -
>  1 file changed, 1 deletion(-)

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

Jason

[PATCH v3 10/20] iommu: Add set_platform_dma 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 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>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 include/linux/iommu.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 68d7d304cdb7..3542461558fa 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -223,6 +223,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: Returning control back to the platform DMA ops. Only
+ *                    valid for IOMMU drivers which don't support default
+ *                    domain.
  * @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
@@ -251,6 +254,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)(struct device *dev);
 	struct iommu_group *(*device_group)(struct device *dev);
 
 	/* Request/Free a list of reserved regions for a device */
-- 
2.34.1

Re: [PATCH v3 10/20] iommu: Add set_platform_dma iommu ops

[Jason Gunthorpe]

On Mon, Nov 28, 2022 at 02:46:38PM +0800, Lu Baolu wrote:
> 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 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>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  include/linux/iommu.h | 4 ++++
>  1 file changed, 4 insertions(+)

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

> + * @set_platform_dma: Returning control back to the platform DMA ops. Only
> + *                    valid for IOMMU drivers which don't support default
> + *                    domain.

I would add:

  This op is to support old IOMMU drivers, new drivers should use
  default domains, and the common IOMMU DMA ops.

Jason

Re: [PATCH v3 10/20] iommu: Add set_platform_dma iommu ops

[Baolu Lu]

On 11/28/22 10:11 PM, Jason Gunthorpe wrote:
> On Mon, Nov 28, 2022 at 02:46:38PM +0800, Lu Baolu wrote:
>> 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 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>
>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>> ---
>>   include/linux/iommu.h | 4 ++++
>>   1 file changed, 4 insertions(+)
> Reviewed-by: Jason Gunthorpe<jgg@nvidia.com>
> 
>> + * @set_platform_dma: Returning control back to the platform DMA ops. Only
>> + *                    valid for IOMMU drivers which don't support default
>> + *                    domain.
> I would add:
> 
>    This op is to support old IOMMU drivers, new drivers should use
>    default domains, and the common IOMMU DMA ops.

Done. Thank you!

Best regards,
baolu

[PATCH v3 11/20] iommu/fsl_pamu: Add set_platform_dma callback

[Lu Baolu]

This IOMMU driver doesn't support default domain. Add the implementation
of set_platform_dma callback so that the IOMMU core could return the
DMA control.

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

diff --git a/drivers/iommu/fsl_pamu_domain.c b/drivers/iommu/fsl_pamu_domain.c
index 4408ac3c49b6..b8c716e7c424 100644
--- a/drivers/iommu/fsl_pamu_domain.c
+++ b/drivers/iommu/fsl_pamu_domain.c
@@ -314,6 +314,14 @@ static void fsl_pamu_detach_device(struct iommu_domain *domain,
 		pr_debug("missing fsl,liodn property at %pOF\n", dev->of_node);
 }
 
+static void fsl_pamu_set_platform_dma(struct device *dev)
+{
+	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+
+	if (domain)
+		fsl_pamu_detach_device(domain, dev);
+}
+
 /* Set the domain stash attribute */
 int fsl_pamu_configure_l1_stash(struct iommu_domain *domain, u32 cpu)
 {
@@ -452,6 +460,7 @@ 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 = 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,
-- 
2.34.1

Re: [PATCH v3 11/20] iommu/fsl_pamu: Add set_platform_dma callback

[Jason Gunthorpe]

On Mon, Nov 28, 2022 at 02:46:39PM +0800, Lu Baolu wrote:
> This IOMMU driver doesn't support default domain. Add the implementation
> of set_platform_dma callback so that the IOMMU core could return the
> DMA control.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/fsl_pamu_domain.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/iommu/fsl_pamu_domain.c b/drivers/iommu/fsl_pamu_domain.c
> index 4408ac3c49b6..b8c716e7c424 100644
> --- a/drivers/iommu/fsl_pamu_domain.c
> +++ b/drivers/iommu/fsl_pamu_domain.c
> @@ -314,6 +314,14 @@ static void fsl_pamu_detach_device(struct iommu_domain *domain,
>  		pr_debug("missing fsl,liodn property at %pOF\n", dev->of_node);
>  }
>  
> +static void fsl_pamu_set_platform_dma(struct device *dev)
> +{
> +	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
> +
> +	if (domain)
> +		fsl_pamu_detach_device(domain, dev);
> +}

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

Then the last patch just deletes the op and the core code

Jason

Re: [PATCH v3 11/20] iommu/fsl_pamu: Add set_platform_dma callback

[Baolu Lu]

On 11/28/22 10:14 PM, Jason Gunthorpe wrote:
> On Mon, Nov 28, 2022 at 02:46:39PM +0800, Lu Baolu wrote:
>> This IOMMU driver doesn't support default domain. Add the implementation
>> of set_platform_dma callback so that the IOMMU core could return the
>> DMA control.
>>
>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>> ---
>>   drivers/iommu/fsl_pamu_domain.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/iommu/fsl_pamu_domain.c b/drivers/iommu/fsl_pamu_domain.c
>> index 4408ac3c49b6..b8c716e7c424 100644
>> --- a/drivers/iommu/fsl_pamu_domain.c
>> +++ b/drivers/iommu/fsl_pamu_domain.c
>> @@ -314,6 +314,14 @@ static void fsl_pamu_detach_device(struct iommu_domain *domain,
>>   		pr_debug("missing fsl,liodn property at %pOF\n", dev->of_node);
>>   }
>>   
>> +static void fsl_pamu_set_platform_dma(struct device *dev)
>> +{
>> +	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
>> +
>> +	if (domain)
>> +		fsl_pamu_detach_device(domain, dev);
>> +}
> 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
> 
> Then the last patch just deletes the op and the core code

Yes. That's better. I will update the series.

Best regards,
baolu

[PATCH v3 12/20] iommu/msm: Add set_platform_dma callback

[Lu Baolu]

This IOMMU driver doesn't support default domain. Add the implementation
of set_platform_dma callback so that the IOMMU core could return the
DMA control.

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

diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
index c60624910872..3eb1bc668cad 100644
--- a/drivers/iommu/msm_iommu.c
+++ b/drivers/iommu/msm_iommu.c
@@ -470,6 +470,14 @@ static void msm_iommu_detach_dev(struct iommu_domain *domain,
 	spin_unlock_irqrestore(&msm_iommu_lock, flags);
 }
 
+static void msm_iommu_set_platform_dma(struct device *dev)
+{
+	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+
+	if (domain)
+		msm_iommu_detach_dev(domain, dev);
+}
+
 static int msm_iommu_map(struct iommu_domain *domain, unsigned long iova,
 			 phys_addr_t pa, size_t pgsize, size_t pgcount,
 			 int prot, gfp_t gfp, size_t *mapped)
@@ -678,6 +686,7 @@ 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 = msm_iommu_set_platform_dma,
 	.pgsize_bitmap = MSM_IOMMU_PGSIZES,
 	.of_xlate = qcom_iommu_of_xlate,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
-- 
2.34.1

[PATCH v3 13/20] iommu/mtk_v1: Add set_platform_dma callback

[Lu Baolu]

This IOMMU driver doesn't support default domain. Add the implementation
of set_platform_dma callback so that the IOMMU core could return the
DMA control.

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

diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index 69682ee068d2..6a815da8a020 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -326,6 +326,14 @@ static void mtk_iommu_v1_detach_device(struct iommu_domain *domain, struct devic
 	mtk_iommu_v1_config(data, dev, false);
 }
 
+static void mtk_iommu_v1_set_platform_dma(struct device *dev)
+{
+	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+
+	if (domain)
+		mtk_iommu_v1_detach_device(domain, dev);
+}
+
 static int mtk_iommu_v1_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)
@@ -585,6 +593,7 @@ 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 = mtk_iommu_v1_set_platform_dma,
 	.owner          = THIS_MODULE,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= mtk_iommu_v1_attach_device,
-- 
2.34.1

[PATCH v3 14/20] iommu/omap: Add set_platform_dma callback

[Lu Baolu]

This IOMMU driver doesn't support default domain. Add the implementation
of set_platform_dma callback so that the IOMMU core could return the
DMA control.

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

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 2fd7702c6709..aa3858a48bbf 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1566,6 +1566,14 @@ static void omap_iommu_detach_dev(struct iommu_domain *domain,
 	spin_unlock(&omap_domain->lock);
 }
 
+static void omap_iommu_set_platform_dma(struct device *dev)
+{
+	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+
+	if (domain)
+		omap_iommu_detach_dev(domain, dev);
+}
+
 static struct iommu_domain *omap_iommu_domain_alloc(unsigned type)
 {
 	struct omap_iommu_domain *omap_domain;
@@ -1737,6 +1745,7 @@ 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 = omap_iommu_set_platform_dma,
 	.pgsize_bitmap	= OMAP_IOMMU_PGSIZES,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= omap_iommu_attach_dev,
-- 
2.34.1

[PATCH v3 15/20] iommu/s390: Add set_platform_dma callback

[Lu Baolu]

This IOMMU driver doesn't support default domain. Add the implementation
of set_platform_dma callback so that the IOMMU core could return the
DMA control.

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

diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
index ed33c6cce083..507bbc2c9b58 100644
--- a/drivers/iommu/s390-iommu.c
+++ b/drivers/iommu/s390-iommu.c
@@ -179,6 +179,14 @@ static void s390_iommu_get_resv_regions(struct device *dev,
 	}
 }
 
+static void s390_iommu_set_platform_dma(struct device *dev)
+{
+	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+
+	if (domain)
+		s390_iommu_detach_device(domain, dev);
+}
+
 static struct iommu_device *s390_iommu_probe_device(struct device *dev)
 {
 	struct zpci_dev *zdev;
@@ -435,6 +443,7 @@ 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 = 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) {
-- 
2.34.1

[PATCH v3 16/20] iommu/gart: Add set_platform_dma callback

[Lu Baolu]

This IOMMU driver doesn't support default domain. Add the implementation
of set_platform_dma callback so that the IOMMU core could return the
DMA control.

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

diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index ed53279d1106..f56e40b5c47a 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -141,6 +141,14 @@ static void gart_iommu_detach_dev(struct iommu_domain *domain,
 	spin_unlock(&gart->dom_lock);
 }
 
+static void gart_iommu_set_platform_dma(struct device *dev)
+{
+	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+
+	if (domain)
+		gart_iommu_detach_dev(domain, dev);
+}
+
 static struct iommu_domain *gart_iommu_domain_alloc(unsigned type)
 {
 	struct iommu_domain *domain;
@@ -270,6 +278,7 @@ 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 = gart_iommu_set_platform_dma,
 	.pgsize_bitmap	= GART_IOMMU_PGSIZES,
 	.of_xlate	= gart_iommu_of_xlate,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
-- 
2.34.1

[PATCH v3 17/20] iommu/tegra: Add set_platform_dma callback

[Lu Baolu]

This IOMMU driver doesn't support default domain. Add the implementation
of set_platform_dma callback so that the IOMMU core could return the
DMA control.

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

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 5b1af40221ec..a36e163e5cc1 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -527,6 +527,14 @@ static void tegra_smmu_detach_dev(struct iommu_domain *domain, struct device *de
 	}
 }
 
+static void tegra_smmu_set_platform_dma(struct device *dev)
+{
+	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+
+	if (domain)
+		tegra_smmu_detach_dev(domain, dev);
+}
+
 static void tegra_smmu_set_pde(struct tegra_smmu_as *as, unsigned long iova,
 			       u32 value)
 {
@@ -965,6 +973,7 @@ 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 = tegra_smmu_set_platform_dma,
 	.of_xlate = tegra_smmu_of_xlate,
 	.pgsize_bitmap = SZ_4K,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
-- 
2.34.1

[PATCH v3 18/20] iommu: Call set_platform_dma if default domain is unavailable

[Lu Baolu]

If the IOMMU driver has no default domain support, call set_platform_dma
explicitly to return the kernel DMA control back to the platform DMA ops.

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

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 7c99d8eb3182..e4966f088184 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2040,16 +2040,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)
-{
-	if (iommu_is_attach_deferred(dev))
-		return;
-
-	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;
@@ -2154,11 +2144,12 @@ 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)
+static int iommu_group_do_set_platform_dma(struct device *dev, void *data)
 {
-	struct iommu_domain *domain = data;
+	const struct iommu_ops *ops = dev_iommu_ops(dev);
 
-	__iommu_detach_device(domain, dev);
+	if (!WARN_ON(!ops->set_platform_dma))
+		ops->set_platform_dma(dev);
 
 	return 0;
 }
@@ -2172,15 +2163,12 @@ 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
-	 * platform specific behavior.
+	 * New drivers should support default domains. Otherwise the NULL
+	 * domain represents returning control back to the platform DMA.
 	 */
 	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);
+		__iommu_group_for_each_dev(group, NULL,
+					   iommu_group_do_set_platform_dma);
 		group->domain = NULL;
 		return 0;
 	}
-- 
2.34.1

Re: [PATCH v3 18/20] iommu: Call set_platform_dma if default domain is unavailable

[Jason Gunthorpe]

On Mon, Nov 28, 2022 at 02:46:46PM +0800, Lu Baolu wrote:
> If the IOMMU driver has no default domain support, call set_platform_dma
> explicitly to return the kernel DMA control back to the platform DMA ops.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/iommu.c | 28 ++++++++--------------------
>  1 file changed, 8 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 7c99d8eb3182..e4966f088184 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2040,16 +2040,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)
> -{
> -	if (iommu_is_attach_deferred(dev))
> -		return;

This removal might want to be its own patch with an explanation.

It looks like 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.

So it should just unconditionally work. It is 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 to check if the device

This maybe the patch that is needed to explain this:

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index d69ebba81bebd8..06f1fe6563bb30 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -993,8 +993,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;
@@ -1760,21 +1760,24 @@ static void probe_alloc_default_domain(struct bus_type *bus,
 
 }
 
-static int iommu_group_do_dma_attach(struct device *dev, void *data)
+static int iommu_group_do_dma_first_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);
+	lockdep_assert_held(&dev->iommu_group->mutex);
 
-	return ret;
+	if (iommu_is_attach_deferred(dev)) {
+		dev->iommu->attach_deferred = 1;
+		return 0;
+	}
+
+	return __iommu_attach_device(domain, dev);
 }
 
-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)
@@ -1839,7 +1842,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);
 
@@ -1971,9 +1974,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;
 }
 
 /**
@@ -2018,7 +2023,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;
@@ -2027,9 +2032,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);
 }
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 1690c334e51631..ebac04a13fff68 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -413,6 +413,7 @@ struct dev_iommu {
 	struct iommu_device		*iommu_dev;
 	void				*priv;
 	u32				max_pasids;
+	u8				attach_deferred;
 };
 
 int iommu_device_register(struct iommu_device *iommu,

Re: [PATCH v3 18/20] iommu: Call set_platform_dma if default domain is unavailable

[Baolu Lu]

Hi Jason,

On 11/28/22 10:57 PM, Jason Gunthorpe wrote:
> On Mon, Nov 28, 2022 at 02:46:46PM +0800, Lu Baolu wrote:
>> If the IOMMU driver has no default domain support, call set_platform_dma
>> explicitly to return the kernel DMA control back to the platform DMA ops.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>>   drivers/iommu/iommu.c | 28 ++++++++--------------------
>>   1 file changed, 8 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 7c99d8eb3182..e4966f088184 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -2040,16 +2040,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)
>> -{
>> -	if (iommu_is_attach_deferred(dev))
>> -		return;
> 
> This removal might want to be its own patch with an explanation.
> 
> It looks like 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.
> 
> So it should just unconditionally work. It is 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 to check if the device
> 
> This maybe the patch that is needed to explain this:
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index d69ebba81bebd8..06f1fe6563bb30 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -993,8 +993,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;
> @@ -1760,21 +1760,24 @@ static void probe_alloc_default_domain(struct bus_type *bus,
>   
>   }
>   
> -static int iommu_group_do_dma_attach(struct device *dev, void *data)
> +static int iommu_group_do_dma_first_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);
> +	lockdep_assert_held(&dev->iommu_group->mutex);
>   
> -	return ret;
> +	if (iommu_is_attach_deferred(dev)) {
> +		dev->iommu->attach_deferred = 1;
> +		return 0;
> +	}
> +
> +	return __iommu_attach_device(domain, dev);
>   }
>   
> -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)
> @@ -1839,7 +1842,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);
>   
> @@ -1971,9 +1974,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;
>   }
>   
>   /**
> @@ -2018,7 +2023,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;
> @@ -2027,9 +2032,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);
>   }
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 1690c334e51631..ebac04a13fff68 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -413,6 +413,7 @@ struct dev_iommu {
>   	struct iommu_device		*iommu_dev;
>   	void				*priv;
>   	u32				max_pasids;
> +	u8				attach_deferred;
>   };
>   
>   int iommu_device_register(struct iommu_device *iommu,

Thanks for the patch! It seems that we also need to call
iommu_group_do_dma_first_attach() in the iommu_probe_device() path?

@@ -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);

By the way, I'd like to put above code in a separated patch of the next
version, can I add your signed-off-by?

--
Best regards,
baolu

Re: [PATCH v3 18/20] iommu: Call set_platform_dma if default domain is unavailable

[Jason Gunthorpe]

On Tue, Jan 03, 2023 at 10:45:24AM +0800, Baolu Lu wrote:

> Thanks for the patch! It seems that we also need to call
> iommu_group_do_dma_first_attach() in the iommu_probe_device() path?

Yes

> By the way, I'd like to put above code in a separated patch of the next
> version, can I add your signed-off-by?

Yes

Jason

[PATCH v3 19/20] iommu: Retire detach_dev callback

[Lu Baolu]

The detach_dev callback of domain ops is not used in the IOMMU core.
Remove this callback to avoid dead code.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 include/linux/iommu.h           | 2 --
 drivers/iommu/fsl_pamu_domain.c | 1 -
 drivers/iommu/msm_iommu.c       | 1 -
 drivers/iommu/mtk_iommu_v1.c    | 1 -
 drivers/iommu/omap-iommu.c      | 1 -
 drivers/iommu/s390-iommu.c      | 1 -
 drivers/iommu/tegra-gart.c      | 1 -
 drivers/iommu/tegra-smmu.c      | 1 -
 8 files changed, 9 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 3542461558fa..4c0491e5708c 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -294,7 +294,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
@@ -315,7 +314,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/drivers/iommu/fsl_pamu_domain.c b/drivers/iommu/fsl_pamu_domain.c
index b8c716e7c424..272d88e415c6 100644
--- a/drivers/iommu/fsl_pamu_domain.c
+++ b/drivers/iommu/fsl_pamu_domain.c
@@ -463,7 +463,6 @@ static const struct iommu_ops fsl_pamu_ops = {
 	.set_platform_dma = 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,
 	}
diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
index 3eb1bc668cad..564f9dc0140d 100644
--- a/drivers/iommu/msm_iommu.c
+++ b/drivers/iommu/msm_iommu.c
@@ -691,7 +691,6 @@ static struct iommu_ops msm_iommu_ops = {
 	.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,
 		/*
diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index 6a815da8a020..c2d80b7a377f 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -597,7 +597,6 @@ static const struct iommu_ops mtk_iommu_v1_ops = {
 	.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,
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index aa3858a48bbf..c3eedab00038 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1749,7 +1749,6 @@ static const struct iommu_ops omap_iommu_ops = {
 	.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,
diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
index 507bbc2c9b58..33dba5ee5e20 100644
--- a/drivers/iommu/s390-iommu.c
+++ b/drivers/iommu/s390-iommu.c
@@ -448,7 +448,6 @@ static const struct iommu_ops s390_iommu_ops = {
 	.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,
diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index f56e40b5c47a..a532b333233f 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -283,7 +283,6 @@ static const struct iommu_ops gart_iommu_ops = {
 	.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,
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index a36e163e5cc1..247d485904c1 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -978,7 +978,6 @@ static const struct iommu_ops tegra_smmu_ops = {
 	.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 v3 20/20] iommu: Rename attach_dev to set_dev

[Lu Baolu]

With the retirement of the detach_dev callback, the naming of attach_dev
isn't meaningful anymore. Rename it to set_dev to restore its real
meaning, that is, setting an iommu domain to a device.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 include/linux/iommu.h                       | 4 ++--
 drivers/iommu/amd/iommu.c                   | 2 +-
 drivers/iommu/apple-dart.c                  | 2 +-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 2 +-
 drivers/iommu/arm/arm-smmu/arm-smmu.c       | 2 +-
 drivers/iommu/arm/arm-smmu/qcom_iommu.c     | 2 +-
 drivers/iommu/exynos-iommu.c                | 2 +-
 drivers/iommu/fsl_pamu_domain.c             | 2 +-
 drivers/iommu/intel/iommu.c                 | 4 ++--
 drivers/iommu/iommu.c                       | 4 ++--
 drivers/iommu/ipmmu-vmsa.c                  | 2 +-
 drivers/iommu/msm_iommu.c                   | 2 +-
 drivers/iommu/mtk_iommu.c                   | 2 +-
 drivers/iommu/mtk_iommu_v1.c                | 2 +-
 drivers/iommu/omap-iommu.c                  | 2 +-
 drivers/iommu/rockchip-iommu.c              | 2 +-
 drivers/iommu/s390-iommu.c                  | 2 +-
 drivers/iommu/sprd-iommu.c                  | 2 +-
 drivers/iommu/sun50i-iommu.c                | 2 +-
 drivers/iommu/tegra-gart.c                  | 2 +-
 drivers/iommu/tegra-smmu.c                  | 2 +-
 drivers/iommu/virtio-iommu.c                | 2 +-
 22 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 4c0491e5708c..1def4b4bb2b9 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -281,7 +281,7 @@ struct iommu_ops {
 
 /**
  * struct iommu_domain_ops - domain specific operations
- * @attach_dev: attach an iommu domain to a device
+ * @set_dev: set an iommu domain to a device
  *  Return:
  * * 0		- success
  * * EINVAL	- can indicate that device and domain are incompatible due to
@@ -313,7 +313,7 @@ struct iommu_ops {
  * @free: Release the domain after use.
  */
 struct iommu_domain_ops {
-	int (*attach_dev)(struct iommu_domain *domain, struct device *dev);
+	int (*set_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/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index bd1970b4f48b..f628bd0f9632 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2388,7 +2388,7 @@ const struct iommu_ops amd_iommu_ops = {
 	.pgsize_bitmap	= AMD_IOMMU_PGSIZES,
 	.def_domain_type = amd_iommu_def_domain_type,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
-		.attach_dev	= amd_iommu_attach_device,
+		.set_dev	= amd_iommu_attach_device,
 		.map_pages	= amd_iommu_map_pages,
 		.unmap_pages	= amd_iommu_unmap_pages,
 		.iotlb_sync_map	= amd_iommu_iotlb_sync_map,
diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
index 6fbe6b275c79..77c9e7d3e1a2 100644
--- a/drivers/iommu/apple-dart.c
+++ b/drivers/iommu/apple-dart.c
@@ -763,7 +763,7 @@ static const struct iommu_ops apple_dart_iommu_ops = {
 	.pgsize_bitmap = -1UL, /* Restricted during dart probe */
 	.owner = THIS_MODULE,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
-		.attach_dev	= apple_dart_attach_dev,
+		.set_dev	= apple_dart_attach_dev,
 		.map_pages	= apple_dart_map_pages,
 		.unmap_pages	= apple_dart_unmap_pages,
 		.flush_iotlb_all = apple_dart_flush_iotlb_all,
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index ab160198edd6..194c304c5ee8 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2859,7 +2859,7 @@ static struct iommu_ops arm_smmu_ops = {
 	.pgsize_bitmap		= -1UL, /* Restricted during device attach */
 	.owner			= THIS_MODULE,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
-		.attach_dev		= arm_smmu_attach_dev,
+		.set_dev		= arm_smmu_attach_dev,
 		.map_pages		= arm_smmu_map_pages,
 		.unmap_pages		= arm_smmu_unmap_pages,
 		.flush_iotlb_all	= arm_smmu_flush_iotlb_all,
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index 719fbca1fe52..e31002d84b4a 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -1567,7 +1567,7 @@ static struct iommu_ops arm_smmu_ops = {
 	.pgsize_bitmap		= -1UL, /* Restricted during device attach */
 	.owner			= THIS_MODULE,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
-		.attach_dev		= arm_smmu_attach_dev,
+		.set_dev		= arm_smmu_attach_dev,
 		.map_pages		= arm_smmu_map_pages,
 		.unmap_pages		= arm_smmu_unmap_pages,
 		.flush_iotlb_all	= arm_smmu_flush_iotlb_all,
diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index d7be3adee426..195add905364 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -560,7 +560,7 @@ static const struct iommu_ops qcom_iommu_ops = {
 	.of_xlate	= qcom_iommu_of_xlate,
 	.pgsize_bitmap	= SZ_4K | SZ_64K | SZ_1M | SZ_16M,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
-		.attach_dev	= qcom_iommu_attach_dev,
+		.set_dev	= qcom_iommu_attach_dev,
 		.map_pages	= qcom_iommu_map,
 		.unmap_pages	= qcom_iommu_unmap,
 		.flush_iotlb_all = qcom_iommu_flush_iotlb_all,
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 29ec713e8a21..7e735929e395 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -1403,7 +1403,7 @@ static const struct iommu_ops exynos_iommu_ops = {
 	.pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE,
 	.of_xlate = exynos_iommu_of_xlate,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
-		.attach_dev	= exynos_iommu_attach_device,
+		.set_dev	= exynos_iommu_attach_device,
 		.map		= exynos_iommu_map,
 		.unmap		= exynos_iommu_unmap,
 		.iova_to_phys	= exynos_iommu_iova_to_phys,
diff --git a/drivers/iommu/fsl_pamu_domain.c b/drivers/iommu/fsl_pamu_domain.c
index 272d88e415c6..c66e48b0ed6f 100644
--- a/drivers/iommu/fsl_pamu_domain.c
+++ b/drivers/iommu/fsl_pamu_domain.c
@@ -462,7 +462,7 @@ static const struct iommu_ops fsl_pamu_ops = {
 	.device_group   = fsl_pamu_device_group,
 	.set_platform_dma = fsl_pamu_set_platform_dma;
 	.default_domain_ops = &(const struct iommu_domain_ops) {
-		.attach_dev	= fsl_pamu_attach_device,
+		.set_dev	= fsl_pamu_attach_device,
 		.iova_to_phys	= fsl_pamu_iova_to_phys,
 		.free		= fsl_pamu_domain_free,
 	}
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index cd71194fe7a6..7bcadb702e00 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4131,7 +4131,7 @@ static int blocking_domain_attach_dev(struct iommu_domain *domain,
 
 static struct iommu_domain blocking_domain = {
 	.ops = &(const struct iommu_domain_ops) {
-		.attach_dev	= blocking_domain_attach_dev,
+		.set_dev	= blocking_domain_attach_dev,
 		.free		= intel_iommu_domain_free
 	}
 };
@@ -4750,7 +4750,7 @@ const struct iommu_ops intel_iommu_ops = {
 	.page_response		= intel_svm_page_response,
 #endif
 	.default_domain_ops = &(const struct iommu_domain_ops) {
-		.attach_dev		= intel_iommu_attach_device,
+		.set_dev		= intel_iommu_attach_device,
 		.map_pages		= intel_iommu_map_pages,
 		.unmap_pages		= intel_iommu_unmap_pages,
 		.iotlb_sync_map		= intel_iommu_iotlb_sync_map,
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index e4966f088184..dca31065cdb5 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1983,10 +1983,10 @@ static int __iommu_attach_device(struct iommu_domain *domain,
 {
 	int ret;
 
-	if (unlikely(domain->ops->attach_dev == NULL))
+	if (unlikely(!domain->ops->set_dev))
 		return -ENODEV;
 
-	ret = domain->ops->attach_dev(domain, dev);
+	ret = domain->ops->set_dev(domain, dev);
 	if (!ret)
 		trace_attach_device_to_domain(dev);
 	return ret;
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 3112822ac7be..8d40a2c150d4 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -860,7 +860,7 @@ static const struct iommu_ops ipmmu_ops = {
 	.pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K,
 	.of_xlate = ipmmu_of_xlate,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
-		.attach_dev	= ipmmu_attach_device,
+		.set_dev	= ipmmu_attach_device,
 		.map_pages	= ipmmu_map,
 		.unmap_pages	= ipmmu_unmap,
 		.flush_iotlb_all = ipmmu_flush_iotlb_all,
diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
index 564f9dc0140d..9f7432443726 100644
--- a/drivers/iommu/msm_iommu.c
+++ b/drivers/iommu/msm_iommu.c
@@ -690,7 +690,7 @@ static struct iommu_ops msm_iommu_ops = {
 	.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,
+		.set_dev	= msm_iommu_attach_dev,
 		.map_pages	= msm_iommu_map,
 		.unmap_pages	= msm_iommu_unmap,
 		/*
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 2022f47529c1..6bd4eb39c08f 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -940,7 +940,7 @@ static const struct iommu_ops mtk_iommu_ops = {
 	.pgsize_bitmap	= SZ_4K | SZ_64K | SZ_1M | SZ_16M,
 	.owner		= THIS_MODULE,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
-		.attach_dev	= mtk_iommu_attach_device,
+		.set_dev	= mtk_iommu_attach_device,
 		.map_pages	= mtk_iommu_map,
 		.unmap_pages	= mtk_iommu_unmap,
 		.flush_iotlb_all = mtk_iommu_flush_iotlb_all,
diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index c2d80b7a377f..785fc1569bc7 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -596,7 +596,7 @@ static const struct iommu_ops mtk_iommu_v1_ops = {
 	.set_platform_dma = mtk_iommu_v1_set_platform_dma,
 	.owner          = THIS_MODULE,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
-		.attach_dev	= mtk_iommu_v1_attach_device,
+		.set_dev	= mtk_iommu_v1_attach_device,
 		.map_pages	= mtk_iommu_v1_map,
 		.unmap_pages	= mtk_iommu_v1_unmap,
 		.iova_to_phys	= mtk_iommu_v1_iova_to_phys,
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index c3eedab00038..6f1031336611 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1748,7 +1748,7 @@ static const struct iommu_ops omap_iommu_ops = {
 	.set_platform_dma = omap_iommu_set_platform_dma,
 	.pgsize_bitmap	= OMAP_IOMMU_PGSIZES,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
-		.attach_dev	= omap_iommu_attach_dev,
+		.set_dev	= omap_iommu_attach_dev,
 		.map		= omap_iommu_map,
 		.unmap		= omap_iommu_unmap,
 		.iova_to_phys	= omap_iommu_iova_to_phys,
diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index f30db22ea5d7..5381dbf624ad 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -1191,7 +1191,7 @@ static const struct iommu_ops rk_iommu_ops = {
 	.pgsize_bitmap = RK_IOMMU_PGSIZE_BITMAP,
 	.of_xlate = rk_iommu_of_xlate,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
-		.attach_dev	= rk_iommu_attach_device,
+		.set_dev	= rk_iommu_attach_device,
 		.map		= rk_iommu_map,
 		.unmap		= rk_iommu_unmap,
 		.iova_to_phys	= rk_iommu_iova_to_phys,
diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
index 33dba5ee5e20..5fa8ea6687e7 100644
--- a/drivers/iommu/s390-iommu.c
+++ b/drivers/iommu/s390-iommu.c
@@ -447,7 +447,7 @@ static const struct iommu_ops s390_iommu_ops = {
 	.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,
+		.set_dev	= s390_iommu_attach_device,
 		.map_pages	= s390_iommu_map_pages,
 		.unmap_pages	= s390_iommu_unmap_pages,
 		.flush_iotlb_all = s390_iommu_flush_iotlb_all,
diff --git a/drivers/iommu/sprd-iommu.c b/drivers/iommu/sprd-iommu.c
index ae94d74b73f4..e105fa476e94 100644
--- a/drivers/iommu/sprd-iommu.c
+++ b/drivers/iommu/sprd-iommu.c
@@ -398,7 +398,7 @@ static const struct iommu_ops sprd_iommu_ops = {
 	.pgsize_bitmap	= SPRD_IOMMU_PAGE_SIZE,
 	.owner		= THIS_MODULE,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
-		.attach_dev	= sprd_iommu_attach_device,
+		.set_dev	= sprd_iommu_attach_device,
 		.map_pages	= sprd_iommu_map,
 		.unmap_pages	= sprd_iommu_unmap,
 		.iotlb_sync_map	= sprd_iommu_sync_map,
diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
index 37b074be87a5..2a35148c2b65 100644
--- a/drivers/iommu/sun50i-iommu.c
+++ b/drivers/iommu/sun50i-iommu.c
@@ -834,7 +834,7 @@ static const struct iommu_ops sun50i_iommu_ops = {
 	.of_xlate	= sun50i_iommu_of_xlate,
 	.probe_device	= sun50i_iommu_probe_device,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
-		.attach_dev	= sun50i_iommu_attach_device,
+		.set_dev	= sun50i_iommu_attach_device,
 		.flush_iotlb_all = sun50i_iommu_flush_iotlb_all,
 		.iotlb_sync_map = sun50i_iommu_iotlb_sync_map,
 		.iotlb_sync	= sun50i_iommu_iotlb_sync,
diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index a532b333233f..2b3944d10735 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -282,7 +282,7 @@ static const struct iommu_ops gart_iommu_ops = {
 	.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,
+		.set_dev	= gart_iommu_attach_dev,
 		.map		= gart_iommu_map,
 		.unmap		= gart_iommu_unmap,
 		.iova_to_phys	= gart_iommu_iova_to_phys,
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 247d485904c1..b9f035069794 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -977,7 +977,7 @@ static const struct iommu_ops tegra_smmu_ops = {
 	.of_xlate = tegra_smmu_of_xlate,
 	.pgsize_bitmap = SZ_4K,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
-		.attach_dev	= tegra_smmu_attach_dev,
+		.set_dev	= tegra_smmu_attach_dev,
 		.map		= tegra_smmu_map,
 		.unmap		= tegra_smmu_unmap,
 		.iova_to_phys	= tegra_smmu_iova_to_phys,
diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index 5b8fe9bfa9a5..c407c4b213db 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -1028,7 +1028,7 @@ static struct iommu_ops viommu_ops = {
 	.of_xlate		= viommu_of_xlate,
 	.owner			= THIS_MODULE,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
-		.attach_dev		= viommu_attach_dev,
+		.set_dev		= viommu_attach_dev,
 		.map_pages		= viommu_map_pages,
 		.unmap_pages		= viommu_unmap_pages,
 		.iova_to_phys		= viommu_iova_to_phys,
-- 
2.34.1

Re: [PATCH v3 20/20] iommu: Rename attach_dev to set_dev

[Robin Murphy]

On 2022-11-28 06:46, Lu Baolu wrote:
> With the retirement of the detach_dev callback, the naming of attach_dev
> isn't meaningful anymore. Rename it to set_dev to restore its real
> meaning, that is, setting an iommu domain to a device.

English grammar alert: this part is confusing, since the usual 
in-context reading* of "set[ting] X to Y" is going to imply assigning a 
value of Y to some unique property of X. Given the actual semantic that 
when we attach the device to the domain, we are setting the (current) 
domain as a property of the device, I think the most logical and 
intuitive abbreviation for this method would be set_domain(), where the 
target device is then clearly implied by the argument (as the target 
domain was for attach_dev()).

FWIW 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, however I do agree that it *is* worth 
switching the terminology to clearly differentiate this internal 
behaviour from the public attach/detach API for unmanaged domains.

Thanks,
Robin.


[*] Note that It's not strictly incorrect, but "set to" in the sense of 
starting work, e.g. "At 6PM I set to cooking dinner", is much less 
commonly used, especially in code. Although I think technically 
set_dev_to(dev, domain) would work gramatically to abbreviate that sense 
of "set device to start using domain", it's still rather obscure and not 
much less ambiguous.

> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>   include/linux/iommu.h                       | 4 ++--
>   drivers/iommu/amd/iommu.c                   | 2 +-
>   drivers/iommu/apple-dart.c                  | 2 +-
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 2 +-
>   drivers/iommu/arm/arm-smmu/arm-smmu.c       | 2 +-
>   drivers/iommu/arm/arm-smmu/qcom_iommu.c     | 2 +-
>   drivers/iommu/exynos-iommu.c                | 2 +-
>   drivers/iommu/fsl_pamu_domain.c             | 2 +-
>   drivers/iommu/intel/iommu.c                 | 4 ++--
>   drivers/iommu/iommu.c                       | 4 ++--
>   drivers/iommu/ipmmu-vmsa.c                  | 2 +-
>   drivers/iommu/msm_iommu.c                   | 2 +-
>   drivers/iommu/mtk_iommu.c                   | 2 +-
>   drivers/iommu/mtk_iommu_v1.c                | 2 +-
>   drivers/iommu/omap-iommu.c                  | 2 +-
>   drivers/iommu/rockchip-iommu.c              | 2 +-
>   drivers/iommu/s390-iommu.c                  | 2 +-
>   drivers/iommu/sprd-iommu.c                  | 2 +-
>   drivers/iommu/sun50i-iommu.c                | 2 +-
>   drivers/iommu/tegra-gart.c                  | 2 +-
>   drivers/iommu/tegra-smmu.c                  | 2 +-
>   drivers/iommu/virtio-iommu.c                | 2 +-
>   22 files changed, 25 insertions(+), 25 deletions(-)
> 
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 4c0491e5708c..1def4b4bb2b9 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -281,7 +281,7 @@ struct iommu_ops {
>   
>   /**
>    * struct iommu_domain_ops - domain specific operations
> - * @attach_dev: attach an iommu domain to a device
> + * @set_dev: set an iommu domain to a device
>    *  Return:
>    * * 0		- success
>    * * EINVAL	- can indicate that device and domain are incompatible due to
> @@ -313,7 +313,7 @@ struct iommu_ops {
>    * @free: Release the domain after use.
>    */
>   struct iommu_domain_ops {
> -	int (*attach_dev)(struct iommu_domain *domain, struct device *dev);
> +	int (*set_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/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index bd1970b4f48b..f628bd0f9632 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -2388,7 +2388,7 @@ const struct iommu_ops amd_iommu_ops = {
>   	.pgsize_bitmap	= AMD_IOMMU_PGSIZES,
>   	.def_domain_type = amd_iommu_def_domain_type,
>   	.default_domain_ops = &(const struct iommu_domain_ops) {
> -		.attach_dev	= amd_iommu_attach_device,
> +		.set_dev	= amd_iommu_attach_device,
>   		.map_pages	= amd_iommu_map_pages,
>   		.unmap_pages	= amd_iommu_unmap_pages,
>   		.iotlb_sync_map	= amd_iommu_iotlb_sync_map,
> diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
> index 6fbe6b275c79..77c9e7d3e1a2 100644
> --- a/drivers/iommu/apple-dart.c
> +++ b/drivers/iommu/apple-dart.c
> @@ -763,7 +763,7 @@ static const struct iommu_ops apple_dart_iommu_ops = {
>   	.pgsize_bitmap = -1UL, /* Restricted during dart probe */
>   	.owner = THIS_MODULE,
>   	.default_domain_ops = &(const struct iommu_domain_ops) {
> -		.attach_dev	= apple_dart_attach_dev,
> +		.set_dev	= apple_dart_attach_dev,
>   		.map_pages	= apple_dart_map_pages,
>   		.unmap_pages	= apple_dart_unmap_pages,
>   		.flush_iotlb_all = apple_dart_flush_iotlb_all,
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index ab160198edd6..194c304c5ee8 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2859,7 +2859,7 @@ static struct iommu_ops arm_smmu_ops = {
>   	.pgsize_bitmap		= -1UL, /* Restricted during device attach */
>   	.owner			= THIS_MODULE,
>   	.default_domain_ops = &(const struct iommu_domain_ops) {
> -		.attach_dev		= arm_smmu_attach_dev,
> +		.set_dev		= arm_smmu_attach_dev,
>   		.map_pages		= arm_smmu_map_pages,
>   		.unmap_pages		= arm_smmu_unmap_pages,
>   		.flush_iotlb_all	= arm_smmu_flush_iotlb_all,
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index 719fbca1fe52..e31002d84b4a 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -1567,7 +1567,7 @@ static struct iommu_ops arm_smmu_ops = {
>   	.pgsize_bitmap		= -1UL, /* Restricted during device attach */
>   	.owner			= THIS_MODULE,
>   	.default_domain_ops = &(const struct iommu_domain_ops) {
> -		.attach_dev		= arm_smmu_attach_dev,
> +		.set_dev		= arm_smmu_attach_dev,
>   		.map_pages		= arm_smmu_map_pages,
>   		.unmap_pages		= arm_smmu_unmap_pages,
>   		.flush_iotlb_all	= arm_smmu_flush_iotlb_all,
> diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> index d7be3adee426..195add905364 100644
> --- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> +++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> @@ -560,7 +560,7 @@ static const struct iommu_ops qcom_iommu_ops = {
>   	.of_xlate	= qcom_iommu_of_xlate,
>   	.pgsize_bitmap	= SZ_4K | SZ_64K | SZ_1M | SZ_16M,
>   	.default_domain_ops = &(const struct iommu_domain_ops) {
> -		.attach_dev	= qcom_iommu_attach_dev,
> +		.set_dev	= qcom_iommu_attach_dev,
>   		.map_pages	= qcom_iommu_map,
>   		.unmap_pages	= qcom_iommu_unmap,
>   		.flush_iotlb_all = qcom_iommu_flush_iotlb_all,
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index 29ec713e8a21..7e735929e395 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -1403,7 +1403,7 @@ static const struct iommu_ops exynos_iommu_ops = {
>   	.pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE,
>   	.of_xlate = exynos_iommu_of_xlate,
>   	.default_domain_ops = &(const struct iommu_domain_ops) {
> -		.attach_dev	= exynos_iommu_attach_device,
> +		.set_dev	= exynos_iommu_attach_device,
>   		.map		= exynos_iommu_map,
>   		.unmap		= exynos_iommu_unmap,
>   		.iova_to_phys	= exynos_iommu_iova_to_phys,
> diff --git a/drivers/iommu/fsl_pamu_domain.c b/drivers/iommu/fsl_pamu_domain.c
> index 272d88e415c6..c66e48b0ed6f 100644
> --- a/drivers/iommu/fsl_pamu_domain.c
> +++ b/drivers/iommu/fsl_pamu_domain.c
> @@ -462,7 +462,7 @@ static const struct iommu_ops fsl_pamu_ops = {
>   	.device_group   = fsl_pamu_device_group,
>   	.set_platform_dma = fsl_pamu_set_platform_dma;
>   	.default_domain_ops = &(const struct iommu_domain_ops) {
> -		.attach_dev	= fsl_pamu_attach_device,
> +		.set_dev	= fsl_pamu_attach_device,
>   		.iova_to_phys	= fsl_pamu_iova_to_phys,
>   		.free		= fsl_pamu_domain_free,
>   	}
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index cd71194fe7a6..7bcadb702e00 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4131,7 +4131,7 @@ static int blocking_domain_attach_dev(struct iommu_domain *domain,
>   
>   static struct iommu_domain blocking_domain = {
>   	.ops = &(const struct iommu_domain_ops) {
> -		.attach_dev	= blocking_domain_attach_dev,
> +		.set_dev	= blocking_domain_attach_dev,
>   		.free		= intel_iommu_domain_free
>   	}
>   };
> @@ -4750,7 +4750,7 @@ const struct iommu_ops intel_iommu_ops = {
>   	.page_response		= intel_svm_page_response,
>   #endif
>   	.default_domain_ops = &(const struct iommu_domain_ops) {
> -		.attach_dev		= intel_iommu_attach_device,
> +		.set_dev		= intel_iommu_attach_device,
>   		.map_pages		= intel_iommu_map_pages,
>   		.unmap_pages		= intel_iommu_unmap_pages,
>   		.iotlb_sync_map		= intel_iommu_iotlb_sync_map,
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index e4966f088184..dca31065cdb5 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1983,10 +1983,10 @@ static int __iommu_attach_device(struct iommu_domain *domain,
>   {
>   	int ret;
>   
> -	if (unlikely(domain->ops->attach_dev == NULL))
> +	if (unlikely(!domain->ops->set_dev))
>   		return -ENODEV;
>   
> -	ret = domain->ops->attach_dev(domain, dev);
> +	ret = domain->ops->set_dev(domain, dev);
>   	if (!ret)
>   		trace_attach_device_to_domain(dev);
>   	return ret;
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index 3112822ac7be..8d40a2c150d4 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -860,7 +860,7 @@ static const struct iommu_ops ipmmu_ops = {
>   	.pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K,
>   	.of_xlate = ipmmu_of_xlate,
>   	.default_domain_ops = &(const struct iommu_domain_ops) {
> -		.attach_dev	= ipmmu_attach_device,
> +		.set_dev	= ipmmu_attach_device,
>   		.map_pages	= ipmmu_map,
>   		.unmap_pages	= ipmmu_unmap,
>   		.flush_iotlb_all = ipmmu_flush_iotlb_all,
> diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
> index 564f9dc0140d..9f7432443726 100644
> --- a/drivers/iommu/msm_iommu.c
> +++ b/drivers/iommu/msm_iommu.c
> @@ -690,7 +690,7 @@ static struct iommu_ops msm_iommu_ops = {
>   	.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,
> +		.set_dev	= msm_iommu_attach_dev,
>   		.map_pages	= msm_iommu_map,
>   		.unmap_pages	= msm_iommu_unmap,
>   		/*
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 2022f47529c1..6bd4eb39c08f 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -940,7 +940,7 @@ static const struct iommu_ops mtk_iommu_ops = {
>   	.pgsize_bitmap	= SZ_4K | SZ_64K | SZ_1M | SZ_16M,
>   	.owner		= THIS_MODULE,
>   	.default_domain_ops = &(const struct iommu_domain_ops) {
> -		.attach_dev	= mtk_iommu_attach_device,
> +		.set_dev	= mtk_iommu_attach_device,
>   		.map_pages	= mtk_iommu_map,
>   		.unmap_pages	= mtk_iommu_unmap,
>   		.flush_iotlb_all = mtk_iommu_flush_iotlb_all,
> diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
> index c2d80b7a377f..785fc1569bc7 100644
> --- a/drivers/iommu/mtk_iommu_v1.c
> +++ b/drivers/iommu/mtk_iommu_v1.c
> @@ -596,7 +596,7 @@ static const struct iommu_ops mtk_iommu_v1_ops = {
>   	.set_platform_dma = mtk_iommu_v1_set_platform_dma,
>   	.owner          = THIS_MODULE,
>   	.default_domain_ops = &(const struct iommu_domain_ops) {
> -		.attach_dev	= mtk_iommu_v1_attach_device,
> +		.set_dev	= mtk_iommu_v1_attach_device,
>   		.map_pages	= mtk_iommu_v1_map,
>   		.unmap_pages	= mtk_iommu_v1_unmap,
>   		.iova_to_phys	= mtk_iommu_v1_iova_to_phys,
> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
> index c3eedab00038..6f1031336611 100644
> --- a/drivers/iommu/omap-iommu.c
> +++ b/drivers/iommu/omap-iommu.c
> @@ -1748,7 +1748,7 @@ static const struct iommu_ops omap_iommu_ops = {
>   	.set_platform_dma = omap_iommu_set_platform_dma,
>   	.pgsize_bitmap	= OMAP_IOMMU_PGSIZES,
>   	.default_domain_ops = &(const struct iommu_domain_ops) {
> -		.attach_dev	= omap_iommu_attach_dev,
> +		.set_dev	= omap_iommu_attach_dev,
>   		.map		= omap_iommu_map,
>   		.unmap		= omap_iommu_unmap,
>   		.iova_to_phys	= omap_iommu_iova_to_phys,
> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> index f30db22ea5d7..5381dbf624ad 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c
> @@ -1191,7 +1191,7 @@ static const struct iommu_ops rk_iommu_ops = {
>   	.pgsize_bitmap = RK_IOMMU_PGSIZE_BITMAP,
>   	.of_xlate = rk_iommu_of_xlate,
>   	.default_domain_ops = &(const struct iommu_domain_ops) {
> -		.attach_dev	= rk_iommu_attach_device,
> +		.set_dev	= rk_iommu_attach_device,
>   		.map		= rk_iommu_map,
>   		.unmap		= rk_iommu_unmap,
>   		.iova_to_phys	= rk_iommu_iova_to_phys,
> diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
> index 33dba5ee5e20..5fa8ea6687e7 100644
> --- a/drivers/iommu/s390-iommu.c
> +++ b/drivers/iommu/s390-iommu.c
> @@ -447,7 +447,7 @@ static const struct iommu_ops s390_iommu_ops = {
>   	.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,
> +		.set_dev	= s390_iommu_attach_device,
>   		.map_pages	= s390_iommu_map_pages,
>   		.unmap_pages	= s390_iommu_unmap_pages,
>   		.flush_iotlb_all = s390_iommu_flush_iotlb_all,
> diff --git a/drivers/iommu/sprd-iommu.c b/drivers/iommu/sprd-iommu.c
> index ae94d74b73f4..e105fa476e94 100644
> --- a/drivers/iommu/sprd-iommu.c
> +++ b/drivers/iommu/sprd-iommu.c
> @@ -398,7 +398,7 @@ static const struct iommu_ops sprd_iommu_ops = {
>   	.pgsize_bitmap	= SPRD_IOMMU_PAGE_SIZE,
>   	.owner		= THIS_MODULE,
>   	.default_domain_ops = &(const struct iommu_domain_ops) {
> -		.attach_dev	= sprd_iommu_attach_device,
> +		.set_dev	= sprd_iommu_attach_device,
>   		.map_pages	= sprd_iommu_map,
>   		.unmap_pages	= sprd_iommu_unmap,
>   		.iotlb_sync_map	= sprd_iommu_sync_map,
> diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
> index 37b074be87a5..2a35148c2b65 100644
> --- a/drivers/iommu/sun50i-iommu.c
> +++ b/drivers/iommu/sun50i-iommu.c
> @@ -834,7 +834,7 @@ static const struct iommu_ops sun50i_iommu_ops = {
>   	.of_xlate	= sun50i_iommu_of_xlate,
>   	.probe_device	= sun50i_iommu_probe_device,
>   	.default_domain_ops = &(const struct iommu_domain_ops) {
> -		.attach_dev	= sun50i_iommu_attach_device,
> +		.set_dev	= sun50i_iommu_attach_device,
>   		.flush_iotlb_all = sun50i_iommu_flush_iotlb_all,
>   		.iotlb_sync_map = sun50i_iommu_iotlb_sync_map,
>   		.iotlb_sync	= sun50i_iommu_iotlb_sync,
> diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
> index a532b333233f..2b3944d10735 100644
> --- a/drivers/iommu/tegra-gart.c
> +++ b/drivers/iommu/tegra-gart.c
> @@ -282,7 +282,7 @@ static const struct iommu_ops gart_iommu_ops = {
>   	.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,
> +		.set_dev	= gart_iommu_attach_dev,
>   		.map		= gart_iommu_map,
>   		.unmap		= gart_iommu_unmap,
>   		.iova_to_phys	= gart_iommu_iova_to_phys,
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index 247d485904c1..b9f035069794 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -977,7 +977,7 @@ static const struct iommu_ops tegra_smmu_ops = {
>   	.of_xlate = tegra_smmu_of_xlate,
>   	.pgsize_bitmap = SZ_4K,
>   	.default_domain_ops = &(const struct iommu_domain_ops) {
> -		.attach_dev	= tegra_smmu_attach_dev,
> +		.set_dev	= tegra_smmu_attach_dev,
>   		.map		= tegra_smmu_map,
>   		.unmap		= tegra_smmu_unmap,
>   		.iova_to_phys	= tegra_smmu_iova_to_phys,
> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> index 5b8fe9bfa9a5..c407c4b213db 100644
> --- a/drivers/iommu/virtio-iommu.c
> +++ b/drivers/iommu/virtio-iommu.c
> @@ -1028,7 +1028,7 @@ static struct iommu_ops viommu_ops = {
>   	.of_xlate		= viommu_of_xlate,
>   	.owner			= THIS_MODULE,
>   	.default_domain_ops = &(const struct iommu_domain_ops) {
> -		.attach_dev		= viommu_attach_dev,
> +		.set_dev		= viommu_attach_dev,
>   		.map_pages		= viommu_map_pages,
>   		.unmap_pages		= viommu_unmap_pages,
>   		.iova_to_phys		= viommu_iova_to_phys,

Re: [PATCH v3 20/20] iommu: Rename attach_dev to set_dev

[Jason Gunthorpe]

On Mon, Nov 28, 2022 at 01:41:56PM +0000, Robin Murphy wrote:
> On 2022-11-28 06:46, Lu Baolu wrote:
> > With the retirement of the detach_dev callback, the naming of attach_dev
> > isn't meaningful anymore. Rename it to set_dev to restore its real
> > meaning, that is, setting an iommu domain to a device.
> 
> English grammar alert: this part is confusing, since the usual in-context
> reading* of "set[ting] X to Y" is going to imply assigning a value of Y to
> some unique property of X. Given the actual semantic that when we attach the
> device to the domain, we are setting the (current) domain as a property of
> the device, I think the most logical and intuitive abbreviation for this
> method would be set_domain(), where the target device is then clearly
> implied by the argument (as the target domain was for attach_dev()).

This is the iommu_domain_ops, it seems a bit weird to call it
set_domain when it is already acting on a domain object.

set_device_domain()

?

> FWIW 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, however I do agree that it *is* worth switching the
> terminology to clearly differentiate this internal behaviour from the public
> attach/detach API for unmanaged domains.

+1

Jason

Re: [PATCH v3 20/20] iommu: Rename attach_dev to set_dev

[Robin Murphy]

On 2022-11-28 15:00, Jason Gunthorpe wrote:
> On Mon, Nov 28, 2022 at 01:41:56PM +0000, Robin Murphy wrote:
>> On 2022-11-28 06:46, Lu Baolu wrote:
>>> With the retirement of the detach_dev callback, the naming of attach_dev
>>> isn't meaningful anymore. Rename it to set_dev to restore its real
>>> meaning, that is, setting an iommu domain to a device.
>>
>> English grammar alert: this part is confusing, since the usual in-context
>> reading* of "set[ting] X to Y" is going to imply assigning a value of Y to
>> some unique property of X. Given the actual semantic that when we attach the
>> device to the domain, we are setting the (current) domain as a property of
>> the device, I think the most logical and intuitive abbreviation for this
>> method would be set_domain(), where the target device is then clearly
>> implied by the argument (as the target domain was for attach_dev()).
> 
> This is the iommu_domain_ops, it seems a bit weird to call it
> set_domain when it is already acting on a domain object.
> 
> set_device_domain()
> 
> ?

Ah, the iommu_domain_ops split had completely slipped my mind - maybe 
with that additional context, assign_dev() might work well enough to 
maintain the pattern while still being sufficiently different?

Otherwise, set_device_domain() (or just set_dev_domain()) sounds fair to me.

Cheers,
Robin.

RE: [PATCH v3 20/20] iommu: Rename attach_dev to set_dev

[Tian, Kevin]

> From: Robin Murphy <robin.murphy@arm.com>
> Sent: Monday, November 28, 2022 11:53 PM
> 
> On 2022-11-28 15:00, Jason Gunthorpe wrote:
> > On Mon, Nov 28, 2022 at 01:41:56PM +0000, Robin Murphy wrote:
> >> On 2022-11-28 06:46, Lu Baolu wrote:
> >>> With the retirement of the detach_dev callback, the naming of
> attach_dev
> >>> isn't meaningful anymore. Rename it to set_dev to restore its real
> >>> meaning, that is, setting an iommu domain to a device.
> >>
> >> English grammar alert: this part is confusing, since the usual in-context
> >> reading* of "set[ting] X to Y" is going to imply assigning a value of Y to
> >> some unique property of X. Given the actual semantic that when we
> attach the
> >> device to the domain, we are setting the (current) domain as a property of
> >> the device, I think the most logical and intuitive abbreviation for this
> >> method would be set_domain(), where the target device is then clearly
> >> implied by the argument (as the target domain was for attach_dev()).
> >
> > This is the iommu_domain_ops, it seems a bit weird to call it
> > set_domain when it is already acting on a domain object.
> >
> > set_device_domain()
> >
> > ?
> 
> Ah, the iommu_domain_ops split had completely slipped my mind - maybe
> with that additional context, assign_dev() might work well enough to
> maintain the pattern while still being sufficiently different?
> 
> Otherwise, set_device_domain() (or just set_dev_domain()) sounds fair to
> me.
> 

This kind of introduces a new concept as 'device domain'...

I prefer to device_set_domain() similar to __iommu_group_set_domain().

Then set_platform_dma() can be device_set_platform_dma().

Both have 'device' as the subject to differentiate from other domain ops.