Re: [PATCH v6 4/5] iommu/mediatek: Add mt8173 IOMMU driver

[Yong Wu <yong.wu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>]

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;
> > +}
> 
> This looks much better than before, thanks.

Thanks.

>