From patchwork Wed May 17 05:15:30 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sricharan Ramabadhran X-Patchwork-Id: 9730051 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 BE2B060363 for ; Wed, 17 May 2017 05:16:08 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id AEE9C28700 for ; Wed, 17 May 2017 05:16:08 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id A17D528713; Wed, 17 May 2017 05:16:08 +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=-1.9 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID 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 000C528700 for ; Wed, 17 May 2017 05:16:07 +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-Transfer-Encoding:Content-Type: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=v7htq4cNPXbcQGUu1TzsL9E4I0ZzGvyWYSoCP6KOHSg=; b=Yp4Ey+wmSImnrK vqAYsBpf9vkBT7VOhMkzoNFAP0T2/mu7KtMcePqd/tIre1WjvLnkWZDd10oOV6OSfz7Hv7INLCJ6Q xLLC1lvaVyJDlqkFGy+KjDXaw9zVH92S6KxATR6KdfgVBZt/e69Kb3jlKssBSkPFvaTv7wD7TGQo/ 5CAHdf4gvWgGsOVPCUClCeLPD7vppvntFdNF3JiqJmE3lkxPMQCGuWBORzl0xhSRb8HtwbpAopvPL C8nK7UyUCruA0HNdT9ivcZBU97G9jpn9vXNESXl9BbalfxxyVK/z1D9wena6XJmLF97uok9SW3MzV BMCTLovqTdk4IeE0PjeQ==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1dArJc-0001i7-Fn; Wed, 17 May 2017 05:16:04 +0000 Received: from smtp.codeaurora.org ([198.145.29.96]) by bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux)) id 1dArJY-0001gp-SZ for linux-arm-kernel@lists.infradead.org; Wed, 17 May 2017 05:16:02 +0000 Received: by smtp.codeaurora.org (Postfix, from userid 1000) id D16866076C; Wed, 17 May 2017 05:15:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1494998139; bh=REtsggjJiOlRDXqfqzAAc+gJhFE2A4hYuLDomkJdAJc=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=koy2ZMzu9AyXVnX6gDFiuDZhijH6XqacsEo4EBEQRsMMRQSAME4Bja/PrkYXcNdxd AJKS7Khqk1vCuULWxybXXP7zLKuXPNrVDlXvXk3rjQKN0uo31C+xTksiuOrB4Y4woa e6Wh1IxTtskZDNiKJRfymg4IXLOCSExXJY85dnLo= Received: from [192.168.1.4] (unknown [122.164.157.29]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: sricharan@smtp.codeaurora.org) by smtp.codeaurora.org (Postfix) with ESMTPSA id 563006076C; Wed, 17 May 2017 05:15:34 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1494998138; bh=REtsggjJiOlRDXqfqzAAc+gJhFE2A4hYuLDomkJdAJc=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=P1dqaKbRlZQ3styMNfNZfPzNVzzxcl8O9Dk4vaMqzmdJQWt4qgNN4XiA0J66PGGUj bLhdCqyhy9EOWZ+H5mXMRZlKOMaQfRs8v41gdcnezpacPg1VPcyPMkrgTd6jBlHHT0 b/hdHK5IGKwi9CouR5IVJTdfMBNPyXp+aicHshIg= DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 563006076C Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=sricharan@codeaurora.org Subject: Re: [PATCH] ARM: dma-mapping: Don't tear third-party mappings To: Laurent Pinchart , Robin Murphy References: <6d9fa319-4401-c47a-7985-9a349838bcaf@arm.com> <20170516151434.18830-1-laurent.pinchart+renesas@ideasonboard.com> <2602abb1-e777-700c-bc95-14e576372537@arm.com> <4043121.T8QnJes2jP@avalon> From: Sricharan R Message-ID: Date: Wed, 17 May 2017 10:45:30 +0530 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: <4043121.T8QnJes2jP@avalon> Content-Language: en-US X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20170516_221600_970771_FC82C10D X-CRM114-Status: GOOD ( 33.18 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Joerg Roedel , Will Deacon , linux-renesas-soc@vger.kernel.org, iommu@lists.linux-foundation.org, Laurent Pinchart , Geert Uytterhoeven , linux-arm-kernel@lists.infradead.org Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP Hi Laurent/Robin, On 5/16/2017 10:14 PM, Laurent Pinchart wrote: > Hi Robin, > > On Tuesday 16 May 2017 16:47:36 Robin Murphy wrote: >> On 16/05/17 16:14, Laurent Pinchart wrote: >>> arch_setup_dma_ops() is used in device probe code paths to create an >>> IOMMU mapping and attach it to the device. The function assumes that the >>> device is attached to a device-specific IOMMU instance (or at least a >>> device-specific TLB in a shared IOMMU instance) and thus creates a >>> separate mapping for every device. >>> >>> On several systems (Renesas R-Car Gen2 being one of them), that >>> assumption is not true, and IOMMU mappings must be shared between >>> multiple devices. In those cases the IOMMU driver knows better than the >>> generic ARM dma-mapping layer and attaches mapping to devices manually >>> with arm_iommu_attach_device(), which sets the DMA ops for the device. >>> >>> The arch_setup_dma_ops() function takes this into account and bails out >>> immediately if the device already has DMA ops assigned. However, the >>> corresponding arch_teardown_dma_ops() function, called from driver >>> unbind code paths (including probe deferral), will tear the mapping down >>> regardless of who created it. When the device is reprobed >>> arch_setup_dma_ops() will be called again but won't perform any >>> operation as the DMA ops will still be set. >>> >>> We need to reset the DMA ops in arch_teardown_dma_ops() to fix this. >>> However, we can't do so unconditionally, as then a new mapping would be >>> created by arch_setup_dma_ops() when the device is reprobed, regardless >>> of whether the device needs to share a mapping or not. We must thus keep >>> track of whether arch_setup_dma_ops() created the mapping, and only in >>> that case tear it down in arch_teardown_dma_ops(). >>> >>> Keep track of that information in the dev_archdata structure. As the >>> structure is embedded in all instances of struct device let's not grow >>> it, but turn the existing dma_coherent bool field into a bitfield that >>> can be used for other purposes. >>> >>> Fixes: 7b07cbefb68d ("iommu: of: Handle IOMMU lookup failure with deferred >>> probing or error") Signed-off-by: Laurent Pinchart >>> --- >>> >>> arch/arm/include/asm/device.h | 3 ++- >>> arch/arm/mm/dma-mapping.c | 5 +++++ >>> 2 files changed, 7 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/arm/include/asm/device.h b/arch/arm/include/asm/device.h >>> index 36ec9c8f6e16..3234fe9bba6e 100644 >>> --- a/arch/arm/include/asm/device.h >>> +++ b/arch/arm/include/asm/device.h >>> @@ -19,7 +19,8 @@ struct dev_archdata { >>> #ifdef CONFIG_XEN >>> const struct dma_map_ops *dev_dma_ops; >>> #endif >>> - bool dma_coherent; >>> + unsigned int dma_coherent:1; >> >> This should only ever be accessed by the Xen DMA code via the >> is_device_dma_coherent() helper, so I can't see the change of storage >> type causing any problems. > > Thank you for double-checking. I agree with your analysis. > >>> + unsigned int dma_ops_setup:1; >>> }; >>> >>> struct omap_device; >>> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c >>> index c742dfd2967b..e0272f9140e2 100644 >>> --- a/arch/arm/mm/dma-mapping.c >>> +++ b/arch/arm/mm/dma-mapping.c >>> @@ -2430,9 +2430,14 @@ void arch_setup_dma_ops(struct device *dev, u64 >>> dma_base, u64 size, >>> dev->dma_ops = xen_dma_ops; >>> } >>> #endif >>> + dev->archdata.dma_ops_setup = true; >>> } >>> >>> void arch_teardown_dma_ops(struct device *dev) >>> { >>> + if (!dev->archdata.dma_ops_setup) >>> + return; >>> + >>> arm_teardown_iommu_dma_ops(dev); >>> + set_dma_ops(dev, NULL); >> >> Should we clear dma_ops_setup here for symmetry? I guess in practice >> it's down to the IOMMU driver so will never change after the first >> probe, but it still feels like a bit of a nagging loose end. > > To make a difference, we would need an IOMMU driver that creates a mapping > after a first round of arch_setup_dma_ops() / arch_teardown_dma_ops() calls, > follow by a second round. I don't think this could happen, but if it did, I > believe we'd be screwed already, as there would be a time were an incorrect > mapping (created by arch_setup_dma_ops() while the IOMMU driver needs to take > care of mapping creation) exists. > Feels correct not to reset this, the iommu drivers in question, seems to creating mapping/attaching in add_device path (which gets called before the clients gets probed) and when the iommu client gets deferred/reprobed that does not happen again even after the first round. >> With that (or firm reassurance that it's OK not to), >> >> Reviewed-by: Robin Murphy >> >> Apologies for being too arm64-focused in the earlier reviews and >> overlooking this. Should the patch supersede 8674/1 currently in >> Russell's incoming box? > > Yes I think it should. Could you please take care of that ? > > You can also add my > was > Tested-by: Laurent Pinchart > > as I've tested that this paptch restores proper IOMMU operation on the Renesas > R-Car H2 Lager board. I believe the problem related to Sricharan's patch > reported by Geert still affects us and needs to be addressed separately. Thanks for the above, i had the same thing to be posted, was just testing it once. There are three patches [1][2], already posted and third one for the issue that Geert pointed i did below (Geert had a patch little differently to ignore -ENODEV). I had this question previously for not propagating errors apart from EPROBE_DEFER, did not have an issue reported at that time. Anyways if the below is ok, i will just send the 3 patches in one set for easy picking up ? [1] https://lkml.org/lkml/2017/5/16/25 [2] The above one that you have. [3] The below one, if its fine ? From 4b379d4b852c41d7b5904c9a9e53deda94039f0a Mon Sep 17 00:00:00 2001 From: Sricharan R Date: Wed, 3 May 2017 14:54:11 +0530 Subject: [PATCH] of: iommu: Ignore all errors except EPROBE_DEFER While deferring the probe of iommu masters, xlate and add_device callback can passback error values like -ENODEV, which means iommu cannot be connected with that master for real reasons. So rather than killing the master's probe for such errors, just ignore the errors and let the master work without an iommu. Signed-off-by: Sricharan R --- drivers/iommu/of_iommu.c | 4 ++++ 1 file changed, 4 insertions(+) -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c index e6e9bec..750ab07 100644 --- a/drivers/iommu/of_iommu.c +++ b/drivers/iommu/of_iommu.c @@ -237,6 +237,10 @@ const struct iommu_ops *of_iommu_configure(struct device *dev, ops = ERR_PTR(err); } + /* Ignore all other errors apart from EPROBE_DEFER */ + if (IS_ERR(ops) && (PTR_ERR(ops) != -EPROBE_DEFER)) + ops = NULL; + return ops; }