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:
+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;
}
//======================
+ 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.)
+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;
+}
[...]
+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;
+}