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"<[email protected]>;
> Date: Thu, Sep 21, 2017 06:47 PM
> To: "Huacai Chen"<[email protected]>; "Christoph Hellwig"<[email protected]>;
> Cc: "Marek Szyprowski"<[email protected]>; "Andrew
> Morton"<[email protected]>; "Fuxin Zhang"<[email protected]>;
> "linux-kernel"<[email protected]>; "James E . J .
> Bottomley"<[email protected]>; "Martin K .
> Petersen"<[email protected]>;
> "linux-scsi"<[email protected]>; "stable"<[email protected]>;
> 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
>>
>
