Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <41b495a6-d1c1-922d-ec4d-febaa01ae75c@amd.com>
Date:   Wed, 31 Aug 2022 13:07:36 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [bug report] dma-buf: Add debug option
Content-Language: en-US
To:     yf.wang@mediatek.com, Daniel Vetter <daniel.vetter@ffwll.ch>,
        Sumit Semwal <sumit.semwal@linaro.org>,
        linux-media@vger.kernel.org
Cc:     wsd_upstream@mediatek.com, Libo Kang <Libo.Kang@mediatek.com>,
        Ning Li <Ning.Li@mediatek.com>, Yong Wu <Yong.Wu@mediatek.com>,
        Miles Chen <miles.chen@mediatek.com>,
        dri-devel@lists.freedesktop.org, linaro-mm-sig@lists.linaro.org,
        linux-kernel@vger.kernel.org
References: <20220831103521.3019-1-yf.wang@mediatek.com>
From:   =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
In-Reply-To: <20220831103521.3019-1-yf.wang@mediatek.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?R2VTb0hDZlBvSHM1ZTE3L202QjBBR2g1SnR6bEVRNSsyZGVZQUJtYTFNOHFB?=
 =?utf-8?B?SUtPaUc2MTFPVHhRMm9KL2tuUXNQc0I4Mkk3d3VLdFJ1T2ZQY3NyY3Q2d0tp?=
 =?utf-8?B?MXRkdGY5OHJ3ZlNTdXZKaWtRYzdsV1ZrKy9EUDhjYmI5N3ZBb3pmajc3amts?=
 =?utf-8?B?TDBWZ3hZWWhyZmJmWHdPVTNjK09ScnBuTXRKb2hLUGE3V1hWQ1gveTlUem4v?=
 =?utf-8?B?WWI1WlhYdjNFb0U2QWZTdzQ5Qks0TWNoZUhvVHpPcjN2Tmh3OHMySnFJeER0?=
 =?utf-8?B?by92QXllRVRlKytXM1BDM1k5VW9EZEJiNFlPaTBmOHhwRzMyMVhPZlkwU3pP?=
 =?utf-8?B?eVhYTU56MVlySVc1QW5XSWxEWWx1NDllTkpSQnl1R2lodkYxUWZyUWxoa3Fr?=
 =?utf-8?B?UzJEdFhkaEVMWVNkSWRSMUI2L0svMENidWFLdzZ0UEEvNEVlZTk2bTg0MXlD?=
 =?utf-8?B?b2draHNNNFV6OVQzNHg2MDBYcVQreU9mZ2EwRCsxZmY2ZVJSNkxORDJYUW93?=
 =?utf-8?B?bitqdGhKYlFOVTI2SURQcHA3YkZEZFMwZW1GOS80LzJpL2Z0bzFqdU9MU0JR?=
 =?utf-8?B?cHdmLytQcTg1aUpldzh0UGk4bTBKM2NMVUNpOGp1NmNyRExOS21PRTZ6a0pI?=
 =?utf-8?B?TnAvQnZZenFRWmNXdnVHbzF0Mzg0MjFiWG1EWkptYnFiTk9FbnBXMGxka2lR?=
 =?utf-8?B?Vlg4eGd5ZkwrVTg3bm1IQWVTT2ZwSmZQNlMwb1VEbEZzbk9VSkZnMWVDOFZE?=
 =?utf-8?B?VzE5UnB4TFowQ0NQbjkyVUh3UGllTlE2WTN4cE1Pa1lSUEl5cG5KV25kZUxK?=
 =?utf-8?B?Z2thand0M0RMQ1dYMGtmWVdDcDZ3THVsdWpPcnYwMlFuNEE4OVp5bWZDSGFz?=
 =?utf-8?B?SGRSQlpiNGE2SmdXeGpya09Rc2xsT01LdFlYY2VkK2tkUzlQaDMwcWRPenZL?=
 =?utf-8?B?Z1RSbjlqZ0ExWmxDN3pEU2IxUXFGWDlkMXlJa1VuYnROcU9SMittbVlBckJG?=
 =?utf-8?B?T0VhMk1NQU1idU5vRGZwNFBXaW5HVC8wd0h4M0xDNFArTDBGWVliS0VwMHc5?=
 =?utf-8?B?TC9YaVUzRGV2RG9qcDJUWkptZDZERnFiTjNpRTBzdE1QU3hBcU1WVnp5UEhD?=
 =?utf-8?B?OEY5dXlUczdKR2ZIQ3VNQ2U0ZW9vSWtQSU5ZNWhDT3ZNTW5MNlMwUDl3K2I1?=
 =?utf-8?B?QXh6OFNMYzRnU29IN0c4RTNrbmNzR3VEM1M5YjltcHlvalRsYXFzZ1RYVkpp?=
 =?utf-8?B?cVRTNlFiaFRQRDlaVlppWFZ3Q2lvUFVjV3lwNUhOdU1oSnlaa2Q0SGZWM21m?=
 =?utf-8?B?aHdWdWxlbmpMMnU0ZTZ1RVlUWXVBYkNKcFpyNE51ZFoxUFM4cGRCZldVdGpn?=
 =?utf-8?B?OWxaUW5ZWkNPYTh0bnkzOVFsSW50b3I4R0R2TlpaclNJSkJnWjlNaVFKcGI5?=
 =?utf-8?B?UnFmUmpjYVVxdDVSY2xVOURtR1hvUWZyQk1DZ1BuTTh6VW5qUVFCbjcwcXh1?=
 =?utf-8?B?RWdtWkZ6bkkvdkFGcWlIWFZxMzBCblNWdDR6NTR3QkI1blpPUy8ySUg4SmJh?=
 =?utf-8?B?RVdlWFJHUlQ4a0d5dEsrdlJ1T3gzSEErYm93em5iajZra1RZUmxOQU1jU2Qy?=
 =?utf-8?B?ZXN0T09YVkhZakZQUUtyMG5tRGpoZlJwR3RiNE1JOTdwS2NiS29HMmZvaTJY?=
 =?utf-8?B?a1VFR2RCOUE4MjVYczdnSUxhWGpyUkNRRUVWZDFqOWY0RjY0M3d1cFB1blIz?=
 =?utf-8?B?dGhmdUxBMXhrdExKK3NBdGQ4cVRrbTVoQ01mUnpVMjNxcER2MTBKVlVZQnZn?=
 =?utf-8?B?aFJIWTVOa2ZzeitwSzhib0tMQTB5b2dUS3lnSjV4VUlvWkJ2TU9tWU1ZV2g2?=
 =?utf-8?B?YTM0UkdMUnNncGVmSFdFMnpFcUtYZURsQXAzWU40MUJBM05WQVJIcXIrVkJk?=
 =?utf-8?B?NnRUZHBXM2JEL1dtcjFiZWt1Uk9tMm9Sb0ZIN0xaakxGeGtuQ1lETTNYSms5?=
 =?utf-8?B?UjVnUW12MHJVMTNnVHpEMllXWXZqMGo5WHRvQ0Y0QVlZd0ZCVkxwaWY4VEJK?=
 =?utf-8?B?SE9ET2ZIbExiajRHSHg3RVFubnowUTc2amJnYUd3eGtWUHVnRUNmNFp5VGJ1?=
 =?utf-8?B?T05PMW4yOXBSUmtlK09iMXVZOFh6N1hZZXVsRXNIeGZscFJjNnlOdkowVUdh?=
 =?utf-8?Q?3ij4wUPokaP5H56SIfVnzoGLNiwgp6GSiBhSZzgNLROB?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 49a69372-424e-4779-240a-08da8b4106b9
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3587.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Aug 2022 11:07:43.0309
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: QZHMjdR3JNUP/Y6QrlBp8uK+6NaInedoQD6Fp7nB4BjjilgXwBc0om0xXEwq6IsE
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR12MB6885
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Yunfei,

well it looks like system_heap_dma_buf_begin_cpu_access() is exactly 
doing what this patch tries to prevent.

In other words the dma_heap implementation is doing something which it 
shouldn't be doing. The patch from Daniel is just surfacing this.

Regards,
Christian.

Am 31.08.22 um 12:35 schrieb yf.wang@mediatek.com:
> Hi Daniel Vetter,
>
> The patch https://patchwork.freedesktop.org/patch/414455/:
> "dma-buf: Add debug option" from Jan. 15, 2021, leads to the following expection:
>
> Backtrace:
>
> [<ffffffc0081a2258>] atomic_notifier_call_chain+0x9c/0xe8
> [<ffffffc0081a2d54>] notify_die+0x114/0x19c
> [<ffffffc0080348d8>] __die+0xec/0x468
> [<ffffffc008034648>] die+0x54/0x1f8
> [<ffffffc0080631e8>] die_kernel_fault+0x80/0xbc
> [<ffffffc0080630fc>] __do_kernel_fault+0x268/0x2d4
> [<ffffffc008062c4c>] do_bad_area+0x68/0x148
> [<ffffffc00a6dab34>] do_translation_fault+0xbc/0x108
> [<ffffffc0080619f8>] do_mem_abort+0x6c/0x1e8
> [<ffffffc00a68f5cc>] el1_abort+0x3c/0x64
> [<ffffffc00a68f54c>] el1h_64_sync_handler+0x5c/0xa0
> [<ffffffc008011ae4>] el1h_64_sync+0x78/0x80
> [<ffffffc008063b9c>] dcache_inval_poc+0x40/0x58
> [<ffffffc009236104>] iommu_dma_sync_sg_for_cpu+0x144/0x280
> [<ffffffc0082b4870>] dma_sync_sg_for_cpu+0xbc/0x110
> [<ffffffc002c7538c>] system_heap_dma_buf_begin_cpu_access+0x144/0x1e0 [system_heap]
> [<ffffffc0094154e4>] dma_buf_begin_cpu_access+0xa4/0x10c
> [<ffffffc004888df4>] isp71_allocate_working_buffer+0x3b0/0xe8c [mtk_hcp]
> [<ffffffc004884a20>] mtk_hcp_allocate_working_buffer+0xc0/0x108 [mtk_hcp]
>
> Because of CONFIG_DMABUF_DEBUG will default enable when DMA_API_DEBUG enable,
> and when not support dma coherent, since the main function of user calling
> dma_buf_begin_cpu_access and dma_buf_end_cpu_access is to do cache sync during
> dma_buf_map_attachment and dma_buf_unmap_attachment, which get PA error from
> sgtable by sg_phys(sg), this leads to the expection.
>
> 1.dma_buf_map_attachement()
>   -.> mangle_sg_table(sg)  // "sg->page_link ^= ~0xffUL" to rotate PA in this patch.
>
> 2.dma_buf_begin_cpu_access()
>   -.> system_heap_dma_buf_begin_cpu_access() in system_heap.c  // do cache sync if mapped attachment before
>      -.> iommu_dma_sync_sg_for_cpu() in dma-iommu.c
>          -.>  arch_sync_dma_for_device(sg_phys(sg), sg->length, dir) // get PA error since PA mix up
>
> 3.dma_buf_end_cpu_access() and dma_buf_begin_cpu_access are similar.
>
> 4.dma_buf_unmap_attachement()
>         -.> mangle_sg_table(sg) // "sg->page_link ^= ~0xffUL" to rotate PA
>
>
>
> drivers/dma-buf/Kconfig:
> config DMABUF_DEBUG
> 	bool "DMA-BUF debug checks"
> 	default y if DMA_API_DEBUG
>
>
> drivers/dma-buf/dma-buf.c:
> static void mangle_sg_table(struct sg_table *sg_table)
> {
> #ifdef CONFIG_DMABUF_DEBUG
> 	int i;
> 	struct scatterlist *sg;
>
> 	/* To catch abuse of the underlying struct page by importers mix
> 	 * up the bits, but take care to preserve the low SG_ bits to
> 	 * not corrupt the sgt. The mixing is undone in __unmap_dma_buf
> 	 * before passing the sgt back to the exporter. */
> 	for_each_sgtable_sg(sg_table, sg, i)
> 		sg->page_link ^= ~0xffUL;
> #endif
> }
>
>
> drivers/iommu/dma-iommu.c:
> static void iommu_dma_sync_sg_for_cpu(struct device *dev,
> 		struct scatterlist *sgl, int nelems,
> 		enum dma_data_direction dir)
> {
> 	struct scatterlist *sg;
> 	int i;
>
> 	if (dev_is_dma_coherent(dev) && !dev_is_untrusted(dev))
> 		return;
>
> 	for_each_sg(sgl, sg, nelems, i) {
> 		if (!dev_is_dma_coherent(dev))
> 			arch_sync_dma_for_cpu(sg_phys(sg), sg->length, dir);
>
> 		if (is_swiotlb_buffer(sg_phys(sg)))
> 			swiotlb_tbl_sync_single(dev, sg_phys(sg), sg->length,
> 						dir, SYNC_FOR_CPU);
> 	}
> }
>
>
> Thanks,
> Yunfei.