From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id C382F17EF for ; Sat, 30 Sep 2023 07:10:13 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id A7BB4C433C8; Sat, 30 Sep 2023 07:10:12 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1696057813; bh=uvGD1csA5JWxarCHp9AYsOjN6Mf9w1sy7vN59u16vz0=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=UO+RYsCedPI73JzxykVPefAXh/+ASlxwiOA9qJ27HKndAAIOP1hfuMNm7lXMXrSNO X0+VdhabYsih7Wq6zJOmoRsag9sO+DkhnAP5Oz9S2Ricf6PBQBWN32oRUCVGWAgzGi didA5lSJoGzH8SKrt/U2Xmgrd/2+VPxi9GkcPFvA= Date: Sat, 30 Sep 2023 09:10:10 +0200 From: Greg Kroah-Hartman To: Chris Leech Cc: Christoph Hellwig , Rasesh Mody , Ariel Elior , Sudarsana Kalluru , Manish Chopra , Nilesh Javali , Manish Rangankar , Jerry Snitselaar , John Meneghini , Lee Duncan , Mike Christie , Hannes Reinecke , netdev@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/3] uio: introduce UIO_DMA_COHERENT type Message-ID: <2023093037-onion-backroom-b4ef@gregkh> References: <20230929170023.1020032-1-cleech@redhat.com> <20230929170023.1020032-2-cleech@redhat.com> Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230929170023.1020032-2-cleech@redhat.com> 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? > > Signed-off-by: Chris Leech > --- > drivers/uio/uio.c | 34 ++++++++++++++++++++++++++++++++++ > include/linux/uio_driver.h | 12 ++++++++++-- > 2 files changed, 44 insertions(+), 2 deletions(-) > > diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c > index 62082d64ece0..f8f1f7ba6378 100644 > --- a/drivers/uio/uio.c > +++ b/drivers/uio/uio.c > @@ -24,6 +24,7 @@ > #include > #include > #include > +#include > > #define UIO_MAX_DEVICES (1U << MINORBITS) > > @@ -759,6 +760,36 @@ static int uio_mmap_physical(struct vm_area_struct *vma) > vma->vm_page_prot); > } > > +static int uio_mmap_dma_coherent(struct vm_area_struct *vma) > +{ > + struct uio_device *idev = vma->vm_private_data; > + int mi = uio_find_mem_index(vma); > + struct uio_mem *mem; > + int rc; > + > + 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; > + rc = dma_mmap_coherent(mem->dma_device, > + vma, > + mem->virtual_addr, > + mem->dma_addr, > + vma->vm_end - vma->vm_start); > + vma->vm_pgoff = mi; > + return rc; > +} > + > static int uio_mmap(struct file *filep, struct vm_area_struct *vma) > { > struct uio_listener *listener = filep->private_data; > @@ -806,6 +837,9 @@ static int uio_mmap(struct file *filep, struct vm_area_struct *vma) > case UIO_MEM_VIRTUAL: > ret = uio_mmap_logical(vma); > break; > + case UIO_MEM_DMA_COHERENT: > + ret = uio_mmap_dma_coherent(vma); > + break; > default: > ret = -EINVAL; > } > diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h > index 47c5962b876b..ede58e984658 100644 > --- a/include/linux/uio_driver.h > +++ b/include/linux/uio_driver.h > @@ -36,11 +36,18 @@ struct uio_map; > */ > 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? 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? thanks, greg k-h