Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754583AbbLPF7w (ORCPT ); Wed, 16 Dec 2015 00:59:52 -0500 Received: from mailgw02.mediatek.com ([218.249.47.111]:32693 "EHLO mailgw02.mediatek.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1751398AbbLPF7t (ORCPT ); Wed, 16 Dec 2015 00:59:49 -0500 X-Listener-Flag: 11101 Message-ID: <1450245573.22854.108.camel@mhfsdcap03> Subject: Re: [PATCH v6 4/5] iommu/mediatek: Add mt8173 IOMMU driver From: Yong Wu To: Robin Murphy CC: Joerg Roedel , Thierry Reding , Mark Rutland , Matthias Brugger , Will Deacon , Daniel Kurtz , Tomasz Figa , Lucas Stach , Rob Herring , Catalin Marinas , , Sasha Hauer , , , , , , , , , , Date: Wed, 16 Dec 2015 13:59:33 +0800 In-Reply-To: <5670098E.40601@arm.com> References: <1449568153-15643-1-git-send-email-yong.wu@mediatek.com> <1449568153-15643-5-git-send-email-yong.wu@mediatek.com> <20151214141656.GG18805@8bytes.org> <1450150132.22854.57.camel@mhfsdcap03> <5670098E.40601@arm.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.2.3-0ubuntu6 Content-Transfer-Encoding: 7bit MIME-Version: 1.0 X-MTK: N Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8564 Lines: 234 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)[] mtk_iommu_attach_device +0xb8/0x178 (151216_09:58:05.207)[] iommu_group_add_device +0x1d8/0x31c (151216_09:58:05.207)[] iommu_group_get_for_dev +0x88/0x108 (151216_09:58:05.207)[] mtk_iommu_add_device+0x14/0x34 (151216_09:58:05.207)[] add_iommu_group+0x20/0x44 (151216_09:58:05.207)[] bus_for_each_dev+0x58/0x98 (151216_09:58:05.207)[] 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/