lkml.org 
[lkml]   [2017]   [Sep]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
SubjectRe: [PATCH V6 1/3] dma-mapping: Introduce device_is_coherent() as ahelper
Date
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?

Huacai

------------------ Original ------------------
From: "Robin Murphy"<robin.murphy@arm.com>;
Date: Thu, Sep 21, 2017 06:47 PM
To: "Huacai Chen"<chenhc@lemote.com>; "Christoph Hellwig"<hch@lst.de>;
Cc: "Marek Szyprowski"<m.szyprowski@samsung.com>; "Andrew Morton"<akpm@linux-foundation.org>; "Fuxin Zhang"<zhangfx@lemote.com>; "linux-kernel"<linux-kernel@vger.kernel.org>; "James E . J . Bottomley"<jejb@linux.vnet.ibm.com>; "Martin K . Petersen"<martin.petersen@oracle.com>; "linux-scsi"<linux-scsi@vger.kernel.org>; "stable"<stable@vger.kernel.org>;
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
>
\
 
 \ /
  Last update: 2017-09-22 04:14    [W:1.211 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site