Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Date:   Sat, 30 Sep 2023 11:08:39 -0700
From:   Chris Leech <cleech@redhat.com>
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     Christoph Hellwig <hch@lst.de>, Rasesh Mody <rmody@marvell.com>,
        Ariel Elior <aelior@marvell.com>,
        Sudarsana Kalluru <skalluru@marvell.com>,
        Manish Chopra <manishc@marvell.com>,
        Nilesh Javali <njavali@marvell.com>,
        Manish Rangankar <mrangankar@marvell.com>,
        Jerry Snitselaar <jsnitsel@redhat.com>,
        John Meneghini <jmeneghi@redhat.com>,
        Lee Duncan <lduncan@suse.com>,
        Mike Christie <michael.christie@oracle.com>,
        Hannes Reinecke <hare@kernel.org>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/3] uio: introduce UIO_DMA_COHERENT type
Message-ID: <ZRhj+GtzkCGWyylI@rhel-developer-toolbox>
References: <20230929170023.1020032-1-cleech@redhat.com>
 <20230929170023.1020032-2-cleech@redhat.com>
 <2023093037-onion-backroom-b4ef@gregkh>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <2023093037-onion-backroom-b4ef@gregkh>
Precedence: bulk

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