| Message ID | 20240131191732.3247996-2-cleech@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | UIO_MEM_DMA_COHERENT for cnic/bnx2/bnx2x | expand |
On Wed, Jan 31, 2024 at 11:17:31AM -0800, Chris Leech wrote: > Add a UIO memtype specifically for sharing dma_alloc_coherent > memory with userspace, backed by dma_mmap_coherent. > > This is mainly for the bnx2/bnx2x/bnx2i "cnic" interface, although there > are a few other uio drivers which map dma_alloc_coherent memory and > could be converted to use dma_mmap_coherent as well. What other drivers could use this? Patches doing the conversion would be welcome, otherwise, again, I am very loath to take this one-off-change for just a single driver that shouldn't be doing this in the first place :) thanks, greg k-h
On Wed, Jan 31, 2024 at 1:29 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Wed, Jan 31, 2024 at 11:17:31AM -0800, Chris Leech wrote: > > Add a UIO memtype specifically for sharing dma_alloc_coherent > > memory with userspace, backed by dma_mmap_coherent. > > > > This is mainly for the bnx2/bnx2x/bnx2i "cnic" interface, although there > > are a few other uio drivers which map dma_alloc_coherent memory and > > could be converted to use dma_mmap_coherent as well. > > What other drivers could use this? Patches doing the conversion would > be welcome, otherwise, again, I am very loath to take this > one-off-change for just a single driver that shouldn't be doing this in > the first place :) uio_pruss and uio_dmem_genirq both appear to mmap dma_alloc_coherent memory as UIO_MEM_PHYS. It might not be an issue on that platforms where those are used, but I'd be happy to include untested patches to convert them for better adherence to the DMA APIs. (sorry for the double send on this Greg, missed the reply-all) - Chris
On Wed, Jan 31, 2024 at 01:44:50PM -0800, Chris Leech wrote: > On Wed, Jan 31, 2024 at 1:29 PM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Wed, Jan 31, 2024 at 11:17:31AM -0800, Chris Leech wrote: > > > Add a UIO memtype specifically for sharing dma_alloc_coherent > > > memory with userspace, backed by dma_mmap_coherent. > > > > > > This is mainly for the bnx2/bnx2x/bnx2i "cnic" interface, although there > > > are a few other uio drivers which map dma_alloc_coherent memory and > > > could be converted to use dma_mmap_coherent as well. > > > > What other drivers could use this? Patches doing the conversion would > > be welcome, otherwise, again, I am very loath to take this > > one-off-change for just a single driver that shouldn't be doing this in > > the first place :) > > uio_pruss and uio_dmem_genirq both appear to mmap dma_alloc_coherent > memory as UIO_MEM_PHYS. It might not be an issue on that platforms > where those are used, but I'd be happy to include untested patches to > convert them for better adherence to the DMA APIs. Yes, they do need fixing.
As the least horrible way out this looks ok:
Reviewed-by: Christoph Hellwig <hch@lst.de>
Bt maybe you can add some commentary why this mem mode exists and
why no one should be using it in new code?
On Thu, Feb 01, 2024 at 05:46:37AM +0100, Christoph Hellwig wrote: > As the least horrible way out this looks ok: > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > Bt maybe you can add some commentary why this mem mode exists and > why no one should be using it in new code? > Good idea, and perhaps a kernel log warning when this is used as well just to prevent anyone new from ever considering it. thanks, greg k-h
Hi, On Wed, Jan 31, 2024 at 11:17:31AM -0800, Chris Leech wrote: > Add a UIO memtype specifically for sharing dma_alloc_coherent > memory with userspace, backed by dma_mmap_coherent. > > This is mainly for the bnx2/bnx2x/bnx2i "cnic" interface, although there > are a few other uio drivers which map dma_alloc_coherent memory and > could be converted to use dma_mmap_coherent as well. > > Signed-off-by: Nilesh Javali <njavali@marvell.com> > Signed-off-by: Chris Leech <cleech@redhat.com> > --- > drivers/uio/uio.c | 40 ++++++++++++++++++++++++++++++++++++++ > include/linux/uio_driver.h | 3 +++ > 2 files changed, 43 insertions(+) > > diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c > index 2d572f6c8ec83..dde3f49855233 100644 > --- a/drivers/uio/uio.c > +++ b/drivers/uio/uio.c > @@ -24,6 +24,7 @@ [..] > +static int uio_mmap_dma_coherent(struct vm_area_struct *vma) > +{ > + struct uio_device *idev = vma->vm_private_data; > + struct uio_mem *mem; > + void *addr; > + int ret = 0; > + int mi; > + > + mi = uio_find_mem_index(vma); > + 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; > + > + addr = (void *)mem->addr; This cast introduces a build error when building with sizeof(void *) != sizeof(phys_addr_t). For example on i386 with PHYS_ADDR_T_64BIT. (Enabled through allmodconfig) drivers/uio/uio.c: In function 'uio_mmap_dma_coherent': drivers/uio/uio.c:795:16: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast] 795 | addr = (void *)mem->addr; | ^ drivers/uio/uio_pruss.c: In function 'pruss_probe': drivers/uio/uio_pruss.c:194:34: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast] 194 | p->mem[2].addr = (phys_addr_t) gdev->ddr_vaddr; | ^ drivers/uio/uio_dmem_genirq.c: In function 'uio_dmem_genirq_open': drivers/uio/uio_dmem_genirq.c:63:39: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast] 63 | uiomem->addr = addr ? (phys_addr_t) addr : DMEM_MAP_ERROR; | ^ drivers/uio/uio_dmem_genirq.c: In function 'uio_dmem_genirq_release': drivers/uio/uio_dmem_genirq.c:92:43: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast] 92 | (void *) uiomem->addr, As you can see some other files are also affected, which seem to be triggered by other but related patches. This is on next-20240312. > + ret = dma_mmap_coherent(mem->dma_device, > + vma, > + addr, > + mem->dma_addr, > + vma->vm_end - vma->vm_start); > + vma->vm_pgoff = mi; > + > + return ret; > +} > + [..] Thomas
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c index 2d572f6c8ec83..dde3f49855233 100644 --- a/drivers/uio/uio.c +++ b/drivers/uio/uio.c @@ -24,6 +24,7 @@ #include <linux/kobject.h> #include <linux/cdev.h> #include <linux/uio_driver.h> +#include <linux/dma-mapping.h> #define UIO_MAX_DEVICES (1U << MINORBITS) @@ -759,6 +760,42 @@ 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; + struct uio_mem *mem; + void *addr; + int ret = 0; + int mi; + + mi = uio_find_mem_index(vma); + 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; + + addr = (void *)mem->addr; + ret = dma_mmap_coherent(mem->dma_device, + vma, + addr, + mem->dma_addr, + vma->vm_end - vma->vm_start); + vma->vm_pgoff = mi; + + return ret; +} + static int uio_mmap(struct file *filep, struct vm_area_struct *vma) { struct uio_listener *listener = filep->private_data; @@ -806,6 +843,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 47c5962b876b0..14d9dd2a07e85 100644 --- a/include/linux/uio_driver.h +++ b/include/linux/uio_driver.h @@ -37,10 +37,12 @@ struct uio_map; struct uio_mem { const char *name; phys_addr_t addr; + dma_addr_t dma_addr; unsigned long offs; resource_size_t size; int memtype; void __iomem *internal_addr; + struct device *dma_device; struct uio_map *map; }; @@ -158,6 +160,7 @@ extern int __must_check #define UIO_MEM_LOGICAL 2 #define UIO_MEM_VIRTUAL 3 #define UIO_MEM_IOVA 4 +#define UIO_MEM_DMA_COHERENT 5 /* defines for uio_port->porttype */ #define UIO_PORT_NONE 0