From patchwork Wed Jan 24 14:55:38 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Robin Murphy X-Patchwork-Id: 10182685 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id DB16060233 for ; Wed, 24 Jan 2018 14:56:13 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id CB009288F7 for ; Wed, 24 Jan 2018 14:56:13 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id BDD83288FC; Wed, 24 Jan 2018 14:56:13 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,RCVD_IN_DNSWL_MED autolearn=unavailable version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [65.50.211.133]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 48FAE288F7 for ; Wed, 24 Jan 2018 14:56:13 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender:Content-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:From: References:To:Subject:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=tghiZKClj6GqbTT8gR+K0DSSTRtUryKzNZgqgzZSYXw=; b=PBZFSQHDsqN1c7xnz+qC/bd5P 76/mTtaFOzt/UZbOU2Xe0qh7nnwtaWyeO32cCX3dpKk8OPf8Qq70qgjTqhcEWyKvGsZyltnuPf8ye u6zCZ0zzw7qCOWIqiWCfp4cl6FoAjwT19uoK+mRCEfLisupVHVq6LEjta29CK59vR9a4wZRgiML9i lylyJJX59cUe6272f7NPjQfZKKBZ+7leTDxcfwDpyyBCKVF5hqppivuuHzIU1v9zBcMueElvB75jQ g3XlkVDR0MUbDI3cmfDLQdZDaDxkRzmlsK0cRELVSqSYh4e+LYgNeD5zDq3FyyvQmPtsm/4T0XKro MPfh4cqtw==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.89 #1 (Red Hat Linux)) id 1eeMTC-0001fn-Py; Wed, 24 Jan 2018 14:56:10 +0000 Received: from foss.arm.com ([217.140.101.70]) by bombadil.infradead.org with esmtp (Exim 4.89 #1 (Red Hat Linux)) id 1eeMSy-0001UH-37; Wed, 24 Jan 2018 14:55:59 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id A4DC480D; Wed, 24 Jan 2018 06:55:42 -0800 (PST) Received: from [10.1.210.88] (e110467-lin.cambridge.arm.com [10.1.210.88]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 23FFC3F41F; Wed, 24 Jan 2018 06:55:39 -0800 (PST) Subject: Re: [PATCH v2] iommu/mediatek: Move attach_device after iommu-group is ready for M4Uv1 To: Yong Wu , Joerg Roedel , Matthias Brugger References: <1516696788-8668-1-git-send-email-yong.wu@mediatek.com> From: Robin Murphy Message-ID: <78721402-989d-d06a-18cc-dd6efd358dfd@arm.com> Date: Wed, 24 Jan 2018 14:55:38 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: <1516696788-8668-1-git-send-email-yong.wu@mediatek.com> Content-Language: en-GB X-BeenThere: linux-mediatek@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: youlin.pei@mediatek.com, arnd@arndb.de, srv_heupstream@mediatek.com, Will Deacon , linux-kernel@vger.kernel.org, iommu@lists.linux-foundation.org, Tomasz Figa , Alex Williamson , linux-mediatek@lists.infradead.org, Bibby Hsieh , honghui.zhang@mediatek.com, linux-arm-kernel@lists.infradead.org Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+patchwork-linux-mediatek=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP On 23/01/18 08:39, Yong Wu wrote: > In the commit 05f80300dc8b ("iommu: Finish making iommu_group support > mandatory"), the iommu framework has supposed all the iommu drivers have > their owner iommu-group, it get rid of the FIXME workarounds while the > group is NULL. But the flow of Mediatek M4U gen1 looks a bit trick that > it will hang at this case: > > ========================================== > Unable to handle kernel NULL pointer dereference at virtual address 00000030 > PC is at mutex_lock+0x28/0x54 > LR is at iommu_attach_device+0xa4/0xd4 > pc : [] lr : [] psr: 60000013 > sp : df0edbb8 ip : df0edbc8 fp : df0edbc4 > r10: c114da14 r9 : df2a3e40 r8 : 00000003 > r7 : df27a210 r6 : df2a90c4 r5 : 00000030 r4 : 00000000 > r3 : df0f8000 r2 : fffff000 r1 : df29c610 r0 : 00000030 > Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none > xxx > (mutex_lock) from [] (iommu_attach_device+0xa4/0xd4) > (iommu_attach_device) from [] (__arm_iommu_attach_device+0x28/0x90) > (__arm_iommu_attach_device) from [] (arm_iommu_attach_device+0x1c/0x30) > (arm_iommu_attach_device) from [] (mtk_iommu_add_device+0xfc/0x214) > (mtk_iommu_add_device) from [] (add_iommu_group+0x3c/0x68) > (add_iommu_group) from [] (bus_for_each_dev+0x78/0xac) > (bus_for_each_dev) from [] (bus_set_iommu+0xb0/0xec) > (bus_set_iommu) from [] (mtk_iommu_probe+0x328/0x368) > (mtk_iommu_probe) from [] (platform_drv_probe+0x5c/0xc0) > (platform_drv_probe) from [] (driver_probe_device+0x2f4/0x4d8) > (driver_probe_device) from [] (__driver_attach+0x10c/0x128) > (__driver_attach) from [] (bus_for_each_dev+0x78/0xac) > (bus_for_each_dev) from [] (driver_attach+0x2c/0x30) > (driver_attach) from [] (bus_add_driver+0x1e0/0x278) > (bus_add_driver) from [] (driver_register+0x88/0x108) > (driver_register) from [] (__platform_driver_register+0x50/0x58) > (__platform_driver_register) from [] (m4u_init+0x24/0x28) > (m4u_init) from [] (do_one_initcall+0xf0/0x17c) > ========================= > > The root cause is that "arm_iommu_attach_device" is called before > "iommu_group_get_for_dev" in the interface "mtk_iommu_add_device". Thus, > We adjust the sequence of this two functions. > > Unfortunately, there is another issue after the solution above, From the > function "iommu_attach_device", Only one device in each a iommu group is > allowed. In Mediatek case, there is only one m4u group, all the devices > are in one group. thus it get fail at this step. > > In order to satisfy this requirement, a new iommu group is allocated for > each a iommu consumer device. But meanwhile, we still have to use the > same domain for all the iommu group. Use a global variable "mtk_domain_v1" > to save the global domain. Argh, sorry for breaking it! Seems I managed to forget just how horrible and fiddly all the arm_iommu_* stuff is :( > CC: Robin Murphy > CC: Honghui Zhang > Fixes: 05f80300dc8b ("iommu: Finish making iommu_group support mandatory") > Reported-by: Ryder Lee > Tested-by: Bibby Hsieh > Signed-off-by: Yong Wu > --- > changes since v1: > Add mtk_domain_v1=NULL in domain_free for symmetry. > > v1: https://patchwork.kernel.org/patch/10176255/ > --- > drivers/iommu/mtk_iommu_v1.c | 42 +++++++++++++++++++----------------------- > 1 file changed, 19 insertions(+), 23 deletions(-) > > diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c > index 542930c..86106bf 100644 > --- a/drivers/iommu/mtk_iommu_v1.c > +++ b/drivers/iommu/mtk_iommu_v1.c > @@ -103,6 +103,9 @@ struct mtk_iommu_domain { > struct mtk_iommu_data *data; > }; > > +/* There is only a iommu domain in M4U gen1. */ > +static struct mtk_iommu_domain *mtk_domain_v1; > + > static struct mtk_iommu_domain *to_mtk_domain(struct iommu_domain *dom) > { > return container_of(dom, struct mtk_iommu_domain, domain); > @@ -251,10 +254,15 @@ static struct iommu_domain *mtk_iommu_domain_alloc(unsigned type) > if (type != IOMMU_DOMAIN_UNMANAGED) > return NULL; > > + /* Always return the same domain. */ > + if (mtk_domain_v1) > + return &mtk_domain_v1->domain; This seems a bit too fragile (and I vaguely recall we may have discussed and rejected this approach for the original driver), since any code doing: unused = iommu_domain_alloc(bus); iommu_domain_free(unused); will pull the rug out from under everyone's feet in a very nasty and unexpected manner. Given that mtk_iommu_create_mapping() is already a giant workaround for the ARM DMA code not understanding groups and default domains, I'd prefer not to have to regress "correct" driver behaviour for the sake of that; how about something like the below diff, is that enough to make things work? Robin. ----->8----- diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c index 542930cd183d..8b90b7a72238 100644 --- a/drivers/iommu/mtk_iommu_v1.c +++ b/drivers/iommu/mtk_iommu_v1.c @@ -376,6 +376,7 @@ static int mtk_iommu_create_mapping(struct device *dev, struct platform_device *m4updev; struct dma_iommu_mapping *mtk_mapping; struct device *m4udev; + struct iommu_group *group; int ret; if (args->args_count != 1) { @@ -418,6 +419,22 @@ static int mtk_iommu_create_mapping(struct device *dev, m4udev->archdata.iommu = mtk_mapping; } + /* + * This is a short-term bodge because the ARM DMA code doesn't + * understand multi-device groups, but we have to call into it + * successfully (and not just rely on a normal IOMMU API attach + * here) in order to set the correct DMA API ops on @dev. + */ + group = iommu_group_alloc(); + if (IS_ERR(group)) { + ret = ERR_PTR(group); + goto err_release_mapping; + } + ret = iommu_group_add_device(group, dev); + iommu_group_put(group); + if (ret) + goto err_release_mapping; + ret = arm_iommu_attach_device(dev, mtk_mapping); if (ret) goto err_release_mapping;