On Mon, 2015-12-14 at 15:16 +0100, Joerg Roedel wrote:

On Tue, Dec 08, 2015 at 05:49:12PM +0800, Yong Wu wrote:

quoted

+static int mtk_iommu_attach_device(struct iommu_domain *domain,
+				   struct device *dev)
+{
+	struct mtk_iommu_domain *dom = to_mtk_domain(domain);
+	struct mtk_iommu_client_priv *priv = dev->archdata.iommu;
+	struct mtk_iommu_data *data;
+	int ret;
+
+	if (!priv)
+		return -ENODEV;
+
+	data = dev_get_drvdata(priv->m4udev);
+	if (!data) {
+		/*
+		 * The DMA core will run earlier than this probe, and it will
+		 * create a default iommu domain for each a iommu device.
+		 * But here there is only one domain called the m4u domain
+		 * which all the multimedia HW share.
+		 * The default domain isn't needed here.
+		 */

The iommu core creates one domain per iommu-group. In your case this
means one default domain per iommu in the system.

Yes. The iommu core will create one domain per iommu-group.
see the next "if" here.

But the domain here is created by the current DMA64. It's from this
function do_iommu_attach which will be called too early and will help
create a default domain for each a iommu device.(my codebase is
v4.4-rc1).


//=====the next "if"===========
} else if (!data->m4u_dom) {
	/*
	 * While a device is added into a iommu group, the iommu core
	 * will create a default domain for each a iommu group.
	 * This default domain is reserved as the m4u domain and is
	 * initiated here.
	 */
	data->m4u_dom = dom;
	if (domain->type == IOMMU_DOMAIN_DMA) {
		ret = iommu_dma_init_domain(domain, 0,
					    DMA_BIT_MASK(32));
		if (ret)
			goto err_uninit_dom;
	}

	ret = mtk_iommu_domain_finalise(data);
	if (ret)
		goto err_uninit_dom;
} 
//======================

quoted

+		iommu_domain_free(domain);

This function is not supposed to free the domain passed to it.

As above this domain is created in the do_iommu_attach which will help
create a default domain for each a iommu device.
We don't need this default domain!

If we don't free it here, there will be a memory leak.

From Robin's comment, He will improve the sequence of the

__iommu_setup_dma_ops in the future.

/*
 * TODO: Right now __iommu_setup_dma_ops() gets called too early to do
 * everything it needs to - the device is only partially created and the
 * IOMMU driver hasn't seen it yet, so it can't have a group. Thus we
 * need this delayed attachment dance. Once IOMMU probe ordering is
sorted
 * to move the arch_setup_dma_ops() call later, all the notifier bits
below
 * become unnecessary, and will go away.
 */

/*
 * Best case: The device is either part of a group which was
 * already attached to a domain in a previous call, or it's
 * been put in a default DMA domain by the IOMMU core.
 */

   But there is no this patch currently, so I add iommu_domain_free
here.
 
   "free the domain" here looks really not good. Then I delete the
iommu_domain_free here(allow this memory leak right now), is it ok?
(It will also works after Robin's change in the future.)

quoted

+static int mtk_iommu_add_device(struct device *dev)
+{
+	struct iommu_group *group;
+
+	if (!dev->archdata.iommu) /* Not a iommu client device */
+		return -ENODEV;
+
+	group = iommu_group_get_for_dev(dev);
+	if (IS_ERR(group))
+		return PTR_ERR(group);
+
+	iommu_group_put(group);
+	return 0;
+}

[...]

quoted

+static struct iommu_group *mtk_iommu_device_group(struct device *dev)
+{
+	struct mtk_iommu_data *data;
+	struct mtk_iommu_client_priv *priv;
+
+	priv = dev->archdata.iommu;
+	if (!priv)
+		return ERR_PTR(-ENODEV);
+
+	/* All the client devices are in the same m4u iommu-group */
+	data = dev_get_drvdata(priv->m4udev);
+	if (!data->m4u_group) {
+		data->m4u_group = iommu_group_alloc();
+		if (IS_ERR(data->m4u_group))
+			dev_err(dev, "Failed to allocate M4U IOMMU group\n");
+	}
+	return data->m4u_group;
+}

This looks much better than before, thanks.

Thanks.