Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1291218 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v6 4/5] iommu/mediatek: Add mt8173 IOMMU driver Joerg Roedel <joro@8bytes.org> - 2015-12-14 15:20 +0100
    Re: [PATCH v6 4/5] iommu/mediatek: Add mt8173 IOMMU driver Yong Wu <yong.wu@mediatek.com> - 2015-12-15 04:30 +0100
      Re: [PATCH v6 4/5] iommu/mediatek: Add mt8173 IOMMU driver Robin Murphy <robin.murphy@arm.com> - 2015-12-15 13:40 +0100
        Re: [PATCH v6 4/5] iommu/mediatek: Add mt8173 IOMMU driver Yong Wu <yong.wu@mediatek.com> - 2015-12-16 07:10 +0100
          Re: [PATCH v6 4/5] iommu/mediatek: Add mt8173 IOMMU driver Robin Murphy <robin.murphy@arm.com> - 2015-12-16 13:50 +0100
            Re: [PATCH v6 4/5] iommu/mediatek: Add mt8173 IOMMU driver Yong Wu <yong.wu@mediatek.com> - 2015-12-17 04:20 +0100
        Re: [PATCH v6 4/5] iommu/mediatek: Add mt8173 IOMMU driver Joerg Roedel <joro@8bytes.org> - 2015-12-16 16:20 +0100

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

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.

> +		iommu_domain_free(domain);

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

> +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.

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

[toc] | [next] | [standalone]

#1291815

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1292113

On 15/12/15 03:28, Yong Wu wrote:
> 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).

I still don't really understand the problem here. The M4U device is 
created early in mtk_iommu_init_fn, so it should be probed and ready to 
go long before anything else. Indeed, for your mtk_iommu_device_group() 
callback to work then everything must already be happening in the right 
order - add_device will only call iommu_group_get_for_dev() if of_xlate 
has populated dev->archdata.iommu, and of_xlate will only do that if the 
M4U was up and running before the client device started probing. 
Furthermore, if mtk_iommu_device_group() *does* work, then the IOMMU 
core will go ahead and allocate the default domain there and then, which 
the arch code should find and use later.

The potential issue I *do* see, looking more closely, is that 
iommu_group_get_for_dev() is setting group->domain but not calling the 
attach_dev callback, which looks wrong...

>
> //=====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.

That already happened. The final version of the arm64 code which was 
merged makes sure that the IOMMU driver always sees the callbacks in the 
desired of_xlate -> add_device -> attach_dev order. The whole point of 
the comment below is that the driver itself *doesn't* have to care about 
the awkward way in which that is currently achieved.

> /*
>   * 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.
>   */

That was before Joerg made the device_group changes which enabled proper 
default domains for platform devices - with those, we should be now be 
hitting the "best case" behaviour every time. In fact I think the "fake 
default domain" workaround shouldn't be needed at all any more, so I 
will add removing it to my giant list of things to do.

>     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;
>>> +}

As long as this works as expected, then AFAICS you shouldn't have to 
have *any* special-case behaviour or tracking of domains at all.

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

[toc] | [prev] | [next] | [standalone]

#1292783

On Tue, 2015-12-15 at 12:37 +0000, Robin Murphy wrote:
> On 15/12/15 03:28, Yong Wu wrote:
> > 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).
> 
> I still don't really understand the problem here. The M4U device is 
> created early in mtk_iommu_init_fn, so it should be probed and ready to 
> go long before anything else. Indeed, for your mtk_iommu_device_group() 
> callback to work then everything must already be happening in the right 
> order - add_device will only call iommu_group_get_for_dev() if of_xlate 
> has populated dev->archdata.iommu, and of_xlate will only do that if the 
> M4U was up and running before the client device started probing. 
> Furthermore, if mtk_iommu_device_group() *does* work, then the IOMMU 
> core will go ahead and allocate the default domain there and then, which 
> the arch code should find and use later.

Thanks. This is very helpful.

I understand your confuse right now and your expectant flow.

Our IOMMU probe was PROBE_DEFER by our SMI device, so currently it probe
was delayed, then have to add the workaround code.

Following your comment above, I test as below. Then the flows seems meet
the "best case" that the iommu core will help create default DMA domain.

@@ -664,19 +636,41 @@ static int mtk_iommu_probe(struct platform_device
*pdev)
for (i = 0; i < larb_nr; i++) {
		struct device_node *larbnode;
		struct platform_device *plarbdev;

		larbnode = of_parse_phandle(dev->of_node, "mediatek,larbs", i);
		if (!larbnode)
			return -EINVAL;

                plarbdev = of_find_device_by_node(larbnode);
                of_node_put(larbnode);
-               if (!plarbdev)
-                       return -EPROBE_DEFER;
+               if (!plarbdev) {
+                       plarbdev = of_platform_device_create(larbnode,
NULL, platform_bus_type.dev_root);
+                       if (IS_ERR(pdev))
+                               return -EPROBE_DEFER;
+               }
}

I only add of_platform_device_create for the SMI local arbiter devices
here.

This is a big improvement for us. If this is ok, I will send a quick
next version for this.

> 
> The potential issue I *do* see, looking more closely, is that 
> iommu_group_get_for_dev() is setting group->domain but not calling the 
> attach_dev callback, which looks wrong...

This is the backtrace,

(151216_09:58:05.207)Call trace:
(151216_09:58:05.207)[<ffffffc000400668>] mtk_iommu_attach_device
+0xb8/0x178
(151216_09:58:05.207)[<ffffffc0003fc55c>] iommu_group_add_device
+0x1d8/0x31c
(151216_09:58:05.207)[<ffffffc0003fc988>] iommu_group_get_for_dev
+0x88/0x108
(151216_09:58:05.207)[<ffffffc0003ffcfc>] mtk_iommu_add_device+0x14/0x34
(151216_09:58:05.207)[<ffffffc0003fb280>] add_iommu_group+0x20/0x44
(151216_09:58:05.207)[<ffffffc000406cec>] bus_for_each_dev+0x58/0x98
(151216_09:58:05.207)[<ffffffc0003fbe8c>] bus_set_iommu+0x9c/0xf8

If I change like above, I will delete the workaround code..

> 
> >
> > //=====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.
> 
> That already happened. The final version of the arm64 code which was 
> merged makes sure that the IOMMU driver always sees the callbacks in the 
> desired of_xlate -> add_device -> attach_dev order. The whole point of 
> the comment below is that the driver itself *doesn't* have to care about 
> the awkward way in which that is currently achieved.
> 
> > /*
> >   * 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.
> >   */
> 
> That was before Joerg made the device_group changes which enabled proper 
> default domains for platform devices - with those, we should be now be 
> hitting the "best case" behaviour every time. In fact I think the "fake 
> default domain" workaround shouldn't be needed at all any more, so I 
> will add removing it to my giant list of things to do.
> 
> >     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;
> >>> +}
> 
> As long as this works as expected, then AFAICS you shouldn't have to 
> have *any* special-case behaviour or tracking of domains at all.

We only need one iommu-group, one iommu domain here.

What's the special-case behavior, how can we track of domains.
Could you help give me a example?

> 
> Robin.


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

[toc] | [prev] | [next] | [standalone]

#1292931

On 16/12/15 05:59, Yong Wu wrote:
> On Tue, 2015-12-15 at 12:37 +0000, Robin Murphy wrote:
>> On 15/12/15 03:28, Yong Wu wrote:
>>> 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).
>>
>> I still don't really understand the problem here. The M4U device is
>> created early in mtk_iommu_init_fn, so it should be probed and ready to
>> go long before anything else. Indeed, for your mtk_iommu_device_group()
>> callback to work then everything must already be happening in the right
>> order - add_device will only call iommu_group_get_for_dev() if of_xlate
>> has populated dev->archdata.iommu, and of_xlate will only do that if the
>> M4U was up and running before the client device started probing.
>> Furthermore, if mtk_iommu_device_group() *does* work, then the IOMMU
>> core will go ahead and allocate the default domain there and then, which
>> the arch code should find and use later.
>
> Thanks. This is very helpful.
>
> I understand your confuse right now and your expectant flow.
>
> Our IOMMU probe was PROBE_DEFER by our SMI device, so currently it probe
> was delayed, then have to add the workaround code.

Aha, I hadn't twigged that there was a dependency on another device that 
could delay the M4U probe, thanks for the clarification. That'll be a 
good case to bear in mind when I eventually get back to the IOMMU probe 
deferral stuff.

> Following your comment above, I test as below. Then the flows seems meet
> the "best case" that the iommu core will help create default DMA domain.
>
> @@ -664,19 +636,41 @@ static int mtk_iommu_probe(struct platform_device
> *pdev)
> for (i = 0; i < larb_nr; i++) {
> 		struct device_node *larbnode;
> 		struct platform_device *plarbdev;
>
> 		larbnode = of_parse_phandle(dev->of_node, "mediatek,larbs", i);
> 		if (!larbnode)
> 			return -EINVAL;
>
>                  plarbdev = of_find_device_by_node(larbnode);
>                  of_node_put(larbnode);
> -               if (!plarbdev)
> -                       return -EPROBE_DEFER;
> +               if (!plarbdev) {
> +                       plarbdev = of_platform_device_create(larbnode,
> NULL, platform_bus_type.dev_root);
> +                       if (IS_ERR(pdev))
> +                               return -EPROBE_DEFER;
> +               }
> }
>
> I only add of_platform_device_create for the SMI local arbiter devices
> here.
>
> This is a big improvement for us. If this is ok, I will send a quick
> next version for this.

In my opinion it's reasonable - we need the whole "IOMMU" to be ready, 
so if we already have to short-cut the creation of the M4U part it only 
seems fair to do the same for the SMI part. That said, would it work to 
just unconditionally poke the larbs in mtk_iommu_init_fn() before you 
poke the M4U itself? It would be nice to keep all that stuff together in 
the same place.

>> The potential issue I *do* see, looking more closely, is that
>> iommu_group_get_for_dev() is setting group->domain but not calling the
>> attach_dev callback, which looks wrong...
>
> This is the backtrace,
>
> (151216_09:58:05.207)Call trace:
> (151216_09:58:05.207)[<ffffffc000400668>] mtk_iommu_attach_device
> +0xb8/0x178
> (151216_09:58:05.207)[<ffffffc0003fc55c>] iommu_group_add_device
> +0x1d8/0x31c
> (151216_09:58:05.207)[<ffffffc0003fc988>] iommu_group_get_for_dev
> +0x88/0x108
> (151216_09:58:05.207)[<ffffffc0003ffcfc>] mtk_iommu_add_device+0x14/0x34
> (151216_09:58:05.207)[<ffffffc0003fb280>] add_iommu_group+0x20/0x44
> (151216_09:58:05.207)[<ffffffc000406cec>] bus_for_each_dev+0x58/0x98
> (151216_09:58:05.207)[<ffffffc0003fbe8c>] bus_set_iommu+0x9c/0xf8

Urgh, now I recall that this isn't even the first time I've been 
confused by the attach being hidden elsewhere. Oh well, problem averted!

> If I change like above, I will delete the workaround code..
>
>>
>>>
>>> //=====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.
>>
>> That already happened. The final version of the arm64 code which was
>> merged makes sure that the IOMMU driver always sees the callbacks in the
>> desired of_xlate -> add_device -> attach_dev order. The whole point of
>> the comment below is that the driver itself *doesn't* have to care about
>> the awkward way in which that is currently achieved.
>>
>>> /*
>>>    * 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.
>>>    */
>>
>> That was before Joerg made the device_group changes which enabled proper
>> default domains for platform devices - with those, we should be now be
>> hitting the "best case" behaviour every time. In fact I think the "fake
>> default domain" workaround shouldn't be needed at all any more, so I
>> will add removing it to my giant list of things to do.
>>
>>>      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;
>>>>> +}
>>
>> As long as this works as expected, then AFAICS you shouldn't have to
>> have *any* special-case behaviour or tracking of domains at all.
>
> We only need one iommu-group, one iommu domain here.
>
> What's the special-case behavior, how can we track of domains.
> Could you help give me a example?

The beauty of it is that you don't need to. If you guarantee all of an 
IOMMU's client devices are in the same group, you know you've only got 
one thing which can be attached to that IOMMU's domains. Therefore, you 
can freely allow as many domains as you like to *exist*, because there 
can never be more than one *active* at any given time - the core code 
enforces that the group is detached from one domain before being 
attached to another, and the driver's attach and detach calls just 
become responsible for switching the given domain's page table in and 
out of the actual hardware. I think it's pretty neat.

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

[toc] | [prev] | [next] | [standalone]

#1293585

On Wed, 2015-12-16 at 12:48 +0000, Robin Murphy wrote:
> On 16/12/15 05:59, Yong Wu wrote:
> > On Tue, 2015-12-15 at 12:37 +0000, Robin Murphy wrote:
> >> On 15/12/15 03:28, Yong Wu wrote:
> >>> 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:
[...]
> > Following your comment above, I test as below. Then the flows seems meet
> > the "best case" that the iommu core will help create default DMA domain.
> >
> > @@ -664,19 +636,41 @@ static int mtk_iommu_probe(struct platform_device
> > *pdev)
> > for (i = 0; i < larb_nr; i++) {
> > 		struct device_node *larbnode;
> > 		struct platform_device *plarbdev;
> >
> > 		larbnode = of_parse_phandle(dev->of_node, "mediatek,larbs", i);
> > 		if (!larbnode)
> > 			return -EINVAL;
> >
> >                  plarbdev = of_find_device_by_node(larbnode);
> >                  of_node_put(larbnode);
> > -               if (!plarbdev)
> > -                       return -EPROBE_DEFER;
> > +               if (!plarbdev) {
> > +                       plarbdev = of_platform_device_create(larbnode,
> > NULL, platform_bus_type.dev_root);
> > +                       if (IS_ERR(pdev))
> > +                               return -EPROBE_DEFER;
> > +               }
> > }
> >
> > I only add of_platform_device_create for the SMI local arbiter devices
> > here.
> >
> > This is a big improvement for us. If this is ok, I will send a quick
> > next version for this.
> 
> In my opinion it's reasonable - we need the whole "IOMMU" to be ready, 

  Thanks.

> so if we already have to short-cut the creation of the M4U part it only 
> seems fair to do the same for the SMI part. That said, would it work to 
> just unconditionally poke the larbs in mtk_iommu_init_fn() before you 
> poke the M4U itself? It would be nice to keep all that stuff together in 
> the same place.

    mtk_iommu_init_fn don't have the larb's "struct device_node". So I
cann't create its platform_device directly.

    I have tried 2 method:
   a) add a mtk_smi_larb_init_fn in the SMI patch.

static int mtk_smi_larb_init_fn(struct device_node *np)
{
	struct platform_device *pdev;

	pdev = of_platform_device_create(np, NULL, platform_bus_type.dev_root);

	return IS_ERR(pdev) ? PTR_ERR(pdev) :  0;
}
IOMMU_OF_DECLARE(mtk_smi_larb, "mediatek,mt8173-smi-larb",
mtk_smi_larb_init_fn);

   This don't work. It will run after mtk_iommu_init_fn. then the larb's
platform_device also don't exist while m4u's probe.

  b) Copy the code below to mtk_iommu_init_fn.

   for (i = 0; i < larb_nr; i++) {
      xxx
      plarbdev = of_platform_device_create(larbnode,
                      NULL, platform_bus_type.dev_root);
   }

   It works. But then there are 2 same code of parsing the SMI local
arbiter(one is in mtk_iommu_init_fn, the other is in mtk_iommu_init_fn).
It looks not good. I think that the one I wrote in the previous mail is
better, It only add 3 lines, What's your opinion?

> >>>>> +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;
> >>>>> +}
> >>
> >> As long as this works as expected, then AFAICS you shouldn't have to
> >> have *any* special-case behaviour or tracking of domains at all.
> >
> > We only need one iommu-group, one iommu domain here.
> >
> > What's the special-case behavior, how can we track of domains.
> > Could you help give me a example?
> 
> The beauty of it is that you don't need to. If you guarantee all of an 
> IOMMU's client devices are in the same group, you know you've only got 
> one thing which can be attached to that IOMMU's domains. Therefore, you 
> can freely allow as many domains as you like to *exist*, because there 
> can never be more than one *active* at any given time - the core code 
> enforces that the group is detached from one domain before being 
> attached to another, and the driver's attach and detach calls just 
> become responsible for switching the given domain's page table in and 
> out of the actual hardware. I think it's pretty neat.

It seems that mtk-iommu can not detach/attach dynamically. the iommu
core don't support iommu_detach_device/iommu_attach_device whose
iommu-group have many devices.(Normally there is only one device in a
iommu-group). 

So currently we only iommu_attach_device while probe, it will never
attach/detach again.

All our multimedia modules are in the m4u'domain and share m4u's
pagetable, They won't change pagetable.

> 
> Robin.


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

[toc] | [prev] | [next] | [standalone]

#1292999

On Tue, Dec 15, 2015 at 12:37:34PM +0000, Robin Murphy wrote:
> The potential issue I *do* see, looking more closely, is that
> iommu_group_get_for_dev() is setting group->domain but not calling
> the attach_dev callback, which looks wrong...

Attaching the device happens from iommu_group_add_device(), which is
called from iommu_group_get_for_dev() right after the domains are
allocated/initialized.


	Joerg

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web