[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-netdev
Subject:    Re: [PATCH 1/3] uio: introduce UIO_DMA_COHERENT type
From:       Chris Leech <cleech () redhat ! com>
Date:       2023-09-30 18:08:39
Message-ID: ZRhj+GtzkCGWyylI () rhel-developer-toolbox
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]