On Mon, Aug 14, 2023 at 11:06:12AM +0800, Baolu Lu wrote:

On 2023/8/3 8:07, Jason Gunthorpe wrote:

quoted

What mtk does during mtk_iommu_v1_set_platform_dma() is actually putting
the iommu into identity mode. Make this available as a proper IDENTITY
domain.

The mtk_iommu_v1_def_domain_type() from
commit 8bbe13f52cb7 ("iommu/mediatek-v1: Add def_domain_type") explains
this was needed to allow probe_finalize() to be called, but now the
IDENTITY domain will do the same job so change the returned
def_domain_type.

mkt_v1 is the only driver that returns IOMMU_DOMAIN_UNMANAGED from
def_domain_type().  This allows the next patch to enforce an IDENTITY
domain policy for this driver.

This code seems to be not working properly.

 * @def_domain_type: device default domain type, return value:
 *              - IOMMU_DOMAIN_IDENTITY: must use an identity domain
 *              - IOMMU_DOMAIN_DMA: must use a dma domain
 *              - 0: use the default setting

The core code is not ready to accept any other return value.

Right, it is not following the spec. The design does do what it is
supposed to though..

quoted

diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index 8a0a5e5d049f4a..cc3e7d53d33ad9 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c

@@ -319,11 +319,27 @@ static int mtk_iommu_v1_attach_device(struct iommu_domain *domain, struct device
  	return 0;
  }
-static void mtk_iommu_v1_set_platform_dma(struct device *dev)
+static int mtk_iommu_v1_identity_attach(struct iommu_domain *identity_domain,
+					struct device *dev)
  {
  	struct mtk_iommu_v1_data *data = dev_iommu_priv_get(dev);
  	mtk_iommu_v1_config(data, dev, false);
+	return 0;
+}
+
+static struct iommu_domain_ops mtk_iommu_v1_identity_ops = {
+	.attach_dev = mtk_iommu_v1_identity_attach,
+};
+
+static struct iommu_domain mtk_iommu_v1_identity_domain = {
+	.type = IOMMU_DOMAIN_IDENTITY,
+	.ops = &mtk_iommu_v1_identity_ops,
+};
+
+static void mtk_iommu_v1_set_platform_dma(struct device *dev)
+{
+	mtk_iommu_v1_identity_attach(&mtk_iommu_v1_identity_domain, dev);

This callback seems to be a dead code now.

Not yet in the series, it is still used. All this patch does is
introduce the identity domain.

quoted

@@ -443,7 +459,7 @@ static int mtk_iommu_v1_create_mapping(struct device *dev, struct of_phandle_arg
  static int mtk_iommu_v1_def_domain_type(struct device *dev)
  {
-	return IOMMU_DOMAIN_UNMANAGED;
+	return IOMMU_DOMAIN_IDENTITY;

def_domain_type can't be used for this purpose. But this seems to be a
temporary code, as it will be removed in patch 09/25.

It looked OK when I checked it, mkt_v1 is really confusing what it
tries to do, but it should call probe_finalize and basically do the
same hacky thing as what UNMANAGED was trying to accomplish.

Did you see something else?

Jason