Re: [PATCH v2 1/3] uio: introduce UIO_MEM_DMA_COHERENT type

[Chris Leech <cleech@redhat.com>]

On Wed, Jan 03, 2024 at 02:41:35PM +0530, Nilesh Javali wrote:
> From: Chris Leech <cleech@redhat.com>
> 
> Add a UIO memtype specifically for sharing dma_alloc_coherent
> memory with userspace, backed by dma_mmap_coherent.
> 
> Signed-off-by: Nilesh Javali <njavali@marvell.com>
> Signed-off-by: Chris Leech <cleech@redhat.com>
> ---
> v2:
> - expose only the dma_addr within uio_mem
> - Cleanup newly added unions comprising virtual_addr
>   and struct device

Nilesh, thanks for taking another look at these changes. If we're taking
another shot at getting uio changes accepted, Greg KH is going to have
to be part of that conversation.

Removing the unions looks good, but I do have some concerns with your
modifications.

On Wed, Jan 03, 2024 at 02:41:35PM +0530, Nilesh Javali wrote:

> +static int uio_mmap_dma_coherent(struct vm_area_struct *vma)
> +{
> +	struct uio_device *idev = vma->vm_private_data;
> +	struct uio_mem *mem;
> +	int ret = 0;
> +	int mi;
> +
> +	mi = uio_find_mem_index(vma);
> +	if (mi < 0)
> +		return -EINVAL;
> +
> +	mem = idev->info->mem + mi;
> +
> +	if (mem->dma_addr & ~PAGE_MASK)
> +		return -ENODEV;
> +	if (vma->vm_end - vma->vm_start > mem->size)
> +		return -EINVAL;
> +
> +	/*
> +	 * UIO uses offset to index into the maps for a device.
> +	 * We need to clear vm_pgoff for dma_mmap_coherent.
> +	 */
> +	vma->vm_pgoff = 0;
> +
> +	ret = dma_mmap_coherent(&idev->dev,
                                ~~~~~~~~~~
This doesn't seem right. You've plugged a struct device into the call,
but it's not the same device used when calling dma_alloc_coherent.  I
don't see a way around needing a way to access the correct device in
uio_mmap_dma_coherent, or a way to do that without attaching it to the
uio_device.

> +				vma,
> +				(void *)mem->addr,
> +				mem->dma_addr,
> +				vma->vm_end - vma->vm_start);
> +	vma->vm_pgoff = mi;
> +
> +	return ret;
> +}
> +

- Chris