Re: [PATCH 1/3] uio: introduce UIO_DMA_COHERENT type

Chris Leech
2023-09-30T18:09:40Z

On Sat, Sep 30, 2023 at 09:10:10AM +0200, Greg Kroah-Hartman wrote:
> On Fri, Sep 29, 2023 at 10:00:21AM -0700, Chris Leech wrote:
> > Add a UIO memtype specificially for sharing dma_alloc_coherent
> > memory with userspace, backed by dma_mmap_coherent.
>
> Are you sure that you can share this type of memory with userspace
> safely? And you are saying what you are doing here, but not why you
> want to do it and who will use it.
>
> What are the userspace implications for accessing this type of memory?

Thanks for taking the time to look at this Greg.
I'm trying to help Marvell fix a regression with these drivers, by
figuring out what the right way to handle this type of mmap is.

The dma_mmap_coherent API exists for exactly this, so I thought making
the uio interface aware of it made sense. There are uio drivers sharing
dma_alloc_coherent memory (uio_dmem_genirq, uio_pruss) using
UIO_MEM_PHYS, but that falls apart in the face of an iommu.

> > struct uio_mem {
> > const char *name;
> > - phys_addr_t addr;
> > + union {
> > + phys_addr_t addr;
> > + dma_addr_t dma_addr;
> > + };
> > unsigned long offs;
> > resource_size_t size;
> > int memtype;
> > - void __iomem *internal_addr;
> > + union {
> > + void __iomem *internal_addr;
> > + void *virtual_addr;
> > + };
> > + struct device *dma_device;
>
> Why are you adding a new struct device here?

dma_mmap_coherent wants it.

> And why the unions? How are you going to verify that they are being
> used correctly? What space savings are you attempting to do here and
> why?

I should have expected that would be questioned, I was being paranoid
about mixing different pointer and address types. I can remove the
unions if putting a dma_addr_t in addr going to be OK.

- Chris