From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752378AbdIVNo5 (ORCPT ); Fri, 22 Sep 2017 09:44:57 -0400 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:57628 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752253AbdIVNoz (ORCPT ); Fri, 22 Sep 2017 09:44:55 -0400 Subject: Re: [PATCH V6 1/3] dma-mapping: Introduce device_is_coherent() as ahelper To: =?UTF-8?B?6ZmI5Y2O5omN?= , Christoph Hellwig Cc: Marek Szyprowski , Andrew Morton , Fuxin Zhang , linux-kernel , "James E . J . Bottomley" , "Martin K . Petersen" , linux-scsi , stable References: <1505811161-25246-1-git-send-email-chenhc@lemote.com> <5e792b21-1d63-9200-b36a-c423385e2a2e@arm.com> From: Robin Murphy Message-ID: <9213ab49-b6e6-d513-6d35-1fe2294aad68@arm.com> Date: Fri, 22 Sep 2017 14:44:51 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 22/09/17 03:13, 陈华才 wrote: > Hi, Robin, > > Before 2.6.36 dma_get_cache_alignment is arch-dependent, and it is unified in commit 4565f0170dfc849b3629c27d7 ("dma-mapping: unify dma_get_cache_alignment implementations"). Should we revert to the old implementation? Not quite - I mean instead of adding an ops->device_is_coherent callback (which cannot really have a safe fallback value either way) and trying to enforce that dma_get_cache_alignment() should be the only valid caller, just add an ops->get_cache_alignment callback directly. Robin. > > Huacai > > ------------------ Original ------------------ > From: "Robin Murphy"; > Date: Thu, Sep 21, 2017 06:47 PM > To: "Huacai Chen"; "Christoph Hellwig"; > Cc: "Marek Szyprowski"; "Andrew Morton"; "Fuxin Zhang"; "linux-kernel"; "James E . J . Bottomley"; "Martin K . Petersen"; "linux-scsi"; "stable"; > Subject: Re: [PATCH V6 1/3] dma-mapping: Introduce device_is_coherent() as ahelper > > > On 19/09/17 09:52, Huacai Chen wrote: >> We will use device_is_coherent() as a helper function, which will be >> used in the next patch. >> >> There is a MIPS-specific plat_device_is_coherent(), but we need a more >> generic solution, so add and use a new function pointer in dma_map_ops. > > I think we're heading in the right direction with the series, but I > still don't like this patch. I can pretty much guarantee that driver > authors *will* abuse a generic device_is_coherent() API to mean "I can > skip other DMA API calls and just use virt_to_phys()". > > I think it would be far better to allow architectures to provide their > own override of dma_get_cache_alignment(), and let the coherency detail > remain internal to the relevant arch implementations. > > [...] >> @@ -697,6 +698,15 @@ static inline void *dma_zalloc_coherent(struct device *dev, size_t size, >> } >> >> #ifdef CONFIG_HAS_DMA >> +static inline int device_is_coherent(struct device *dev) >> +{ >> + const struct dma_map_ops *ops = get_dma_ops(dev); >> + if (ops && ops->device_is_coherent) >> + return ops->device_is_coherent(dev); >> + else >> + return 1; /* compatible behavior */ > > That is also quite scary - if someone now adds a new > dma_get_cache_alignemnt() call and dutifully passes a non-NULL device, > they will now get back an alignment of 1 on all non-coherent platforms > except MIPS: hello data corruption. > > Robin. > >> +} >> + >> static inline int dma_get_cache_alignment(void) >> { >> #ifdef ARCH_DMA_MINALIGN >> >