From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 48F7DC00144 for ; Mon, 1 Aug 2022 12:28:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234381AbiHAM2S (ORCPT ); Mon, 1 Aug 2022 08:28:18 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57650 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234464AbiHAM1y (ORCPT ); Mon, 1 Aug 2022 08:27:54 -0400 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id E82D7904D0 for ; Mon, 1 Aug 2022 05:07:18 -0700 (PDT) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 326EC139F; Mon, 1 Aug 2022 05:07:18 -0700 (PDT) Received: from [10.57.10.23] (unknown [10.57.10.23]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 6162E3F73B; Mon, 1 Aug 2022 05:07:15 -0700 (PDT) Message-ID: <19a0a508-fb39-8b06-ce0f-ce26767ef57f@arm.com> Date: Mon, 1 Aug 2022 13:07:10 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH] dma-debug: Remove warning in dma_debug_entry Content-Language: en-GB To: yf.wang@mediatek.com, Christoph Hellwig , Marek Szyprowski , Matthias Brugger , "open list:DMA MAPPING HELPERS" , open list , "moderated list:ARM/Mediatek SoC support" , "moderated list:ARM/Mediatek SoC support" Cc: wsd_upstream@mediatek.com, Libo Kang , Ning Li , Yong Wu , Miles Chen References: <20220730085657.31977-1-yf.wang@mediatek.com> From: Robin Murphy In-Reply-To: <20220730085657.31977-1-yf.wang@mediatek.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2022-07-30 09:56, yf.wang@mediatek.com wrote: > From: Yunfei Wang > > The same physical address can be mapped multiple times, and it is not > required to have attributes such as DMA_ATTR_SKIP_CPU_SYNC, but > dma_debug_entry will report an warning if active_cacheline_insert > returns -EEXIST, which can cause a lot of trouble. > > For example, if two dma engine share the same dma-buf, the two dma > engine drivers will call the DMA API to map their own dma address > respectively, which is normal follow, but dma_debug_entry will > report an warning. > > In addition, if active_cacheline_insert returns -EEXIST, the > active_cacheline_inc_overlap function already has the logic to > check overlapping mappings counter, if it exceeded > ACTIVE_CACHELINE_MAX_OVERLAP overlapping mappings, it already > has a warning. > And also, if warning as panic is turned on, it will cause KE. > > So, dma_debug_entry does not need report warning. I'm sure this has come up before, but I can't remember enough of the context to easily search for the previous thread. In short, this check is not entirely correct, but removing it isn't right either, since that would be too much of a step backwards in terms of debug coverage IMO. What we need here is to do a bit more work to differentiate between mappings of the exact same buffer, which are allowed, vs. any other kind of partial overlaps which are still illegal and definitely deserve a warning. Furthermore, even for the valid dma-buf import case I'm not convinced that the SKIP_CPU_SYNC check can be dropped either. If one device can import a buffer while another device is already accessing it, there are definitely combinations of parameters which could lead to potential data loss (e.g. a non-coherent DMA_TO_DEVICE mapping during a non-coherent DMA_FROM_DEVICE access, a or non-coherent DMA_FROM_DEVICE mapping during a coherent DMA_TO_DEVICE access). Thanks, Robin. > Signed-off-by: Yunfei Wang > --- > kernel/dma/debug.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c > index 2caafd13f8aa..ad731f7858c9 100644 > --- a/kernel/dma/debug.c > +++ b/kernel/dma/debug.c > @@ -566,9 +566,6 @@ static void add_dma_entry(struct dma_debug_entry *entry, unsigned long attrs) > if (rc == -ENOMEM) { > pr_err_once("cacheline tracking ENOMEM, dma-debug disabled\n"); > global_disable = true; > - } else if (rc == -EEXIST && !(attrs & DMA_ATTR_SKIP_CPU_SYNC)) { > - err_printk(entry->dev, entry, > - "cacheline tracking EEXIST, overlapping mappings aren't supported\n"); > } > } > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id ED94DC00144 for ; Mon, 1 Aug 2022 12:19:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:Content-Type: Content-Transfer-Encoding:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:From:References:Cc:To:Subject: MIME-Version:Date:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=BdUHyYBXx+JT6ThcKpW9ek5fgqdFbtOb6MkiMb6fm9g=; b=Fqn1JKlkqMr+8J 6trSSIslPijtsYT6cMkI1vRCYJQx3ljgTXujimrKJfPdeWIDz5yYja6cHQjsAnYutgFY5gYemeEtD C4oOK2A8ajxJVA3P2OR2i3caiZ9p6CPtz7M74VAAexUivNDN0RUbePBJPDZj8Wo4KL9+kCekIq5X4 MBW9TMohRwYAgbjRewBBFWaOP2dMam6P+HkALKQ7RV2K1y5OUaosgI0xIGDOOr37m13xqTG1GbndJ DdCQlon02I4GcA8oHsKDudYA3HEDjTKRmfA6eH3IlRTSor2nKDMC1uk89MMCrU49PBWo6XiqMjnTB SEAcWlVxLWoL5cDZ1YzQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oIUN0-0063k8-7m; Mon, 01 Aug 2022 12:18:02 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oIUCe-005tsv-AZ; Mon, 01 Aug 2022 12:07:23 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 326EC139F; Mon, 1 Aug 2022 05:07:18 -0700 (PDT) Received: from [10.57.10.23] (unknown [10.57.10.23]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 6162E3F73B; Mon, 1 Aug 2022 05:07:15 -0700 (PDT) Message-ID: <19a0a508-fb39-8b06-ce0f-ce26767ef57f@arm.com> Date: Mon, 1 Aug 2022 13:07:10 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH] dma-debug: Remove warning in dma_debug_entry Content-Language: en-GB To: yf.wang@mediatek.com, Christoph Hellwig , Marek Szyprowski , Matthias Brugger , "open list:DMA MAPPING HELPERS" , open list , "moderated list:ARM/Mediatek SoC support" , "moderated list:ARM/Mediatek SoC support" Cc: wsd_upstream@mediatek.com, Libo Kang , Ning Li , Yong Wu , Miles Chen References: <20220730085657.31977-1-yf.wang@mediatek.com> From: Robin Murphy In-Reply-To: <20220730085657.31977-1-yf.wang@mediatek.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220801_050720_522848_694C858D X-CRM114-Status: GOOD ( 22.79 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 2022-07-30 09:56, yf.wang@mediatek.com wrote: > From: Yunfei Wang > > The same physical address can be mapped multiple times, and it is not > required to have attributes such as DMA_ATTR_SKIP_CPU_SYNC, but > dma_debug_entry will report an warning if active_cacheline_insert > returns -EEXIST, which can cause a lot of trouble. > > For example, if two dma engine share the same dma-buf, the two dma > engine drivers will call the DMA API to map their own dma address > respectively, which is normal follow, but dma_debug_entry will > report an warning. > > In addition, if active_cacheline_insert returns -EEXIST, the > active_cacheline_inc_overlap function already has the logic to > check overlapping mappings counter, if it exceeded > ACTIVE_CACHELINE_MAX_OVERLAP overlapping mappings, it already > has a warning. > And also, if warning as panic is turned on, it will cause KE. > > So, dma_debug_entry does not need report warning. I'm sure this has come up before, but I can't remember enough of the context to easily search for the previous thread. In short, this check is not entirely correct, but removing it isn't right either, since that would be too much of a step backwards in terms of debug coverage IMO. What we need here is to do a bit more work to differentiate between mappings of the exact same buffer, which are allowed, vs. any other kind of partial overlaps which are still illegal and definitely deserve a warning. Furthermore, even for the valid dma-buf import case I'm not convinced that the SKIP_CPU_SYNC check can be dropped either. If one device can import a buffer while another device is already accessing it, there are definitely combinations of parameters which could lead to potential data loss (e.g. a non-coherent DMA_TO_DEVICE mapping during a non-coherent DMA_FROM_DEVICE access, a or non-coherent DMA_FROM_DEVICE mapping during a coherent DMA_TO_DEVICE access). Thanks, Robin. > Signed-off-by: Yunfei Wang > --- > kernel/dma/debug.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c > index 2caafd13f8aa..ad731f7858c9 100644 > --- a/kernel/dma/debug.c > +++ b/kernel/dma/debug.c > @@ -566,9 +566,6 @@ static void add_dma_entry(struct dma_debug_entry *entry, unsigned long attrs) > if (rc == -ENOMEM) { > pr_err_once("cacheline tracking ENOMEM, dma-debug disabled\n"); > global_disable = true; > - } else if (rc == -EEXIST && !(attrs & DMA_ATTR_SKIP_CPU_SYNC)) { > - err_printk(entry->dev, entry, > - "cacheline tracking EEXIST, overlapping mappings aren't supported\n"); > } > } > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel