[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZRhj+GtzkCGWyylI@rhel-developer-toolbox>
Date: Sat, 30 Sep 2023 11:08:39 -0700
From: Chris Leech <cleech@...hat.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Christoph Hellwig <hch@....de>, Rasesh Mody <rmody@...vell.com>,
Ariel Elior <aelior@...vell.com>,
Sudarsana Kalluru <skalluru@...vell.com>,
Manish Chopra <manishc@...vell.com>,
Nilesh Javali <njavali@...vell.com>,
Manish Rangankar <mrangankar@...vell.com>,
Jerry Snitselaar <jsnitsel@...hat.com>,
John Meneghini <jmeneghi@...hat.com>,
Lee Duncan <lduncan@...e.com>,
Mike Christie <michael.christie@...cle.com>,
Hannes Reinecke <hare@...nel.org>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] uio: introduce UIO_DMA_COHERENT type
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
Powered by blists - more mailing lists