diff mbox series

[v6,1/4] uio: introduce UIO_MEM_DMA_COHERENT type

Message ID 20240205200137.138302-1-cleech@redhat.com
State New
Headers show
Series [v6,1/4] uio: introduce UIO_MEM_DMA_COHERENT type | expand

Commit Message

Chris Leech Feb. 5, 2024, 8:01 p.m. UTC 
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 will
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>
---
v6: added kdoc comments

 drivers/uio/uio.c          | 47 ++++++++++++++++++++++++++++++++++++++
 include/linux/uio_driver.h | 13 +++++++++++
 2 files changed, 60 insertions(+)

Comments

Christoph Hellwig Feb. 12, 2024, 6:56 a.m. UTC | #1 
Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Guenter Roeck March 22, 2024, 2:16 p.m. UTC | #2 
On Mon, Feb 05, 2024 at 12:01:37PM -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 will
> 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>
> ---

Building i386:allyesconfig ... failed
--------------
Error log:
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;
      |                ^
cc1: all warnings being treated as errors
make[5]: [scripts/Makefile.build:244: drivers/uio/uio.o] Error 1 (ignored)
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,
      |                                           ^
cc1: all warnings being treated as errors
make[5]: [scripts/Makefile.build:244: drivers/uio/uio_dmem_genirq.o] Error 1 (ignored)
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;
      |                                  ^
cc1: all warnings being treated as errors

Caused by this patch and "uio_dmem_genirq: UIO_MEM_DMA_COHERENT conversion" as well
as "uio_pruss: UIO_MEM_DMA_COHERENT conversion".

I'd suggest to make uio dependent on 64 bit if 32 bit is no longer supported
to prevent waste of test builds resources.

Guenter
Greg KH March 22, 2024, 2:30 p.m. UTC | #3 
On Fri, Mar 22, 2024 at 07:16:19AM -0700, Guenter Roeck wrote:
> On Mon, Feb 05, 2024 at 12:01:37PM -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 will
> > 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>
> > ---
> 
> Building i386:allyesconfig ... failed
> --------------
> Error log:
> 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;
>       |                ^

So on 32bit systems phys_addr_t != the same size as (void *)?  How is
that possible?  We also are doing an explicit cast here, how does this
not work?

Ah, do you have CONFIG_X86_PAE enabled?  That would cause that mess,
ick.


> cc1: all warnings being treated as errors
> make[5]: [scripts/Makefile.build:244: drivers/uio/uio.o] Error 1 (ignored)
> 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,
>       |                                           ^
> cc1: all warnings being treated as errors
> make[5]: [scripts/Makefile.build:244: drivers/uio/uio_dmem_genirq.o] Error 1 (ignored)
> 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;
>       |                                  ^
> cc1: all warnings being treated as errors
> 
> Caused by this patch and "uio_dmem_genirq: UIO_MEM_DMA_COHERENT conversion" as well
> as "uio_pruss: UIO_MEM_DMA_COHERENT conversion".
> 
> I'd suggest to make uio dependent on 64 bit if 32 bit is no longer supported
> to prevent waste of test builds resources.

Perhaps disable it if PHYS_ADDR_T_64BIT is not enabled?

Chris, can you make up a patch?  Odd that this didn't show up in 0-day
before this, does it not test 32bit builds anymore?

thanks,

greg k-h
Guenter Roeck March 22, 2024, 3:23 p.m. UTC | #4 
On 3/22/24 07:30, Greg Kroah-Hartman wrote:
> On Fri, Mar 22, 2024 at 07:16:19AM -0700, Guenter Roeck wrote:
>> On Mon, Feb 05, 2024 at 12:01:37PM -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 will
>>> 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>
>>> ---
>>
>> Building i386:allyesconfig ... failed
>> --------------
>> Error log:
>> 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;
>>        |                ^
> 
> So on 32bit systems phys_addr_t != the same size as (void *)?  How is
> that possible?  We also are doing an explicit cast here, how does this
> not work?

phys_addr_t is not always equivalent to the size of a pointer.
PHYS_ADDR_T_64BIT is a configuration option, after all, and it
is independent of "config 64BIT".

> 
> Ah, do you have CONFIG_X86_PAE enabled?  That would cause that mess,
> ick.
> 
> 
>> cc1: all warnings being treated as errors
>> make[5]: [scripts/Makefile.build:244: drivers/uio/uio.o] Error 1 (ignored)
>> 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,
>>        |                                           ^
>> cc1: all warnings being treated as errorsphys_addr_t
>> make[5]: [scripts/Makefile.build:244: drivers/uio/uio_dmem_genirq.o] Error 1 (ignored)
>> 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;
>>        |                                  ^
>> cc1: all warnings being treated as errors
>>
>> Caused by this patch and "uio_dmem_genirq: UIO_MEM_DMA_COHERENT conversion" as well
>> as "uio_pruss: UIO_MEM_DMA_COHERENT conversion".
>>
>> I'd suggest to make uio dependent on 64 bit if 32 bit is no longer supported
>> to prevent waste of test builds resources.
> 
> Perhaps disable it if PHYS_ADDR_T_64BIT is not enabled?
> 

i386:allyesconfig sets

CONFIG_X86_HAVE_PAE=y
CONFIG_X86_PAE=y
CONFIG_PHYS_ADDR_T_64BIT=y

so that would not really help. The problem here is that this is a 32-bit build,
meaning pointers are 32 bit, while phys_addr_t is 64 bit. I did not check the
code, but it will simply not work if the pointer is supposed to reflect a
physical address. The new dependency would have to check if sizeof(void *) is
larger or equal to sizeof(phys_addr_t). Even then the code would need a double
cast since it is also possible that sizeof(void *) > sizeof(phys_addr_t).

Guenter
diff mbox series

Patch

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 2d572f6c8ec83..bb77de6fa067e 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,49 @@  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->addr & ~PAGE_MASK)
+		return -ENODEV;
+	if (mem->dma_addr & ~PAGE_MASK)
+		return -ENODEV;
+	if (!mem->dma_device)
+		return -ENODEV;
+	if (vma->vm_end - vma->vm_start > mem->size)
+		return -EINVAL;
+
+	dev_warn(mem->dma_device,
+		 "use of UIO_MEM_DMA_COHERENT is highly discouraged");
+
+	/*
+	 * 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 +850,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..18238dc8bfd35 100644
--- a/include/linux/uio_driver.h
+++ b/include/linux/uio_driver.h
@@ -28,19 +28,26 @@  struct uio_map;
  *			logical, virtual, or physical & phys_addr_t
  *			should always be large enough to handle any of
  *			the address types)
+ * @dma_addr:		DMA handle set by dma_alloc_coherent, used with
+ *			UIO_MEM_DMA_COHERENT only (@addr should be the
+ *			void * returned from the same dma_alloc_coherent call)
  * @offs:               offset of device memory within the page
  * @size:		size of IO (multiple of page size)
  * @memtype:		type of memory addr points to
  * @internal_addr:	ioremap-ped version of addr, for driver internal use
+ * @dma_device:		device struct that was passed to dma_alloc_coherent,
+ *			used with UIO_MEM_DMA_COHERENT only
  * @map:		for use by the UIO core only.
  */
 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 +165,12 @@  extern int __must_check
 #define UIO_MEM_LOGICAL	2
 #define UIO_MEM_VIRTUAL 3
 #define UIO_MEM_IOVA	4
+/*
+ * UIO_MEM_DMA_COHERENT exists for legacy drivers that had been getting by with
+ * improperly mapping DMA coherent allocations through the other modes.
+ * Do not use in new drivers.
+ */
+#define UIO_MEM_DMA_COHERENT	5
 
 /* defines for uio_port->porttype */
 #define UIO_PORT_NONE	0