Re: [PATCH 3/7] drivers: dma-coherent: Account dma_pfn_offset when used with device tree

[Robin Murphy]

On 15/02/17 09:59, Vladimir Murzin wrote:
> dma_declare_coherent_memory() and friends are designed to account
> difference in CPU and device addresses. However, when it is used with
> reserved memory regions there is assumption that CPU and device have
> the same view on address space. This assumption gets invalid when
> reserved memory for coherent DMA allocations is referenced by device
> with non-empty "dma-range" property.
> 
> Simply feeding device address as rmem->base + dev->dma_pfn_offset
> would not work due to reserved memory region can be shared, so this
> patch turns device address to be expressed with help of CPU address
> and device's dma_pfn_offset.
> 
> For the case where device tree is not used and device sees memory
> different to CPU we explicitly set device's dma_pfn_offset to
> accomplish such difference. The latter might look controversial, but
> it seems only a few drivers set device address different to CPU's:
> - drivers/usb/host/ohci-sm501.c
> - arch/sh/drivers/pci/fixups-dreamcast.c
> so they can be screwed only if dma_pfn_offset there is set and not in
> sync with device address range - we try to catch such cases with
> WARN_ON.
> 
> Cc: Michal Nazarewicz <mina86@xxxxxxxxxx>
> Cc: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> Cc: Yoshinori Sato <ysato@xxxxxxxxxxxxxxxxxxxx>
> Cc: Rich Felker <dalias@xxxxxxxx>
> Cc: Roger Quadros <rogerq@xxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Tested-by: Benjamin Gaignard <benjamin.gaignard@xxxxxxxxxx>
> Tested-by: Andras Szemzo <sza@xxxxxx>
> Tested-by: Alexandre TORGUE <alexandre.torgue@xxxxxx>
> Signed-off-by: Vladimir Murzin <vladimir.murzin@xxxxxxx>
> ---
>  drivers/base/dma-coherent.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/dma-coherent.c b/drivers/base/dma-coherent.c
> index 640a7e6..c59708c 100644
> --- a/drivers/base/dma-coherent.c
> +++ b/drivers/base/dma-coherent.c
> @@ -18,6 +18,12 @@ struct dma_coherent_mem {
>  	spinlock_t	spinlock;
>  };
>  
> +static inline dma_addr_t dma_get_device_base(struct device *dev,
> +					     struct dma_coherent_mem * mem)
> +{
> +	return (mem->pfn_base - dev->dma_pfn_offset) << PAGE_SHIFT;
> +}
> +
>  static bool dma_init_coherent_memory(
>  	phys_addr_t phys_addr, dma_addr_t device_addr, size_t size, int flags,
>  	struct dma_coherent_mem **mem)
> @@ -83,9 +89,16 @@ static void dma_release_coherent_memory(struct dma_coherent_mem *mem)
>  static int dma_assign_coherent_memory(struct device *dev,
>  				      struct dma_coherent_mem *mem)
>  {
> +	unsigned long dma_pfn_offset = mem->pfn_base - PFN_DOWN(mem->device_base);
> +
>  	if (dev->dma_mem)
>  		return -EBUSY;
>  
> +	if (dev->dma_pfn_offset)
> +		WARN_ON(dma_pfn_offset && (dev->dma_pfn_offset != dma_pfn_offset));
> +	else
> +		dev->dma_pfn_offset = dma_pfn_offset;

This makes me rather uneasy - I can well imagine a device sharing the
CPU physical address map of external system memory, but having its own
view of its local coherent memory such that pfn_base != device_base
still. I know for a fact we've had internal FPGA tiles set up that way,
although whether it was entirely intentional is another matter... ;)

In that situation, setting dev->dma_pfn_offset like this would break
streaming DMA for such devices. Could we not keep the pool-specific
offset and the device-specific offset independent, apply whichever is
non-zero, and scream if both are set?

Robin.

> +
>  	dev->dma_mem = mem;
>  	/* FIXME: this routine just ignores DMA_MEMORY_INCLUDES_CHILDREN */
>  
> @@ -133,7 +146,7 @@ void *dma_mark_declared_memory_occupied(struct device *dev,
>  		return ERR_PTR(-EINVAL);
>  
>  	spin_lock_irqsave(&mem->spinlock, flags);
> -	pos = (device_addr - mem->device_base) >> PAGE_SHIFT;
> +	pos = PFN_DOWN(device_addr - dma_get_device_base(dev, mem));
>  	err = bitmap_allocate_region(mem->bitmap, pos, get_order(size));
>  	spin_unlock_irqrestore(&mem->spinlock, flags);
>  
> @@ -186,7 +199,7 @@ int dma_alloc_from_coherent(struct device *dev, ssize_t size,
>  	/*
>  	 * Memory was found in the per-device area.
>  	 */
> -	*dma_handle = mem->device_base + (pageno << PAGE_SHIFT);
> +	*dma_handle = dma_get_device_base(dev, mem) + (pageno << PAGE_SHIFT);
>  	*ret = mem->virt_base + (pageno << PAGE_SHIFT);
>  	dma_memory_map = (mem->flags & DMA_MEMORY_MAP);
>  	spin_unlock_irqrestore(&mem->spinlock, flags);
>