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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1450150132.22854.57.camel@mhfsdcap03>
Date:	Tue, 15 Dec 2015 11:28:52 +0800
From:	Yong Wu <yong.wu@...iatek.com>
To:	Joerg Roedel <joro@...tes.org>
CC:	Thierry Reding <treding@...dia.com>,
	Mark Rutland <mark.rutland@....com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	"Robin Murphy" <robin.murphy@....com>,
	Will Deacon <will.deacon@....com>,
	"Daniel Kurtz" <djkurtz@...gle.com>,
	Tomasz Figa <tfiga@...gle.com>,
	Lucas Stach <l.stach@...gutronix.de>,
	Rob Herring <robh+dt@...nel.org>,
	"Catalin Marinas" <catalin.marinas@....com>,
	<linux-mediatek@...ts.infradead.org>,
	Sasha Hauer <kernel@...gutronix.de>,
	<srv_heupstream@...iatek.com>, <devicetree@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>,
	<iommu@...ts.linux-foundation.org>, <pebolle@...cali.nl>,
	<arnd@...db.de>, <mitchelh@...eaurora.org>,
	<p.zabel@...gutronix.de>, <yingjoe.chen@...iatek.com>
Subject: Re: [PATCH v6 4/5] iommu/mediatek: Add mt8173 IOMMU driver

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.

> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/